[Security Solution] blocklist create/edit form

[joeypoon]

Summary

Add blocklist create/edit form.

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)
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

For maintainers

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

[joeypoon]

Still doing some cleanup, validation, and missing tests but functionally it's complete and works. Feel free to start reviewing.

[elasticmachine]

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

[joeypoon]

still working on tests but everything works now so ready for review. there is a bug where adding first blocklist doesn't auto refresh list page as well, looking into this.

[kevinlog]

@joeypoon this is looking great! I checked it out and tried it. The basic functionality is there, I can add/edit/delete entries and the manifest creates the entries.

I did find some issues.

Width of is one of component

When I try to copy/paste hashes it work as expected, however the width of the component breaks the flyout.

Blocklist:

A similar component in Event Filters works much better, can we fix the width of the input/component to make it work better?

Event Filters:

Per policy works, but I'm not seeing the right UI feedback:

I'm not see the checkbox check when I click it.

I do the Policy actually get selected when created, however:

Manifest generation for hash.*

When we generate the manifest for hashes, we need to split them out based on type - sha256, md5, etc. Trusted Apps does something like this.

In Trusted Apps, we add something as a hash, it's represented as hash.* in the UI

However, if you look at the manifest, it will be linked up to a specific hash type.

{
  "entries": [
    {
      "type": "simple",
      "entries": [
        {
          "field": "process.hash.sha256",
          "operator": "included",
          "type": "exact_cased",
          "value": "559e66074a45750f5244809b316a63c6c09abd3d0969d4616f7d4bec6328583f"
        }
      ]
    }
  ]
}

We need to do something similar for Blocklist.

UI:

Manifest entry:

{
  "entries": [
    {
      "type": "simple",
      "entries": [
        {
          "field": "process.hash.*",
          "operator": "included",
          "type": "exact_cased_any",
          "value": [
            "559e66074a45750f5244809b316a63c6c09abd3d0969d4616f7d4bec6328583f"
          ]
        }
      ]
    }
  ]
}

I know we can have multiple hash types here. When creating the manifest, we can check and split those into the correct categories by checking which hash type it is and splitting it into separate entries.

Something like:

{
  "entries": [
    {
      "type": "simple",
      "entries": [
        {
          "field": "process.hash.md5",
          "operator": "included",
          "type": "exact_cased_any",
          "value": [
            "559e66074a45750f5244809b316a63c6c09abd3d0969d4616f7d4bec6328583f",
            "559e66074a45750f5244809b316a63c6c09abd3d0969d4616f7d4bec6328583c"
          ]
        }
      ]
    },
    {
      "type": "simple",
      "entries": [
        {
          "field": "process.hash.sha256",
          "operator": "included",
          "type": "exact_cased_any",
          "value": [
            "559e66074a45750f5244809b316a63c6c09abd3d0969d4616f7d4bec6328583f"
          ]
        }
      ]
    }
  ]
}

In manifest generation, Linux entries for process paths should always be exact_cased_any for the type

When creating a Linux option, the manifest should always the process path to have the type exact_cased_any. Trusted Apps does something similar.

If you look at the Linux entry created now for process path, you get:

{
  "entries": [
    {
      "type": "simple",
      "entries": [
        {
          "field": "process.executable",
          "operator": "included",
          "type": "exact_caseless_any",
          "value": [
            "/opt/bin/asdad",
            "/opt/bin/adaseses"
          ]
        }
      ]
    }
  ]
}

You should get:

{
  "entries": [
    {
      "type": "simple",
      "entries": [
        {
          "field": "process.executable",
          "operator": "included",
          "type": "exact_cased_any",
          "value": [
            "/opt/bin/asdad",
            "/opt/bin/adaseses"
          ]
        }
      ]
    }
  ]
}

[paul-tavares]

Left some feedback and will try to check this out tomorrow and run it locally.

Overall, I found the approach to keeping track of error/warning/visited a little confusing by storing it in a simple object with two properties that hold array's of React nodes, but lets see how others feel about that. I asked for a quick explanation on the Type you defined on how that data is used and what it holds

[paul-tavares]

Why are these react nodes?

I'm also having a hard time understanding the fact that they are both arrays. What is stored in them exactly?

[joeypoon]

helpText (warnings) and errors on EuiFormRow are typed as React.ReactNode[]. these are the actual warning/error messages we're passing directly into EuiFormRow.

[paul-tavares]

Ok. thanks for that. ReactNode alway accepts string, but Its ok to store JSX here if that is preferable.

Can you explain the relation ship between the array of nodes for name and the array of nodes for value in the same object? thats the part that is confusing me.

[joeypoon]

I can probably have better naming here. the only places we need validation messages are on the name input and value input. the values under ItemValidation.name are all the validation messages for the name input and the values under ItemValidation.value are all the validation messages for the value input.

[dasansol92]

This is looking awesome! Left some questions/suggestions, let me know if anything is not clear at all 🙂

[ashokaditya]

Look good so far.

I took it for a spin and I noticed a few things

