auditlog: add logouts

ijon · ijon · commit 1ea1662e5474 · 2024-09-11T02:54:01.000+03:00
Add audit logging for web logout operation.
Web logout now require proper authentication as audit record
without name of the logged out user is meaningless.
Previously web lougout 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;
@@ -123,3 +126,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`	`}`