[Security Solution] Siem signals -> alerts as data field and index aliases

[marshallmain]

Summary

Addresses #100103

The purpose of this work is to make the .siem-signals indices forwards-compatible and the alerts-as-data indices backwards-compatible. The field aliases introduced in this PR map the signal fields to their corresponding kibana.alert counterparts (if they exist). Some deprecated signal fields such as signal.parent will be removed in kibana.alert so they won't be available anymore. Forwards-compatibility on .siem-signals means we only have to maintain a single set of queries/visualizations in our UI and they will still work on old data. Backwards compatibility on alerts as data allows existing user rules, queries, and visualizations against .siem-signals to work seamlessly against the new alerts-as-data indices as well.

Testing:
Adding the aliases is done by visiting the Alerts page in the Security Solution with the proper permissions (the same way an admin visits the page initially to create the .siem-signals resources). Admins do not need any permissions beyond what is currently required (https://www.elastic.co/guide/en/security/7.x/detections-permissions-section.html).

Done:

• Create field aliases mapping signals field names to new alerts as data field names (so new visualizations work on old data)
• Create field aliases mapping alerts as data field names to signals field names (so existing visualizations continue to work on new data)
• Add alerts-as-data index aliases to existing .siem-signals indices (so queries on new indices find old data as well)
• Update existing siem signals templates to ensure all siem signals indices will have the correct mappings, even if the siem signals index rollsover after upgrade
• Replace legacy siem signal templates with new component template style
• Add RBAC required fields to .siem-signals indices as runtime fields or const keyword so RBAC works on old data
  • space_ids: const_keyword, consumer: const_keyword, producer: const_keyword, rule type: const_keyword

Follow up work:

• Once [RAC][Security Solution] Add base Security Rule Type #105096 is merged, add the generated aliases into the alerts as data mappings
• Add .siem-signals index aliases to the new alerts-as-data indices (so existing queries and privileges will work on new data)
  • RuleDataClient should take an optional additional alias to add to bootstrapped index
  • When bootstrapping index, RuleDataClient needs to also create a template for the specific alias in order to specify the lifecycle policy and rollover_alias for the concrete indices backing the alias

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

Pinging @elastic/security-detections-response (Team:Detections and Resp)

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)

[marshallmain]

I removed this check so that we can add the .alerts-security.alerts alias to the .siem-signals indices before the .alerts-security.alerts concrete indices are actually created. Otherwise, we would have to wait until the first new alert was written to .alerts before we could add the alias to .siem-signals. This way we can always treat all existing alerts as though they are in .alerts and don't have to worry about handling old and new alerts separately.

[marshallmain]

This check has now been replaced with a more specific check - querying to find concrete indices rather than the alias.

[marshallmain]

illegal_argument_exception is returned if the alias exists but has no write_index.

[weltenwort]

Unfortunately illegal_argument_exception can be returned for many reasons other than the lack of a write index. I can imagine two ways to reduce the risk of misinterpretation:

1. Parse the error message (has other risks).
2. Query the alias for is_write_alias.

Do you think it's safe enough to just assume the reason?

[marshallmain]

That's a good point, I'll add logic similar to the old "alias exists" check in createWriteTargetIfNeeded but instead checking to ensure the alias has no write target before attempting to create a write target for it. You're right that otherwise we could end up with illegal_argument_exceptions that cause us to try creating a new write target and that would cause problems if the alias already has a write target.

[marshallmain]

I replaced the aliasExists check with a more general "index exists" check on the AAD indices. This makes the createWriteTargetIfNeeded function safer to call optimistically when we encounter index_not_found or illegal_argument exceptions while still allowing the security solution to add AAD aliases to our .siem-signals indices. Now, if we do get an illegal_argument_exception for a reason other than "no concrete AAD indices exist", then createWriteTargetIfNeeded will return without making new indices and potentially making things worse.

[weltenwort]

Since I'm lacking any context about the SIEM signals indices I limited myself to inspecting the changes in the rule_registry. I left two questions about that below.

