Skip to content

Commit

Permalink
[SIEM][Detections] Restrict ML rule modification to ML Admins (#65583) (
Browse files Browse the repository at this point in the history
#66102)

* Move common ML types and functions into siem/common

These revolve around capabilities/permissions checks which were
previously only used on the client. Now that we have need to make
similar checks on the server, we can/should move these up to be shared.

* Use ML's Capabilities type in lieu of our own

There was already some drift between these types preventing our
helpers from being used with the ML services; this will prevent further
drift.

* Add authorization helpers for ML

Some of this responsibility will move to ML services in the near future,
but for now we still need to restrict SIEM users from performing certain
actions.

* Use mlAuthz on our import rule route

The tests were a little tricky because the use of spyOn/clear was
preventing (rather, clearing the mocks from) the use of jest.mock().

I found a workaround with mockRestore(), which was easy to verify
because the mock would throw an error if it wasn't removed, and we'd
import multiple rules if a default mock was used.

The threading through of ML can go away if/when ML adds their services
to the request handler context.

* Add mlAuthz checks to remaining rule routes

* Remove validateLicenseForRuleType

This is now unused and redundant with the mlAuthz module.

* Fix failing tests

These were missed when the helpers were moved to common/, but are also
unneeded.

* Cleanup: fixing type errors

* Clean up some types from ML

A recent upstream refactor in ML added top-level exports; this uses them
where possible.

* Refactor mlAuthz to defer authz validation until validator is called

This prevents us from unnecessarily calling ml services if e.g. we're
not dealing with an ML rule.

This also adds a failing test for the next-to-be-implemented feature:
cashing the async validation for subsequent validator calls.

* Cache validation promise

The purpose of the `buildMlAuthz` function is to store state (request,
license, ml). Since `validateMlAuthz` should be idempotent for the
duration of this object's lifecycle, we should cache the result the
first time we call it; this is effectively memoization since the
arguments do not change.

* Make our result caching more explicit

Extracts a caching helper function.

* Add additional unit tests around some edge cases

This is the best form of documentation, thanks Frank!

* Remove redundant test setup

* Empty messages are invalid

If we somehow generate an empty message string, the validation should
fail as we were attempting to assign _something_ as a failure message.

* Fix validity logic

valid: message !== null was the opposite of what I wanted; a validation
is valid if it has no message (i.e. it's undefined).

* Prevent patching of ML rules by non-ML admins

This required refactoring patchRules to accept the rule to be patched,
so that we can check its attributes before performing the update.

* Fix our update_prepackaged_rules route

patchRules no longer does the fetch; we need to perform this ourselves.

* Fix update_prepackaged_rules tests

This notably synchronizes the entirety of the updates, as our tests were
failing due to the asynchronous nature of the updates.

* Remove id and ruleId from patchRules parameters

Instead of fetching the rule within patchRules, we now pass it in.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
  • Loading branch information
rylnd and elasticmachine authored May 12, 2020
1 parent f7eb711 commit 4eb2cdf
Show file tree
Hide file tree
Showing 65 changed files with 935 additions and 380 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,14 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { MlCapabilities } from './types';
import { MlCapabilitiesResponse } from '../../../ml/common/types/capabilities';

export const emptyMlCapabilities: MlCapabilities = {
export const emptyMlCapabilities: MlCapabilitiesResponse = {
capabilities: {
canAccessML: false,
canGetAnnotations: false,
canCreateAnnotation: false,
canDeleteAnnotation: false,
canGetJobs: false,
canCreateJob: false,
canDeleteJob: false,
Expand All @@ -26,11 +30,8 @@ export const emptyMlCapabilities: MlCapabilities = {
canCreateFilter: false,
canDeleteFilter: false,
canFindFileStructure: false,
canGetDataFrame: false,
canDeleteDataFrame: false,
canPreviewDataFrame: false,
canCreateDataFrame: false,
canStartStopDataFrame: false,
canCreateDatafeed: false,
canDeleteDatafeed: false,
canGetDataFrameAnalytics: false,
canDeleteDataFrameAnalytics: false,
canCreateDataFrameAnalytics: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { hasMlAdminPermissions } from './has_ml_admin_permissions';
import { cloneDeep } from 'lodash/fp';
import { emptyMlCapabilities } from '../empty_ml_capabilities';
import { emptyMlCapabilities } from './empty_ml_capabilities';

describe('has_ml_admin_permissions', () => {
let mlCapabilities = cloneDeep(emptyMlCapabilities);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,21 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { MlCapabilities } from '../types';
import { MlCapabilitiesResponse } from '../../../ml/common/types/capabilities';

export const hasMlAdminPermissions = (capabilities: MlCapabilities): boolean =>
export const hasMlAdminPermissions = (capabilities: MlCapabilitiesResponse): boolean =>
getDataFeedPermissions(capabilities) &&
getJobPermissions(capabilities) &&
getFilterPermissions(capabilities) &&
getCalendarPermissions(capabilities);

const getDataFeedPermissions = ({ capabilities }: MlCapabilities): boolean =>
const getDataFeedPermissions = ({ capabilities }: MlCapabilitiesResponse): boolean =>
capabilities.canGetDatafeeds &&
capabilities.canStartStopDatafeed &&
capabilities.canUpdateDatafeed &&
capabilities.canPreviewDatafeed;

const getJobPermissions = ({ capabilities }: MlCapabilities): boolean =>
const getJobPermissions = ({ capabilities }: MlCapabilitiesResponse): boolean =>
capabilities.canCreateJob &&
capabilities.canGetJobs &&
capabilities.canUpdateJob &&
Expand All @@ -27,8 +27,8 @@ const getJobPermissions = ({ capabilities }: MlCapabilities): boolean =>
capabilities.canCloseJob &&
capabilities.canForecastJob;

const getFilterPermissions = ({ capabilities }: MlCapabilities) =>
const getFilterPermissions = ({ capabilities }: MlCapabilitiesResponse) =>
capabilities.canGetFilters && capabilities.canCreateFilter && capabilities.canDeleteFilter;

const getCalendarPermissions = ({ capabilities }: MlCapabilities) =>
const getCalendarPermissions = ({ capabilities }: MlCapabilitiesResponse) =>
capabilities.canCreateCalendar && capabilities.canGetCalendars && capabilities.canDeleteCalendar;
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { cloneDeep } from 'lodash/fp';
import { hasMlUserPermissions } from './has_ml_user_permissions';
import { emptyMlCapabilities } from '../empty_ml_capabilities';
import { emptyMlCapabilities } from './empty_ml_capabilities';

describe('has_ml_user_permissions', () => {
let mlCapabilities = cloneDeep(emptyMlCapabilities);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { MlCapabilities } from '../types';
import { MlCapabilitiesResponse } from '../../../ml/common/types/capabilities';

export const hasMlUserPermissions = (capabilities: MlCapabilities): boolean =>
export const hasMlUserPermissions = (capabilities: MlCapabilitiesResponse): boolean =>
capabilities.capabilities.canGetJobs &&
capabilities.capabilities.canGetDatafeeds &&
capabilities.capabilities.canGetCalendars;
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { isJobStarted, isJobLoading, isJobFailed } from './ml_helpers';
import { isJobStarted, isJobLoading, isJobFailed } from './helpers';

describe('isJobStarted', () => {
test('returns false if only jobState is enabled', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { RuleType } from './types';
import { RuleType } from '../detection_engine/types';

// Based on ML Job/Datafeed States from x-pack/legacy/plugins/ml/common/constants/states.js
const enabledStates = ['started', 'opened'];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { useState, useEffect } from 'react';
import { DEFAULT_ANOMALY_SCORE } from '../../../../common/constants';
import { anomaliesTableData } from '../api/anomalies_table_data';
import { InfluencerInput, Anomalies, CriteriaFields } from '../types';
import { hasMlUserPermissions } from '../permissions/has_ml_user_permissions';
import { hasMlUserPermissions } from '../../../../common/machine_learning/has_ml_user_permissions';
import { useSiemJobs } from '../../ml_popover/hooks/use_siem_jobs';
import { useMlCapabilities } from '../../ml_popover/hooks/use_ml_capabilities';
import { useStateToaster, errorToToaster } from '../../toasters';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { InfluencerInput, MlCapabilities } from '../types';
import { MlCapabilitiesResponse } from '../../../../../ml/public';
import { KibanaServices } from '../../../lib/kibana';
import { InfluencerInput } from '../types';

export interface Body {
jobIds: string[];
Expand All @@ -20,8 +21,8 @@ export interface Body {
maxExamples: number;
}

export const getMlCapabilities = async (signal: AbortSignal): Promise<MlCapabilities> => {
return KibanaServices.get().http.fetch<MlCapabilities>('/api/ml/ml_capabilities', {
export const getMlCapabilities = async (signal: AbortSignal): Promise<MlCapabilitiesResponse> => {
return KibanaServices.get().http.fetch<MlCapabilitiesResponse>('/api/ml/ml_capabilities', {
method: 'GET',
asSystemRequest: true,
signal,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@

import React, { useState, useEffect } from 'react';

import { MlCapabilities } from '../types';
import { MlCapabilitiesResponse } from '../../../../../ml/public';
import { emptyMlCapabilities } from '../../../../common/machine_learning/empty_ml_capabilities';
import { getMlCapabilities } from '../api/get_ml_capabilities';
import { emptyMlCapabilities } from '../empty_ml_capabilities';
import { errorToToaster, useStateToaster } from '../../toasters';

import * as i18n from './translations';

interface MlCapabilitiesProvider extends MlCapabilities {
interface MlCapabilitiesProvider extends MlCapabilitiesResponse {
capabilitiesFetched: boolean;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ import React from 'react';
import { useAnomaliesTableData } from '../anomaly/use_anomalies_table_data';
import { HeaderSection } from '../../header_section';

import { hasMlUserPermissions } from '../../../../common/machine_learning/has_ml_user_permissions';
import * as i18n from './translations';
import { getAnomaliesHostTableColumnsCurated } from './get_anomalies_host_table_columns';
import { convertAnomaliesToHosts } from './convert_anomalies_to_hosts';
import { Loader } from '../../loader';
import { getIntervalFromAnomalies } from '../anomaly/get_interval_from_anomalies';
import { AnomaliesHostTableProps } from '../types';
import { hasMlUserPermissions } from '../permissions/has_ml_user_permissions';
import { useMlCapabilities } from '../../ml_popover/hooks/use_ml_capabilities';
import { BasicTable } from './basic_table';
import { hostEquality } from './host_equality';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import React from 'react';
import { useAnomaliesTableData } from '../anomaly/use_anomalies_table_data';
import { HeaderSection } from '../../header_section';

import { hasMlUserPermissions } from '../../../../common/machine_learning/has_ml_user_permissions';
import * as i18n from './translations';
import { convertAnomaliesToNetwork } from './convert_anomalies_to_network';
import { Loader } from '../../loader';
import { AnomaliesNetworkTableProps } from '../types';
import { getAnomaliesNetworkTableColumnsCurated } from './get_anomalies_network_table_columns';
import { useMlCapabilities } from '../../ml_popover/hooks/use_ml_capabilities';
import { hasMlUserPermissions } from '../permissions/has_ml_user_permissions';
import { BasicTable } from './basic_table';
import { networkEquality } from './network_equality';
import { getCriteriaFromNetworkType } from '../criteria/get_criteria_from_network_type';
Expand Down
47 changes: 2 additions & 45 deletions x-pack/plugins/siem/public/components/ml/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { Influencer } from '../../../../ml/public';

import { HostsType } from '../../store/hosts/model';
import { NetworkType } from '../../store/network/model';
import { FlowTarget } from '../../graphql/types';

export interface Influencer {
influencer_field_name: string;
influencer_field_values: string[];
}

export interface Source {
job_id: string;
result_type: string;
Expand All @@ -35,11 +32,6 @@ export interface Source {
influencers: Influencer[];
}

export interface Influencer {
influencer_field_name: string;
influencer_field_values: string[];
}

export interface CriteriaFields {
fieldName: string;
fieldValue: string;
Expand Down Expand Up @@ -100,41 +92,6 @@ export type AnomaliesNetworkTableProps = HostOrNetworkProps & {
flowTarget?: FlowTarget;
};

export interface MlCapabilities {
capabilities: {
canGetJobs: boolean;
canCreateJob: boolean;
canDeleteJob: boolean;
canOpenJob: boolean;
canCloseJob: boolean;
canForecastJob: boolean;
canGetDatafeeds: boolean;
canStartStopDatafeed: boolean;
canUpdateJob: boolean;
canUpdateDatafeed: boolean;
canPreviewDatafeed: boolean;
canGetCalendars: boolean;
canCreateCalendar: boolean;
canDeleteCalendar: boolean;
canGetFilters: boolean;
canCreateFilter: boolean;
canDeleteFilter: boolean;
canFindFileStructure: boolean;
canGetDataFrame: boolean;
canDeleteDataFrame: boolean;
canPreviewDataFrame: boolean;
canCreateDataFrame: boolean;
canStartStopDataFrame: boolean;
canGetDataFrameAnalytics: boolean;
canDeleteDataFrameAnalytics: boolean;
canCreateDataFrameAnalytics: boolean;
canStartStopDataFrameAnalytics: boolean;
};
isPlatinumOrTrialLicense: boolean;
mlFeatureEnabledInSpace: boolean;
upgradeInProgress: boolean;
}

const sourceOrDestination = ['source.ip', 'destination.ip'];

export const isDestinationOrSource = (value: string | null): value is DestinationOrSource =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@
import { mockSiemJobs } from './__mocks__/api';
import { filterJobs, getStablePatternTitles, searchFilter } from './helpers';

jest.mock('../ml/permissions/has_ml_admin_permissions', () => ({
hasMlAdminPermissions: () => true,
}));

describe('helpers', () => {
describe('filterJobs', () => {
test('returns all jobs when no filter is suplied', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { useEffect, useState } from 'react';
import { DEFAULT_INDEX_KEY } from '../../../../common/constants';
import { checkRecognizer, getJobsSummary, getModules } from '../api';
import { SiemJob } from '../types';
import { hasMlUserPermissions } from '../../ml/permissions/has_ml_user_permissions';
import { hasMlUserPermissions } from '../../../../common/machine_learning/has_ml_user_permissions';
import { errorToToaster, useStateToaster } from '../../toasters';
import { useUiSetting$ } from '../../../lib/kibana';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
isJobLoading,
isJobFailed,
isJobStarted,
} from '../../../../common/detection_engine/ml_helpers';
} from '../../../../common/machine_learning/helpers';
import { SiemJob } from '../types';

const StaticSwitch = styled(EuiSwitch)`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ import { MlPopover } from './ml_popover';

jest.mock('../../lib/kibana');

jest.mock('../ml/permissions/has_ml_admin_permissions', () => ({
hasMlAdminPermissions: () => true,
}));

describe('MlPopover', () => {
test('shows upgrade popover on mouse click', () => {
const wrapper = mountWithIntl(<MlPopover />);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import styled from 'styled-components';

import { useKibana } from '../../lib/kibana';
import { METRIC_TYPE, TELEMETRY_EVENT, track } from '../../lib/telemetry';
import { hasMlAdminPermissions } from '../ml/permissions/has_ml_admin_permissions';
import { hasMlAdminPermissions } from '../../../common/machine_learning/has_ml_admin_permissions';
import { errorToToaster, useStateToaster, ActionToaster } from '../toasters';
import { setupMlJob, startDatafeeds, stopDatafeeds } from './api';
import { filterJobs } from './helpers';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { InspectButton, InspectButtonContainer } from '../../../inspect';
import { HostItem } from '../../../../graphql/types';
import { Loader } from '../../../loader';
import { IPDetailsLink } from '../../../links';
import { hasMlUserPermissions } from '../../../ml/permissions/has_ml_user_permissions';
import { hasMlUserPermissions } from '../../../../../common/machine_learning/has_ml_user_permissions';
import { useMlCapabilities } from '../../../ml_popover/hooks/use_ml_capabilities';
import { AnomalyScores } from '../../../ml/score/anomaly_scores';
import { Anomalies, NarrowDateRange } from '../../../ml/types';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import { Loader } from '../../../loader';
import { Anomalies, NarrowDateRange } from '../../../ml/types';
import { AnomalyScores } from '../../../ml/score/anomaly_scores';
import { useMlCapabilities } from '../../../ml_popover/hooks/use_ml_capabilities';
import { hasMlUserPermissions } from '../../../ml/permissions/has_ml_user_permissions';
import { hasMlUserPermissions } from '../../../../../common/machine_learning/has_ml_user_permissions';
import { InspectButton, InspectButtonContainer } from '../../../inspect';

interface OwnProps {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { FormattedRelative } from '@kbn/i18n/react';
import * as H from 'history';
import React, { Dispatch } from 'react';

import { isMlRule } from '../../../../../common/detection_engine/ml_helpers';
import { isMlRule } from '../../../../../common/machine_learning/helpers';
import { Rule, RuleStatus } from '../../../../containers/detection_engine/rules';
import { getEmptyTagValue } from '../../../../components/empty_value';
import { FormattedDate } from '../../../../components/formatted_date';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ import { showRulesTable } from './helpers';
import { allRulesReducer, State } from './reducer';
import { RulesTableFilters } from './rules_table_filters/rules_table_filters';
import { useMlCapabilities } from '../../../../components/ml_popover/hooks/use_ml_capabilities';
import { hasMlAdminPermissions } from '../../../../components/ml/permissions/has_ml_admin_permissions';
import { hasMlAdminPermissions } from '../../../../../common/machine_learning/has_ml_admin_permissions';

const SORT_FIELD = 'enabled';
const initialState: State = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import React from 'react';
import styled from 'styled-components';
import { EuiBadge, EuiIcon, EuiLink, EuiToolTip } from '@elastic/eui';

import { isJobStarted } from '../../../../../../common/detection_engine/ml_helpers';
import { isJobStarted } from '../../../../../../common/machine_learning/helpers';
import { useKibana } from '../../../../../lib/kibana';
import { SiemJob } from '../../../../../components/ml_popover/types';
import { ListItems } from './types';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
} from '@elastic/eui';

import styled from 'styled-components';
import { isJobStarted } from '../../../../../../common/detection_engine/ml_helpers';
import { isJobStarted } from '../../../../../../common/machine_learning/helpers';
import { FieldHook, getFieldValidityAndErrorMessage } from '../../../../../shared_imports';
import { useSiemJobs } from '../../../../../components/ml_popover/hooks/use_siem_jobs';
import { useKibana } from '../../../../../lib/kibana';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
EuiText,
} from '@elastic/eui';

import { isMlRule } from '../../../../../../common/detection_engine/ml_helpers';
import { isMlRule } from '../../../../../../common/machine_learning/helpers';
import { RuleType } from '../../../../../../common/detection_engine/types';
import { FieldHook } from '../../../../../shared_imports';
import { useKibana } from '../../../../../lib/kibana';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import styled from 'styled-components';
import deepEqual from 'fast-deep-equal';

import { DEFAULT_INDEX_KEY } from '../../../../../../common/constants';
import { isMlRule } from '../../../../../../common/detection_engine/ml_helpers';
import { isMlRule } from '../../../../../../common/machine_learning/helpers';
import { IIndexPattern } from '../../../../../../../../../src/plugins/data/public';
import { useFetchIndexPatterns } from '../../../../../containers/detection_engine/rules';
import { DEFAULT_TIMELINE_TITLE } from '../../../../../components/timeline/translations';
Expand Down Expand Up @@ -38,7 +38,7 @@ import {
import { schema } from './schema';
import * as i18n from './translations';
import { filterRuleFieldsForType, RuleFields } from '../../create/helpers';
import { hasMlAdminPermissions } from '../../../../../components/ml/permissions/has_ml_admin_permissions';
import { hasMlAdminPermissions } from '../../../../../../common/machine_learning/has_ml_admin_permissions';

const CommonUseField = getUseField({ component: Field });

Expand Down
Loading

0 comments on commit 4eb2cdf

Please sign in to comment.