Skip to content

Commit 0e68d8e

Browse files
molotkov-andmolotkov-and
authored
Merge 74377a6 into e524a8e 
2 parents e524a8e + 74377a6 commit 0e68d8e

File tree

7 files changed

+181
-26
lines changed

7 files changed

+181
-26
lines changed

‎ydb/core/security/ldap_auth_provider/ldap_auth_provider.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,7 @@ class TLdapAuthProvider : public NActors::TActorBootstrapped<TLdapAuthProvider>
321321
response.Status = NKikimrLdap::ErrorToStatus(result);
322322
response.Error = {.Message = ERROR_MESSAGE, .LogMessage = logErrorMessage, .Retryable = NKikimrLdap::IsRetryableError(result)};
323323
LDAP_LOG_D(logErrorMessage);
324+
NKikimrLdap::MsgFree(searchMessage);
324325
return response;
325326
}
326327
const int countEntries = NKikimrLdap::CountEntries(request.Ld, searchMessage);
@@ -357,6 +358,7 @@ class TLdapAuthProvider : public NActors::TActorBootstrapped<TLdapAuthProvider>
357358
LDAPMessage* searchMessage = nullptr;
358359
int result = NKikimrLdap::Search(ld, Settings.GetBaseDn(), NKikimrLdap::EScope::SUBTREE, filter, NKikimrLdap::noAttributes, 0, &searchMessage);
359360
if (!NKikimrLdap::IsSuccess(result)) {
361+
NKikimrLdap::MsgFree(searchMessage);
360362
return {};
361363
}
362364
const int countEntries = NKikimrLdap::CountEntries(ld, searchMessage);
@@ -403,6 +405,7 @@ class TLdapAuthProvider : public NActors::TActorBootstrapped<TLdapAuthProvider>
403405
LDAPMessage* searchMessage = nullptr;
404406
int result = NKikimrLdap::Search(ld, Settings.GetBaseDn(), NKikimrLdap::EScope::SUBTREE, filter, RequestedAttributes, 0, &searchMessage);
405407
if (!NKikimrLdap::IsSuccess(result)) {
408+
NKikimrLdap::MsgFree(searchMessage);
406409
return;
407410
}
408411
if (NKikimrLdap::CountEntries(ld, searchMessage) == 0) {

‎ydb/core/security/ldap_auth_provider/ldap_auth_provider_linux.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,11 @@ NKikimr::TEvLdapAuthProvider::EStatus ErrorToStatus(int err) {
158158
bool IsRetryableError(int error) {
159159
switch (error) {
160160
case LDAP_SERVER_DOWN:
161+
case LDAP_TIMEOUT:
162+
case LDAP_CONNECT_ERROR:
163+
case LDAP_BUSY:
164+
case LDAP_UNAVAILABLE:
165+
case LDAP_ADMINLIMIT_EXCEEDED:
161166
return true;
162167
}
163168
return false;

‎ydb/core/security/ldap_auth_provider/ldap_auth_provider_ut.cpp

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1168,6 +1168,61 @@ void CheckRequiredLdapSettings(std::function<void(NKikimrProto::TLdapAuthenticat
11681168
ldapServer.Stop();
11691169
}
11701170

1171+
void LdapRefreshGroupsInfoWithError(const ESecurityConnectionType& secureType) {
1172+
TString login = "ldapuser";
1173+
TString password = "ldapUserPassword";
1174+
1175+
TLdapKikimrServer server(InitLdapSettings, secureType);
1176+
auto responses = TCorrectLdapResponse::GetResponses(login);
1177+
LdapMock::TLdapMockResponses updatedResponses = responses;
1178+
LdapMock::TSearchResponseInfo responseServerBusy {
1179+
.ResponseEntries = {}, // Server is busy, can retry attempt
1180+
.ResponseDone = {.Status = LdapMock::EStatus::BUSY}
1181+
};
1182+
1183+
auto& searchResponse = responses.SearchResponses.front();
1184+
searchResponse.second = responseServerBusy;
1185+
LdapMock::TLdapSimpleServer ldapServer(server.GetLdapPort(), {responses, updatedResponses}, secureType == ESecurityConnectionType::LDAPS_SCHEME);
1186+
1187+
auto loginResponse = GetLoginResponse(server, login, password);
1188+
TTestActorRuntime* runtime = server.GetRuntime();
1189+
TActorId sender = runtime->AllocateEdgeActor();
1190+
runtime->Send(new IEventHandle(MakeTicketParserID(), sender, new TEvTicketParser::TEvAuthorizeTicket(loginResponse.Token)), 0);
1191+
TAutoPtr<IEventHandle> handle;
1192+
TEvTicketParser::TEvAuthorizeTicketResult* ticketParserResult = runtime->GrabEdgeEvent<TEvTicketParser::TEvAuthorizeTicketResult>(handle);
1193+
1194+
// Server is busy, return retryable error
1195+
UNIT_ASSERT_C(!ticketParserResult->Error.empty(), "Expected return error message");
1196+
UNIT_ASSERT(ticketParserResult->Token == nullptr);
1197+
UNIT_ASSERT_STRINGS_EQUAL(ticketParserResult->Error.Message, "Could not login via LDAP");
1198+
UNIT_ASSERT_EQUAL(ticketParserResult->Error.Retryable, true);
1199+
1200+
Sleep(TDuration::Seconds(3));
1201+
ldapServer.UpdateResponses();
1202+
Sleep(TDuration::Seconds(7));
1203+
1204+
runtime->Send(new IEventHandle(MakeTicketParserID(), sender, new TEvTicketParser::TEvAuthorizeTicket(loginResponse.Token)), 0);
1205+
ticketParserResult = runtime->GrabEdgeEvent<TEvTicketParser::TEvAuthorizeTicketResult>(handle);
1206+
1207+
// After refresh ticket, server return success
1208+
UNIT_ASSERT_C(ticketParserResult->Error.empty(), ticketParserResult->Error);
1209+
UNIT_ASSERT(ticketParserResult->Token != nullptr);
1210+
const TString ldapDomain = "@ldap";
1211+
UNIT_ASSERT_VALUES_EQUAL(ticketParserResult->Token->GetUserSID(), login + ldapDomain);
1212+
const auto& fetchedGroups = ticketParserResult->Token->GetGroupSIDs();
1213+
THashSet<TString> groups(fetchedGroups.begin(), fetchedGroups.end());
1214+
1215+
THashSet<TString> expectedGroups = TCorrectLdapResponse::GetAllGroups(ldapDomain);
1216+
expectedGroups.insert("all-users@well-known");
1217+
1218+
UNIT_ASSERT_VALUES_EQUAL(fetchedGroups.size(), expectedGroups.size());
1219+
for (const auto& expectedGroup : expectedGroups) {
1220+
UNIT_ASSERT_C(groups.contains(expectedGroup), "Can not find " + expectedGroup);
1221+
}
1222+
1223+
ldapServer.Stop();
1224+
}
1225+
11711226
Y_UNIT_TEST_SUITE(LdapAuthProviderTest) {
11721227
Y_UNIT_TEST(LdapServerIsUnavailable) {
11731228
CheckRequiredLdapSettings(InitLdapSettingsWithUnavailableHost, "Could not login via LDAP", ESecurityConnectionType::START_TLS);
@@ -1246,6 +1301,10 @@ Y_UNIT_TEST_SUITE(LdapAuthProviderTest_LdapsScheme) {
12461301
Y_UNIT_TEST(LdapRefreshRemoveUserBad) {
12471302
LdapRefreshRemoveUserBad(ESecurityConnectionType::LDAPS_SCHEME);
12481303
}
1304+
1305+
Y_UNIT_TEST(LdapRefreshGroupsInfoWithError) {
1306+
LdapRefreshGroupsInfoWithError(ESecurityConnectionType::LDAPS_SCHEME);
1307+
}
12491308
}
12501309

12511310
Y_UNIT_TEST_SUITE(LdapAuthProviderTest_StartTls) {
@@ -1304,6 +1363,10 @@ Y_UNIT_TEST_SUITE(LdapAuthProviderTest_StartTls) {
13041363
Y_UNIT_TEST(LdapRefreshRemoveUserBad) {
13051364
LdapRefreshRemoveUserBad(ESecurityConnectionType::START_TLS);
13061365
}
1366+
1367+
Y_UNIT_TEST(LdapRefreshGroupsInfoWithError) {
1368+
LdapRefreshGroupsInfoWithError(ESecurityConnectionType::START_TLS);
1369+
}
13071370
}
13081371

13091372
Y_UNIT_TEST_SUITE(LdapAuthProviderTest_nonSecure) {
@@ -1362,6 +1425,10 @@ Y_UNIT_TEST_SUITE(LdapAuthProviderTest_nonSecure) {
13621425
Y_UNIT_TEST(LdapRefreshRemoveUserBad) {
13631426
LdapRefreshRemoveUserBad(ESecurityConnectionType::NON_SECURE);
13641427
}
1428+
1429+
Y_UNIT_TEST(LdapRefreshGroupsInfoWithError) {
1430+
LdapRefreshGroupsInfoWithError(ESecurityConnectionType::NON_SECURE);
1431+
}
13651432
}
13661433

13671434
} // NKikimr

‎ydb/core/security/ldap_auth_provider/ldap_auth_provider_win.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,11 @@ NKikimr::TEvLdapAuthProvider::EStatus ErrorToStatus(int err) {
147147
bool IsRetryableError(int error) {
148148
switch (error) {
149149
case LDAP_SERVER_DOWN:
150+
case LDAP_TIMEOUT:
151+
case LDAP_CONNECT_ERROR:
152+
case LDAP_BUSY:
153+
case LDAP_UNAVAILABLE:
154+
case LDAP_ADMIN_LIMIT_EXCEEDED:
150155
return true;
151156
}
152157
return false;

‎ydb/core/security/ticket_parser_impl.h

Lines changed: 34 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1741,12 +1741,16 @@ class TTicketParserImpl : public TActorBootstrapped<TDerived> {
17411741
void SetError(const TString& key, TTokenRecord& record, const TEvTicketParser::TError& error) {
17421742
record.Error = error;
17431743
TInstant now = TlsActivationContext->Now();
1744+
TStringBuilder errorLogMessage;
1745+
if (error.HasLogMessage()) {
1746+
errorLogMessage << " (" << error.LogMessage << ")";
1747+
}
17441748
if (record.Error.Retryable) {
17451749
record.ExpireTime = GetExpireTime(record, now);
17461750
record.SetErrorRefreshTime(this, now);
17471751
CounterTicketsErrorsRetryable->Inc();
17481752
BLOG_D("Ticket " << record.GetMaskedTicket() << " ("
1749-
<< record.PeerName << ") has now retryable error message '" << error.Message << "'");
1753+
<< record.PeerName << ") has now retryable error message '" << error.Message << errorLogMessage << "'");
17501754
if (record.RefreshRetryableErrorImmediately) {
17511755
record.RefreshRetryableErrorImmediately = false;
17521756
GetDerived()->CanRefreshTicket(key, record);
@@ -1759,7 +1763,7 @@ class TTicketParserImpl : public TActorBootstrapped<TDerived> {
17591763
record.SetOkRefreshTime(this, now);
17601764
CounterTicketsErrorsPermanent->Inc();
17611765
BLOG_D("Ticket " << record.GetMaskedTicket() << " ("
1762-
<< record.PeerName << ") has now permanent error message '" << error.Message << "'");
1766+
<< record.PeerName << ") has now permanent error message '" << error.Message << errorLogMessage << "'");
17631767
}
17641768
CounterTicketsErrors->Inc();
17651769
record.IsLowAccessServiceRequestPriority = true;
@@ -1841,7 +1845,7 @@ class TTicketParserImpl : public TActorBootstrapped<TDerived> {
18411845

18421846
template <typename TTokenRecord>
18431847
bool CanRefreshLoginTicket(const TTokenRecord& record) {
1844-
return record.TokenType == TDerived::ETokenType::Login && record.Error.empty();
1848+
return record.TokenType == TDerived::ETokenType::Login && record.Error.Retryable;
18451849
}
18461850

18471851
template <typename TTokenRecord>
@@ -1862,30 +1866,34 @@ class TTicketParserImpl : public TActorBootstrapped<TDerived> {
18621866

18631867
template <typename TTokenRecord>
18641868
bool RefreshLoginTicket(const TString& key, TTokenRecord& record) {
1865-
GetDerived()->ResetTokenRecord(record);
1866-
const TString userSID = record.GetToken()->GetUserSID();
1867-
if (record.IsExternalAuthEnabled()) {
1868-
return RefreshTicketViaExternalAuthProvider(key, record);
1869-
}
1870-
const TString& database = Config.GetDomainLoginOnly() ? DomainName : record.Database;
1871-
auto itLoginProvider = LoginProviders.find(database);
1872-
if (itLoginProvider == LoginProviders.end()) {
1873-
return false;
1874-
}
1875-
NLogin::TLoginProvider& loginProvider(itLoginProvider->second);
1876-
if (loginProvider.CheckUserExists(userSID)) {
1877-
const std::vector<TString> providerGroups = loginProvider.GetGroupsMembership(userSID);
1878-
const TVector<NACLib::TSID> groups(providerGroups.begin(), providerGroups.end());
1879-
SetToken(key, record, new NACLib::TUserToken({
1880-
.OriginalUserToken = record.Ticket,
1881-
.UserSID = userSID,
1882-
.GroupSIDs = groups,
1883-
.AuthType = record.GetAuthType()
1884-
}));
1885-
} else {
1886-
SetError(key, record, {.Message = "User not found", .Retryable = false});
1869+
if (record.Error.empty()) {
1870+
GetDerived()->ResetTokenRecord(record);
1871+
const TString userSID = record.GetToken()->GetUserSID();
1872+
if (record.IsExternalAuthEnabled()) {
1873+
return RefreshTicketViaExternalAuthProvider(key, record);
1874+
}
1875+
const TString& database = Config.GetDomainLoginOnly() ? DomainName : record.Database;
1876+
auto itLoginProvider = LoginProviders.find(database);
1877+
if (itLoginProvider == LoginProviders.end()) {
1878+
return false;
1879+
}
1880+
NLogin::TLoginProvider& loginProvider(itLoginProvider->second);
1881+
if (loginProvider.CheckUserExists(userSID)) {
1882+
const std::vector<TString> providerGroups = loginProvider.GetGroupsMembership(userSID);
1883+
const TVector<NACLib::TSID> groups(providerGroups.begin(), providerGroups.end());
1884+
SetToken(key, record, new NACLib::TUserToken({
1885+
.OriginalUserToken = record.Ticket,
1886+
.UserSID = userSID,
1887+
.GroupSIDs = groups,
1888+
.AuthType = record.GetAuthType()
1889+
}));
1890+
} else {
1891+
SetError(key, record, {.Message = "User not found", .Retryable = false});
1892+
}
1893+
return true;
18871894
}
1888-
return true;
1895+
GetDerived()->ResetTokenRecord(record);
1896+
return CanInitLoginToken(key, record);
18891897
}
18901898

18911899
template <typename TTokenRecord>

‎ydb/core/security/ticket_parser_ut.cpp

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,72 @@ Y_UNIT_TEST_SUITE(TTicketParserTest) {
278278
UNIT_ASSERT_VALUES_EQUAL(result->Token->GetGroupSIDs().size(), 3);
279279
}
280280

281+
Y_UNIT_TEST(LoginRefreshGroupsWithError) {
282+
using namespace Tests;
283+
TPortManager tp;
284+
ui16 kikimrPort = tp.GetPort(2134);
285+
ui16 grpcPort = tp.GetPort(2135);
286+
NKikimrProto::TAuthConfig authConfig;
287+
authConfig.SetUseBlackBox(false);
288+
authConfig.SetUseLoginProvider(true);
289+
authConfig.SetRefreshTime("5s");
290+
auto settings = TServerSettings(kikimrPort, authConfig);
291+
settings.SetDomainName("Root");
292+
settings.CreateTicketParser = NKikimr::CreateTicketParser;
293+
TServer server(settings);
294+
server.EnableGRpc(grpcPort);
295+
server.GetRuntime()->SetLogPriority(NKikimrServices::TICKET_PARSER, NLog::PRI_TRACE);
296+
server.GetRuntime()->SetLogPriority(NKikimrServices::GRPC_CLIENT, NLog::PRI_TRACE);
297+
TClient client(settings);
298+
NClient::TKikimr kikimr(client.GetClientConfig());
299+
client.InitRootScheme();
300+
TTestActorRuntime* runtime = server.GetRuntime();
301+
302+
NLogin::TLoginProvider provider;
303+
304+
provider.Audience = "/Root";
305+
provider.RotateKeys();
306+
307+
TActorId sender = runtime->AllocateEdgeActor();
308+
309+
provider.CreateGroup({.Group = "group1"});
310+
provider.CreateUser({.User = "user1", .Password = "password1"});
311+
provider.AddGroupMembership({.Group = "group1", .Member = "user1"});
312+
313+
NLogin::TLoginProvider emptyProvider;
314+
emptyProvider.Audience = "/Root";
315+
316+
runtime->Send(new IEventHandle(MakeTicketParserID(), sender, new TEvTicketParser::TEvUpdateLoginSecurityState(emptyProvider.GetSecurityState())), 0);
317+
318+
auto loginResponse = provider.LoginUser({.User = "user1", .Password = "password1"});
319+
320+
UNIT_ASSERT_VALUES_EQUAL(loginResponse.Error, "");
321+
322+
runtime->Send(new IEventHandle(MakeTicketParserID(), sender, new TEvTicketParser::TEvAuthorizeTicket(loginResponse.Token)), 0);
323+
324+
TAutoPtr<IEventHandle> handle;
325+
326+
TEvTicketParser::TEvAuthorizeTicketResult* result = runtime->GrabEdgeEvent<TEvTicketParser::TEvAuthorizeTicketResult>(handle);
327+
UNIT_ASSERT_C(!result->Error.empty(), "Expected return error message");
328+
UNIT_ASSERT(result->Token == nullptr);
329+
UNIT_ASSERT_STRINGS_EQUAL(result->Error.Message, "Security state is empty");
330+
UNIT_ASSERT_EQUAL(result->Error.Retryable, true);
331+
332+
Sleep(TDuration::Seconds(3));
333+
runtime->Send(new IEventHandle(MakeTicketParserID(), sender, new TEvTicketParser::TEvUpdateLoginSecurityState(provider.GetSecurityState())), 0);
334+
Sleep(TDuration::Seconds(7));
335+
336+
runtime->Send(new IEventHandle(MakeTicketParserID(), sender, new TEvTicketParser::TEvAuthorizeTicket(loginResponse.Token)), 0);
337+
338+
result = runtime->GrabEdgeEvent<TEvTicketParser::TEvAuthorizeTicketResult>(handle);
339+
340+
UNIT_ASSERT_C(result->Error.empty(), result->Error);
341+
UNIT_ASSERT(result->Token != nullptr);
342+
UNIT_ASSERT_VALUES_EQUAL(result->Token->GetUserSID(), "user1");
343+
UNIT_ASSERT(result->Token->IsExist("group1"));
344+
UNIT_ASSERT_VALUES_EQUAL(result->Token->GetGroupSIDs().size(), 2);
345+
}
346+
281347
Y_UNIT_TEST(LoginCheckRemovedUser) {
282348
using namespace Tests;
283349
TPortManager tp;

‎ydb/library/testlib/service_mocks/ldap_mock/ldap_defines.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ enum EStatus {
1212
SUCCESS = 0x00,
1313
PROTOCOL_ERROR = 0x02,
1414
INVALID_CREDENTIALS = 0x31,
15+
BUSY = 0x33,
1516
};
1617

1718
enum EProtocolOp {

0 commit comments

Comments
 (0)