Aside from that I'm seeing when running Kibana against a cluster with existing .siem-signals indices:

[error][plugins][ruleRegistry] Failed to PUT mapping for alias .siem-signals-amsterdam: Missing required parameter: body

[weltenwort]

Unfortunately illegal_argument_exception can be returned for many reasons other than the lack of a write index. I can imagine two ways to reduce the risk of misinterpretation:

1. Parse the error message (has other risks).
2. Query the alias for is_write_alias.

Do you think it's safe enough to just assume the reason?

[weltenwort]

What if the index already exists, but isn't set as the write index for the alias? Wouldn't we have to add the alias here?

[marshallmain]

Is it possible to get into a scenario where the index exists but the alias has no write target, other than by an admin manually creating the index but creating it incorrectly? My assumption was that this function is the only place where the .alerts index can be created so we can rely on it being correctly associated with the alias as the write index if it exists. I was under the impression that the error suppression here was to avoid throwing an error if multiple rule executors attempt to create the index at the same time.

Perhaps if we get a resource_already_exists_exception we could fetch the index settings to ensure that whatever created the index created it correctly?

[weltenwort]

I can't think of a way this code would produce inconsistent alias settings, but I've seen many situations in which backup scripts have not correctly restored all aliases. We might not be able to automatically fix all such situations, but we should try to to make the error message as useful as possible IMHO.

What if we split setting the write index for an existing index into a separate function and call it or createWriteTargetIfNeeded depending on the original error message (index_not_found_exception or illegal_argument_exceptions + alias check)?

[marshallmain]

For both index_not_found_exception and illegal_argument_exception we will want to be able to create a new index to be the write target. Can we punt on adding the alias to existing indices for now and just log the errors? For the security solution use case, we need to be able to create a write index when the alias already exists and has no write index, but we don't need to support adding the alias to existing AAD indices.

Going back to the original question

What if the index already exists, but isn't set as the write index for the alias? Wouldn't we have to add the alias here?

I think not - if the index exists but isn't set as the write index, something has gone wrong but I don't think we have enough information to safely attempt to automatically fix it. If this happens due to a scenario like backup scripts not restoring everything correctly, then (with the latest changes I pushed up) the initial bulk index would fail, we'd call createWriteTargetIfNeeded, and it would find the existing AAD indices even if they aren't aliased correctly so it would not modify the alias or make any indices. The writer would then attempt to write again, fail again, and throw the error up to the rule executor to be logged and maybe displayed in the UI. At that point the error should be available to bubble up to users and tell them that their alias has no write index. Does that sound reasonable?

Re: automatically fixing things, I'm imagining a case where a user restores .alerts-000002 and .alerts-000003 from backups but accidentally sets neither as the write index. If we optimistically create .alerts-000001 as the new write index and set it up with ILM, then everything works fine for a while but eventually when ES attempts to rollover to .alerts-000002 the rollover fails and would require manual intervention to get back to normal. To me, it seems worse to have the system seem to work normally for a while and then fail much later since it would be harder to figure out the root cause (it's easier to see the correlation between restoring backups and failing to write alerts if there's not a long time between the restore and when alert writes started failing).

[weltenwort]

good points 👍 showing a relatively accurate error to the user should be enough for now

[marshallmain]

@weltenwort re:

[error][plugins][ruleRegistry] Failed to PUT mapping for alias .siem-signals-amsterdam: Missing required parameter: body

good catch, thanks! .siem-signals index templates were still legacy templates which don't work with the simulate index template API we use to update index mappings. Adding the AAD aliases to .siem-signals indices caused them to be found by the index mapping update routine, leading to the failure. To fix it, I updated the security solution code to replace the legacy templates with index templates v2.

[weltenwort]

If the index already exists, will we ever get here? indicesExist will have been true in line 127. The error is silent in that case.

[marshallmain]

It's still possible to get here if multiple code paths attempt to create the index at the same time. It can be triggered in testing if the index doesn't exist yet with a construct like

