diff --git a/ydb/core/driver_lib/run/kikimr_services_initializers.cpp b/ydb/core/driver_lib/run/kikimr_services_initializers.cpp index 7a3a57060475..df480b3edb72 100644 --- a/ydb/core/driver_lib/run/kikimr_services_initializers.cpp +++ b/ydb/core/driver_lib/run/kikimr_services_initializers.cpp @@ -2263,7 +2263,7 @@ void TKqpServiceInitializer::InitializeServices(NActors::TActorSystemSetup* setu TActorSetupCmd(finalize, TMailboxType::HTSwap, appData->UserPoolId))); if (appData->FeatureFlags.GetEnableSchemaSecrets()) { - auto describeSchemaSecretsService = NKqp::CreateDescribeSchemaSecretsService(); + auto describeSchemaSecretsService = NKqp::TDescribeSchemaSecretsServiceFactory().CreateService(); setup->LocalServices.push_back(std::make_pair( NKqp::MakeKqpDescribeSchemaSecretServiceId(NodeId), TActorSetupCmd(describeSchemaSecretsService, TMailboxType::HTSwap, appData->UserPoolId))); diff --git a/ydb/core/kqp/common/events/script_executions.h b/ydb/core/kqp/common/events/script_executions.h index 5f8e6a387477..d44e5c05e4db 100644 --- a/ydb/core/kqp/common/events/script_executions.h +++ b/ydb/core/kqp/common/events/script_executions.h @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -281,7 +282,7 @@ struct TEvSaveScriptExternalEffectRequest : public TEventLocal UserToken; std::vector Sinks; std::vector SecretNames; }; @@ -386,7 +387,7 @@ struct TEvSaveScriptFinalStatusResponse : public TEventLocal UserToken; std::vector Sinks; std::vector SecretNames; Ydb::StatusIds::StatusCode Status; diff --git a/ydb/core/kqp/executer_actor/kqp_executer_impl.h b/ydb/core/kqp/executer_actor/kqp_executer_impl.h index 65a34468445a..980246c60677 100644 --- a/ydb/core/kqp/executer_actor/kqp_executer_impl.h +++ b/ydb/core/kqp/executer_actor/kqp_executer_impl.h @@ -946,7 +946,7 @@ class TKqpExecuterBase : public TActor { } void GetSecretsSnapshot() { - RegisterDescribeSecretsActor(this->SelfId(), UserToken ? UserToken->GetUserSID() : "", SecretNames, this->ActorContext().ActorSystem()); + RegisterDescribeSecretsActor(this->SelfId(), UserToken, Database, SecretNames, this->ActorContext().ActorSystem()); } void GetResourcesSnapshot() { diff --git a/ydb/core/kqp/federated_query/kqp_federated_query_actors.cpp b/ydb/core/kqp/federated_query/kqp_federated_query_actors.cpp index c2c44d9d2ba5..eeb36c520712 100644 --- a/ydb/core/kqp/federated_query/kqp_federated_query_actors.cpp +++ b/ydb/core/kqp/federated_query/kqp_federated_query_actors.cpp @@ -1,11 +1,14 @@ #include "kqp_federated_query_actors.h" #include +#include #include #include #include #define LOG_D(stream) LOG_DEBUG_S(*TlsActivationContext, NKikimrServices::SCHEMA_SECRET_CACHE, stream) +#define LOG_N(stream) LOG_NOTICE_S(*TlsActivationContext, NKikimrServices::SCHEMA_SECRET_CACHE, stream) +#define LOG_W(stream) LOG_WARN_S(*TlsActivationContext, NKikimrServices::SCHEMA_SECRET_CACHE, stream) namespace NKikimr::NKqp { @@ -99,72 +102,106 @@ class TDescribeSecretsActor: public NActors::TActorBootstrapped& secretIds, NThreading::TPromise promise) { + return new TDescribeSecretsActor(ownerUserId, secretIds, promise); +} + } // anonymous namespace -void TDescribeSchemaSecretsService::Handle(TEvResolveSecret::TPtr& ev) { - LOG_D("TEvResolveSecret: name=" << ev->Get()->SecretName << ", request cookie=" << LastCookie); +void TDescribeSchemaSecretsService::HandleIncomingRequest(TEvResolveSecret::TPtr& ev) { + LOG_D("TEvResolveSecret: names=" << JoinSeq(',', ev->Get()->SecretNames) << ", request cookie=" << LastCookie); + + if (ev->Get()->SecretNames.empty()) { + LOG_W("TEvResolveSecret: request cookie=" << ev->Cookie << ", empty secret names list"); + static const auto emptyRequest = TEvDescribeSecretsResponse::TDescription(Ydb::StatusIds::BAD_REQUEST, { NYql::TIssue("empty secret names list") }); + ev->Get()->Promise.SetValue(emptyRequest); + return; + } SaveIncomingRequestInfo(*ev->Get()); - SendSchemeCacheRequest(ev->Get()->SecretName); + SendSchemeCacheRequests(*ev->Get()); } -void TDescribeSchemaSecretsService::Handle(TEvTxProxySchemeCache::TEvNavigateKeySetResult::TPtr& ev) { +void TDescribeSchemaSecretsService::HandleSchemeCacheResponse(TEvTxProxySchemeCache::TEvNavigateKeySetResult::TPtr& ev) { LOG_D("TEvNavigateKeySetResult: request cookie=" << ev->Cookie); - Y_ENSURE(SecretNameInFlight.contains(ev->Cookie), "such request cookie is not registered"); - const auto& secretName = SecretNameInFlight[ev->Cookie]; + auto respIt = ResolveInFlight.find(ev->Cookie); + Y_ENSURE(respIt != ResolveInFlight.end(), "such request cookie is not registered"); TAutoPtr request = ev->Get()->Request; - if (request->ResultSet.empty() || request->ResultSet.front().Status != NSchemeCache::TSchemeCacheNavigate::EStatus::Ok) { - LOG_D("TEvNavigateKeySetResult: request cookie=" << ev->Cookie << ", SchemeCache error"); - FillResponse(ev->Cookie, TEvDescribeSecretsResponse::TDescription(Ydb::StatusIds::BAD_REQUEST, { NYql::TIssue("secret `" + secretName + "` not found") })); + if (HandleSchemeCacheErrorsIfAny(ev->Cookie, *request)) { return; } - const auto& secretDescription = request->ResultSet.front().SecretInfo->Description; - Y_ENSURE(!secretDescription.HasValue(), "SchemeCache must never contain secret values"); - - const auto secretIt = SecretNameToValue.find(secretName); - if (secretIt != SecretNameToValue.end()) { // some secret version is in cache - const auto secretDescription = request->ResultSet.front().SecretInfo->Description; - if (secretDescription.GetVersion() <= secretIt->second.Version) { // cache contains the most recent version - LOG_D("TEvNavigateKeySetResult: request cookie=" << ev->Cookie << ", fill value from secret cache"); - FillResponse(ev->Cookie, TEvDescribeSecretsResponse::TDescription(std::vector{secretIt->second.Value})); - return; + for (const auto& entry: request->ResultSet) { + const auto& secretDescription = entry.SecretInfo->Description; + Y_ENSURE(!secretDescription.HasValue(), "SchemeCache must never contain secret values"); + + const TString secretPath = CanonizePath(entry.Path); + const auto secretIt = VersionedSecrets.find(secretPath); + + if (secretIt != VersionedSecrets.end() && + (LocalCacheHasActualVersion(secretIt->second, secretDescription.GetVersion()) && + LocalCacheHasActualObject(secretIt->second, request->ResultSet.front().Self->Info.GetPathId()))) + { + // some secret version is in cache + ++respIt->second.FilledSecretsCnt; + } else { + // make TxProxy request + TAutoPtr navigateRequest(new TEvTxUserProxy::TEvNavigate()); + Y_ENSURE(!request->DatabaseName.empty(), "Database name must be set in TxProxy requests"); + navigateRequest->Record.SetDatabaseName(request->DatabaseName); + NKikimrSchemeOp::TDescribePath* record = navigateRequest->Record.MutableDescribePath(); + record->SetPath(secretPath); + record->MutableOptions()->SetReturnSecretValue(true); + Send(MakeTxProxyID(), navigateRequest.Release(), 0, ev->Cookie); } - SecretNameToValue.erase(secretIt); // no need to store outdated value } - TAutoPtr req(new TEvTxUserProxy::TEvNavigate()); - NKikimrSchemeOp::TDescribePath* record = req->Record.MutableDescribePath(); - record->SetPath(secretName); - record->MutableOptions()->SetReturnSecretValue(true); - // TODO(yurikiselev): Deal with UserToken [issue:25472] - Send(MakeTxProxyID(), req.Release(), 0, ev->Cookie); + FillResponseIfFinished(ev->Cookie, respIt->second); } -void TDescribeSchemaSecretsService::Handle(NSchemeShard::TEvSchemeShard::TEvDescribeSchemeResult::TPtr& ev) { +void TDescribeSchemaSecretsService::HandleSchemeShardResponse(NSchemeShard::TEvSchemeShard::TEvDescribeSchemeResult::TPtr& ev) { LOG_D("TEvDescribeSchemeResult: request cookie=" << ev->Cookie); - Y_ENSURE(SecretNameInFlight.contains(ev->Cookie), "such request cookie is not registered"); - const auto& secretName = SecretNameInFlight[ev->Cookie]; - const auto &rec = ev->Get()->GetRecord(); + const auto respIt = ResolveInFlight.find(ev->Cookie); + if (respIt == ResolveInFlight.end()) { + Y_ENSURE(respIt->second.Secrets.size() > 1, "This is possible only for batch requests"); + LOG_N("TEvDescribeSchemeResult: request cookie=" << ev->Cookie << "skipped response handling due to previous errors"); + // no need to fill response, since it has been filled on the first SchemeShard error + return; + } + + const auto& rec = ev->Get()->GetRecord(); + const auto& secretName = CanonizePath(rec.GetPath()); if (rec.GetStatus() != NKikimrScheme::EStatus::StatusSuccess) { - LOG_D("TEvDescribeSchemeResult: request cookie=" << ev->Cookie << ", SchemeShard error"); + LOG_N("TEvDescribeSchemeResult: request cookie=" << ev->Cookie << ", SchemeShard error"); FillResponse(ev->Cookie, TEvDescribeSecretsResponse::TDescription(Ydb::StatusIds::BAD_REQUEST, { NYql::TIssue("secret `" + secretName + "` not found") })); return; } + if (const auto it = SchemeBoardSubscribers.find(secretName); it == SchemeBoardSubscribers.end()) { + SchemeBoardSubscribers[secretName] = Register(CreateSchemeBoardSubscriber(SelfId(), secretName)); + } + const auto& secretValue = rec.GetPathDescription().GetSecretDescription().GetValue(); const auto& secretVersion = rec.GetPathDescription().GetSecretDescription().GetVersion(); - SecretNameToValue[secretName] = TVersionedSecret{.Version = secretVersion, .Value = secretValue}; - FillResponse(ev->Cookie, TEvDescribeSecretsResponse::TDescription(std::vector{secretValue})); + VersionedSecrets[secretName] = TVersionedSecret{ + .SecretVersion = secretVersion, + .PathId = rec.GetPathId(), + .Name = secretName, + .Value = secretValue, + }; + + ++respIt->second.FilledSecretsCnt; + + FillResponseIfFinished(ev->Cookie, respIt->second); } -void TDescribeSchemaSecretsService::FillResponse(const ui64 requestId, const TEvDescribeSecretsResponse::TDescription& response) { - SecretNameInFlight.erase(requestId); - ResolveInFlight[requestId].SetValue(response); - ResolveInFlight.erase(requestId); +void TDescribeSchemaSecretsService::FillResponse(const ui64& requestId, const TEvDescribeSecretsResponse::TDescription& response) { + auto respIt = ResolveInFlight.find(requestId); + respIt->second.Result.SetValue(response); + ResolveInFlight.erase(respIt); } void TDescribeSchemaSecretsService::Bootstrap() { @@ -172,52 +209,149 @@ void TDescribeSchemaSecretsService::Bootstrap() { Become(&TDescribeSchemaSecretsService::StateWait); } -void TDescribeSchemaSecretsService::SaveIncomingRequestInfo(const TEvResolveSecret& req) { - ResolveInFlight[LastCookie] = req.Promise; - SecretNameInFlight[LastCookie] = req.SecretName; +void TDescribeSchemaSecretsService::SaveIncomingRequestInfo(const TEvResolveSecret& ev) { + TResponseContext ctx; + for (size_t i = 0; i < ev.SecretNames.size(); ++i) { + ctx.Secrets[ev.SecretNames[i]] = i; + ctx.Result = ev.Promise; + } + ResolveInFlight[LastCookie] = std::move(ctx); } -void TDescribeSchemaSecretsService::SendSchemeCacheRequest(const TString& secretName) { +void TDescribeSchemaSecretsService::SendSchemeCacheRequests(const TEvResolveSecret& ev) { + const auto userToken = ev.UserToken; TAutoPtr request(new NSchemeCache::TSchemeCacheNavigate()); - NSchemeCache::TSchemeCacheNavigate::TEntry entry; - entry.Operation = NSchemeCache::TSchemeCacheNavigate::OpPath; - entry.Path = SplitPath(secretName); - request->ResultSet.emplace_back(entry); - // TODO(yurikiselev): Deal with UserToken [issue:25472] + for (const auto& secretName : ev.SecretNames) { + NSchemeCache::TSchemeCacheNavigate::TEntry entry; + entry.Operation = NSchemeCache::TSchemeCacheNavigate::OpPath; + entry.Path = SplitPath(secretName); + if (userToken && userToken->GetUserSID()) { + entry.Access = NACLib::SelectRow; + } + request->ResultSet.emplace_back(entry); + } + if (userToken && userToken->GetUserSID()) { + request->UserToken = userToken; + } + request->DatabaseName = ev.Database; Send(MakeSchemeCacheID(), new TEvTxProxySchemeCache::TEvNavigateKeySet(request), 0, LastCookie++); } -NThreading::TFuture DescribeSecret(const TString& secretName, const TString& ownerUserId, TActorSystem* actorSystem) { - auto promise = NThreading::NewPromise(); - if (actorSystem->AppData()->FeatureFlags.GetEnableSchemaSecrets() && TStringBuf(secretName).StartsWith("/")) { - actorSystem->Send( - MakeKqpDescribeSchemaSecretServiceId(actorSystem->NodeId), - new TDescribeSchemaSecretsService::TEvResolveSecret(ownerUserId, secretName, promise)); - } else { - actorSystem->Register(CreateDescribeSecretsActor(ownerUserId, {secretName}, promise)); +bool TDescribeSchemaSecretsService::LocalCacheHasActualVersion(const TVersionedSecret& secret, const ui64& cacheSecretVersion) { + // altering secret value does not change secret path id, so have to check secret version + return secret.SecretVersion == cacheSecretVersion; +} + +bool TDescribeSchemaSecretsService::LocalCacheHasActualObject(const TVersionedSecret& secret, const ui64& cacheSecretPathId) { + // This helps with the case when the secret was dropped and created again with the same name. + // Secret version will become zero again, which would not lead to a secret cache update. + return secret.PathId == cacheSecretPathId; +} + +bool TDescribeSchemaSecretsService::HandleSchemeCacheErrorsIfAny(const ui64& requestId, NSchemeCache::TSchemeCacheNavigate& result) { + if (result.ResultSet.empty()) { + LOG_N("TEvNavigateKeySetResult: request cookie=" << requestId << ", SchemeCache error"); + FillResponse(requestId, TEvDescribeSecretsResponse::TDescription(Ydb::StatusIds::BAD_REQUEST, { NYql::TIssue("secrets were not found") })); + return true; } - return promise.GetFuture(); + + for (const auto& entry: result.ResultSet) { + if (entry.Status != NSchemeCache::TSchemeCacheNavigate::EStatus::Ok) { + const auto secretPath = CanonizePath(entry.Path); + LOG_N("TEvNavigateKeySetResult: request cookie=" << requestId << ", unauthorized SchemeCache request for secret=" << secretPath); + FillResponse(requestId, TEvDescribeSecretsResponse::TDescription(Ydb::StatusIds::BAD_REQUEST, { NYql::TIssue("secret `" + secretPath + "` not found") })); + + return true; + } + } + return false; } -IActor* CreateDescribeSecretsActor(const TString& ownerUserId, const std::vector& secretIds, NThreading::TPromise promise) { - return new TDescribeSecretsActor(ownerUserId, secretIds, promise); +void TDescribeSchemaSecretsService::FillResponseIfFinished(const ui64& requestId, const TResponseContext& responseCtx) { + if (responseCtx.FilledSecretsCnt != responseCtx.Secrets.size()) { + return; + } + + std::vector secretValues; + secretValues.resize(responseCtx.Secrets.size()); + for (const auto& secret : responseCtx.Secrets) { + const auto& secretPath = secret.first; + auto it = VersionedSecrets.find(secret.first); + if (it == VersionedSecrets.end()) { + LOG_N("FillResponseIfFinished: request cookie=" << requestId << ", secret `" << secretPath << "` was dropped during request"); + FillResponse(requestId, TEvDescribeSecretsResponse::TDescription(Ydb::StatusIds::BAD_REQUEST, { NYql::TIssue("secret `" + secretPath + "` not found") })); + return; + } + + Y_ENSURE(secret.second < secretValues.size()); + secretValues[secret.second] = it->second.Value; + } + FillResponse(requestId, TEvDescribeSecretsResponse::TDescription(secretValues)); +} + +void TDescribeSchemaSecretsService::HandleNotifyUpdate(TSchemeBoardEvents::TEvNotifyUpdate::TPtr& ev) { + Y_UNUSED(ev); +} + +void TDescribeSchemaSecretsService::HandleNotifyDelete(TSchemeBoardEvents::TEvNotifyDelete::TPtr& ev) { + const TString& secretName = CanonizePath(ev->Get()->Path); + + if (SecretUpdateListener) { + SecretUpdateListener->HandleNotifyDelete(secretName); + } + + VersionedSecrets.erase(secretName); + + const auto subscriberIt = SchemeBoardSubscribers.find(secretName); + Y_ENSURE(subscriberIt != SchemeBoardSubscribers.end()); + Send(subscriberIt->second, new TEvents::TEvPoisonPill()); + SchemeBoardSubscribers.erase(subscriberIt); } -void RegisterDescribeSecretsActor(const NActors::TActorId& replyActorId, const TString& ownerUserId, const std::vector& secretIds, NActors::TActorSystem* actorSystem) { +NThreading::TFuture DescribeSecret( + const TVector& secretNames, + const TIntrusiveConstPtr userToken, + const TString& database, + TActorSystem* actorSystem +) { auto promise = NThreading::NewPromise(); - actorSystem->Register(CreateDescribeSecretsActor(ownerUserId, secretIds, promise)); + if (UseSchemaSecrets(AppData()->FeatureFlags, secretNames)) { + actorSystem->Send( + MakeKqpDescribeSchemaSecretServiceId(actorSystem->NodeId), + new TDescribeSchemaSecretsService::TEvResolveSecret(userToken, database, secretNames, promise) + ); + return promise.GetFuture(); + } - promise.GetFuture().Subscribe([actorSystem, replyActorId](const NThreading::TFuture& result){ + actorSystem->Register(CreateDescribeSecretsActor(userToken ? userToken->GetUserSID() : "", secretNames, promise)); + return promise.GetFuture(); +} + +void RegisterDescribeSecretsActor( + const NActors::TActorId& replyActorId, + const TIntrusiveConstPtr userToken, + const TString& database, + const std::vector& secretIds, + NActors::TActorSystem* actorSystem +) { + TVector secretNames{secretIds.begin(), secretIds.end()}; + auto future = DescribeSecret(secretNames, userToken, database, actorSystem); + future.Subscribe([actorSystem, replyActorId](const NThreading::TFuture& result){ actorSystem->Send(replyActorId, new TEvDescribeSecretsResponse(result.GetValue())); }); } -NThreading::TFuture DescribeExternalDataSourceSecrets(const NKikimrSchemeOp::TAuth& authDescription, const TString& ownerUserId, TActorSystem* actorSystem) { +NThreading::TFuture DescribeExternalDataSourceSecrets( + const NKikimrSchemeOp::TAuth& authDescription, + const TIntrusiveConstPtr userToken, + const TString& database, + TActorSystem* actorSystem +) { switch (authDescription.identity_case()) { case NKikimrSchemeOp::TAuth::kServiceAccount: { const TString& saSecretId = authDescription.GetServiceAccount().GetSecretName(); - return DescribeSecret(saSecretId, ownerUserId, actorSystem); + return DescribeSecret({saSecretId}, userToken, database, actorSystem); } case NKikimrSchemeOp::TAuth::kNone: @@ -225,28 +359,24 @@ NThreading::TFuture DescribeExternalDa case NKikimrSchemeOp::TAuth::kBasic: { const TString& passwordSecretId = authDescription.GetBasic().GetPasswordSecretName(); - return DescribeSecret(passwordSecretId, ownerUserId, actorSystem); + return DescribeSecret({passwordSecretId}, userToken, database, actorSystem); } case NKikimrSchemeOp::TAuth::kMdbBasic: { const TString& saSecretId = authDescription.GetMdbBasic().GetServiceAccountSecretName(); const TString& passwordSecreId = authDescription.GetMdbBasic().GetPasswordSecretName(); - auto promise = NThreading::NewPromise(); - actorSystem->Register(CreateDescribeSecretsActor(ownerUserId, {saSecretId, passwordSecreId}, promise)); - return promise.GetFuture(); + return DescribeSecret({saSecretId, passwordSecreId}, userToken, database, actorSystem); } case NKikimrSchemeOp::TAuth::kAws: { const TString& awsAccessKeyIdSecretId = authDescription.GetAws().GetAwsAccessKeyIdSecretName(); const TString& awsAccessKeyKeySecretId = authDescription.GetAws().GetAwsSecretAccessKeySecretName(); - auto promise = NThreading::NewPromise(); - actorSystem->Register(CreateDescribeSecretsActor(ownerUserId, {awsAccessKeyIdSecretId, awsAccessKeyKeySecretId}, promise)); - return promise.GetFuture(); + return DescribeSecret({awsAccessKeyIdSecretId, awsAccessKeyKeySecretId}, userToken, database, actorSystem); } case NKikimrSchemeOp::TAuth::kToken: { const TString& tokenSecretId = authDescription.GetToken().GetTokenSecretName(); - return DescribeSecret(tokenSecretId, ownerUserId, actorSystem); + return DescribeSecret({tokenSecretId}, userToken, database, actorSystem); } case NKikimrSchemeOp::TAuth::IDENTITY_NOT_SET: @@ -254,8 +384,26 @@ NThreading::TFuture DescribeExternalDa } } -IActor* CreateDescribeSchemaSecretsService() { +IActor* TDescribeSchemaSecretsServiceFactory::CreateService() { return new TDescribeSchemaSecretsService(); } +bool UseSchemaSecrets(const NKikimr::TFeatureFlags& flags, const TVector& secretNames) { + if (!flags.GetEnableSchemaSecrets()) { + return false; + } + + for (const auto& secretName : secretNames) { + if (!secretName.StartsWith('/')) { + return false; + } + } + + return true; // New secrets are enabled and all of them start with '/' +} + +bool UseSchemaSecrets(const NKikimr::TFeatureFlags& flags, const TString& secretName) { + return flags.GetEnableSchemaSecrets() && secretName.StartsWith('/'); +} + } // namespace NKikimr::NKqp diff --git a/ydb/core/kqp/federated_query/kqp_federated_query_actors.h b/ydb/core/kqp/federated_query/kqp_federated_query_actors.h index 3eaa63a4e751..e8a41e90a60f 100644 --- a/ydb/core/kqp/federated_query/kqp_federated_query_actors.h +++ b/ydb/core/kqp/federated_query/kqp_federated_query_actors.h @@ -2,15 +2,16 @@ #include #include +#include +#include +#include +#include #include #include #include -#include -#include -#include -#include +#include namespace NKikimr::NKqp { @@ -24,60 +25,127 @@ class TDescribeSchemaSecretsService: public NActors::TActorBootstrapped { public: TEvResolveSecret( - const TString& ownerUserId, - const TString& secretName, + const TIntrusiveConstPtr userToken, + const TString& database, + const TVector& secretNames, NThreading::TPromise promise ) - : UserToken(NACLib::TUserToken{ownerUserId, TVector{}}) - , SecretName(secretName) + : UserToken(userToken) + , Database(database) + , SecretNames(secretNames) , Promise(promise) { + Y_ENSURE(!Database.empty(), "Database name must be set in secret requests"); } public: - const NACLib::TUserToken UserToken; - const TString SecretName; + const TIntrusiveConstPtr UserToken; + const TString Database; + const TVector SecretNames; NThreading::TPromise Promise; }; +private: + struct TVersionedSecret { + ui64 SecretVersion = 0; + ui64 PathId = 0; + TString Name; + TString Value; + }; + + struct TResponseContext { + using TIncomingOrderId = ui64; + THashMap Secrets; + NThreading::TPromise Result; + size_t FilledSecretsCnt = 0; + }; + private: STRICT_STFUNC(StateWait, - hFunc(TEvResolveSecret, Handle); - hFunc(TEvTxProxySchemeCache::TEvNavigateKeySetResult, Handle); - hFunc(NSchemeShard::TEvSchemeShard::TEvDescribeSchemeResult, Handle); + hFunc(TEvResolveSecret, HandleIncomingRequest); + hFunc(TEvTxProxySchemeCache::TEvNavigateKeySetResult, HandleSchemeCacheResponse); + hFunc(NSchemeShard::TEvSchemeShard::TEvDescribeSchemeResult, HandleSchemeShardResponse); + hFunc(TSchemeBoardEvents::TEvNotifyDelete, HandleNotifyDelete); + hFunc(TSchemeBoardEvents::TEvNotifyUpdate, HandleNotifyUpdate); cFunc(NActors::TEvents::TEvPoison::EventType, PassAway); ) - void Handle(TEvResolveSecret::TPtr& ev); - void Handle(TEvTxProxySchemeCache::TEvNavigateKeySetResult::TPtr& ev); - void Handle(NSchemeShard::TEvSchemeShard::TEvDescribeSchemeResult::TPtr& ev); - void FillResponse(const ui64 requestId, const TEvDescribeSecretsResponse::TDescription& response); - void SaveIncomingRequestInfo(const TEvResolveSecret& req); - void SendSchemeCacheRequest(const TString& secretName); + void HandleIncomingRequest(TEvResolveSecret::TPtr& ev); + void HandleSchemeCacheResponse(TEvTxProxySchemeCache::TEvNavigateKeySetResult::TPtr& ev); + void HandleSchemeShardResponse(NSchemeShard::TEvSchemeShard::TEvDescribeSchemeResult::TPtr& ev); + void HandleNotifyDelete(TSchemeBoardEvents::TEvNotifyDelete::TPtr& ev); + void HandleNotifyUpdate(TSchemeBoardEvents::TEvNotifyUpdate::TPtr& ev); + + void FillResponse(const ui64& requestId, const TEvDescribeSecretsResponse::TDescription& response); + void SaveIncomingRequestInfo(const TEvResolveSecret& ev); + void SendSchemeCacheRequests(const TEvResolveSecret& ev); + bool LocalCacheHasActualVersion(const TVersionedSecret& secret, const ui64& cacheSecretVersion); + bool LocalCacheHasActualObject(const TVersionedSecret& secret, const ui64& cacheSecretPathId); + bool HandleSchemeCacheErrorsIfAny(const ui64& requestId, NSchemeCache::TSchemeCacheNavigate& result); + void FillResponseIfFinished(const ui64& requestId, const TResponseContext& responseCtx); public: TDescribeSchemaSecretsService() = default; void Bootstrap(); -private: - struct TVersionedSecret { - ui64 Version; - TString Value; +public: + // For tests only + class ISecretUpdateListener : public TThrRefBase { + public: + virtual void HandleNotifyDelete(const TString& secretName) = 0; + virtual ~ISecretUpdateListener() = default; }; + void SetSecretUpdateListener(ISecretUpdateListener* secretUpdateListener) { + SecretUpdateListener = secretUpdateListener; + } +private: ui64 LastCookie = 0; - THashMap> ResolveInFlight; - THashMap SecretNameInFlight; - THashMap SecretNameToValue; + THashMap ResolveInFlight; + THashMap VersionedSecrets; + THashMap SchemeBoardSubscribers; + ISecretUpdateListener* SecretUpdateListener; }; -IActor* CreateDescribeSecretsActor(const TString& ownerUserId, const std::vector& secretIds, NThreading::TPromise promise); +void RegisterDescribeSecretsActor( + const NActors::TActorId& replyActorId, + const TIntrusiveConstPtr userToken, + const TString& database, + const std::vector& secretIds, + NActors::TActorSystem* actorSystem +); + +NThreading::TFuture DescribeExternalDataSourceSecrets( + const NKikimrSchemeOp::TAuth& authDescription, + const TIntrusiveConstPtr userToken, + const TString& database, + TActorSystem* actorSystem +); -void RegisterDescribeSecretsActor(const TActorId& replyActorId, const TString& ownerUserId, const std::vector& secretIds, TActorSystem* actorSystem); +IActor* CreateDescribeSchemaSecretsService(); -NThreading::TFuture DescribeExternalDataSourceSecrets(const NKikimrSchemeOp::TAuth& authDescription, const TString& ownerUserId, TActorSystem* actorSystem); +class IDescribeSchemaSecretsServiceFactory { +public: + using TPtr = std::shared_ptr; -IActor* CreateDescribeSchemaSecretsService(); + virtual IActor* CreateService() = 0; + virtual ~IDescribeSchemaSecretsServiceFactory() = default; +}; + +class TDescribeSchemaSecretsServiceFactory : public IDescribeSchemaSecretsServiceFactory { +public: + IActor* CreateService() override; +}; + +NThreading::TFuture DescribeSecret( + const TVector& secretNames, + const TIntrusiveConstPtr userToken, + const TString& database, + TActorSystem* actorSystem +); + +bool UseSchemaSecrets(const NKikimr::TFeatureFlags& flags, const TVector& secretNames); +bool UseSchemaSecrets(const NKikimr::TFeatureFlags& flags, const TString& secretName); } // namespace NKikimr::NKqp diff --git a/ydb/core/kqp/federated_query/kqp_federated_query_actors_ut.cpp b/ydb/core/kqp/federated_query/kqp_federated_query_actors_ut.cpp index 9117efb63bd2..72eda376d0df 100644 --- a/ydb/core/kqp/federated_query/kqp_federated_query_actors_ut.cpp +++ b/ydb/core/kqp/federated_query/kqp_federated_query_actors_ut.cpp @@ -26,20 +26,35 @@ namespace { } NThreading::TPromise - ResolveSecret(const TString& userId, const TString& secretName, NKikimr::NKqp::TKikimrRunner& kikimr) { + ResolveSecret(const TVector& secretNames, NKikimr::NKqp::TKikimrRunner& kikimr, const TIntrusiveConstPtr userToken = nullptr) { auto promise = NThreading::NewPromise(); - const auto evResolveSecret = new NKikimr::NKqp::TDescribeSchemaSecretsService::TEvResolveSecret(userId,secretName, promise); + const auto evResolveSecret = new NKikimr::NKqp::TDescribeSchemaSecretsService::TEvResolveSecret(userToken, "/Root", secretNames, promise); auto actorSystem = kikimr.GetTestServer().GetRuntime()->GetActorSystem(0); actorSystem->Send(NKikimr::NKqp::MakeKqpDescribeSchemaSecretServiceId(actorSystem->NodeId), evResolveSecret); return promise; } + + NThreading::TPromise + ResolveSecret(const TString& secretName, NKikimr::NKqp::TKikimrRunner& kikimr, const TIntrusiveConstPtr userToken = nullptr) { + return ResolveSecret(TVector{secretName}, kikimr, userToken); + } + + void AssertBadRequest(NThreading::TPromise promise, const TString& err) { + UNIT_ASSERT_VALUES_EQUAL(Ydb::StatusIds::BAD_REQUEST, promise.GetFuture().GetValueSync().Status); + UNIT_ASSERT_VALUES_EQUAL(err, promise.GetFuture().GetValueSync().Issues.ToString()); + } + + TIntrusiveConstPtr GetUserToken(const TString& userSid = "", const TVector& groupSids = {}) { + if (userSid.empty() && groupSids.empty()) { + return nullptr; + } + return new NACLib::TUserToken(userSid, groupSids); + } } Y_UNIT_TEST_SUITE(DescribeSchemaSecretsService) { Y_UNIT_TEST(GetNewValue) { - NKikimrConfig::TAppConfig appCfg; - appCfg.MutableQueryServiceConfig()->AddAvailableExternalDataSources("ObjectStorage"); - NKikimr::NKqp::TKikimrRunner kikimr{ NKikimr::NKqp::TKikimrSettings(appCfg) }; + NKikimr::NKqp::TKikimrRunner kikimr; kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); @@ -49,15 +64,13 @@ Y_UNIT_TEST_SUITE(DescribeSchemaSecretsService) { CreateSchemaSecret(secretName, secretValue, session); for (int i = 0; i < 3; ++i) { - auto promise = ResolveSecret("ownerId", "/Root/secret-name", kikimr); + auto promise = ResolveSecret("/Root/secret-name", kikimr); UNIT_ASSERT_VALUES_EQUAL(secretValue, promise.GetFuture().GetValueSync().SecretValues[0]); } } Y_UNIT_TEST(GetUpdatedValue) { - NKikimrConfig::TAppConfig appCfg; - appCfg.MutableQueryServiceConfig()->AddAvailableExternalDataSources("ObjectStorage"); - NKikimr::NKqp::TKikimrRunner kikimr{ NKikimr::NKqp::TKikimrSettings(appCfg) }; + NKikimr::NKqp::TKikimrRunner kikimr; kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); @@ -66,35 +79,63 @@ Y_UNIT_TEST_SUITE(DescribeSchemaSecretsService) { TString secretValue = "secret-value"; CreateSchemaSecret(secretName, secretValue, session); - auto promise = ResolveSecret("ownerId", "/Root/secret-name", kikimr); + auto promise = ResolveSecret("/Root/secret-name", kikimr); UNIT_ASSERT_VALUES_EQUAL(secretValue, promise.GetFuture().GetValueSync().SecretValues[0]); for (int i = 0; i < 3; ++i) { - TString newSecretValue = secretName + "-" + ToString(i); + TString newSecretValue = secretValue + "-" + ToString(i); AlterSchemaSecret(secretName, newSecretValue, session); - auto promise = ResolveSecret("ownerId", "/Root/secret-name", kikimr); + auto promise = ResolveSecret("/Root/secret-name", kikimr); UNIT_ASSERT_VALUES_EQUAL(newSecretValue, promise.GetFuture().GetValueSync().SecretValues[0]); } } Y_UNIT_TEST(GetUnexistingValue) { - NKikimrConfig::TAppConfig appCfg; - appCfg.MutableQueryServiceConfig()->AddAvailableExternalDataSources("ObjectStorage"); - NKikimr::NKqp::TKikimrRunner kikimr{ NKikimr::NKqp::TKikimrSettings(appCfg) }; + NKikimr::NKqp::TKikimrRunner kikimr; kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); - auto promise = ResolveSecret("ownerId", "/Root/secret-not-exist", kikimr); + auto promise = ResolveSecret("/Root/secret-not-exist", kikimr); - UNIT_ASSERT_VALUES_EQUAL(Ydb::StatusIds::BAD_REQUEST, promise.GetFuture().GetValueSync().Status); + AssertBadRequest(promise, "
: Error: secret `/Root/secret-not-exist` not found\n"); } Y_UNIT_TEST(GetDroppedValue) { - NKikimrConfig::TAppConfig appCfg; - appCfg.MutableQueryServiceConfig()->AddAvailableExternalDataSources("ObjectStorage"); - NKikimr::NKqp::TKikimrRunner kikimr{ NKikimr::NKqp::TKikimrSettings(appCfg) }; + class TTestSecretUpdateListener : public NKikimr::NKqp::TDescribeSchemaSecretsService::ISecretUpdateListener { + public: + NThreading::TPromise DeletionPromise = NThreading::NewPromise(); + + public: + void HandleNotifyDelete(const TString& secretName) override { + Y_ENSURE(!DeletionPromise.HasValue()); // only one call of HandleNotifyDelete is expected + DeletionPromise.SetValue(secretName); + } + }; + + class TTestDescribeSchemaSecretsServiceFactory : public NKikimr::NKqp::IDescribeSchemaSecretsServiceFactory { + public: + TTestDescribeSchemaSecretsServiceFactory(NKikimr::NKqp::TDescribeSchemaSecretsService::ISecretUpdateListener* secretUpdateListener) + : SecretUpdateListener(secretUpdateListener) + { + } + + NActors::IActor* CreateService() override { + auto* service = new NKikimr::NKqp::TDescribeSchemaSecretsService(); + service->SetSecretUpdateListener(SecretUpdateListener); + return service; + } + + private: + NKikimr::NKqp::TDescribeSchemaSecretsService::ISecretUpdateListener* SecretUpdateListener; + }; + + NKikimr::NKqp::TKikimrSettings settings; + auto secretUpdateListener = MakeHolder(); + auto factory = std::make_shared(secretUpdateListener.Get()); + settings.SetDescribeSchemaSecretsServiceFactory(factory); + NKikimr::NKqp::TKikimrRunner kikimr(settings); kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); @@ -103,20 +144,25 @@ Y_UNIT_TEST_SUITE(DescribeSchemaSecretsService) { TString secretValue = "secret-value"; CreateSchemaSecret(secretName, secretValue, session); - auto promise = ResolveSecret("ownerId", "/Root/secret-name", kikimr); + auto promise = ResolveSecret("/Root/secret-name", kikimr); UNIT_ASSERT_VALUES_EQUAL(secretValue, promise.GetFuture().GetValueSync().SecretValues[0]); DropSchemaSecret(secretName, session); + UNIT_ASSERT_VALUES_EQUAL("/Root/secret-name", secretUpdateListener->DeletionPromise.GetFuture().GetValueSync()); - promise = ResolveSecret("ownerId", "/Root/secret-name", kikimr); - UNIT_ASSERT_VALUES_EQUAL(Ydb::StatusIds::BAD_REQUEST, promise.GetFuture().GetValueSync().Status); + promise = ResolveSecret("/Root/secret-name", kikimr); + AssertBadRequest(promise, "
: Error: secret `/Root/secret-name` not found\n"); + + secretValue += "-updated"; + CreateSchemaSecret(secretName, secretValue, session); + + promise = ResolveSecret("/Root/secret-name", kikimr); + UNIT_ASSERT_VALUES_EQUAL(secretValue, promise.GetFuture().GetValueSync().SecretValues[0]); } Y_UNIT_TEST(GetInParallel) { static const int SECRETS_CNT = 5; - NKikimrConfig::TAppConfig appCfg; - appCfg.MutableQueryServiceConfig()->AddAvailableExternalDataSources("ObjectStorage"); - NKikimr::NKqp::TKikimrRunner kikimr{ NKikimr::NKqp::TKikimrSettings(appCfg) }; + NKikimr::NKqp::TKikimrRunner kikimr; kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); @@ -129,7 +175,7 @@ Y_UNIT_TEST_SUITE(DescribeSchemaSecretsService) { } std::vector> promises; for (const auto& [secretName, secretValue] : secrets) { - promises.push_back(ResolveSecret("ownerId", secretName, kikimr)); + promises.push_back(ResolveSecret(secretName, kikimr)); } for (int i = 0; i < SECRETS_CNT; ++i) { @@ -143,13 +189,222 @@ Y_UNIT_TEST_SUITE(DescribeSchemaSecretsService) { AlterSchemaSecret(secrets[i].first, secrets[i].second, session); } for (const auto& [secretName, secretValue] : secrets) { - promises.push_back(ResolveSecret("ownerId", secretName, kikimr)); + promises.push_back(ResolveSecret(secretName, kikimr)); } for (int i = 0; i < SECRETS_CNT; ++i) { UNIT_ASSERT_VALUES_EQUAL(secrets[i].second, promises[i].GetFuture().GetValueSync().SecretValues[0]); } } + + Y_UNIT_TEST(FailWithoutGrants) { + NKikimr::NKqp::TKikimrRunner kikimr; + kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); + + const TString secretName = "/Root/secret-name"; + const TString secretValue = "secret-value"; + auto adminSession = kikimr.GetTableClient(NYdb::NTable::TClientSettings().AuthToken("root@builtin")) + .CreateSession().GetValueSync().GetSession(); + + CreateSchemaSecret(secretName, secretValue, adminSession); + + auto promise = ResolveSecret(secretName, kikimr, GetUserToken("root@builtin")); + UNIT_ASSERT_VALUES_EQUAL(secretValue, promise.GetFuture().GetValueSync().SecretValues[0]); + + const auto userToken = GetUserToken("user@builtin"); + { // assert no grants by default + auto promise = ResolveSecret("/Root/secret-name", kikimr, userToken); + AssertBadRequest(promise, "
: Error: secret `/Root/secret-name` not found\n"); + } + + // provide grants + const auto grantResult = adminSession.ExecuteSchemeQuery( + Sprintf("GRANT 'ydb.granular.select_row' ON `%s` TO `%s`;", secretName.data(), "user@builtin") + ).GetValueSync(); + UNIT_ASSERT_C(grantResult.GetStatus() == NYdb::EStatus::SUCCESS, grantResult.GetIssues().ToString()); + + { // assert grants are ok + auto promise = ResolveSecret("/Root/secret-name", kikimr, userToken); + UNIT_ASSERT_VALUES_EQUAL(secretValue, promise.GetFuture().GetValueSync().SecretValues[0]); + } + + // revoke grants + const auto revokeResult = adminSession.ExecuteSchemeQuery( + Sprintf("REVOKE 'ydb.granular.select_row' ON `%s` FROM `%s`;", secretName.data(), "user@builtin") + ).GetValueSync(); + UNIT_ASSERT_C(revokeResult.GetStatus() == NYdb::EStatus::SUCCESS, grantResult.GetIssues().ToString()); + + { // assert no grants after revoking + auto promise = ResolveSecret("/Root/secret-name", kikimr, userToken); + AssertBadRequest(promise, "
: Error: secret `/Root/secret-name` not found\n"); + } + } + + Y_UNIT_TEST(GroupGrants) { + NKikimr::NKqp::TKikimrRunner kikimr; + kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); + + const TString secretName = "/Root/secret-name"; + const TString secretValue = "secret-value"; + auto adminSession = kikimr.GetTableClient(NYdb::NTable::TClientSettings().AuthToken("root@builtin")) + .CreateSession().GetValueSync().GetSession(); + + CreateSchemaSecret(secretName, secretValue, adminSession); + + auto promise = ResolveSecret(secretName, kikimr, GetUserToken("root@builtin")); + UNIT_ASSERT_VALUES_EQUAL(secretValue, promise.GetFuture().GetValueSync().SecretValues[0]); + + const auto userToken = GetUserToken("user@builtin", {"group"}); + { // assert no grants by default + auto promise = ResolveSecret("/Root/secret-name", kikimr, userToken); + AssertBadRequest(promise, "
: Error: secret `/Root/secret-name` not found\n"); + } + + const auto createGroupResult = adminSession.ExecuteSchemeQuery( + Sprintf("CREATE GROUP `group` WITH USER `user@builtin`;") + ).GetValueSync(); + UNIT_ASSERT_C(createGroupResult.GetStatus() == NYdb::EStatus::SUCCESS, createGroupResult.GetIssues().ToString()); + + const auto grantResult = adminSession.ExecuteSchemeQuery( + Sprintf("GRANT 'ydb.granular.select_row' ON `%s` TO `%s`;", secretName.data(), "group") + ).GetValueSync(); + UNIT_ASSERT_C(grantResult.GetStatus() == NYdb::EStatus::SUCCESS, grantResult.GetIssues().ToString()); + + { // assert group grants are ok + auto promise = ResolveSecret("/Root/secret-name", kikimr, userToken); + UNIT_ASSERT_VALUES_EQUAL(secretValue, promise.GetFuture().GetValueSync().SecretValues[0]); + } + + // revoke grants + const auto revokeResult = adminSession.ExecuteSchemeQuery( + Sprintf("REVOKE 'ydb.granular.select_row' ON `%s` FROM `%s`;", secretName.data(), "group") + ).GetValueSync(); + UNIT_ASSERT_C(revokeResult.GetStatus() == NYdb::EStatus::SUCCESS, grantResult.GetIssues().ToString()); + + { // assert no grants after revoking + auto promise = ResolveSecret("/Root/secret-name", kikimr, userToken); + AssertBadRequest(promise, "
: Error: secret `/Root/secret-name` not found\n"); + } + } + + Y_UNIT_TEST(BatchRequest) { + NKikimr::NKqp::TKikimrRunner kikimr; + kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); + + const TString secretName1 = "/Root/secret-name-1"; + const TString secretValue1 = "secret-value-1"; + const TString secretName2 = "/Root/secret-name-2"; + const TString secretValue2 = "secret-value-2"; + const TString secretName3 = "/Root/secret-name-3"; + const TString secretValue3 = "secret-value-3"; + + auto db = kikimr.GetTableClient(); + auto session = db.CreateSession().GetValueSync().GetSession(); + + CreateSchemaSecret(secretName1, secretValue1, session); + CreateSchemaSecret(secretName2, secretValue2, session); + CreateSchemaSecret(secretName3, secretValue3, session); + + { // nothing from cache + auto promise = ResolveSecret({secretName1, secretName2}, kikimr); + UNIT_ASSERT_VALUES_EQUAL(secretValue1, promise.GetFuture().GetValueSync().SecretValues[0]); + UNIT_ASSERT_VALUES_EQUAL(secretValue2, promise.GetFuture().GetValueSync().SecretValues[1]); + } + + { // something from cache + auto promise = ResolveSecret({secretName2, secretName3}, kikimr); + UNIT_ASSERT_VALUES_EQUAL(secretValue2, promise.GetFuture().GetValueSync().SecretValues[0]); + UNIT_ASSERT_VALUES_EQUAL(secretValue3, promise.GetFuture().GetValueSync().SecretValues[1]); + } + + { // all from cache + auto promise = ResolveSecret({secretName1, secretName2, secretName3}, kikimr); + UNIT_ASSERT_VALUES_EQUAL(secretValue1, promise.GetFuture().GetValueSync().SecretValues[0]); + UNIT_ASSERT_VALUES_EQUAL(secretValue2, promise.GetFuture().GetValueSync().SecretValues[1]); + UNIT_ASSERT_VALUES_EQUAL(secretValue3, promise.GetFuture().GetValueSync().SecretValues[2]); + } + } + + Y_UNIT_TEST(BigBatchRequest) { + NKikimr::NKqp::TKikimrRunner kikimr; + kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); + + TVector names; + TVector values; + for (int i = 0; i < 10; ++i) { + names.push_back("/Root/secret-name-" + ToString(i)); + values.push_back("secret-value-" + ToString(i)); + } + + auto db = kikimr.GetTableClient(); + auto session = db.CreateSession().GetValueSync().GetSession(); + + for (size_t i = 0; i < names.size(); ++i) { + CreateSchemaSecret(names[i], values[i], session); + } + + { // nothing from cache + auto promise = ResolveSecret(names, kikimr); + for (size_t i = 0; i < names.size(); ++i) { + UNIT_ASSERT_VALUES_EQUAL(values[i], promise.GetFuture().GetValueSync().SecretValues[i]); + } + } + + { // something from cache + auto promise = ResolveSecret(names, kikimr); + for (size_t i = 0; i < names.size(); ++i) { + UNIT_ASSERT_VALUES_EQUAL(values[i], promise.GetFuture().GetValueSync().SecretValues[i]); + } + } + } + + Y_UNIT_TEST(EmptyBatch) { + NKikimr::NKqp::TKikimrRunner kikimr; + kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); + + auto promise = ResolveSecret(TVector{}, kikimr); + AssertBadRequest(promise, "
: Error: empty secret names list\n"); + } + + Y_UNIT_TEST(MixedGrantsInBatch) { + NKikimr::NKqp::TKikimrRunner kikimr; + kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); + + auto adminSession = kikimr.GetTableClient(NYdb::NTable::TClientSettings().AuthToken("root@builtin")) + .CreateSession().GetValueSync().GetSession(); + + TVector names; + TVector values; + for (int i = 0; i < 2; ++i) { + names.push_back("/Root/secret-name-" + ToString(i)); + values.push_back("secret-value-" + ToString(i)); + CreateSchemaSecret(names.back(), values.back(), adminSession); + } + + auto grantResult = adminSession.ExecuteSchemeQuery( + Sprintf("GRANT 'ydb.granular.select_row' ON `%s` TO `%s`;", names[0].data(), "user@builtin") + ).GetValueSync(); + UNIT_ASSERT_C(grantResult.GetStatus() == NYdb::EStatus::SUCCESS, grantResult.GetIssues().ToString()); + + auto userToken = GetUserToken("user@builtin"); + { // user has grants for names[0], has no grants for names[1] + auto promise = ResolveSecret({names[0], names[1]}, kikimr, userToken); + AssertBadRequest(promise, "
: Error: secret `/Root/secret-name-1` not found\n"); + } + + grantResult = adminSession.ExecuteSchemeQuery( + Sprintf("GRANT 'ydb.granular.select_row' ON `%s` TO `%s`;", names[1].data(), "user@builtin") + ).GetValueSync(); + UNIT_ASSERT_C(grantResult.GetStatus() == NYdb::EStatus::SUCCESS, grantResult.GetIssues().ToString()); + + { // user has grants for all names[0] + auto promise = ResolveSecret({names[0], names[1]}, kikimr, userToken); + for (size_t i = 0; i < values.size(); ++i) { + UNIT_ASSERT_VALUES_EQUAL(values[i], promise.GetFuture().GetValueSync().SecretValues[i]); + } + } + } + } } diff --git a/ydb/core/kqp/federated_query/ut/ya.make b/ydb/core/kqp/federated_query/ut/ya.make index 6570bfc7a3fe..7aad9798112a 100644 --- a/ydb/core/kqp/federated_query/ut/ya.make +++ b/ydb/core/kqp/federated_query/ut/ya.make @@ -1,5 +1,7 @@ UNITTEST_FOR(ydb/core/kqp/federated_query) +SIZE(MEDIUM) + PEERDIR( ydb/core/kqp/federated_query ydb/public/api/protos diff --git a/ydb/core/kqp/federated_query/ut_service/ya.make b/ydb/core/kqp/federated_query/ut_service/ya.make index 54e96db48023..9ae593231431 100644 --- a/ydb/core/kqp/federated_query/ut_service/ya.make +++ b/ydb/core/kqp/federated_query/ut_service/ya.make @@ -1,5 +1,7 @@ UNITTEST_FOR(ydb/core/kqp/federated_query) +SIZE(MEDIUM) + PEERDIR( ydb/core/kqp/federated_query ydb/core/kqp/ut/common diff --git a/ydb/core/kqp/finalize_script_service/kqp_finalize_script_actor.cpp b/ydb/core/kqp/finalize_script_service/kqp_finalize_script_actor.cpp index b4a176121a56..2e2a92b3ecda 100644 --- a/ydb/core/kqp/finalize_script_service/kqp_finalize_script_actor.cpp +++ b/ydb/core/kqp/finalize_script_service/kqp_finalize_script_actor.cpp @@ -86,7 +86,7 @@ class TScriptFinalizerActor : public TActorBootstrapped { } void FetchSecrets() { - RegisterDescribeSecretsActor(SelfId(), UserToken, SecretNames, ActorContext().ActorSystem()); + RegisterDescribeSecretsActor(SelfId(), UserToken, Database, SecretNames, ActorContext().ActorSystem()); } void Handle(TEvDescribeSecretsResponse::TPtr& ev) { @@ -226,7 +226,7 @@ class TScriptFinalizerActor : public TActorBootstrapped { TString CustomerSuppliedId; std::vector Sinks; - TString UserToken; + TIntrusiveConstPtr UserToken; std::vector SecretNames; std::unordered_map SecureParams; }; diff --git a/ydb/core/kqp/gateway/behaviour/external_data_source/manager.cpp b/ydb/core/kqp/gateway/behaviour/external_data_source/manager.cpp index d325abd4ced3..e16638765213 100644 --- a/ydb/core/kqp/gateway/behaviour/external_data_source/manager.cpp +++ b/ydb/core/kqp/gateway/behaviour/external_data_source/manager.cpp @@ -24,8 +24,13 @@ using TYqlConclusion = TConclusionImpl; TAsyncStatus ValidateExternalDatasourceSecrets(const NKikimrSchemeOp::TExternalDataSourceDescription& externalDataSourceDesc, const TExternalDataSourceManager::TInternalModificationContext& context) { const auto& externalData = context.GetExternalData(); - const auto& userToken = externalData.GetUserToken(); - auto describeFuture = DescribeExternalDataSourceSecrets(externalDataSourceDesc.GetAuth(), userToken ? userToken->GetUserSID() : "", externalData.GetActorSystem()); + const std::optional& userToken = externalData.GetUserToken(); + auto describeFuture = DescribeExternalDataSourceSecrets( + externalDataSourceDesc.GetAuth(), + userToken ? new NACLib::TUserToken(*userToken) : nullptr, + externalData.GetDatabase(), + externalData.GetActorSystem() + ); return describeFuture.Apply([](const NThreading::TFuture& f) { if (const auto& value = f.GetValue(); value.Status != Ydb::StatusIds::SUCCESS) { diff --git a/ydb/core/kqp/gateway/kqp_metadata_loader.cpp b/ydb/core/kqp/gateway/kqp_metadata_loader.cpp index aa5046e6966f..0dbb0fca2ca0 100644 --- a/ydb/core/kqp/gateway/kqp_metadata_loader.cpp +++ b/ydb/core/kqp/gateway/kqp_metadata_loader.cpp @@ -572,9 +572,14 @@ void UpdateExternalDataSourceSecretsValue(TTableMetadataResult& externalDataSour } } -NThreading::TFuture LoadExternalDataSourceSecretValues(const NSchemeCache::TSchemeCacheNavigate::TEntry& entry, const TIntrusiveConstPtr& userToken, TActorSystem* actorSystem) { +NThreading::TFuture LoadExternalDataSourceSecretValues( + const NSchemeCache::TSchemeCacheNavigate::TEntry& entry, + const TIntrusiveConstPtr& userToken, + const TString& database, + TActorSystem* actorSystem +) { const auto& authDescription = entry.ExternalDataSourceInfo->Description.GetAuth(); - return DescribeExternalDataSourceSecrets(authDescription, userToken ? userToken->GetUserSID() : "", actorSystem); + return DescribeExternalDataSourceSecrets(authDescription, userToken, database, actorSystem); } } // anonymous namespace @@ -976,7 +981,7 @@ NThreading::TFuture TKqpTableMetadataLoader::LoadTableMeta if (externalPath) { externalDataSourceMetadata.Metadata->ExternalSource.TableLocation = *externalPath; } - LoadExternalDataSourceSecretValues(entry, userToken, ActorSystem) + LoadExternalDataSourceSecretValues(entry, userToken, database, ActorSystem) .Subscribe([promise, externalDataSourceMetadata, settings, table, database, externalPath, this](const TFuture& result) mutable { UpdateExternalDataSourceSecretsValue(externalDataSourceMetadata, result.GetValue()); diff --git a/ydb/core/kqp/provider/yql_kikimr_gateway_ut.cpp b/ydb/core/kqp/provider/yql_kikimr_gateway_ut.cpp index b7b21caf3c25..eac754a219be 100644 --- a/ydb/core/kqp/provider/yql_kikimr_gateway_ut.cpp +++ b/ydb/core/kqp/provider/yql_kikimr_gateway_ut.cpp @@ -428,7 +428,17 @@ Y_UNIT_TEST_SUITE(KikimrIcGateway) { UNIT_ASSERT_C(createSecretQueryResult.GetStatus() == NYdb::EStatus::SUCCESS, createSecretQueryResult.GetIssues().ToString()); } - Y_UNIT_TEST_TWIN(TestLoadServiceAccountSecretValueFromExternalDataSourceMetadata, UseSchemaSecrets) { + template + void CreateSecret(TString& secretName, const TString& secretValue, TSession& session) { + if constexpr (UseSchemaSecrets) { + secretName = "/Root/" + secretName; + CreateSchemaSecret(secretName, secretValue, session); + } else { + CreateSecretObject(secretName, secretValue, session); + } + } + + Y_UNIT_TEST_QUAD(TestLoadServiceAccountSecretValueFromExternalDataSourceMetadata, UseSchemaSecrets, UseAuthToken) { NKikimrConfig::TAppConfig appCfg; appCfg.MutableQueryServiceConfig()->AddAvailableExternalDataSources("ObjectStorage"); TKikimrRunner kikimr{ NKqp::TKikimrSettings(appCfg) }; @@ -436,18 +446,12 @@ Y_UNIT_TEST_SUITE(KikimrIcGateway) { if (UseSchemaSecrets) { kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); } - auto db = kikimr.GetTableClient(); + auto db = UseAuthToken ? kikimr.GetTableClient(NYdb::NTable::TClientSettings().AuthToken("root@builtin")) : kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); - TString secretId; + TString secretId = "mySaSecretId"; TString secretValue = "mySaSecretValue"; - if (UseSchemaSecrets) { - secretId = "/Root/mySaSecretId"; - CreateSchemaSecret(secretId, secretValue, session); - } else { - secretId = "mySaSecretId"; - CreateSecretObject(secretId, secretValue, session); - } + CreateSecret(secretId, secretValue, session); TString externalDataSourceName = "/Root/ExternalDataSource"; TString externalTableName = "/Root/ExternalTable"; @@ -469,7 +473,11 @@ Y_UNIT_TEST_SUITE(KikimrIcGateway) { auto result = session.ExecuteSchemeQuery(query).GetValueSync(); UNIT_ASSERT_C(result.GetStatus() == NYdb::EStatus::SUCCESS, result.GetIssues().ToString()); - auto responseFuture = GetIcGateway(kikimr.GetTestServer())->LoadTableMetadata(TestCluster, externalTableName, IKikimrGateway::TLoadTableMetadataSettings()); + auto gateway = GetIcGateway(kikimr.GetTestServer()); + if (UseAuthToken) { + gateway->SetToken(TestCluster, new NACLib::TUserToken("root@builtin", {})); + } + auto responseFuture = gateway->LoadTableMetadata(TestCluster, externalTableName, IKikimrGateway::TLoadTableMetadataSettings()); responseFuture.Wait(); auto response = responseFuture.GetValue(); @@ -488,15 +496,9 @@ Y_UNIT_TEST_SUITE(KikimrIcGateway) { auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); - TString secretId; + TString secretId = "myPasswordSecretId"; TString secretValue = "pswd"; - if (UseSchemaSecrets) { - secretId = "/Root/myPasswordSecretId"; - CreateSchemaSecret(secretId, secretValue, session); - } else { - secretId = "myPasswordSecretId"; - CreateSecretObject(secretId, secretValue, session); - } + CreateSecret(secretId, secretValue, session); TString externalDataSourceName = "/Root/ExternalDataSource"; auto query = TStringBuilder() << R"( @@ -519,21 +521,24 @@ Y_UNIT_TEST_SUITE(KikimrIcGateway) { UNIT_ASSERT_VALUES_EQUAL(response.Metadata->ExternalSource.Password, secretValue); } - Y_UNIT_TEST(TestLoadMdbBasicSecretValueFromExternalDataSourceMetadata) { + Y_UNIT_TEST_TWIN(TestLoadMdbBasicSecretValueFromExternalDataSourceMetadata, UseSchemaSecrets) { NKikimrConfig::TAppConfig appCfg; appCfg.MutableQueryServiceConfig()->AddAvailableExternalDataSources("PostgreSQL"); TKikimrRunner kikimr{ NKqp::TKikimrSettings(appCfg) }; kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableExternalDataSources(true); + if (UseSchemaSecrets) { + kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); + } auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); TString secretPasswordId = "myPasswordSecretId"; TString secretPasswordValue = "pswd"; - CreateSecretObject(secretPasswordId, secretPasswordValue, session); + CreateSecret(secretPasswordId, secretPasswordValue, session); TString secretSaId = "mySa"; TString secretSaValue = "sign(mySa)"; - CreateSecretObject(secretSaId, secretSaValue, session); + CreateSecret(secretSaId, secretSaValue, session); TString externalDataSourceName = "/Root/ExternalDataSource"; auto query = TStringBuilder() << R"( @@ -559,21 +564,24 @@ Y_UNIT_TEST_SUITE(KikimrIcGateway) { UNIT_ASSERT_VALUES_EQUAL(response.Metadata->ExternalSource.ServiceAccountIdSignature, secretSaValue); } - Y_UNIT_TEST(TestLoadAwsSecretValueFromExternalDataSourceMetadata) { + Y_UNIT_TEST_TWIN(TestLoadAwsSecretValueFromExternalDataSourceMetadata, UseSchemaSecrets) { NKikimrConfig::TAppConfig appCfg; appCfg.MutableQueryServiceConfig()->AddAvailableExternalDataSources("ObjectStorage"); TKikimrRunner kikimr{ NKqp::TKikimrSettings(appCfg) }; kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableExternalDataSources(true); + if (UseSchemaSecrets) { + kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); + } auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); TString awsAccessKeyIdSecretId = "awsAccessKeyIdSecretId"; TString awsAccessKeyIdSecretValue = "key"; - CreateSecretObject(awsAccessKeyIdSecretId, awsAccessKeyIdSecretValue, session); + CreateSecret(awsAccessKeyIdSecretId, awsAccessKeyIdSecretValue, session); TString awsSecretAccessKeySecretId = "awsSecretAccessKeySecretId"; TString awsSecretAccessKeySecretValue = "value"; - CreateSecretObject(awsSecretAccessKeySecretId, awsSecretAccessKeySecretValue, session); + CreateSecret(awsSecretAccessKeySecretId, awsSecretAccessKeySecretValue, session); TString externalDataSourceName = "/Root/ExternalDataSource"; auto query = TStringBuilder() << R"( @@ -598,21 +606,24 @@ Y_UNIT_TEST_SUITE(KikimrIcGateway) { UNIT_ASSERT_VALUES_EQUAL(response.Metadata->ExternalSource.DataSourceAuth.GetAws().GetAwsRegion(), "ru-central-1"); } - Y_UNIT_TEST(TestLoadDataSourceProperties) { + Y_UNIT_TEST_TWIN(TestLoadDataSourceProperties, UseSchemaSecrets) { NKikimrConfig::TAppConfig appCfg; appCfg.MutableQueryServiceConfig()->AddAvailableExternalDataSources("PostgreSQL"); TKikimrRunner kikimr{ NKqp::TKikimrSettings(appCfg) }; kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableExternalDataSources(true); + if (UseSchemaSecrets) { + kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); + } auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); TString secretPasswordId = "myPasswordSecretId"; TString secretPasswordValue = "pswd"; - CreateSecretObject(secretPasswordId, secretPasswordValue, session); + CreateSecret(secretPasswordId, secretPasswordValue, session); TString secretSaId = "mySa"; TString secretSaValue = "sign(mySa)"; - CreateSecretObject(secretSaId, secretSaValue, session); + CreateSecret(secretSaId, secretSaValue, session); TString externalDataSourceName = "/Root/ExternalDataSource"; auto query = TStringBuilder() << R"( @@ -659,15 +670,9 @@ Y_UNIT_TEST_SUITE(KikimrIcGateway) { auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); - TString secretTokenId; + TString secretTokenId = "myTokenSecretId"; TString secretTokenValue = "token"; - if (UseSchemaSecrets) { - secretTokenId = "/Root/myTokenSecretId"; - CreateSchemaSecret(secretTokenId, secretTokenValue, session); - } else { - secretTokenId = "myTokenSecretId"; - CreateSecretObject(secretTokenId, secretTokenValue, session); - } + CreateSecret(secretTokenId, secretTokenValue, session); TString externalDataSourceName = "/Root/ExternalDataSource"; auto query = TStringBuilder() << R"( @@ -689,15 +694,18 @@ Y_UNIT_TEST_SUITE(KikimrIcGateway) { UNIT_ASSERT_VALUES_EQUAL(response.Metadata->ExternalSource.Properties.GetProperties().size(), 0); } - Y_UNIT_TEST(TestSecretsExistingValidation) { + Y_UNIT_TEST_TWIN(TestSecretsExistingValidation, UseSchemaSecrets) { NKikimrConfig::TAppConfig appCfg; appCfg.MutableQueryServiceConfig()->AddAvailableExternalDataSources("ObjectStorage"); TKikimrRunner kikimr{ NKqp::TKikimrSettings(appCfg) }; kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableExternalDataSources(true); + if (UseSchemaSecrets) { + kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); + } auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); - TString secretId = "unexisting_secret_name"; + TString secretId = UseSchemaSecrets ? "/Root/unexisting_secret_name" : "unexisting_secret_name"; auto query = TStringBuilder() << R"( CREATE EXTERNAL DATA SOURCE `/Root/ExternalDataSource` WITH ( SOURCE_TYPE="YT", @@ -708,7 +716,11 @@ Y_UNIT_TEST_SUITE(KikimrIcGateway) { auto result = session.ExecuteSchemeQuery(query).GetValueSync(); UNIT_ASSERT_C(result.GetStatus() == NYdb::EStatus::BAD_REQUEST, result.GetIssues().ToString()); - UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToOneLineString(), TStringBuilder() << "secret with name '" << secretId << "' not found"); + if (UseSchemaSecrets) { + UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToOneLineString(), TStringBuilder() << "secret `" << secretId << "` not found"); + } else { + UNIT_ASSERT_STRING_CONTAINS(result.GetIssues().ToOneLineString(), TStringBuilder() << "secret with name '" << secretId << "' not found"); + } } Y_UNIT_TEST(TestCreateResourcePool) { diff --git a/ydb/core/kqp/proxy_service/kqp_script_executions.cpp b/ydb/core/kqp/proxy_service/kqp_script_executions.cpp index 54f563111f89..292a56c3e58a 100644 --- a/ydb/core/kqp/proxy_service/kqp_script_executions.cpp +++ b/ydb/core/kqp/proxy_service/kqp_script_executions.cpp @@ -115,6 +115,24 @@ void TimestampToProtoWithSaturation(google::protobuf::Timestamp* proto, TInstant ); } +bool GetUserGroupSids(const std::string& serializedUserGroupSids, TVector& userGroupSids) { + NJson::TJsonValue value; + if (!NJson::ReadJsonTree(serializedUserGroupSids, &value) || value.GetType() != NJson::JSON_ARRAY) { + return false; + } + + const auto sidsSize = value.GetIntegerRobust(); + userGroupSids.reserve(sidsSize); + for (i64 i = 0; i < sidsSize; ++i) { + const NJson::TJsonValue* userSid = nullptr; + value.GetValuePointer(i, &userSid); + Y_ENSURE(userSid); + + userGroupSids.emplace_back(userSid->GetString()); + } + return true; +} + class TQueryBase : public NKikimr::TQueryBase { public: struct TSettings { @@ -974,22 +992,10 @@ class TRestartScriptOperationQuery : public TQueryBase { TVector userGroupSids; if (const auto serializedUserGroupSids = result.ColumnParser("user_group_sids").GetOptionalJsonDocument()) { - NJson::TJsonValue value; - if (!NJson::ReadJsonTree(*serializedUserGroupSids, &value) || value.GetType() != NJson::JSON_ARRAY) { + if (!GetUserGroupSids(*serializedUserGroupSids, userGroupSids)) { Finish(Ydb::StatusIds::INTERNAL_ERROR, "User group sids are corrupted"); return; } - - const auto sidsSize = value.GetIntegerRobust(); - - userGroupSids.reserve(sidsSize); - for (i64 i = 0; i < sidsSize; ++i) { - const NJson::TJsonValue* userSid = nullptr; - value.GetValuePointer(i, &userSid); - Y_ENSURE(userSid); - - userGroupSids.emplace_back(userSid->GetString()); - } } queryRequest.SetUserToken(NACLib::TUserToken( @@ -3735,6 +3741,7 @@ class TSaveScriptFinalStatusActor : public TQueryBase { meta, customer_supplied_id, user_token, + user_group_sids, script_sinks, script_secret_names, retry_state, @@ -3798,7 +3805,14 @@ class TSaveScriptFinalStatusActor : public TQueryBase { } if (const auto userToken = result.ColumnParser("user_token").GetOptionalUtf8()) { - Response->UserToken = *userToken; + TVector userGroupSids; + if (const auto serializedUserGroupSids = result.ColumnParser("user_group_sids").GetOptionalJsonDocument()) { + if (!GetUserGroupSids(*serializedUserGroupSids, userGroupSids)) { + Finish(Ydb::StatusIds::INTERNAL_ERROR, "User group sids are corrupted"); + return; + } + } + Response->UserToken = new NACLib::TUserToken(TString(*userToken), userGroupSids); } if (SerializedSinks = result.ColumnParser("script_sinks").GetOptionalJsonDocument()) { diff --git a/ydb/core/kqp/ut/common/kqp_ut_common.cpp b/ydb/core/kqp/ut/common/kqp_ut_common.cpp index d43e2dce4356..ca0b03cdf536 100644 --- a/ydb/core/kqp/ut/common/kqp_ut_common.cpp +++ b/ydb/core/kqp/ut/common/kqp_ut_common.cpp @@ -211,6 +211,10 @@ TKikimrRunner::TKikimrRunner(const TKikimrSettings& settings) { ServerSettings->SetFederatedQuerySetupFactory(settings.FederatedQuerySetupFactory); } + if (settings.DescribeSchemaSecretsServiceFactory) { + ServerSettings->SetDescribeSchemaSecretsServiceFactory(settings.DescribeSchemaSecretsServiceFactory); + } + Server.Reset(MakeHolder(*ServerSettings)); if (settings.GrpcServerOptions) { diff --git a/ydb/core/kqp/ut/common/kqp_ut_common.h b/ydb/core/kqp/ut/common/kqp_ut_common.h index c31af35a91b9..776b6d76891b 100644 --- a/ydb/core/kqp/ut/common/kqp_ut_common.h +++ b/ydb/core/kqp/ut/common/kqp_ut_common.h @@ -81,6 +81,7 @@ struct TKikimrSettings: public TTestFeatureFlagsHolder { TMaybe Storage = Nothing(); bool InitFederatedQuerySetupFactory = false; NKqp::IKqpFederatedQuerySetupFactory::TPtr FederatedQuerySetupFactory = std::make_shared(); + NKqp::IDescribeSchemaSecretsServiceFactory::TPtr DescribeSchemaSecretsServiceFactory = std::make_shared(); NMonitoring::TDynamicCounterPtr CountersRoot = MakeIntrusive(); std::shared_ptr S3ActorsFactory = NYql::NDq::CreateDefaultS3ActorsFactory(); NKikimrConfig::TImmediateControlsConfig Controls; @@ -109,6 +110,7 @@ struct TKikimrSettings: public TTestFeatureFlagsHolder { TKikimrSettings& SetStorage(const NFake::TStorage& storage) { Storage = storage; return *this; }; TKikimrSettings& SetInitFederatedQuerySetupFactory(bool value) { InitFederatedQuerySetupFactory = value; return *this; }; TKikimrSettings& SetFederatedQuerySetupFactory(NKqp::IKqpFederatedQuerySetupFactory::TPtr value) { FederatedQuerySetupFactory = value; return *this; }; + TKikimrSettings& SetDescribeSchemaSecretsServiceFactory(NKqp::IDescribeSchemaSecretsServiceFactory::TPtr value) { DescribeSchemaSecretsServiceFactory = value; return *this; }; TKikimrSettings& SetUseRealThreads(bool value) { UseRealThreads = value; return *this; }; TKikimrSettings& SetEnableForceFollowers(bool value) { EnableForceFollowers = value; return *this; }; TKikimrSettings& SetS3ActorsFactory(std::shared_ptr value) { S3ActorsFactory = std::move(value); return *this; }; diff --git a/ydb/core/kqp/ut/federated_query/datastreams/datastreams_ut.cpp b/ydb/core/kqp/ut/federated_query/datastreams/datastreams_ut.cpp index a541bcac547d..6c540685cab1 100644 --- a/ydb/core/kqp/ut/federated_query/datastreams/datastreams_ut.cpp +++ b/ydb/core/kqp/ut/federated_query/datastreams/datastreams_ut.cpp @@ -86,6 +86,7 @@ class TStreamingTestFixture : public NUnitTest::TBaseFixture { runtime.SetLogPriority(NKikimrServices::STREAMS_STORAGE_SERVICE, NLog::PRI_DEBUG); runtime.SetLogPriority(NKikimrServices::STREAMS_CHECKPOINT_COORDINATOR, NLog::PRI_DEBUG); } + Kikimr->GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(UseSchemaSecrets()); } return Kikimr; @@ -263,9 +264,23 @@ class TStreamingTestFixture : public NUnitTest::TBaseFixture { )); } - void CreatePqSourceBasicAuth(const TString& pqSourceName) { + void CreatePqSourceBasicAuth(const TString& pqSourceName, const bool useSchemaSecrets = false) { + const TString secretName = useSchemaSecrets ? "/Root/secret_local_password" : "secret_local_password"; + if (useSchemaSecrets) { + ExecQuery(fmt::format(R"( + CREATE SECRET `{secret_name}` WITH (value = "1234"); + )", + "secret_name"_a = secretName + )); + } else { + ExecQuery(fmt::format(R"( + CREATE OBJECT `{secret_name}` (TYPE SECRET) WITH (value = "1234"); + )", + "secret_name"_a = secretName + )); + } + ExecQuery(fmt::format(R"( - CREATE OBJECT secret_local_password (TYPE SECRET) WITH (value = "1234"); CREATE EXTERNAL DATA SOURCE `{pq_source}` WITH ( SOURCE_TYPE = "Ydb", LOCATION = "{pq_location}", @@ -493,6 +508,10 @@ class TStreamingTestFixture : public NUnitTest::TBaseFixture { UNIT_ASSERT_C(!Kikimr, "Kikimr runner is already initialized, can not setup " << info); } + virtual bool UseSchemaSecrets() { + return false; + } + private: std::optional AppConfig; TIntrusivePtr PqGateway; @@ -509,6 +528,13 @@ class TStreamingTestFixture : public NUnitTest::TBaseFixture { std::shared_ptr TopicClient; }; +class TStreamingWithSchemaSecretsTestFixture : public TStreamingTestFixture { +public: + bool UseSchemaSecrets() override { + return true; + } +}; + } // anonymous namespace Y_UNIT_TEST_SUITE(KqpFederatedQueryDatastreams) { @@ -625,35 +651,36 @@ Y_UNIT_TEST_SUITE(KqpFederatedQueryDatastreams) { }); } - Y_UNIT_TEST_F(ReadTopicBasic, TStreamingTestFixture) { - TString sourceName = "sourceName"; - TString topicName = "topicName"; - TString tableName = "tableName"; - - CreateTopic(topicName); + Y_UNIT_TEST_F(ReadTopicBasic, TStreamingWithSchemaSecretsTestFixture) { + for (int i = 0; i < 2; ++i) { + const bool useSchemaSecrets = static_cast(i); + const TString sourceName = "sourceName" + ToString(i); + const TString topicName = "topicName" + ToString(i); + CreateTopic(topicName); - CreatePqSourceBasicAuth(sourceName); + CreatePqSourceBasicAuth(sourceName, useSchemaSecrets); - const auto scriptExecutionOperation = ExecScript(fmt::format(R"( - SELECT * FROM `{source}`.`{topic}` - WITH ( - FORMAT="json_each_row", - SCHEMA=( - key String NOT NULL, - value String NOT NULL - )) - LIMIT 1; - )", - "source"_a=sourceName, - "topic"_a=topicName - )); + const auto scriptExecutionOperation = ExecScript(fmt::format(R"( + SELECT * FROM `{source}`.`{topic}` + WITH ( + FORMAT="json_each_row", + SCHEMA=( + key String NOT NULL, + value String NOT NULL + )) + LIMIT 1; + )", + "source"_a=sourceName, + "topic"_a=topicName + )); - WriteTopicMessage(topicName, R"({"key": "key1", "value": "value1"})"); + WriteTopicMessage(topicName, R"({"key": "key1", "value": "value1"})"); - CheckScriptResult(scriptExecutionOperation, 2, 1, [](TResultSetParser& result) { - UNIT_ASSERT_VALUES_EQUAL(result.ColumnParser(0).GetString(), "key1"); - UNIT_ASSERT_VALUES_EQUAL(result.ColumnParser(1).GetString(), "value1"); - }); + CheckScriptResult(scriptExecutionOperation, 2, 1, [](TResultSetParser& result) { + UNIT_ASSERT_VALUES_EQUAL(result.ColumnParser(0).GetString(), "key1"); + UNIT_ASSERT_VALUES_EQUAL(result.ColumnParser(1).GetString(), "value1"); + }); + } } Y_UNIT_TEST_F(InsertTopicBasic, TStreamingTestFixture) { diff --git a/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp b/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp index b9c783814f64..943b7c87a0e3 100644 --- a/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp +++ b/ydb/core/kqp/ut/scheme/kqp_scheme_ut.cpp @@ -52,6 +52,19 @@ TStatus ExecuteGeneric(NYdb::NQuery::TQueryClient& queryClient, TSession& sessio } } +template +void CreateSecret(TString& secretName, const TString& secretValue, TSession& session) { + TString query; + if constexpr (UseSchemaSecrets) { + secretName = "/Root/" + secretName; + query = Sprintf("CREATE SECRET `%s` WITH (value=\"%s\")", secretName.c_str(), secretValue.c_str()); + } else { + query = Sprintf("CREATE OBJECT %s (TYPE SECRET) WITH value=\"%s\"", secretName.c_str(), secretValue.c_str()); + } + const auto queryResult = session.ExecuteSchemeQuery(query).GetValueSync(); + UNIT_ASSERT_EQUAL_C(NYdb::EStatus::SUCCESS, queryResult.GetStatus(), queryResult.GetIssues().ToString()); +} + } Y_UNIT_TEST_SUITE(KqpScheme) { @@ -9156,10 +9169,13 @@ Y_UNIT_TEST_SUITE(KqpScheme) { } } - Y_UNIT_TEST(CreateAsyncReplicationWithTokenSecret) { + Y_UNIT_TEST_TWIN(CreateAsyncReplicationWithTokenSecret, UseSchemaSecrets) { using namespace NReplication; TKikimrRunner kikimr("root@builtin"); + if (UseSchemaSecrets) { + kikimr.GetTestServer().GetRuntime()->GetAppData(0).FeatureFlags.SetEnableSchemaSecrets(true); + } auto repl = TReplicationClient(kikimr.GetDriver(), TCommonClientSettings().Database("/Root")); auto db = kikimr.GetTableClient(); auto session = db.CreateSession().GetValueSync().GetSession(); @@ -9180,17 +9196,20 @@ Y_UNIT_TEST_SUITE(KqpScheme) { // ok { + TString secretId = "mysecretname"; + const TString secretValue = "root@builtin"; + CreateSecret(secretId, secretValue, session); + auto query = Sprintf(R"( --!syntax_v1 - CREATE OBJECT mysecret (TYPE SECRET) WITH (value = "root@builtin"); CREATE ASYNC REPLICATION `/Root/replication` FOR `/Root/table` AS `/Root/replica` WITH ( ENDPOINT = "%s", DATABASE = "/Root", - TOKEN_SECRET_NAME = "mysecret" + TOKEN_SECRET_NAME = "%s" ); - )", kikimr.GetEndpoint().c_str()); + )", kikimr.GetEndpoint().c_str(), secretId.c_str()); const auto result = session.ExecuteSchemeQuery(query).GetValueSync(); UNIT_ASSERT_VALUES_EQUAL_C(result.GetStatus(), EStatus::SUCCESS, result.GetIssues().ToString()); diff --git a/ydb/core/testlib/test_client.cpp b/ydb/core/testlib/test_client.cpp index b2957509149c..618a88062428 100644 --- a/ydb/core/testlib/test_client.cpp +++ b/ydb/core/testlib/test_client.cpp @@ -1284,7 +1284,7 @@ namespace Tests { Runtime->RegisterService(MakeDatabaseMetadataCacheId(Runtime->GetNodeId(nodeIdx)), metadataCacheId, nodeIdx); } { - IActor* describeSchemaSecretsService = NKqp::CreateDescribeSchemaSecretsService(); + IActor* describeSchemaSecretsService = Settings->DescribeSchemaSecretsServiceFactory->CreateService(); TActorId describeSchemaSecretsServiceId = Runtime->Register(describeSchemaSecretsService, nodeIdx, userPoolId); Runtime->RegisterService(NKqp::MakeKqpDescribeSchemaSecretServiceId(Runtime->GetNodeId(nodeIdx)), describeSchemaSecretsServiceId, nodeIdx); } diff --git a/ydb/core/testlib/test_client.h b/ydb/core/testlib/test_client.h index 2aab9628311a..4d33607f7ef1 100644 --- a/ydb/core/testlib/test_client.h +++ b/ydb/core/testlib/test_client.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -172,6 +173,7 @@ namespace Tests { TString MeteringFilePath; TString AwsRegion; NKqp::IKqpFederatedQuerySetupFactory::TPtr FederatedQuerySetupFactory = std::make_shared(); + NKqp::IDescribeSchemaSecretsServiceFactory::TPtr DescribeSchemaSecretsServiceFactory = std::make_shared(); NYql::ISecuredServiceAccountCredentialsFactory::TPtr CredentialsFactory; NMiniKQL::TComputationNodeFactory ComputationFactory; NYql::IYtGateway::TPtr YtGateway; @@ -238,6 +240,7 @@ namespace Tests { TServerSettings& SetMeteringFilePath(const TString& path) { EnableMetering = true; MeteringFilePath = path; return *this; } TServerSettings& SetAwsRegion(const TString& value) { AwsRegion = value; return *this; } TServerSettings& SetFederatedQuerySetupFactory(NKqp::IKqpFederatedQuerySetupFactory::TPtr value) { FederatedQuerySetupFactory = value; return *this; } + TServerSettings& SetDescribeSchemaSecretsServiceFactory(NKqp::IDescribeSchemaSecretsServiceFactory::TPtr value) { DescribeSchemaSecretsServiceFactory = value; return *this; } TServerSettings& SetCredentialsFactory(NYql::ISecuredServiceAccountCredentialsFactory::TPtr credentialsFactory) { CredentialsFactory = std::move(credentialsFactory); return *this; } TServerSettings& SetComputationFactory(NMiniKQL::TComputationNodeFactory computationFactory) { ComputationFactory = std::move(computationFactory); return *this; } TServerSettings& SetYtGateway(NYql::IYtGateway::TPtr ytGateway) { YtGateway = std::move(ytGateway); return *this; } diff --git a/ydb/core/tx/replication/controller/replication.cpp b/ydb/core/tx/replication/controller/replication.cpp index fc2e25c8136e..87eace2e7328 100644 --- a/ydb/core/tx/replication/controller/replication.cpp +++ b/ydb/core/tx/replication/controller/replication.cpp @@ -42,7 +42,7 @@ class TReplication::TImpl: public TLagProvider { return; } - SecretResolver = ctx.Register(CreateSecretResolver(ctx.SelfID, ReplicationId, PathId, secretName, ++SecretResolverCookie)); + SecretResolver = ctx.Register(CreateSecretResolver(ctx.SelfID, ReplicationId, PathId, secretName, ++SecretResolverCookie, Database)); } ui64 GetExpectedSecretResolverCookie() const { diff --git a/ydb/core/tx/replication/controller/secret_resolver.cpp b/ydb/core/tx/replication/controller/secret_resolver.cpp index b04b1d152605..c1b8376275f8 100644 --- a/ydb/core/tx/replication/controller/secret_resolver.cpp +++ b/ydb/core/tx/replication/controller/secret_resolver.cpp @@ -2,6 +2,8 @@ #include "private_events.h" #include "secret_resolver.h" +#include +#include #include #include #include @@ -10,6 +12,8 @@ #include #include +#include + namespace NKikimr::NReplication::NController { class TSecretResolver: public TActorBootstrapped { @@ -40,8 +44,20 @@ class TSecretResolver: public TActorBootstrapped { } SecretId = NMetadata::NSecret::TSecretId(entry.SecurityObject->GetOwnerSID(), SecretName); - Send(NMetadata::NProvider::MakeServiceId(SelfId().NodeId()), - new NMetadata::NProvider::TEvAskSnapshot(SnapshotFetcher())); + if (NKqp::UseSchemaSecrets(AppData()->FeatureFlags, SecretId.GetSecretId())) { + const TVector secretNames{SecretId.GetSecretId()}; + auto userToken = MakeIntrusiveConst(entry.SecurityObject->GetOwnerSID(), TVector()); + const auto actorSystem = ActorContext().ActorSystem(); + const auto replyActorId = SelfId(); + auto future = NKqp::DescribeSecret(secretNames, userToken, Database, actorSystem); + future.Subscribe([actorSystem, replyActorId](const NThreading::TFuture& result) { + actorSystem->Send(replyActorId, new NKqp::TEvDescribeSecretsResponse(result.GetValue())); + }); + return; + } else { + Send(NMetadata::NProvider::MakeServiceId(SelfId().NodeId()), + new NMetadata::NProvider::TEvAskSnapshot(SnapshotFetcher())); + } } void Handle(NMetadata::NProvider::TEvRefreshSubscriberData::TPtr& ev) { @@ -55,6 +71,15 @@ class TSecretResolver: public TActorBootstrapped { Reply(secretValue.DetachResult()); } + void Handle(NKqp::TEvDescribeSecretsResponse::TPtr& ev) { + if (ev->Get()->Description.Status != Ydb::StatusIds::SUCCESS) { + return Reply(false, ev->Get()->Description.Issues.ToOneLineString()); + } + + Y_ENSURE(ev->Get()->Description.SecretValues.size() == 1); + Reply(ev->Get()->Description.SecretValues[0]); + } + template void Reply(Args&&... args) { Send(Parent, new TEvPrivate::TEvResolveSecretResult(ReplicationId, std::forward(args)...), 0, Cookie); @@ -66,12 +91,13 @@ class TSecretResolver: public TActorBootstrapped { return NKikimrServices::TActivity::REPLICATION_CONTROLLER_SECRET_RESOLVER; } - explicit TSecretResolver(const TActorId& parent, ui64 rid, const TPathId& pathId, const TString& secretName, const ui64 cookie) + explicit TSecretResolver(const TActorId& parent, ui64 rid, const TPathId& pathId, const TString& secretName, const ui64 cookie, const TString& database) : Parent(parent) , ReplicationId(rid) , PathId(pathId) , SecretName(secretName) , Cookie(cookie) + , Database(database) , LogPrefix("SecretResolver", ReplicationId) { } @@ -97,6 +123,7 @@ class TSecretResolver: public TActorBootstrapped { switch (ev->GetTypeRewrite()) { hFunc(TEvTxProxySchemeCache::TEvNavigateKeySetResult, Handle); hFunc(NMetadata::NProvider::TEvRefreshSubscriberData, Handle); + hFunc(NKqp::TEvDescribeSecretsResponse, Handle); sFunc(TEvents::TEvWakeup, Bootstrap); sFunc(TEvents::TEvPoison, PassAway); } @@ -108,6 +135,7 @@ class TSecretResolver: public TActorBootstrapped { const TPathId PathId; const TString SecretName; const ui64 Cookie; + const TString Database; const TActorLogPrefix LogPrefix; static constexpr auto RetryInterval = TDuration::Seconds(1); @@ -115,8 +143,8 @@ class TSecretResolver: public TActorBootstrapped { }; // TSecretResolver -IActor* CreateSecretResolver(const TActorId& parent, ui64 rid, const TPathId& pathId, const TString& secretName, const ui64 cookie) { - return new TSecretResolver(parent, rid, pathId, secretName, cookie); +IActor* CreateSecretResolver(const TActorId& parent, ui64 rid, const TPathId& pathId, const TString& secretName, const ui64 cookie, const TString& database) { + return new TSecretResolver(parent, rid, pathId, secretName, cookie, database); } } diff --git a/ydb/core/tx/replication/controller/secret_resolver.h b/ydb/core/tx/replication/controller/secret_resolver.h index dd3b0af55c43..fa6809101584 100644 --- a/ydb/core/tx/replication/controller/secret_resolver.h +++ b/ydb/core/tx/replication/controller/secret_resolver.h @@ -4,6 +4,6 @@ namespace NKikimr::NReplication::NController { -IActor* CreateSecretResolver(const TActorId& parent, ui64 rid, const TPathId& pathId, const TString& secretName, const ui64 cookie); +IActor* CreateSecretResolver(const TActorId& parent, ui64 rid, const TPathId& pathId, const TString& secretName, const ui64 cookie, const TString& database); } diff --git a/ydb/core/tx/replication/controller/ya.make b/ydb/core/tx/replication/controller/ya.make index 5c9eb00c2de6..d483b7335b07 100644 --- a/ydb/core/tx/replication/controller/ya.make +++ b/ydb/core/tx/replication/controller/ya.make @@ -4,6 +4,8 @@ PEERDIR( ydb/core/base ydb/core/discovery ydb/core/engine/minikql + ydb/core/kqp/common/events + ydb/core/kqp/federated_query ydb/core/protos ydb/core/tablet ydb/core/tablet_flat diff --git a/ydb/core/tx/schemeshard/schemeshard__operation_create_secret.cpp b/ydb/core/tx/schemeshard/schemeshard__operation_create_secret.cpp index 84b056b8941f..ba3c4264f08c 100644 --- a/ydb/core/tx/schemeshard/schemeshard__operation_create_secret.cpp +++ b/ydb/core/tx/schemeshard/schemeshard__operation_create_secret.cpp @@ -12,12 +12,8 @@ namespace { using namespace NKikimr; using namespace NSchemeShard; -TString InterruptInheritanceExceptDescribe(const TString& parentAcl, const bool inheritPermissions) { - if (inheritPermissions) { // don't change parent ACL - return parentAcl; - } - - NACLib::TACL secObj(parentAcl); +TString InterruptInheritanceExceptDescribe(const TString& initialAcl) { + NACLib::TACL secObj(initialAcl); NACLib::TACL resultSecObj; resultSecObj.SetInterruptInheritance(true); for (auto& ace : *secObj.MutableACE()) { @@ -250,7 +246,15 @@ class TCreateSecret : public TSubOperation { if (!acl.empty()) { secretPath->ApplyACL(acl); } else { - secretPath->ACL = InterruptInheritanceExceptDescribe(parentPath.GetEffectiveACL(), createSecretProto.GetInheritPermissions()); + /** By default, secrets should not inherit permissions from their parent object, except for the DescribeSchema grant. + * This is done to prevent users from accidentally granting permissions to such sensitive objects. + * However, the DescribeSchema grant is quite harmless and allows users to view the object's ACL to request access from the owner. + * There is also an inherit_permissions flag, which, when set, allows grant inheritance similarly to all other schema objects. + */ + if (!createSecretProto.GetInheritPermissions()) { + secretPath->ACL = InterruptInheritanceExceptDescribe(dstPath.GetEffectiveACL()); + secretPath->ACLVersion++; + } } NKikimrSchemeOp::TSecretDescription secretDescription; diff --git a/ydb/core/tx/schemeshard/ut_secret/ut_secret.cpp b/ydb/core/tx/schemeshard/ut_secret/ut_secret.cpp index 72f3ea47a4ff..ec2d4dbaf1b2 100644 --- a/ydb/core/tx/schemeshard/ut_secret/ut_secret.cpp +++ b/ydb/core/tx/schemeshard/ut_secret/ut_secret.cpp @@ -31,6 +31,50 @@ namespace { opts.SetReturnSecretValue(true); return DescribePath(runtime, path, opts); } + + void AssertHasAccess( + const int directoryId, + const ui32 inheritance, + const bool expectedHasAccess, + TTestBasicRuntime& runtime, + ui64& txId, + TTestEnv& env + ) { + /** This test + * - creates a new directory "/MyRoot/dir" + ToString(directoryId) + * - provide to the user some grants to this directory + * - creates a secret in the new directory with InheritPermissions=True + * - check grants for the secret + */ + const TString user = "some-user"; + const auto userToken = NACLib::TUserToken(NACLib::TUserToken::TUserTokenInitFields{.UserSID = user}); + const TString& workingDir = "/MyRoot"; + + // create container dir + NACLib::TDiffACL diffACL; + diffACL.AddAccess(NACLib::EAccessType::Allow, NACLib::DescribeSchema, user, inheritance); + AsyncModifyACL(runtime, ++txId, workingDir, "dir" + ToString(directoryId), diffACL.SerializeAsString(), /* newOwner */ ""); + env.TestWaitNotification(runtime, txId); + + // create secret + const TString workingDirPath = workingDir + "/dir" + ToString(directoryId); + const TString secretName = "secret-name"; + TestCreateSecret(runtime, ++txId, workingDirPath, + Sprintf(R"( + Name: "%s" + Value: "test-value" + InheritPermissions: false + )", secretName.data()) + ); + env.TestWaitNotification(runtime, txId); + const TString secretPath = workingDirPath + "/" + secretName; + TestLs(runtime, secretPath, false, NLs::PathExist); + + // assert access + const auto describeResult = DescribePath(runtime, secretPath).GetPathDescription().GetSelf(); + const TSecurityObject secObj(describeResult.GetOwner(), describeResult.GetEffectiveACL(), false); + UNIT_ASSERT_VALUES_EQUAL(expectedHasAccess, secObj.CheckAccess(NACLib::DescribeSchema, userToken)); + } } Y_UNIT_TEST_SUITE(TSchemeShardSecretTest) { @@ -205,11 +249,24 @@ Y_UNIT_TEST_SUITE(TSchemeShardSecretTest) { ); env.TestWaitNotification(runtime, txId); - TestDescribeResult(DescribePath(runtime, "/MyRoot/dir/test-secret"), { - NLs::HasRight("+(DS):user1"), NLs::HasEffectiveRight("+(DS):user1"), - NLs::HasRight("+(AS):user1"), NLs::HasEffectiveRight("+(AS):user1"), - NLs::HasRight("-(DS):user2"), NLs::HasEffectiveRight("-(DS):user2"), - NLs::HasRight("+(AS):user2"), NLs::HasEffectiveRight("+(AS):user2")}); + const auto secretDescribePath = DescribePath(runtime, "/MyRoot/dir/test-secret"); + TestDescribeResult(secretDescribePath, { + NLs::HasNoRight("+(DS):user1"), NLs::HasEffectiveRight("+(DS):user1"), + NLs::HasNoRight("+(AS):user1"), NLs::HasEffectiveRight("+(AS):user1"), + NLs::HasNoRight("-(DS):user2"), NLs::HasEffectiveRight("-(DS):user2"), + NLs::HasNoRight("+(AS):user2"), NLs::HasEffectiveRight("+(AS):user2")}); + + auto describeResult = secretDescribePath.GetPathDescription().GetSelf(); + const NACLib::TACL secretAcl(describeResult.GetEffectiveACL()); + const auto parentDescribePath = DescribePath(runtime, "/MyRoot/dir"); + describeResult = parentDescribePath.GetPathDescription().GetSelf(); + const NACLib::TACL parentAcl(describeResult.GetEffectiveACL()); + + // Cannot compare rules themselves because they are actually different: i.e. there's an Inherited=true flag at the secret aces + UNIT_ASSERT_EQUAL_C( + secretAcl.GetACE().size(), parentAcl.GetACE().size(), + "Secret ACL must be inherited, hence the number of rules must should be the same") + ; } Y_UNIT_TEST(CreateSecretNoInheritPermissions) { @@ -281,13 +338,48 @@ Y_UNIT_TEST_SUITE(TSchemeShardSecretTest) { env.TestWaitNotification(runtime, txId); const auto describeResult = DescribePath(runtime, "/MyRoot/dir/subdir/test-secret").GetPathDescription().GetSelf(); - const TSecurityObject secObj(describeResult.GetOwner(), /*isEffective ? self.GetEffectiveACL() :*/ describeResult.GetACL(), false); + const TSecurityObject secObj(describeResult.GetOwner(), describeResult.GetEffectiveACL(), false); const auto user1Token = NACLib::TUserToken(NACLib::TUserToken::TUserTokenInitFields{.UserSID = "user1"}); const auto user2Token = NACLib::TUserToken(NACLib::TUserToken::TUserTokenInitFields{.UserSID = "user2"}); UNIT_ASSERT_C(secObj.CheckAccess(NACLib::DescribeSchema, user1Token), "user1 should have grant (inherited from dir)"); UNIT_ASSERT_C(!secObj.CheckAccess(NACLib::DescribeSchema, user2Token), "user2 should have no grant (inherited from subdir)"); } + Y_UNIT_TEST(InheritPermissionsWithDifferentInheritanceTypes) { + TTestBasicRuntime runtime; + TTestEnv env(runtime); + ui64 txId = 100; + + for (int i = 1; i <= 6; ++i) { + AsyncMkDir(runtime, ++txId, "/MyRoot", "dir" + ToString(i)); + env.TestWaitNotification(runtime, txId); + } + + // If a user has the DescribeSchema grant on a directory with the default inheritance type, + // then they will have the DescribeSchema grant on the nested secret + AssertHasAccess(1, NACLib::EInheritanceType::DefaultInheritanceType, /* expectedHasAccess */ true, runtime, txId, env); + + // If a user has the DescribeSchema grant on a directory with inheritance type equals to InheritNone, + // then they will NOT have the DescribeSchema grant on the nested secret + AssertHasAccess(2, NACLib::EInheritanceType::InheritNone, /* expectedHasAccess */ false, runtime, txId, env); + + // If a user has the DescribeSchema grant on a directory with inheritance type equals to InheritObject, + // then they will have the DescribeSchema grant on the nested secret (since secrets are objects) + AssertHasAccess(3, NACLib::EInheritanceType::InheritObject, /* expectedHasAccess */ true, runtime, txId, env); + + // If a user has the DescribeSchema grant on a directory with inheritance type equals to InheritContainer, + // then they will NOT have the DescribeSchema grant on the nested secret (since secrets are objects, but not containers) + AssertHasAccess(4, NACLib::EInheritanceType::InheritContainer, /* expectedHasAccess */ false, runtime, txId, env); + + // If a user has the DescribeSchema grant on a directory with inheritance type equals to InheritOnly, + // then they will NOT have the DescribeSchema grant on the nested secret ... + AssertHasAccess(5, NACLib::EInheritanceType::InheritOnly, /* expectedHasAccess */ false, runtime, txId, env); + + // ... but with the InheritObject type as well, they will have the DescribeSchema grant + AssertHasAccess(6, NACLib::EInheritanceType::InheritOnly | NACLib::EInheritanceType::InheritObject, + /* expectedHasAccess */ true, runtime, txId, env); + } + Y_UNIT_TEST(AsyncCreateDifferentSecrets) { TTestBasicRuntime runtime; TTestEnv env(runtime); diff --git a/ydb/core/tx/tx_proxy/schemereq.cpp b/ydb/core/tx/tx_proxy/schemereq.cpp index 956c9c3c05fe..1db1fdbca6b7 100644 --- a/ydb/core/tx/tx_proxy/schemereq.cpp +++ b/ydb/core/tx/tx_proxy/schemereq.cpp @@ -772,7 +772,7 @@ struct TBaseSchemeReq: public TActorBootstrapped { case NKikimrSchemeOp::ESchemeOpDropContinuousBackup: case NKikimrSchemeOp::ESchemeOpAlterResourcePool: case NKikimrSchemeOp::ESchemeOpAlterBackupCollection: - case NKikimrSchemeOp::ESchemeOpAlterSecret: // TODO(yurikiselev): Change grants according to discussed [issue:23460] + case NKikimrSchemeOp::ESchemeOpAlterSecret: case NKikimrSchemeOp::ESchemeOpAlterStreamingQuery: { auto toResolve = TPathToResolve(pbModifyScheme); @@ -781,11 +781,11 @@ struct TBaseSchemeReq: public TActorBootstrapped { ResolveForACL.push_back(toResolve); break; } - case NKikimrSchemeOp::ESchemeOpCreateSecret: // TODO(yurikiselev): Change grants according to discussed [issue:23460] + case NKikimrSchemeOp::ESchemeOpCreateSecret: { auto toResolve = TPathToResolve(pbModifyScheme); toResolve.Path = workingDir; - toResolve.RequireAccess = NACLib::EAccessRights::AlterSchema | accessToUserAttrs; + toResolve.RequireAccess = NACLib::EAccessRights::CreateTable | accessToUserAttrs; ResolveForACL.push_back(toResolve); break; } diff --git a/ydb/library/aclib/aclib.cpp b/ydb/library/aclib/aclib.cpp index 6251c1e11f23..2d5edc4c18fd 100644 --- a/ydb/library/aclib/aclib.cpp +++ b/ydb/library/aclib/aclib.cpp @@ -513,7 +513,7 @@ TString TACL::ToString(const NACLibProto::TACE& ace) { str << ':'; str << ace.GetSID(); auto inh = ace.GetInheritanceType(); - if (inh != (EInheritanceType::InheritContainer | EInheritanceType::InheritObject)) { + if (inh != EInheritanceType::DefaultInheritanceType) { str << ':'; if (inh == EInheritanceType::InheritNone) str << '-'; @@ -670,7 +670,7 @@ void TACL::FromString(NACLibProto::TACE& ace, const TString& string) { auto end_pos = string.find(':', start_pos); ace.SetSID(string.substr(start_pos, end_pos == TString::npos ? end_pos : end_pos - start_pos)); if (end_pos == TString::npos) { - ace.SetInheritanceType(EInheritanceType::InheritContainer | EInheritanceType::InheritObject); + ace.SetInheritanceType(EInheritanceType::DefaultInheritanceType); return; } ui32 inheritanceType = 0; diff --git a/ydb/library/aclib/aclib.h b/ydb/library/aclib/aclib.h index e71a0db07331..a3c7b7e70aad 100644 --- a/ydb/library/aclib/aclib.h +++ b/ydb/library/aclib/aclib.h @@ -66,6 +66,8 @@ enum EInheritanceType : ui32 { // bitmask InheritObject = 0x01, // this ACE will inherit on child objects InheritContainer = 0x02, // this ACE will inherit on child containers InheritOnly = 0x04, // this ACE will not be used for access checking but for inheritance only + + DefaultInheritanceType = InheritObject | InheritContainer, }; enum class EDiffType : ui32 { @@ -120,8 +122,8 @@ class TACL : public NACLibProto::TACL { public: TACL() = default; TACL(const TString& string); // proto format - std::pair AddAccess(EAccessType type, ui32 access, const TSID& sid, ui32 inheritance = InheritObject | InheritContainer); - std::pair RemoveAccess(NACLib::EAccessType type, ui32 access, const NACLib::TSID& sid, ui32 inheritance = InheritObject | InheritContainer); + std::pair AddAccess(EAccessType type, ui32 access, const TSID& sid, ui32 inheritance = DefaultInheritanceType); + std::pair RemoveAccess(NACLib::EAccessType type, ui32 access, const NACLib::TSID& sid, ui32 inheritance = DefaultInheritanceType); std::pair RemoveAccess(const NACLibProto::TACE& filter); bool HasAccess(const NACLib::TSID& sid); std::pair ClearAccess(); @@ -142,8 +144,8 @@ class TDiffACL : public NACLibProto::TDiffACL { public: TDiffACL() = default; TDiffACL(const TString& string); - void AddAccess(EAccessType type, ui32 access, const TSID& sid, ui32 inheritance = InheritObject | InheritContainer); - void RemoveAccess(NACLib::EAccessType type, ui32 access, const NACLib::TSID& sid, ui32 inheritance = InheritObject | InheritContainer); + void AddAccess(EAccessType type, ui32 access, const TSID& sid, ui32 inheritance = DefaultInheritanceType); + void RemoveAccess(NACLib::EAccessType type, ui32 access, const NACLib::TSID& sid, ui32 inheritance = DefaultInheritanceType); void AddAccess(const NACLibProto::TACE& access); void RemoveAccess(const NACLibProto::TACE& access); void ClearAccess(); @@ -162,8 +164,8 @@ class TSecurityObject : public NACLibProto::TSecurityObject { ui32 GetEffectiveAccessRights(const TUserToken& user) const; TSecurityObject MergeWithParent(const NACLibProto::TSecurityObject& parent) const; // returns effective ACL as result of merging parent with this NACLibProto::TACL GetImmediateACL() const; - void AddAccess(EAccessType type, ui32 access, const TSID& sid, ui32 inheritance = InheritObject | InheritContainer); - void RemoveAccess(NACLib::EAccessType type, ui32 access, const NACLib::TSID& sid, ui32 inheritance = InheritObject | InheritContainer); + void AddAccess(EAccessType type, ui32 access, const TSID& sid, ui32 inheritance = DefaultInheritanceType); + void RemoveAccess(NACLib::EAccessType type, ui32 access, const NACLib::TSID& sid, ui32 inheritance = DefaultInheritanceType); void ApplyDiff(const NACLibProto::TDiffACL& diffACL); void ClearAccess(); TInstant GetExpireTime() const; diff --git a/ydb/tests/functional/security/test_grants.py b/ydb/tests/functional/security/test_grants.py index 4fb997954a41..efeaf0518826 100644 --- a/ydb/tests/functional/security/test_grants.py +++ b/ydb/tests/functional/security/test_grants.py @@ -9,7 +9,10 @@ CLUSTER_CONFIG = dict( additional_log_configs={ # 'TX_PROXY': LogLevels.DEBUG, - } + }, + extra_feature_flags=[ + "enable_schema_secrets", + ], ) DATABASE = "/Root/test" TABLE_NAME = "table" @@ -29,6 +32,10 @@ CREATE_TOPIC_GRANTS = ['ydb.granular.create_queue'] DROP_TOPIC_GRANTS = ['ydb.granular.remove_schema'] +CREATE_SECRET_GRANTS = ['ydb.granular.create_table'] +ALTER_SECRET_GRANTS = ['ydb.granular.alter_schema', 'ydb.granular.describe_schema'] +DROP_SECRET_GRANTS = ['ydb.granular.remove_schema', 'ydb.granular.describe_schema'] + def run_query(config, query): with ydb.Driver(config) as driver: @@ -299,3 +306,56 @@ def test_pq_grants(ydb_cluster): ydb_cluster.remove_database(DATABASE) ydb_cluster.unregister_and_stop_slots(database_nodes) + + +def test_secret_grants(ydb_cluster): + ydb_cluster.create_database(DATABASE, storage_pool_units_count={"hdd": 1}) + database_nodes = ydb_cluster.register_and_start_slots(DATABASE, count=1) + ydb_cluster.wait_tenant_up(DATABASE) + + tenant_admin_config = ydb.DriverConfig( + endpoint="%s:%s" % (ydb_cluster.nodes[1].host, ydb_cluster.nodes[1].port), + database=DATABASE, + ) + + # CREATE SECRET + user1_config = create_user(ydb_cluster, tenant_admin_config, "user1") + secret_name = f"{DATABASE}/secret" + create_secret_query = f"CREATE SECRET `{secret_name}` WITH (value = \"one\");" + _test_grants( + tenant_admin_config, + user1_config, + 'user1', + create_secret_query, + DATABASE, + CREATE_SECRET_GRANTS, + "Access denied", + ) + + # ALTER SECRET + user2_config = create_user(ydb_cluster, tenant_admin_config, "user2") + alter_secret_query = f"ALTER SECRET `{secret_name}` WITH (value = \"two\");" + _test_grants( + tenant_admin_config, + user2_config, + 'user2', + alter_secret_query, + secret_name, + ALTER_SECRET_GRANTS, + "Access denied", + ) + + # DROP SECRET + drop_secret_query = f"DROP SECRET `{secret_name}`;" + _test_grants( + tenant_admin_config, + user2_config, + 'user2', + drop_secret_query, + secret_name, + DROP_SECRET_GRANTS, + "Access denied", + ) + + ydb_cluster.remove_database(DATABASE) + ydb_cluster.unregister_and_stop_slots(database_nodes)