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

[RAC][Alerts] - Addition of RBAC to unified alerts index #96014

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b412645
adding alerts actions to security plugin
yctercero Mar 31, 2021
0bbb4d9
adds kibana security feature privileges and updates alerts actions to…
dhurley14 Mar 31, 2021
7d3578b
[RAC] [RBAC] adds rac client initialization to plugin setup / startup…
dhurley14 Apr 1, 2021
80183a3
wip - updating kibana features plugin to include rac and some renamin…
yctercero Apr 2, 2021
2b00b22
still wip
yctercero Apr 2, 2021
21f6e5f
testing out how to view owners for alerts when registering rac plugin…
dhurley14 Apr 2, 2021
5a34390
adds a working racClient to request context and picks up owners suppl…
dhurley14 Apr 5, 2021
fee5036
adds elasticsearch client to provide functionality for rac client, sh…
dhurley14 Apr 5, 2021
1583cd6
adding tests, cleanup, almost there mvp
yctercero Apr 6, 2021
57dbec5
updates unit tests to remove the 'spaceid' from the authz action and …
dhurley14 Apr 6, 2021
bfc2334
fixes bug where we did not assign getSpaces function in rac factory i…
dhurley14 Apr 7, 2021
0498d22
parameterized calls into racClient.get() to match solutions, adds mor…
dhurley14 Apr 8, 2021
9ef683a
adds missing variables from rebase conflicts with master
dhurley14 Apr 12, 2021
3c06ce8
fixes changes from rebase with master
dhurley14 Apr 20, 2021
273ea82
adds an 'owner' field to the siem-signals mapping, working authz get …
dhurley14 Apr 29, 2021
ef85289
Revert "adds an 'owner' field to the siem-signals mapping, working au…
dhurley14 Apr 29, 2021
ee9aae9
adds an 'owner' field to the siem-signals mapping, working authz get …
dhurley14 Apr 30, 2021
87ac3e9
adds consumer / owner field to alerts created by rule registry (#11)
dhurley14 May 6, 2021
350aa97
fixes merge conflicts and updates logging, defaults to always writing…
dhurley14 May 12, 2021
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
10 changes: 6 additions & 4 deletions x-pack/plugins/apm/common/alert_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import type { ValuesType } from 'utility-types';
import type { ActionGroup } from '../../alerting/common';
import { ANOMALY_SEVERITY, ANOMALY_THRESHOLD } from './ml_constants';

export const APM_SERVER_FEATURE_ID = 'apm';

export enum AlertType {
ErrorCount = 'apm.error_rate', // ErrorRate was renamed to ErrorCount but the key is kept as `error_rate` for backwards-compat.
TransactionErrorRate = 'apm.transaction_error_rate',
Expand Down Expand Up @@ -43,7 +45,7 @@ export const ALERT_TYPES_CONFIG: Record<
actionGroups: [THRESHOLD_MET_GROUP],
defaultActionGroupId: THRESHOLD_MET_GROUP_ID,
minimumLicenseRequired: 'basic',
producer: 'apm',
producer: APM_SERVER_FEATURE_ID,
},
[AlertType.TransactionDuration]: {
name: i18n.translate('xpack.apm.transactionDurationAlert.name', {
Expand All @@ -52,7 +54,7 @@ export const ALERT_TYPES_CONFIG: Record<
actionGroups: [THRESHOLD_MET_GROUP],
defaultActionGroupId: THRESHOLD_MET_GROUP_ID,
minimumLicenseRequired: 'basic',
producer: 'apm',
producer: APM_SERVER_FEATURE_ID,
},
[AlertType.TransactionDurationAnomaly]: {
name: i18n.translate('xpack.apm.transactionDurationAnomalyAlert.name', {
Expand All @@ -61,7 +63,7 @@ export const ALERT_TYPES_CONFIG: Record<
actionGroups: [THRESHOLD_MET_GROUP],
defaultActionGroupId: THRESHOLD_MET_GROUP_ID,
minimumLicenseRequired: 'basic',
producer: 'apm',
producer: APM_SERVER_FEATURE_ID,
},
[AlertType.TransactionErrorRate]: {
name: i18n.translate('xpack.apm.transactionErrorRateAlert.name', {
Expand All @@ -70,7 +72,7 @@ export const ALERT_TYPES_CONFIG: Record<
actionGroups: [THRESHOLD_MET_GROUP],
defaultActionGroupId: THRESHOLD_MET_GROUP_ID,
minimumLicenseRequired: 'basic',
producer: 'apm',
producer: APM_SERVER_FEATURE_ID,
},
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
import React, { useCallback, useMemo } from 'react';
import { useParams } from 'react-router-dom';
import { useKibana } from '../../../../../../../src/plugins/kibana_react/public';
import { AlertType } from '../../../../common/alert_types';
import {
AlertType,
APM_SERVER_FEATURE_ID,
} from '../../../../common/alert_types';
import { getInitialAlertValues } from '../get_initial_alert_values';
import { TriggersAndActionsUIPublicPluginStart } from '../../../../../triggers_actions_ui/public';
interface Props {
Expand Down Expand Up @@ -38,7 +41,7 @@ export function AlertingFlyout(props: Props) {
() =>
alertType &&
triggersActionsUi.getAddAlertFlyout({
consumer: 'apm',
consumer: APM_SERVER_FEATURE_ID,
onClose: onCloseAddFlyout,
alertTypeId: alertType,
canChangeTrigger: false,
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/apm/server/feature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@

import { i18n } from '@kbn/i18n';
import { LicenseType } from '../../licensing/common/types';
import { AlertType } from '../common/alert_types';
import { AlertType, APM_SERVER_FEATURE_ID } from '../common/alert_types';
import { DEFAULT_APP_CATEGORIES } from '../../../../src/core/server';
import {
LicensingPluginSetup,
LicensingApiRequestHandlerContext,
} from '../../licensing/server';

export const APM_FEATURE = {
id: 'apm',
id: APM_SERVER_FEATURE_ID,
name: i18n.translate('xpack.apm.featureRegistry.apmFeatureName', {
defaultMessage: 'APM and User Experience',
}),
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/apm/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ export function mergeConfigs(
export const plugin = (initContext: PluginInitializerContext) =>
new APMPlugin(initContext);

export { APM_SERVER_FEATURE_ID } from '../common/alert_types';
export { APMPlugin } from './plugin';
export { APMPluginSetup } from './types';
export { APMServerRouteRepository } from './routes/get_global_apm_server_route_repository';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { apmActionVariables } from './action_variables';
import { alertingEsClient } from './alerting_es_client';
import { RegisterRuleDependencies } from './register_apm_alerts';
import { createAPMLifecycleRuleType } from './create_apm_lifecycle_rule_type';
import { APM_SERVER_FEATURE_ID } from '../../../common/alert_types';

const paramsSchema = schema.object({
windowSize: schema.number(),
Expand Down Expand Up @@ -55,7 +56,7 @@ export function registerErrorCountAlertType({
apmActionVariables.interval,
],
},
producer: 'apm',
producer: APM_SERVER_FEATURE_ID,
minimumLicenseRequired: 'basic',
executor: async ({ services, params }) => {
const config = await config$.pipe(take(1)).toPromise();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ import { schema } from '@kbn/config-schema';
import { take } from 'rxjs/operators';
import { QueryContainer } from '@elastic/elasticsearch/api/types';
import { parseEnvironmentUrlParam } from '../../../common/environment_filter_values';
import { AlertType, ALERT_TYPES_CONFIG } from '../../../common/alert_types';
import {
AlertType,
ALERT_TYPES_CONFIG,
APM_SERVER_FEATURE_ID,
} from '../../../common/alert_types';
import {
PROCESSOR_EVENT,
SERVICE_ENVIRONMENT,
Expand Down Expand Up @@ -65,7 +69,7 @@ export function registerTransactionDurationAlertType({
apmActionVariables.interval,
],
},
producer: 'apm',
producer: APM_SERVER_FEATURE_ID,
minimumLicenseRequired: 'basic',
executor: async ({ services, params }) => {
const config = await config$.pipe(take(1)).toPromise();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
AlertType,
ALERT_TYPES_CONFIG,
ANOMALY_ALERT_SEVERITY_TYPES,
APM_SERVER_FEATURE_ID,
} from '../../../common/alert_types';
import { getMLJobs } from '../service_map/get_service_anomalies';
import { apmActionVariables } from './action_variables';
Expand Down Expand Up @@ -70,7 +71,7 @@ export function registerTransactionDurationAnomalyAlertType({
apmActionVariables.triggerValue,
],
},
producer: 'apm',
producer: APM_SERVER_FEATURE_ID,
minimumLicenseRequired: 'basic',
executor: async ({ services, params }) => {
if (!ml) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@

import { schema } from '@kbn/config-schema';
import { take } from 'rxjs/operators';
import { AlertType, ALERT_TYPES_CONFIG } from '../../../common/alert_types';
import {
AlertType,
ALERT_TYPES_CONFIG,
APM_SERVER_FEATURE_ID,
} from '../../../common/alert_types';
import {
EVENT_OUTCOME,
PROCESSOR_EVENT,
Expand Down Expand Up @@ -59,7 +63,7 @@ export function registerTransactionErrorRateAlertType({
apmActionVariables.interval,
],
},
producer: 'apm',
producer: APM_SERVER_FEATURE_ID,
minimumLicenseRequired: 'basic',
executor: async ({ services, params: alertParams }) => {
const config = await config$.pipe(take(1)).toPromise();
Expand Down
28 changes: 28 additions & 0 deletions x-pack/plugins/features/common/feature_kibana_privileges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,34 @@ export interface FeatureKibanaPrivileges {
*/
read?: readonly string[];
};

/**
* Solutions should specify owners of alerts here which will provide the solution read / write access to those alerts.
*/
rac?: {
Copy link
Contributor

@gmmorris gmmorris Apr 13, 2021

Choose a reason for hiding this comment

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

The name rac suggests this controls access to Rules and Cases as well as Alerts.
I know this isn't true for Rules (as that's controlled by the alerting RBAC).
Not sure about Cases.

I recognise the complication with terminology update, as our privileges would have to stay as alerting for now (as opposed to changing them to be under rules, as that would be a breaking change I believe [cc @legrego ?]), but it feels like naming these privileges rac will add to the confusion.

So, should this be changed to something more specific? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Feature IDs and sub-feature privilege ids are the only entities that cannot be renamed, as these get translated into Elasticsearch Application Privileges, which are then directly assigned to roles. I thought your features were named actions and stackAlerts, unless I'm thinking of something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

naming is hard, what about just SolutionAlerts? I do agree with you @gmmorris

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought your features were named actions and stackAlerts, unless I'm thinking of something else?

Ah, sorry, I'm referring to the key in the privileges object, rather than the feature IDs.
We use alerting:

@XavierM I don't think SolutionAlerts is accurate as it'll be used by Stack Rules too.
Am I right that this will define the RBAC for the alerts as data operations? Feels like this should probably be alertsAsData or alerts, and we should change ours to be rules 🤔

Though... it raises the question: shouldn't the alerts privileges be inferred from rules privileges? Otherwise a user can create a rule but can't read the alerts produced by that rule 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

@mikecote I think we might need to do another terminology change on our end - change our privileges key to rules instead of alerting 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Though... it raises the question: shouldn't the alerts privileges be inferred from rules privileges? Otherwise a user can create a rule but can't read the alerts produced by that rule 😬

We probably have a good reason to have different access controls for alerts, but IMHO inheriting those privileges from the rules make a lot of sense, and is the simplest model I can think of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgieselaar for t1 analysts, they're able to update alerts (status) but not able to write on rules.

@gmmorris I think the renaming to rules would be super helpful

/**
* List of owners of alerts which users should have full read/write access to when granted this privilege.
* @example
* ```ts
* {
* all: ['securitySolution']
* }
* ```
*/
all?: readonly string[];

/**
* List of owners of alerts which users should have read-only access to when granted this privilege.
* @example
* ```ts
* {
* read: ['securitySolution', 'observability']
* }
* ```
*/
read?: readonly string[];
};

/**
* If your feature requires access to specific saved objects, then specify your access needs here.
*/
Expand Down
9 changes: 9 additions & 0 deletions x-pack/plugins/features/common/kibana_feature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@ export interface KibanaFeatureConfig {
*/
alerting?: readonly string[];

/**
* If your feature grants access to specific alerts, you can specify them here to control visibility based on the current space.
*/
rac?: readonly string[];

/**
* Feature privilege definition.
*
Expand Down Expand Up @@ -191,6 +196,10 @@ export class KibanaFeature {
return this.config.reserved;
}

public get rac() {
return this.config.rac;
}

public toRaw() {
return { ...this.config } as KibanaFeatureConfig;
}
Expand Down
30 changes: 29 additions & 1 deletion x-pack/plugins/features/server/feature_schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const managementSchema = Joi.object().pattern(
);
const catalogueSchema = Joi.array().items(Joi.string().regex(uiCapabilitiesRegex));
const alertingSchema = Joi.array().items(Joi.string());
const racSchema = Joi.array().items(Joi.string());

const appCategorySchema = Joi.object({
id: Joi.string().required(),
Expand All @@ -56,6 +57,10 @@ const kibanaPrivilegeSchema = Joi.object({
all: Joi.array().items(Joi.string()).required(),
read: Joi.array().items(Joi.string()).required(),
}).required(),
rac: Joi.object({
all: racSchema,
read: racSchema,
}),
ui: Joi.array().items(Joi.string().regex(uiCapabilitiesRegex)).required(),
});

Expand Down Expand Up @@ -113,6 +118,7 @@ const kibanaFeatureSchema = Joi.object({
management: managementSchema,
catalogue: catalogueSchema,
alerting: alertingSchema,
rac: racSchema,
privileges: Joi.object({
all: kibanaPrivilegeSchema,
read: kibanaPrivilegeSchema,
Expand Down Expand Up @@ -161,7 +167,7 @@ export function validateKibanaFeature(feature: KibanaFeatureConfig) {
throw validateResult.error;
}
// the following validation can't be enforced by the Joi schema, since it'd require us looking "up" the object graph for the list of valid value, which they explicitly forbid.
const { app = [], management = {}, catalogue = [], alerting = [] } = feature;
const { app = [], management = {}, catalogue = [], alerting = [], rac = [] } = feature;

const unseenApps = new Set(app);

Expand All @@ -176,6 +182,8 @@ export function validateKibanaFeature(feature: KibanaFeatureConfig) {

const unseenAlertTypes = new Set(alerting);

const unseenRacTypes = new Set(rac);

function validateAppEntry(privilegeId: string, entry: readonly string[] = []) {
entry.forEach((privilegeApp) => unseenApps.delete(privilegeApp));

Expand Down Expand Up @@ -219,6 +227,23 @@ export function validateKibanaFeature(feature: KibanaFeatureConfig) {
}
}

function validateRacEntry(privilegeId: string, entry: FeatureKibanaPrivileges['rac']) {
const all = entry?.all ?? [];
const read = entry?.read ?? [];

all.forEach((privilegeOwner) => unseenRacTypes.delete(privilegeOwner));
read.forEach((privilegeOwner) => unseenRacTypes.delete(privilegeOwner));

const unknownRacEntries = difference([...all, ...read], rac);
if (unknownRacEntries.length > 0) {
throw new Error(
`Feature privilege ${
feature.id
}.${privilegeId} has unknown rac entries: ${unknownRacEntries.join(', ')}`
);
}
}

function validateManagementEntry(
privilegeId: string,
managementEntry: Record<string, readonly string[]> = {}
Expand Down Expand Up @@ -280,6 +305,8 @@ export function validateKibanaFeature(feature: KibanaFeatureConfig) {

validateManagementEntry(privilegeId, privilegeDefinition.management);
validateAlertingEntry(privilegeId, privilegeDefinition.alerting);

// validateRacEntry(privilegeId, privilegeDefinition.rac);
});

const subFeatureEntries = feature.subFeatures ?? [];
Expand All @@ -290,6 +317,7 @@ export function validateKibanaFeature(feature: KibanaFeatureConfig) {
validateCatalogueEntry(subFeaturePrivilege.id, subFeaturePrivilege.catalogue);
validateManagementEntry(subFeaturePrivilege.id, subFeaturePrivilege.management);
validateAlertingEntry(subFeaturePrivilege.id, subFeaturePrivilege.alerting);
// validateRacEntry(subFeaturePrivilege.id, subFeaturePrivilege.rac);
});
});
});
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/monitoring/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"requiredPlugins": [
"licensing",
"features",
"ruleRegistry",
"data",
"navigation",
"kibanaLegacy",
Expand Down
37 changes: 36 additions & 1 deletion x-pack/plugins/monitoring/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,19 @@ export class MonitoringPlugin
logger: this.log,
});
initInfraSource(config, plugins.infra);
router.get({ path: '/monitoring-myfakepath', validate: false }, async (context, req, res) => {
try {
const racClient = await context.ruleRegistry?.getRacClient();
const thing = await racClient?.find({ owner: 'observability' });
console.error('THE THING!!!', JSON.stringify(thing.body, null, 2));
return res.ok({ body: { success: true, alerts: thing.body.hits.hits } });
} catch (err) {
console.error('monitoring route threw an error');
console.error(err);
return res.unauthorized({ body: { message: err.message } });
// return res.customError({ statusCode: err.statusCode, body: { message: err.message } });
}
});
}

return {
Expand Down Expand Up @@ -244,8 +257,30 @@ export class MonitoringPlugin
}),
category: DEFAULT_APP_CATEGORIES.management,
app: ['monitoring', 'kibana'],
rac: ['observability'],
catalogue: ['monitoring'],
privileges: null,
privileges: {
all: {
rac: {
all: ['observability'],
},
savedObject: {
all: [],
read: [],
},
ui: ['show', 'save', 'alerting:show', 'alerting:save'],
},
read: {
rac: {
all: ['observability'],
},
savedObject: {
all: [],
read: [],
},
ui: ['show', 'save', 'alerting:show', 'alerting:save'],
},
},
alerting: ALERTS,
reserved: {
description: i18n.translate('xpack.monitoring.feature.reserved.description', {
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/monitoring/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import type {
ActionsApiRequestHandlerContext,
} from '../../actions/server';
import type { AlertingApiRequestHandlerContext } from '../../alerting/server';
import { RacApiRequestHandlerContext } from '../../rule_registry/server';
import {
PluginStartContract as AlertingPluginStartContract,
PluginSetupContract as AlertingPluginSetupContract,
Expand Down Expand Up @@ -58,6 +59,7 @@ export interface RequestHandlerContextMonitoringPlugin extends RequestHandlerCon
actions?: ActionsApiRequestHandlerContext;
alerting?: AlertingApiRequestHandlerContext;
infra: InfraRequestHandlerContext;
ruleRegistry?: RacApiRequestHandlerContext;
}

export interface PluginsStart {
Expand Down
Loading