ruleDataClient.getWriter().bulk(request);
ruleDataClient.getWriter().bulk(request);

in this case both calls race to attempt to make the index and often one hits a resource_already_exists_exception. It's also theoretically possible that some other source (e.g. user, backup script) could make the index in between the indices exist check and the index creation, so by fetching the index after getting the exception we can check that it was created correctly.

[weltenwort]

True. But if the lack of a write index was the cause for this function being called we'd just fail silently because indicesExist === true. Would raising an exception in an else branch for the top level check do the trick?

[marshallmain]

Ah I see. I was counting on the bulk retry to fail again in that case and throw the error, but I didn't notice that it wasn't checking for errors on the retry. I added handling for both in 28722b1 - the retry bulk call now throws errors if it encounters any, and if createWriteTargetIfNeeded finds existing indices then it checks to make sure one of them is the write index and throws an error otherwise. I think the bulk retry error handling alone would be sufficient to avoid silently failing, but checking the alias specifically inside createWriteTargetIfNeeded allows us to have a more specific error message for that case so I added both.

[michaelolo24]

Are these going to be shared? There are a bunch of fields in security_solution, not in here and I'm just trying to figure out whether they should be added here or just kept within security_solution i.e. all the original_event fields here:

kibana/x-pack/plugins/security_solution/public/common/components/drag_and_drop/helpers.ts

Line 113 in b5e8db2

