From aee204e8ac3ce09ac5bc984d7fefa065c19f89d9 Mon Sep 17 00:00:00 2001 From: Vasily Gerasimov Date: Mon, 5 Aug 2024 14:03:21 +0000 Subject: [PATCH] Check client certificate/token when option EnforceUserTokenCheckRequirement is on --- ydb/core/grpc_services/grpc_request_proxy.cpp | 19 ++- .../grpc_request_proxy_simple.cpp | 19 ++- ydb/library/grpc/server/grpc_request.h | 18 ++- ydb/services/ydb/ydb_common_ut.h | 2 +- ydb/services/ydb/ydb_register_node_ut.cpp | 45 ++++++ ydb/services/ydb/ydb_ut.cpp | 128 ++++++++++-------- 6 files changed, 157 insertions(+), 74 deletions(-) diff --git a/ydb/core/grpc_services/grpc_request_proxy.cpp b/ydb/core/grpc_services/grpc_request_proxy.cpp index 5a9c0771480f..cf4eafb7e395 100644 --- a/ydb/core/grpc_services/grpc_request_proxy.cpp +++ b/ydb/core/grpc_services/grpc_request_proxy.cpp @@ -420,9 +420,22 @@ void TGRpcRequestProxyImpl::HandleUndelivery(TEvents::TEvUndelivered::TPtr& ev) bool TGRpcRequestProxyImpl::IsAuthStateOK(const IRequestProxyCtx& ctx) { const auto& state = ctx.GetAuthState(); - return state.State == NYdbGrpc::TAuthState::AS_OK || - state.State == NYdbGrpc::TAuthState::AS_FAIL && state.NeedAuth == false || - state.NeedAuth == false && !ctx.GetYdbToken(); + if (state.State == NYdbGrpc::TAuthState::AS_OK) { + return true; + } + + const bool authorizationParamsAreSet = ctx.GetYdbToken() || !ctx.FindClientCertPropertyValues().empty(); + if (!state.NeedAuth && !authorizationParamsAreSet) { + return true; + } + + if (!state.NeedAuth && state.State == NYdbGrpc::TAuthState::AS_FAIL) { + if (AppData()->EnforceUserTokenCheckRequirement && authorizationParamsAreSet) { + return false; + } + return true; + } + return false; } void TGRpcRequestProxyImpl::MaybeStartTracing(IRequestProxyCtx& ctx) { diff --git a/ydb/core/grpc_services/grpc_request_proxy_simple.cpp b/ydb/core/grpc_services/grpc_request_proxy_simple.cpp index 3ad80fbbf542..c2274c3d7be3 100644 --- a/ydb/core/grpc_services/grpc_request_proxy_simple.cpp +++ b/ydb/core/grpc_services/grpc_request_proxy_simple.cpp @@ -172,9 +172,22 @@ void TGRpcRequestProxySimple::HandleUndelivery(TEvents::TEvUndelivered::TPtr& ev bool TGRpcRequestProxySimple::IsAuthStateOK(const IRequestProxyCtx& ctx) { const auto& state = ctx.GetAuthState(); - return state.State == NYdbGrpc::TAuthState::AS_OK || - state.State == NYdbGrpc::TAuthState::AS_FAIL && state.NeedAuth == false || - state.NeedAuth == false && !ctx.GetYdbToken(); + if (state.State == NYdbGrpc::TAuthState::AS_OK) { + return true; + } + + const bool authorizationParamsAreSet = ctx.GetYdbToken() || !ctx.FindClientCertPropertyValues().empty(); + if (!state.NeedAuth && !authorizationParamsAreSet) { + return true; + } + + if (!state.NeedAuth && state.State == NYdbGrpc::TAuthState::AS_FAIL) { + if (AppData()->EnforceUserTokenCheckRequirement && authorizationParamsAreSet) { + return false; + } + return true; + } + return false; } template diff --git a/ydb/library/grpc/server/grpc_request.h b/ydb/library/grpc/server/grpc_request.h index 8a4774fcc2f6..3be0cb44ee9f 100644 --- a/ydb/library/grpc/server/grpc_request.h +++ b/ydb/library/grpc/server/grpc_request.h @@ -73,12 +73,11 @@ class TGRpcRequestImpl , RequestLimiter_(std::move(limiter)) , Writer_(new grpc::ServerAsyncResponseWriter>(&this->Context)) , StateFunc_(&TThis::SetRequestDone) + , Request_(google::protobuf::Arena::CreateMessage(&Arena_)) + , AuthState_(Server_->NeedAuth()) { - AuthState_ = Server_->NeedAuth() ? TAuthState(true) : TAuthState(false); - Request_ = google::protobuf::Arena::CreateMessage(&Arena_); Y_ABORT_UNLESS(Request_); GRPC_LOG_DEBUG(Logger_, "[%p] created request Name# %s", this, Name_); - FinishPromise_ = NThreading::NewPromise(); } TGRpcRequestImpl(TService* server, @@ -101,13 +100,12 @@ class TGRpcRequestImpl , RequestLimiter_(std::move(limiter)) , StreamWriter_(new grpc::ServerAsyncWriter>(&this->Context)) , StateFunc_(&TThis::SetRequestDone) + , Request_(google::protobuf::Arena::CreateMessage(&Arena_)) + , AuthState_(Server_->NeedAuth()) + , StreamAdaptor_(CreateStreamAdaptor()) { - AuthState_ = Server_->NeedAuth() ? TAuthState(true) : TAuthState(false); - Request_ = google::protobuf::Arena::CreateMessage(&Arena_); Y_ABORT_UNLESS(Request_); GRPC_LOG_DEBUG(Logger_, "[%p] created streaming request Name# %s", this, Name_); - FinishPromise_ = NThreading::NewPromise(); - StreamAdaptor_ = CreateStreamAdaptor(); } TAsyncFinishResult GetFinishFuture() override { @@ -549,7 +547,7 @@ class TGRpcRequestImpl } using TStateFunc = bool (TThis::*)(bool); - TService* Server_; + TService* Server_ = nullptr; TOnRequest Cb_; TRequestCallback RequestCallback_; TStreamRequestCallback StreamRequestCallback_; @@ -561,9 +559,9 @@ class TGRpcRequestImpl THolder>> Writer_; THolder>> StreamWriter_; TStateFunc StateFunc_; - TIn* Request_; google::protobuf::Arena Arena_; + TIn* Request_ = nullptr; TOnNextReply NextReplyCb_; ui32 RequestSize = 0; ui32 ResponseSize = 0; @@ -577,7 +575,7 @@ class TGRpcRequestImpl using TFixedEvent = TQueueFixedEvent; TFixedEvent OnFinishTag = { this, &TGRpcRequestImpl::OnFinish }; - NThreading::TPromise FinishPromise_; + NThreading::TPromise FinishPromise_ = NThreading::NewPromise(); bool SkipUpdateCountersOnError = false; IStreamAdaptor::TPtr StreamAdaptor_; std::atomic ClientLost_ = false; diff --git a/ydb/services/ydb/ydb_common_ut.h b/ydb/services/ydb/ydb_common_ut.h index ec13ba11ec31..ec837f9730d2 100644 --- a/ydb/services/ydb/ydb_common_ut.h +++ b/ydb/services/ydb/ydb_common_ut.h @@ -150,7 +150,7 @@ class TBasicKikimrWithGrpcAndRootSchema { NYdbGrpc::TServerOptions grpcOption; if (TestSettings::AUTH) { - grpcOption.SetUseAuth(true); + grpcOption.SetUseAuth(appConfig.GetDomainsConfig().GetSecurityConfig().GetEnforceUserTokenRequirement()); // In real life UseAuth is initialized with EnforceUserTokenRequirement. To avoid incorrect tests we must do the same. } grpcOption.SetPort(grpc); if (TestSettings::SSL) { diff --git a/ydb/services/ydb/ydb_register_node_ut.cpp b/ydb/services/ydb/ydb_register_node_ut.cpp index c5d3216cd1b0..a797dc4cdef0 100644 --- a/ydb/services/ydb/ydb_register_node_ut.cpp +++ b/ydb/services/ydb/ydb_register_node_ut.cpp @@ -54,6 +54,7 @@ struct TKikimrServerForTestNodeRegistration : TBasicKikimrWithGrpcAndRootSchema< struct TServerInitialization { bool EnforceUserToken = false; + bool EnforceCheckUserToken = false; // Takes effect when EnforceUserToken = false bool EnableDynamicNodeAuth = false; bool EnableWrongIdentity = false; bool SetNodeAuthValues = false; @@ -72,6 +73,9 @@ struct TKikimrServerForTestNodeRegistration : TBasicKikimrWithGrpcAndRootSchema< if (serverInitialization.EnforceUserToken) { securityConfig.SetEnforceUserTokenRequirement(true); } + if (serverInitialization.EnforceCheckUserToken) { + securityConfig.SetEnforceUserTokenCheckRequirement(true); + } if (serverInitialization.EnableDynamicNodeAuth) { config.MutableClientCertificateAuthorization()->SetRequestClientCertificate(true); // config.MutableFeatureFlags()->SetEnableDynamicNodeAuthorization(true); @@ -884,6 +888,47 @@ Y_UNIT_TEST(ServerWithoutCertVerification_ClientDoesNotProvideClientCerts) { } } +Y_UNIT_TEST(ServerWithCertVerification_AuthNotRequired) { + // Scenario when we want to turn on secure node registration, but to check it in safe way + const TCertAndKey& caCert = TKikimrTestWithServerCert::GetCACertAndKey(); + TProps props = TProps::AsClientServer(); + const TCertAndKey clientServerCert = GenerateSignedCert(caCert, props); + + props.Organization = "Enemy Org"; + const TCertAndKey clientServerEnemyCert = GenerateSignedCert(caCert, props); + + TKikimrServerForTestNodeRegistration server({ + .EnforceUserToken = false, // still allow not secure way + .EnforceCheckUserToken = true, // when attempt to register with cert arrives, check it as if EnforceUserToken was switched on + .EnableDynamicNodeAuth = true, + .SetNodeAuthValues = true, + .RegisterNodeAllowedSids = {"DefaultClientAuth@cert"} + }); + ui16 grpc = server.GetPort(); + TString location = TStringBuilder() << "localhost:" << grpc; + + SetLogPriority(server); + + TDriverConfig secureConnectionConfig; + secureConnectionConfig.UseSecureConnection(caCert.Certificate.c_str()) + .UseClientCertificate(clientServerCert.Certificate.c_str(),clientServerCert.PrivateKey.c_str()) + .SetEndpoint(location); + + TDriverConfig insecureConnectionConfig; + insecureConnectionConfig.UseSecureConnection(caCert.Certificate.c_str()) + .SetEndpoint(location); + + TDriverConfig enemyConnectionConfig; + enemyConnectionConfig.UseSecureConnection(caCert.Certificate.c_str()) + .UseClientCertificate(clientServerEnemyCert.Certificate.c_str(),clientServerEnemyCert.PrivateKey.c_str()) + .SetEndpoint(location); + + CheckGood(RegisterNode(secureConnectionConfig)); + CheckGood(RegisterNode(insecureConnectionConfig)); // without token and cert // EnforceUserToken = false + CheckAccessDenied(RegisterNode(insecureConnectionConfig.SetAuthToken("invalid token")), "Unknown token"); + CheckAccessDeniedRegisterNode(RegisterNode(enemyConnectionConfig), "Client certificate failed verification"); +} + } namespace { diff --git a/ydb/services/ydb/ydb_ut.cpp b/ydb/services/ydb/ydb_ut.cpp index bd454054b197..75b499423586 100644 --- a/ydb/services/ydb/ydb_ut.cpp +++ b/ydb/services/ydb/ydb_ut.cpp @@ -189,6 +189,38 @@ Y_UNIT_TEST_SUITE(TGRpcClientLowTest) { UNIT_ASSERT(allDoneOk); } + std::pair MakeTestRequest(NGRpcProxy::TGRpcClientConfig& clientConfig, const TString& database, const TString& token) { + NYdbGrpc::TCallMeta meta; + if (token) { // empty token => no token + meta.Aux.push_back({YDB_AUTH_TICKET_HEADER, token}); + } + meta.Aux.push_back({YDB_DATABASE_HEADER, database}); + + NYdbGrpc::TGRpcClientLow clientLow; + auto connection = clientLow.CreateGRpcServiceConnection(clientConfig); + + Ydb::StatusIds::StatusCode status; + grpc::StatusCode gStatus; + + do { + auto promise = NThreading::NewPromise(); + Ydb::Table::CreateSessionRequest request; + NYdbGrpc::TResponseCallback responseCb = + [&status, &gStatus, promise](NYdbGrpc::TGrpcStatus&& grpcStatus, Ydb::Table::CreateSessionResponse&& response) mutable { + UNIT_ASSERT(!grpcStatus.InternalError); + gStatus = grpc::StatusCode(grpcStatus.GRpcStatusCode); + auto deferred = response.operation(); + status = deferred.status(); + promise.SetValue(); + }; + + connection->DoRequest(request, std::move(responseCb), &Ydb::Table::V1::TableService::Stub::AsyncCreateSession, meta); + promise.GetFuture().Wait(); + } while (status == Ydb::StatusIds::UNAVAILABLE); + Cerr << "TestRequest(database=\"" << database << "\", token=\"" << token << "\") => {" << Ydb::StatusIds::StatusCode_Name(status) << ", " << int(gStatus) << "}" << Endl; + return std::make_pair(status, gStatus); + } + Y_UNIT_TEST(GrpcRequestProxy) { NKikimrConfig::TAppConfig appConfig; appConfig.MutableDomainsConfig()->MutableSecurityConfig()->SetEnforceUserTokenRequirement(true); @@ -197,79 +229,61 @@ Y_UNIT_TEST_SUITE(TGRpcClientLowTest) { ui16 grpc = server.GetPort(); TString location = TStringBuilder() << "localhost:" << grpc; auto clientConfig = NGRpcProxy::TGRpcClientConfig(location); - auto doTest = [&](const TString& database) { - NYdbGrpc::TCallMeta meta; - meta.Aux.push_back({YDB_AUTH_TICKET_HEADER, "root@builtin"}); - meta.Aux.push_back({YDB_DATABASE_HEADER, database}); - - NYdbGrpc::TGRpcClientLow clientLow; - auto connection = clientLow.CreateGRpcServiceConnection(clientConfig); - Ydb::StatusIds::StatusCode status; - int gStatus; + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "/Root", "root@builtin"), std::make_pair(Ydb::StatusIds::SUCCESS, grpc::StatusCode::OK)); + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "/blabla", "root@builtin"), std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, grpc::StatusCode::UNAUTHENTICATED)); + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "blabla", "root@builtin"), std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, grpc::StatusCode::UNAUTHENTICATED)); + } - do { - auto promise = NThreading::NewPromise(); - Ydb::Table::CreateSessionRequest request; - NYdbGrpc::TResponseCallback responseCb = - [&status, &gStatus, promise](NYdbGrpc::TGrpcStatus&& grpcStatus, Ydb::Table::CreateSessionResponse&& response) mutable { - UNIT_ASSERT(!grpcStatus.InternalError); - gStatus = grpcStatus.GRpcStatusCode; - auto deferred = response.operation(); - status = deferred.status(); - promise.SetValue(); - }; + Y_UNIT_TEST(GrpcRequestProxyWithoutToken) { + NKikimrConfig::TAppConfig appConfig; + appConfig.MutableDomainsConfig()->MutableSecurityConfig()->SetEnforceUserTokenRequirement(true); + TKikimrWithGrpcAndRootSchemaWithAuth server(appConfig); - connection->DoRequest(request, std::move(responseCb), &Ydb::Table::V1::TableService::Stub::AsyncCreateSession, meta); - promise.GetFuture().Wait(); - } while (status == Ydb::StatusIds::UNAVAILABLE); - return std::make_pair(status, gStatus); - }; + ui16 grpc = server.GetPort(); + TString location = TStringBuilder() << "localhost:" << grpc; + auto clientConfig = NGRpcProxy::TGRpcClientConfig(location); - UNIT_ASSERT_VALUES_EQUAL(doTest("/Root"), std::make_pair(Ydb::StatusIds::SUCCESS, 0)); - UNIT_ASSERT_VALUES_EQUAL(doTest("/blabla"), std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, 16)); - UNIT_ASSERT_VALUES_EQUAL(doTest("blabla"), std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, 16)); + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "/Root", ""), std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, grpc::StatusCode::UNAUTHENTICATED)); + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "/blabla", ""), std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, grpc::StatusCode::UNAUTHENTICATED)); + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "blabla", ""), std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, grpc::StatusCode::UNAUTHENTICATED)); } - Y_UNIT_TEST(GrpcRequestProxyWithoutToken) { + void GrpcRequestProxyCheckTokenWhenItIsSpecified(bool enforceUserTokenCheckRequirement) { NKikimrConfig::TAppConfig appConfig; - appConfig.MutableDomainsConfig()->MutableSecurityConfig()->SetEnforceUserTokenRequirement(true); + appConfig.MutableDomainsConfig()->MutableSecurityConfig()->SetEnforceUserTokenRequirement(false); + appConfig.MutableDomainsConfig()->MutableSecurityConfig()->SetEnforceUserTokenCheckRequirement(enforceUserTokenCheckRequirement); TKikimrWithGrpcAndRootSchemaWithAuth server(appConfig); ui16 grpc = server.GetPort(); TString location = TStringBuilder() << "localhost:" << grpc; auto clientConfig = NGRpcProxy::TGRpcClientConfig(location); - auto doTest = [&](const TString& database) { - NYdbGrpc::TCallMeta meta; - meta.Aux.push_back({YDB_DATABASE_HEADER, database}); - NYdbGrpc::TGRpcClientLow clientLow; - auto connection = clientLow.CreateGRpcServiceConnection(clientConfig); + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "/Root", ""), std::make_pair(Ydb::StatusIds::SUCCESS, grpc::StatusCode::OK)); + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "/blabla", ""), std::make_pair(Ydb::StatusIds::SUCCESS, grpc::StatusCode::OK)); + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "blabla", ""), std::make_pair(Ydb::StatusIds::SUCCESS, grpc::StatusCode::OK)); - Ydb::StatusIds::StatusCode status; - grpc::StatusCode gStatus; + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "/Root", "root@builtin"), std::make_pair(Ydb::StatusIds::SUCCESS, grpc::StatusCode::OK)); + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "/blabla", "root@builtin"), std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, grpc::StatusCode::UNAUTHENTICATED)); + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "blabla", "root@builtin"), std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, grpc::StatusCode::UNAUTHENTICATED)); - do { - auto promise = NThreading::NewPromise(); - Ydb::Table::CreateSessionRequest request; - NYdbGrpc::TResponseCallback responseCb = - [&status, &gStatus, promise](NYdbGrpc::TGrpcStatus&& grpcStatus, Ydb::Table::CreateSessionResponse&& response) mutable { - UNIT_ASSERT(!grpcStatus.InternalError); - gStatus = grpc::StatusCode(grpcStatus.GRpcStatusCode); - auto deferred = response.operation(); - status = deferred.status(); - promise.SetValue(); - }; + const auto reqResultWithInvalidToken = MakeTestRequest(clientConfig, "/Root", "invalid token"); + if (enforceUserTokenCheckRequirement) { + UNIT_ASSERT_EQUAL(reqResultWithInvalidToken, std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, grpc::StatusCode::UNAUTHENTICATED)); + } else { + UNIT_ASSERT_EQUAL(reqResultWithInvalidToken, std::make_pair(Ydb::StatusIds::SUCCESS, grpc::StatusCode::OK)); + } - connection->DoRequest(request, std::move(responseCb), &Ydb::Table::V1::TableService::Stub::AsyncCreateSession, meta); - promise.GetFuture().Wait(); - } while (status == Ydb::StatusIds::UNAVAILABLE); - return std::make_pair(status, gStatus); - }; + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "/blabla", "invalid token"), std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, grpc::StatusCode::UNAUTHENTICATED)); + UNIT_ASSERT_EQUAL(MakeTestRequest(clientConfig, "blabla", "invalid token"), std::make_pair(Ydb::StatusIds::STATUS_CODE_UNSPECIFIED, grpc::StatusCode::UNAUTHENTICATED)); + } - UNIT_ASSERT_EQUAL(doTest("/Root").second, grpc::StatusCode::UNAUTHENTICATED); - UNIT_ASSERT_EQUAL(doTest("/blabla").second, grpc::StatusCode::UNAUTHENTICATED); - UNIT_ASSERT_EQUAL(doTest("blabla").second, grpc::StatusCode::UNAUTHENTICATED); + Y_UNIT_TEST(GrpcRequestProxyCheckTokenWhenItIsSpecified_Ignore) { + GrpcRequestProxyCheckTokenWhenItIsSpecified(false); + } + + Y_UNIT_TEST(GrpcRequestProxyCheckTokenWhenItIsSpecified_Check) { + GrpcRequestProxyCheckTokenWhenItIsSpecified(true); } Y_UNIT_TEST(BiStreamPing) { @@ -5614,7 +5628,7 @@ Y_UNIT_TEST(DisableWritesToDatabase) { TTenants tenants(server); tenants.Run(tenantPath, 1); - + TString table = Sprintf("%s/table", tenantPath.c_str()); ExecSQL(server, sender, Sprintf(R"( CREATE TABLE `%s` (