-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 normalizer for case-insensitivity + deprecate _tags field (new OS field) #77379
Conversation
Pinging @elastic/endpoint-response (Team:Endpoint Response) |
Pinging @elastic/siem (Team:SIEM) |
Pinging @elastic/endpoint-app-team (Feature:Endpoint) |
153eb41
to
e7db631
Compare
Pinging @elastic/ingest-management (Team:Ingest Management) |
x-pack/plugins/security_solution/server/endpoint/lib/artifacts/lists.ts
Outdated
Show resolved
Hide resolved
migration((doc as unknown) as SavedObjectUnsanitizedDoc<OldExceptionListSoSchema>) | ||
).toEqual({ | ||
attributes: { | ||
_tags: ['1234', 'os:windows'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With _tags
deleted from the exceptionListItemSchema won't migrated exception list items fail to validate against it in update_exception_list_item_route
and others if the items still contain _tags
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marshallmain No, the validation is performed against an object that will already have _tags
removed from the SO response.
const { entries, description, created_by, created_at, name, _tags, id } = exceptionListItem; | ||
const os = osFromTagsList(_tags); | ||
const { entries, description, created_by, created_at, name, os_types, id } = exceptionListItem; | ||
const os = os_types.length ? os_types[0] : 'unknown'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😮
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah? 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trusted apps supports only 1 OS per entry, thus why we return a string rather than string[]
. unknown
should never happen (🤞 )
const migrateEntry = (entryToMigrate: EntryType): EntryType => { | ||
const newEntry = entryToMigrate; | ||
if (entriesNested.is(entryToMigrate) && entriesNested.is(newEntry)) { | ||
newEntry.entries = entryToMigrate.entries.map((nestedEntry) => | ||
migrateEntry(nestedEntry) | ||
) as NonEmptyNestedEntriesArray; | ||
} | ||
newEntry.field = entryToMigrate.field.replace('.text', '.caseless'); | ||
return newEntry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does making newEntry
do anything here? it looks like a reference to the same object as entryToMigrate
. The if
conditions appear to be redundant as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marshallmain Thanks, will fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marshallmain Oh, this was actually to get around the no-param-reassign
eslint rule. Leaving it as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the unit test failures are fixed
One other note is that the exception migration from |
@marshallmain I believe @jonathan-buttner is working on that... |
Oops sorry I had missed this previously. Yeah we implemented a check when the user is interacting with the security solution app and in the ingest manager app that checks for a new endpoint package and installs it: |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from a Trusted Apps standpoint. I assume you are still working on getting the changes in for the .caseless
change for trusted apps, right?
const { entries, description, created_by, created_at, name, _tags, id } = exceptionListItem; | ||
const os = osFromTagsList(_tags); | ||
const { entries, description, created_by, created_at, name, os_types, id } = exceptionListItem; | ||
const os = os_types.length ? os_types[0] : 'unknown'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trusted apps supports only 1 OS per entry, thus why we return a string rather than string[]
. unknown
should never happen (🤞 )
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
async chunks size
distributable file count
page load bundle size
Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to Trusted Apps code for the .caseless
field looks good 👍
…ensitivity + deprecate _tags field (new OS field) (#77379) (#79376) * Finish adding .lower to exceptionable fields * Add back migrations * .lower -> .caseless * Add separate field for os type * updates * Type updates * Switch over to osTypes * get rid of _tags * Add tests for schema validation * Remove remaining references to _tags * Another round of test fixes * DefaultArray tests * More test fixes * Fix remaining test failures * types / tests * more test updates * lowercase os values * Address feedback + fix test failure * tests * Fix integration test * process.executable.path -> process.executable.caseless Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
This PR ensures that
*.caseless
fields appearing in exceptions are translated correctly so that the endpoint can use them for case-insensitive comparisons.A migration is added to convert
*.text
fields appearing in exceptions to*.caseless
so that the detection engine will perform case-insensitive comparisons in the same way that the endpoint does (without tokenization).Requires updating to a new endpoint package which contains the below fix:
Related:
Additionally, this PR deprecates usage of
_tags
in thelists
plugin, in favor ofos_types
, which is more explicit. We were only using it for OS designation, so the relevant tags will be migrated. The field will remain in ES for now, but it has been removed from all schemas. This will prevent the confusion between_tags
andtags
and will prevent the user from inadvertently updating_tags
to a value that breaks internal functionality.Finally, this PR fixes the button text when adding an endpoint exception:
Checklist
Delete any items that are not applicable to this PR.
For maintainers