const isAllowlistedNonBrowserField = [

[marshallmain]

Yeah the fields here are expected to be used by all solutions that use alerts as data. The original_event fields are security-specific because other solutions are not attempting to preserve the whole source documents (so they don't need to preserve the original event.* fields when they populate fields like event.kind: signal)

[banderror]

Shouldn't it be kibana.consumers? Here it's kibana.alert.consumers.

[marshallmain]

I'm not sure. For now I just made it similar to the existing ALERT_PRODUCER. I think as long as we're using the constants from this package, follow up naming discussions can be had to change the field names as needed.

[banderror]

I'm just looking at the spec and I can't find kibana.alert.consumers there, but:

I think it's kibana.consumers because the concept of shareability between features might be more broad than the alerting domain.

Not something that this PR should be responsible for, so I think we can merge it as is, but I'd double-check this naming at the next alerts-as-data meeting.

[banderror]

Is my understanding correct that kibana.alert.owner is going to get removed in favour of kibana.consumers?

[banderror]

Shouldn't it be kibana.consumers? Here it's kibana.alert.consumers.

[banderror]

Nit: throw new (although seems like there's no difference for the built-in Error constructor, it may or may not work with custom exceptions)

[marshallmain]

as of ES5 they're supposed to be equivalent, right? https://es5.github.io/#x15.11.1

[banderror]

Yes, but for a custom exception this might not work.

Example:

class CustomError extends Error {
  constructor(value) {
    super('Hello this is my message');
    this.value = value;
  }
}

let e = CustomError(42);
console.log(e.value); // ?

Actually, I just checked it in Chrome, and calling CustomError(42) without new just throws TypeError: Class constructor CustomError cannot be invoked without 'new', seems like V8 has a special handling for class constructors.

However, this won't work the same way for any constructor defined as a function. In general, something like that would be needed:

function User(name) {
  if (!new.target) { // if you run me without new
    return new User(name); // ...I will add new for you
  }

  this.name = name;
}

let john = User("John"); // redirects call to new User
alert(john.name); // John

See https://javascript.info/constructor-new#constructor-mode-test-new-target

Maybe I was overthinking when trying to explain it, but for me it's quite simple - let's use new everywhere and in all cases when calling constructors for the sake of safety and consistency. It's like ===.

Sorry, I'm 🚲 🏠 🎨 'ing

[banderror]

Nit: throw new

[banderror]

When can we get illegal_argument_exception and why?

[marshallmain]

illegal_argument_exception can be returned when trying to write to an alias that exists but has no write index

[banderror]

👍 thank you

[banderror]

Seems like it sequentially updates mappings of every concrete index matching the alias (index). If there's a lot of concrete indices, this can take additional time spent for network roundtrips etc.

Is there a reason why we can't update all concrete indices at once via the alias?

If yes, should we maybe split it into buckets of N indices (e.g. 10 or 25), run updates in parallel within each bucket and process buckets sequentially?

[marshallmain]

Yeah bucketing these requests and making them in parallel would be much better. Unfortunately we can't do them all at once because we need to extract the original version from the mapping to preserve it, but we also need to add the aliases_version so that we know which indices have had the compatibility-aliases added. If we do it all at once, then if we apply the mapping across indices with different versions then the versions will be wrong for at least some of them.

[banderror]

Unfortunately we can't do them all at once because we need to extract the original version from the mapping to preserve it

Is it because the PUT mappings request replaces the whole _meta object? Sounds counter-intuitive since PUT mappings behaves more like a PATCH, so I'd expect it to replace _meta.aliases_version and preserve _meta.version.

Maybe I'm just not getting this (if yes, sorry about that):

If we do it all at once, then if we apply the mapping across indices with different versions then the versions will be wrong for at least some of them.

[marshallmain]

Oh yeah PUT mapping behaves like a normal PUT with the _meta field unlike the mappings and replaces the whole thing. I was expecting the same thing you did initially and tried including only the aliases_version in the meta object and it ended up removing the version field.

[banderror]

Similar question here: are getTemplateExists and deleteTemplate now deprecated and shouldn't be used (so should be deleted probably)?
Or maybe can we update them with the new implementation or rename to getLegacyTemplateExists and add a new one getNewTemplateExists?

[marshallmain]

I think they're unused now. Do you think it's worth using functions like that as simple wrappers of ES API calls? Without having any extra logic in the functions it seems like they're extra overhead without much benefit.

[banderror]

Thank you for adding comments here and in other places, it's very helpful 👍

[banderror]

Nit: I'd rename these 2 constants to ALERTS_AS_DATA_FIELD_ALIASES_VERSION and ALERTS_AS_DATA_FIELD_ALIASES_VERSION_FIELD.

[banderror]

This is not used yet, right?
Are they going to be included into an existing component template (security.alerts-mappings), a new component template or into the index template?

[marshallmain]

Yeah this isn't used yet. I had planned to add them into the mapping as a single unit, but the problem is you can't add aliases to a mapping unless the field that is being aliased exists in that mapping - and since we're splitting the mapping into different component templates, we need to add the appropriate aliases to each component individually. I'm afraid we have to add aliases to the technical fields too, which is currently handled by the rule registry.

[banderror]

I see, sounds not very good... Maybe we can workaround it somehow, I'll need to think about it and play with ES API... I don't have a clear suggestion right now.
Np at this point!

[marshallmain]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 6bcb6ae
• Storybooks Preview
• Documentation Changes

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
observability	508.0KB	508.2KB	+160.0B
securitySolution	6.5MB	6.5MB	+160.0B
total			+320.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
apm	44.6KB	44.7KB	+160.0B
infra	149.4KB	149.5KB	+160.0B
uptime	34.9KB	35.1KB	+160.0B
total			+480.0B

Unknown metric groups

API count

id	before	after	diff
securitySolution	1324	1325	+1

API count missing comments

id	before	after	diff
securitySolution	1273	1274	+1

History

• 💚 Build #143045 succeeded 498f9c4
• 💚 Build #143021 succeeded 0214b61
• 💔 Build #142867 failed 4275da7
• 💔 Build #142782 failed 5828090
• 💔 Build #142749 failed fc475fd

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[banderror]

I will go ahead and approve this PR. It looks good enough to me, minor issues can be addressed in follow-up PRs. I think at this point we need to start integrating our PRs more often to start testing everything in integration asap.

Thanks @marshallmain, it's a big and complex one 💪

[kibanamachine]

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 106049

[michaelolo24]

?