Skip to content

Commit

Permalink
Fix NPE in auditing authenticationSuccess for non-existing run-as user (
Browse files Browse the repository at this point in the history
#91171) (#91240)

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)
  • Loading branch information
ywangd committed Nov 2, 2022
1 parent 4be14ee commit 3f24a51
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 11 deletions.
5 changes: 5 additions & 0 deletions docs/changelog/91171.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 91171
summary: Fix NPE in auditing `authenticationSuccess` for non-existing run-as user
area: Audit
type: bug
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -1623,14 +1623,16 @@ LogEntryBuilder withAuthentication(Authentication authentication) {
final Authentication.RealmRef authenticatedBy = authentication.getAuthenticatedBy();
if (authentication.isRunAs()) {
final Authentication.RealmRef lookedUpBy = authentication.getLookedUpBy();
logEntry.with(PRINCIPAL_REALM_FIELD_NAME, lookedUpBy.getName())
.with(PRINCIPAL_RUN_BY_FIELD_NAME, authentication.getAuthenticatingSubject().getUser().principal())
if (lookedUpBy != null) {
logEntry.with(PRINCIPAL_REALM_FIELD_NAME, lookedUpBy.getName());
if (lookedUpBy.getDomain() != null) {
logEntry.with(PRINCIPAL_DOMAIN_FIELD_NAME, lookedUpBy.getDomain().name());
}
}
logEntry.with(PRINCIPAL_RUN_BY_FIELD_NAME, authentication.getAuthenticatingSubject().getUser().principal())
// API key can run-as, when that happens, the following field will be _es_api_key,
// not the API key owner user's realm.
.with(PRINCIPAL_RUN_BY_REALM_FIELD_NAME, authenticatedBy.getName());
if (lookedUpBy.getDomain() != null) {
logEntry.with(PRINCIPAL_DOMAIN_FIELD_NAME, lookedUpBy.getDomain().name());
}
if (authenticatedBy.getDomain() != null) {
logEntry.with(PRINCIPAL_RUN_BY_DOMAIN_FIELD_NAME, authenticatedBy.getDomain().name());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2458,7 +2458,6 @@ public void testAuthenticationSuccessRest() throws Exception {
traceId(threadContext, checkedFields);
forwardedFor(threadContext, checkedFields);
assertMsg(logger, checkedFields.map());

CapturingLogger.output(logger.getName(), Level.INFO).clear();

// audit for authn with API Key
Expand All @@ -2484,6 +2483,32 @@ public void testAuthenticationSuccessRest() throws Exception {
traceId(threadContext, checkedFields);
forwardedFor(threadContext, checkedFields);
assertMsg(logger, checkedFields.map());
CapturingLogger.output(logger.getName(), Level.INFO).clear();

// authentication success but run-as user does not exist
authentication = AuthenticationTestHelper.builder().realm().build(false).runAs(new User(randomAlphaOfLengthBetween(3, 8)), null);
checkedFields = new MapBuilder<>(commonFields);
auditTrail.authenticationSuccess(requestId, authentication, request);
checkedFields.put(LoggingAuditTrail.EVENT_TYPE_FIELD_NAME, LoggingAuditTrail.REST_ORIGIN_FIELD_VALUE)
.put(LoggingAuditTrail.EVENT_ACTION_FIELD_NAME, "authentication_success")
.put(LoggingAuditTrail.REALM_FIELD_NAME, authentication.getAuthenticatingSubject().getRealm().getName())
.put(LoggingAuditTrail.ORIGIN_TYPE_FIELD_NAME, LoggingAuditTrail.REST_ORIGIN_FIELD_VALUE)
.put(LoggingAuditTrail.ORIGIN_ADDRESS_FIELD_NAME, NetworkAddress.format(address))
.put(LoggingAuditTrail.REQUEST_METHOD_FIELD_NAME, request.method().toString())
.put(LoggingAuditTrail.REQUEST_ID_FIELD_NAME, requestId)
.put(LoggingAuditTrail.URL_PATH_FIELD_NAME, "_uri");
if (includeRequestBody && Strings.hasLength(expectedMessage)) {
checkedFields.put(LoggingAuditTrail.REQUEST_BODY_FIELD_NAME, expectedMessage);
}
if (params.isEmpty() == false) {
checkedFields.put(LoggingAuditTrail.URL_QUERY_FIELD_NAME, "foo=bar&evac=true");
}
authentication(authentication, checkedFields);
opaqueId(threadContext, checkedFields);
traceId(threadContext, checkedFields);
forwardedFor(threadContext, checkedFields);
assertMsg(logger, checkedFields.map());
CapturingLogger.output(logger.getName(), Level.INFO).clear();
}

public void testAuthenticationSuccessTransport() throws Exception {
Expand Down Expand Up @@ -2881,12 +2906,16 @@ private static void authentication(Authentication authentication, MapBuilder<Str
final RealmRef authenticatedBy = authentication.getAuthenticatedBy();
if (authentication.isRunAs()) {
final RealmRef lookedUpBy = authentication.getLookedUpBy();
checkedFields.put(LoggingAuditTrail.PRINCIPAL_REALM_FIELD_NAME, lookedUpBy.getName())
.put(LoggingAuditTrail.PRINCIPAL_RUN_BY_FIELD_NAME, authentication.getAuthenticatingSubject().getUser().principal())
.put(LoggingAuditTrail.PRINCIPAL_RUN_BY_REALM_FIELD_NAME, authenticatedBy.getName());
if (lookedUpBy.getDomain() != null) {
checkedFields.put(LoggingAuditTrail.PRINCIPAL_DOMAIN_FIELD_NAME, lookedUpBy.getDomain().name());
if (lookedUpBy != null) {
checkedFields.put(LoggingAuditTrail.PRINCIPAL_REALM_FIELD_NAME, lookedUpBy.getName());
if (lookedUpBy.getDomain() != null) {
checkedFields.put(LoggingAuditTrail.PRINCIPAL_DOMAIN_FIELD_NAME, lookedUpBy.getDomain().name());
}
}
checkedFields.put(
LoggingAuditTrail.PRINCIPAL_RUN_BY_FIELD_NAME,
authentication.getAuthenticatingSubject().getUser().principal()
).put(LoggingAuditTrail.PRINCIPAL_RUN_BY_REALM_FIELD_NAME, authenticatedBy.getName());
if (authenticatedBy.getDomain() != null) {
checkedFields.put(LoggingAuditTrail.PRINCIPAL_RUN_BY_DOMAIN_FIELD_NAME, authenticatedBy.getDomain().name());
}
Expand Down

0 comments on commit 3f24a51

Please sign in to comment.