From f6c9ccbd019a074fcfa08155e65aaa759160ea8e Mon Sep 17 00:00:00 2001 From: ijon Date: Fri, 21 Feb 2025 14:08:59 +0300 Subject: security: database admin can not administer database admins (#14873) --- ydb/core/tx/tx_proxy/schemereq.cpp | 60 +++++++++++++++++++++++++++++++++----- 1 file changed, 52 insertions(+), 8 deletions(-) diff --git a/ydb/core/tx/tx_proxy/schemereq.cpp b/ydb/core/tx/tx_proxy/schemereq.cpp index b01aac3ca0b..c16abd2785d 100644 --- a/ydb/core/tx/tx_proxy/schemereq.cpp +++ b/ydb/core/tx/tx_proxy/schemereq.cpp @@ -44,7 +44,6 @@ struct TBaseSchemeReq: public TActorBootstrapped { struct TPathToResolve { const NKikimrSchemeOp::TModifyScheme& ModifyScheme; ui32 RequireAccess = NACLib::EAccessRights::NoAccess; - bool AllowedByLevel = true; // Params for NSchemeCache::TSchemeCacheNavigate::TEntry TVector Path; @@ -63,6 +62,7 @@ struct TBaseSchemeReq: public TActorBootstrapped { bool CheckDatabaseAdministrator = false; bool IsClusterAdministrator = false; bool IsDatabaseAdministrator = false; + NACLib::TSID DatabaseOwner; TBaseSchemeReq(const TTxProxyServices &services, ui64 txid, TAutoPtr request, const TIntrusivePtr &txProxyMon) : Services(services) @@ -1107,7 +1107,7 @@ struct TBaseSchemeReq: public TActorBootstrapped { // Check admin restrictions and special cases if (modifyScheme.GetOperationType() == NKikimrSchemeOp::ESchemeOpAlterLogin) { - // User management allowed to any user or (if configured so) to admins only + // User management is allowed to any user or (if configured so) to admins only if (checkAdmin && !isAdmin) { const auto errString = MakeAccessDeniedError(ctx, "attempt to manage user"); auto issue = MakeIssue(NKikimrIssues::TIssuesIds::ACCESS_DENIED, errString); @@ -1116,25 +1116,55 @@ struct TBaseSchemeReq: public TActorBootstrapped { } allowACLBypass = checkAdmin && isAdmin; + const auto& alterLogin = modifyScheme.GetAlterLogin(); + // Any user can change their own password (but nothing else) - auto isUserChangesOwnPassword = [](const auto& modifyScheme, const NACLib::TSID& subjectSid) { - const auto& alter = modifyScheme.GetAlterLogin(); - if (alter.GetAlterCase() == NKikimrSchemeOp::TAlterLogin::kModifyUser) { - const auto& targetUser = alter.GetModifyUser(); + auto isUserChangesOwnPassword = [](const auto& alterLogin, const NACLib::TSID& subjectSid) { + if (alterLogin.GetAlterCase() == NKikimrSchemeOp::TAlterLogin::kModifyUser) { + const auto& targetUser = alterLogin.GetModifyUser(); if (targetUser.HasPassword() && !targetUser.HasCanLogin()) { return (subjectSid == targetUser.GetUser()); } } return false; }; - allowACLBypass = allowACLBypass || isUserChangesOwnPassword(modifyScheme, UserToken->GetUserSID()); + allowACLBypass = allowACLBypass || isUserChangesOwnPassword(alterLogin, UserToken->GetUserSID()); + + // Database admin is not allowed to manage group of database admins (its the privilege of cluster admins). + if (IsDatabaseAdministrator) { + TString group; + switch (alterLogin.GetAlterCase()) { + case NKikimrSchemeOp::TAlterLogin::kAddGroupMembership: + group = alterLogin.GetAddGroupMembership().GetGroup(); + break; + case NKikimrSchemeOp::TAlterLogin::kRemoveGroupMembership: + group = alterLogin.GetRemoveGroupMembership().GetGroup(); + break; + case NKikimrSchemeOp::TAlterLogin::kRemoveGroup: + group = alterLogin.GetRemoveGroup().GetGroup(); + break; + case NKikimrSchemeOp::TAlterLogin::kRenameGroup: + group = alterLogin.GetRenameGroup().GetGroup(); + break; + default: + break; + } + if (!group.empty() && group == DatabaseOwner) { + const auto errString = MakeAccessDeniedError(ctx, entry.Path, TStringBuilder() + << "attempt to administer database admin group by the database admin" + ); + auto issue = MakeIssue(NKikimrIssues::TIssuesIds::ACCESS_DENIED, errString); + ReportStatus(TEvTxUserProxy::TEvProposeTransactionStatus::EStatus::AccessDenied, nullptr, &issue, ctx); + return false; + } + } } else if (modifyScheme.GetOperationType() == NKikimrSchemeOp::ESchemeOpModifyACL) { // Only the owner of the schema object (path) can transfer their ownership away. // Or admins (if configured so). const auto& newOwner = modifyScheme.GetModifyACL().GetNewOwner(); if (!newOwner.empty()) { - // That modifyACL is changing the owner + // This modifyACL is changing the owner auto isObjectOwner = [](const auto& userToken, const NACLib::TSID& owner) { return userToken->IsExist(owner); }; @@ -1150,6 +1180,18 @@ struct TBaseSchemeReq: public TActorBootstrapped { ReportStatus(TEvTxUserProxy::TEvProposeTransactionStatus::EStatus::AccessDenied, nullptr, &issue, ctx); return false; } + + // Database admin is not allowed to change ownership of its own database + if (IsDatabaseAdministrator && IsDB(entry)) { + const auto errString = MakeAccessDeniedError(ctx, entry.Path, TStringBuilder() + << "attempt to change database ownership by the database admin" + << " from " << owner + << " to " << newOwner + ); + auto issue = MakeIssue(NKikimrIssues::TIssuesIds::ACCESS_DENIED, errString); + ReportStatus(TEvTxUserProxy::TEvProposeTransactionStatus::EStatus::AccessDenied, nullptr, &issue, ctx); + return false; + } } // Admins can always change ACLs @@ -1305,6 +1347,7 @@ struct TBaseSchemeReq: public TActorBootstrapped { } const auto& database = request.ResultSet.front(); + DatabaseOwner = database.Self->Info.GetOwner(); IsDatabaseAdministrator = NKikimr::IsDatabaseAdministrator(&UserToken.value(), database.Self->Info.GetOwner()); LOG_DEBUG_S(ctx, NKikimrServices::TX_PROXY, "Actor# " << ctx.SelfID.ToString() << " txid# " << TxId @@ -1314,6 +1357,7 @@ struct TBaseSchemeReq: public TActorBootstrapped { << " CheckDatabaseAdministrator: " << CheckDatabaseAdministrator << " IsClusterAdministrator: " << IsClusterAdministrator << " IsDatabaseAdministrator: " << IsDatabaseAdministrator + << " DatabaseOwner: " << DatabaseOwner ); static_cast(this)->Start(ctx); -- cgit v1.3