change IDL gen, remove security token from opCtx

[jannaerin]

In this patch:

• Added a concatenate_with_tenant_id_and_db namespace type for IDL defined commands
• Removed securityToken from opCtx and put on the request object

Not in this patch:

• Changes to logging
• Changes to audit

Example of command parse method (I realized it might be nice to see the generated code):

CreateCommand CreateCommand::parse(const IDLParserErrorContext& ctxt, const OpMsgRequest& request) {
    NamespaceString localNS;
    CreateCommand object(localNS);
    object.parseProtected(ctxt, request);
    return object;
}
void CreateCommand::parseProtected(const IDLParserErrorContext& ctxt, const OpMsgRequest& request) {
    std::bitset<23> usedFields;
    const size_t kCappedBit = 0;
    const size_t kAutoIndexIdBit = 1;
    const size_t kIdIndexBit = 2;
    const size_t kSizeBit = 3;
    const size_t kMaxBit = 4;
    const size_t kStorageEngineBit = 5;
    const size_t kValidatorBit = 6;
    const size_t kValidationLevelBit = 7;
    const size_t kValidationActionBit = 8;
    const size_t kIndexOptionDefaultsBit = 9;
    const size_t kViewOnBit = 10;
    const size_t kPipelineBit = 11;
    const size_t kCollationBit = 12;
    const size_t kRecordPreImagesBit = 13;
    const size_t kChangeStreamPreAndPostImagesBit = 14;
    const size_t kTimeseriesBit = 15;
    const size_t kClusteredIndexBit = 16;
    const size_t kExpireAfterSecondsBit = 17;
    const size_t kEncryptedFieldsBit = 18;
    const size_t kTempBit = 19;
    const size_t kFlagsBit = 20;
    const size_t kDbNameBit = 21;
    const size_t kDollarTenantIdBit = 22;
    BSONElement commandElement;
    bool firstFieldFound = false;

    for (const auto& element :request.body) {
        const auto fieldName = element.fieldNameStringData();

        if (firstFieldFound == false) {
            commandElement = element;
            firstFieldFound = true;
            continue;
        }

        if (fieldName == kCappedFieldName) {
            if (MONGO_likely(ctxt.checkAndAssertTypes(element, {Bool, NumberLong, NumberInt, NumberDecimal, NumberDouble}))) {
                if (MONGO_unlikely(usedFields[kCappedBit])) {
                    ctxt.throwDuplicateField(element);
                }

                usedFields.set(kCappedBit);

                _capped = element.trueValue();
            }
        }
       ******
       // continue setting all other fields, removing to be more concise
      *******
        else if (fieldName == kDbNameFieldName) {
            if (MONGO_likely(ctxt.checkAndAssertType(element, String))) {
                if (MONGO_unlikely(usedFields[kDbNameBit])) {
                    ctxt.throwDuplicateField(element);
                }

                usedFields.set(kDbNameBit);

                _hasDbName = true;
                _dbName = element.str();
            }
        }
        else if (fieldName == kDollarTenantIdFieldName) {
            if (MONGO_unlikely(usedFields[kDollarTenantIdBit])) {
                ctxt.throwDuplicateField(element);
            }

            usedFields.set(kDollarTenantIdBit);

            _dollarTenantId = TenantId::parseFromBSON(element);
        }
        else {
            if (!mongo::isGenericArgument(fieldName)) {
                ctxt.throwUnknownField(fieldName);
            }
        }
    }


    if (MONGO_unlikely(!usedFields.all())) {
        if (!usedFields[kDbNameBit]) {
            ctxt.throwMissingField(kDbNameFieldName);
        }
    }

    boost::optional<TenantId> tenantId;

    if (request.securityToken.nFields() > 0) {
        _securityToken.emplace(auth::SecurityToken::parse({"Security Token"}, request.securityToken));
        uassert(ErrorCodes::BadValue, "Security token authenticated user requires a valid Tenant ID", _securityToken->getAuthenticatedUser().getTenant());
        tenantId.emplace(*_securityToken->getAuthenticatedUser().getTenant());
    }

    if (!tenantId && _dollarTenantId) {
        tenantId.emplace(*_dollarTenantId);
    }
    invariant(_nss.isEmpty());

    _nss = ctxt.parseNSCollectionRequired(_dbName, commandElement, false, tenantId);
}

[jannaerin]

Thanks for the quick initial feedback! I actually didn't make changes to either logging or audit in this patch - I'm curious:

