Fix auditing of nameless API Keys (#59531)

API keys can be created nameless using the grant endpoint (it is a bug, see #59484). This change ensures auditing doesn't throw when such an API Key is used for authn.
elastic · Jul 14, 2020 · 6d6d565 · 6d6d565
1 parent 4eb310c
commit 6d6d565
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 9 deletions.
diff --git a/...urity/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java b/...urity/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java
@@ -832,8 +832,11 @@ LogEntryBuilder withAuthentication(Authentication authentication) {
             logEntry.with(PRINCIPAL_FIELD_NAME, authentication.getUser().principal());
             logEntry.with(AUTHENTICATION_TYPE_FIELD_NAME, authentication.getAuthenticationType().toString());
             if (Authentication.AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
-                logEntry.with(API_KEY_ID_FIELD_NAME, (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY))
-                        .with(API_KEY_NAME_FIELD_NAME, (String) authentication.getMetadata().get(ApiKeyService.API_KEY_NAME_KEY));
+                logEntry.with(API_KEY_ID_FIELD_NAME, (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY));
+                String apiKeyName = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_NAME_KEY);
+                if (apiKeyName != null) {
+                    logEntry.with(API_KEY_NAME_FIELD_NAME, apiKeyName);
+                }
                 String creatorRealmName = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_CREATOR_REALM_NAME);
                 if (creatorRealmName != null) {
                     // can be null for API keys created before version 7.7

diff --git a/.../src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java b/.../src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java
@@ -1455,9 +1455,11 @@ private static void authentication(Authentication authentication, MapBuilder<Str
         if (Authentication.AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
             assert false == authentication.getUser().isRunAs();
             checkedFields.put(LoggingAuditTrail.API_KEY_ID_FIELD_NAME,
-                    (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY))
-                    .put(LoggingAuditTrail.API_KEY_NAME_FIELD_NAME,
-                            (String) authentication.getMetadata().get(ApiKeyService.API_KEY_NAME_KEY));
+                    (String) authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY));
+            String apiKeyName = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_NAME_KEY);
+            if (apiKeyName != null) {
+                checkedFields.put(LoggingAuditTrail.API_KEY_NAME_FIELD_NAME, apiKeyName);
+            }
             String creatorRealmName = (String) authentication.getMetadata().get(ApiKeyService.API_KEY_CREATOR_REALM_NAME);
             if (creatorRealmName != null) {
                 checkedFields.put(LoggingAuditTrail.PRINCIPAL_REALM_FIELD_NAME, creatorRealmName);

diff --git a/...gin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java b/...gin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java
@@ -866,10 +866,18 @@ public static Authentication createApiKeyAuthentication(ApiKeyService apiKeyServ
             AuthenticationResult authenticationResult = authenticationResultFuture.get();
             if (randomBoolean()) {
                 // maybe remove realm name to simulate old API Key authentication
+                assert authenticationResult.getStatus() == AuthenticationResult.Status.SUCCESS;
                 Map<String, Object> authenticationResultMetadata = new HashMap<>(authenticationResult.getMetadata());
                 authenticationResultMetadata.remove(ApiKeyService.API_KEY_CREATOR_REALM_NAME);
                 authenticationResult = AuthenticationResult.success(authenticationResult.getUser(), authenticationResultMetadata);
             }
+            if (randomBoolean()) {
+                // simulate authentication with nameless API Key, see https://github.com/elastic/elasticsearch/issues/59484
+                assert authenticationResult.getStatus() == AuthenticationResult.Status.SUCCESS;
+                Map<String, Object> authenticationResultMetadata = new HashMap<>(authenticationResult.getMetadata());
+                authenticationResultMetadata.remove(ApiKeyService.API_KEY_NAME_KEY);
+                authenticationResult = AuthenticationResult.success(authenticationResult.getUser(), authenticationResultMetadata);
+            }
 
             final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
             final SecurityContext securityContext = new SecurityContext(Settings.EMPTY, threadContext);

diff --git a/.../rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/120_api_key_auth.yml b/.../rolling-upgrade/src/test/resources/rest-api-spec/test/mixed_cluster/120_api_key_auth.yml
@@ -3,18 +3,17 @@
 
   - skip:
       features: headers
-      version: "all"
-      reason: "API key realm name is in metadata since v7.5. https://github.com/elastic/elasticsearch/issues/59425"
 
   - do:
       security.create_api_key:
         body:  >
           {
-            "name": "my-api-key"
+            "name": "api-key-in-mixed-cluster"
           }
-  - match: { name: "my-api-key" }
+  - match: { name: "api-key-in-mixed-cluster" }
   - is_true: id
   - is_true: api_key
+  - set: { id: api_key_id }
   - transform_and_set: { login_creds: "#base64EncodeCredentials(id,api_key)" }
 
   - do:
@@ -23,3 +22,13 @@
       nodes.info: {}
   - match: { _nodes.failed: 0 }
 
+  - do:
+      security.invalidate_api_key:
+        body:  >
+          {
+            "id": "${api_key_id}"
+          }
+  - length: { "invalidated_api_keys" : 1 }
+  - match: { "invalidated_api_keys.0" : "${api_key_id}" }
+  - length: { "previously_invalidated_api_keys" : 0 }
+  - match: { "error_count" : 0 }