Skip to content

Commit

Permalink
Retain APIKey when disabling/enabling a rule (#131581)
Browse files Browse the repository at this point in the history 
* Retain APIKey when disabling/enabling a rule
  • Loading branch information
@ersin-erdal
ersin-erdal authored May 18, 2022
1 parent d828e9b commit de29010
Show file tree
Hide file tree
Showing 24 changed files with 2,015 additions and 1,123 deletions.
3 changes: 1 addition & 2 deletions docs/user/alerting/alerting-setup.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,9 @@ Rules and connectors are isolated to the {kib} space in which they were created.
Rules are authorized using an <<api-keys,API key>> associated with the last user to edit the rule. This API key captures a snapshot of the user's privileges at the time of edit and is subsequently used to run all background tasks associated with the rule, including condition checks like {es} queries and triggered actions. The following rule actions will re-generate the API key:

* Creating a rule
* Enabling a disabled rule
* Updating a rule

[IMPORTANT]
==============================================
If a rule requires certain privileges, such as index privileges, to run, and a user without those privileges updates, disables, or re-enables the rule, the rule will no longer function. Conversely, if a user with greater or administrator privileges modifies the rule, it will begin running with increased privileges.
If a rule requires certain privileges, such as index privileges, to run, and a user without those privileges updates the rule, the rule will no longer function. Conversely, if a user with greater or administrator privileges modifies the rule, it will begin running with increased privileges.
==============================================
78 changes: 28 additions & 50 deletions x-pack/plugins/alerting/server/rules_client/rules_client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1828,7 +1828,7 @@ export class RulesClient {
}

private async enableWithOCC({ id }: { id: string }) {
let apiKeyToInvalidate: string | null = null;
let existingApiKey: string | null = null;
let attributes: RawRule;
let version: string | undefined;

Expand All @@ -1837,14 +1837,11 @@ export class RulesClient {
await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser<RawRule>('alert', id, {
namespace: this.namespace,
});
apiKeyToInvalidate = decryptedAlert.attributes.apiKey;
existingApiKey = decryptedAlert.attributes.apiKey;
attributes = decryptedAlert.attributes;
version = decryptedAlert.version;
} catch (e) {
// We'll skip invalidating the API key since we failed to load the decrypted saved object
this.logger.error(
`enable(): Failed to load API key to invalidate on alert ${id}: ${e.message}`
);
this.logger.error(`enable(): Failed to load API key of alert ${id}: ${e.message}`);
// Still attempt to load the attributes and version using SOC
const alert = await this.unsecuredSavedObjectsClient.get<RawRule>('alert', id);
attributes = alert.attributes;
Expand Down Expand Up @@ -1886,19 +1883,10 @@ export class RulesClient {
if (attributes.enabled === false) {
const username = await this.getUserName();

let createdAPIKey = null;
try {
createdAPIKey = await this.createAPIKey(
this.generateAPIKeyName(attributes.alertTypeId, attributes.name)
);
} catch (error) {
throw Boom.badRequest(`Error enabling rule: could not create API key - ${error.message}`);
}

const updateAttributes = this.updateMeta({
...attributes,
...(!existingApiKey && (await this.createNewAPIKeySet({ attributes, username }))),
enabled: true,
...this.apiKeyAsAlertAttributes(createdAPIKey, username),
updatedBy: username,
updatedAt: new Date().toISOString(),
executionStatus: {
Expand All @@ -1909,15 +1897,10 @@ export class RulesClient {
warning: null,
},
});

try {
await this.unsecuredSavedObjectsClient.update('alert', id, updateAttributes, { version });
} catch (e) {
// Avoid unused API key
await bulkMarkApiKeysForInvalidation(
{ apiKeys: updateAttributes.apiKey ? [updateAttributes.apiKey] : [] },
this.logger,
this.unsecuredSavedObjectsClient
);
throw e;
}
const scheduledTask = await this.scheduleRule({
Expand All @@ -1930,16 +1913,28 @@ export class RulesClient {
await this.unsecuredSavedObjectsClient.update('alert', id, {
scheduledTaskId: scheduledTask.id,
});
if (apiKeyToInvalidate) {
await bulkMarkApiKeysForInvalidation(
{ apiKeys: [apiKeyToInvalidate] },
this.logger,
this.unsecuredSavedObjectsClient
);
}
}
}

private async createNewAPIKeySet({
attributes,
username,
}: {
attributes: RawRule;
username: string | null;
}): Promise<Pick<RawRule, 'apiKey' | 'apiKeyOwner'>> {
let createdAPIKey = null;
try {
createdAPIKey = await this.createAPIKey(
this.generateAPIKeyName(attributes.alertTypeId, attributes.name)
);
} catch (error) {
throw Boom.badRequest(`Error creating API key for rule: ${error.message}`);
}

return this.apiKeyAsAlertAttributes(createdAPIKey, username);
}

public async disable({ id }: { id: string }): Promise<void> {
return await retryIfConflicts(
this.logger,
Expand All @@ -1949,7 +1944,6 @@ export class RulesClient {
}

private async disableWithOCC({ id }: { id: string }) {
let apiKeyToInvalidate: string | null = null;
let attributes: RawRule;
let version: string | undefined;

Expand All @@ -1958,14 +1952,10 @@ export class RulesClient {
await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser<RawRule>('alert', id, {
namespace: this.namespace,
});
apiKeyToInvalidate = decryptedAlert.attributes.apiKey;
attributes = decryptedAlert.attributes;
version = decryptedAlert.version;
} catch (e) {
// We'll skip invalidating the API key since we failed to load the decrypted saved object
this.logger.error(
`disable(): Failed to load API key to invalidate on alert ${id}: ${e.message}`
);
this.logger.error(`disable(): Failed to load API key of alert ${id}: ${e.message}`);
// Still attempt to load the attributes and version using SOC
const alert = await this.unsecuredSavedObjectsClient.get<RawRule>('alert', id);
attributes = alert.attributes;
Expand Down Expand Up @@ -2058,26 +2048,14 @@ export class RulesClient {
...attributes,
enabled: false,
scheduledTaskId: null,
apiKey: null,
apiKeyOwner: null,
updatedBy: await this.getUserName(),
updatedAt: new Date().toISOString(),
}),
{ version }
);

await Promise.all([
attributes.scheduledTaskId
? this.taskManager.removeIfExists(attributes.scheduledTaskId)
: null,
apiKeyToInvalidate
? await bulkMarkApiKeysForInvalidation(
{ apiKeys: [apiKeyToInvalidate] },
this.logger,
this.unsecuredSavedObjectsClient
)
: null,
]);
if (attributes.scheduledTaskId) {
await this.taskManager.removeIfExists(attributes.scheduledTaskId);
}
}
}

Expand Down
67 changes: 11 additions & 56 deletions x-pack/plugins/alerting/server/rules_client/tests/disable.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { auditLoggerMock } from '@kbn/security-plugin/server/audit/mocks';
import { getBeforeSetup, setGlobalDate } from './lib';
import { eventLoggerMock } from '@kbn/event-log-plugin/server/event_logger.mock';
import { TaskStatus } from '@kbn/task-manager-plugin/server';
import { bulkMarkApiKeysForInvalidation } from '../../invalidate_pending_api_keys/bulk_mark_api_keys_for_invalidation';

jest.mock('../../invalidate_pending_api_keys/bulk_mark_api_keys_for_invalidation', () => ({
bulkMarkApiKeysForInvalidation: jest.fn(),
Expand Down Expand Up @@ -111,6 +110,7 @@ describe('disable()', () => {
attributes: {
...existingAlert.attributes,
apiKey: Buffer.from('123:abc').toString('base64'),
apiKeyOwner: 'elastic',
},
version: '123',
references: [],
Expand Down Expand Up @@ -206,11 +206,11 @@ describe('disable()', () => {
alertTypeId: 'myType',
enabled: false,
meta: {
versionApiKeyLastmodified: kibanaVersion,
versionApiKeyLastmodified: 'v7.10.0',
},
scheduledTaskId: null,
apiKey: null,
apiKeyOwner: null,
apiKey: 'MTIzOmFiYw==',
apiKeyOwner: 'elastic',
updatedAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
actions: [
Expand All @@ -230,12 +230,6 @@ describe('disable()', () => {
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
{ apiKeys: ['MTIzOmFiYw=='] },
expect.any(Object),
expect.any(Object)
);
});

test('disables the rule with calling event log to "recover" the alert instances from the task state', async () => {
Expand Down Expand Up @@ -282,11 +276,11 @@ describe('disable()', () => {
alertTypeId: 'myType',
enabled: false,
meta: {
versionApiKeyLastmodified: kibanaVersion,
versionApiKeyLastmodified: 'v7.10.0',
},
scheduledTaskId: null,
apiKey: null,
apiKeyOwner: null,
apiKey: 'MTIzOmFiYw==',
apiKeyOwner: 'elastic',
updatedAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
actions: [
Expand All @@ -306,12 +300,6 @@ describe('disable()', () => {
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
{ apiKeys: ['MTIzOmFiYw=='] },
expect.any(Object),
expect.any(Object)
);

expect(eventLogger.logEvent).toHaveBeenCalledTimes(1);
expect(eventLogger.logEvent.mock.calls[0][0]).toStrictEqual({
Expand Down Expand Up @@ -369,11 +357,11 @@ describe('disable()', () => {
alertTypeId: 'myType',
enabled: false,
meta: {
versionApiKeyLastmodified: kibanaVersion,
versionApiKeyLastmodified: 'v7.10.0',
},
scheduledTaskId: null,
apiKey: null,
apiKeyOwner: null,
apiKey: 'MTIzOmFiYw==',
apiKeyOwner: 'elastic',
updatedAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
actions: [
Expand All @@ -393,12 +381,6 @@ describe('disable()', () => {
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
{ apiKeys: ['MTIzOmFiYw=='] },
expect.any(Object),
expect.any(Object)
);

expect(eventLogger.logEvent).toHaveBeenCalledTimes(0);
expect(rulesClientParams.logger.warn).toHaveBeenCalledWith(
Expand All @@ -408,7 +390,6 @@ describe('disable()', () => {

test('falls back when getDecryptedAsInternalUser throws an error', async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail'));

await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledWith('alert', '1');
expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
Expand All @@ -422,12 +403,7 @@ describe('disable()', () => {
schedule: { interval: '10s' },
alertTypeId: 'myType',
enabled: false,
meta: {
versionApiKeyLastmodified: kibanaVersion,
},
scheduledTaskId: null,
apiKey: null,
apiKeyOwner: null,
updatedAt: '2019-02-12T21:01:22.479Z',
updatedBy: 'elastic',
actions: [
Expand All @@ -447,7 +423,6 @@ describe('disable()', () => {
}
);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
});

test(`doesn't disable already disabled alerts`, async () => {
Expand All @@ -463,14 +438,6 @@ describe('disable()', () => {
await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled();
expect(taskManager.removeIfExists).not.toHaveBeenCalled();
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
});

test(`doesn't invalidate when no API key is used`, async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce(existingAlert);

await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
});

test('swallows error when failing to load decrypted saved object', async () => {
Expand All @@ -479,9 +446,8 @@ describe('disable()', () => {
await rulesClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled();
expect(taskManager.removeIfExists).toHaveBeenCalled();
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
expect(rulesClientParams.logger.error).toHaveBeenCalledWith(
'disable(): Failed to load API key to invalidate on alert 1: Fail'
'disable(): Failed to load API key of alert 1: Fail'
);
});

Expand All @@ -493,17 +459,6 @@ describe('disable()', () => {
);
});

test('swallows error when invalidate API key throws', async () => {
unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Fail'));
await rulesClient.disable({ id: '1' });
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledTimes(1);
expect(bulkMarkApiKeysForInvalidation).toHaveBeenCalledWith(
{ apiKeys: ['MTIzOmFiYw=='] },
expect.any(Object),
expect.any(Object)
);
});

test('throws when failing to remove task from task manager', async () => {
taskManager.removeIfExists.mockRejectedValueOnce(new Error('Failed to remove task'));

Expand Down
Loading

0 comments on commit de29010

Please sign in to comment.