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] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated #75563

Merged
merged 26 commits into from
Sep 16, 2020
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
917c17d
Revert "removed ESO migration from alerting"
gmmorris Aug 19, 2020
e2df4b5
add `meta` field to Alert and mark pre 7.10 alerts as legacy
gmmorris Aug 20, 2020
5a7ee01
exempt legacy alerts from rbac
gmmorris Sep 3, 2020
c564990
Merge branch 'master' into alerting-exempt-rbac
gmmorris Sep 3, 2020
6f7f628
test legacy for superuser and none actions user
gmmorris Sep 4, 2020
b226e32
test legacy for space_1_all and restricted users
gmmorris Sep 4, 2020
0714965
added test for global read user
gmmorris Sep 4, 2020
6c99cde
Merge branch 'master' into alerting-exempt-rbac
gmmorris Sep 4, 2020
f661e32
migrate as legacy
gmmorris Sep 4, 2020
a184efa
simplified migration as we now migrate all alerts pre 7.10
gmmorris Sep 4, 2020
5e58006
cleaned up migration code
gmmorris Sep 4, 2020
b0a5c87
fixed tests
gmmorris Sep 7, 2020
f039444
Merge branch 'master' into alerting-exempt-rbac
gmmorris Sep 7, 2020
a58eb4e
added docs and cleaned up code
gmmorris Sep 7, 2020
c8b8815
corrected name of variable
gmmorris Sep 7, 2020
2330880
use constant where possible
gmmorris Sep 7, 2020
5afd31d
added docs
gmmorris Sep 7, 2020
1ef7d8b
Merge branch 'master' into alerting-exempt-rbac
gmmorris Sep 9, 2020
2a9694f
update meta field only when api key is updated
gmmorris Sep 9, 2020
2940bb2
Merge branch 'master' into alerting-exempt-rbac
gmmorris Sep 10, 2020
e0a6050
Merge branch 'master' into alerting-exempt-rbac
elasticmachine Sep 14, 2020
a0fa54e
Merge branch 'master' into alerting-exempt-rbac
gmmorris Sep 14, 2020
332d14a
fixed merge issue
gmmorris Sep 14, 2020
696a7cb
Merge branch 'master' into alerting-exempt-rbac
gmmorris Sep 15, 2020
b32e62e
migrate siem consumer
gmmorris Sep 15, 2020
b486243
typo
gmmorris Sep 16, 2020
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
16 changes: 12 additions & 4 deletions x-pack/plugins/actions/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -280,12 +280,14 @@ The following table describes the properties of the `options` object.
| params | The `params` value to give the action type executor. | object |
| spaceId | The space id the action is within. | string |
| apiKey | The Elasticsearch API key to use for context. (Note: only required and used when security is enabled). | string |
| source | The source of the execution, either an HTTP request or a reference to a Saved Object. | object, optional |

## Example

This example makes action `3c5b2bd4-5424-4e4b-8cf5-c0a58c762cc5` send an email. The action plugin will load the saved object and find what action type to call with `params`.

```typescript
const request: KibanaRequest = { ... };
const actionsClient = await server.plugins.actions.getActionsClientWithRequest(request);
await actionsClient.enqueueExecution({
id: '3c5b2bd4-5424-4e4b-8cf5-c0a58c762cc5',
Expand All @@ -296,6 +298,7 @@ await actionsClient.enqueueExecution({
subject: 'My email subject',
body: 'My email body',
},
source: asHttpRequestExecutionSource(request),
});
```

Expand All @@ -305,10 +308,11 @@ This api runs the action and asynchronously returns the result of running the ac

The following table describes the properties of the `options` object.

| Property | Description | Type |
| -------- | ---------------------------------------------------- | ------ |
| id | The id of the action you want to execute. | string |
| params | The `params` value to give the action type executor. | object |
| Property | Description | Type |
| -------- | ------------------------------------------------------------------------------------ | ------ |
| id | The id of the action you want to execute. | string |
| params | The `params` value to give the action type executor. | object |
| source | The source of the execution, either an HTTP request or a reference to a Saved Object.| object, optional |

## Example

