[Security] Adds pre-packaged rule updates through the "Prebuilt Security Detection Rules" Fleet integration

[rw-access]

Summary

Related to https://github.com/elastic/security-team/issues/17
Rewrite of PR #91949 to use security-rule saved object instead of manual JSON.

This allows for the prepackaged/builtin rules for the Detection Engine to be updated via Fleet. When installing the Fleet integration, rules are installed as the security-rule SO. In this PR, we automatically install the integration if it's available. From there, we can look at the security-rule SOs, union them with the prepackaged rules that are compiled in via JSON, and take the latest version of each. That builds the complete set of rules that users can install.

PR dependencies were

• Add kibana/security-rule asset type package-spec#142
• [Security] Add security detection engine package integrations#797
• Fix the saved object type for security_rule assets integrations#824
• Generate an integrations package from a release detection-rules#983
• Change asset type to security_rule detection-rules#1054
• Change the JSON schema for the security_rule Kibana asset detection-rules#1066

Release note

• Adds pre-packaged rule updates through the "Prebuilt Security Detection Rules" Fleet integration.

UX

Some of these screenshots might be from mixed runs and have inconsistent timestamps, but I confirmed the end results.

1. Install rules from FS on a fresh install
   
   

2. Enable the "Prebuilt Security Detection Rules Integration"
   

3. Go back to the detections page. An update is recognized! Note: the banner doesn't mention that there are new rules, it only counts updates to existing rules
   

4. Install the rules. One was updated "(updated) Attempt to modify an Okta Policy" , and one was new: "New Rule from Fleet." Under this current design, rules are never deleted.
   

Next Steps

It's a little odd that Fleet installs the integration to a policy. Opened #96525 to discuss that. We could hide the package until there's a workflow. (internal: true in the manifest?)

There's no logic in place to automatically download and install the security_detection_engine package behind the scenes. It's still a net win regardless. I plan to make a follow-up PR to automatically install that package.

Probably should write some mocks? Could use a hand for the ideal approach.

Checklist

Delete any items that are not applicable to this PR.

• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios

For maintainers

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

[elasticmachine]

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

[FrankHassanabad]

If this total is zero, won't this exit the loop automatically? Just curious, I have used this newer API before. I couldn't tell from the docs.

In other parts of the code base for example I see:

  const ruleUuids: string[] = [];

  for await (const response of pitFinder.find()) {
    ruleUuids.push(...response.saved_objects.map((object) => object.id));
  }

  await pitFinder.close();

  return ruleUuids;

Something like that where they're not looking for totals and just looping until done and then doing a close.

[rw-access]

good question. i couldn't tell either. any guess who we should ping to double check?

[FrankHassanabad]

bummer the .find doesn't take a template input and forces you to do that so as IRuleAssetSavedObject below.

[FrankHassanabad]

fwiw, @ts-expect-error count outside of test files is increasing per release when it should be be decreasing or staying steady or rising only slightly.

Maintainers, operators, and general outsiders from security_solutions who are cutting across our solution pepper them in and then later I think that creates the situation where it feels like the norm to whip them out and use them as if it is a normal typescript thing when in reality they're considered a code smell and something that should be fixed soon if possible.

Granted in a few situations where typescript is very wrong or we have to do an import of JS it is something we cannot avoid. However, we should avoid them altogether even if typescript is slightly wrong but we can work around typescript.

Can we can this code for example to be this:

  const fleetUpdates = fleetRules.filter((r) => {
    const ruleId = ruleMap.get(r.rule_id);
    return ruleId == null || ruleId.version < r.version;
  });

Then that removes the ts-expect-error.

For example, a maintainer can now change that code to have accidental character addition like this below and if it passes the unit tests it will end up shipping broken.

  const fleetUpdates = fleetRules.filter(
    // @ts-expect-error data ruleMap.get() is safe because of ruleMap.has() check
    (r) => !ruleMap_extra_chars.has(r.rule_id) || ruleMap.get(r.rule_id).version < r.version
  );

Even in tests I try to avoid them if I can. My scale of good to bad goes like this:

• When possible use regular typescript/javascript, including generics, templates, etc...
• Ok, I have to work a bit around typescript with a change in code, but I still get type safety.
• Ok, if I have to use an "as cast" like "as " I will, but I tried to avoid it.
• Ok, I have to add a bang, "!" to turn something off. I really tried not to and don't want to but it's needed. I did try to use ? or another technique but failed.
• Ok, if I have to use an "as unknown as case", things are getting worse, but still not too bad, and the alternatives are worse than just doing it in the code.
• Finally, I really have no choice here, I must use a ts-expect-error and hope that it goes away very soon. I understand it's easy to break things :-( in this area of code by accident.

