From a9277e874039f32ce0d848fcdfb10de705c96fd9 Mon Sep 17 00:00:00 2001 From: Shreyas Kalyan Date: Mon, 4 Feb 2019 14:08:32 -0500 Subject: SERVER-39058 Synchronize user set modification in AuthorizationSession with Client --- src/mongo/db/auth/authorization_session.h | 19 +++++++++---- src/mongo/db/auth/authorization_session_impl.cpp | 32 +++++++++++++++++----- src/mongo/db/auth/authorization_session_impl.h | 8 ++++-- src/mongo/db/auth/authorization_session_test.cpp | 4 +-- src/mongo/db/commands/authentication_commands.cpp | 6 ++-- src/mongo/db/commands/kill_op_cmd_base.cpp | 3 +- src/mongo/db/index_builder.cpp | 2 +- src/mongo/db/pipeline/mongo_process_common.cpp | 2 +- src/mongo/db/repl/bgsync.cpp | 2 +- src/mongo/db/repl/oplog_applier.cpp | 2 +- ...replication_coordinator_external_state_impl.cpp | 2 +- src/mongo/db/repl/task_runner.cpp | 2 +- src/mongo/db/s/migration_destination_manager.cpp | 2 +- .../db/s/transaction_coordinator_futures_util.cpp | 3 +- src/mongo/db/s/txn_two_phase_commit_cmds.cpp | 2 +- src/mongo/db/ttl.cpp | 2 +- src/mongo/embedded/embedded_auth_session.cpp | 10 +++++-- 17 files changed, 68 insertions(+), 35 deletions(-) diff --git a/src/mongo/db/auth/authorization_session.h b/src/mongo/db/auth/authorization_session.h index 1c1377df18b..0c4bc0d4f4a 100644 --- a/src/mongo/db/auth/authorization_session.h +++ b/src/mongo/db/auth/authorization_session.h @@ -170,12 +170,15 @@ public: virtual std::string getAuthenticatedUserNamesToken() = 0; // Removes any authenticated principals whose authorization credentials came from the given - // database, and revokes any privileges that were granted via that principal. - virtual void logoutDatabase(StringData dbname) = 0; + // database, and revokes any privileges that were granted via that principal. This function + // modifies state. Synchronizes with the Client lock. + virtual void logoutDatabase(OperationContext* opCtx, StringData dbname) = 0; // Adds the internalSecurity user to the set of authenticated users. - // Used to grant internal threads full access. - virtual void grantInternalAuthorization() = 0; + // Used to grant internal threads full access. Takes in the Client + // as a parameter so it can take out a lock on the client. + virtual void grantInternalAuthorization(Client* client) = 0; + virtual void grantInternalAuthorization(OperationContext* opCtx) = 0; // Generates a vector of default privileges that are granted to any user, // regardless of which roles that user does or does not possess. @@ -338,8 +341,12 @@ public: // authenticated user. If either object has impersonated users, // those users will be considered as 'authenticated' for the purpose of this check. // - // The existence of 'opClient' must be guaranteed through locks taken by the caller. - virtual bool isCoauthorizedWithClient(Client* opClient) = 0; + // The existence of 'opClient' must be guaranteed through locks taken by the caller, + // as demonstrated by opClientLock which must be a lock taken on opClient. + // + // Returns true if the current auth session and the opClient's auth session have users + // in common. + virtual bool isCoauthorizedWithClient(Client* opClient, WithLock opClientLock) = 0; // Returns true if the session and 'userNameIter' share an authenticated user, or if both have // no authenticated users. Impersonated users are not considered as 'authenticated' for the diff --git a/src/mongo/db/auth/authorization_session_impl.cpp b/src/mongo/db/auth/authorization_session_impl.cpp index 65e7558a7f5..f73e8a46b6d 100644 --- a/src/mongo/db/auth/authorization_session_impl.cpp +++ b/src/mongo/db/auth/authorization_session_impl.cpp @@ -49,6 +49,7 @@ #include "mongo/db/client.h" #include "mongo/db/jsobj.h" #include "mongo/db/namespace_string.h" +#include "mongo/db/operation_context.h" #include "mongo/db/pipeline/aggregation_request.h" #include "mongo/db/pipeline/lite_parsed_pipeline.h" #include "mongo/util/assert_util.h" @@ -138,6 +139,7 @@ Status AuthorizationSessionImpl::addAndAuthorizeUser(OperationContext* opCtx, return AuthorizationManager::authenticationFailedStatus; } + stdx::lock_guard lk(*opCtx->getClient()); _authenticatedUsers.add(std::move(user)); // If there are any users and roles in the impersonation data, clear it out. @@ -167,7 +169,8 @@ User* AuthorizationSessionImpl::getSingleUser() { return lookupUser(userName); } -void AuthorizationSessionImpl::logoutDatabase(StringData dbname) { +void AuthorizationSessionImpl::logoutDatabase(OperationContext* opCtx, StringData dbname) { + stdx::lock_guard lk(*opCtx->getClient()); _authenticatedUsers.removeByDBName(dbname); clearImpersonatedUserData(); _buildAuthenticatedRolesVector(); @@ -192,11 +195,20 @@ std::string AuthorizationSessionImpl::getAuthenticatedUserNamesToken() { return ret; } -void AuthorizationSessionImpl::grantInternalAuthorization() { +void AuthorizationSessionImpl::grantInternalAuthorization(Client* client) { + stdx::lock_guard lk(*client); _authenticatedUsers.add(internalSecurity.user); _buildAuthenticatedRolesVector(); } +/** + * Overloaded function - takes in the opCtx of the current AuthSession + * and calls the function above. + */ +void AuthorizationSessionImpl::grantInternalAuthorization(OperationContext* opCtx) { + grantInternalAuthorization(opCtx->getClient()); +} + PrivilegeVector AuthorizationSessionImpl::getDefaultPrivileges() { PrivilegeVector defaultPrivileges; @@ -720,10 +732,6 @@ void AuthorizationSessionImpl::_refreshUserInfoAsNeeded(OperationContext* opCtx) while (it != _authenticatedUsers.end()) { auto& user = *it; if (!user->isValid()) { - // The user is invalid, so make sure that we erase it from _authenticateUsers at the - // end of this block. - auto removeGuard = makeGuard([&] { _authenticatedUsers.removeAt(it++); }); - // Make a good faith effort to acquire an up-to-date user object, since the one // we've cached is marked "out-of-date." UserName name = user->getName(); @@ -731,6 +739,15 @@ void AuthorizationSessionImpl::_refreshUserInfoAsNeeded(OperationContext* opCtx) auto swUser = authMan.acquireUserForSessionRefresh(opCtx, name, user->getID()); auto& status = swUser.getStatus(); + + // Take out a lock on the client here to ensure that no one reads while + // _authenticatedUsers is being modified. + stdx::lock_guard lk(*opCtx->getClient()); + + // The user is invalid, so make sure that we erase it from _authenticateUsers at the + // end of this block. + auto removeGuard = makeGuard([&] { _authenticatedUsers.removeAt(it++); }); + switch (status.code()) { case ErrorCodes::OK: { updatedUser = std::move(swUser.getValue()); @@ -911,7 +928,7 @@ void AuthorizationSessionImpl::setImpersonatedUserData(std::vector use _impersonationFlag = true; } -bool AuthorizationSessionImpl::isCoauthorizedWithClient(Client* opClient) { +bool AuthorizationSessionImpl::isCoauthorizedWithClient(Client* opClient, WithLock opClientLock) { auto getUserNames = [](AuthorizationSession* authSession) { if (authSession->isImpersonating()) { return authSession->getImpersonatedUserNames(); @@ -939,6 +956,7 @@ bool AuthorizationSessionImpl::isCoauthorizedWith(UserNameIterator userNameIter) if (!getAuthorizationManager().isAuthEnabled()) { return true; } + if (!userNameIter.more() && !isAuthenticated()) { return true; } diff --git a/src/mongo/db/auth/authorization_session_impl.h b/src/mongo/db/auth/authorization_session_impl.h index 0b6026a1e4a..2cdc380ec77 100644 --- a/src/mongo/db/auth/authorization_session_impl.h +++ b/src/mongo/db/auth/authorization_session_impl.h @@ -94,9 +94,11 @@ public: std::string getAuthenticatedUserNamesToken() override; - void logoutDatabase(StringData dbname) override; + void logoutDatabase(OperationContext* opCtx, StringData dbname) override; - void grantInternalAuthorization() override; + void grantInternalAuthorization(Client* client) override; + + void grantInternalAuthorization(OperationContext* opCtx) override; PrivilegeVector getDefaultPrivileges() override; @@ -186,7 +188,7 @@ public: void clearImpersonatedUserData() override; - bool isCoauthorizedWithClient(Client* opClient) override; + bool isCoauthorizedWithClient(Client* opClient, WithLock opClientLock) override; bool isCoauthorizedWith(UserNameIterator userNameIter) override; diff --git a/src/mongo/db/auth/authorization_session_test.cpp b/src/mongo/db/auth/authorization_session_test.cpp index 4e7d6751ba5..b0ec73151e4 100644 --- a/src/mongo/db/auth/authorization_session_test.cpp +++ b/src/mongo/db/auth/authorization_session_test.cpp @@ -228,7 +228,7 @@ TEST_F(AuthorizationSessionTest, AddUserAndCheckAuthorization) { ASSERT_TRUE( authzSession->isAuthorizedForActionsOnResource(testFooCollResource, ActionType::insert)); - authzSession->logoutDatabase("test"); + authzSession->logoutDatabase(_opCtx.get(), "test"); ASSERT_TRUE( authzSession->isAuthorizedForActionsOnResource(otherFooCollResource, ActionType::insert)); ASSERT_TRUE( @@ -236,7 +236,7 @@ TEST_F(AuthorizationSessionTest, AddUserAndCheckAuthorization) { ASSERT_FALSE( authzSession->isAuthorizedForActionsOnResource(testFooCollResource, ActionType::collMod)); - authzSession->logoutDatabase("admin"); + authzSession->logoutDatabase(_opCtx.get(), "admin"); ASSERT_FALSE( authzSession->isAuthorizedForActionsOnResource(otherFooCollResource, ActionType::insert)); ASSERT_FALSE( diff --git a/src/mongo/db/commands/authentication_commands.cpp b/src/mongo/db/commands/authentication_commands.cpp index 90fe580b0b6..7666955b122 100644 --- a/src/mongo/db/commands/authentication_commands.cpp +++ b/src/mongo/db/commands/authentication_commands.cpp @@ -121,7 +121,7 @@ Status _authenticateX509(OperationContext* opCtx, const UserName& user, const BS } } - authorizationSession->grantInternalAuthorization(); + authorizationSession->grantInternalAuthorization(client); } // Handle normal client authentication, only applies to client-server connections else { @@ -336,14 +336,14 @@ public: const BSONObj& cmdObj, BSONObjBuilder& result) { AuthorizationSession* authSession = AuthorizationSession::get(Client::getCurrent()); - authSession->logoutDatabase(dbname); + authSession->logoutDatabase(opCtx, dbname); if (getTestCommandsEnabled() && dbname == "admin") { // Allows logging out as the internal user against the admin database, however // this actually logs out of the local database as well. This is to // support the auth passthrough test framework on mongos (since you can't use the // local database on a mongos, so you can't logout as the internal user // without this). - authSession->logoutDatabase("local"); + authSession->logoutDatabase(opCtx, "local"); } return true; } diff --git a/src/mongo/db/commands/kill_op_cmd_base.cpp b/src/mongo/db/commands/kill_op_cmd_base.cpp index 1306013467d..f6394998de4 100644 --- a/src/mongo/db/commands/kill_op_cmd_base.cpp +++ b/src/mongo/db/commands/kill_op_cmd_base.cpp @@ -95,7 +95,8 @@ KillOpCmdBase::findOpForKilling(Client* client, unsigned int opId) { OperationContext* opToKill = std::get<1>(*lockAndOpCtx); if (authzSession->isAuthorizedForActionsOnResource(ResourcePattern::forClusterResource(), ActionType::killop) || - authzSession->isCoauthorizedWithClient(opToKill->getClient())) { + authzSession->isCoauthorizedWithClient(opToKill->getClient(), + std::get<0>(*lockAndOpCtx))) { return lockAndOpCtx; } } diff --git a/src/mongo/db/index_builder.cpp b/src/mongo/db/index_builder.cpp index 4d9c82548b0..891c18a9153 100644 --- a/src/mongo/db/index_builder.cpp +++ b/src/mongo/db/index_builder.cpp @@ -111,7 +111,7 @@ void IndexBuilder::run() { unreplicatedWrites.emplace(opCtx.get()); } - AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization(); + AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization(opCtx.get()); { stdx::lock_guard lk(*(opCtx->getClient())); diff --git a/src/mongo/db/pipeline/mongo_process_common.cpp b/src/mongo/db/pipeline/mongo_process_common.cpp index 3ace59e5e23..5db5dd993ee 100644 --- a/src/mongo/db/pipeline/mongo_process_common.cpp +++ b/src/mongo/db/pipeline/mongo_process_common.cpp @@ -66,7 +66,7 @@ std::vector MongoProcessCommon::getCurrentOps( // If auth is disabled, ignore the allUsers parameter. if (ctxAuth->getAuthorizationManager().isAuthEnabled() && userMode == CurrentOpUserMode::kExcludeOthers && - !ctxAuth->isCoauthorizedWithClient(client)) { + !ctxAuth->isCoauthorizedWithClient(client, lk)) { continue; } diff --git a/src/mongo/db/repl/bgsync.cpp b/src/mongo/db/repl/bgsync.cpp index 3181bc132ee..140371c2895 100644 --- a/src/mongo/db/repl/bgsync.cpp +++ b/src/mongo/db/repl/bgsync.cpp @@ -167,7 +167,7 @@ bool BackgroundSync::_inShutdown_inlock() const { void BackgroundSync::_run() { Client::initThread("rsBackgroundSync"); - AuthorizationSession::get(cc())->grantInternalAuthorization(); + AuthorizationSession::get(cc())->grantInternalAuthorization(&cc()); while (!inShutdown()) { try { diff --git a/src/mongo/db/repl/oplog_applier.cpp b/src/mongo/db/repl/oplog_applier.cpp index eb06a50d681..a3c4e1953f8 100644 --- a/src/mongo/db/repl/oplog_applier.cpp +++ b/src/mongo/db/repl/oplog_applier.cpp @@ -57,7 +57,7 @@ std::unique_ptr OplogApplier::makeWriterPool(int threadCount) { options.maxThreads = options.minThreads = static_cast(threadCount); options.onCreateThread = [](const std::string&) { Client::initThread(getThreadName()); - AuthorizationSession::get(cc())->grantInternalAuthorization(); + AuthorizationSession::get(cc())->grantInternalAuthorization(&cc()); }; auto pool = stdx::make_unique(options); pool->startup(); diff --git a/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp b/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp index dba73cf46cd..8c6636f86c4 100644 --- a/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp +++ b/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp @@ -150,7 +150,7 @@ auto makeThreadPool(const std::string& poolName) { threadPoolOptions.poolName = poolName; threadPoolOptions.onCreateThread = [](const std::string& threadName) { Client::initThread(threadName.c_str()); - AuthorizationSession::get(cc())->grantInternalAuthorization(); + AuthorizationSession::get(cc())->grantInternalAuthorization(&cc()); }; return stdx::make_unique(threadPoolOptions); } diff --git a/src/mongo/db/repl/task_runner.cpp b/src/mongo/db/repl/task_runner.cpp index f14a7a34937..2226ad2bcba 100644 --- a/src/mongo/db/repl/task_runner.cpp +++ b/src/mongo/db/repl/task_runner.cpp @@ -135,7 +135,7 @@ void TaskRunner::_runTasks() { // client used to create the operation context. Client* client = &cc(); if (AuthorizationManager::get(client->getServiceContext())->isAuthEnabled()) { - AuthorizationSession::get(client)->grantInternalAuthorization(); + AuthorizationSession::get(client)->grantInternalAuthorization(client); } ServiceContext::UniqueOperationContext opCtx; diff --git a/src/mongo/db/s/migration_destination_manager.cpp b/src/mongo/db/s/migration_destination_manager.cpp index def44ce2e83..5113d76d537 100644 --- a/src/mongo/db/s/migration_destination_manager.cpp +++ b/src/mongo/db/s/migration_destination_manager.cpp @@ -698,7 +698,7 @@ void MigrationDestinationManager::_migrateThread() { if (AuthorizationManager::get(opCtx->getServiceContext())->isAuthEnabled()) { - AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization(); + AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization(opCtx.get()); } try { diff --git a/src/mongo/db/s/transaction_coordinator_futures_util.cpp b/src/mongo/db/s/transaction_coordinator_futures_util.cpp index 81d101c91e8..c3b68b56887 100644 --- a/src/mongo/db/s/transaction_coordinator_futures_util.cpp +++ b/src/mongo/db/s/transaction_coordinator_futures_util.cpp @@ -96,7 +96,8 @@ Future AsyncWorkScheduler::scheduleRemot opCtx) { // Note: This internal authorization is tied to the lifetime of 'opCtx', which is // destroyed by 'scheduleWork' immediately after this lambda ends. - AuthorizationSession::get(Client::getCurrent())->grantInternalAuthorization(); + AuthorizationSession::get(Client::getCurrent()) + ->grantInternalAuthorization(Client::getCurrent()); if (MONGO_FAIL_POINT(hangWhileTargetingLocalHost)) { LOG(0) << "Hit hangWhileTargetingLocalHost failpoint"; diff --git a/src/mongo/db/s/txn_two_phase_commit_cmds.cpp b/src/mongo/db/s/txn_two_phase_commit_cmds.cpp index 276c569fd66..d883cbffc76 100644 --- a/src/mongo/db/s/txn_two_phase_commit_cmds.cpp +++ b/src/mongo/db/s/txn_two_phase_commit_cmds.cpp @@ -287,7 +287,7 @@ public: auto uniqueOpCtx = Client::getCurrent()->makeOperationContext(); auto opCtx = uniqueOpCtx.get(); - AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization(); + AuthorizationSession::get(opCtx->getClient())->grantInternalAuthorization(opCtx); auto requestOpMsg = OpMsgRequest::fromDBAndBody(NamespaceString::kAdminDb, abortRequestObj) diff --git a/src/mongo/db/ttl.cpp b/src/mongo/db/ttl.cpp index 93db8042248..ddbbba429f5 100644 --- a/src/mongo/db/ttl.cpp +++ b/src/mongo/db/ttl.cpp @@ -82,7 +82,7 @@ public: virtual void run() { ThreadClient tc(name(), getGlobalServiceContext()); - AuthorizationSession::get(cc())->grantInternalAuthorization(); + AuthorizationSession::get(cc())->grantInternalAuthorization(&cc()); while (!globalInShutdownDeprecated()) { { diff --git a/src/mongo/embedded/embedded_auth_session.cpp b/src/mongo/embedded/embedded_auth_session.cpp index 13a06665d8c..92b8741aa9a 100644 --- a/src/mongo/embedded/embedded_auth_session.cpp +++ b/src/mongo/embedded/embedded_auth_session.cpp @@ -97,11 +97,15 @@ public: UASSERT_NOT_IMPLEMENTED; } - void grantInternalAuthorization() override { + void grantInternalAuthorization(Client* client) override { // Always okay to do something, on embedded. } - void logoutDatabase(const StringData) override { + void grantInternalAuthorization(OperationContext* opCtx) override { + // Always okay to do something, on embedded. + } + + void logoutDatabase(OperationContext* opCtx, const StringData) override { UASSERT_NOT_IMPLEMENTED; } @@ -237,7 +241,7 @@ public: UASSERT_NOT_IMPLEMENTED; } - bool isCoauthorizedWithClient(Client*) override { + bool isCoauthorizedWithClient(Client*, WithLock opClientLock) override { return true; } -- cgit v1.2.1