Skip to content

Commit

Permalink
[ResponseOps][Alerts] Fix authorization issues with discover as con…
Browse files Browse the repository at this point in the history
…sumers (elastic#192321)

## Summary

Alerts use its own RBAC model. The RBAC relies on a property called
`consumer`. The consumer is tight coupled with the feature ID. It
denotes the user's access to the rule and the alerts. For example, a
user with access to the "Logs" feature has access only to alerts and
rules with the `consumer` set as `logs`. Users can create an ES Query
rule from Discover. When the feature was
[implemented](elastic#124534) (v8.3.0)
the consumer was set to `discover`. Then it
[changed](elastic#166032) (v8.11.0) to
`stackAlerts` (visible only on the stack management page) and then
[to](elastic#171364) (v8.12.0) `alerts`
so it can be visible in Observability. Users who created rules that
generated alerts with the `discover` consumer cannot see the alerts
generated by the rule when they upgrade Kibana to 8.11+ even as
superusers. This PR fixes the issues around the `discover` consumer.

I added the following alert document to the `data.json.gz` to test for
alerts with `discover` consumer.

```
{
  "type": "doc",
  "value": {
    "id": "1b75bfe9-d2f5-47e9-bac6-b082dd9c9e97",
    "index": ".internal.alerts-stack.alerts-default-000001",
    "source": {
      "@timestamp": "2021-10-19T14:00:38.749Z",
      "event.action": "active",
      "event.kind": "signal",
      "kibana.alert.duration.us": 1370302000,
      "kibana.alert.evaluation.threshold": -1,
      "kibana.alert.evaluation.value": 80,
      "kibana.alert.instance.id": "query matched",
      "kibana.alert.reason": "Document count is 80 in the last 100d in .kibana_alerting_cases index. Alert when greater than -1.",
      "kibana.alert.rule.category": "Elasticsearch query",
      "kibana.alert.rule.consumer": "discover",
      "kibana.alert.rule.name": "EsQuery discover",
      "kibana.alert.rule.producer": "stackAlerts",
      "kibana.alert.rule.rule_type_id": ".es-query",
      "kibana.alert.rule.uuid": "25c14920-faa7-4a9a-830c-ce32c8211237",
      "kibana.alert.start": "2021-10-19T15:00:41.555Z",
      "kibana.alert.status": "active",
      "kibana.alert.time_range": {
        "gte": "2021-10-19T15:00:41.555Z"
      },
      "kibana.alert.uuid": "23237979-75bf-4b68-a210-ce5056b93356",
      "kibana.alert.workflow_status": "open",
      "kibana.space_ids": [
        "default"
      ],
      "kibana.version": "8.0.0",
      "tags": []
    }
  }
}
```

## Testing

1. Create a rule with the consumer as `discover`. See
elastic#184595 for instructions.
2. Go to the rule details page.
3. Verify that you do not get any error toaster and you can see the
alerts.

Fixes: elastic#184595

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

## Release notes
Fix an issue with rules not being accessible created from Discover
before 8.11.0.

---------

Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 396931f)
  • Loading branch information
cnasikas committed Sep 30, 2024
1 parent fb1c9e3 commit c7d3d8c
Show file tree
Hide file tree
Showing 7 changed files with 180 additions and 47 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import { fromKueryExpression } from '@kbn/es-query';
import { KueryNode, fromKueryExpression, toKqlExpression } from '@kbn/es-query';
import { KibanaRequest } from '@kbn/core/server';
import { ruleTypeRegistryMock } from '../rule_type_registry.mock';
import { securityMock } from '@kbn/security-plugin/server/mocks';
Expand Down Expand Up @@ -910,20 +910,19 @@ describe('AlertingAuthorization', () => {
getSpaceId,
});
ruleTypeRegistry.list.mockReturnValue(setOfAlertTypes);
expect(
(
await alertAuthorization.getFindAuthorizationFilter(AlertingAuthorizationEntity.Rule, {
type: AlertingAuthorizationFilterType.KQL,
fieldNames: {
ruleTypeId: 'path.to.rule_type_id',
consumer: 'consumer-field',
},
})
).filter
).toEqual(
fromKueryExpression(
`((path.to.rule_type_id:myAppAlertType and consumer-field:(alerts or myApp or myOtherApp or myAppWithSubFeature)) or (path.to.rule_type_id:mySecondAppAlertType and consumer-field:(alerts or myApp or myOtherApp or myAppWithSubFeature)) or (path.to.rule_type_id:myOtherAppAlertType and consumer-field:(alerts or myApp or myOtherApp or myAppWithSubFeature)))`
)

const filter = (
await alertAuthorization.getFindAuthorizationFilter(AlertingAuthorizationEntity.Rule, {
type: AlertingAuthorizationFilterType.KQL,
fieldNames: {
ruleTypeId: 'path.to.rule_type_id',
consumer: 'consumer-field',
},
})
).filter;

expect(toKqlExpression(filter as KueryNode)).toMatchInlineSnapshot(
`"((path.to.rule_type_id: myAppAlertType AND (consumer-field: alerts OR consumer-field: discover OR consumer-field: myApp OR consumer-field: myOtherApp OR consumer-field: myAppWithSubFeature)) OR (path.to.rule_type_id: mySecondAppAlertType AND (consumer-field: alerts OR consumer-field: discover OR consumer-field: myApp OR consumer-field: myOtherApp OR consumer-field: myAppWithSubFeature)) OR (path.to.rule_type_id: myOtherAppAlertType AND (consumer-field: alerts OR consumer-field: discover OR consumer-field: myApp OR consumer-field: myOtherApp OR consumer-field: myAppWithSubFeature)))"`
);
});
test('throws if user has no privileges to any rule type', async () => {
Expand Down Expand Up @@ -1274,6 +1273,10 @@ describe('AlertingAuthorization', () => {
"all": true,
"read": true,
},
"discover": Object {
"all": true,
"read": true,
},
"myApp": Object {
"all": true,
"read": true,
Expand Down Expand Up @@ -1311,6 +1314,10 @@ describe('AlertingAuthorization', () => {
"all": true,
"read": true,
},
"discover": Object {
"all": true,
"read": true,
},
"myApp": Object {
"all": true,
"read": true,
Expand Down Expand Up @@ -2251,20 +2258,18 @@ describe('AlertingAuthorization', () => {
});
});
test('creates a filter based on the privileged types', async () => {
expect(
(
await alertAuthorization.getFindAuthorizationFilter(AlertingAuthorizationEntity.Rule, {
type: AlertingAuthorizationFilterType.KQL,
fieldNames: {
ruleTypeId: 'path.to.rule_type_id',
consumer: 'consumer-field',
},
})
).filter
).toEqual(
fromKueryExpression(
`path.to.rule_type_id:.esQuery and consumer-field:(alerts or stackAlerts or discover)`
)
const filter = (
await alertAuthorization.getFindAuthorizationFilter(AlertingAuthorizationEntity.Rule, {
type: AlertingAuthorizationFilterType.KQL,
fieldNames: {
ruleTypeId: 'path.to.rule_type_id',
consumer: 'consumer-field',
},
})
).filter;

expect(toKqlExpression(filter as KueryNode)).toMatchInlineSnapshot(
`"(path.to.rule_type_id: .esQuery AND (consumer-field: alerts OR consumer-field: discover OR consumer-field: stackAlerts))"`
);
});
});
Expand Down Expand Up @@ -2557,21 +2562,20 @@ describe('AlertingAuthorization', () => {
expect(ruleTypeRegistry.get).toHaveBeenCalledTimes(1);
});
});

test('creates a filter based on the privileged types', async () => {
expect(
(
await alertAuthorization.getFindAuthorizationFilter(AlertingAuthorizationEntity.Rule, {
type: AlertingAuthorizationFilterType.KQL,
fieldNames: {
ruleTypeId: 'path.to.rule_type_id',
consumer: 'consumer-field',
},
})
).filter
).toEqual(
fromKueryExpression(
`(path.to.rule_type_id:.esQuery and consumer-field:(alerts or stackAlerts or logs or discover)) or (path.to.rule_type_id:.logs-threshold-o11y and consumer-field:(alerts or stackAlerts or logs or discover)) or (path.to.rule_type_id:.threshold-rule-o11y and consumer-field:(alerts or stackAlerts or logs or discover))`
)
const filter = (
await alertAuthorization.getFindAuthorizationFilter(AlertingAuthorizationEntity.Rule, {
type: AlertingAuthorizationFilterType.KQL,
fieldNames: {
ruleTypeId: 'path.to.rule_type_id',
consumer: 'consumer-field',
},
})
).filter;

expect(toKqlExpression(filter as KueryNode)).toMatchInlineSnapshot(
`"((path.to.rule_type_id: .esQuery AND (consumer-field: alerts OR consumer-field: discover OR consumer-field: stackAlerts OR consumer-field: logs)) OR (path.to.rule_type_id: .logs-threshold-o11y AND (consumer-field: alerts OR consumer-field: discover OR consumer-field: stackAlerts OR consumer-field: logs)) OR (path.to.rule_type_id: .threshold-rule-o11y AND (consumer-field: alerts OR consumer-field: discover OR consumer-field: stackAlerts OR consumer-field: logs)))"`
);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { KueryNode } from '@kbn/es-query';
import { SecurityPluginSetup } from '@kbn/security-plugin/server';
import { PluginStartContract as FeaturesPluginStart } from '@kbn/features-plugin/server';
import { Space } from '@kbn/spaces-plugin/server';
import { STACK_ALERTS_FEATURE_ID } from '@kbn/rule-data-utils';
import { RegistryRuleType } from '../rule_type_registry';
import { ALERTING_FEATURE_ID, RuleTypeRegistry } from '../types';
import {
Expand Down Expand Up @@ -88,6 +89,8 @@ export interface ConstructorOptions {
authorization?: SecurityPluginSetup['authz'];
}

const DISCOVER_FEATURE_ID = 'discover';

export class AlertingAuthorization {
private readonly ruleTypeRegistry: RuleTypeRegistry;
private readonly request: KibanaRequest;
Expand Down Expand Up @@ -135,7 +138,7 @@ export class AlertingAuthorization {

this.allPossibleConsumers = this.featuresIds.then((featuresIds) => {
return featuresIds.size
? asAuthorizedConsumers([ALERTING_FEATURE_ID, ...featuresIds], {
? asAuthorizedConsumers([ALERTING_FEATURE_ID, DISCOVER_FEATURE_ID, ...featuresIds], {
read: true,
all: true,
})
Expand Down Expand Up @@ -328,7 +331,22 @@ export class AlertingAuthorization {
hasAllRequested: boolean;
authorizedRuleTypes: Set<RegistryAlertTypeWithAuth>;
}> {
const fIds = featuresIds ?? (await this.featuresIds);
const fIds = new Set(featuresIds ?? (await this.featuresIds));

/**
* Temporary hack to fix issues with the discover consumer.
* Issue: https://github.com/elastic/kibana/issues/184595.
* PR https://github.com/elastic/kibana/pull/183756 will
* remove the hack and fix it in a generic way.
*
* The discover consumer should be authorized
* as the stackAlerts consumer.
*/
if (fIds.has(DISCOVER_FEATURE_ID)) {
fIds.delete(DISCOVER_FEATURE_ID);
fIds.add(STACK_ALERTS_FEATURE_ID);
}

if (this.authorization && this.shouldCheckAuthorization()) {
const checkPrivileges = this.authorization.checkPrivilegesDynamicallyWithRequest(
this.request
Expand All @@ -347,11 +365,15 @@ export class AlertingAuthorization {
>();
const allPossibleConsumers = await this.allPossibleConsumers;
const addLegacyConsumerPrivileges = (legacyConsumer: string) =>
legacyConsumer === ALERTING_FEATURE_ID || isEmpty(featuresIds);
legacyConsumer === ALERTING_FEATURE_ID ||
legacyConsumer === DISCOVER_FEATURE_ID ||
isEmpty(featuresIds);

for (const feature of fIds) {
const featureDef = this.features
.getKibanaFeatures()
.find((kFeature) => kFeature.id === feature);

for (const ruleTypeId of featureDef?.alerting ?? []) {
const ruleTypeAuth = ruleTypesWithAuthorization.find((rtwa) => rtwa.id === ruleTypeId);
if (ruleTypeAuth) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export function getAlertFormatters(fieldFormats: FieldFormatsRegistry) {
const producer = rowData?.find(({ field }) => field === ALERT_RULE_PRODUCER)?.value?.[0];
const consumer: AlertConsumers = observabilityFeatureIds.includes(producer)
? 'observability'
: producer && (value === 'alerts' || value === 'stackAlerts')
: producer && (value === 'alerts' || value === 'stackAlerts' || value === 'discover')
? producer
: value;
const consumerData = alertProducersData[consumer];
Expand Down
Binary file not shown.
18 changes: 18 additions & 0 deletions x-pack/test/rule_registry/common/lib/authentication/roles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,23 @@ export const logsOnlyAllSpacesAll: Role = {
},
};

export const stackAlertsOnlyAllSpacesAll: Role = {
name: 'stack_alerts_only_all_spaces_all',
privileges: {
elasticsearch: {
indices: [],
},
kibana: [
{
feature: {
stackAlerts: ['all'],
},
spaces: ['*'],
},
],
},
};

/**
* This role exists to test that the alert search strategy allows
* users who do not have access to security solutions the ability
Expand Down Expand Up @@ -494,6 +511,7 @@ export const allRoles = [
securitySolutionOnlyReadSpacesAll,
observabilityOnlyAllSpacesAll,
logsOnlyAllSpacesAll,
stackAlertsOnlyAllSpacesAll,
observabilityOnlyReadSpacesAll,
observabilityOnlyAllSpacesAllWithReadESIndices,
observabilityMinReadAlertsRead,
Expand Down
8 changes: 8 additions & 0 deletions x-pack/test/rule_registry/common/lib/authentication/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
observabilityMinReadAlertsAllSpacesAll,
observabilityOnlyAllSpacesAllWithReadESIndices,
securitySolutionOnlyAllSpacesAllWithReadESIndices,
stackAlertsOnlyAllSpacesAll,
} from './roles';
import { User } from './types';

Expand Down Expand Up @@ -176,6 +177,12 @@ export const logsOnlySpacesAll: User = {
roles: [logsOnlyAllSpacesAll.name],
};

export const stackAlertsOnlySpacesAll: User = {
username: 'stack_alerts_only_all_spaces_all',
password: 'stack_alerts_only_all_spaces_all',
roles: [stackAlertsOnlyAllSpacesAll.name],
};

export const obsOnlySpacesAllEsRead: User = {
username: 'obs_only_all_spaces_all_es_read',
password: 'obs_only_all_spaces_all_es_read',
Expand Down Expand Up @@ -290,6 +297,7 @@ export const allUsers = [
secOnlyReadSpacesAll,
obsOnlySpacesAll,
logsOnlySpacesAll,
stackAlertsOnlySpacesAll,
obsSecSpacesAll,
obsSecReadSpacesAll,
obsMinReadAlertsRead,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import {
obsOnlySpacesAll,
logsOnlySpacesAll,
secOnlySpacesAllEsReadAll,
stackAlertsOnlySpacesAll,
superUser,
} from '../../../common/lib/authentication/users';

type RuleRegistrySearchResponseWithErrors = RuleRegistrySearchResponse & {
Expand Down Expand Up @@ -346,6 +348,85 @@ export default ({ getService }: FtrProviderContext) => {
});
});

describe('discover', () => {
before(async () => {
await esArchiver.load('x-pack/test/functional/es_archives/observability/alerts');
});
after(async () => {
await esArchiver.unload('x-pack/test/functional/es_archives/observability/alerts');
});

it('should return alerts from .es-query rule type with consumer discover with access only to stack rules', async () => {
const result = await secureBsearch.send<RuleRegistrySearchResponse>({
supertestWithoutAuth,
auth: {
username: stackAlertsOnlySpacesAll.username,
password: stackAlertsOnlySpacesAll.password,
},
referer: 'test',
kibanaVersion,
internalOrigin: 'Kibana',
options: {
featureIds: ['discover'],
},
strategy: 'privateRuleRegistryAlertsSearchStrategy',
});

expect(result.rawResponse.hits.total).to.eql(1);

const consumers = result.rawResponse.hits.hits.map((hit) => {
return hit.fields?.['kibana.alert.rule.consumer'];
});

expect(consumers.every((consumer) => consumer === 'discover'));
});

it('should return alerts from .es-query rule type with consumer discover as superuser', async () => {
const result = await secureBsearch.send<RuleRegistrySearchResponse>({
supertestWithoutAuth,
auth: {
username: superUser.username,
password: superUser.password,
},
referer: 'test',
kibanaVersion,
internalOrigin: 'Kibana',
options: {
featureIds: ['discover'],
},
strategy: 'privateRuleRegistryAlertsSearchStrategy',
});

expect(result.rawResponse.hits.total).to.eql(1);

const consumers = result.rawResponse.hits.hits.map((hit) => {
return hit.fields?.['kibana.alert.rule.consumer'];
});

expect(consumers.every((consumer) => consumer === 'discover'));
});

it('should not return alerts from .es-query rule type with consumer discover without access to stack rules', async () => {
const result = await secureBsearch.send<RuleRegistrySearchResponseWithErrors>({
supertestWithoutAuth,
auth: {
username: logsOnlySpacesAll.username,
password: logsOnlySpacesAll.password,
},
referer: 'test',
kibanaVersion,
internalOrigin: 'Kibana',
options: {
featureIds: ['discover'],
},
strategy: 'privateRuleRegistryAlertsSearchStrategy',
});

expect(result.statusCode).to.be(500);
expect(result.message).to.be('Unauthorized to find alerts for any rule types');
});
});

describe('empty response', () => {
it('should return an empty response', async () => {
const result = await secureBsearch.send<RuleRegistrySearchResponse>({
Expand Down

0 comments on commit c7d3d8c

Please sign in to comment.