Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Exceptions] Add lowercase normalized fields for case-insensitive matching #79

Merged
merged 13 commits into from
Oct 1, 2020

Conversation

madirey
Copy link
Contributor

@madirey madirey commented Sep 10, 2020

This PR adds *.caseless multifields for fields that will be used for case-insensitive exceptions, so that the detection engine can perform case-insensitive comparisons in the same way that the endpoint does (without tokenization).

Related:

@madirey madirey marked this pull request as ready for review September 14, 2020 17:25
@madirey madirey changed the title [Draft][Security Solution] Add lowercase normalized fields for case-insensitive matching [Security Solution][Exceptions] Add lowercase normalized fields for case-insensitive matching Sep 14, 2020
Copy link
Collaborator

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these changes go in the Ext section under each file since they modify the core ECS fields? Thoughts @marshallmain ?

- name: text
type: text

- name: parent.command_line
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process.parent isn't explicitly defined in the core ECS as of version 1.6. It's now created by duplicating all the process fields and storing them on process.parent. So I think you can remove the parent.* one heres since you already have them defined on process.

@@ -153,7 +153,7 @@ $(ROOT_DIR)/out:
build-package: $(ROOT_DIR)/out
rm -rf $(PACKAGES_DIR)
mkdir -p $(PACKAGES_DIR)/endpoint/$(PACKAGE_VERSION)
cp -r $(ROOT_DIR)/package/endpoint/ $(PACKAGES_DIR)/endpoint/$(PACKAGE_VERSION)
cp -r $(ROOT_DIR)/package/endpoint/* $(PACKAGES_DIR)/endpoint/$(PACKAGE_VERSION)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@madirey
Copy link
Contributor Author

madirey commented Sep 21, 2020

@jonathan-buttner @marshallmain Let's talk about this today... these are just multi-field additions to existing fields, so I was thinking we could get away with this... if they go into Ext, that makes our job a little harder as we'd need to set them in the pipeline. Not sure what's the best option going forward, but happy to discuss.

Alternatively, we could use this as a stop-gap and push to get these included in ECS?

@madirey madirey force-pushed the case-insensitive-matches branch from b911543 to 77fb983 Compare September 21, 2020 12:01
@jonathan-buttner
Copy link
Collaborator

@jonathan-buttner @marshallmain Let's talk about this today... these are just multi-field additions to existing fields, so I was thinking we could get away with this... if they go into Ext, that makes our job a little harder as we'd need to set them in the pipeline. Not sure what's the best option going forward, but happy to discuss.

Alternatively, we could use this as a stop-gap and push to get these included in ECS?

Yeah that makes sense. I'm happy to chat whenever.

@marshallmain
Copy link
Collaborator

I think it's simpler to add the extra multi-fields to the core ECS fields rather than having a new custom field. We've had user confusion about the existing multi-fields on core ECS fields (.text) already and having a completely separate field might be more confusing.

The downside is if ECS does add official .caseless fields and they're not exactly the same as our caseless fields then upgrading the package to a new ECS version is potentially a breaking change. Does the ingest manager have any way of dealing with new package versions that have breaking changes compared to the installed version? @jonathan-buttner

- name: caseless
type: keyword
normalizer: lowercase
- name: text
Copy link
Collaborator

@jonathan-buttner jonathan-buttner Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madirey since ECS already defines the path.text as type text, if you remove that from here does it still get outputted or does it get removed?

Basically I'm wondering if we can avoid having to define the -name: text fields in these files.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like ECS doesn't support merging custom and core multi_fields. I'll open an issue in the ECS repo.

@jonathan-buttner
Copy link
Collaborator

I think it's simpler to add the extra multi-fields to the core ECS fields rather than having a new custom field. We've had user confusion about the existing multi-fields on core ECS fields (.text) already and having a completely separate field might be more confusing.

The downside is if ECS does add official .caseless fields and they're not exactly the same as our caseless fields then upgrading the package to a new ECS version is potentially a breaking change. Does the ingest manager have any way of dealing with new package versions that have breaking changes compared to the installed version? @jonathan-buttner

The general guidance from the ingest team is to avoid making breaking changes until a major version change 7.x -> 8.0. I think the way the ingest manager app handles installing a new package is it forces a rollover on the index so the new mapping being applied shouldn't collide with the old index. Is that accurate @ruflin ?

@madirey It'd probably be a good idea to create a ticket in ECS to discuss the .caseless field as well. I think I'm ok with adding them here but if we can get them in ECS core eventually too that'd be great.

@ruflin
Copy link

ruflin commented Sep 22, 2020

@jonathan-buttner At first, ingest manager does try to update the mapping. This works if there is no conflict. If there is a conflict, it will update the template and trigger a rollover.

Copy link
Collaborator

@marshallmain marshallmain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants