SERVER-34446: Remove normalization of SCRAM-SHA-256 prinicpal names

7 files changed, 39 insertions, 158 deletions

diff --git a/jstests/auth/sasl_mechanism_discovery.js b/jstests/auth/sasl_mechanism_discovery.js
index 7ea4c75f5b4..2d5404178fd 100644
--- a/jstests/auth/sasl_mechanism_discovery.js
+++ b/jstests/auth/sasl_mechanism_discovery.js
@@ -4,30 +4,36 @@
     "use strict";
 
     function runTest(conn) {
+        function checkMechs(userid, mechs) {
+            const res =
+                assert.commandWorked(db.runCommand({isMaster: 1, saslSupportedMechs: userid}));
+            assert.eq(mechs.sort(), res.saslSupportedMechs.sort(), tojson(res));
+        }
+
         var db = conn.getDB("admin");
         var externalDB = conn.getDB("$external");
 
-        // Check that unknown or users produce the correct errors.
+        // Check that unknown or invalid users produce the correct errors.
         assert.commandFailedWithCode(db.runCommand({isMaster: 1, saslSupportedMechs: "test.bogus"}),
                                      ErrorCodes.UserNotFound);
 
         assert.commandFailedWithCode(db.runCommand({isMaster: 1, saslSupportedMechs: "bogus"}),
                                      ErrorCodes.BadValue);
 
+        // Create a legacy user.
+        assert.commandWorked(db.adminCommand({setFeatureCompatibilityVersion: "3.6"}));
+        assert.commandWorked(db.runCommand({createUser: "legacyUser", pwd: "pwd", roles: []}));
+        checkMechs("admin.legacyUser", ["SCRAM-SHA-1"]);
+
         // Enable SCRAM-SHA-256.
         assert.commandWorked(db.adminCommand({setFeatureCompatibilityVersion: "4.0"}));
 
-        function checkMechs(userid, mechs) {
-            const res =
-                assert.commandWorked(db.runCommand({isMaster: 1, saslSupportedMechs: userid}));
-            assert.eq(mechs.sort(), res.saslSupportedMechs.sort(), tojson(res));
-        }
+        // Legacy users continue expressing SCRAM-SHA-1
+        checkMechs("admin.legacyUser", ["SCRAM-SHA-1"]);
 
         // Make users.
         assert.commandWorked(db.runCommand({createUser: "user", pwd: "pwd", roles: []}));
         assert.commandWorked(externalDB.runCommand({createUser: "user", roles: []}));
-        assert.commandWorked(db.runCommand(
-            {createUser: "IX", pwd: "pwd", roles: [], mechanisms: ["SCRAM-SHA-256"]}));
 
         // Internal users should support scram methods.
         checkMechs("admin.user", ["SCRAM-SHA-256", "SCRAM-SHA-1"]);
@@ -39,16 +45,19 @@
             checkMechs("$external.user", []);
         }
 
-        // Check non-normalized name finds normalized user.
-        const IXchar = "\u2168";
-        const IXuserid = "admin." + IXchar;
-        checkMechs(IXuserid, ["SCRAM-SHA-256"]);
-
-        // Check that names with compatibility equivalence collide.
+        // Users with explicit mechs should only support those mechanisms
         assert.commandWorked(db.runCommand(
-            {createUser: IXchar, pwd: "pwd", roles: [], mechanisms: ["SCRAM-SHA-1"]}));
-        assert.commandFailedWithCode(db.runCommand({isMaster: 1, saslSupportedMechs: IXuserid}),
-                                     ErrorCodes.BadValue);
+            {createUser: "256Only", pwd: "pwd", roles: [], mechanisms: ["SCRAM-SHA-256"]}));
+        checkMechs("admin.256Only", ["SCRAM-SHA-256"]);
+        assert.commandWorked(db.runCommand(
+            {createUser: "1Only", pwd: "pwd", roles: [], mechanisms: ["SCRAM-SHA-1"]}));
+        checkMechs("admin.1Only", ["SCRAM-SHA-1"]);
+
+        // Users with normalized and unnormalized names do not conflict
+        assert.commandWorked(db.runCommand({createUser: "IX", pwd: "pwd", roles: []}));
+        checkMechs("admin.IX", ["SCRAM-SHA-1", "SCRAM-SHA-256"]);
+        assert.commandWorked(db.runCommand({createUser: "\u2168", pwd: "pwd", roles: []}));
+        checkMechs("admin.\u2168", ["SCRAM-SHA-1", "SCRAM-SHA-256"]);
     }
 
     // Test standalone.
