CalvinKirs commented on code in PR #66115:
URL: https://github.com/apache/doris/pull/66115#discussion_r4034270349
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/Password.java:
##########
@@ -37,4 +45,16 @@ public byte[] getPassword() {
public void setPassword(byte[] password) {
this.password = password;
}
+
+ public byte[] getSecondaryPassword() {
+ return secondaryPassword;
+ }
+
+ public void setSecondaryPassword(byte[] secondaryPassword) {
+ this.secondaryPassword = secondaryPassword;
+ }
+
+ public boolean hasSecondaryPassword() {
Review Comment:
The concern behind the GRANT gate was a credential parked in this slot that
nothing surfaces. After this PR an admin still has no way to see which accounts
hold a secondary. `SHOW ALL GRANTS` / `SHOW PROC '/auth'` already have a
`Password` column driven by `User.hasPassword()` (`Auth.getAuthInfo`), so
exposing it is a one-liner there, e.g. `Yes (dual)`. I'd rather have that in
this PR than wait for #66198, since it is the auditing half of the same concern.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/UserManager.java:
##########
@@ -351,14 +415,65 @@ public Map<String, List<User>> getNameToUsers() {
}
public void setPassword(UserIdentity userIdentity, byte[] password,
boolean errOnNonExist) throws DdlException {
+ setPassword(userIdentity, password, errOnNonExist, false);
+ }
+
+ /**
+ * Set the user's password, with MySQL-compatible dual password semantics:
+ * with {@code retainCurrent} ("RETAIN CURRENT PASSWORD") the previous
+ * primary password becomes the secondary password and remains valid for
+ * authentication; without it an existing secondary password remains
+ * UNCHANGED (MySQL: "If an account has a secondary password and you
+ * change its primary password without specifying RETAIN CURRENT PASSWORD,
+ * the secondary password remains unchanged."). Setting an EMPTY password
+ * empties the secondary password as well, even with retain (also MySQL).
+ */
+ public void setPassword(UserIdentity userIdentity, byte[] password,
boolean errOnNonExist,
+ boolean retainCurrent) throws DdlException {
+ User user = getUserByUserIdentity(userIdentity);
+ if (user == null) {
+ if (errOnNonExist) {
+ throw new DdlException("user " + userIdentity + " does not
exist");
+ }
+ return;
+ }
+ Password oldPassword = user.getPassword();
+ byte[] carried;
+ if (password == null || password.length == 0) {
+ // an empty new password empties the secondary as well, even with
+ // RETAIN CURRENT PASSWORD (MySQL semantics)
+ carried = null;
+ } else if (retainCurrent) {
+ carried = oldPassword == null ? null : oldPassword.getPassword();
+ } else {
+ carried = oldPassword == null ? null :
oldPassword.getSecondaryPassword();
+ }
+ // Build the full Password first and swap it in as ONE reference
+ // assignment: authentication reads a single Password snapshot, so
+ // there must never be a window where the new primary is visible
+ // without the carried secondary — that window would reject exactly
+ // the old-password consumers this feature keeps alive.
+ Password newPassword = new Password(password);
+ newPassword.setSecondaryPassword(carried);
+ user.setPassword(newPassword);
+ }
+
+ /**
+ * MySQL-compatible "ALTER USER ... DISCARD OLD PASSWORD": drop the
+ * retained secondary password. A user without one is a silent no-op
+ * (MySQL behavior).
+ */
+ public void discardOldPassword(UserIdentity userIdentity, boolean
errOnNonExist) throws DdlException {
Review Comment:
Two small things here.
For a domain account (`u@['example.com']`) this only clears the domain
entry; the resolved-IP entries keep their copy until the next `DomainResolver`
tick (10s) rebuilds them, so the old password stays valid for up to that long
after DISCARD. Same lag a plain password change has today, so fine, but the
"evict early" wording in the comments does not hold for domain accounts. A note
here would save the next reader the trace.
Also this mutates the `Password` in place while `setPassword` swaps the
whole object. It is safe because both run under the Auth write lock and every
auth path holds the read lock, but it contradicts the single-snapshot comment
on `matchUserPassword`. Swapping in a new `Password(primary)` would keep one
rule.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/UserManager.java:
##########
@@ -211,13 +216,58 @@ private String hasRemotePasswd(boolean plain, byte[]
remotePasswd) {
return remotePasswd.length == 0 ? "NO" : "YES";
}
- private boolean comparePassword(Password curUserPassword, byte[]
remotePasswd,
+ // matchUserPassword results: which stored password slot matched.
+ private static final int MATCH_NONE = 0;
+ private static final int MATCH_PRIMARY = 1;
+ private static final int MATCH_SECONDARY = 2;
+
+ /**
+ * Try the primary password, then the retained secondary one
+ * (MySQL-compatible dual password: "RETAIN CURRENT PASSWORD"), against a
+ * SINGLE snapshot of the user's Password object (a concurrent password
+ * change swaps the whole object, so re-reading it could compare the two
+ * slots of two different generations). Returns which slot matched; the
+ * caller reports a secondary-slot match (log + metric) only AFTER account
+ * lock/expiration policy passes, so a rejected login never counts as a
+ * successful secondary authentication.
+ */
+ private int matchUserPassword(User user, byte[] remotePasswd,
+ byte[] randomString, String remotePasswdStr, boolean plain) {
+ Password pwd = user.getPassword();
+ if (comparePassword(pwd.getPassword(), remotePasswd, randomString,
remotePasswdStr, plain)) {
+ return MATCH_PRIMARY;
+ }
+ if (pwd.hasSecondaryPassword()
+ && comparePassword(pwd.getSecondaryPassword(),
+ remotePasswd, randomString, remotePasswdStr, plain)) {
+ return MATCH_SECONDARY;
+ }
+ return MATCH_NONE;
+ }
+
+ /**
+ * Report an authentication that succeeded via the retained secondary
+ * password (log + metric), so operators can tell when all consumers have
+ * converged on the new password. Call only after the account passed
+ * lock/expiration policy.
+ */
+ private void reportSecondaryPasswordAuth(int matchedSlot, String
userDescription) {
+ if (matchedSlot != MATCH_SECONDARY) {
+ return;
+ }
+ LOG.info("user {} authenticated with retained secondary password",
userDescription);
Review Comment:
One INFO line per connection that authenticates with the secondary. For the
fleet-of-agents case in the description this fires on every reconnect until
they converge, which can be a lot of log for information the counter already
carries. DEBUG here, or keep INFO and accept the volume; just flagging it.
##########
fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/DualPasswordTest.java:
##########
@@ -0,0 +1,577 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.mysql.privilege;
+
+import org.apache.doris.alter.AlterUserOpType;
+import org.apache.doris.analysis.PasswordOptions;
+import org.apache.doris.analysis.RedirectStatus;
+import org.apache.doris.analysis.UserDesc;
+import org.apache.doris.analysis.UserIdentity;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.common.AnalysisException;
+import org.apache.doris.common.AuthenticationException;
+import org.apache.doris.common.DdlException;
+import org.apache.doris.datasource.InternalCatalog;
+import org.apache.doris.metric.LongCounterMetric;
+import org.apache.doris.metric.Metric.MetricUnit;
+import org.apache.doris.metric.MetricRepo;
+import org.apache.doris.mysql.MysqlPassword;
+import org.apache.doris.nereids.parser.NereidsParser;
+import org.apache.doris.nereids.trees.plans.commands.AlterUserCommand;
+import org.apache.doris.nereids.trees.plans.commands.CreateUserCommand;
+import org.apache.doris.nereids.trees.plans.commands.SetOptionsCommand;
+import org.apache.doris.nereids.trees.plans.commands.info.CreateUserInfo;
+import org.apache.doris.persist.AlterUserOperationLog;
+import org.apache.doris.persist.EditLog;
+import org.apache.doris.persist.PrivInfo;
+import org.apache.doris.persist.gson.GsonUtils;
+import org.apache.doris.qe.ConnectContext;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Sets;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+import java.lang.reflect.Field;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * MySQL-compatible dual password:
+ * ALTER USER ... IDENTIFIED BY ... RETAIN CURRENT PASSWORD keeps the previous
+ * password valid (secondary slot) until the next password change without
+ * RETAIN, or an explicit ALTER USER ... DISCARD OLD PASSWORD.
+ */
+public class DualPasswordTest {
+
+ private Auth auth;
+ private Env env = Mockito.mock(Env.class);
+ private EditLog editLog = Mockito.mock(EditLog.class);
+ private AccessControllerManager accessManager =
Mockito.mock(AccessControllerManager.class);
+ private InternalCatalog internalCatalog =
Mockito.mock(InternalCatalog.class);
+ private MockedStatic<Env> mockedEnvStatic;
+
+ @BeforeEach
+ public void setUp() throws Exception {
+ auth = new Auth();
+ mockedEnvStatic = Mockito.mockStatic(Env.class);
+ mockedEnvStatic.when(Env::getCurrentEnv).thenReturn(env);
+ Mockito.when(env.getAuth()).thenReturn(auth);
+ Mockito.when(env.getEditLog()).thenReturn(editLog);
+ Mockito.when(env.getAccessManager()).thenReturn(accessManager);
+ // ConnectContext.setEnv reads the internal catalog name
+ Mockito.when(internalCatalog.getName()).thenReturn("internal");
+ Mockito.when(env.getInternalCatalog()).thenReturn(internalCatalog);
+ }
+
+ @AfterEach
+ public void tearDown() {
+ mockedEnvStatic.close();
+ ConnectContext.remove();
+ }
+
+ /** A connected session for executing parsed commands. */
+ private ConnectContext ctxFor(UserIdentity currentUser) {
+ ConnectContext ctx = new ConnectContext();
+ ctx.setEnv(env);
+ ctx.setCurrentUserIdentity(currentUser);
+ ctx.setThreadLocalInfo();
+ return ctx;
+ }
+
+ private void grantPriv(boolean hasGrantPriv) {
+
Mockito.when(accessManager.checkGlobalPriv(Mockito.any(ConnectContext.class),
+ Mockito.eq(PrivPredicate.GRANT))).thenReturn(hasGrantPriv);
+ }
+
+ private UserIdentity createUser(String name) throws DdlException {
+ UserIdentity userIdentity = new UserIdentity(name, "%");
+ userIdentity.setIsAnalyzed();
+ CreateUserCommand createUserCommand = new CreateUserCommand(new
CreateUserInfo(new UserDesc(userIdentity)));
+ auth.createUser(createUserCommand.getInfo());
+ return userIdentity;
+ }
+
+ private boolean canLogin(String user, String plainPassword) {
+ try {
+ auth.checkPlainPassword(user, "192.168.1.1", plainPassword, null);
+ return true;
+ } catch (AuthenticationException e) {
+ return false;
+ }
+ }
+
+ @Test
+ public void testRetainEvictAndDiscard() throws DdlException {
+ UserIdentity user = createUser("rot");
+
+ // initial password p1
+ auth.setPassword(user, MysqlPassword.makeScrambledPassword("p1"));
+ Assertions.assertTrue(canLogin("rot", "p1"));
+ Assertions.assertFalse(canLogin("rot", "p2"));
+
+ // p2 RETAIN CURRENT PASSWORD -> p1 and p2 both authenticate
+ auth.setPasswordInternal(user,
MysqlPassword.makeScrambledPassword("p2"), null,
+ true, false, true /* retain */, false);
+ Assertions.assertTrue(canLogin("rot", "p2"));
+ Assertions.assertTrue(canLogin("rot", "p1"));
+ Assertions.assertFalse(canLogin("rot", "p0"));
+
+ // p3 RETAIN -> the one-secondary rule evicts p1; p2 + p3 authenticate
+ auth.setPasswordInternal(user,
MysqlPassword.makeScrambledPassword("p3"), null,
+ true, false, true /* retain */, false);
+ Assertions.assertTrue(canLogin("rot", "p3"));
+ Assertions.assertTrue(canLogin("rot", "p2"));
+ Assertions.assertFalse(canLogin("rot", "p1"));
+
+ // p4 WITHOUT retain -> the secondary REMAINS UNCHANGED (MySQL: "the
+ // secondary password remains unchanged"); the replaced primary p3 is
+ // simply gone -> p4 + p2 authenticate, p3 does not
+ auth.setPasswordInternal(user,
MysqlPassword.makeScrambledPassword("p4"), null,
+ true, false, false /* no retain */, false);
+ Assertions.assertTrue(canLogin("rot", "p4"));
+ Assertions.assertFalse(canLogin("rot", "p3"));
+ Assertions.assertTrue(canLogin("rot", "p2"));
+
+ // p5 RETAIN, then DISCARD OLD PASSWORD (via the replay path, which is
+ // also what a follower executes) -> only p5 remains
+ auth.setPasswordInternal(user,
MysqlPassword.makeScrambledPassword("p5"), null,
+ true, false, true /* retain */, false);
+ Assertions.assertTrue(canLogin("rot", "p4"));
+ auth.replayAlterUser(new
AlterUserOperationLog(AlterUserOpType.DISCARD_OLD_PASSWORD,
Review Comment:
This constructs an `AlterUserOperationLog` with `op = DISCARD_OLD_PASSWORD`,
which is the one shape the PR promises never to journal; it only works because
`replayAlterUser` falls through to `log.getOp()`. Use
`AlterUserOperationLog.discardOldPassword(user)` so the test exercises the
entry a follower actually sees (same at the second call below).
##########
regression-test/suites/account_p0/test_dual_password.groovy:
##########
@@ -0,0 +1,181 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// MySQL-compatible dual password: RETAIN CURRENT PASSWORD keeps the previous
+// password valid as a secondary password until the next retaining change or
+// an explicit DISCARD OLD PASSWORD. Covers rotation, eviction, discard, the
+// empty-password rules, the privilege gate on the clause, and the
+// interaction with the password history / expiration policies.
+suite("test_dual_password", "account,nonConcurrent") {
+ def user = "test_dual_password_user"
+ def tokens = context.config.jdbcUrl.split('/')
+ def url = tokens[0] + "//" + tokens[2] + "/" + "information_schema" + "?"
+
+ def canLogin = { String password ->
+ try {
+ connect(user, password, url) {
+ sql "SELECT 1"
+ }
+ return true
+ } catch (Exception e) {
+ logger.info("login of ${user} with '${password}' refused: " +
e.getMessage())
+ assertTrue(e.getMessage().contains("Access denied") ||
e.getMessage().contains("authentication failed")
+ || e.getMessage().contains("password has expired"),
e.getMessage())
+ return false
+ }
+ }
+
+ def grantClusterUsage = {
+ if (isCloudMode()) {
+ def clusters = sql "SHOW CLUSTERS"
+ assertTrue(!clusters.isEmpty())
+ sql """GRANT USAGE_PRIV ON CLUSTER `${clusters[0][0]}` TO
'${user}'@'%'"""
+ }
+ }
+
+ try_sql "DROP USER IF EXISTS '${user}'@'%'"
+ sql "CREATE USER '${user}'@'%' IDENTIFIED BY 'p1'"
+ grantClusterUsage()
+ assertTrue(canLogin("p1"))
+ assertFalse(canLogin("p2"))
+
+ // 1. rotation with RETAIN: both the new primary and the retained
+ // secondary authenticate, anything else does not
+ sql "SET PASSWORD FOR '${user}'@'%' = PASSWORD('p2') RETAIN CURRENT
PASSWORD"
+ assertTrue(canLogin("p2"))
+ assertTrue(canLogin("p1"))
+ assertFalse(canLogin("p3"))
+
+ // 2. a second RETAIN evicts the older secondary (one secondary slot)
+ sql "ALTER USER '${user}'@'%' IDENTIFIED BY 'p3' RETAIN CURRENT PASSWORD"
+ assertTrue(canLogin("p3"))
+ assertTrue(canLogin("p2"))
+ assertFalse(canLogin("p1"))
+
+ // 3. a change WITHOUT retain replaces the primary and leaves the
+ // secondary unchanged (MySQL semantics)
+ sql "ALTER USER '${user}'@'%' IDENTIFIED BY 'p4'"
+ assertTrue(canLogin("p4"))
+ assertFalse(canLogin("p3"))
+ assertTrue(canLogin("p2"))
+
+ // 4. DISCARD OLD PASSWORD drops the secondary; repeating it with no
+ // secondary present is a silent no-op
+ sql "ALTER USER '${user}'@'%' DISCARD OLD PASSWORD"
+ assertTrue(canLogin("p4"))
+ assertFalse(canLogin("p2"))
+ sql "ALTER USER '${user}'@'%' DISCARD OLD PASSWORD"
+ assertTrue(canLogin("p4"))
+
+ // 5. the clause is privileged even on one's own account: a plain
+ // self-service SET PASSWORD works, RETAIN CURRENT PASSWORD does not
+ connect(user, "p4", url) {
+ sql "SET PASSWORD = PASSWORD('p5')"
+ test {
+ sql "SET PASSWORD = PASSWORD('p6') RETAIN CURRENT PASSWORD"
+ exception "Access denied"
+ }
+ }
+ assertTrue(canLogin("p5"))
+ assertFalse(canLogin("p4"))
+ assertFalse(canLogin("p6"))
+
+ // 6. RETAIN cannot be combined with an empty new password in a way that
+ // keeps a secondary: an empty new password empties the secondary too
+ sql "SET PASSWORD FOR '${user}'@'%' = PASSWORD('p7') RETAIN CURRENT
PASSWORD"
+ assertTrue(canLogin("p5"))
+ sql "ALTER USER '${user}'@'%' IDENTIFIED BY '' RETAIN CURRENT PASSWORD"
+ assertTrue(canLogin(""))
+ assertFalse(canLogin("p7"))
+ assertFalse(canLogin("p5"))
+
+ // 7. RETAIN on an account whose primary password is empty fails
+ test {
+ sql "ALTER USER '${user}'@'%' IDENTIFIED BY 'p8' RETAIN CURRENT
PASSWORD"
+ exception "cannot be retained"
+ }
+ assertTrue(canLogin(""))
+ assertFalse(canLogin("p8"))
+
+ // 8. RETAIN without a password change is rejected
+ test {
+ sql "ALTER USER '${user}'@'%' RETAIN CURRENT PASSWORD"
+ exception "RETAIN CURRENT PASSWORD requires a password change"
+ }
+
+ // 9. password history: a retaining change is still a password change,
+ // so the new password must not contradict the history
+ sql "ALTER USER '${user}'@'%' PASSWORD_HISTORY 2"
+ sql "ALTER USER '${user}'@'%' IDENTIFIED BY 'h1'"
+ sql "ALTER USER '${user}'@'%' IDENTIFIED BY 'h2' RETAIN CURRENT PASSWORD"
+ assertTrue(canLogin("h2"))
+ assertTrue(canLogin("h1"))
+ test {
+ sql "ALTER USER '${user}'@'%' IDENTIFIED BY 'h1' RETAIN CURRENT
PASSWORD"
+ exception "contradict the password history policy"
+ }
+ // DISCARD is not a password change: the history is left as it was, so
+ // the discarded secondary still contradicts it and a fresh value passes
+ sql "ALTER USER '${user}'@'%' DISCARD OLD PASSWORD"
+ assertFalse(canLogin("h1"))
+ test {
+ sql "ALTER USER '${user}'@'%' IDENTIFIED BY 'h1'"
+ exception "contradict the password history policy"
+ }
+ sql "ALTER USER '${user}'@'%' IDENTIFIED BY 'h3' RETAIN CURRENT PASSWORD"
+ assertTrue(canLogin("h3"))
+ assertTrue(canLogin("h2"))
+ sql "ALTER USER '${user}'@'%' PASSWORD_HISTORY 0"
+
+ // 10. password expiration: a retaining change restarts the expiry
+ // clock (it is a password change); DISCARD does not touch it
+ sql "ALTER USER '${user}'@'%' PASSWORD_EXPIRE INTERVAL 8 SECOND"
Review Comment:
Timing is tight here: an 8-second expiry with 5s + 4s sleeps and about six
JDBC connects in between leaves roughly 2s for the `e1` check after DISCARD to
land under 8s. `test_alter_user` runs with similar margins so not blocking on
it, but doubling both the interval and the sleeps would make this less likely
to flake on a loaded runner.
##########
fe/fe-core/src/main/java/org/apache/doris/persist/AlterUserOperationLog.java:
##########
@@ -55,10 +65,30 @@ public AlterUserOperationLog(AlterUserOpType opType,
UserIdentity userIdent, byt
this.comment = comment;
}
+ /**
+ * The journal entry for "ALTER USER ... DISCARD OLD PASSWORD". Its carrier
+ * op is SET_PASSWORD_POLICY with {@link PasswordOptions#UNSET_OPTION}: on
+ * a pre-feature binary that replays as a no-op (every UNSET branch of the
+ * policy update returns early and no password is journaled), whereas an
+ * unknown AlterUserOpType name would deserialize as null and fail replay,
+ * and an OP_SET_PASSWORD carrier would append the primary to the password
+ * history and refresh the password creation time.
+ */
+ public static AlterUserOperationLog discardOldPassword(UserIdentity
userIdent) {
Review Comment:
Confirmed the pre-feature replay is a no-op for the policy values (every
UNSET branch returns early, `HistoryPolicy.update` skips a null password). The
one side effect left is `getOrCreatePolicy` inserting a default
`PasswordPolicy` for a user that had none. Harmless, but it means the carrier
is not strictly a no-op, so worth a word in the Javadoc for whoever next
touches `SET_PASSWORD_POLICY` replay.
--
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]