Expand All @@ -324,6 +328,10 @@ const result = await actionsClient.execute({
subject: 'My email subject',
body: 'My email body',
},
source: asSavedObjectExecutionSource({
id: '573891ae-8c48-49cb-a197-0cd5ec34a88b',
Copy link
Member

Choose a reason for hiding this comment

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

The spaceId is not in here - presumably not needed today since alerts are space-specific and we will have the space via some other context. I don't think we need to pass it along here, but not completely sure. I think once we start using this value in the event log, we'll definitely need the spaceId, but again I think we can get that from some other context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a fair point... the reference object doesn't support spaceId at the moment so we wouldn't be able to use it even if we add it in.
Once SOs can reference SOs in other spaces, that'll make sense here too.

type: 'alert'
}),
});
```

Expand Down
13 changes: 10 additions & 3 deletions x-pack/plugins/actions/server/actions_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
} from './create_execute_function';
import { ActionsAuthorization } from './authorization/actions_authorization';
import { ActionType } from '../common';
import { shouldLegacyRbacApplyBySource } from './authorization/should_legacy_rbac_apply_by_source';

// We are assuming there won't be many actions. This is why we will load
// all the actions in advance and assume the total count to not go over 10000.
Expand Down Expand Up @@ -298,13 +299,19 @@ export class ActionsClient {
public async execute({
actionId,
params,
source,
Copy link
Member

Choose a reason for hiding this comment

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

nice! Looking forward to plumbing this through to the event log, we'll finally be able to track and action execution to an alert, which we can't do today.

}: Omit<ExecuteOptions, 'request'>): Promise<ActionTypeExecutorResult<unknown>> {
await this.authorization.ensureAuthorized('execute');
return this.actionExecutor.execute({ actionId, params, request: this.request });
if (!(await shouldLegacyRbacApplyBySource(this.unsecuredSavedObjectsClient, source))) {
await this.authorization.ensureAuthorized('execute');
}
Comment on lines +304 to +306
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to keep this kind of check limited to the Authorization class itself, but as this authorization will have been initialized prior to us knowing what the source is, that isn't really possible without adding the source as an argument to the ensureAuthorized method, which actually felt like a worse solution as most operations won't have that.
Considering we'll be able to remove this all together in 8.0.0 I think it's best to keep this check here as is.

return this.actionExecutor.execute({ actionId, params, source, request: this.request });
}

public async enqueueExecution(options: EnqueueExecutionOptions): Promise<void> {
await this.authorization.ensureAuthorized('execute');
const { source } = options;
if (!(await shouldLegacyRbacApplyBySource(this.unsecuredSavedObjectsClient, source))) {
await this.authorization.ensureAuthorized('execute');
}
return this.executionEnqueuer(this.unsecuredSavedObjectsClient, options);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { ActionsAuthorization } from './actions_authorization';
import { actionsAuthorizationAuditLoggerMock } from './audit_logger.mock';
import { ActionsAuthorizationAuditLogger, AuthorizationResult } from './audit_logger';
import { ACTION_SAVED_OBJECT_TYPE, ACTION_TASK_PARAMS_SAVED_OBJECT_TYPE } from '../saved_objects';
import { AuthenticatedUser } from '../../../security/server';

const request = {} as KibanaRequest;

Expand All @@ -19,12 +20,13 @@ const mockAuthorizationAction = (type: string, operation: string) => `${type}/${
function mockSecurity() {
const security = securityMock.createSetup();
const authorization = security.authz;
const authentication = security.authc;
// typescript is having trouble inferring jest's automocking
(authorization.actions.savedObject.get as jest.MockedFunction<
typeof authorization.actions.savedObject.get
>).mockImplementation(mockAuthorizationAction);
authorization.mode.useRbacForRequest.mockReturnValue(true);
return { authorization };
return { authorization, authentication };
}

beforeEach(() => {
Expand Down Expand Up @@ -192,4 +194,38 @@ describe('ensureAuthorized', () => {
]
`);
});

test('exempts users from requiring privileges to execute actions when shouldUseLegacyRbac is true', async () => {
const { authorization, authentication } = mockSecurity();
const checkPrivileges: jest.MockedFunction<ReturnType<
typeof authorization.checkPrivilegesDynamicallyWithRequest
>> = jest.fn();
authorization.checkPrivilegesDynamicallyWithRequest.mockReturnValue(checkPrivileges);
const actionsAuthorization = new ActionsAuthorization({
request,
authorization,
authentication,
auditLogger,
shouldUseLegacyRbac: true,
});

authentication.getCurrentUser.mockReturnValueOnce(({
username: 'some-user',
} as unknown) as AuthenticatedUser);

await actionsAuthorization.ensureAuthorized('execute', 'myType');

expect(authorization.actions.savedObject.get).not.toHaveBeenCalled();
expect(checkPrivileges).not.toHaveBeenCalled();

expect(auditLogger.actionsAuthorizationSuccess).toHaveBeenCalledTimes(1);
expect(auditLogger.actionsAuthorizationFailure).not.toHaveBeenCalled();
expect(auditLogger.actionsAuthorizationSuccess.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"some-user",
"execute",
"myType",
]
`);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@ export interface ConstructorOptions {
request: KibanaRequest;
auditLogger: ActionsAuthorizationAuditLogger;
authorization?: SecurityPluginSetup['authz'];
authentication?: SecurityPluginSetup['authc'];
// In order to support legacy Alerts which predate the introduciton of the
gmmorris marked this conversation as resolved.
Show resolved Hide resolved
// Actions feature in Kibana we need a way of "dialing down" the level of
// authorization for certain opearations.
// Specifically, we want to allow these old alerts and their scheduled
// actions to continue to execute - which requires that we exempt auth on
// `get` for Connectors and `execute` for Action execution when used by
// these legacy alerts
shouldUseLegacyRbac?: boolean;
}

const operationAlias: Record<
Expand All @@ -27,33 +36,58 @@ const operationAlias: Record<
list: (authorization) => authorization.actions.savedObject.get(ACTION_SAVED_OBJECT_TYPE, 'find'),
};

const LEGACY_RBAC_EXEMPT_OPERATIONS = new Set(['get', 'execute']);

export class ActionsAuthorization {
private readonly request: KibanaRequest;
private readonly authorization?: SecurityPluginSetup['authz'];
private readonly authentication?: SecurityPluginSetup['authc'];
private readonly auditLogger: ActionsAuthorizationAuditLogger;
private readonly shouldUseLegacyRbac: boolean;

constructor({ request, authorization, auditLogger }: ConstructorOptions) {
constructor({
request,
authorization,
authentication,
auditLogger,
shouldUseLegacyRbac = false,
}: ConstructorOptions) {
this.request = request;
this.authorization = authorization;
this.authentication = authentication;
this.auditLogger = auditLogger;
this.shouldUseLegacyRbac = shouldUseLegacyRbac;
}

public async ensureAuthorized(operation: string, actionTypeId?: string) {
const { authorization } = this;
if (authorization?.mode?.useRbacForRequest(this.request)) {
const checkPrivileges = authorization.checkPrivilegesDynamicallyWithRequest(this.request);
const { hasAllRequested, username } = await checkPrivileges({
kibana: operationAlias[operation]
? operationAlias[operation](authorization)
: authorization.actions.savedObject.get(ACTION_SAVED_OBJECT_TYPE, operation),
});
if (hasAllRequested) {
this.auditLogger.actionsAuthorizationSuccess(username, operation, actionTypeId);
} else {
throw Boom.forbidden(
this.auditLogger.actionsAuthorizationFailure(username, operation, actionTypeId)
if (this.isOperationExemptDueToLegacyRbac(operation)) {
this.auditLogger.actionsAuthorizationSuccess(
this.authentication?.getCurrentUser(this.request)?.username ?? '',
operation,
actionTypeId
);
} else {
const checkPrivileges = authorization.checkPrivilegesDynamicallyWithRequest(this.request);
const { hasAllRequested, username } = await checkPrivileges({
kibana: operationAlias[operation]
? operationAlias[operation](authorization)
: authorization.actions.savedObject.get(ACTION_SAVED_OBJECT_TYPE, operation),
});
if (hasAllRequested) {
this.auditLogger.actionsAuthorizationSuccess(username, operation, actionTypeId);
this.auditLogger.actionsAuthorizationSuccess(username, operation, actionTypeId);
} else {
throw Boom.forbidden(
this.auditLogger.actionsAuthorizationFailure(username, operation, actionTypeId)
);
}
}
}
}

private isOperationExemptDueToLegacyRbac(operation: string) {
return this.shouldUseLegacyRbac && LEGACY_RBAC_EXEMPT_OPERATIONS.has(operation);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { shouldLegacyRbacApplyBySource } from './should_legacy_rbac_apply_by_source';
import { savedObjectsClientMock } from '../../../../../src/core/server/mocks';
import uuid from 'uuid';
import { asSavedObjectExecutionSource } from '../lib';

const unsecuredSavedObjectsClient = savedObjectsClientMock.create();

describe(`#shouldLegacyRbacApplyBySource`, () => {
test('should return false if no source is provided', async () => {
expect(await shouldLegacyRbacApplyBySource(unsecuredSavedObjectsClient)).toEqual(false);
});

test('should return false if source is not an alert', async () => {
expect(
await shouldLegacyRbacApplyBySource(
unsecuredSavedObjectsClient,
asSavedObjectExecutionSource({
type: 'action',
id: uuid.v4(),
})
)
).toEqual(false);
});

test('should return false if source alert is not marked as legacy', async () => {
const id = uuid.v4();
unsecuredSavedObjectsClient.get.mockResolvedValue(mockAlert({ id }));
expect(
await shouldLegacyRbacApplyBySource(
unsecuredSavedObjectsClient,
asSavedObjectExecutionSource({
type: 'alert',
id,
})
)
).toEqual(false);
});

test('should return true if source alert is marked as legacy', async () => {
const id = uuid.v4();
unsecuredSavedObjectsClient.get.mockResolvedValue(
mockAlert({ id, attributes: { meta: { versionApiKeyLastmodified: 'pre-7.10.0' } } })
);
expect(
await shouldLegacyRbacApplyBySource(
unsecuredSavedObjectsClient,
asSavedObjectExecutionSource({
type: 'alert',
id,
})
)
).toEqual(true);
});

test('should return false if source alert is marked as modern', async () => {
const id = uuid.v4();
unsecuredSavedObjectsClient.get.mockResolvedValue(
mockAlert({ id, attributes: { meta: { versionApiKeyLastmodified: '7.10.0' } } })
);
expect(
await shouldLegacyRbacApplyBySource(
unsecuredSavedObjectsClient,
asSavedObjectExecutionSource({
type: 'alert',
id,
})
)
).toEqual(false);
});

test('should return false if source alert is marked with a last modified version', async () => {
gmmorris marked this conversation as resolved.
Show resolved Hide resolved
const id = uuid.v4();
unsecuredSavedObjectsClient.get.mockResolvedValue(mockAlert({ id, attributes: { meta: {} } }));
expect(
await shouldLegacyRbacApplyBySource(
unsecuredSavedObjectsClient,
asSavedObjectExecutionSource({
type: 'alert',
id,
})
)
).toEqual(false);
});
});

const mockAlert = (overrides: Record<string, unknown> = {}) => ({
id: '1',
type: 'alert',
attributes: {
consumer: 'myApp',
schedule: { interval: '10s' },
alertTypeId: 'myType',
enabled: false,
actions: [
{
group: 'default',
id: '1',
actionTypeId: '1',
actionRef: '1',
params: {
foo: true,
},
},
],
},
version: '123',
references: [],
...overrides,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { SavedObjectsClientContract } from 'src/core/server';
import { ActionExecutionSource, isSavedObjectExecutionSource } from '../lib';
import { ALERT_SAVED_OBJECT_TYPE } from '../saved_objects';

const LEGACY_VERSION = 'pre-7.10.0';

export async function shouldLegacyRbacApplyBySource(
unsecuredSavedObjectsClient: SavedObjectsClientContract,
executionSource?: ActionExecutionSource<unknown>
): Promise<boolean> {
return isSavedObjectExecutionSource(executionSource) &&
executionSource?.source?.type === ALERT_SAVED_OBJECT_TYPE
? (
await unsecuredSavedObjectsClient.get<{
meta?: {
versionApiKeyLastmodified?: string;
};
}>(ALERT_SAVED_OBJECT_TYPE, executionSource.source.id)
).attributes.meta?.versionApiKeyLastmodified === LEGACY_VERSION
: false;
}
Loading