[rw-access]

thanks for the explanation, that's good to understand more of your thought process and approach!
i am definitely not a fan of ts-expect-error style suppression for a whole line either. especially since a future error could be introduced and it's already suppressed. that's not good.

in this case, it seems the typesystem was unable to infer how has and get relate to each other, so even though what i did looks safe it guessed wrong.

storing .get() in a variable seems like just the right solution. you can explicitly check that against null, and then the other check it knows is non-null. i'm trying that change out locally now and running through the type checking

[rw-access]

Is == null or === undefined more idiomatic in this case?

[rw-access]

updated in 3df98ad (for == null)

[FrankHassanabad]

I think you're shoe horning a bit here and this isn't the validateAllPrepackagedRules as it is written is really what you want maybe.

The ts-expect-error kind of gives it away as a bit as maybe something doesn't seem right.

If validateAllPrepackagedRules throws if the rule is not validated and was written when rules on a file system are a thing. Is that what you want? You want a throw here if it is not valid? Also it will say in the error message:

within the folder rules/prepackaged_rules 

I don't know if that's what you want either. The best bet would be to do something like this where you add a new function that is specific to checking and validating and throwing a specific error:

export const validateAllPrepackagedRulesFromSavedObjects = (
  rules: Array<Array<SavedObject<IRuleAssetSOAttributes & SavedObjectAttributes>>>
): AddPrepackagedRulesSchemaDecoded[] => {
  return rules.map((rule) => {
    const decoded = addPrepackagedRulesSchema.decode(rule);
    const checked = exactCheck(rule, decoded);

    const onLeft = (errors: t.Errors): AddPrepackagedRulesSchemaDecoded => {
      const ruleName = rule[0].attributes.name ? rule[0].attributes.name : '(rule name unknown)';
      const ruleId = rule[0].attributes.rule_id
        ? rule[0].attributes.rule_id
        : '(rule rule_id unknown)';
      throw new BadRequestError(
        `name: "${ruleName}", rule_id: "${ruleId}" within the saved object rules/prepackaged_rules ` +
          `is not a valid detection engine rule. Expect the system ` +
          `to not work with pre-packaged rules until this rule is fixed ` +
          `or the file is removed. Error is: ${formatErrors(
            errors
          ).join()}, Full rule contents are:\n${JSON.stringify(rule, null, 2)}`
      );
    };

    const onRight = (schema: AddPrepackagedRulesSchema): AddPrepackagedRulesSchemaDecoded => {
      return schema as AddPrepackagedRulesSchemaDecoded;
    };
    return pipe(checked, fold(onLeft, onRight));
  });
};

/**
 * Retrieve and validate rules that were installed from Fleet as saved objects.
 */
export const getFleetInstalledRules = async (
  client: RuleAssetSavedObjectsClient
): Promise<AddPrepackagedRulesSchemaDecoded[]> => {
  const fleetResponse = await client.all();
  const fleetRules = fleetResponse.map((so) => so.attributes);
  return validateAllPrepackagedRulesFromSavedObjects(fleetRules);
};

Then you can remove the ts-expect-error but also look at that bit of weirdness where I have two arrays?

      const ruleName = rule[0].attributes.name ? rule[0].attributes.name : '(rule name unknown)';
      const ruleId = rule[0].attributes.rule_id
        ? rule[0].attributes.rule_id
        : '(rule rule_id unknown)';

I don't think even that is 100% but it's closer to your intent of validating and then casting the data from a SO key/value generic data structure into a more non-generic data structure which has guaranteed fields such as the io-ts ones. I think maybe there just needs to be a loop over the inner array or something else is possibly off, but it does show at least progress here to where restructuring with a new function would be better.

[rw-access]

yeah, fair enough to make a separate function to validate here, so we don't assume any more than absolutely necessary about the incoming dynamically typed data.

I definitely need to do this check, I just mistakenly assumed that the validateAllPrepackagedRules would also be appropriate here.

    const decoded = addPrepackagedRulesSchema.decode(rule);
    const checked = exactCheck(rule, decoded);

[rw-access]

41536f0

[FrankHassanabad]

LGTM, thanks for all the nice updates and removal of ts-expect-errors! Code looks great here.

[rw-access]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

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

Metrics [docs]

✅ unchanged

History

• 💛 Build #118992 was flaky 41536f0
• 💛 Build #118925 was flaky 84248b3
• 💚 Build #118908 succeeded 1e56114
• 💚 Build #118886 succeeded dc48e1a
• 💚 Build #118572 succeeded a1febb9

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

[kibanamachine]

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.