KIKIMR-11163 checking connect right is aware of AllowYdbRequestsWithoutDatabase

ref:af7ee6441e2f7adfd61508339c2fd9b6bf9dfd3a

4 files changed, 247 insertions, 36 deletions

diff --git a/ydb/core/grpc_services/grpc_request_check_actor.h b/ydb/core/grpc_services/grpc_request_check_actor.h
index 71d00ef108..d4bef15c09 100644
--- a/ydb/core/grpc_services/grpc_request_check_actor.h
+++ b/ydb/core/grpc_services/grpc_request_check_actor.h
@@ -74,11 +74,13 @@ public:
         const TSchemeBoardEvents::TDescribeSchemeResult& schemeData,
         TIntrusivePtr<TSecurityObject> securityObject,
         TAutoPtr<TEventHandle<TEvent>> request,
-        TGrpcProxyCounters::TPtr counters)
+        TGrpcProxyCounters::TPtr counters,
+        bool skipCheckConnectRigths)
         : Owner_(owner)
         , Request_(std::move(request))
         , Counters_(counters)
         , SecurityObject_(std::move(securityObject))
+        , SkipCheckConnectRigths_(skipCheckConnectRigths)
     {
         GrpcRequestBaseCtx_ = Request_->Get();
         TMaybe<TString> authToken = GrpcRequestBaseCtx_->GetYdbToken();
@@ -94,22 +96,11 @@ public:
     void Bootstrap(const TActorContext& ctx) {
         TBase::Become(&TSelf::DbAccessStateFunc);
 
-        if (SecurityObject_) {
-            const ui32 access = NACLib::ConnectDatabase;
-            if (!SecurityObject_->CheckAccess(access, TBase::GetSerializedToken())) {
-                const TString error = TStringBuilder()
-                    << "User has no permission to perform query on this database"
-                    << ", database: " << CheckedDatabaseName_
-                    << ", user: " << TBase::GetUserSID()
-                    << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName();
-                LOG_INFO(*TlsActivationContext, NKikimrServices::GRPC_PROXY_NO_CONNECT_ACCESS, "%s", error.c_str());
-
-                Counters_->IncDatabaseAccessDenyCounter();
-                if (AppData()->FeatureFlags.GetCheckDatabaseAccessPermission()) {
-                    LOG_ERROR(*TlsActivationContext, NKikimrServices::GRPC_SERVER, "%s", error.c_str());
-                    ReplyUnauthorizedAndDie(MakeIssue(NKikimrIssues::TIssuesIds::ACCESS_DENIED, error));
-                    return;
-                }
+        {
+            auto [error, issue] = CheckConnectRight();
+            if (error) {
+                ReplyUnauthorizedAndDie(*issue);
+                return;
             }
         }
 
@@ -340,6 +331,67 @@ private:
         TBase::PassAway();
     }
 
+    std::pair<bool, std::optional<NYql::TIssue>> CheckConnectRight() {
+        if (SkipCheckConnectRigths_) {
+            LOG_DEBUG_S(*TlsActivationContext, NKikimrServices::GRPC_PROXY_NO_CONNECT_ACCESS,
+                        "Skip check permission connect db, AllowYdbRequestsWithoutDatabase is off, there is no db provided from user"
+                        << ", database: " << CheckedDatabaseName_
+                        << ", user: " << TBase::GetUserSID()
+                        << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName());
+            return {false, std::nullopt};
+        }
+
+        if (TBase::IsUserAdmin()) {
+            LOG_DEBUG_S(*TlsActivationContext, NKikimrServices::GRPC_PROXY_NO_CONNECT_ACCESS,
+                        "Skip check permission connect db, user is a admin"
+                        << ", database: " << CheckedDatabaseName_
+                        << ", user: " << TBase::GetUserSID()
+                        << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName());
+            return {false, std::nullopt};
+        }
+
+        if (!TBase::GetSecurityToken()) {
+            if (!TBase::IsTokenRequired()) {
+                LOG_DEBUG_S(*TlsActivationContext, NKikimrServices::GRPC_PROXY_NO_CONNECT_ACCESS,
+                            "Skip check permission connect db, token is not required, there is no token provided"
+                            << ", database: " << CheckedDatabaseName_
+                            << ", user: " << TBase::GetUserSID()
+                            << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName());
+                return {false, std::nullopt};
+            }
+        }
+
+        if (!SecurityObject_) {
+            LOG_DEBUG_S(*TlsActivationContext, NKikimrServices::GRPC_PROXY_NO_CONNECT_ACCESS,
+                        "Skip check permission connect db, no SecurityObject_"
+                        << ", database: " << CheckedDatabaseName_
+                        << ", user: " << TBase::GetUserSID()
+                        << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName());
+            return {false, std::nullopt};
+        }
+
+        const ui32 access = NACLib::ConnectDatabase;
+        if (SecurityObject_->CheckAccess(access, TBase::GetSerializedToken())) {
+            return {false, std::nullopt};
+        }
+
+        const TString error = TStringBuilder()
+            << "User has no permission to perform query on this database"
+            << ", database: " << CheckedDatabaseName_
+            << ", user: " << TBase::GetUserSID()
+            << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName();
+        LOG_INFO(*TlsActivationContext, NKikimrServices::GRPC_PROXY_NO_CONNECT_ACCESS, "%s", error.c_str());
+
+        Counters_->IncDatabaseAccessDenyCounter();
+
+        if (!AppData()->FeatureFlags.GetCheckDatabaseAccessPermission()) {
+            return {false, std::nullopt};
+        }
+
+        LOG_ERROR(*TlsActivationContext, NKikimrServices::GRPC_SERVER, "%s", error.c_str());
+        return {true, MakeIssue(NKikimrIssues::TIssuesIds::ACCESS_DENIED, error)};
+    }
+
     const TActorId Owner_;
     TAutoPtr<TEventHandle<TEvent>> Request_;
     TGrpcProxyCounters::TPtr Counters_;
@@ -347,6 +399,7 @@ private:
     TString CheckedDatabaseName_;
     IRequestProxyCtx* GrpcRequestBaseCtx_;
     NRpcService::TRlConfig* RlConfig = nullptr;
+    bool SkipCheckConnectRigths_ = false;
 };
 
 // default behavior - attributes in schema
@@ -387,9 +440,10 @@ IActor* CreateGrpcRequestCheckActor(
     const TSchemeBoardEvents::TDescribeSchemeResult& schemeData,
     TIntrusivePtr<TSecurityObject> securityObject,
     TAutoPtr<TEventHandle<TEvent>> request,
-    TGrpcProxyCounters::TPtr counters) {
+    TGrpcProxyCounters::TPtr counters,
+    bool skipCheckConnectRigths) {
 
-    return new TGrpcRequestCheckActor<TEvent>(owner, schemeData, std::move(securityObject), std::move(request), counters);
+    return new TGrpcRequestCheckActor<TEvent>(owner, schemeData, std::move(securityObject), std::move(request), counters, skipCheckConnectRigths);
 }
 
 }
diff --git a/ydb/core/grpc_services/grpc_request_proxy.cpp b/ydb/core/grpc_services/grpc_request_proxy.cpp
index 088b84d33b..a1e7accb87 100644
--- a/ydb/core/grpc_services/grpc_request_proxy.cpp
+++ b/ydb/core/grpc_services/grpc_request_proxy.cpp
@@ -192,6 +192,9 @@ private:
         TString databaseName;
         const TDatabaseInfo* database = nullptr;
         bool skipResourceCheck = false;
+        // do not check connect rights for the deprecated requests without database
+        // remove this along with AllowYdbRequestsWithoutDatabase flag
+        bool skipCheckConnectRigths = false;
 
         if (state.State == NGrpc::TAuthState::AS_NOT_PERFORMED) {
             const auto& maybeDatabaseName = requestBaseCtx->GetDatabaseName();
@@ -204,6 +207,7 @@ private:
                 } else {
                     databaseName = RootDatabase;
                     skipResourceCheck = true;
+                    skipCheckConnectRigths = true;
                 }
             }
             auto it = Databases.find(databaseName);