1. Is it necessary/useful for the tenant id show up in every log line and/or audit event? If we do take this approach then any log line/audit event that contains a namespace will contain the tenant id as a part of the namespace (or db name).
2. How does the tenantId show up in the audit log today? For process logging, it will only show up in the namespace, if a namespace is logged (as far as I'm aware).

I'll respond to your other comments later today as well.

On Thu, Feb 24, 2022 at 11:09 AM Sara Golemon @.***> wrote:

@.**** requested changes on this pull request.

Here's my initial thoughts, still working through finer details.

I'm also wondering if we can avoid putting the entire SecurityToken on the Command objects and keep it to ONLY the TenantID. The authenticatedUser portion really only belongs to authorization processing.

Also, can you save me some effort and help me understand where logging and auditing are getting tenant ID from now? (sorry, I'm headed straight to another meeting and it's not leaping out at me)

In src/mongo/db/auth/security_token.cpp https://github.com/mongodb/mongo/pull/1451#discussion_r814010369:

@@ -107,27 +106,28 @@ BSONObj signSecurityToken(BSONObj obj) { return signedToken.obj(); }

-SecurityToken verifySecurityToken(BSONObj obj) { +SecurityToken verifySecurityToken(SecurityToken token, BSONObj tokenObj) {

This implies the parsing of the token is done elsewhere and this is just a "btw, make sure it's valid."

Could we make the validator BE the parser? Just call this SecurityToken parseSecurityToken(BSONObj) and no do any explicit SecurityToken::parse elsewhere. I just worry that the verify will get accidentally missed at some future point as logic moves and shifts.

In buildscripts/idl/idl/generator.py https://github.com/mongodb/mongo/pull/1451#discussion_r814016299:

@@ -1784,6 +1790,13 @@ def gen_bson_deserializer_methods(self, struct): field_usage_check.add_final_checks() self._writer.write_empty_line()

•        if isinstance(struct, ast.Command):
  

•            optional_block_start = 'if (auto securityToken = bsonObject["authenticatedUser"]) {'
  

•            with self._block(optional_block_start, '}'):
  

•                self._writer.write_line('_securityToken.emplace(auth::SecurityToken::parse({"Security Token"}, securityToken.Obj()));')
  

This is unsafe. Every Command's request() will potentially have an unverified SecurityToken claiming it has an authenticatedUser inside it which may or may not be valid.

I know you're adding a check in service_entry_point_common, but I don't try the assumption baked in to that not to rot. Please see my note on verifySecurityToken() and apply that. I know that's going to complicate linking, so it might be more expedient to make a proxy wrapper for SecurityToken which enforces validation before access to authenticatedUser, but as-is this is not secure by design.

In src/mongo/db/service_entry_point_common.cpp https://github.com/mongodb/mongo/pull/1451#discussion_r814018549:

@@ -1311,11 +1312,16 @@ void ExecCommandDatabase::_initiateCommand() { });

 rpc::readRequestMetadata(opCtx, request, command->requiresAuth());

• uassert(ErrorCodes::Unauthorized,

• if (auto token = _invocation->securityToken()) {

I know you have a check in the generated IDL, but I'd like an addition validation here that not dollar tenant exists on the invocation when we have a security token. e.g.:

if (auto token = _invocation->securityToken()) { uassert(ErrorCodes::OperationFailed, "$tenant may not be specified when using a security token", !_invocation->ns().tenantId()); ...

---

In src/mongo/db/auth/authorization_session.h https://github.com/mongodb/mongo/pull/1451#discussion_r814024713:

@@ -145,7 +145,7 @@ class AuthorizationSession { * Adds the User identified by "UserName" to the authorization session, acquiring privileges * for it in the process. */

• virtual Status addAndAuthorizeUser(OperationContext* opCtx, const UserName& userName) = 0;

• virtual Status addAndAuthorizeUser(OperationContext* opCtx, const UserName& userName, bool fromSecurityToken = false) = 0;

Please avoid using bool parameters. It will be MUCH more self-documenting to do:

virtual Status addAndAuthorizeUser(OperationContext* opCtx, const UserName& userName, AuthorizationMode mode = AuthorizationMode::kConnection) = 0;

Then in the guard pass AuthorizationMode::kSecurityToken instead of an opaque true. And in AuthorizationSessionImpl::addAndAuthorizeUser() add a uassert() which validates that the assumptions made match the declared reality.

In src/mongo/db/auth/authorization_session_impl.cpp https://github.com/mongodb/mongo/pull/1451#discussion_r814027814:

@@ -265,14 +266,14 @@ Status AuthorizationSessionImpl::addAndAuthorizeUser(OperationContext* opCtx,

 stdx::lock_guard<Client> lk(*opCtx->getClient());

• if (auto token = auth::getSecurityToken(opCtx)) {

• if (fromSecurityToken) {

Per comment in header file:

if (mode == AuthenticationMode::kSecurityToken) { uassert(6161501, ...); validateSecurityTokenUserPrivileges(...); } invariant(mode != AuthenticationMode::kNone); _authenticationMode = mode;

---

In src/mongo/db/auth/authorization_session.h https://github.com/mongodb/mongo/pull/1451#discussion_r814031740:

@@ -145,7 +145,7 @@ class AuthorizationSession { * Adds the User identified by "UserName" to the authorization session, acquiring privileges * for it in the process. */

• virtual Status addAndAuthorizeUser(OperationContext* opCtx, const UserName& userName) = 0;

• virtual Status addAndAuthorizeUser(OperationContext* opCtx, const UserName& userName, bool fromSecurityToken = false) = 0;

Actually, just thought about this some more in context of my concerns about the signature validation not being performed reliably.

How about if we have two interface methods:

/**

• Connection based authentication using saslStart/saslContinue or authenticate / virtual Status addAndAuthorizeUser(OperationContext, const UserName&) = 0;

/**

• Security Token based authentication used with mutltitenancy support. / virtual Status addAndAuthorizeUser(OperationContext, const SecurityToken&) = 0;

Then the signature validation happens at the same time as authorization grants, which will dramatically reduce the chances that we fail at this.

— Reply to this email directly, view it on GitHub https://github.com/mongodb/mongo/pull/1451#pullrequestreview-892625359, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB433UWTL5S2BE2YOB4CSU3U4ZJ27ANCNFSM5PHUZ2HQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

[sgolemon]

1. Is it necessary/useful for the tenant id show up in every log line and/or audit event? If we do take this approach then any log line/audit event that contains a namespace will contain the tenant id as a part of the namespace (or db name).

I was very specifically asked to make this part of my work, yes. Not all log messages contain namespaces, many don't.

2. How does the tenantId show up in the audit log today? For process logging, it will only show up in the namespace, if a namespace is logged (as far as I'm aware).

If an TenantID is active, then a new low-level field is included at the same nesting as "id", "severity", or "category". Similar with auditing (where it's easier to comprehend due to the structure of both systems). https://github.com/10gen/mongo-enterprise-modules/blob/536a2e918d6fae3334b0ca6cb3d789914db73164/src/audit/audit_event.cpp#L72-L76

[jannaerin]

1. Got it, I'll continue to think about it - maybe we do leave it on the opCtx just for logging, I'm not sure the best way to handle this yet.
2. Right, we added that in the last number of months but prior to that change did the tenant ID show up in audit logs (I guess by "today" I meant today in Atlas Serverless)? Also, do serverless customers have access to the audit logs?

On Thu, Feb 24, 2022 at 12:23 PM Sara Golemon @.***> wrote:

1. Is it necessary/useful for the tenant id show up in every log line and/or audit event? If we do take this approach then any log line/audit event that contains a namespace will contain the tenant id as a part of the namespace (or db name).

I was very specifically asked to make this part of my work, yes. Not all log messages contain namespaces, many don't.

1. How does the tenantId show up in the audit log today? For process logging, it will only show up in the namespace, if a namespace is logged (as far as I'm aware).

If an TenantID is active, then a new low-level field is included at the same nesting as "id", "severity", or "category". Similar with auditing (where it's easier to comprehend due to the structure of both systems). https://github.com/10gen/mongo-enterprise-modules/blob/536a2e918d6fae3334b0ca6cb3d789914db73164/src/audit/audit_event.cpp#L72-L76

— Reply to this email directly, view it on GitHub https://github.com/mongodb/mongo/pull/1451#issuecomment-1050082777, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB433UVHL77ZMGFZLKATXWTU4ZSPHANCNFSM5PHUZ2HQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

[EshaMaharishi]

2. Just a note that audit logs aren't supported in Serverless today.

[sgolemon]

Just a note that audit logs aren't supported in Serverless today.

True. During the scope, audit log support was a "nice to have", but it would be very good to continue supporting it. I also feel like it's the easier support since we (almost) always have a valid Client passed along, and can pull out authorized security token user (including tenant) from the auth session. System log is more complex because we only have global scope to work with there.