Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 25 additions & 20 deletions x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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) {
Expand Down Expand Up @@ -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(', ')}"]`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think the intent was to join with double quotes?

Suggested change
logger.error(`Failed to invalidate API Keys [ids="${apiKeyIds.join(', ')}"]`);
logger.error(`Failed to invalidate API Keys [ids="${apiKeyIds.join(", ")}"]`);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TS formatter trying to replace back to a single quotes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh I see, I was thinking the quotes were part of the logs, oops. All good.

} 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;
}
15 changes: 3 additions & 12 deletions x-pack/plugins/fleet/server/services/agents/unenroll.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
]);

Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/fleet/server/services/api_keys/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/fleet/server/services/api_keys/security.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -88,7 +88,7 @@ export async function invalidateAPIKey(soClient: SavedObjectsClientContract, id:

try {
const res = await security.authc.apiKeys.invalidate(request, {
id,
ids,
});

return res;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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'],
Expand All @@ -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({
Expand All @@ -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({
Expand All @@ -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();
});
Expand All @@ -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: [],
Expand All @@ -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({
Expand All @@ -395,7 +395,7 @@ describe('API Keys', () => {
})
);
const result = await apiKeys.invalidateAsInternalUser({
id: '123',
ids: ['123'],
name: 'abc',
} as any);
expect(result).toEqual({
Expand Down
42 changes: 25 additions & 17 deletions x-pack/plugins/security/server/authentication/api_keys/api_keys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
}

/**
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@ export {
CreateAPIKeyResult,
InvalidateAPIKeyResult,
CreateAPIKeyParams,
InvalidateAPIKeyParams,
InvalidateAPIKeysParams,
GrantAPIKeyResult,
} from './api_keys';
2 changes: 1 addition & 1 deletion x-pack/plugins/security/server/authentication/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ export type {
CreateAPIKeyResult,
InvalidateAPIKeyResult,
CreateAPIKeyParams,
InvalidateAPIKeyParams,
InvalidateAPIKeysParams,
GrantAPIKeyResult,
} from './api_keys';
2 changes: 1 addition & 1 deletion x-pack/plugins/security/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down