raghav-reglobe commented on code in PR #66115:
URL: https://github.com/apache/doris/pull/66115#discussion_r4045317943
##########
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:
Added here: `Auth.getAuthInfo` now prints `Yes (dual)` while the account
holds a retained secondary (via a new `User.hasSecondaryPassword()`), so `SHOW
ALL GRANTS`, `SHOW GRANTS FOR` and `SHOW PROC '/auth'` all show it. Covered in
`DualPasswordTest` and the regression suite across retain, a plain change and
discard. The follow-up in #66198 is now only the delegated privilege.
##########
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:
Both taken. `discardOldPassword` swaps in `new Password(primary)` instead of
clearing the slot, so a password change and a discard follow the same
one-snapshot rule, and the javadoc says that a domain account is cleared in its
domain entry only, with the resolved-IP entries following at the next resolver
refresh (10 s), the same lag a plain password change has. The "evict early"
comment in `Auth` is reworded to match.
##########
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:
Moved to DEBUG (guarded). The metric carries the count; the line was only
ever useful to name the account, and at INFO it would indeed be one line per
reconnect for every consumer still on the old password.
##########
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:
Added to the javadoc: the one side effect a pre-feature replay keeps is
`getOrCreatePolicy` inserting a default `PasswordPolicy` for a user that had
none, the same row its next login check would create, so the carrier is a no-op
for the policy values rather than for the policy table.
##########
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:
Right, that op is the one shape the PR never journals. Both call sites now
replay `AlterUserOperationLog.discardOldPassword(user)`, so the test sees the
same entry a follower does.
##########
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:
Doubled: `PASSWORD_EXPIRE INTERVAL 16 SECOND` with 10 s + 8 s sleeps, so the
check after `DISCARD` has about 6 s of margin instead of 2.
--
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]
