Skip to content

Commit 795aa32

Browse files
ijonijon
authored
security: make domain_login_only+enable_strict_* mode work (#14557)
Make `domain_login_only`+`enable_strict_acl_check`+`enable_strict_user_management` work. - grpc-proxy: fix bug that require admin to have explicit `ydb.database.connect` permission on a database to be able to work with it (while original intention was that admins should bypass database connect check) - ticket-parser: allow users from the root database to authenticate in a tenant database - allow cluster admin to set the owner of a database (e.g. database admin) at the root schemeshard, without checking target sid for existence, effectively bypassing `enable_strict_acl_check` restriction in that particular case
1 parent bbb7d71 commit 795aa32

File tree

11 files changed

+121
-41
lines changed

11 files changed

+121
-41
lines changed

‎ydb/core/grpc_services/grpc_request_check_actor.h

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -520,15 +520,6 @@ class TGrpcRequestCheckActor
520520
return {false, std::nullopt};
521521
}
522522

523-
if (TBase::IsUserAdmin()) {
524-
LOG_DEBUG_S(*TlsActivationContext, NKikimrServices::GRPC_PROXY_NO_CONNECT_ACCESS,
525-
"Skip check permission connect db, user is a admin"
526-
<< ", database: " << CheckedDatabaseName_
527-
<< ", user: " << TBase::GetUserSID()
528-
<< ", from ip: " << GrpcRequestBaseCtx_->GetPeerName());
529-
return {false, std::nullopt};
530-
}
531-
532523
if (!TBase::GetSecurityToken()) {
533524
if (!TBase::IsTokenRequired()) {
534525
LOG_DEBUG_S(*TlsActivationContext, NKikimrServices::GRPC_PROXY_NO_CONNECT_ACCESS,
@@ -549,8 +540,23 @@ class TGrpcRequestCheckActor
549540
return {false, std::nullopt};
550541
}
551542

552-
const ui32 access = NACLib::ConnectDatabase;
553543
const auto& parsedToken = TBase::GetParsedToken();
544+
const auto& databaseOwner = SecurityObject_->GetOwnerSID();
545+
546+
// admins can connect to databases without having connect rights:
547+
// - cluster admin -- to any database
548+
// - database admin -- to their database
549+
const bool isAdmin = TBase::IsUserAdmin() || (parsedToken && IsDatabaseAdministrator(parsedToken.Get(), databaseOwner));
550+
if (isAdmin) {
551+
LOG_DEBUG_S(*TlsActivationContext, NKikimrServices::GRPC_PROXY_NO_CONNECT_ACCESS,
552+
"Skip check permission connect db, user is a admin"
553+
<< ", database: " << CheckedDatabaseName_
554+
<< ", user: " << TBase::GetUserSID()
555+
<< ", from ip: " << GrpcRequestBaseCtx_->GetPeerName());
556+
return {false, std::nullopt};
557+
}
558+
559+
const ui32 access = NACLib::ConnectDatabase;
554560
if (parsedToken && SecurityObject_->CheckAccess(access, *parsedToken)) {
555561
return {false, std::nullopt};
556562
}
@@ -562,7 +568,7 @@ class TGrpcRequestCheckActor
562568
}
563569

564570
const TString error = "No permission to connect to the database";
565-
LOG_INFO_S(TlsActivationContext->AsActorContext(), NKikimrServices::GRPC_SERVER,
571+
LOG_INFO_S(TlsActivationContext->AsActorContext(), NKikimrServices::GRPC_SERVER,
566572
error
567573
<< ": " << CheckedDatabaseName_
568574
<< ", user: " << TBase::GetUserSID()

‎ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2952,7 +2952,7 @@ Y_UNIT_TEST_SUITE(KqpScheme) {
29522952
auto result = userSession.AlterTable("/Root/SecondaryKeys/Index/indexImplTable", tableSettings).ExtractValueSync();
29532953
UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::UNAUTHORIZED, result.GetIssues().ToString());
29542954
UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToString(),
2955-
"Error: Access denied for user@builtin on path Root/SecondaryKeys/Index/indexImplTable"
2955+
"Error: Access denied for user@builtin on path /Root/SecondaryKeys/Index/indexImplTable"
29562956
);
29572957
}
29582958
// grant necessary permission

‎ydb/core/security/secure_request.h

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,9 @@ class TSecureRequestActor : public TBase {
4444
return static_cast<TDerived*>(this)->OnAccessDenied(result.Error, ctx);
4545
}
4646
} else {
47-
if (RequireAdminAccess) {
48-
UserAdmin = IsTokenAllowed(result.Token.Get(), GetAdministrationAllowedSIDs());
49-
if (!UserAdmin) {
50-
return static_cast<TDerived*>(this)->OnAccessDenied(TEvTicketParser::TError{.Message = "Administrative access denied", .Retryable = false}, ctx);
51-
52-
}
47+
UserAdmin = IsTokenAllowed(result.Token.Get(), GetAdministrationAllowedSIDs());
48+
if (RequireAdminAccess && !UserAdmin) {
49+
return static_cast<TDerived*>(this)->OnAccessDenied(TEvTicketParser::TError{.Message = "Administrative access denied", .Retryable = false}, ctx);
5350
}
5451
}
5552
AuthorizeTicketResult = ev.Get()->Release();

‎ydb/core/security/ticket_parser_impl.h

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@
2828
#include <util/stream/file.h>
2929
#include <util/string/vector.h>
3030

31+
#include <util/string/join.h>
32+
3133
namespace NKikimr {
3234

3335
inline bool IsRetryableGrpcError(const NYdbGrpc::TGrpcStatus& status) {
@@ -722,10 +724,43 @@ class TTicketParserImpl : public TActorBootstrapped<TDerived> {
722724
return true;
723725
}
724726

727+
template <typename TTokenRecord>
728+
const TVector<TString> GetLookupDatabases(const TTokenRecord& record) {
729+
TVector<TString> result;
730+
result.push_back(DomainName);
731+
if (!Config.GetDomainLoginOnly() && !record.Database.empty() && record.Database != DomainName) {
732+
result.push_back(record.Database);
733+
}
734+
std::reverse(result.begin(), result.end());
735+
return result;
736+
}
737+
725738
template <typename TTokenRecord>
726739
bool CanInitLoginToken(const TString& key, TTokenRecord& record) {
727740
if (UseLoginProvider && (record.TokenType == TDerived::ETokenType::Unknown || record.TokenType == TDerived::ETokenType::Login)) {
728-
TString database = (Config.GetDomainLoginOnly() || record.Database.empty()) ? DomainName : record.Database;
741+
// Lookup the token in the login provider for the target database, with the possible fallback to the root (domain) database.
742+
//
743+
// Target database could be unspecified (some anonymous or backward-compatible mode).
744+
// Target database may be the same as the root database.
745+
//
746+
// In a special case, when DomainLoginOnly = false and a user from the root database attempts to
747+
// access a tenant database, target database must be selected between the two candidates: tenant and the root,
748+
// based on the database (or audience) embedded in the token itself.
749+
auto database = NLogin::TLoginProvider::GetTokenAudience(record.Ticket);
750+
BLOG_TRACE("CanInitLoginToken, domain db " << DomainName << ", request db " << record.Database
751+
<< ", token db " << database << ", DomainLoginOnly " << Config.GetDomainLoginOnly()
752+
);
753+
if (database.empty()) {
754+
database = DomainName;
755+
}
756+
const auto& lookupDatabases = GetLookupDatabases(record);
757+
BLOG_TRACE("CanInitLoginToken, target database candidates(" << lookupDatabases.size() << "): " << JoinSeq(", ", lookupDatabases));
758+
if (std::find(lookupDatabases.begin(), lookupDatabases.end(), database) == lookupDatabases.end()) {
759+
SetError(key, record, {.Message = "Wrong audience"});
760+
CounterTicketsLogin->Inc();
761+
BLOG_TRACE("CanInitLoginToken, A1 error Wrong audience");
762+
return true;
763+
}
729764
auto itLoginProvider = LoginProviders.find(database);
730765
if (itLoginProvider != LoginProviders.end()) {
731766
NLogin::TLoginProvider& loginProvider(itLoginProvider->second);
@@ -735,8 +770,10 @@ class TTicketParserImpl : public TActorBootstrapped<TDerived> {
735770
record.TokenType = TDerived::ETokenType::Login;
736771
SetError(key, record, {.Message = response.Error, .Retryable = response.ErrorRetryable});
737772
CounterTicketsLogin->Inc();
773+
BLOG_TRACE("CanInitLoginToken, database " << database << ", A2 error " << response.Error);
738774
return true;
739775
}
776+
BLOG_TRACE("CanInitLoginToken, database " << database << ", A3 error");
740777
} else {
741778
record.TokenType = TDerived::ETokenType::Login;
742779
record.ExpireTime = ToInstant(response.ExpiresAt);
@@ -760,14 +797,17 @@ class TTicketParserImpl : public TActorBootstrapped<TDerived> {
760797
.GroupSIDs = groups,
761798
.AuthType = record.GetAuthType()
762799
}));
800+
BLOG_TRACE("CanInitLoginToken, database " << database << ", A4 success");
763801
return true;
764802
}
765803
} else {
766804
if (record.TokenType == TDerived::ETokenType::Login) {
767805
SetError(key, record, {.Message = "Login state is not available yet", .Retryable = false});
768806
CounterTicketsLogin->Inc();
807+
BLOG_TRACE("CanInitLoginToken, database " << database << ", A5 error");
769808
return true;
770809
}
810+
BLOG_TRACE("CanInitLoginToken, database " << database << ", A6 error");
771811
}
772812
}
773813
return false;
@@ -1870,7 +1910,14 @@ class TTicketParserImpl : public TActorBootstrapped<TDerived> {
18701910
if (record.IsExternalAuthEnabled()) {
18711911
return RefreshTicketViaExternalAuthProvider(key, record);
18721912
}
1873-
const TString& database = (Config.GetDomainLoginOnly() || record.Database.empty()) ? DomainName : record.Database;
1913+
auto database = NLogin::TLoginProvider::GetTokenAudience(record.Ticket);
1914+
if (database.empty()) {
1915+
database = DomainName;
1916+
}
1917+
const auto& lookupDatabases = GetLookupDatabases(record);
1918+
if (std::find(lookupDatabases.begin(), lookupDatabases.end(), database) == lookupDatabases.end()) {
1919+
return false;
1920+
}
18741921
auto itLoginProvider = LoginProviders.find(database);
18751922
if (itLoginProvider == LoginProviders.end()) {
18761923
return false;

‎ydb/core/tx/schemeshard/schemeshard__operation_modify_acl.cpp

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
#include "schemeshard__operation_part.h"
22
#include "schemeshard_impl.h"
33

4+
#include <ydb/core/base/auth.h>
5+
46
namespace {
57

68
using namespace NKikimr;
@@ -57,20 +59,33 @@ class TModifyACL: public TSubOperationBase {
5759
return result;
5860
}
5961

62+
bool isAdmin = (context.UserToken && IsAdministrator(AppData(), context.UserToken.Get()));
63+
6064
if (acl && AppData()->FeatureFlags.GetEnableStrictAclCheck()) {
6165
NACLib::TDiffACL diffACL(acl);
6266
for (const NACLibProto::TDiffACE& diffACE : diffACL.GetDiffACE()) {
6367
if (static_cast<NACLib::EDiffType>(diffACE.GetDiffType()) == NACLib::EDiffType::Add) {
64-
if (!CheckSidExistsOrIsNonYdb(context.SS->LoginProvider.Sids, diffACE.GetACE().GetSID())) {
68+
// add diff type is allowed if:
69+
// - subject is a cluster administrator
70+
// - or target sid is an external one (not a ydb-local)
71+
// - or target sid is a local one and exist in this database
72+
const auto& targetSid = diffACE.GetACE().GetSID();
73+
bool allowed = (isAdmin || CheckSidExistsOrIsNonYdb(context.SS->LoginProvider.Sids, targetSid));
74+
if (!allowed) {
6575
result->SetError(NKikimrScheme::StatusPreconditionFailed,
66-
TStringBuilder() << "SID " << diffACE.GetACE().GetSID() << " not found in database `" << databaseName << "`");
76+
TStringBuilder() << "SID " << targetSid << " not found in database `" << databaseName << "`");
6777
return result;
6878
}
6979
} // remove diff type is allowed in any case
7080
}
7181
}
7282
if (owner && AppData()->FeatureFlags.GetEnableStrictAclCheck()) {
73-
if (!CheckSidExistsOrIsNonYdb(context.SS->LoginProvider.Sids, owner)) {
83+
// ownership transfer is allowed if:
84+
// - subject is a cluster administrator
85+
// - or target sid is an external one (not a ydb-local)
86+
// - or target sid is a local one and exist in this database
87+
bool allowed = (isAdmin || CheckSidExistsOrIsNonYdb(context.SS->LoginProvider.Sids, owner));
88+
if (!allowed) {
7489
result->SetError(NKikimrScheme::StatusPreconditionFailed,
7590
TStringBuilder() << "Owner SID " << owner << " not found in database `" << databaseName << "`");
7691
return result;

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1001,7 +1001,7 @@ struct TBaseSchemeReq: public TActorBootstrapped<TDerived> {
10011001

10021002
if (resolveTask.ModifyScheme.GetOperationType() == NKikimrSchemeOp::ESchemeOpAlterUserAttributes) {
10031003
// ESchemeOpAlterUserAttributes applies on GSS when path is DB
1004-
// but on GSS in other cases
1004+
// but on TSS in other cases
10051005
if (IsDB(resolveResult)) {
10061006
return resolveResult.DomainInfo->DomainKey.OwnerId;
10071007
} else {
@@ -1018,7 +1018,7 @@ struct TBaseSchemeReq: public TActorBootstrapped<TDerived> {
10181018

10191019
TString MakeAccessDeniedError(const TActorContext& ctx, const TVector<TString>& path, const TString& part) {
10201020
const TString msg = TStringBuilder() << "Access denied for " << GetUserSID(UserToken)
1021-
<< " on path " << JoinPath(path)
1021+
<< " on path " << CanonizePath(JoinPath(path))
10221022
;
10231023
LOG_ERROR_S(ctx, NKikimrServices::TX_PROXY, "Actor# " << ctx.SelfID.ToString() << " txid# " << TxId
10241024
<< ", " << msg << ", " << part
@@ -1151,6 +1151,9 @@ struct TBaseSchemeReq: public TActorBootstrapped<TDerived> {
11511151
return false;
11521152
}
11531153
}
1154+
1155+
// Admins can always change ACLs
1156+
allowACLBypass = isAdmin;
11541157
}
11551158

11561159
ui32 access = requestIt->RequireAccess;
@@ -1520,7 +1523,7 @@ void TFlatSchemeReq::HandleWorkingDir(TEvTxProxySchemeCache::TEvNavigateKeySetRe
15201523
if (!workingDir || workingDir->size() >= parts.size()) {
15211524
const TString errText = TStringBuilder()
15221525
<< "Cannot resolve working dir"
1523-
<< " workingDir# " << (workingDir ? JoinPath(*workingDir) : "null")
1526+
<< " workingDir# " << (workingDir ? CanonizePath(JoinPath(*workingDir)) : "null")
15241527
<< " path# " << JoinPath(parts);
15251528
LOG_ERROR_S(ctx, NKikimrServices::TX_PROXY, "Actor# " << ctx.SelfID.ToString() << " txid# " << TxId
15261529
<< ", " << errText

‎ydb/library/login/login.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -385,7 +385,7 @@ bool TLoginProvider::ShouldUnlockAccount(const TSidRecord& sid) const {
385385

386386
bool TLoginProvider::IsLockedOut(const TSidRecord& user) const {
387387
Y_ABORT_UNLESS(user.Type == NLoginProto::ESidType::USER);
388-
388+
389389
if (AccountLockout.AttemptThreshold == 0) {
390390
return false;
391391
}
@@ -533,6 +533,7 @@ TLoginProvider::TValidateTokenResponse TLoginProvider::ValidateToken(const TVali
533533
// we check audience manually because we want an explicit error instead of wrong key id in case of databases mismatch
534534
auto audience = decoded_token.get_audience();
535535
if (audience.empty() || TString(*audience.begin()) != Audience) {
536+
response.WrongAudience = true;
536537
response.Error = "Wrong audience";
537538
return response;
538539
}

‎ydb/library/login/login.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ class TLoginProvider {
8686
struct TValidateTokenResponse : TBasicResponse {
8787
bool TokenUnrecognized = false;
8888
bool ErrorRetryable = false;
89+
bool WrongAudience = false;
8990
TString User;
9091
std::optional<std::vector<TString>> Groups;
9192
std::chrono::system_clock::time_point ExpiresAt;

‎ydb/services/ydb/ydb_login_ut.cpp

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ class TLoginClientConnection {
4848

4949
void CreateUser(TString user, TString password) {
5050
TClient client(*(Server.ServerSettings));
51-
client.SetSecurityToken("root@builtin");
51+
client.SetSecurityToken(RootToken);
5252
auto status = client.CreateUser("/Root", user, password);
5353
UNIT_ASSERT_VALUES_EQUAL(status, NMsgBusProxy::MSTATUS_OK);
5454
}
@@ -60,22 +60,22 @@ class TLoginClientConnection {
6060
else
6161
acl.RemoveAccess(NACLib::EAccessType::Allow, access, user);
6262
TClient client(*(Server.ServerSettings));
63-
client.SetSecurityToken("root@builtin");
63+
client.SetSecurityToken(RootToken);
6464
client.ModifyACL("", "Root", acl.SerializeAsString());
6565
}
6666

6767
void TestConnectRight(TString token, TString expectedErrorReason) {
6868
const TString sql = "SELECT 1;";
6969
const auto result = ExecuteSql(token, sql);
70-
70+
7171
if (expectedErrorReason.empty()) {
7272
UNIT_ASSERT_C(result.GetStatus() == NYdb::EStatus::SUCCESS, result.GetIssues().ToString());
7373
} else {
7474
UNIT_ASSERT_C(result.GetStatus() != NYdb::EStatus::SUCCESS, result.GetIssues().ToString());
7575
UNIT_ASSERT_STRING_CONTAINS_C(result.GetIssues().ToString(), expectedErrorReason, result.GetIssues().ToString());
7676
}
7777
}
78-
78+
7979
void TestDescribeRight(TString token, TString expectedErrorReason) {
8080
TClient client(*(Server.ServerSettings));
8181
client.SetSecurityToken(token);
@@ -111,6 +111,7 @@ class TLoginClientConnection {
111111
authConfig->SetUseLoginProvider(true);
112112
authConfig->SetEnableLoginAuthentication(isLoginAuthenticationEnabled);
113113
appConfig.MutableDomainsConfig()->MutableSecurityConfig()->SetEnforceUserTokenRequirement(true);
114+
appConfig.MutableDomainsConfig()->MutableSecurityConfig()->AddAdministrationAllowedSIDs(RootToken);
114115
appConfig.MutableFeatureFlags()->SetCheckDatabaseAccessPermission(true);
115116
appConfig.MutableFeatureFlags()->SetAllowYdbRequestsWithoutDatabase(false);
116117

@@ -124,6 +125,7 @@ class TLoginClientConnection {
124125
}
125126

126127
private:
128+
TString RootToken = "root@builtin";
127129
TKikimrWithGrpcAndRootSchemaWithAuth Server;
128130
NYdb::TDriver Connection;
129131
NConsoleClient::TDummyClient Client;
@@ -176,7 +178,7 @@ Y_UNIT_TEST_SUITE(TGRpcAuthentication) {
176178
loginConnection.CreateUser(User, Password);
177179

178180
auto token = loginConnection.GetToken(User, Password);
179-
181+
180182
loginConnection.TestConnectRight(token, "No permission to connect to the database");
181183

182184
loginConnection.Stop();
@@ -188,7 +190,7 @@ Y_UNIT_TEST_SUITE(TGRpcAuthentication) {
188190
loginConnection.ModifyACL(true, User, NACLib::EAccessRights::ConnectDatabase);
189191

190192
auto token = loginConnection.GetToken(User, Password);
191-
193+
192194
loginConnection.TestConnectRight(token, "");
193195
loginConnection.TestDescribeRight(token, "Access denied");
194196

0 commit comments

Comments
 (0)