[Alerting] Replaced single invalidateApiKey request with the bulk. (#…

…87401) * Replaced single invalidateApiKey request with the bulk * fixed failing test * Extended invalidate method to support multiple invalidation. Updated fleets plugin usage of this API. * fixed due to comments
elastic · Jan 7, 2021 · 794c6b3 · 794c6b3
1 parent f4042dd
commit 794c6b3
Show file tree

Hide file tree

Showing 10 changed files with 69 additions and 65 deletions.
diff --git a/x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts b/x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
@@ -12,7 +12,7 @@ import {
   SavedObjectsClientContract,
 } from 'kibana/server';
 import { EncryptedSavedObjectsClient } from '../../../encrypted_saved_objects/server';
-import { InvalidateAPIKeyParams, SecurityPluginStart } from '../../../security/server';
+import { InvalidateAPIKeysParams, SecurityPluginStart } from '../../../security/server';
 import {
   RunContext,
   TaskManagerSetupContract,
@@ -27,8 +27,8 @@ import { InvalidatePendingApiKey } from '../types';
 const TASK_TYPE = 'alerts_invalidate_api_keys';
 export const TASK_ID = `Alerts-${TASK_TYPE}`;
 
-const invalidateAPIKey = async (
-  params: InvalidateAPIKeyParams,
+const invalidateAPIKeys = async (
+  params: InvalidateAPIKeysParams,
   securityPluginStart?: SecurityPluginStart
 ): Promise<InvalidateAPIKeyResult> => {
   if (!securityPluginStart) {
@@ -193,30 +193,35 @@ async function invalidateApiKeys(
   encryptedSavedObjectsClient: EncryptedSavedObjectsClient,
   securityPluginStart?: SecurityPluginStart
 ) {
-  // TODO: This could probably send a single request to ES now that the invalidate API supports multiple ids in a single request
   let totalInvalidated = 0;
-  await Promise.all(
+  const apiKeyIds = await Promise.all(
     apiKeysToInvalidate.saved_objects.map(async (apiKeyObj) => {
       const decryptedApiKey = await encryptedSavedObjectsClient.getDecryptedAsInternalUser<InvalidatePendingApiKey>(
         'api_key_pending_invalidation',
         apiKeyObj.id
       );
-      const apiKeyId = decryptedApiKey.attributes.apiKeyId;
-      const response = await invalidateAPIKey({ id: apiKeyId }, securityPluginStart);
-      if (response.apiKeysEnabled === true && response.result.error_count > 0) {
-        logger.error(`Failed to invalidate API Key [id="${apiKeyObj.attributes.apiKeyId}"]`);
-      } else {
-        try {
-          await savedObjectsClient.delete('api_key_pending_invalidation', apiKeyObj.id);
-          totalInvalidated++;
-        } catch (err) {
-          logger.error(
-            `Failed to cleanup api key "${apiKeyObj.attributes.apiKeyId}". Error: ${err.message}`
-          );
-        }
-      }
+      return decryptedApiKey.attributes.apiKeyId;
     })
   );
-  logger.debug(`Total invalidated api keys "${totalInvalidated}"`);
+  if (apiKeyIds.length > 0) {
+    const response = await invalidateAPIKeys({ ids: apiKeyIds }, securityPluginStart);
+    if (response.apiKeysEnabled === true && response.result.error_count > 0) {
+      logger.error(`Failed to invalidate API Keys [ids="${apiKeyIds.join(', ')}"]`);
+    } else {
+      await Promise.all(
+        apiKeysToInvalidate.saved_objects.map(async (apiKeyObj) => {
+          try {
+            await savedObjectsClient.delete('api_key_pending_invalidation', apiKeyObj.id);
+            totalInvalidated++;
+          } catch (err) {
+            logger.error(
+              `Failed to delete invalidated API key "${apiKeyObj.attributes.apiKeyId}". Error: ${err.message}`
+            );
+          }
+        })
+      );
+    }
+  }
+  logger.debug(`Total invalidated API keys "${totalInvalidated}"`);
   return totalInvalidated;
 }
diff --git a/x-pack/plugins/fleet/server/services/agents/unenroll.ts b/x-pack/plugins/fleet/server/services/agents/unenroll.ts
@@ -3,7 +3,6 @@
  * or more contributor license agreements. Licensed under the Elastic License;
  * you may not use this file except in compliance with the Elastic License.
  */
-import { chunk } from 'lodash';
 import { SavedObjectsClientContract } from 'src/core/server';
 import { AgentSOAttributes } from '../../types';
 import { AGENT_SAVED_OBJECT_TYPE } from '../../constants';
@@ -76,10 +75,10 @@ export async function forceUnenrollAgent(soClient: SavedObjectsClientContract, a
 
   await Promise.all([
     agent.access_api_key_id
-      ? APIKeyService.invalidateAPIKey(soClient, agent.access_api_key_id)
+      ? APIKeyService.invalidateAPIKeys(soClient, [agent.access_api_key_id])
       : undefined,
     agent.default_api_key_id
-      ? APIKeyService.invalidateAPIKey(soClient, agent.default_api_key_id)
+      ? APIKeyService.invalidateAPIKeys(soClient, [agent.default_api_key_id])
       : undefined,
   ]);
 
@@ -124,16 +123,8 @@ export async function forceUnenrollAgents(
   });
 
   // Invalidate all API keys
-  // ES doesn't provide a bulk invalidate API, so this could take a long time depending on
-  // number of keys to invalidate. We run these in batches to avoid overloading ES.
   if (apiKeys.length) {
-    const BATCH_SIZE = 500;
-    const batches = chunk(apiKeys, BATCH_SIZE);
-    for (const apiKeysBatch of batches) {
-      await Promise.all(
-        apiKeysBatch.map((apiKey) => APIKeyService.invalidateAPIKey(soClient, apiKey))
-      );
-    }
+    APIKeyService.invalidateAPIKeys(soClient, apiKeys);
   }
 
   // Update the necessary agents

diff --git a/x-pack/plugins/fleet/server/services/api_keys/enrollment_api_key.ts b/x-pack/plugins/fleet/server/services/api_keys/enrollment_api_key.ts
@@ -9,7 +9,7 @@ import Boom from '@hapi/boom';
 import { SavedObjectsClientContract, SavedObject } from 'src/core/server';
 import { EnrollmentAPIKey, EnrollmentAPIKeySOAttributes } from '../../types';
 import { ENROLLMENT_API_KEYS_SAVED_OBJECT_TYPE } from '../../constants';
-import { createAPIKey, invalidateAPIKey } from './security';
+import { createAPIKey, invalidateAPIKeys } from './security';
 import { agentPolicyService } from '../agent_policy';
 import { appContextService } from '../app_context';
 import { normalizeKuery } from '../saved_object';
@@ -66,7 +66,7 @@ export async function getEnrollmentAPIKey(soClient: SavedObjectsClientContract,
 export async function deleteEnrollmentApiKey(soClient: SavedObjectsClientContract, id: string) {
   const enrollmentApiKey = await getEnrollmentAPIKey(soClient, id);
 
-  await invalidateAPIKey(soClient, enrollmentApiKey.api_key_id);
+  await invalidateAPIKeys(soClient, [enrollmentApiKey.api_key_id]);
 
   await soClient.update(ENROLLMENT_API_KEYS_SAVED_OBJECT_TYPE, id, {
     active: false,

diff --git a/x-pack/plugins/fleet/server/services/api_keys/index.ts b/x-pack/plugins/fleet/server/services/api_keys/index.ts
@@ -10,7 +10,7 @@ import { EnrollmentAPIKeySOAttributes, EnrollmentAPIKey } from '../../types';
 import { createAPIKey } from './security';
 import { escapeSearchQueryPhrase } from '../saved_object';
 
-export { invalidateAPIKey } from './security';
+export { invalidateAPIKeys } from './security';
 export * from './enrollment_api_key';
 
 export async function generateOutputApiKey(

diff --git a/x-pack/plugins/fleet/server/services/api_keys/security.ts b/x-pack/plugins/fleet/server/services/api_keys/security.ts
@@ -64,7 +64,7 @@ export async function authenticate(callCluster: CallESAsCurrentUser) {
   }
 }
 
-export async function invalidateAPIKey(soClient: SavedObjectsClientContract, id: string) {
+export async function invalidateAPIKeys(soClient: SavedObjectsClientContract, ids: string[]) {
   const adminUser = await outputService.getAdminUser(soClient);
   if (!adminUser) {
     throw new Error('No admin user configured');
@@ -88,7 +88,7 @@ export async function invalidateAPIKey(soClient: SavedObjectsClientContract, id:
 
   try {
     const res = await security.authc.apiKeys.invalidate(request, {
-      id,
+      ids,
     });
 
     return res;

diff --git a/x-pack/plugins/security/server/authentication/api_keys/api_keys.test.ts b/x-pack/plugins/security/server/authentication/api_keys/api_keys.test.ts
@@ -289,7 +289,7 @@ describe('API Keys', () => {
     it('returns null when security feature is disabled', async () => {
       mockLicense.isEnabled.mockReturnValue(false);
       const result = await apiKeys.invalidate(httpServerMock.createKibanaRequest(), {
-        id: '123',
+        ids: ['123'],
       });
       expect(result).toBeNull();
       expect(
@@ -309,7 +309,7 @@ describe('API Keys', () => {
         })
       );
       const result = await apiKeys.invalidate(httpServerMock.createKibanaRequest(), {
-        id: '123',
+        ids: ['123'],
       });
       expect(result).toEqual({
         invalidated_api_keys: ['api-key-id-1'],
@@ -323,7 +323,7 @@ describe('API Keys', () => {
       });
     });
 
-    it(`Only passes id as a parameter`, async () => {
+    it(`Only passes ids as a parameter`, async () => {
       mockLicense.isEnabled.mockReturnValue(true);
       mockScopedClusterClient.asCurrentUser.security.invalidateApiKey.mockResolvedValueOnce(
         securityMock.createApiResponse({
@@ -335,7 +335,7 @@ describe('API Keys', () => {
         })
       );
       const result = await apiKeys.invalidate(httpServerMock.createKibanaRequest(), {
-        id: '123',
+        ids: ['123'],
         name: 'abc',
       } as any);
       expect(result).toEqual({
@@ -354,7 +354,7 @@ describe('API Keys', () => {
   describe('invalidateAsInternalUser()', () => {
     it('returns null when security feature is disabled', async () => {
       mockLicense.isEnabled.mockReturnValue(false);
-      const result = await apiKeys.invalidateAsInternalUser({ id: '123' });
+      const result = await apiKeys.invalidateAsInternalUser({ ids: ['123'] });
       expect(result).toBeNull();
       expect(mockClusterClient.asInternalUser.security.invalidateApiKey).not.toHaveBeenCalled();
     });
@@ -370,7 +370,7 @@ describe('API Keys', () => {
           },
         })
       );
-      const result = await apiKeys.invalidateAsInternalUser({ id: '123' });
+      const result = await apiKeys.invalidateAsInternalUser({ ids: ['123'] });
       expect(result).toEqual({
         invalidated_api_keys: ['api-key-id-1'],
         previously_invalidated_api_keys: [],
@@ -383,7 +383,7 @@ describe('API Keys', () => {
       });
     });
 
-    it('Only passes id as a parameter', async () => {
+    it('Only passes ids as a parameter', async () => {
       mockLicense.isEnabled.mockReturnValue(true);
       mockClusterClient.asInternalUser.security.invalidateApiKey.mockResolvedValueOnce(
         securityMock.createApiResponse({
@@ -395,7 +395,7 @@ describe('API Keys', () => {
         })
       );
       const result = await apiKeys.invalidateAsInternalUser({
-        id: '123',
+        ids: ['123'],
         name: 'abc',
       } as any);
       expect(result).toEqual({

diff --git a/x-pack/plugins/security/server/authentication/api_keys/api_keys.ts b/x-pack/plugins/security/server/authentication/api_keys/api_keys.ts
@@ -39,10 +39,10 @@ interface GrantAPIKeyParams {
 }
 
 /**
- * Represents the params for invalidating an API key
+ * Represents the params for invalidating multiple API keys
  */
-export interface InvalidateAPIKeyParams {
-  id: string;
+export interface InvalidateAPIKeysParams {
+  ids: string[];
 }
 
 /**
@@ -222,16 +222,16 @@ export class APIKeys {
   }
 
   /**
-   * Tries to invalidate an API key.
+   * Tries to invalidate an API keys.
    * @param request Request instance.
-   * @param params The params to invalidate an API key.
+   * @param params The params to invalidate an API keys.
    */
-  async invalidate(request: KibanaRequest, params: InvalidateAPIKeyParams) {
+  async invalidate(request: KibanaRequest, params: InvalidateAPIKeysParams) {
     if (!this.license.isEnabled()) {
       return null;
     }
 
-    this.logger.debug('Trying to invalidate an API key as current user');
+    this.logger.debug(`Trying to invalidate ${params.ids.length} an API key as current user`);
 
     let result;
     try {
@@ -240,40 +240,48 @@ export class APIKeys {
         await this.clusterClient
           .asScoped(request)
           .asCurrentUser.security.invalidateApiKey<InvalidateAPIKeyResult>({
-            body: { ids: [params.id] },
+            body: { ids: params.ids },
           })
       ).body;
-      this.logger.debug('API key was invalidated successfully as current user');
+      this.logger.debug(
+        `API keys by ids=[${params.ids.join(', ')}] was invalidated successfully as current user`
+      );
     } catch (e) {
-      this.logger.error(`Failed to invalidate API key as current user: ${e.message}`);
+      this.logger.error(
+        `Failed to invalidate API keys by ids=[${params.ids.join(', ')}] as current user: ${
+          e.message
+        }`
+      );
       throw e;
     }
 
     return result;
   }
 
   /**
-   * Tries to invalidate an API key by using the internal user.
-   * @param params The params to invalidate an API key.
+   * Tries to invalidate the API keys by using the internal user.
+   * @param params The params to invalidate the API keys.
    */
-  async invalidateAsInternalUser(params: InvalidateAPIKeyParams) {
+  async invalidateAsInternalUser(params: InvalidateAPIKeysParams) {
     if (!this.license.isEnabled()) {
       return null;
     }
 
-    this.logger.debug('Trying to invalidate an API key');
+    this.logger.debug(`Trying to invalidate ${params.ids.length} API keys`);
 
     let result;
     try {
       // Internal user needs `cluster:admin/xpack/security/api_key/invalidate` privilege to use this API
       result = (
         await this.clusterClient.asInternalUser.security.invalidateApiKey<InvalidateAPIKeyResult>({
-          body: { ids: [params.id] },
+          body: { ids: params.ids },
         })
       ).body;
-      this.logger.debug('API key was invalidated successfully');
+      this.logger.debug(`API keys by ids=[${params.ids.join(', ')}] was invalidated successfully`);
     } catch (e) {
-      this.logger.error(`Failed to invalidate API key: ${e.message}`);
+      this.logger.error(
+        `Failed to invalidate API keys by ids=[${params.ids.join(', ')}]: ${e.message}`
+      );
       throw e;
     }
 

diff --git a/x-pack/plugins/security/server/authentication/api_keys/index.ts b/x-pack/plugins/security/server/authentication/api_keys/index.ts
@@ -9,6 +9,6 @@ export {
   CreateAPIKeyResult,
   InvalidateAPIKeyResult,
   CreateAPIKeyParams,
-  InvalidateAPIKeyParams,
+  InvalidateAPIKeysParams,
   GrantAPIKeyResult,
 } from './api_keys';
diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts
@@ -28,6 +28,6 @@ export type {
   CreateAPIKeyResult,
   InvalidateAPIKeyResult,
   CreateAPIKeyParams,
-  InvalidateAPIKeyParams,
+  InvalidateAPIKeysParams,
   GrantAPIKeyResult,
 } from './api_keys';
diff --git a/x-pack/plugins/security/server/index.ts b/x-pack/plugins/security/server/index.ts
@@ -24,7 +24,7 @@ import {
 // functions or removal of exports should be considered as a breaking change.
 export type {
   CreateAPIKeyResult,
-  InvalidateAPIKeyParams,
+  InvalidateAPIKeysParams,
   InvalidateAPIKeyResult,
   GrantAPIKeyResult,
 } from './authentication';