morningman commented on PR #61440:
URL: https://github.com/apache/doris/pull/61440#issuecomment-5295017567
## 1. [Blocking] Remove the accidentally committed empty file `git`
A zero-byte file named `git` (blob `e69de29`, the empty object) was added at
the repository root. It has no ASF license header, and **it is the sole cause
of the failing License Check job**:
```
INFO Totally checked 40193 files, valid: 9, invalid: 1, ignored: 40183,
fixed: 0
WARNING Failed to determine the comment style of file: git
ERROR the following files don't have a valid license header:
git
```
This looks like a mistyped command (something along the lines of `git> git
status`) that later got swept up by `git add .`. `AGENTS.md` also requires that
a commit contain only files related to the change at hand.
```bash
git rm git && git commit --amend --no-edit
```
Every other CI job — CheckStyle, FE UT, P0 Regression, Cloud, Performance —
is green, so this one-line fix turns the whole board green.
## 2. [Blocking] Settle the default value with the maintainers, and add the
warning that was asked for
CalvinKirs asked for the default to be `false`:
> Default should be false. Defaulting to true keeps the vulnerable behavior
— insecure by default for a security fix. Both Spring Security and Doris reject
empty passwords unconditionally. Suggest defaulting false with true as explicit
opt-in; if it stays true, please flag the risk loudly in ldap.conf.
You replied with the backward-compatibility rationale and offered to change
it if that is the call. That thread never converged, and the code still
defaults to `true` — worth resolving before this moves forward.
I lean toward CalvinKirs's position, for reasons that go beyond "secure by
default":
1. **The legacy and plugin paths disagree.** The 4.1+ plugin path rejects
empty passwords unconditionally. Defaulting to `true` means a cluster's
behavior changes abruptly on upgrade to 4.1, with no warning to the operator
while they are still on 4.0. Defaulting to `false` aligns the two paths and
makes the upgrade path smooth.
2. **A missing `conf/ldap.conf` also lands on the default.**
`DorisFE.java:157` is `if (new File(.../ldap.conf).exists())` — if the file is
absent, the whole load is skipped. So deployments with no explicit
configuration get the most permissive value.
3. Your compatibility concern about the backports is real. A middle ground:
**default to `false` on master/4.1, keep `true` on the 3.1.x/4.0.x backports**,
and call it out prominently in the release note. That avoids breaking existing
deployments on a patch release without carrying an insecure default into the
new version.
Whichever value wins, the loud warning in `ldap.conf` that CalvinKirs asked
for is **still missing**. `conf/ldap.conf:53-54` currently reads as a neutral
description, and it also does not mention that the flag is legacy-only — which
is the point that took the most explaining in the PR discussion.
```diff
-## ldap_allow_empty_pass - allow to connect to ldap with empty pass
(enabled by default)
-# ldap_allow_empty_pass = true
+## ldap_allow_empty_pass - allow LDAP users to log in with an empty
password.
+##
+## SECURITY WARNING: LDAP treats a bind with a non-empty DN and an empty
password as an
+## unauthenticated bind and normally reports it as successful. Leaving this
enabled means
+## anyone who knows a valid LDAP user name can log in to Doris without a
password.
+## Set this to false unless you have a specific reason to keep the legacy
behaviour.
+##
+## This setting applies to the legacy LDAP authentication path only. The
plugin-based
+## LDAP authentication introduced in 4.1 always rejects empty passwords.
+# ldap_allow_empty_pass = true
```
## 3. [High] Drop `ERR_EMPTY_PASSWORD`, or actually report it to the client
Right now it falls between two stools: a new `ErrorCode` enum constant was
added, but it is only ever used as a format string inside two `LOG.debug` calls
and is **never reported to any client**. That carries three costs:
1. **It permanently consumes a public error code.** `ErrorCode` is part of
Doris's user-facing error contract; once 6001 is taken it is awkward to
reclaim. Spending an error code on a log message is a poor trade.
2. **The message text teaches users to weaken security.** `"Set
ldap_allow_empty_pass=true to allow."` — if this ever does get reported, it
discloses a server-side configuration key to an unauthenticated client and
tells them how to disable the protection. (The wording came from a bot
suggestion, but that suggestion assumed the message would be shown to users.)
3. **The SQL state does not match the use.** `HY000` is the generic bucket.
If this is meant for clients, an authentication error should use `28000`
(invalid authorization specification), consistent with MySQL's
`ER_ACCESS_DENIED_ERROR`. morrySnow made a related point earlier, pointing at
MySQL's `ER_LDAP_EMPTY_USERDN_PASSWORD` / `ER_NOT_VALID_PASSWORD`.
**Recommended: delete `ERR_EMPTY_PASSWORD` entirely** and express the same
thing through the single log line from suggestion 4.
The reasoning: returning a uniform `ERR_ACCESS_DENIED_ERROR` for every
authentication failure, without distinguishing the cause, is the correct
practice — it denies an attacker a signal to distinguish between failure modes.
The current code already behaves that way. If that behavior is right, the enum
constant is unnecessary.
If the maintainers do want users to see a specific message, then **report it
for real**, and make two changes at the same time: switch the SQL state to
`28000`, and drop `Set ldap_allow_empty_pass=true to allow.` from the text
(something like `Password is required.` instead). Keep the configuration key
name in the server-side log only.
## 4. [High] Collapse three log statements into one, include the client IP,
and use parameterized logging
Once the check moved down into `LdapManager` (per CalvinKirs's suggestion),
the log statements at the two call sites should have moved with it. They did
not, so a single rejected login now produces up to two entries at two different
levels in two different classes.
There is a more practical problem underneath that. For security auditing the
field that matters most is the **source IP**, and it currently ends up in the
wrong place:
- The `LOG.info` in `LdapManager` has **no IP** — the two-argument
`checkUserPasswd(fullName, passwd)` never receives `remoteIp`.
- The call-site logs **do** have the IP, but they are `LOG.debug`, which is
off by default in production.
So under a default configuration, the one line an operator can actually see
is the one missing the useful field.
Suggested consolidation — delete the `LOG.debug` blocks at
`LdapAuthenticator.java:117-121` and `Auth.java:241-244`, and rewrite the one
in `LdapManager`:
```diff
- //not allow to login in case when empty password is specified but such
mode is disabled by configuration
- public boolean checkLoginWithEmptyPasswordForLdapIsAllowed(String
fullName, String passwd) {
- if (Strings.isNullOrEmpty(passwd) &&
!LdapConfig.ldap_allow_empty_pass) {
- LOG.info("User: [" + fullName + "] login rejected: empty LDAP
password is prohibited");
- return false;
- } else {
- return true;
- }
- }
+ /**
+ * LDAP treats a bind with an empty password as an unauthenticated bind
and usually reports
+ * it as successful, so an empty password must be rejected before it
reaches the LDAP server.
+ */
+ @VisibleForTesting
+ boolean isEmptyPasswordLoginAllowed(String fullName, String passwd) {
+ if (!Strings.isNullOrEmpty(passwd) ||
LdapConfig.ldap_allow_empty_pass) {
+ return true;
+ }
+ LOG.warn("Rejected LDAP login with empty password, user={},
ldapAllowEmptyPass=false", fullName);
+ return false;
+ }
```
What changed and why:
- **Parameterized `{}`** instead of string concatenation, matching every
other log statement in `LdapManager` (e.g.
`LOG.debug("LdapManager.checkUserPasswd: user={}, result={}, elapsed={}ms",
...)`).
- **`WARN` instead of `INFO`.** A rejected authentication attempt is a
security event; `WARN` is visible under default settings and is easier for
log-alerting rules to match. If the concern is log amplification from an
unauthenticated, repeatable trigger, then the right move is the opposite — drop
it to `DEBUG` and rely on the existing `ERR_ACCESS_DENIED_ERROR` reporting.
Either is defensible; the current state (INFO, no IP, plus duplicate DEBUG
lines at the call sites) is the one position that gets the worst of both.
- `key=value` with camelCase keys (`ldapAllowEmptyPass=false`), per the FE
logging conventions.
- Early return, dropping the `else`.
- **Narrower visibility.** `LdapManagerTest` lives in the same package as
`LdapManager`, so package-private plus `@VisibleForTesting` is enough. There is
no need to promote an internal authentication predicate to `public` API just so
Mockito can spy on it.
- **Naming.** `checkLoginWithEmptyPasswordForLdapIsAllowed` is 43 characters
and reads awkwardly. `isEmptyPasswordLoginAllowed` follows the usual Java
convention for boolean methods.
If you want the IP in the log, it can be threaded down: `Auth` goes through
the four-argument overload which already has `remoteHost`, and
`LdapAuthenticator` has `remoteIp` in scope, so the plumbing is cheap.
## 5. [High] Revert the `fe-common` test dependency in the plugin module,
and trim those three tests
```xml
<!-- reference for fe-common for compatibility integration tests for PR
#61440 -->
<dependency>
<groupId>org.apache.doris</groupId>
<artifactId>fe-common</artifactId>
<version>${project.version}</version>
<scope>test</scope>
</dependency>
```
This is the **only** dependency from anywhere under `fe-authentication` to
`fe-common`. That module is deliberately decoupled from FE internals — the
javadoc at `LdapClient.java:41` says so explicitly: *"Configuration from Map
instead of global LdapConfig"*. This PR punches through that isolation for the
sake of three test methods.
To be fair about the risk: it is `test` scope, it does not reach the shaded
artifact, there is no dependency cycle (`fe-common` comes before
`fe-authentication` in the reactor and does not depend on it), and CI does
compile. But the cost is not trivial either — `fe-common` drags in
hadoop-common, hadoop-aws, netty, protobuf, antlr and their transitive closure,
so the plugin module's test classpath now has to coexist with Spring LDAP. The
next guava or jackson conflict there will be expensive to diagnose.
And the benefit is zero. The plugin's production code cannot read
`LdapConfig` at all; empty credentials are rejected at
`LdapAuthenticationPlugin.java:116` before anything else happens:
```java
byte[] credentialBytes = request.getCredential();
if (credentialBytes == null || credentialBytes.length == 0) {
return AuthenticationResult.failure("Password is required for LDAP
authentication");
}
```
So setting a static field the code under test structurally cannot observe,
then asserting the result is unchanged, is a vacuous assertion. It would not
reliably catch a future regression where the plugin *did* start reading that
field.
Worth noting separately:
`testAuthenticateEmptyPasswordAndAllowEmptyPassDefault` and
`testAuthenticateEmptyPasswordAndAllowEmptyPassTrue` are **byte-for-byte
identical** apart from the method name and comment — "default" and "true"
cannot differ here, because both explicitly assign `true`.
**Suggested: revert the pom change and restore the original single
`testAuthenticateEmptyPassword`.** It already covers the behavior that matters.
If you want to leave a record of the intent — "the plugin is unaffected by
the legacy flag" — the cheapest way is a comment on the existing test, or the
note in `ldap.conf` from suggestion 2. It does not justify a module dependency.
One more thing: drop `for PR #61440` from the pom comment. The PR number
tells a future reader nothing; a comment there should explain *why* the
dependency is needed.
## 6. [Medium] The assertion in `testCheckUserNullPasswdMocked` is a no-op
```java
LdapManager spyManager = Mockito.spy(ldapManager);
Assert.assertFalse(spyManager.checkUserPasswd(USER1, null));
Mockito.verify(spyManager,
Mockito.times(0)).checkLoginWithEmptyPasswordForLdapIsAllowed(USER1, "");
```
The comment says the test verifies that the method is not invoked in the
null-password case, but the `verify` uses `USER1, ""` as the expected
arguments. If the production code did call
`checkLoginWithEmptyPasswordForLdapIsAllowed(USER1, null)`, this assertion
would **still pass** — it only checks that no invocation happened with the
exact argument pair `(USER1, "")`, not that no invocation happened at all.
```diff
- Mockito.verify(spyManager,
Mockito.times(0)).checkLoginWithEmptyPasswordForLdapIsAllowed(USER1, "");
+ Mockito.verify(spyManager, Mockito.never())
+
.checkLoginWithEmptyPasswordForLdapIsAllowed(Mockito.anyString(),
Mockito.any());
```
## 7. [Medium] Remove the four spy-based call-sequence tests
`testCheckUserEmptyPasswdAllowedMocked`,
`testCheckUserEmptyPasswdDisabledMockedDenied`,
`testCheckUserEmptyPasswdDisabledMockedAllowed` and
`testCheckUserNullPasswdMocked` assert **implementation details** — that
`checkLoginWithEmptyPasswordForLdapIsAllowed` was called once and `getUserInfo`
zero times. The behavior they describe is already covered by their non-mocked
counterparts: `testCheckUserEmptyPasswdAllowed`,
`testCheckUserEmptyPasswdDisabled` and `testCheckUserNullPasswd`.
The problem with tests shaped this way is that any behavior-preserving
refactor — inlining the check into `checkUserPasswd`, restructuring the
short-circuit — turns them red even though nothing actually broke. They are
also the only reason `checkLoginWithEmptyPasswordForLdapIsAllowed` had to
become `public` (see suggestion 4).
The one assertion with independent value is
`Mockito.times(0)).getUserInfo(USER1)`: it guarantees that no query is sent to
the LDAP server when the flag is off. If you want to keep that property, keep a
single test and name it for the intent, e.g.
`testEmptyPasswordRejectedWithoutQueryingLdapServer`.
## 8. [Medium] Fix the inverted comment in `LdapManager`
```java
// extra check for PR 61440 to disable login with empty LDAP password in
case when specific property is true
```
**The logic is stated backwards**: empty-password login is disabled when the
property is `false`, and allowed when it is `true`. The comment also just
restates the line below it. Better to delete it and document the non-obvious
constraint instead — why the check has to run before `getUserInfo()`:
```diff
- // extra check for PR 61440 to disable login with empty LDAP
password in case when specific property is true
- if (!checkLoginWithEmptyPasswordForLdapIsAllowed(fullName, passwd))
{
+ // Must run before getUserInfo(): the cache may hold an empty
password from a login
+ // that happened while ldap_allow_empty_pass was still enabled.
+ if (!isEmptyPasswordLoginAllowed(fullName, passwd)) {
return false;
}
```
Along the same lines, `LdapAuthenticator.java:111` (`// extra check for
login with empty LDAP password was added in checkUserPasswd.`) and
`Auth.java:241` (`// extra log to identify case covered by PR 61440 ...`) can
be cleaned up together with suggestion 4.
## 9. [Low] Move `ldap_allow_empty_pass` up with the other fields
The new field sits **after** the static `getConnectionURL()` method at
`LdapConfig.java:214-218`, while the rest of the file follows a
fields-then-methods layout. Moving it below `ldap_use_ssl` and above
`getConnectionURL()` keeps the file consistent.
## 10. [Low] Test comment wording and redundant hooks
A few comments have grammar issues worth cleaning up in passing:
- `//running test with specified value - ldap_allow_empty_pass is be true` —
`is be`
- `//tests checks existing default flow method by method - what user with
empty ldap password can login` — `tests checks` → `test checks`; `what user ...
can login` → `that a user ... can log in` (the `what`/`that` mix-up recurs
across several comments)
- `//test check existing feature that user with null ldap password can't
login in any case` — `test check` → `test checks`
Also, `LdapAuthenticationPluginIntegrationTest` sets `ldap_allow_empty_pass
= true` in both `@BeforeEach` and `@AfterEach`, carrying the same typo'd
comment twice. If suggestion 5 is taken and that group is reverted, both hooks
disappear; if it is kept, only the `@AfterEach` reset is needed.
## 11. [Low] Add a test that pins "the cache must not bypass the check"
Nothing currently locks in the constraint that the check runs ahead of the
cache fast path. The scenario needs a restart to construct in practice, since
the config is not runtime-mutable — but it is precisely the property most
likely to be broken by a future refactor:
```java
@Test
public void testCachedEmptyPasswordIsRejectedAfterFlagDisabled() {
LdapManager ldapManager = new LdapManager();
Deencapsulation.setField(ldapManager, "ldapClient", ldapClient);
mockClient(true, true);
// Empty password succeeds and gets cached while the flag is still
enabled.
Assert.assertTrue(ldapManager.checkUserPasswd(USER1, ""));
Assert.assertEquals("", ldapManager.getUserInfo(USER1).getPasswd());
// Once disabled, the cached entry must not short-circuit the new check.
LdapConfig.ldap_allow_empty_pass = false;
Assert.assertFalse(ldapManager.checkUserPasswd(USER1, ""));
}
```
--
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]
