From 9eed3e7356305bcada92c7632a541ae81c533746 Mon Sep 17 00:00:00 2001 From: azevaykin Date: Wed, 4 Dec 2024 14:33:24 +0000 Subject: [PATCH 1/5] Audit log for "Access Denied" --- ydb/core/grpc_services/audit_log.cpp | 25 ++++++++++++++ ydb/core/grpc_services/audit_log.h | 1 + .../grpc_services/grpc_request_check_actor.h | 33 ++++++++----------- 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/ydb/core/grpc_services/audit_log.cpp b/ydb/core/grpc_services/audit_log.cpp index 14d77ab2f343..adf63eb9b799 100644 --- a/ydb/core/grpc_services/audit_log.cpp +++ b/ydb/core/grpc_services/audit_log.cpp @@ -35,6 +35,31 @@ void AuditLogConn(const IRequestProxyCtx* ctx, const TString& database, const TS ); } +void AuditLogAccessDenied(const IRequestProxyCtx* ctx, const TString& database, const TString& userSID, const TString& sanitizedToken) +{ + static const TString GrpcConnComponentName = "grpc-access-denied"; + + const TString remoteAddress = NKikimr::NAddressClassifier::ExtractAddress(ctx->GetPeerName()); + + AUDIT_LOG( + AUDIT_PART("component", GrpcConnComponentName) + + AUDIT_PART("remote_address", remoteAddress) + AUDIT_PART("subject", userSID) + AUDIT_PART("sanitized_token", (!sanitizedToken.empty() ? sanitizedToken : EmptyValue)) + AUDIT_PART("database", database) + AUDIT_PART("operation", ctx->GetRequestName()) + ); + + LOG_INFO_S(TlsActivationContext->AsActorContext(), NKikimrServices::GRPC_SERVER, "AUDIT: " + << "User has no permission to perform query on this database " << database + << ", subject " << userSID + << ", sanitized_token " << (!sanitizedToken.empty() ? sanitizedToken : EmptyValue) + << ", operation " << ctx->GetRequestName() + << ", remote_address " << remoteAddress + ); +} + void AuditLog(ui32 status, const TAuditLogParts& parts) { static const TString GrpcProxyComponentName = "grpc-proxy"; diff --git a/ydb/core/grpc_services/audit_log.h b/ydb/core/grpc_services/audit_log.h index 051d3e9d4af3..7bc8b5756050 100644 --- a/ydb/core/grpc_services/audit_log.h +++ b/ydb/core/grpc_services/audit_log.h @@ -9,6 +9,7 @@ class IRequestCtxMtSafe; // grpc "connections" log void AuditLogConn(const IRequestProxyCtx* reqCtx, const TString& database, const TString& userSID, const TString& sanitizedToken); +void AuditLogAccessDenied(const IRequestProxyCtx* reqCtx, const TString& database, const TString& userSID, const TString& sanitizedToken); using TAuditLogParts = TVector>; diff --git a/ydb/core/grpc_services/grpc_request_check_actor.h b/ydb/core/grpc_services/grpc_request_check_actor.h index 68d887e45651..9f5340729091 100644 --- a/ydb/core/grpc_services/grpc_request_check_actor.h +++ b/ydb/core/grpc_services/grpc_request_check_actor.h @@ -170,9 +170,10 @@ class TGrpcRequestCheckActor } { - auto [error, issue] = CheckConnectRight(); + bool error = CheckConnectRight(); if (error) { - ReplyUnauthorizedAndDie(*issue); + AuditLogAccessDenied(GrpcRequestBaseCtx_, CheckedDatabaseName_, TBase::GetUserSID(), TBase::GetSanitizedToken()); + ReplyUnauthorizedAndDie(); return; } } @@ -430,8 +431,8 @@ class TGrpcRequestCheckActor } private: - void ReplyUnauthorizedAndDie(const NYql::TIssue& issue) { - GrpcRequestBaseCtx_->RaiseIssue(issue); + void ReplyUnauthorizedAndDie() { + GrpcRequestBaseCtx_->RaiseIssue(MakeIssue(NKikimrIssues::TIssuesIds::ACCESS_DENIED)); GrpcRequestBaseCtx_->ReplyWithYdbStatus(Ydb::StatusIds::UNAUTHORIZED); GrpcRequestBaseCtx_->FinishSpan(); PassAway(); @@ -509,14 +510,14 @@ class TGrpcRequestCheckActor PassAway(); } - std::pair> CheckConnectRight() { + bool CheckConnectRight() { if (SkipCheckConnectRights_) { 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}; + return false; } if (TBase::IsUserAdmin()) { @@ -525,7 +526,7 @@ class TGrpcRequestCheckActor << ", database: " << CheckedDatabaseName_ << ", user: " << TBase::GetUserSID() << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName()); - return {false, std::nullopt}; + return false; } if (!TBase::GetSecurityToken()) { @@ -535,7 +536,7 @@ class TGrpcRequestCheckActor << ", database: " << CheckedDatabaseName_ << ", user: " << TBase::GetUserSID() << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName()); - return {false, std::nullopt}; + return false; } } @@ -545,30 +546,22 @@ class TGrpcRequestCheckActor << ", database: " << CheckedDatabaseName_ << ", user: " << TBase::GetUserSID() << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName()); - return {false, std::nullopt}; + return false; } const ui32 access = NACLib::ConnectDatabase; const auto& parsedToken = TBase::GetParsedToken(); if (parsedToken && SecurityObject_->CheckAccess(access, *parsedToken)) { - return {false, std::nullopt}; + return false; } - 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}; + return false; } - LOG_INFO(*TlsActivationContext, NKikimrServices::GRPC_SERVER, "%s", error.c_str()); - return {true, MakeIssue(NKikimrIssues::TIssuesIds::ACCESS_DENIED, error)}; + return true; } const TActorId Owner_; From a42b9b80941bfc87068d0f50bdf1ec6b9cbbce23 Mon Sep 17 00:00:00 2001 From: azevaykin Date: Thu, 5 Dec 2024 14:27:20 +0000 Subject: [PATCH 2/5] Test fix --- ydb/services/ydb/ydb_login_ut.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ydb/services/ydb/ydb_login_ut.cpp b/ydb/services/ydb/ydb_login_ut.cpp index 525ba593c456..bb70d767d551 100644 --- a/ydb/services/ydb/ydb_login_ut.cpp +++ b/ydb/services/ydb/ydb_login_ut.cpp @@ -169,7 +169,7 @@ Y_UNIT_TEST_SUITE(TGRpcAuthentication) { UNIT_ASSERT_NO_EXCEPTION(token = loginProvider->GetAuthInfo()); UNIT_ASSERT(!token.empty()); - loginConnection.TestConnectRight(token, "User has no permission"); + loginConnection.TestConnectRight(token, "Access denied"); loginConnection.Stop(); } From 30d2f15a510a3f673aedcd4d9c4f059b36c7afba Mon Sep 17 00:00:00 2001 From: azevaykin Date: Fri, 6 Dec 2024 13:05:35 +0000 Subject: [PATCH 3/5] Review fixes --- ydb/core/grpc_services/audit_log.cpp | 39 +++++++------------ ydb/core/grpc_services/audit_log.h | 2 +- .../grpc_services/grpc_request_check_actor.h | 34 +++++++++------- ydb/services/ydb/ydb_login_ut.cpp | 2 +- 4 files changed, 37 insertions(+), 40 deletions(-) diff --git a/ydb/core/grpc_services/audit_log.cpp b/ydb/core/grpc_services/audit_log.cpp index adf63eb9b799..efb1b56028e9 100644 --- a/ydb/core/grpc_services/audit_log.cpp +++ b/ydb/core/grpc_services/audit_log.cpp @@ -35,31 +35,6 @@ void AuditLogConn(const IRequestProxyCtx* ctx, const TString& database, const TS ); } -void AuditLogAccessDenied(const IRequestProxyCtx* ctx, const TString& database, const TString& userSID, const TString& sanitizedToken) -{ - static const TString GrpcConnComponentName = "grpc-access-denied"; - - const TString remoteAddress = NKikimr::NAddressClassifier::ExtractAddress(ctx->GetPeerName()); - - AUDIT_LOG( - AUDIT_PART("component", GrpcConnComponentName) - - AUDIT_PART("remote_address", remoteAddress) - AUDIT_PART("subject", userSID) - AUDIT_PART("sanitized_token", (!sanitizedToken.empty() ? sanitizedToken : EmptyValue)) - AUDIT_PART("database", database) - AUDIT_PART("operation", ctx->GetRequestName()) - ); - - LOG_INFO_S(TlsActivationContext->AsActorContext(), NKikimrServices::GRPC_SERVER, "AUDIT: " - << "User has no permission to perform query on this database " << database - << ", subject " << userSID - << ", sanitized_token " << (!sanitizedToken.empty() ? sanitizedToken : EmptyValue) - << ", operation " << ctx->GetRequestName() - << ", remote_address " << remoteAddress - ); -} - void AuditLog(ui32 status, const TAuditLogParts& parts) { static const TString GrpcProxyComponentName = "grpc-proxy"; @@ -80,5 +55,19 @@ void AuditLog(ui32 status, const TAuditLogParts& parts) ); } +void AuditLogConnectDbAccessDenied(const IRequestProxyCtx* ctx, const TString& database, const TString& userSID, const TString& sanitizedToken) +{ + if (::NKikimr::NAudit::AUDIT_LOG_ENABLED.load()) { + AuditLog(Ydb::StatusIds::UNAUTHORIZED, { + {"remote_address", NKikimr::NAddressClassifier::ExtractAddress(ctx->GetPeerName())}, + {"subject", userSID}, + {"sanitized_token", (!sanitizedToken.empty() ? sanitizedToken : EmptyValue)}, + {"database", database}, + {"operation", ctx->GetRequestName()}, + {"reason", "No permission to connect to the database"}, + }); + } +} + } } diff --git a/ydb/core/grpc_services/audit_log.h b/ydb/core/grpc_services/audit_log.h index 7bc8b5756050..76aca9e85160 100644 --- a/ydb/core/grpc_services/audit_log.h +++ b/ydb/core/grpc_services/audit_log.h @@ -9,12 +9,12 @@ class IRequestCtxMtSafe; // grpc "connections" log void AuditLogConn(const IRequestProxyCtx* reqCtx, const TString& database, const TString& userSID, const TString& sanitizedToken); -void AuditLogAccessDenied(const IRequestProxyCtx* reqCtx, const TString& database, const TString& userSID, const TString& sanitizedToken); using TAuditLogParts = TVector>; // grpc "operations" log void AuditLog(ui32 status, const TAuditLogParts& parts); +void AuditLogConnectDbAccessDenied(const IRequestProxyCtx* reqCtx, const TString& database, const TString& userSID, const TString& sanitizedToken); } } diff --git a/ydb/core/grpc_services/grpc_request_check_actor.h b/ydb/core/grpc_services/grpc_request_check_actor.h index 9f5340729091..5e690bffe093 100644 --- a/ydb/core/grpc_services/grpc_request_check_actor.h +++ b/ydb/core/grpc_services/grpc_request_check_actor.h @@ -170,10 +170,10 @@ class TGrpcRequestCheckActor } { - bool error = CheckConnectRight(); + auto [error, issue] = CheckConnectRight(); if (error) { - AuditLogAccessDenied(GrpcRequestBaseCtx_, CheckedDatabaseName_, TBase::GetUserSID(), TBase::GetSanitizedToken()); - ReplyUnauthorizedAndDie(); + AuditLogConnectDbAccessDenied(GrpcRequestBaseCtx_, CheckedDatabaseName_, TBase::GetUserSID(), TBase::GetSanitizedToken()); + ReplyUnauthorizedAndDie(*issue); return; } } @@ -431,8 +431,8 @@ class TGrpcRequestCheckActor } private: - void ReplyUnauthorizedAndDie() { - GrpcRequestBaseCtx_->RaiseIssue(MakeIssue(NKikimrIssues::TIssuesIds::ACCESS_DENIED)); + void ReplyUnauthorizedAndDie(const NYql::TIssue& issue) { + GrpcRequestBaseCtx_->RaiseIssue(issue); GrpcRequestBaseCtx_->ReplyWithYdbStatus(Ydb::StatusIds::UNAUTHORIZED); GrpcRequestBaseCtx_->FinishSpan(); PassAway(); @@ -510,14 +510,14 @@ class TGrpcRequestCheckActor PassAway(); } - bool CheckConnectRight() { + std::pair> CheckConnectRight() { if (SkipCheckConnectRights_) { 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; + return {false, std::nullopt}; } if (TBase::IsUserAdmin()) { @@ -526,7 +526,7 @@ class TGrpcRequestCheckActor << ", database: " << CheckedDatabaseName_ << ", user: " << TBase::GetUserSID() << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName()); - return false; + return {false, std::nullopt}; } if (!TBase::GetSecurityToken()) { @@ -536,7 +536,7 @@ class TGrpcRequestCheckActor << ", database: " << CheckedDatabaseName_ << ", user: " << TBase::GetUserSID() << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName()); - return false; + return {false, std::nullopt}; } } @@ -546,22 +546,30 @@ class TGrpcRequestCheckActor << ", database: " << CheckedDatabaseName_ << ", user: " << TBase::GetUserSID() << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName()); - return false; + return {false, std::nullopt}; } const ui32 access = NACLib::ConnectDatabase; const auto& parsedToken = TBase::GetParsedToken(); if (parsedToken && SecurityObject_->CheckAccess(access, *parsedToken)) { - return false; + return {false, std::nullopt}; } Counters_->IncDatabaseAccessDenyCounter(); if (!AppData()->FeatureFlags.GetCheckDatabaseAccessPermission()) { - return false; + return {false, std::nullopt}; } - return true; + const TString error = "No permission to connect to the database"; + LOG_INFO_S(TlsActivationContext->AsActorContext(), NKikimrServices::GRPC_SERVER, "AUDIT: " + << error + << ", database: " << CheckedDatabaseName_ + << ", user: " << TBase::GetUserSID() + << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName() + ); + + return {true, MakeIssue(NKikimrIssues::TIssuesIds::ACCESS_DENIED, error)};; } const TActorId Owner_; diff --git a/ydb/services/ydb/ydb_login_ut.cpp b/ydb/services/ydb/ydb_login_ut.cpp index bb70d767d551..26350905af41 100644 --- a/ydb/services/ydb/ydb_login_ut.cpp +++ b/ydb/services/ydb/ydb_login_ut.cpp @@ -169,7 +169,7 @@ Y_UNIT_TEST_SUITE(TGRpcAuthentication) { UNIT_ASSERT_NO_EXCEPTION(token = loginProvider->GetAuthInfo()); UNIT_ASSERT(!token.empty()); - loginConnection.TestConnectRight(token, "Access denied"); + loginConnection.TestConnectRight(token, "No permission to connect to the database"); loginConnection.Stop(); } From bd3ac9b51c279845db1dec6a1cdc6dccc0a9f14b Mon Sep 17 00:00:00 2001 From: azevaykin Date: Fri, 6 Dec 2024 13:09:48 +0000 Subject: [PATCH 4/5] Log fix --- ydb/core/grpc_services/grpc_request_check_actor.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ydb/core/grpc_services/grpc_request_check_actor.h b/ydb/core/grpc_services/grpc_request_check_actor.h index 5e690bffe093..4da1cdea4435 100644 --- a/ydb/core/grpc_services/grpc_request_check_actor.h +++ b/ydb/core/grpc_services/grpc_request_check_actor.h @@ -562,9 +562,10 @@ class TGrpcRequestCheckActor } const TString error = "No permission to connect to the database"; - LOG_INFO_S(TlsActivationContext->AsActorContext(), NKikimrServices::GRPC_SERVER, "AUDIT: " + LOG_INFO_S(TlsActivationContext->AsActorContext(), NKikimrServices::GRPC_SERVER, + "AUDIT: " << error - << ", database: " << CheckedDatabaseName_ + << ": " << CheckedDatabaseName_ << ", user: " << TBase::GetUserSID() << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName() ); From a1852a1af71dfd3b7b036f990283450aef94db6a Mon Sep 17 00:00:00 2001 From: azevaykin Date: Mon, 9 Dec 2024 07:17:06 +0000 Subject: [PATCH 5/5] Remove AUDIT --- ydb/core/grpc_services/grpc_request_check_actor.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ydb/core/grpc_services/grpc_request_check_actor.h b/ydb/core/grpc_services/grpc_request_check_actor.h index 4da1cdea4435..53f7aab28dad 100644 --- a/ydb/core/grpc_services/grpc_request_check_actor.h +++ b/ydb/core/grpc_services/grpc_request_check_actor.h @@ -563,8 +563,7 @@ class TGrpcRequestCheckActor const TString error = "No permission to connect to the database"; LOG_INFO_S(TlsActivationContext->AsActorContext(), NKikimrServices::GRPC_SERVER, - "AUDIT: " - << error + error << ": " << CheckedDatabaseName_ << ", user: " << TBase::GetUserSID() << ", from ip: " << GrpcRequestBaseCtx_->GetPeerName()