Skip to content

Commit

Permalink
auditlog: add logouts (#9050)
Browse files Browse the repository at this point in the history
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 logout was anonymous.

KIKIMR-21764
  • Loading branch information
ijon authored Sep 11, 2024
1 parent adb5d68 commit ab1832e
Show file tree
Hide file tree
Showing 6 changed files with 229 additions and 10 deletions.
59 changes: 57 additions & 2 deletions ydb/core/security/login_page.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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> {
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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");
Expand All @@ -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();
}

Expand All @@ -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> {
Expand Down
1 change: 1 addition & 0 deletions ydb/core/security/ya.make
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions ydb/core/tx/schemeshard/schemeshard_audit_log.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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/login_page.cpp, AuditLogWebUILogout()
AUDIT_LOG(
AUDIT_PART("component", SchemeshardComponentName)
AUDIT_PART("remote_address", (!peerName.empty() ? peerName : EmptyValue))
Expand Down
6 changes: 6 additions & 0 deletions ydb/core/tx/schemeshard/schemeshard_audit_log.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ class TEvCreateImportRequest;
class TEvCreateImportResponse;
}

namespace NHttp {
class THttpIncomingRequest;
}

namespace NKikimr::NSchemeShard {

class TSchemeShard;
Expand All @@ -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);

}
155 changes: 154 additions & 1 deletion ydb/core/tx/schemeshard/ut_login/ut_login.cpp
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -123,3 +126,153 @@ 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=127.0.0.1");
UNIT_ASSERT_STRING_CONTAINS(last, "subject=user1");
UNIT_ASSERT_STRING_CONTAINS(last, "operation=LOGOUT");
UNIT_ASSERT_STRING_CONTAINS(last, "status=SUCCESS");
}
}
}
15 changes: 8 additions & 7 deletions ydb/library/login/login.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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";
Expand All @@ -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();
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit ab1832e

Please sign in to comment.