morningman commented on PR #61440:
URL: https://github.com/apache/doris/pull/61440#issuecomment-5317293608
<!-- doris-repo-review:v1:begin -->
### Local pipeline review — ✅ PASS
```yaml
schema: doris-repo-review/v1
status: PASS
pr: apache/doris#61440
commit: 3ee24dfcc664baf0ff8ee470ad2e2d93c8098241
base: 168d07778338761de5da5f0e97c36eee1e265b4b
reviewed_at: 2026-08-17T23:19+08:00
reviewer: morningman
model: claude-opus-5[1m]
effort: max
findings: {blocker: 0, major: 0, minor: 2, nit: 4}
rounds: 2
converged: false
```
**Notes for maintainers**
-
`fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/ldap/LdapManager.java:137`
— this WARN is the only server-side record of the rejection:
`ErrorReport.report` in `LdapAuthenticator` writes no log,
`AuthenticatorManager`'s IP-carrying WARN is suppressed because
`LdapAuthenticator` returns the null-`failureSummary` `failedResponse`
singleton, and `AuditEvent.EventType.CONNECTION` is never constructed. Worth
adding the client IP; also the message spells the key `ldapAllowEmptyPass`,
which occurs nowhere else in the tree (the snake_case `ldap_allow_empty_pass`
occurs ten times), so it is not greppable from the name in `ldap.conf`.
-
`fe/fe-common/src/main/java/org/apache/doris/common/LdapConfig.java:209-210` —
the absence of `mutable = true` is now a documented security contract, but no
test fails if it is added back, and an earlier automated review on this PR did
argue for adding it. `ConfigTest.testSecurityPathConfigsAreNotRuntimeMutable`
is the precedent; note that appending the key to that array would fail with
"does not exist" rather than "is not mutable", because `ldapConfFields` is
unpopulated in a unit-test JVM — assert on the annotation instead.
- `conf/ldap.conf:55-56` — "normally reports it as successful" is
server-dependent rather than universal (RFC 4513 §5.1.2 says servers SHOULD
reject unauthenticated binds; AD accepts, default OpenLDAP does not). Nothing
in the tree establishes either outcome. The case for the `false` default is
stronger when stated accurately.
- Chokepoint completeness was verified rather than assumed:
`fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/ldap/LdapClient.java:175`
is the only end-user LDAP bind in fe-core and its only caller sits below the
gate; MySQL, HTTP, Arrow Flight, stream load and the `FrontendServiceImpl`
Thrift RPCs all funnel through `LdapAuthenticator` or
`Auth.checkPlainPassword`. `root`/`admin` are excluded twice and cannot be
locked out. The `conf/ldap.conf` claim that the fe-authentication plugin always
rejects empty passwords is true.
- `converged: false` is reported honestly rather than as a problem: round 1
closed the correctness, configuration, compatibility and coverage ground; round
2 re-attacked it without result but did add three new `Nit` candidates (naming
and comment wording), so the loop was stopped at 2 of a maximum 3 rounds
without reaching a clean zero-new-candidate state. No Blocker or Major was
found in either round.
<sub>Reviewed locally with the `doris-repo-review` pipeline (a local port of
`.github/workflows/code-review-runner.yml`). This is not a CI status
check.</sub>
<!-- doris-repo-review:v1:end -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
