Skip to content

Commit 42430e8

Browse files
authored
Fix NPE in auditing authenticationSuccess for non-existing run-as user (#91171)
When run-as fails because the target user does not exist, the authentication is created with a null lookup realm. It is then rejected at authorization time. But for authentication, it is treated as success. This can lead to NPE when auditing the authenticationSuccess event. This PR fixes the NPE by checking whether lookup realm is null before using it. Relates: #91126 (comment)
1 parent 315d4c7 commit 42430e8

File tree

3 files changed

+47
-11
lines changed

3 files changed

+47
-11
lines changed

docs/changelog/91171.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 91171
2+
summary: Fix NPE in auditing `authenticationSuccess` for non-existing run-as user
3+
area: Audit
4+
type: bug
5+
issues: []

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1629,14 +1629,16 @@ LogEntryBuilder withAuthentication(Authentication authentication) {
16291629
final Authentication.RealmRef authenticatedBy = authentication.getAuthenticatingSubject().getRealm();
16301630
if (authentication.isRunAs()) {
16311631
final Authentication.RealmRef lookedUpBy = authentication.getEffectiveSubject().getRealm();
1632-
logEntry.with(PRINCIPAL_REALM_FIELD_NAME, lookedUpBy.getName())
1633-
.with(PRINCIPAL_RUN_BY_FIELD_NAME, authentication.getAuthenticatingSubject().getUser().principal())
1632+
if (lookedUpBy != null) {
1633+
logEntry.with(PRINCIPAL_REALM_FIELD_NAME, lookedUpBy.getName());
1634+
if (lookedUpBy.getDomain() != null) {
1635+
logEntry.with(PRINCIPAL_DOMAIN_FIELD_NAME, lookedUpBy.getDomain().name());
1636+
}
1637+
}
1638+
logEntry.with(PRINCIPAL_RUN_BY_FIELD_NAME, authentication.getAuthenticatingSubject().getUser().principal())
16341639
// API key can run-as, when that happens, the following field will be _es_api_key,
16351640
// not the API key owner user's realm.
16361641
.with(PRINCIPAL_RUN_BY_REALM_FIELD_NAME, authenticatedBy.getName());
1637-
if (lookedUpBy.getDomain() != null) {
1638-
logEntry.with(PRINCIPAL_DOMAIN_FIELD_NAME, lookedUpBy.getDomain().name());
1639-
}
16401642
if (authenticatedBy.getDomain() != null) {
16411643
logEntry.with(PRINCIPAL_RUN_BY_DOMAIN_FIELD_NAME, authenticatedBy.getDomain().name());
16421644
}

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2471,7 +2471,6 @@ public void testAuthenticationSuccessRest() throws Exception {
24712471
traceId(threadContext, checkedFields);
24722472
forwardedFor(threadContext, checkedFields);
24732473
assertMsg(logger, checkedFields.map());
2474-
24752474
CapturingLogger.output(logger.getName(), Level.INFO).clear();
24762475

24772476
// audit for authn with API Key
@@ -2497,6 +2496,32 @@ public void testAuthenticationSuccessRest() throws Exception {
24972496
traceId(threadContext, checkedFields);
24982497
forwardedFor(threadContext, checkedFields);
24992498
assertMsg(logger, checkedFields.map());
2499+
CapturingLogger.output(logger.getName(), Level.INFO).clear();
2500+
2501+
// authentication success but run-as user does not exist
2502+
authentication = AuthenticationTestHelper.builder().realm().build(false).runAs(new User(randomAlphaOfLengthBetween(3, 8)), null);
2503+
checkedFields = new MapBuilder<>(commonFields);
2504+
auditTrail.authenticationSuccess(requestId, authentication, request);
2505+
checkedFields.put(LoggingAuditTrail.EVENT_TYPE_FIELD_NAME, LoggingAuditTrail.REST_ORIGIN_FIELD_VALUE)
2506+
.put(LoggingAuditTrail.EVENT_ACTION_FIELD_NAME, "authentication_success")
2507+
.put(LoggingAuditTrail.REALM_FIELD_NAME, authentication.getAuthenticatingSubject().getRealm().getName())
2508+
.put(LoggingAuditTrail.ORIGIN_TYPE_FIELD_NAME, LoggingAuditTrail.REST_ORIGIN_FIELD_VALUE)
2509+
.put(LoggingAuditTrail.ORIGIN_ADDRESS_FIELD_NAME, NetworkAddress.format(address))
2510+
.put(LoggingAuditTrail.REQUEST_METHOD_FIELD_NAME, request.method().toString())
2511+
.put(LoggingAuditTrail.REQUEST_ID_FIELD_NAME, requestId)
2512+
.put(LoggingAuditTrail.URL_PATH_FIELD_NAME, "_uri");
2513+
if (includeRequestBody && Strings.hasLength(expectedMessage)) {
2514+
checkedFields.put(LoggingAuditTrail.REQUEST_BODY_FIELD_NAME, expectedMessage);
2515+
}
2516+
if (params.isEmpty() == false) {
2517+
checkedFields.put(LoggingAuditTrail.URL_QUERY_FIELD_NAME, "foo=bar&evac=true");
2518+
}
2519+
authentication(authentication, checkedFields);
2520+
opaqueId(threadContext, checkedFields);
2521+
traceId(threadContext, checkedFields);
2522+
forwardedFor(threadContext, checkedFields);
2523+
assertMsg(logger, checkedFields.map());
2524+
CapturingLogger.output(logger.getName(), Level.INFO).clear();
25002525
}
25012526

25022527
public void testAuthenticationSuccessTransport() throws Exception {
@@ -2896,12 +2921,16 @@ private static void authentication(Authentication authentication, MapBuilder<Str
28962921
final RealmRef authenticatedBy = authentication.getAuthenticatingSubject().getRealm();
28972922
if (authentication.isRunAs()) {
28982923
final RealmRef lookedUpBy = authentication.getEffectiveSubject().getRealm();
2899-
checkedFields.put(LoggingAuditTrail.PRINCIPAL_REALM_FIELD_NAME, lookedUpBy.getName())
2900-
.put(LoggingAuditTrail.PRINCIPAL_RUN_BY_FIELD_NAME, authentication.getAuthenticatingSubject().getUser().principal())
2901-
.put(LoggingAuditTrail.PRINCIPAL_RUN_BY_REALM_FIELD_NAME, authenticatedBy.getName());
2902-
if (lookedUpBy.getDomain() != null) {
2903-
checkedFields.put(LoggingAuditTrail.PRINCIPAL_DOMAIN_FIELD_NAME, lookedUpBy.getDomain().name());
2924+
if (lookedUpBy != null) {
2925+
checkedFields.put(LoggingAuditTrail.PRINCIPAL_REALM_FIELD_NAME, lookedUpBy.getName());
2926+
if (lookedUpBy.getDomain() != null) {
2927+
checkedFields.put(LoggingAuditTrail.PRINCIPAL_DOMAIN_FIELD_NAME, lookedUpBy.getDomain().name());
2928+
}
29042929
}
2930+
checkedFields.put(
2931+
LoggingAuditTrail.PRINCIPAL_RUN_BY_FIELD_NAME,
2932+
authentication.getAuthenticatingSubject().getUser().principal()
2933+
).put(LoggingAuditTrail.PRINCIPAL_RUN_BY_REALM_FIELD_NAME, authenticatedBy.getName());
29052934
if (authenticatedBy.getDomain() != null) {
29062935
checkedFields.put(LoggingAuditTrail.PRINCIPAL_RUN_BY_DOMAIN_FIELD_NAME, authenticatedBy.getDomain().name());
29072936
}

0 commit comments

Comments
 (0)