security: database admin can not administer database admins - 2 (#16679)

Database admin is not allowed to make changes to other database admins.
Only cluster admins can.

Database admins:
- cannot change passwords or login blocking flags for other database admins
- cannot change their own login blocking flags
- can change their own passwords

Author: ijon

ydb/core/base/local_user_token.cpp

@@ -0,0 +1,22 @@
+#include <ydb/library/aclib/aclib.h>
+#include <ydb/library/login/login.h>
+#include <ydb/library/login/protos/login.pb.h>
+
+#include "local_user_token.h"
+
+namespace NKikimr {
+
+NACLib::TUserToken BuildLocalUserToken(const NLogin::TLoginProvider& loginProvider, const TString& user) {
+    const auto providerGroups = loginProvider.GetGroupsMembership(user);
+    const TVector<NACLib::TSID> groups(providerGroups.begin(), providerGroups.end());
+    //NOTE: TVector vs std::vector incompatibility between TUserToken and TLoginProvider
+    return NACLib::TUserToken(user, groups);
+}
+
+NACLib::TUserToken BuildLocalUserToken(const NLoginProto::TSecurityState& state, const TString& user) {
+    NLogin::TLoginProvider loginProvider;
+    loginProvider.UpdateSecurityState(state);
+    return BuildLocalUserToken(loginProvider, user);
+}
+
+}

ydb/core/base/local_user_token.h

@@ -0,0 +1,23 @@
+#pragma once
+
+#include <util/generic/string.h>
+#include <ydb/library/aclib/aclib.h>
+
+
+namespace NLoginProto {
+class TSecurityState;
+}
+namespace NLogin {
+class TLoginProvider;
+}
+
+namespace NKikimr {
+
+// Recreates user token from local user login/sid and it's database login provider or security state.
+// Token should be used to determine access level only (e.g. cluster/database admin status),
+// and not for authentication.
+// See methods in ydb/core/base/auth.h.
+NACLib::TUserToken BuildLocalUserToken(const NLogin::TLoginProvider& loginProvider, const TString& user);
+NACLib::TUserToken BuildLocalUserToken(const NLoginProto::TSecurityState& state, const TString& user);
+
+}

ydb/core/base/ya.make

@@ -30,6 +30,8 @@ SRCS(
     group_stat.h
     hive.h
     interconnect_channels.h
+    local_user_token.cpp
+    local_user_token.h
     localdb.cpp
     localdb.h
     location.h

ydb/core/tx/schemeshard/schemeshard__login.cpp

@@ -1,6 +1,7 @@
 #include <ydb/library/security/util.h>
 #include <ydb/core/protos/auth.pb.h>
 #include <ydb/core/base/auth.h>
+#include <ydb/core/base/local_user_token.h>
 
 #include "schemeshard_impl.h"
 
@@ -89,10 +90,7 @@ struct TSchemeShard::TTxLogin : TSchemeShard::TRwTxBase {
 private:
     bool IsAdmin() const {
         const auto& user = Request->Get()->Record.GetUser();
-        const auto providerGroups = Self->LoginProvider.GetGroupsMembership(user);
-        const TVector<NACLib::TSID> groups(providerGroups.begin(), providerGroups.end());
-        const auto userToken = NACLib::TUserToken(user, groups);
-
+        const auto userToken = NKikimr::BuildLocalUserToken(Self->LoginProvider, user);
         return IsAdministrator(AppData(), &userToken);
     }
 

ydb/core/tx/schemeshard/schemeshard__operation_alter_login.cpp

@@ -5,6 +5,8 @@
 
 #include <ydb/library/security/util.h>
 #include <ydb/core/base/auth.h>
+#include <ydb/core/base/local_user_token.h>
+
 
 #include <ydb/core/protos/auth.pb.h>
 
@@ -293,7 +295,7 @@ class TAlterLogin: public TSubOperationBase {
 
     NLogin::TLoginProvider::TBasicResponse CanRemoveSid(TOperationContext& context, const TString sid, const TString& sidType) {
         if (!AppData()->FeatureFlags.GetEnableStrictAclCheck()) {
-            return {}; 
+            return {};
         }
 
         auto subTree = context.SS->ListSubTree(context.SS->RootPathId(), context.Ctx);
@@ -316,9 +318,7 @@ class TAlterLogin: public TSubOperationBase {
     }
 
     void AddIsUserAdmin(const TString& user, NLogin::TLoginProvider& loginProvider, TParts& additionalParts) {
-        const auto providerGroups = loginProvider.GetGroupsMembership(user);
-        const TVector<NACLib::TSID> groups(providerGroups.begin(), providerGroups.end());
-        const auto userToken = NACLib::TUserToken(user, groups);
+        const auto userToken = NKikimr::BuildLocalUserToken(loginProvider, user);
 
         if (IsAdministrator(AppData(), &userToken)) {
             additionalParts.emplace_back("login_user_level", "admin");

ydb/core/tx/tx_proxy/schemereq.cpp

@@ -2,6 +2,7 @@
 
 #include <ydb/core/base/appdata.h>
 #include <ydb/core/base/auth.h>
+#include <ydb/core/base/local_user_token.h>
 #include <ydb/core/base/path.h>
 #include <ydb/core/base/tablet_pipe.h>
 #include <ydb/core/base/tx_processing.h>
@@ -10,6 +11,9 @@
 #include <ydb/core/protos/schemeshard/operations.pb.h>
 #include <ydb/core/tx/schemeshard/schemeshard.h>
 
+#include <ydb/library/login/login.h>
+#include <ydb/library/login/protos/login.pb.h>
+
 #include <ydb/library/aclib/aclib.h>
 #include <ydb/library/actors/core/hfunc.h>
 #include <ydb/library/protobuf_printer/security_printer.h>
@@ -63,6 +67,7 @@ struct TBaseSchemeReq: public TActorBootstrapped<TDerived> {
     bool IsClusterAdministrator = false;
     bool IsDatabaseAdministrator = false;
     NACLib::TSID DatabaseOwner;
+    NLoginProto::TSecurityState DatabaseSecurityState;
 
     TBaseSchemeReq(const TTxProxyServices &services, ui64 txid, TAutoPtr<TEvTxProxyReq::TEvSchemeRequest> request, const TIntrusivePtr<TTxProxyMon> &txProxyMon)
         : Services(services)
@@ -1141,7 +1146,8 @@ struct TBaseSchemeReq: public TActorBootstrapped<TDerived> {
 
                 allowACLBypass = (checkAdmin && isAdmin) || isUserChangesOwnPassword;
 
-                // Database admin is not allowed to manage group of database admins (its the privilege of cluster admins).
+                // Database admin is not allowed to manage group of database admins or change other database admins
+                // (its the privilege of cluster admins).
                 if (checkAdmin && IsDatabaseAdministrator) {
                     TString group;
                     switch (alterLogin.GetAlterCase()) {
@@ -1168,6 +1174,19 @@ struct TBaseSchemeReq: public TActorBootstrapped<TDerived> {
                         ReportStatus(TEvTxUserProxy::TEvProposeTransactionStatus::EStatus::AccessDenied, nullptr, &issue, ctx);
                         return false;
                     }
+                    // Database admin still can change its own password
+                    if (alterLogin.GetAlterCase() == NKikimrSchemeOp::TAlterLogin::kModifyUser && !isUserChangesOwnPassword) {
+                        const auto& targetUser = alterLogin.GetModifyUser();
+                        const auto targetUserToken = NKikimr::BuildLocalUserToken(DatabaseSecurityState, targetUser.GetUser());
+                        if (NKikimr::IsDatabaseAdministrator(&targetUserToken, DatabaseOwner)) {
+                            const auto errString = MakeAccessDeniedError(ctx, entry.Path, TStringBuilder()
+                                << "attempt to change other database admin 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) {
@@ -1357,9 +1376,12 @@ struct TBaseSchemeReq: public TActorBootstrapped<TDerived> {
             return Die(ctx);
         }
 
-        const auto& database = request.ResultSet.front();
-        DatabaseOwner = database.Self->Info.GetOwner();
-        IsDatabaseAdministrator = NKikimr::IsDatabaseAdministrator(&UserToken.value(), database.Self->Info.GetOwner());
+        const auto& entry = request.ResultSet.front();
+
+        DatabaseOwner = entry.Self->Info.GetOwner();
+        DatabaseSecurityState = entry.DomainDescription->Description.GetSecurityState();
+
+        IsDatabaseAdministrator = NKikimr::IsDatabaseAdministrator(&UserToken.value(), entry.Self->Info.GetOwner());
 
         LOG_DEBUG_S(ctx, NKikimrServices::TX_PROXY, "Actor# " << ctx.SelfID.ToString() << " txid# " << TxId
             << " HandleResolveDatabase,"

ydb/core/tx/tx_proxy/ya.make

@@ -51,6 +51,7 @@ PEERDIR(
     ydb/core/tx/tx_allocator
     ydb/core/tx/tx_allocator_client
     ydb/library/aclib
+    ydb/library/login
     ydb/library/mkql_proto/protos
     ydb/public/lib/base
 )

ydb/library/login/login.cpp

@@ -343,7 +343,7 @@ TLoginProvider::TRemoveGroupResponse TLoginProvider::RemoveGroup(const TString&
     return response;
 }
 
-std::vector<TString> TLoginProvider::GetGroupsMembership(const TString& member) {
+std::vector<TString> TLoginProvider::GetGroupsMembership(const TString& member) const {
     std::vector<TString> groups;
     std::unordered_set<TString> visited;
     std::deque<TString> queue;

ydb/library/login/login.h

@@ -225,7 +225,7 @@ class TLoginProvider {
         const TCacheSettings& cacheSettings);
     ~TLoginProvider();
 
-    std::vector<TString> GetGroupsMembership(const TString& member);
+    std::vector<TString> GetGroupsMembership(const TString& member) const;
     static TString GetTokenAudience(const TString& token);
     static std::chrono::system_clock::time_point GetTokenExpiresAt(const TString& token);
     static TString SanitizeJwtToken(const TString& token);

ydb/tests/functional/tenants/test_user_administration.py

@@ -96,8 +96,10 @@ def prepared_tenant_db(ydb_cluster, ydb_endpoint, ydb_database_module_scope):
 
             session.execute_scheme("create group ordinarygroup")
             session.execute_scheme("create user dbadmin2 password '1234'")
+            session.execute_scheme("create user dbadmin3 password '1234' nologin")
+            session.execute_scheme("create user dbadmin4 password '1234'")
             session.execute_scheme("create group dbsubadmins")
-            session.execute_scheme('alter group dbadmins add user dbadmin2, dbsubadmins')
+            session.execute_scheme('alter group dbadmins add user dbadmin2, dbadmin3, dbadmin4, dbsubadmins')
 
         # setup for database admins, second
         # make dbadmin the real admin of the database
@@ -114,20 +116,24 @@ def login_user(endpoint, database, user, password):
     return credentials._make_token_request()['access_token']
 
 
-def test_ordinaryuser_can_change_password_for_himself(ydb_endpoint, prepared_root_db, prepared_tenant_db, ydb_client):
+@pytest.mark.parametrize('subject_user', [
+    'ordinaryuser',
+    pytest.param('dbadmin4', id='dbadmin')
+])
+def test_user_can_change_password_for_himself(ydb_endpoint, prepared_root_db, prepared_tenant_db, ydb_client, subject_user):
     database_path = prepared_tenant_db
 
-    user_auth_token = login_user(ydb_endpoint, database_path, 'ordinaryuser', '1234')
+    user_auth_token = login_user(ydb_endpoint, database_path, subject_user, '1234')
     credentials = ydb.AuthTokenCredentials(user_auth_token)
 
     with ydb_client(database_path, credentials=credentials) as driver:
         driver.wait()
 
         pool = ydb.SessionPool(driver)
         with pool.checkout() as session:
-            session.execute_scheme("alter user ordinaryuser password '4321'")
+            session.execute_scheme(f"alter user {subject_user} password '4321'")
 
-    user_auth_token = login_user(ydb_endpoint, database_path, 'ordinaryuser', '4321')
+    user_auth_token = login_user(ydb_endpoint, database_path, subject_user, '4321')
 
 
 def test_database_admin_cant_change_database_owner(ydb_endpoint, prepared_root_db, prepared_tenant_db, ydb_client):
@@ -154,7 +160,6 @@ def test_database_admin_cant_change_database_owner(ydb_endpoint, prepared_root_d
     pytest.param('alter group dbadmins drop user dbsubadmins', id='remove-subgroup'),
     pytest.param('drop group dbadmins', id='remove-admin-group'),
     pytest.param('alter group dbadmins rename to dbadminsdemoted', id='rename-admin-group'),
-
 ])
 def test_database_admin_cant_change_database_admin_group(ydb_endpoint, prepared_root_db, prepared_tenant_db, ydb_client, query):
     database_path = prepared_tenant_db
@@ -174,6 +179,30 @@ def test_database_admin_cant_change_database_admin_group(ydb_endpoint, prepared_
             assert 'Access denied.' in exc_info.value.message
 
 
+@pytest.mark.parametrize('query', [
+    pytest.param('alter user dbadmin2 password "4321"', id='change-password'),
+    pytest.param('alter user dbadmin2 nologin', id='block'),
+    pytest.param('alter user dbadmin3 login', id='unblock'),
+])
+def test_database_admin_cant_change_database_admin_user(ydb_endpoint, prepared_root_db, prepared_tenant_db, ydb_client, query):
+    database_path = prepared_tenant_db
+
+    user_auth_token = login_user(ydb_endpoint, database_path, 'dbadmin', '1234')
+    credentials = ydb.AuthTokenCredentials(user_auth_token)
+
+    with ydb_client(database_path, credentials=credentials) as driver:
+        driver.wait()
+
+        pool = ydb.SessionPool(driver)
+        with pool.checkout() as session:
+            with pytest.raises(ydb.issues.Error) as exc_info:
+                session.execute_scheme(query)
+
+        assert exc_info.type is ydb.issues.Unauthorized
+        logger.debug(exc_info.value.message)
+        assert 'Access denied.' in exc_info.value.message
+
+
 def test_database_admin_can_create_user(ydb_endpoint, prepared_root_db, prepared_tenant_db, ydb_client):
     database_path = prepared_tenant_db