From 7435f68e0ecf6f6dd3400a7ff6ea75889c179556 Mon Sep 17 00:00:00 2001 From: Varun Ravichandran Date: Tue, 18 Mar 2025 13:26:46 -0700 Subject: [PATCH] SERVER-101941 Require cluster user privileges to append dollar audit on mongos (#33713) GitOrigin-RevId: 39bc10e94fe285cc50b9e7c0138fcaab1b624f0c --- jstests/auth/impersonation-deny.js | 37 +++++--- .../db/service_entry_point_shard_role.cpp | 16 ---- src/mongo/rpc/metadata/audit_metadata.cpp | 30 +++++-- .../rpc/metadata/audit_metadata_test.cpp | 87 +++++++++++++++++++ 4 files changed, 133 insertions(+), 37 deletions(-) diff --git a/jstests/auth/impersonation-deny.js b/jstests/auth/impersonation-deny.js index a7fbc3e452c..b23e6f12567 100644 --- a/jstests/auth/impersonation-deny.js +++ b/jstests/auth/impersonation-deny.js @@ -2,9 +2,10 @@ // @tags: [requires_replication] import {ReplSetTest} from "jstests/libs/replsettest.js"; +import {ShardingTest} from "jstests/libs/shardingtest.js"; -function testMongod(mongod, systemuserpwd = undefined) { - const admin = mongod.getDB('admin'); +function runTest(conn, keyFile = undefined) { + const admin = conn.getDB('admin'); admin.createUser({user: 'admin', pwd: 'admin', roles: ['root']}); function assertError(cmd, msg, code) { @@ -56,15 +57,14 @@ function testMongod(mongod, systemuserpwd = undefined) { ErrorCodes.Unauthorized); admin.logout(); - if (systemuserpwd !== undefined) { - // On a ReplSet, our impersonation payload should be fine with cluster user. + if (keyFile !== undefined) { + // On a ReplSet or mongos, our impersonation payload should be fine with cluster user. jsTest.log('Positive test, impersonation is okay when we\'re local.__system'); - const local = mongod.getDB('local'); - local.auth('__system', systemuserpwd); - assert.commandWorked(admin.runCommand(kImpersonatedUserHello)); - assert.commandWorked(admin.runCommand(kImpersonatedClientHello)); - local.logout(); + authutil.asCluster(conn, keyFile, () => { + assert.commandWorked(admin.runCommand(kImpersonatedUserHello)); + assert.commandWorked(admin.runCommand(kImpersonatedClientHello)); + }); } jsTest.log('End'); @@ -72,18 +72,31 @@ function testMongod(mongod, systemuserpwd = undefined) { { const standalone = MongoRunner.runMongod({auth: ''}); - testMongod(standalone); + runTest(standalone); MongoRunner.stopMongod(standalone); } { const kKeyfile = 'jstests/libs/key1'; - const kKey = cat(kKeyfile).replace(/[\011-\015\040]/g, ''); const rst = new ReplSetTest({nodes: 2}); rst.startSet({keyFile: kKeyfile}); rst.initiate(); rst.awaitSecondaryNodes(); - testMongod(rst.getPrimary(), kKey); + runTest(rst.getPrimary(), kKeyfile); rst.stopSet(); } + +{ + const kKeyfile = 'jstests/libs/key1'; + + const st = new ShardingTest({ + mongos: 1, + config: 1, + shard: 2, + keyFile: kKeyfile, + other: {mongosOptions: {auth: null}, configOptions: {auth: null}, rsOptions: {auth: null}} + }); + runTest(st.s0, kKeyfile); + st.stop(); +} diff --git a/src/mongo/db/service_entry_point_shard_role.cpp b/src/mongo/db/service_entry_point_shard_role.cpp index 097eafaf518..ddc07c3fae0 100644 --- a/src/mongo/db/service_entry_point_shard_role.cpp +++ b/src/mongo/db/service_entry_point_shard_role.cpp @@ -1651,22 +1651,6 @@ void ExecCommandDatabase::_initiateCommand() { "Skipping command execution for help request")); } - if (auto auditUserAttrs = rpc::AuditUserAttrs::get(opCtx); - auditUserAttrs && auditUserAttrs->getIsImpersonating()) { - uassert( - ErrorCodes::Unauthorized, - "Unauthorized use of impersonation metadata.", - authzSession->isAuthorizedForClusterActions({ActionType::impersonate}, boost::none)); - } - - if (auto auditClientAttrs = rpc::AuditClientAttrs::get(client); - auditClientAttrs && auditClientAttrs->getIsImpersonating()) { - uassert( - ErrorCodes::Unauthorized, - "Unauthorized use of impersonation metadata.", - authzSession->isAuthorizedForClusterActions({ActionType::impersonate}, boost::none)); - } - _invocation->checkAuthorization(opCtx, _execContext.getRequest()); if (!opCtx->getClient()->isInDirectClient() && diff --git a/src/mongo/rpc/metadata/audit_metadata.cpp b/src/mongo/rpc/metadata/audit_metadata.cpp index e4e9c7536e1..3d81a770847 100644 --- a/src/mongo/rpc/metadata/audit_metadata.cpp +++ b/src/mongo/rpc/metadata/audit_metadata.cpp @@ -54,7 +54,7 @@ namespace mongo { namespace rpc { void setAuditClientMetadata(OperationContext* opCtx, - const boost::optional& data, + const ImpersonatedClientMetadata& clientMetadata, boost::optional& clientSessionGuard) { // TODO SERVER-83990: remove if (!gFeatureFlagExposeClientIpInAuditLogs.isEnabledUseLastLTSFCVWhenUninitialized( @@ -66,20 +66,32 @@ void setAuditClientMetadata(OperationContext* opCtx, return; } - if (!data || !data->getClientMetadata()) { - return; - } - - clientSessionGuard.emplace(opCtx->getClient(), data->getClientMetadata().value()); + clientSessionGuard.emplace(opCtx->getClient(), clientMetadata); } void setAuditMetadata(OperationContext* opCtx, const boost::optional& data, boost::optional& clientSessionGuard) { - setAuditClientMetadata(opCtx, data, clientSessionGuard); if (data) { - if (auto& user = data->getUser()) { - rpc::AuditUserAttrs::set(opCtx, *user, data->getRoles(), true /* isImpersonating */); + const auto& user = data->getUser(); + const auto& roles = data->getRoles(); + const auto& clientMetadata = data->getClientMetadata(); + if (user || !roles.empty() || clientMetadata) { + // Only set $impersonatedUser, $impersonatedRoles, or $impersonatedClient if the client + // is authorized for cluster-level privileges. + auto* authzSession = AuthorizationSession::get(opCtx->getClient()); + uassert(ErrorCodes::Unauthorized, + "Unauthorized use of impersonation metadata.", + authzSession->isAuthorizedForClusterActions({ActionType::impersonate}, + boost::none)); + + if (clientMetadata) { + setAuditClientMetadata(opCtx, clientMetadata.value(), clientSessionGuard); + } + + if (user || !roles.empty()) { + rpc::AuditUserAttrs::set(opCtx, *user, roles, true /* isImpersonating */); + } } } } diff --git a/src/mongo/rpc/metadata/audit_metadata_test.cpp b/src/mongo/rpc/metadata/audit_metadata_test.cpp index 298dcb51d0d..da0b70d205b 100644 --- a/src/mongo/rpc/metadata/audit_metadata_test.cpp +++ b/src/mongo/rpc/metadata/audit_metadata_test.cpp @@ -157,6 +157,9 @@ TEST_F(AuditMetadataTest, ReadAuditMetadata) { ASSERT_TRUE(auditClientAttrs); ASSERT_TRUE(auditUserAttrs); + ASSERT_EQ(auditUserAttrs->getUser().getUser(), kUserName); + ASSERT_EQ(auditUserAttrs->getUser().getDatabaseName().toString_forTest(), kDBName); + ASSERT_EQ(auditClientAttrs->getRemote(), HostAndPort::parse(kRemoteAddr)); ASSERT_EQ(kExpectedProxies.size(), auditClientAttrs->getProxies().size()); for (size_t i = 0; i < auditClientAttrs->getProxies().size(); i++) { @@ -173,6 +176,90 @@ TEST_F(AuditMetadataTest, ReadAuditMetadata) { ASSERT_TRUE(auditUserAttrs); } +TEST_F(AuditMetadataTest, ReadAuditMetadataUserOnly) { + auto auditClientAttrs = rpc::AuditClientAttrs::get(client()); + auto auditUserAttrs = rpc::AuditUserAttrs::get(opCtx()); + ASSERT_FALSE(auditClientAttrs); + ASSERT_FALSE(auditUserAttrs); + + // Construct an AuditMetadata object representing a parsed $audit object. + BSONObj dollarAudit = BSON("$impersonatedUser" << BSON("user" << kUserName << "db" << kDBName) + << "$impersonatedRoles" << BSONArray()); + AuditMetadata parsedDollarAudit = + AuditMetadata::parse(IDLParserContext{kImpersonationMetadataSectionName}, dollarAudit); + GenericArguments requestArgs; + requestArgs.setDollarAudit(parsedDollarAudit); + + { + boost::optional clientSessionGuard; + ASSERT_FALSE(clientSessionGuard); + rpc::readRequestMetadata(opCtx(), requestArgs, false, clientSessionGuard); + ASSERT_FALSE(clientSessionGuard.has_value()); + + // Now, AuditClientAttrs and AuditUserAttrs should be updated to store the user + // info supplied by requestArgs. Since there was nothing in $impersonatedClient, + // auditClientAttrs should still be empty. + auditClientAttrs = rpc::AuditClientAttrs::get(client()); + auditUserAttrs = rpc::AuditUserAttrs::get(opCtx()); + ASSERT_FALSE(auditClientAttrs); + ASSERT_TRUE(auditUserAttrs); + + ASSERT_EQ(auditUserAttrs->getUser().getUser(), kUserName); + ASSERT_EQ(auditUserAttrs->getUser().getDatabaseName().toString_forTest(), kDBName); + } + + // After clientSessionGuard goes out of scope, the AuditClientAttrs should be cleared. Since + // AuditUserAttrs is scoped to a single OperationContext and that is still alive, it will be + // nonempty. + auditClientAttrs = rpc::AuditClientAttrs::get(client()); + auditUserAttrs = rpc::AuditUserAttrs::get(opCtx()); + ASSERT_FALSE(auditClientAttrs); + ASSERT_TRUE(auditUserAttrs); +} + +TEST_F(AuditMetadataTest, ReadAuditMetadataClientOnly) { + auto auditClientAttrs = rpc::AuditClientAttrs::get(client()); + auto auditUserAttrs = rpc::AuditUserAttrs::get(opCtx()); + ASSERT_FALSE(auditClientAttrs); + ASSERT_FALSE(auditUserAttrs); + + // Construct an AuditMetadata object representing a parsed $audit object. + BSONObj dollarAudit = + BSON("$impersonatedRoles" << BSONArray() << "$impersonatedClient" + << BSON("hosts" + << BSON_ARRAY(kRemoteAddr << kLocalAddr << kProxyAddr))); + AuditMetadata parsedDollarAudit = + AuditMetadata::parse(IDLParserContext{kImpersonationMetadataSectionName}, dollarAudit); + GenericArguments requestArgs; + requestArgs.setDollarAudit(parsedDollarAudit); + + { + boost::optional clientSessionGuard; + ASSERT_FALSE(clientSessionGuard); + rpc::readRequestMetadata(opCtx(), requestArgs, false, clientSessionGuard); + ASSERT_TRUE(clientSessionGuard.has_value()); + + // Now, AuditClientAttrs should be updated to store the client + // info supplied by requestArgs. + auditClientAttrs = rpc::AuditClientAttrs::get(client()); + auditUserAttrs = rpc::AuditUserAttrs::get(opCtx()); + ASSERT_TRUE(auditClientAttrs); + ASSERT_FALSE(auditUserAttrs); + + ASSERT_EQ(auditClientAttrs->getRemote(), HostAndPort::parse(kRemoteAddr)); + ASSERT_EQ(kExpectedProxies.size(), auditClientAttrs->getProxies().size()); + for (size_t i = 0; i < auditClientAttrs->getProxies().size(); i++) { + ASSERT_EQ(auditClientAttrs->getProxies()[i], HostAndPort::parse(kExpectedProxies[i])); + } + } + + // After clientSessionGuard goes out of scope, the AuditClientAttrs should be cleared. + auditClientAttrs = rpc::AuditClientAttrs::get(client()); + auditUserAttrs = rpc::AuditUserAttrs::get(opCtx()); + ASSERT_FALSE(auditClientAttrs); + ASSERT_FALSE(auditUserAttrs); +} + TEST_F(AuditMetadataTest, WriteAuditMetadata) { setUpTestData();