24-3: auditlog: add logouts (#9052)

ijon · web-flow · commit 7da87a5ea7dc · 2024-09-11T14:04:02.000+03:00
Add audit logging for web logout operation. merged ... (#9050) from main Web logout now require proper authentication as audit record without name of the logged out user is meaningless. Previously web logout was anonymous. KIKIMR-21764
diff --git a/ydb/core/security/login_page.cpp b/ydb/core/security/login_page.cpp
@@ -6,6 +6,8 @@
 #include <library/cpp/json/json_reader.h>
 #include <library/cpp/json/json_writer.h>
 
+#include <ydb/core/util/address_classifier.h>
+#include <ydb/core/audit/audit_log.h>
 #include <ydb/core/base/tablet_pipe.h>
 #include <ydb/core/tx/scheme_cache/scheme_cache.h>
 #include <ydb/core/tx/schemeshard/schemeshard.h>
@@ -21,6 +23,24 @@ using namespace NKikimr;
 using namespace NSchemeShard;
 using namespace NMonitoring;
 
+void AuditLogWebUILogout(const NHttp::THttpIncomingRequest& request, const TString& userSID) {
+    static const TString WebLoginComponentName = "web-login";
+    static const TString LogoutOperationName = "LOGOUT";
+    static const TString EmptyValue = "{none}";
+
+    auto remoteAddress = NKikimr::NAddressClassifier::ExtractAddress(request.Address->ToString());
+
+    // NOTE: audit field set here must be in sync with ydb/core/tx/schemeshard/schemeshard_audit_log.h, AuditLogLogin()
+    AUDIT_LOG(
+        AUDIT_PART("component", WebLoginComponentName)
+        AUDIT_PART("remote_address", (!remoteAddress.empty() ? remoteAddress : EmptyValue))
+        AUDIT_PART("subject", (!userSID.empty() ? userSID : EmptyValue))
+        //NOTE: no database specified as web logout considered cluster-wide
+        AUDIT_PART("operation", LogoutOperationName)
+        AUDIT_PART("status", TString("SUCCESS"))
+    );
+}
+
 using THttpResponsePtr = THolder<NMon::IEvHttpInfoRes>;
 
 class TLoginRequest : public NActors::TActorBootstrapped<TLoginRequest> {
@@ -251,6 +271,8 @@ class TLogoutRequest : public NActors::TActorBootstrapped<TLogoutRequest> {
     STATEFN(StateWork) {
         switch (ev->GetTypeRewrite()) {
             hFunc(TEvents::TEvPoisonPill, HandlePoisonPill);
+            hFunc(TEvTicketParser::TEvAuthorizeTicketResult, Handle);
+            cFunc(TEvents::TSystem::Wakeup, HandleTimeout);
         }
     }
 
@@ -265,13 +287,42 @@ class TLogoutRequest : public NActors::TActorBootstrapped<TLogoutRequest> {
             return ReplyErrorAndPassAway("400", "Bad Request", "Invalid method");
         }
 
-        ReplyDeleteCookieAndPassAway();
+        NHttp::TCookies cookies(NHttp::THeaders(Request->Headers)["Cookie"]);
+        TStringBuf ydbSessionId = cookies["ydb_session_id"];
+        if (ydbSessionId.empty()) {
+            return ReplyErrorAndPassAway("401", "Unauthorized", "No ydb_session_id cookie");
+        }
+
+        Send(NKikimr::MakeTicketParserID(), new NKikimr::TEvTicketParser::TEvAuthorizeTicket({
+            .Database = TString(),
+            .Ticket = TString("Login ") + ydbSessionId,
+            .PeerName = Request->Address->ToString(),
+        }));
+
+        Become(&TThis::StateWork, Timeout, new TEvents::TEvWakeup());
+    }
+
+    void Handle(TEvTicketParser::TEvAuthorizeTicketResult::TPtr& ev) {
+        const TEvTicketParser::TEvAuthorizeTicketResult& result = *ev->Get();
+        if (result.Error) {
+            return ReplyErrorAndPassAway("403", "Forbidden", result.Error.Message);
+        }
+        if (result.Token == nullptr) {
+            return ReplyErrorAndPassAway("403", "Forbidden", "Empty token");
+        }
+
+        ReplyDeleteCookieAndPassAway(result.Token->GetUserSID());
     }
 
     void HandlePoisonPill(TEvents::TEvPoisonPill::TPtr&) {
         PassAway();
     }
 
+    void HandleTimeout() {
+        ALOG_ERROR(NActorsServices::HTTP, Request->Address << " " << Request->Method << " " << Request->URL << " timeout");
+        ReplyErrorAndPassAway("504", "Gateway Timeout", "Timeout");
+    }
+
     void ReplyOptionsAndPassAway() {
         NHttp::THeadersBuilder headers;
         headers.Set("Allow", "OPTIONS, POST");
@@ -291,12 +342,15 @@ class TLogoutRequest : public NActors::TActorBootstrapped<TLogoutRequest> {
         headers.Set("Access-Control-Allow-Methods", "OPTIONS, GET, POST");
     }
 
-    void ReplyDeleteCookieAndPassAway() {
+    void ReplyDeleteCookieAndPassAway(const TString& userSID) {
         ALOG_DEBUG(NActorsServices::HTTP, "Logout success");
         NHttp::THeadersBuilder headers;
         SetCORS(headers);
         headers.Set("Set-Cookie", "ydb_session_id=; Max-Age=0");
         Send(Sender, new NHttp::TEvHttpProxy::TEvHttpOutgoingResponse(Request->CreateResponse("200", "OK", headers)));
+
+        AuditLogWebUILogout(*Request, userSID);
+
         PassAway();
     }
 
@@ -314,6 +368,7 @@ class TLogoutRequest : public NActors::TActorBootstrapped<TLogoutRequest> {
 protected:
     TActorId Sender;
     NHttp::THttpIncomingRequestPtr Request;
+    TDuration Timeout = TDuration::Seconds(5);
 };
 
 class TLoginService : public TActor<TLoginService> {
diff --git a/ydb/core/security/ya.make b/ydb/core/security/ya.make
@@ -16,6 +16,7 @@ PEERDIR(
     ydb/library/grpc/actor_client
     library/cpp/monlib/service/pages
     library/cpp/openssl/io
+    ydb/core/audit
     ydb/core/base
     ydb/core/protos
     ydb/library/aclib
diff --git a/ydb/core/tx/schemeshard/schemeshard_audit_log.cpp b/ydb/core/tx/schemeshard/schemeshard_audit_log.cpp
@@ -3,6 +3,8 @@
 #include <ydb/public/api/protos/ydb_export.pb.h>
 #include <ydb/public/api/protos/ydb_import.pb.h>
 
+#include <ydb/library/actors/http/http.h>
+
 #include <ydb/core/protos/flat_tx_scheme.pb.h>
 #include <ydb/core/protos/export.pb.h>
 #include <ydb/core/protos/import.pb.h>
@@ -383,6 +385,7 @@ void AuditLogLogin(const NKikimrScheme::TEvLogin& request, const NKikimrScheme::
     TPath databasePath = TPath::Root(SS);
     auto peerName = NKikimr::NAddressClassifier::ExtractAddress(request.GetPeerName());
 
+    // NOTE: audit field set here must be in sync with ydb/core/security/audit_log.h, AuditLogWebUILogout()
     AUDIT_LOG(
         AUDIT_PART("component", SchemeshardComponentName)
         AUDIT_PART("remote_address", (!peerName.empty() ? peerName : EmptyValue))
diff --git a/ydb/core/tx/schemeshard/schemeshard_audit_log.h b/ydb/core/tx/schemeshard/schemeshard_audit_log.h
@@ -20,6 +20,10 @@ class TEvCreateImportRequest;
 class TEvCreateImportResponse;
 }
 
+namespace NHttp {
+class THttpIncomingRequest;
+}
+
 namespace NKikimr::NSchemeShard {
 
 class TSchemeShard;
@@ -36,4 +40,6 @@ void AuditLogImportStart(const NKikimrImport::TEvCreateImportRequest& request, c
 void AuditLogImportEnd(const TImportInfo& importInfo, TSchemeShard* SS);
 
 void AuditLogLogin(const NKikimrScheme::TEvLogin& request, const NKikimrScheme::TEvLoginResult& response, TSchemeShard* SS);
+void AuditLogWebUILogout(const NHttp::THttpIncomingRequest& request, const TString& userSID);
+
 }
diff --git a/ydb/core/tx/schemeshard/ut_login/ut_login.cpp b/ydb/core/tx/schemeshard/ut_login/ut_login.cpp
@@ -1,9 +1,12 @@
 #include <util/string/join.h>
 
+#include <ydb/library/login/login.h>
+#include <ydb/library/actors/http/http_proxy.h>
 #include <ydb/core/tx/schemeshard/ut_helpers/helpers.h>
 #include <ydb/core/tx/schemeshard/ut_helpers/auditlog_helpers.h>
-#include <ydb/library/login/login.h>
 #include <ydb/core/protos/auth.pb.h>
+#include <ydb/core/security/ticket_parser.h>
+#include <ydb/core/security/login_page.h>
 
 using namespace NKikimr;
 using namespace NSchemeShard;
@@ -126,3 +129,152 @@ Y_UNIT_TEST_SUITE(TSchemeShardLoginTest) {
         UNIT_ASSERT_STRING_CONTAINS(last, "login_auth_domain={none}");
     }
 }
+
+namespace NSchemeShardUT_Private {
+
+void EatWholeString(NHttp::THttpIncomingRequestPtr request, const TString& data) {
+    request->EnsureEnoughSpaceAvailable(data.size());
+    auto size = std::min(request->Avail(), data.size());
+    memcpy(request->Pos(), data.data(), size);
+    request->Advance(size);
+}
+
+NHttp::THttpIncomingRequestPtr MakeLoginRequest(const TString& user, const TString& password) {
+    TString payload = [](const auto& user, const auto& password) {
+        NJson::TJsonValue value;
+        value["user"] = user;
+        value["password"] = password;
+        return NJson::WriteJson(value, false);
+    }(user, password);
+    TStringBuilder text;
+    text << "POST /login HTTP/1.1\r\n"
+        << "Host: test.ydb\r\n"
+        << "Content-Type: application/json\r\n"
+        << "Content-Length: " << payload.size() << "\r\n"
+        << "\r\n"
+        << payload;
+    NHttp::THttpIncomingRequestPtr request = new NHttp::THttpIncomingRequest();
+    EatWholeString(request, text);
+    // WebLoginService will crash without address
+    request->Address = std::make_shared<TSockAddrInet>("127.0.0.1", 0);
+    // Cerr << "TEST: http login request: " << text << Endl;
+    return request;
+}
+
+NHttp::THttpIncomingRequestPtr MakeLogoutRequest(const TString& cookieName, const TString& cookieValue) {
+    TStringBuilder text;
+    text << "POST /logout HTTP/1.1\r\n"
+        << "Host: test.ydb\r\n"
+        << "Content-Type: text/plain\r\n"
+        << "Cookie: " << cookieName << "=" << cookieValue << "\r\n"
+        << "\r\n";
+    NHttp::THttpIncomingRequestPtr request = new NHttp::THttpIncomingRequest();
+    EatWholeString(request, text);
+    // WebLoginService will crash without address
+    request->Address = std::make_shared<TSockAddrInet>("127.0.0.1", 0);
+    // Cerr << "TEST: http logout request: " << text << Endl;
+    return request;
+}
+
+}
+
+Y_UNIT_TEST_SUITE(TWebLoginService) {
+
+    Y_UNIT_TEST(Logout) {
+        TTestBasicRuntime runtime;
+        std::vector<std::string> lines;
+        runtime.AuditLogBackends = std::move(CreateTestAuditLogBackends(lines));
+        TTestEnv env(runtime);
+
+        // Add ticket parser to the mix
+        {
+            NKikimrProto::TAuthConfig authConfig;
+            authConfig.SetUseBlackBox(false);
+            authConfig.SetUseLoginProvider(true);
+
+            IActor* ticketParser = NKikimr::CreateTicketParser({.AuthConfig = authConfig});
+            TActorId ticketParserId = runtime.Register(ticketParser);
+            runtime.RegisterService(NKikimr::MakeTicketParserID(), ticketParserId);
+        }
+
+        UNIT_ASSERT_VALUES_EQUAL(lines.size(), 1);  // alter root subdomain
+
+        ui64 txId = 100;
+
+        TestCreateAlterLoginCreateUser(runtime, ++txId, "/MyRoot", "user1", "password1", {{NKikimrScheme::StatusSuccess}});
+        UNIT_ASSERT_VALUES_EQUAL(lines.size(), 2);  // +user creation
+
+        // test body
+        const auto target = runtime.Register(CreateWebLoginService());
+        const auto edge = runtime.AllocateEdgeActor();
+
+        TString ydbSessionId;
+        {
+            runtime.Send(new IEventHandle(target, edge, new NHttp::TEvHttpProxy::TEvHttpIncomingRequest(
+                MakeLoginRequest("user1", "password1")
+            )));
+
+            TAutoPtr<IEventHandle> handle;
+            auto responseEv = runtime.GrabEdgeEvent<NHttp::TEvHttpProxy::TEvHttpOutgoingResponse>(handle);
+            UNIT_ASSERT_STRINGS_EQUAL(responseEv->Response->Status, "200");
+            NHttp::THeaders headers(responseEv->Response->Headers);
+            NHttp::TCookies cookies(headers["Set-Cookie"]);
+            ydbSessionId = cookies["ydb_session_id"];
+        }
+        UNIT_ASSERT_VALUES_EQUAL(lines.size(), 3);  // +user login
+
+        // New security keys are created in the subdomain as a consequence of a login.
+        // In real system they are transferred to the ticket parser by the grpc-proxy
+        // on receiving subdomain update notification.
+        // Here there are no grpc-proxy, so we should transfer keys to the ticket parser manually.
+        {
+            const auto describe = DescribePath(runtime, "/MyRoot");
+            const auto& securityState = describe.GetPathDescription().GetDomainDescription().GetSecurityState();
+            TActorId edge = runtime.AllocateEdgeActor();
+            runtime.Send(new IEventHandle(MakeTicketParserID(), edge, new TEvTicketParser::TEvUpdateLoginSecurityState(securityState)), 0);
+        }
+
+        // Then we are ready to test some authentication on /logout
+        {  // no cookie
+            runtime.Send(new IEventHandle(target, edge, new NHttp::TEvHttpProxy::TEvHttpIncomingRequest(
+                MakeLogoutRequest("not-an-ydb_session_id", ydbSessionId)
+            )));
+
+            TAutoPtr<IEventHandle> handle;
+            auto responseEv = runtime.GrabEdgeEvent<NHttp::TEvHttpProxy::TEvHttpOutgoingResponse>(handle);
+            UNIT_ASSERT_STRINGS_EQUAL(responseEv->Response->Status, "401");
+
+            // no audit record for actions without auth
+            UNIT_ASSERT_VALUES_EQUAL(lines.size(), 3);
+        }
+        {  // bad cookie
+            runtime.Send(new IEventHandle(target, edge, new NHttp::TEvHttpProxy::TEvHttpIncomingRequest(
+                MakeLogoutRequest("ydb_session_id", "jklhagsfjhg")
+            )));
+
+            TAutoPtr<IEventHandle> handle;
+            auto responseEv = runtime.GrabEdgeEvent<NHttp::TEvHttpProxy::TEvHttpOutgoingResponse>(handle);
+            UNIT_ASSERT_STRINGS_EQUAL(responseEv->Response->Status, "403");
+
+            // no audit record for actions without auth
+            UNIT_ASSERT_VALUES_EQUAL(lines.size(), 3);
+        }
+        {  // good cookie
+            runtime.Send(new IEventHandle(target, edge, new NHttp::TEvHttpProxy::TEvHttpIncomingRequest(
+                MakeLogoutRequest("ydb_session_id", ydbSessionId)
+            )));
+
+            TAutoPtr<IEventHandle> handle;
+            auto responseEv = runtime.GrabEdgeEvent<NHttp::TEvHttpProxy::TEvHttpOutgoingResponse>(handle);
+            UNIT_ASSERT_STRINGS_EQUAL(responseEv->Response->Status, "200");
+
+            UNIT_ASSERT_VALUES_EQUAL(lines.size(), 4);  // +user web logout
+
+            auto last = FindAuditLine(lines, "operation=LOGOUT");
+            UNIT_ASSERT_STRING_CONTAINS(last, "component=web-login");
+            UNIT_ASSERT_STRING_CONTAINS(last, "remote_address=");  // can't check the value
+            UNIT_ASSERT_STRING_CONTAINS(last, "operation=LOGOUT");
+            UNIT_ASSERT_STRING_CONTAINS(last, "status=SUCCESS");
+        }
+    }
+}
diff --git a/ydb/library/login/login.cpp b/ydb/library/login/login.cpp
@@ -8,10 +8,7 @@
 #include <openssl/pem.h>
 #include <openssl/rand.h>
 
-#include <util/generic/singleton.h>
 #include <util/string/builder.h>
-#include <util/string/cast.h>
-#include <util/string/hex.h>
 
 #include <deque>
 
@@ -339,7 +336,8 @@ TLoginProvider::TLoginUserResponse TLoginProvider::LoginUser(const TLoginUserReq
             .set_issued_at(now)
             .set_expires_at(expires_at);
     if (!Audience.empty()) {
-        token.set_audience(Audience);
+        // jwt.h require audience claim to be a set
+        token.set_audience(std::set<std::string>{Audience});
     }
 
     if (request.ExternalAuth) {
@@ -381,7 +379,7 @@ TLoginProvider::TValidateTokenResponse TLoginProvider::ValidateToken(const TVali
     try {
         jwt::decoded_jwt decoded_token = jwt::decode(request.Token);
         if (Audience) {
-            // we check audience manually because we wan't this error instead of wrong key id in case of databases mismatch
+            // we check audience manually because we want an explicit error instead of wrong key id in case of databases mismatch
             auto audience = decoded_token.get_audience();
             if (audience.empty() || TString(*audience.begin()) != Audience) {
                 response.Error = "Wrong audience";
@@ -394,7 +392,8 @@ TLoginProvider::TValidateTokenResponse TLoginProvider::ValidateToken(const TVali
             auto verifier = jwt::verify()
                 .allow_algorithm(jwt::algorithm::ps256(key->PublicKey));
             if (Audience) {
-                verifier.with_audience(std::set<std::string>({Audience}));
+                // jwt.h require audience claim to be a set
+                verifier.with_audience(std::set<std::string>{Audience});
             }
             verifier.verify(decoded_token);
             response.User = decoded_token.get_subject();
@@ -435,8 +434,10 @@ TLoginProvider::TValidateTokenResponse TLoginProvider::ValidateToken(const TVali
                 response.Error = "Key not found";
             }
         }
-    } catch (const jwt::token_verification_exception& e) {
+    } catch (const jwt::signature_verification_exception& e) {
         response.Error = e.what(); // invalid token signature
+    } catch (const jwt::token_verification_exception& e) {
+        response.Error = e.what(); // invalid token
     } catch (const std::invalid_argument& e) {
         response.Error = "Token is not in correct format";
         response.TokenUnrecognized = true;

Original file line number	Diff line number	Diff line change
`@@ -20,6 +20,10 @@ class TEvCreateImportRequest;`
`20`	`20`	`class TEvCreateImportResponse;`
`21`	`21`	`}`
`22`	`22`
	`23`	`+namespace NHttp {`
	`24`	`+class THttpIncomingRequest;`
	`25`	`+}`
	`26`	`+`
`23`	`27`	`namespace NKikimr::NSchemeShard {`
`24`	`28`
`25`	`29`	`class TSchemeShard;`
`@@ -36,4 +40,6 @@ void AuditLogImportStart(const NKikimrImport::TEvCreateImportRequest& request, c`
`36`	`40`	`void AuditLogImportEnd(const TImportInfo& importInfo, TSchemeShard* SS);`
`37`	`41`
`38`	`42`	`void AuditLogLogin(const NKikimrScheme::TEvLogin& request, const NKikimrScheme::TEvLoginResult& response, TSchemeShard* SS);`
	`43`	`+void AuditLogWebUILogout(const NHttp::THttpIncomingRequest& request, const TString& userSID);`
	`44`	`+`
`39`	`45`	`}`