diff --git a/jstests/auth/user_management_commands_mechanisms.js b/jstests/auth/user_management_commands_mechanisms.js
index 76c778d6c28..8acd695920a 100644
--- a/jstests/auth/user_management_commands_mechanisms.js
+++ b/jstests/auth/user_management_commands_mechanisms.js
@@ -183,21 +183,11 @@
         test.updateUser('user', {pwd: 'passEmpty', mechanisms: []});
     });
 
-    // Fail when passing a non-normalized username and producing SHA-256 creds.
-    assert.throws(function() {
-        test.createUser({user: "\u2168", pwd: 'pass', roles: jsTest.basicUserRoles});
-    });
-
     // Succeed if we're using SHA-1 only.
     test.createUser(
         {user: "\u2168", pwd: 'pass', roles: jsTest.basicUserRoles, mechanisms: ['SCRAM-SHA-1']});
     checkUser("\u2168", 'pass', true, false);
 
-    // Then fail again trying to promote that user to SHA-256.
-    assert.throws(function() {
-        test.updateUser("\u2168", {pwd: 'pass'});
-    });
-
     // Demonstrate that usersInfo returns all users with mechanisms lists
     const allUsersInfo = assert.commandWorked(test.runCommand({usersInfo: 1}));
     allUsersInfo.users.forEach(function(userObj) {
diff --git a/src/mongo/client/sasl_scram_client_conversation.cpp b/src/mongo/client/sasl_scram_client_conversation.cpp
index 227a793486e..6c2fa950e22 100644
--- a/src/mongo/client/sasl_scram_client_conversation.cpp
+++ b/src/mongo/client/sasl_scram_client_conversation.cpp
@@ -90,12 +90,8 @@ StatusWith<bool> SaslSCRAMClientConversation::_firstStep(std::string* outputData
     binaryNonce[1] = sr->nextInt64();
     binaryNonce[2] = sr->nextInt64();
 
-    auto swUser =
-        saslPrep(_saslClientSession->getParameter(SaslClientSession::parameterUser).toString());
-    if (!swUser.isOK()) {
-        return swUser.getStatus();
-    }
-    auto user = swUser.getValue();
+    std::string user =
+        _saslClientSession->getParameter(SaslClientSession::parameterUser).toString();
 
     encodeSCRAMUsername(user);
     _clientNonce = base64::encode(reinterpret_cast<char*>(binaryNonce), sizeof(binaryNonce));
diff --git a/src/mongo/db/auth/sasl_mechanism_registry.cpp b/src/mongo/db/auth/sasl_mechanism_registry.cpp
index 1572acb2794..69c5f495e25 100644
--- a/src/mongo/db/auth/sasl_mechanism_registry.cpp
+++ b/src/mongo/db/auth/sasl_mechanism_registry.cpp
@@ -43,60 +43,6 @@ namespace mongo {
 namespace {
 const auto getSASLServerMechanismRegistry =
     ServiceContext::declareDecoration<std::unique_ptr<SASLServerMechanismRegistry>>();
-
-/**
- * Fetch credentials for the named user either using the unnormalized form provided,
- * or the form returned from saslPrep().
- * If both forms exist as different user records, produce an error.
- */
-User* getUserPtr(OperationContext* opCtx, const UserName& userName) {
-    AuthorizationManager* authManager = AuthorizationManager::get(opCtx->getServiceContext());
-
-    User* rawObj = nullptr;
-    const auto rawStatus = authManager->acquireUser(opCtx, userName, &rawObj);
-    auto rawGuard = MakeGuard([authManager, &rawObj] {
-        if (rawObj) {
-            authManager->releaseUser(rawObj);
-        }
-    });
-
-    // Attempt to normalize the provided username (excluding DB portion).
-    // If saslPrep() fails, then there can't possibly be another user with
-    // compatibility equivalence, so fall-through.
-    const auto swPrepUser = saslPrep(userName.getUser());
-    if (swPrepUser.isOK()) {
-        UserName prepUserName(swPrepUser.getValue(), userName.getDB());
-        if (prepUserName != userName) {
-            // User has a SASLPREPable name which differs from the raw presentation.
-            // Double check that we don't have a different user by that new name.
-            User* prepObj = nullptr;
-            const auto prepStatus = authManager->acquireUser(opCtx, prepUserName, &prepObj);
-
-            auto prepGuard = MakeGuard([authManager, &prepObj] {
-                if (prepObj) {
-                    authManager->releaseUser(prepObj);
-                }
-            });
-
-            if (prepStatus.isOK()) {
-                // If both statuses are OK, then we have two distinct users with "different" names.
-                uassert(ErrorCodes::BadValue,
-                        "Two users exist with names exhibiting compatibility equivalence",
-                        !rawStatus.isOK());
-                // Otherwise, only the normalized version exists.
-                User* returnObj = nullptr;
-                std::swap(returnObj, prepObj);
-                return returnObj;
-            }
-        }
-    }
-
-    uassertStatusOK(rawStatus);
-    User* returnObj = nullptr;
-    std::swap(returnObj, rawObj);
-    return returnObj;
-}
-
 }  // namespace
 
 SASLServerMechanismRegistry& SASLServerMechanismRegistry::get(ServiceContext* serviceContext) {
@@ -133,12 +79,17 @@ void SASLServerMechanismRegistry::advertiseMechanismNamesForUser(OperationContex
     if (saslSupportedMechs.type() == BSONType::String) {
 
         const auto userName = uassertStatusOK(UserName::parse(saslSupportedMechs.String()));
-        const auto userPtr = getUserPtr(opCtx, userName);
-        auto guard = MakeGuard([&opCtx, &userPtr] {
-            AuthorizationManager* authManager =
-                AuthorizationManager::get(opCtx->getServiceContext());
-            authManager->releaseUser(userPtr);
+
+        AuthorizationManager* authManager = AuthorizationManager::get(opCtx->getServiceContext());
+
+        User* user = nullptr;
+        const auto status = authManager->acquireUser(opCtx, userName, &user);
+        auto guard = MakeGuard([authManager, &user] {
+            if (user) {
+                authManager->releaseUser(user);
+            }
         });
+        uassertStatusOK(status);
 
         BSONArrayBuilder mechanismsBuilder;
         auto& map = _getMapRef(userName.getDB());
@@ -151,7 +102,7 @@ void SASLServerMechanismRegistry::advertiseMechanismNamesForUser(OperationContex
                 continue;
             }
 
-            if (factoryIt.second->canMakeMechanismForUser(userPtr)) {
+            if (factoryIt.second->canMakeMechanismForUser(user)) {
                 mechanismsBuilder << factoryIt.first;
             }
         }
diff --git a/src/mongo/db/auth/sasl_mechanism_registry_test.cpp b/src/mongo/db/auth/sasl_mechanism_registry_test.cpp
index 8c8515adb43..1fdea125852 100644
--- a/src/mongo/db/auth/sasl_mechanism_registry_test.cpp
+++ b/src/mongo/db/auth/sasl_mechanism_registry_test.cpp
@@ -179,40 +179,6 @@ public:
                                                         << "roles"
                                                         << BSONArray()),
                                                    BSONObj()));
-
-
-        ASSERT_OK(authManagerExternalState->insert(
-            opCtx.get(),
-            NamespaceString("admin.system.users"),
-            BSON("_id"
-                 << "test.collision"
-                 << "user"
-                 << "collision"
-                 << "db"
-                 << "test"
-                 << "credentials"
-                 << BSON("SCRAM-SHA-256"
-                         << scram::Secrets<SHA256Block>::generateCredentials("collision", 15000))
-                 << "roles"
-                 << BSONArray()),
-            BSONObj()));
-
-        // A user whose name does not equal "test.collision"'s, but ends in a Zero Width Joiner.
-        ASSERT_OK(authManagerExternalState->insert(
-            opCtx.get(),
-            NamespaceString("admin.system.users"),
-            BSON("_id"
-                 << "test.collision‍"  // This string ends in a ZWJ
-                 << "user"
-                 << "collision‍"  // This string ends in a ZWJ
-                 << "db"
-                 << "test"
-                 << "credentials"
-                 << BSON("SCRAM-SHA-256"
-                         << scram::Secrets<SHA256Block>::generateCredentials("collision", 15000))
-                 << "roles"
-                 << BSONArray()),
-            BSONObj()));
     }
 
     ServiceContextNoop serviceContext;
@@ -271,24 +237,6 @@ TEST_F(MechanismRegistryTest, invalidUserCantAdvertiseMechs) {
         AssertionException);
 }
 
-TEST_F(MechanismRegistryTest, collisionsPreventAdvertisement) {
-    registry.registerFactory<FooMechanismFactory<true>>(
-        SASLServerMechanismRegistry::kNoValidateGlobalMechanisms);
-
-    BSONObjBuilder builder;
-
-    registry.advertiseMechanismNamesForUser(opCtx.get(),
-                                            BSON("isMaster" << 1 << "saslSupportedMechs"
-                                                            << "test.collision"),
-                                            &builder);
-    ASSERT_THROWS(
-        registry.advertiseMechanismNamesForUser(opCtx.get(),
-                                                BSON("isMaster" << 1 << "saslSupportedMechs"
-                                                                << "test.collision‍"),
-                                                &builder),
-        AssertionException);
-}
-
 TEST_F(MechanismRegistryTest, strongMechCanAdvertise) {
     registry.registerFactory<BarMechanismFactory<true>>(
         SASLServerMechanismRegistry::kNoValidateGlobalMechanisms);
diff --git a/src/mongo/db/auth/sasl_scram_server_conversation.cpp b/src/mongo/db/auth/sasl_scram_server_conversation.cpp
index 25cc6659247..c82bb5965c6 100644
--- a/src/mongo/db/auth/sasl_scram_server_conversation.cpp
+++ b/src/mongo/db/auth/sasl_scram_server_conversation.cpp
@@ -165,12 +165,6 @@ StatusWith<std::tuple<bool, std::string>> SaslSCRAMServerMechanism<Policy>::_fir
     ServerMechanismBase::_principalName = input[0].substr(2);
     decodeSCRAMUsername(ServerMechanismBase::_principalName);
 
-    auto swUser = saslPrep(ServerMechanismBase::ServerMechanismBase::_principalName);
-    if (!swUser.isOK()) {
-        return swUser.getStatus();
-    }
-    ServerMechanismBase::ServerMechanismBase::_principalName = std::move(swUser.getValue());
-
     if (!authzId.empty() && ServerMechanismBase::_principalName != authzId) {
         return Status(ErrorCodes::BadValue,
                       str::stream() << "SCRAM user name " << ServerMechanismBase::_principalName
diff --git a/src/mongo/db/commands/user_management_commands.cpp b/src/mongo/db/commands/user_management_commands.cpp
index aa126b229da..fd3519d4871 100644
--- a/src/mongo/db/commands/user_management_commands.cpp
+++ b/src/mongo/db/commands/user_management_commands.cpp
@@ -666,13 +666,6 @@ Status buildCredentials(BSONObjBuilder* builder, const auth::CreateOrUpdateUserA
     }
 
     if (buildSCRAMSHA256) {
-        const auto swPreppedName = saslPrep(args.userName.getUser());
-        if (!swPreppedName.isOK() || (swPreppedName.getValue() != args.userName.getUser())) {
-            return {
-                ErrorCodes::BadValue,
-                "Username must be normalized according to SASLPREP rules when using SCRAM-SHA-256"};
-        }
-
         // FCV check is deferred till this point so that the suitability checks can be performed
         // regardless.
         const auto fcv = serverGlobalParams.featureCompatibility.getVersion();