1. upon creating a blocklist from the empty page, the item is added and a success toast is shown, but the entry doesn't show on the list page right away. It does show up on a refresh.
2. Also, I could add an entry without a name.
3. On editing the entry, changing the field from hash/signature (with its corresponding input value) to path doesn't trigger a warning. Same if you change to hash/signature from path. I would expect the same kind of warnings as we see in TA for field changes when input value already exists.

[ashokaditya]

Agree with @dasansol92. Better to keep this as a separate type from TA entry types. Besides blocklist expects only match_any for now and not the other two.

[joeypoon]

linux manifest + policy edit selection fixed. noticed an issue with validation state being out of sync, looking into that now.

[joeypoon]

considered making this more extensible such as passing entry overrides or something but figured we shouldn't over-engineer it right now.

[paul-tavares]

🔥 🔥
This is awesome. Thanks for all the changes from the prior review. I left a few more, but nothing that should hold this from commit.

[paul-tavares]

maybe on next commit, consider renaming this to isLoadingPolicies instead?

[paul-tavares]

I would like @dasansol92 comment here.
Ideally , we should not have read/write transforms in this base class because it should all be using ExceptionListItemSchema and so there should be no need for transformation of data. I understand from our discussion over slack that this was in response to the UI using just a hash field for the user to enter any of the supported hashes, which we then calculate the field name based on the value entered (ex. process.hash.md5|sha1|sha256).
I'm just not sure if pushing that data value manipulation down to the API service is the right thing to do - to me, it feels move like a UI component prop type of mapping when setting/reading the value the user entered.

That being said, having a "middleware" callback for read/write is not a bad idea, so I'm 👍 this for now. We should really discuss if we want to continue to use those TrustedApp** types, since we have been trying to remove all of them in support of only ExceptionListItemSchema

(FYI: the mappers you moved around were originally in the server API for trusted apps, but moved to the UI when we deleted the TA specific API and started using the List Exceptions API. Our intent was to then have a second pass through to delete all of the types and adjust TA to be just like every other artifact)

cc/ @ashokaditya since he also recently moved around some TA types to the @kbn/** library

[dasansol92]

After thinking a bit more about it and after reading the comment above, I'm agree with what @paul-tavares has written.

As this is needed for UI maybe would be better to have it in an upper level. Also, we have plans to remove this types so we can just add this on that removal plan.

I would say let's move forward with this approach but let's take note about it in order to move/remove it when we get rid of the TA types.

PS: I like the way @joeypoon included this as a kind of hooks in this class. Let's keep this in mind for future pre/post validations, modifications, transformations, etc in frontend side.

[kevinlog]

check it out again and the earlier issues are resolved!

✅ Process path entries for Linux should be exacted_cased_any.

I am now seeing exact_cased_any for Linux paths

{
  "entries": [
    {
      "type": "simple",
      "entries": [
        {
          "field": "process.executable",
          "operator": "included",
          "type": "exact_cased_any",
          "value": [
            "/usr/bin"
          ]
        }
      ]
    }
  ]
}

and Windows paths are also correct:

{
  "entries": [
    {
      "type": "simple",
      "entries": [
        {
          "field": "process.executable",
          "operator": "included",
          "type": "exact_caseless_any",
          "value": [
            "C:\\Name\\Name"
          ]
        }
      ]
    }
  ]
}

✅ Policy selection on Edit
This is also working for me now. When I saw the policy selection and open it back up for editing, the changes are reflected in the UI.

[kevinlog]

LGTM!

[paul-tavares]

@joeypoon one thing I noticed in Kevin's screen capture is that we truncate the values (good), but am wondering, since things like paths can be very long, if we should use a larger size value for the flyout.

@kevinlog ^^ what do you think?

[joeypoon]

@joeypoon one thing I noticed in Kevin's screen capture is that we truncate the values (good), but am wondering, since things like paths can be very long, if we should use a larger size value for the flyout.

@kevinlog ^^ what do you think?

I like this idea. I bumped it up to L size along with test fix.

[ashokaditya]

I notice that I can create blocklist item without a name.

[joeypoon]

@elasticmachine merge upstream

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 15c891c
• Storybooks Preview
• Cloud Deployment
• Webpack Bundle Analyzer

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
securitySolution	2934	2938	+4

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
@kbn/securitysolution-utils	26	27	+1

Async chunks

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

id	before	after	diff
securitySolution	4.7MB	4.8MB	+6.4KB

Page load bundle

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

id	before	after	diff
securitySolution	245.6KB	245.6KB	+7.0B

Unknown metric groups

API count

id	before	after	diff
@kbn/securitysolution-utils	28	29	+1

History

• 💔 Build #30798 failed 641f261
• 💔 Build #30767 failed 7422451
• 💔 Build #30699 failed 429a6b74ccb7b6ae4e8c1ce1f69e0396d0332fac
• 💔 Build #30679 failed e55a91f6db3d564132d85b5a83d83df0f2b88741
• 💔 Build #30286 failed 5f9f492

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