@@ -249,7 +253,7 @@ private:
 
             Register(CreateGrpcRequestCheckActor<TEvent>(SelfId(),
                 database->SchemeBoardResult->DescribeSchemeResult,
-                database->SecurityObject, event.Release(), Counters));
+                database->SecurityObject, event.Release(), Counters, skipCheckConnectRigths));
             return;
         }
 
diff --git a/ydb/core/security/secure_request.h b/ydb/core/security/secure_request.h
index e2da6611db..04e276a897 100644
--- a/ydb/core/security/secure_request.h
+++ b/ydb/core/security/secure_request.h
@@ -32,10 +32,6 @@ private:
         return !SecurityToken.empty() || !GetDefaultUserSIDs().empty();
     }
 
-    bool IsTokenRequired() const {
-        return GetEnforceUserTokenRequirement() || (RequireAdminAccess && !GetAdministrationAllowedSIDs().empty());
-    }
-
     void Handle(TEvTicketParser::TEvAuthorizeTicketResult::TPtr& ev, const TActorContext& ctx) {
         const TEvTicketParser::TEvAuthorizeTicketResult& result(*ev->Get());
         if (!result.Error.empty()) {
@@ -133,6 +129,10 @@ public:
     }
 
 public:
+    bool IsTokenRequired() const {
+        return GetEnforceUserTokenRequirement() || (RequireAdminAccess && !GetAdministrationAllowedSIDs().empty());
+    }
+
     void Bootstrap(const TActorContext& ctx) {
         if (IsTokenRequired() && !IsTokenExists()) {
             return static_cast<TDerived*>(this)->OnAccessDenied(TEvTicketParser::TError{"Access denied without user token", false}, ctx);
diff --git a/ydb/services/ydb/ydb_table_ut.cpp b/ydb/services/ydb/ydb_table_ut.cpp
index 5cd24a6b82..4861c06b92 100644
--- a/ydb/services/ydb/ydb_table_ut.cpp
+++ b/ydb/services/ydb/ydb_table_ut.cpp
@@ -974,44 +974,197 @@ Y_UNIT_TEST_SUITE(YdbYqlClient) {
         }
     }
 
-    Y_UNIT_TEST(SecurityDbAccessPerm) {
+    Y_UNIT_TEST(ConnectDbAclIsStrictlyChecked) {
         NKikimrConfig::TAppConfig appConfig;
         appConfig.MutableFeatureFlags()->SetCheckDatabaseAccessPermission(true);
-        TKikimrWithGrpcAndRootSchema server(appConfig);
+        appConfig.MutableFeatureFlags()->SetAllowYdbRequestsWithoutDatabase(false);
+        appConfig.MutableDomainsConfig()->MutableSecurityConfig()->SetEnforceUserTokenRequirement(true);
+        appConfig.MutableDomainsConfig()->MutableSecurityConfig()->AddDefaultUserSIDs("test_user_no_rights@builtin");
+        TKikimrWithGrpcAndRootSchemaWithAuth server(appConfig);
+
+        server.Server_->GetRuntime()->SetLogPriority(NKikimrServices::GRPC_PROXY_NO_CONNECT_ACCESS, NActors::NLog::PRI_DEBUG);
 
         ui16 grpc = server.GetPort();
+
+        { // no db
+            TString location = TStringBuilder() << "localhost:" << grpc;
+            auto driver = NYdb::TDriver(
+                TDriverConfig()
+                    .SetEndpoint(location));
+
+            NYdb::NTable::TClientSettings settings;
+            settings.AuthToken("root@builtin");
+
+            NYdb::NTable::TTableClient client(driver, settings);
+            auto call = [] (NYdb::NTable::TTableClient& client) -> NYdb::TStatus {
+                Cerr << "Call\n";
+                return client.CreateSession().ExtractValueSync();
+            };
+            auto status = client.RetryOperationSync(call);
+
+            UNIT_ASSERT_VALUES_EQUAL_C(status.GetStatus(), EStatus::CLIENT_UNAUTHENTICATED, status.GetIssues().ToString());
+
+        }
         TString location = TStringBuilder() << "localhost:" << grpc;
         auto driver = NYdb::TDriver(
             TDriverConfig()
-                .SetAuthToken("root@builtin")
-                .SetEndpoint(location));
+                .SetEndpoint(location)
+                .SetDatabase("/Root"));
+
+        { // no token
+            NYdb::NTable::TClientSettings settings;
+            NYdb::NTable::TTableClient client(driver, settings);
+            auto call = [] (NYdb::NTable::TTableClient& client) -> NYdb::TStatus {
+                Cerr << "Call\n";
+                return client.CreateSession().ExtractValueSync();
+            };
+            auto status = client.RetryOperationSync(call);
+
+            UNIT_ASSERT_VALUES_EQUAL_C(status.GetStatus(), EStatus::CLIENT_UNAUTHENTICATED, status.GetIssues().ToString());
+        }
+
+
+        { // empty token
+            NYdb::NTable::TClientSettings settings;
+            settings.AuthToken("");
+            NYdb::NTable::TTableClient client(driver, settings);
+
+            auto call = [] (NYdb::NTable::TTableClient& client) -> NYdb::TStatus {
+                Cerr << "Call\n";
+                return client.CreateSession().ExtractValueSync();
+            };
+            auto status = client.RetryOperationSync(call);
+
+            UNIT_ASSERT_VALUES_EQUAL_C(status.GetStatus(), EStatus::CLIENT_UNAUTHENTICATED, status.GetIssues().ToString());
+        }
+
+        { // no connect right
+            TString location = TStringBuilder() << "localhost:" << grpc;
+            auto driver = NYdb::TDriver(
+                TDriverConfig()
+                    .SetEndpoint(location)
+                    .SetDatabase("/Root"));
 
-        {
             NYdb::NTable::TClientSettings settings;
             settings.AuthToken("test_user@builtin");
             NYdb::NTable::TTableClient client(driver, settings);
-            auto session = client.CreateSession().ExtractValueSync();
-            UNIT_ASSERT_VALUES_EQUAL_C(session.GetStatus(), EStatus::UNAUTHORIZED, session.GetIssues().ToString());
+
+            auto call = [] (NYdb::NTable::TTableClient& client) -> NYdb::TStatus {
+                return client.CreateSession().ExtractValueSync();
+            };
+            auto status = client.RetryOperationSync(call);
+
+            UNIT_ASSERT_VALUES_EQUAL_C(status.GetStatus(), EStatus::UNAUTHORIZED, status.GetIssues().ToString());
         }
 
-        {
-            auto scheme = NYdb::NScheme::TSchemeClient(driver);
+        { // set connect
+            NYdb::TCommonClientSettings settings;
+            settings.AuthToken("root@builtin");
+            auto scheme = NYdb::NScheme::TSchemeClient(driver, settings);
             auto status = scheme.ModifyPermissions("/Root",
                 NYdb::NScheme::TModifyPermissionsSettings()
                     .AddGrantPermissions(
                         NYdb::NScheme::TPermissions("test_user@builtin", TVector<TString>{"ydb.database.connect"})
                     )
                 ).ExtractValueSync();
-            UNIT_ASSERT_EQUAL(status.IsTransportError(), false);
             UNIT_ASSERT_VALUES_EQUAL_C(status.GetStatus(), EStatus::SUCCESS, status.GetIssues().ToString());
+            UNIT_ASSERT_VALUES_EQUAL_C(status.IsTransportError(), false, status.GetIssues().ToString());
         }
 
+        ui32 attemps = 2; // system is notified asynchronously, so it may see old acl for awhile
+        while (attemps) { // accept connect right
+            --attemps;
+
+            NYdb::NTable::TClientSettings settings;
+            settings.AuthToken("test_user@builtin");
+            NYdb::NTable::TTableClient client(driver, settings);
+
+            auto call = [] (NYdb::NTable::TTableClient& client) -> NYdb::TStatus {
+                return client.CreateSession().ExtractValueSync();
+            };
+            auto status = client.RetryOperationSync(call);
+
+            if (attemps && status.GetStatus() == EStatus::UNAUTHORIZED) {
+                continue;
+            }
+
+            UNIT_ASSERT_VALUES_EQUAL_C(status.GetStatus(), EStatus::SUCCESS, status.GetIssues().ToString());
+        }
+    }
+
+    Y_UNIT_TEST(ConnectDbAclIsOffWhenYdbRequestsWithoutDatabase) {
+        NKikimrConfig::TAppConfig appConfig;
+        appConfig.MutableFeatureFlags()->SetCheckDatabaseAccessPermission(true);
+        appConfig.MutableFeatureFlags()->SetAllowYdbRequestsWithoutDatabase(true);
+        appConfig.MutableDomainsConfig()->MutableSecurityConfig()->SetEnforceUserTokenRequirement(false);
+        appConfig.MutableDomainsConfig()->MutableSecurityConfig()->AddDefaultUserSIDs("test_user_no_rights@builtin");
+        TKikimrWithGrpcAndRootSchema server(appConfig);
+
+        ui16 grpc = server.GetPort();
         {
+            TString location = TStringBuilder() << "localhost:" << grpc;
+            auto driver = NYdb::TDriver(
+                TDriverConfig()
+                    .SetEndpoint(location)
+                    .SetDatabase("/Root"));
+
+            // with db
+            NYdb::NTable::TClientSettings settings;
+            settings.AuthToken("test_user@builtin");
+            NYdb::NTable::TTableClient client(driver, settings);
+
+            auto call = [] (NYdb::NTable::TTableClient& client) -> NYdb::TStatus {
+                return client.CreateSession().ExtractValueSync();
+            };
+            auto status = client.RetryOperationSync(call);
+
+            UNIT_ASSERT_VALUES_EQUAL_C(status.GetStatus(), EStatus::UNAUTHORIZED, status.GetIssues().ToString());
+        }
+
+        {
+            TString location = TStringBuilder() << "localhost:" << grpc;
+            auto driver = NYdb::TDriver(
+                TDriverConfig()
+                    .SetEndpoint(location));
+
+            // without db
             NYdb::NTable::TClientSettings settings;
             settings.AuthToken("test_user@builtin");
             NYdb::NTable::TTableClient client(driver, settings);
-            auto session = client.CreateSession().ExtractValueSync();
-            UNIT_ASSERT_VALUES_EQUAL_C(session.GetStatus(), EStatus::SUCCESS, session.GetIssues().ToString());
+
+            auto call = [] (NYdb::NTable::TTableClient& client) -> NYdb::TStatus {
+                return client.CreateSession().ExtractValueSync();
+            };
+            auto status = client.RetryOperationSync(call);
+
+            UNIT_ASSERT_VALUES_EQUAL_C(status.GetStatus(), EStatus::SUCCESS, status.GetIssues().ToString());
+        }
+    }
+
+    Y_UNIT_TEST(ConnectDbAclIsOffWhenTokenIsOptionalAndNull) {
+        NKikimrConfig::TAppConfig appConfig;
+        appConfig.MutableFeatureFlags()->SetCheckDatabaseAccessPermission(true);
+        appConfig.MutableFeatureFlags()->SetAllowYdbRequestsWithoutDatabase(false);
+        appConfig.MutableDomainsConfig()->MutableSecurityConfig()->SetEnforceUserTokenRequirement(false);
+        appConfig.MutableDomainsConfig()->MutableSecurityConfig()->AddDefaultUserSIDs("test_user_no_rights@builtin");
+        TKikimrWithGrpcAndRootSchema server(appConfig);
+
+        ui16 grpc = server.GetPort();
+        TString location = TStringBuilder() << "localhost:" << grpc;
+        auto driver = NYdb::TDriver(
+            TDriverConfig()
+                .SetEndpoint(location));
+
+        { // no token
+            NYdb::NTable::TClientSettings settings;
+            NYdb::NTable::TTableClient client(driver, settings);
+
+            auto call = [] (NYdb::NTable::TTableClient& client) -> NYdb::TStatus {
+                return client.CreateSession().ExtractValueSync();
+            };
+            auto status = client.RetryOperationSync(call);
+
+            UNIT_ASSERT_VALUES_EQUAL_C(status.GetStatus(), EStatus::SUCCESS, status.GetIssues().ToString());
         }
     }
 /*