Skip to content

Commit 5b416ec

Browse files
ijonijon
authored
25-1: security: user administration bundle (#16234)
Improvements for `enable_strict_user_management`+`enable_database_admin`+`domain_login_only` mode: - fix corner case of "any user must be able to change password for themself" - add more tests on database admin actions in tenant database
1 parent a3aa016 commit 5b416ec

File tree

3 files changed

+214
-14
lines changed

3 files changed

+214
-14
lines changed

‎ydb/core/tx/tx_proxy/schemereq.cpp

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1107,31 +1107,42 @@ struct TBaseSchemeReq: public TActorBootstrapped<TDerived> {
11071107

11081108
// Check admin restrictions and special cases
11091109
if (modifyScheme.GetOperationType() == NKikimrSchemeOp::ESchemeOpAlterLogin) {
1110-
// User management is allowed to any user or (if configured so) to admins only
1111-
if (checkAdmin && !isAdmin) {
1112-
const auto errString = MakeAccessDeniedError(ctx, "attempt to manage user");
1113-
auto issue = MakeIssue(NKikimrIssues::TIssuesIds::ACCESS_DENIED, errString);
1114-
ReportStatus(TEvTxUserProxy::TEvProposeTransactionStatus::EStatus::AccessDenied, nullptr, &issue, ctx);
1115-
return false;
1116-
}
1117-
allowACLBypass = checkAdmin && isAdmin;
1118-
1110+
// EnableStrictUserManagement == false:
1111+
// - any user can manage users|groups, but require AlterSchema right
1112+
//
1113+
// EnableStrictUserManagement == true:
1114+
// - user can change password for himself, and does not require AlterSchema right
1115+
// - only admins can manage users|groups, and does not require AlterSchema right
1116+
// - database admin can't change other database admins
1117+
// - database admin can't change database admin group
1118+
// - database admin can't change database owner
1119+
//
11191120
const auto& alterLogin = modifyScheme.GetAlterLogin();
11201121

1121-
// Any user can change their own password (but nothing else)
1122-
auto isUserChangesOwnPassword = [](const auto& alterLogin, const NACLib::TSID& subjectSid) {
1122+
// User changes password for himself (and only password)
1123+
bool isUserChangesOwnPassword = [](const auto& alterLogin, const NACLib::TSID& subjectSid) {
11231124
if (alterLogin.GetAlterCase() == NKikimrSchemeOp::TAlterLogin::kModifyUser) {
11241125
const auto& targetUser = alterLogin.GetModifyUser();
11251126
if (targetUser.HasPassword() && !targetUser.HasCanLogin()) {
11261127
return (subjectSid == targetUser.GetUser());
11271128
}
11281129
}
11291130
return false;
1130-
};
1131-
allowACLBypass = allowACLBypass || isUserChangesOwnPassword(alterLogin, UserToken->GetUserSID());
1131+
}(alterLogin, UserToken->GetUserSID());
1132+
1133+
bool allowManageUser = !checkAdmin || isAdmin || isUserChangesOwnPassword;
1134+
1135+
if (!allowManageUser) {
1136+
const auto errString = MakeAccessDeniedError(ctx, "attempt to manage user");
1137+
auto issue = MakeIssue(NKikimrIssues::TIssuesIds::ACCESS_DENIED, errString);
1138+
ReportStatus(TEvTxUserProxy::TEvProposeTransactionStatus::EStatus::AccessDenied, nullptr, &issue, ctx);
1139+
return false;
1140+
}
1141+
1142+
allowACLBypass = (checkAdmin && isAdmin) || isUserChangesOwnPassword;
11321143

11331144
// Database admin is not allowed to manage group of database admins (its the privilege of cluster admins).
1134-
if (IsDatabaseAdministrator) {
1145+
if (checkAdmin && IsDatabaseAdministrator) {
11351146
TString group;
11361147
switch (alterLogin.GetAlterCase()) {
11371148
case NKikimrSchemeOp::TAlterLogin::kAddGroupMembership:

‎ydb/tests/functional/tenants/test_user_administration.py

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
# -*- coding: utf-8 -*-
2+
import copy
3+
import logging
4+
5+
import pytest
6+
7+
from ydb.tests.library.harness.kikimr_config import KikimrConfigGenerator
8+
from ydb.tests.library.harness.util import LogLevels
9+
from ydb.tests.oss.ydb_sdk_import import ydb
10+
11+
logger = logging.getLogger(__name__)
12+
13+
14+
# local configuration for the ydb cluster (fetched by ydb_cluster_configuration fixture)
15+
CLUSTER_CONFIG = dict(
16+
additional_log_configs={
17+
# more logs
18+
'TX_PROXY': LogLevels.DEBUG,
19+
# less logs
20+
'FLAT_TX_SCHEMESHARD': LogLevels.INFO,
21+
'KQP_PROXY': LogLevels.CRIT,
22+
'KQP_WORKER': LogLevels.CRIT,
23+
'KQP_GATEWAY': LogLevels.CRIT,
24+
'GRPC_PROXY': LogLevels.ERROR,
25+
'TX_DATASHARD': LogLevels.ERROR,
26+
'TX_PROXY_SCHEME_CACHE': LogLevels.ERROR,
27+
'GRPC_SERVER': LogLevels.CRIT,
28+
'KQP_YQL': LogLevels.ERROR,
29+
'KQP_SESSION': LogLevels.CRIT,
30+
'KQP_COMPILE_ACTOR': LogLevels.CRIT,
31+
'PERSQUEUE_CLUSTER_TRACKER': LogLevels.CRIT,
32+
'SCHEME_BOARD_SUBSCRIBER': LogLevels.CRIT,
33+
'SCHEME_BOARD_POPULATOR': LogLevels.CRIT,
34+
},
35+
extra_feature_flags=[
36+
'enable_strict_acl_check',
37+
'enable_strict_user_management',
38+
'enable_database_admin',
39+
],
40+
# do not clutter logs with resource pools auto creation
41+
enable_resource_pools=False,
42+
enforce_user_token_requirement=True,
43+
# make all bootstrap and setup under this user
44+
default_clusteradmin='root@builtin',
45+
)
46+
47+
48+
# ydb_fixtures.ydb_cluster_configuration local override
49+
@pytest.fixture(scope='module')
50+
def ydb_cluster_configuration():
51+
conf = copy.deepcopy(CLUSTER_CONFIG)
52+
return conf
53+
54+
55+
@pytest.fixture(scope='module')
56+
def ydb_configurator(ydb_cluster_configuration):
57+
config_generator = KikimrConfigGenerator(**ydb_cluster_configuration)
58+
config_generator.yaml_config['auth_config'] = {
59+
'domain_login_only': False,
60+
}
61+
config_generator.yaml_config['domains_config']['disable_builtin_security'] = True
62+
security_config = config_generator.yaml_config['domains_config']['security_config']
63+
security_config['administration_allowed_sids'].append('clusteradmin')
64+
return config_generator
65+
66+
67+
@pytest.fixture(scope='module')
68+
def prepared_root_db(ydb_cluster, ydb_root, ydb_endpoint):
69+
cluster_admin = ydb.AuthTokenCredentials(ydb_cluster.config.default_clusteradmin)
70+
71+
# prepare root database
72+
driver_config = ydb.DriverConfig(ydb_endpoint, ydb_root, credentials=cluster_admin)
73+
with ydb.Driver(driver_config) as driver:
74+
pool = ydb.SessionPool(driver)
75+
with pool.checkout() as session:
76+
session.execute_scheme("create user clusteradmin password '1234'")
77+
78+
79+
@pytest.fixture(scope='module')
80+
def prepared_tenant_db(ydb_cluster, ydb_endpoint, ydb_database_module_scope):
81+
cluster_admin = ydb.AuthTokenCredentials(ydb_cluster.config.default_clusteradmin)
82+
83+
# prepare tenant database
84+
database_path = ydb_database_module_scope
85+
driver_config = ydb.DriverConfig(ydb_endpoint, database_path, credentials=cluster_admin)
86+
with ydb.Driver(driver_config) as driver:
87+
pool = ydb.SessionPool(driver)
88+
with pool.checkout() as session:
89+
# setup for database admins, first
90+
session.execute_scheme("create user dbadmin password '1234'")
91+
session.execute_scheme('create group dbadmins')
92+
session.execute_scheme('alter group dbadmins add user dbadmin')
93+
94+
# additional setup for individual tests
95+
session.execute_scheme("create user ordinaryuser password '1234'")
96+
97+
session.execute_scheme("create group ordinarygroup")
98+
session.execute_scheme("create user dbadmin2 password '1234'")
99+
session.execute_scheme("create group dbsubadmins")
100+
session.execute_scheme('alter group dbadmins add user dbadmin2, dbsubadmins')
101+
102+
# setup for database admins, second
103+
# make dbadmin the real admin of the database
104+
driver.scheme_client.modify_permissions(database_path, ydb.ModifyPermissionsSettings().change_owner('dbadmins'))
105+
106+
return database_path
107+
108+
109+
# python sdk does not legally expose login method, so that is a crude way
110+
# to get auth-token in exchange to credentials
111+
def login_user(endpoint, database, user, password):
112+
driver_config = ydb.DriverConfig(endpoint, database)
113+
credentials = ydb.StaticCredentials(driver_config, user, password)
114+
return credentials._make_token_request()['access_token']
115+
116+
117+
def test_ordinaryuser_can_change_password_for_himself(ydb_endpoint, prepared_root_db, prepared_tenant_db, ydb_client):
118+
database_path = prepared_tenant_db
119+
120+
user_auth_token = login_user(ydb_endpoint, database_path, 'ordinaryuser', '1234')
121+
credentials = ydb.AuthTokenCredentials(user_auth_token)
122+
123+
with ydb_client(database_path, credentials=credentials) as driver:
124+
driver.wait()
125+
126+
pool = ydb.SessionPool(driver)
127+
with pool.checkout() as session:
128+
session.execute_scheme("alter user ordinaryuser password '4321'")
129+
130+
user_auth_token = login_user(ydb_endpoint, database_path, 'ordinaryuser', '4321')
131+
132+
133+
def test_database_admin_cant_change_database_owner(ydb_endpoint, prepared_root_db, prepared_tenant_db, ydb_client):
134+
database_path = prepared_tenant_db
135+
136+
user_auth_token = login_user(ydb_endpoint, database_path, 'dbadmin', '1234')
137+
credentials = ydb.AuthTokenCredentials(user_auth_token)
138+
139+
with ydb_client(database_path, credentials=credentials) as driver:
140+
driver.wait()
141+
142+
with pytest.raises(ydb.issues.Error) as exc_info:
143+
driver.scheme_client.modify_permissions(database_path, ydb.ModifyPermissionsSettings().change_owner('ordinaryuser'))
144+
145+
assert exc_info.type is ydb.issues.Unauthorized
146+
assert 'Access denied for dbadmin' in exc_info.value.message
147+
148+
149+
@pytest.mark.parametrize('query', [
150+
pytest.param('alter group dbadmins add user ordinaryuser', id='add-user'),
151+
pytest.param('alter group dbadmins drop user dbadmin', id='remove-himself'),
152+
pytest.param('alter group dbadmins drop user dbadmin2', id='remove-other-admin'),
153+
pytest.param('alter group dbadmins add user ordinarygroup', id='add-subgroup'),
154+
pytest.param('alter group dbadmins drop user dbsubadmins', id='remove-subgroup'),
155+
pytest.param('drop group dbadmins', id='remove-admin-group'),
156+
pytest.param('alter group dbadmins rename to dbadminsdemoted', id='rename-admin-group'),
157+
158+
])
159+
def test_database_admin_cant_change_database_admin_group(ydb_endpoint, prepared_root_db, prepared_tenant_db, ydb_client, query):
160+
database_path = prepared_tenant_db
161+
162+
user_auth_token = login_user(ydb_endpoint, database_path, 'dbadmin', '1234')
163+
credentials = ydb.AuthTokenCredentials(user_auth_token)
164+
165+
with ydb_client(database_path, credentials=credentials) as driver:
166+
driver.wait()
167+
168+
pool = ydb.SessionPool(driver)
169+
with pool.checkout() as session:
170+
with pytest.raises(ydb.issues.Error) as exc_info:
171+
session.execute_scheme(query)
172+
173+
assert exc_info.type is ydb.issues.Unauthorized
174+
assert 'Access denied.' in exc_info.value.message
175+
176+
177+
def test_database_admin_can_create_user(ydb_endpoint, prepared_root_db, prepared_tenant_db, ydb_client):
178+
database_path = prepared_tenant_db
179+
180+
user_auth_token = login_user(ydb_endpoint, database_path, 'dbadmin', '1234')
181+
credentials = ydb.AuthTokenCredentials(user_auth_token)
182+
183+
with ydb_client(database_path, credentials=credentials) as driver:
184+
driver.wait()
185+
186+
pool = ydb.SessionPool(driver)
187+
with pool.checkout() as session:
188+
session.execute_scheme("create user testuser password '1234'")

‎ydb/tests/functional/tenants/ya.make

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ TEST_SRCS(
1313
test_system_views.py
1414
test_auth_system_views.py
1515
test_publish_into_schemeboard_with_common_ssring.py
16+
test_user_administration.py
1617
test_users_groups_with_acl.py
1718
)
1819

0 commit comments

Comments
 (0)