raghav-reglobe commented on code in PR #66115:
URL: https://github.com/apache/doris/pull/66115#discussion_r3668506840
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/UserManager.java:
##########
@@ -351,14 +376,59 @@ 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();
+ }
user.setPassword(password);
+ user.getPassword().setSecondaryPassword(carried);
Review Comment:
Done as suggested — the change now builds the full `Password` (primary +
carried secondary) and swaps it via the `setPassword(Password)` overload, and
`matchUserPassword` reads `user.getPassword()` once into a local so both slots
come from the same generation. Same treatment for the resolver path:
`createUserWithoutLock` gained a `Password`-taking variant so resolved-IP users
are built with both slots before being published.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/AlterUserOpType.java:
##########
@@ -25,5 +25,10 @@ public enum AlterUserOpType {
LOCK_ACCOUNT,
UNLOCK_ACCOUNT,
MODIFY_COMMENT,
- SET_TLS_REQUIRE
+ SET_TLS_REQUIRE,
+ // MySQL-compatible "DISCARD OLD PASSWORD": evict the retained secondary
+ // password. NB: journaled via OP_ALTER_USER — all FEs must run a version
+ // that knows this value before it is first executed (standard rolling
+ // upgrade order: masters last).
+ DISCARD_OLD_PASSWORD
Review Comment:
Fixed — `DISCARD OLD PASSWORD` now journals via `OP_SET_PASSWORD`/`PrivInfo`
(discard flag, `passwd` = the unchanged primary), never `OP_ALTER_USER`. An
older FE replays it as a harmless same-password set; the enum value remains for
in-memory dispatch only. Test pins that `logAlterUser` is never called for
DISCARD and that a legacy binary's interpretation of the entry is a no-op.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/UserManager.java:
##########
@@ -211,13 +213,36 @@ private String hasRemotePasswd(boolean plain, byte[]
remotePasswd) {
return remotePasswd.length == 0 ? "NO" : "YES";
}
- private boolean comparePassword(Password curUserPassword, byte[]
remotePasswd,
+ /**
+ * Try the primary password, then the retained secondary one
+ * (MySQL-compatible dual password: "RETAIN CURRENT PASSWORD"). A match on
+ * the secondary slot is logged and counted so operators can tell when all
+ * consumers have converged on the new password.
+ */
+ private boolean matchUserPassword(User user, String userDescription,
byte[] remotePasswd,
+ byte[] randomString, String remotePasswdStr, boolean plain) {
+ if (comparePassword(user.getPassword().getPassword(), remotePasswd,
randomString, remotePasswdStr, plain)) {
+ return true;
+ }
+ if (user.getPassword().hasSecondaryPassword()
Review Comment:
Fixed — resolver-materialized IP users are built from a full `Password`
carrying both slots; new test rotates a domain account with RETAIN and asserts
the retained password survives a resolver refresh.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/UserManager.java:
##########
@@ -211,13 +213,36 @@ private String hasRemotePasswd(boolean plain, byte[]
remotePasswd) {
return remotePasswd.length == 0 ? "NO" : "YES";
}
- private boolean comparePassword(Password curUserPassword, byte[]
remotePasswd,
+ /**
+ * Try the primary password, then the retained secondary one
+ * (MySQL-compatible dual password: "RETAIN CURRENT PASSWORD"). A match on
+ * the secondary slot is logged and counted so operators can tell when all
+ * consumers have converged on the new password.
+ */
+ private boolean matchUserPassword(User user, String userDescription,
byte[] remotePasswd,
+ byte[] randomString, String remotePasswdStr, boolean plain) {
+ if (comparePassword(user.getPassword().getPassword(), remotePasswd,
randomString, remotePasswdStr, plain)) {
+ return true;
+ }
+ if (user.getPassword().hasSecondaryPassword()
+ && comparePassword(user.getPassword().getSecondaryPassword(),
+ remotePasswd, randomString, remotePasswdStr, plain)) {
+ LOG.info("user {} authenticated with retained secondary password",
userDescription);
Review Comment:
Fixed — `matchUserPassword` returns the matched slot; the log +
`secondary_password_auth_total` increment happen only after
`checkAccountLockedAndPasswordExpiration` passes. Test locks an account and
asserts both slots reject and the counter does not move.
##########
fe/fe-core/src/test/java/org/apache/doris/mysql/privilege/DualPasswordTest.java:
##########
@@ -0,0 +1,268 @@
+// 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.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.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.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.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+/**
+ * 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 MockedStatic<Env> mockedEnvStatic;
+
+ @Before
+ 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);
+ }
+
+ @After
+ public void tearDown() {
+ mockedEnvStatic.close();
+ }
+
+ 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"));
+ Assert.assertTrue(canLogin("rot", "p1"));
+ Assert.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);
+ Assert.assertTrue(canLogin("rot", "p2"));
+ Assert.assertTrue(canLogin("rot", "p1"));
+ Assert.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);
+ Assert.assertTrue(canLogin("rot", "p3"));
+ Assert.assertTrue(canLogin("rot", "p2"));
+ Assert.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);
+ Assert.assertTrue(canLogin("rot", "p4"));
+ Assert.assertFalse(canLogin("rot", "p3"));
+ Assert.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);
+ Assert.assertTrue(canLogin("rot", "p4"));
+ auth.replayAlterUser(new
AlterUserOperationLog(AlterUserOpType.DISCARD_OLD_PASSWORD,
+ user, null, null, PasswordOptions.UNSET_OPTION, null));
+ Assert.assertTrue(canLogin("rot", "p5"));
+ Assert.assertFalse(canLogin("rot", "p4"));
+
+ // DISCARD with no secondary present: silent no-op (MySQL: discards
+ // the secondary password, "if one exists")
+ auth.replayAlterUser(new
AlterUserOperationLog(AlterUserOpType.DISCARD_OLD_PASSWORD,
+ user, null, null, PasswordOptions.UNSET_OPTION, null));
+ Assert.assertTrue(canLogin("rot", "p5"));
+ }
+
+ @Test
+ public void testRetainRequiresNonEmptyCurrentPassword() throws
DdlException {
+ UserIdentity user = createUser("empty_cur");
+ // MySQL: "If you specify RETAIN CURRENT PASSWORD for an account that
+ // has an empty primary password, the statement fails."
+ Assert.assertThrows(DdlException.class, () ->
auth.setPasswordInternal(user,
+ MysqlPassword.makeScrambledPassword("p1"), null, true, false,
true /* retain */, false));
+ }
+
+ @Test
+ public void testEmptyNewPasswordEmptiesSecondary() throws DdlException {
+ UserIdentity user = createUser("empty_new");
+ auth.setPassword(user, MysqlPassword.makeScrambledPassword("p1"));
+ auth.setPasswordInternal(user,
MysqlPassword.makeScrambledPassword("p2"), null,
+ true, false, true /* retain */, false);
+ Assert.assertTrue(canLogin("empty_new", "p1"));
+
+ // MySQL: "If the new password ... is empty, the secondary password
+ // becomes empty as well, even if RETAIN CURRENT PASSWORD is given."
+ auth.setPasswordInternal(user, new byte[0], null,
+ true, false, true /* retain */, false);
+ Assert.assertTrue(canLogin("empty_new", ""));
+ Assert.assertFalse(canLogin("empty_new", "p1"));
+ Assert.assertFalse(canLogin("empty_new", "p2"));
+ }
+
+ @Test
+ public void testRetainReplay() throws DdlException {
+ UserIdentity user = createUser("replayer");
+ auth.setPassword(user, MysqlPassword.makeScrambledPassword("p1"));
+
+ // a follower replaying OP_SET_PASSWORD with retainPasswd=true must
+ // reach the same dual-slot state as the master
+ auth.replaySetPassword(new PrivInfo(user, null,
+ MysqlPassword.makeScrambledPassword("p2"), null, null, true /*
retain */));
+ Assert.assertTrue(canLogin("replayer", "p2"));
+ Assert.assertTrue(canLogin("replayer", "p1"));
+
+ // and a replay without the flag (all journals written before this
+ // feature, plus any plain password change) behaves like a plain
+ // change: the primary is replaced, the secondary remains UNCHANGED
+ // (MySQL: "the secondary password remains unchanged")
+ auth.replaySetPassword(new PrivInfo(user, null,
+ MysqlPassword.makeScrambledPassword("p3"), null, null));
+ Assert.assertTrue(canLogin("replayer", "p3"));
+ Assert.assertFalse(canLogin("replayer", "p2"));
+ Assert.assertTrue(canLogin("replayer", "p1"));
+ }
+
+ @Test
+ public void testGsonCompat() {
+ // round trip preserves the secondary slot
+ Password password = new
Password(MysqlPassword.makeScrambledPassword("p2"));
+
password.setSecondaryPassword(MysqlPassword.makeScrambledPassword("p1"));
+ Password reloaded =
GsonUtils.GSON.fromJson(GsonUtils.GSON.toJson(password), Password.class);
+ Assert.assertArrayEquals(password.getPassword(),
reloaded.getPassword());
+ Assert.assertArrayEquals(password.getSecondaryPassword(),
reloaded.getSecondaryPassword());
+ Assert.assertTrue(reloaded.hasSecondaryPassword());
+
+ // an image/journal written BEFORE this feature deserializes with an
+ // absent secondary slot -> unchanged single-password behavior
+ Password legacy = GsonUtils.GSON.fromJson(
+ GsonUtils.GSON.toJson(new
Password(MysqlPassword.makeScrambledPassword("p1"))), Password.class);
+ Assert.assertFalse(legacy.hasSecondaryPassword());
+ Assert.assertNull(legacy.getSecondaryPassword());
+ }
+
+ @Test
+ public void testParser() {
+ NereidsParser parser = new NereidsParser();
+
+ AlterUserCommand retain = (AlterUserCommand) parser.parseSingle(
+ "ALTER USER u1 IDENTIFIED BY 'x' RETAIN CURRENT PASSWORD");
+ Assert.assertTrue(retain.getAlterUserInfo().isRetainCurrentPassword());
+ Assert.assertFalse(retain.getAlterUserInfo().isDiscardOldPassword());
+
+ AlterUserCommand discard = (AlterUserCommand) parser.parseSingle(
+ "ALTER USER u1 DISCARD OLD PASSWORD");
+ Assert.assertTrue(discard.getAlterUserInfo().isDiscardOldPassword());
+
Assert.assertFalse(discard.getAlterUserInfo().isRetainCurrentPassword());
+
+ AlterUserCommand plain = (AlterUserCommand) parser.parseSingle(
+ "ALTER USER u1 IDENTIFIED BY 'x'");
+ Assert.assertFalse(plain.getAlterUserInfo().isRetainCurrentPassword());
+ Assert.assertFalse(plain.getAlterUserInfo().isDiscardOldPassword());
+
+ // DISCARD and OLD are nonReserved: still valid as identifiers
+ parser.parseSingle("SELECT old, discard FROM discard.old");
+ }
+
+ @Test
+ public void testValidateRejectsRetainWithoutPasswordChange() {
+ // the clause parses on its own; the semantic rejection (RETAIN
+ // requires a password change) lives in AlterUserInfo.validate()
+ NereidsParser parser = new NereidsParser();
+ AlterUserCommand cmd = (AlterUserCommand) parser.parseSingle(
+ "ALTER USER u1 RETAIN CURRENT PASSWORD");
+ Assert.assertTrue(cmd.getAlterUserInfo().isRetainCurrentPassword());
+ Assert.assertThrows(AnalysisException.class, () ->
cmd.getAlterUserInfo().validate());
+ }
+
+ @Test
+ public void testSetPasswordRetainOverload() throws DdlException {
+ // SET PASSWORD ... RETAIN CURRENT PASSWORD routes through
+ // Auth.setPassword(user, pw, retain=true) — the self-service path a
+ // service account uses to rotate its own credential with an overlap.
+ UserIdentity user = createUser("setpw");
+ auth.setPassword(user, MysqlPassword.makeScrambledPassword("p1"));
+ Assert.assertTrue(canLogin("setpw", "p1"));
+
+ auth.setPassword(user, MysqlPassword.makeScrambledPassword("p2"), true
/* retain */);
+ Assert.assertTrue(canLogin("setpw", "p2")); // new primary
+ Assert.assertTrue(canLogin("setpw", "p1")); // retained
secondary
+
+ // a plain SET PASSWORD (retain=false) preserves the secondary (MySQL)
+ auth.setPassword(user, MysqlPassword.makeScrambledPassword("p3"),
false);
+ Assert.assertTrue(canLogin("setpw", "p3"));
+ Assert.assertTrue(canLogin("setpw", "p1")); // still valid
+ Assert.assertFalse(canLogin("setpw", "p2")); // replaced
primary gone
+ }
+
+ @Test
+ public void testSetPasswordRetainParses() {
+ // The clause parses on SET PASSWORD; no exception, RETAIN accepted.
+ NereidsParser parser = new NereidsParser();
+ parser.parseSingle("SET PASSWORD = PASSWORD('x') RETAIN CURRENT
PASSWORD");
Review Comment:
Done — parsed `SET PASSWORD ... RETAIN` and `ALTER USER` commands are
executed under a `ConnectContext` with redirect-status, journaled-`PrivInfo`,
login-state, and negative-privilege assertions.
--
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]
