From 11231edff502dac79e1977f1daa4eae2bd186543 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 28 Oct 2022 16:00:52 +1100 Subject: [PATCH 1/2] Fix NPE in auditing authenticationSuccess when run-as user does not exist 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: https://github.com/elastic/elasticsearch/pull/91126#discussion_r1005472501 --- .../audit/logfile/LoggingAuditTrail.java | 12 +++--- .../audit/logfile/LoggingAuditTrailTests.java | 41 ++++++++++++++++--- 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java index 36d4e82011bea..ae200aa7c4ae7 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java @@ -1629,14 +1629,16 @@ LogEntryBuilder withAuthentication(Authentication authentication) { final Authentication.RealmRef authenticatedBy = authentication.getAuthenticatingSubject().getRealm(); if (authentication.isRunAs()) { final Authentication.RealmRef lookedUpBy = authentication.getEffectiveSubject().getRealm(); - 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()); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java index 5b4e62506c7cd..e458d4ef7666e 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java @@ -2471,7 +2471,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 @@ -2497,6 +2496,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 { @@ -2896,12 +2921,16 @@ private static void authentication(Authentication authentication, MapBuilder Date: Fri, 28 Oct 2022 16:06:47 +1100 Subject: [PATCH 2/2] Update docs/changelog/91171.yaml --- docs/changelog/91171.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/91171.yaml diff --git a/docs/changelog/91171.yaml b/docs/changelog/91171.yaml new file mode 100644 index 0000000000000..5f5fd84b8dbbf --- /dev/null +++ b/docs/changelog/91171.yaml @@ -0,0 +1,5 @@ +pr: 91171 +summary: Fix NPE in auditing `authenticationSuccess` for non-existing run-as user +area: Audit +type: bug +issues: []