Allow usage of saved object references in alerts

[timroes]

When storing alerts at the moment there is no way (as far as I could tell) for the alert type (implementation) to make use of saved object references if the alert itself needs to reference other saved objects.

As an example the discover alert will serialize a search source and thus have a reference onto an index pattern saved object stored inside the alert. If this is not properly put into the saved object references when storing, this would break sharing between spaces (and other security related efforts) again. As soon as you'd share an index pattern now with another space its id will be changed and all existing alerts on that index pattern would be broken, since they reference a now non existing id.

Thus an alert need to be able to emit SavedObjectReferences besides it's params when saving and needs to get the parameters and saved object references back for injection in all places the alert is "loaded"/"used" again (like the executor).

This is blocking building any alerts that would make use/reference any other saved object (like index pattern).

cc @legrego

[elasticmachine]

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

[mikecote]

Relates to: #85173

[mikecote]

Another use case from @spong (#85173)

In discussing ways to resolve data integrity issues between Detection Rules and Exception Lists the best path forward seems to be leveraging the existing Saved Object References array, however this isn't currently exposed to those building on top of Alerting SO's. As a workaround, we've been storing our references within AlertParams, which is not searchable, and makes linking back to Rules slow (table scan) unless a back-reference is stored.

[mikecote]

Another feature that may require the usage of references is the import/export of alerts (#50266). We may need to link alerts to another saved object to properly support import/export functionality (ex: creating new ids on import).

[pmuellr]

Thinking through what we need here. If we just wanted to extend the existing APIs, it would be something like this:

• ability to pass SO refs on create and update
• ability to return SO refs on get and find

There's a bit of a collision in that we currently already use SO refs for actions. So we'll need to make sure that when we create the names of the SO refs for the actions, they don't collide with whatever names are being passed for the new SO refs params. We have a simplistic scheme for it now where the name is action_${index of action in actions list}, but I think we'll have to do better. UUID, perhaps?

Or we could say if you are going to add SO refs, you should follow a pattern of using a unique prefix, and action_ is already taken.

I'm wondering if we will have any issues with multiple clients trying to manage these. I guess given the current requirements, we'll have Discover (and other Kibana apps) adding SO refs, and also RAC and Security adding SO refs. Will this get cumbersome for clients to search for and update the SO refs they are interested in? Could one client inadvertently delete an SO ref another client needs?

And I wonder if we should expose the current action SO refs in the same fashion. I think not, but then we'll have a weird disconnect that looking at the SO refs in the alert objects at the client level won't show the actions, but they are actually there in the raw SO's.

Also wondering if this is something we'd expose at the API client level but not the HTTP level. At least the create/update - I suspect returning them on get/find is fine for HTTP. Keeping them out of the HTTP API means less of chance of a customer accidentally deleting critical SO refs.

[mikecote]

Also wondering if this is something we'd expose at the API client level but not the HTTP level. At least the create/update - I suspect returning them on get/find is fine for HTTP. Keeping them out of the HTTP API means less of chance of a customer accidentally deleting critical SO refs.

Yeah, in theory, the alert type will know how to extract the references from the params object and we could get away without changing out APIs.

There's a bit of a collision in that we currently already use SO refs for actions.

Since we have control over this, we could catch the collision between the rule type / API-provided references and our "reserved" reference names and return an error.

[spong]

Thinking through what we need here. If we just wanted to extend the existing APIs, it would be something like this:

• ability to pass SO refs on create and update
• ability to return SO refs on get and find

The above implementation sounds good to me @pmuellr, with the note that we could also add items to the references array during an SO migration as well (covering the case here #85173 where we would move over the exceptions reference from a dedicated field when migrating the security rule type).

@mikecote and I spoke about this earlier today with regards to being able to use the SO Management UI to export Security Rules and all associated Exceptions and Value lists. We're planning to do the rule migration in 7.14-7.15, and so having these changes for 7.15 if possible would be ideal.

[ymao1]

Currently working on this issue and am taking the approach of providing a framework level hook to allow individual rule types to register functions for extracting/injecting saved object references from and into a rule's parameters.

export interface AlertType<
  Params extends AlertTypeParams = never,
  ExtractedParams extends AlertTypeParams = never
  State extends AlertTypeState = never,
  InstanceState extends AlertInstanceState = never,
  InstanceContext extends AlertInstanceContext = never,
  ActionGroupIds extends string = never,
  RecoveryActionGroupId extends string = never
> {
  id: string;
  name: string;
  actionGroups: Array<ActionGroup<ActionGroupIds>>;
  defaultActionGroupId: ActionGroup<ActionGroupIds>['id'];
  recoveryActionGroup?: ActionGroup<RecoveryActionGroupId>;
  executor: ExecutorType;
  producer: string;
  actionVariables?: {};
  minimumLicenseRequired: LicenseType;
  /**
   * These are the new hooks
   **/
  useSavedObjectReferences?: {
    extractReferences: (params: Params) => RuleParamsAndRefs<ExtractedParams>;
    injectReferences: (params: SavedObjectAttributes, references: SavedObjectReference[]) => Params;
  };
}

Then in the alerts_client, the extractReferences hook would be called on create and update and the injectReferences would be called on get and find. There would be some minimal checks to ensure none of the extracted reference names clash with the names we use for actions (action_${number}) but otherwise the implementation of these hooks would be left up to the rule type.

Example hooks implementation:

useSavedObjectReferences: {
  extractReferences: (params: Params): RuleParamsAndRefs<ExtractedParams> => {
    const { testSavedObjectId, ...otherParams } = params;

    const testSavedObjectRef = 'testRef_0';
    const references = [
      {
        name: `testRef_0`,
        id: testSavedObjectId,
        type: 'index-pattern',
      },
    ];
    return { params: { ...otherParams, testSavedObjectRef }, references };
  },
  injectReferences: (params: SavedObjectAttributes, references: SavedObjectReference[]) => {
    const { testSavedObjectRef, ...otherParams } = params;
    const reference = references.find((ref) => ref.name === testSavedObjectRef);
    if (!reference) {
      throw new Error(`Test reference "${testSavedObjectRef}"`);
    }
    return { ...otherParams, testSavedObjectId: reference.id } as Params;
  },
},

Would this approach work for your needs @spong @timroes?

[ymao1]

There are two approaches we can take with migrations:

Approach 1 - Framework handles migrations

Alerting framework detects whether useSavedObjectReferences.extractReferences hook is implemented for a rule type and applies it to rules of this rule type.

Pros

• Rule type would not need to write its own migration, they would just need to implement the useSavedObjectReferences.extractReferences hook and the migration would be handled for them

Cons

• Framework would need to remember to add this migration for every release in order to properly migrate rule types that have added this hook for the release. If we forget, the rules will be in a bad state.
• Framework would need to be careful not the run the migration on rules that have already been migrated. Suppose security implemented the hook in 7.14 and APM implemented it in 7.15. We would need to have a way to detect that the security rules are already migrated in 7.14 and skip migrating them for 7.15. This could be difficult since the framework doesn't always know the details of the rule type params. In addition, if a rule type changed their extractReferences implementation in a version, we would not know whether the change requires a migration or not.
• It would be easy for a rule type to implement a useSavedObjectReferences.extractReferences and not write migration tests for it since they don't actively have to think about the migration. This could lead to improperly migrated rules.

Approach 2 - Rule types handle migrations

When a rule type producer implements the useSavedObjectReferences.extractReferences hook, they must write the migration for that rule type.

Pros

• Since each rule type is writing its own migration, they can ensure that all the edge cases for their rule type are handled and that rule type specific tests are written for the migration.

Cons

• Rule type producers would need to remember to write this migration. Since the rule definition is within each solution's plugin, alerting framework may not know that this hook has been implemented. If rule type producer doesn't realize that a migration is needed, this could lead to a forgotten migration.

I am thinking we should go with Approach 2 since we currently have two downstream solutions that would use these saved object reference hooks and one of them has not yet implemented their rule type (meaning they won't need a migration). We can make sure to work closely with security when they implement these hooks to make sure the correct migrations are written. Interested in other opinions if there are any.

[spong]

Heya @ymao1, so all of the above sounds good to me from the Security Solution perspective, but let experiment a little bit tomorrow just to make sure there aren't any corner cases I'm missing. I think we should be good with Approach 2 as outlined for migrations as well.

For some context from the migration perspective, I'm thinking we can just add the associated SO references during the migration from the single siem-signals rule type to new dedicated rule types for each security rule (EQL, Threshold, etc), and that should play along nice with Approach 2.

[mikecote]

I'm +1 on Approach 2. To mitigate the cons of this approach, we could make our documentation clear not to forget about migration in such use cases.

[spong]

So I think we're good here @ymao1! Chatted a little with @mikecote today as well and the only extra item that came up was if the SO References[] would also be exposed via the Rules API. We were both along the lines of keeping access server side and Security can add any additional specific API's it needs for linking/unlinking any associated Exception Lists to Rules as we develop features that require this behavior.

Please ping me on the PR for this when ready and I'll check it out and create some test migrations to ensure everything is good to go from the security side. Thanks for your efforts here! 🙂

[timroes]

Just a note on prefixing the "alerting internal" references with action_ and preventing clashing by checking for reserved keyword: Why not simply prefix all custom extracted references, before saving them? i.e. get the list of SavedObjectReference[] via the above API, but before saving, itterate over them and change each of those ref's name to params:${originalRefName}, and then before calling the inject method strip actually retrieve all params:.* references and strip the params: of their name. That way would (a) not require any kind of "reserved keyword" (as long as you internally in your code make sure you're not suddenly naming internal references params:*) and (b) would also more easily allow ONLY passing in the custom references that were extracted into the inject method, by using that custom prefix to filter down references before injecting.

[ymao1]

@timroes Thanks Tim! That sounds like a good idea!

[stacey-gammon]

I didn't quite follow approach 1, but it sounds like approach 2 is closer to what we do with other persistable state situations.

We don't have documentation yet, but until then, I recommend syncing with @ppisljar on the App Services team to help you work through how to use the persistable state interfaces.

One thing that is confusing me is this part:

When a rule type producer implements the useSavedObjectReferences.extractReferences hook, they must write the migration for that rule type.

In other cases of persistable state, a producer may not implement extractReferences but still need a migration, or vice versa.

As for how to make producers aware about the need for writing migrations, this is indeed a weak spot in the architecture, but one we don't have a good idea to work around it right now. We are thinking through improvements.

[ymao1]

One thing that is confusing me is this part:

When a rule type producer implements the useSavedObjectReferences.extractReferences hook, they must write the migration for that rule type.

In other cases of persistable state, a producer may not implement extractReferences but still need a migration, or vice versa.

@stacey-gammon This comment is specific to this issue, which will allow producers of existing rule types to update their rule type with a useSavedObjectReferences.extractReferences. If they do that, they will need to write a migration to update user's existing rule saved objects. You are correct that in any case where a producer changes the schema of their rule type (with or without using extractReferences), they will need a migration.

[ppisljar]

take a look at https://github.com/elastic/kibana/blob/master/src/plugins/kibana_utils/common/persistable_state/index.ts and please ping me on slack as this is probably not clear enough and docs are still missing. If alert type implements PersistableState interface it can provide inject and extract functions which should inject/extract SO references as well as object of versioned migrate functions. the plugin providing specific alert type definition (persistable state definition) is (should be) the only one with internal knowledge of shared state to perform migration or other manipulations on it.

the migration then should be handled by SO migration system (as each alert is represented by SO ?)