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

[RFC] Wildcard - stage 2 proposal #970

Merged
merged 30 commits into from
Oct 2, 2020

Conversation

ebeahan
Copy link
Member

@ebeahan ebeahan commented Sep 17, 2020

Summary

Revisions to the stage 1 wildcard adoption for consideration to be accepted as a stage 2 proposal.

The most significant addition is the list of fields which are the current candidates for wildcard. Discussion on any fields that should be added or removed is welcomed.

Criteria for consideration

  • Opened pull request for this draft revising the existing proposal
  • Outlined initial field definitions
  • Included a real world example source document
  • Identifies scope of impact of changes to ingestion mechanisms (e.g. beats/logstash), usage mechanisms (e.g. Kibana applications, detections), and the ECS project (e.g. docs, tooling)
  • Subject matter experts weighed in on technical utility of field definitions in the pull request

Markdown preview of this RFC

@ebeahan ebeahan added the RFC label Sep 17, 2020
@ebeahan ebeahan self-assigned this Sep 17, 2020
@@ -0,0 +1,13 @@
---
- name: process
Copy link
Contributor

Choose a reason for hiding this comment

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

do these apply to process.parent asa well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, now that process.parent is managed by the field reuse mechanism, this will indeed apply to it :-)

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Thanks for the new table. I have a few nitpicks as comments below, but the meat of the review will be here.

  • I agree with source, destination, file, os, registry and user_agent fields you've selected
  • I agree with the url fields you've selected so far
  • I have a few challenges on some of the other fields selected, noted below. But mostly good selection there as well, overall 👍

Here are a few more fields I think we should migrate to wildcard now, and have reflected in the RFC's YAML files, for experimental release in 1.7.0:

  • Changes to source & destination should be mirrored to client & server as well. So client.domain, client.registered_domain and same under server.*
  • agent.build.original
  • error.type
  • event.original
    • add index: true in the RFC YAML file as well. Right now it's index false in ECS.
  • http.request.referrer
  • log.file.path, log.logger
  • organization.name
  • as.organization.name (this is not a reuse of the organization field set, it's defined explicitly)
  • tls.client.issuer, tls.client.subject, tls.server.issuer, tls.server.subject
  • x509.issuer.distinguished_name, x509.subject.distinguished_name
  • url.path

Here are a few I would like to consider to migrate in the future. I'd say capture those just in markdown for now:

  • geo.name
  • registry.data.strings
  • pe.product
  • dns.question.name, dns.answers.data

Here are two fields I think we should migrate at 8.0, because they represent a breaking change. Therefore let's capture them in the RFC text, but not in the YAML files.

  • message
  • error.message

Questions / challenges

  • Why migrate agent.name? I doubt this field is widely used. When it is used, I doubt the cardinality is very high.
  • I would remove host.name from the list for now. It's the main identifier of a host for Elastic Security, so lots of aggregations and filtering around host.name would become a bit slower if we migrate it. However I think it's fine to leave host.hostname as wildcard.
  • I would also not migrate host.domain as in this case, it's rarely going to be a fqdn, but rather an AD domain name. Moreover, this is not going to be suspicious data where users need to do wildcard searches, IMO.
  • Why process.name? It's a short executable name in Posix envs. Is it pretty long under Windows? (The long process title should be captured at process.title)
  • I would remove user.domain for the same reason as host.domain

Maybe we can simply capture the "considered" and the "8.0" suggestions I'm making via an additional column in the table for now.

rfcs/text/0001-wildcard-data-type.md Outdated Show resolved Hide resolved
rfcs/text/0001-wildcard-data-type.md Outdated Show resolved Hide resolved
rfcs/text/0001-wildcard-data-type.md Outdated Show resolved Hide resolved
@webmat
Copy link
Contributor

webmat commented Oct 1, 2020

@rw-access Do you think pe.product and registry.data.strings would make sense as wildcard as well?

@webmat
Copy link
Contributor

webmat commented Oct 1, 2020

@dainperkins Do you think dns.question.name and dns.answers.data would make sense as wildcard?

@rw-access
Copy link
Contributor

pe.product - probably not.
registry.path, registry.key, registry.data.strings are all good candidates

@webmat
Copy link
Contributor

webmat commented Oct 1, 2020

@rw-access Yeah .path and .key are already part of the plan. We'll add .data.strings as well. Thanks!

@ebeahan
Copy link
Member Author

ebeahan commented Oct 1, 2020

Changes to source & destination should be mirrored to client & server as well. So client.domain, client.registered_domain and same under server.*
agent.build.original
error.type
event.original
http.request.referrer
log.file.path, log.logger
organization.name
as.organization.name (this is not a reuse of the organization field set, it's defined explicitly)
tls.client.issuer, tls.client.subject, tls.server.issuer, tls.server.subject
x509.issuer.distinguished_name, x509.subject.distinguished_name
url.path

Agree with all the above 👍

Here are a few I would like to consider to migrate in the future. I'd say capture those just in markdown for now:

geo.name
registry.data.strings
pe.product
dns.question.name, dns.answers.data

Is the thinking to let these fields and their usage mature more and revisit wildcard later on?

Here are two fields I think we should migrate at 8.0, because they represent a breaking change. Therefore let's capture them in the RFC text, but not in the YAML files.

message
error.message

A separate conversation, but would we add a text multi-field as well for message and error.message?

Why migrate agent.name? I doubt this field is widely used. When it is used, I doubt the cardinality is very high.

I'm fine with removing it since it's not widely used.

I would remove host.name from the list for now. It's the main identifier of a host for Elastic Security, so lots of aggregations and filtering around host.name would become a bit slower if we migrate it. However I think it's fine to leave host.hostname as wildcard.

👍

I would also not migrate host.domain as in this case, it's rarely going to be a fqdn, but rather an AD domain name. Moreover, this is not going to be suspicious data where users need to do wildcard searches, IMO.

Fair. Even in fairly large AD environments, it's probably unlikely to have more than 100s or perhaps low 1000s of unique domains.

Why process.name? It's a short executable name in Posix envs. Is it pretty long under Windows? (The long process title should be captured at process.title)

Not extremely long. From some winlogbeat-* indices I have from testing, I see process.name fields with values like SpeechModelDownload.exe and TrustedInstaller.exe.

I think I keyed off this one due to the text multi-field, but I'm ok if we omit it from the wildcard list.

I would remove user.domain for the same reason as host.domain

++

@webmat
Copy link
Contributor

webmat commented Oct 1, 2020

under consideration

geo.name, registry.data.strings, pe.product, dns.question.name, dns.answers.data
Is the thinking to let these fields and their usage mature more and revisit wildcard later on?

No it's purely because I wasn't 100% sure myself. So I wanted to put them on the table for consideration without slowing things down.

With Ross' feedback above, we can remove pe.product from the list of considered for now, and convert registry.data.strings right now.

So this leaves only geo.name, dns.question.name and dns.answers.data on the "considered" list.

text to wildcard

A separate conversation, but would we add a text multi-field as well for message and error.message?

For message absolutely; see Windows Event logs :-)
For error.message I would think so as well, but I don't have as clear cut an example when this would be a huge win.

process.name

Ok this could make sense, but I'm still hesitant. Perhaps we add it to the list of fields under consideration for now?

@ebeahan
Copy link
Member Author

ebeahan commented Oct 1, 2020

process.name
Ok this could make sense, but I'm still hesitant. Perhaps we add it to the list of fields under consideration for now?

I'm onboard with that approach.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Thanks for grabbing my commits @ebeahan. Here's a few more notes:

  • When building artifacts with build/ve/bin/python scripts/generator.py --include rfcs/text/0001/, I see the field definition for dns.answers disappearing from the csv, beats/fields.ecs.yml, ecs_flat.yml and ecs_nested.yml. A problem we should tackle separately from this RFC.
  • Note that I didn't end up adding anything as "considered", for now. I've directly added both DNS fields based on another discussion out of band. Since this only left geo.name to be considered, I've directly migrated it instead. Stage 2 is experimental, so we can walk this back if this is a problem.
  • I see you've removed process.name since I cast doubt on it. But looking back at further .text and keyword discussion #570 I see Craig was pushing for more flexibility for that field. Let's ignore my doubts and go with your initial gut feeling, and migrate it to wildcard 😬
  • In line with the point above, and with another request from Craig on further .text and keyword discussion #570, let's also move pe.original_file_name to wildcard

I think we'll be good after this.

rfcs/text/0001-wildcard-data-type.md Outdated Show resolved Hide resolved
@leehinman
Copy link
Contributor

Overall LGTM

One small question/worry. event.original can be large for some events (eg. cloudtrail > 32k) Given that many events have event.original and it can be large the impact on indexing could be significant. Of course the value of having wildcard indexing would be high too.

@webmat
Copy link
Contributor

webmat commented Oct 2, 2020

Good point @leehinman, thanks.

Right now ECS doesn't deal very well with non indexed fields. Currently event.original is not indexed. The docs for this field have a sentence under the description that says it's not indexed. But the sentence doesn't stand out, and it's followed immediately by "type: keyword".

I think we could leave it non-indexed but still "migrate" it to wildcard.

Separately from this RFC I think we could improve on these non indexed fields, and say something to the tone of "this field is not indexed, but if users want to index it anyway, we suggest type: wildcard"

@ebeahan
Copy link
Member Author

ebeahan commented Oct 2, 2020

When building artifacts with build/ve/bin/python scripts/generator.py --include rfcs/text/0001/, I see the field definition for dns.answers disappearing from the csv, beats/fields.ecs.yml, ecs_flat.yml and ecs_nested.yml. A problem we should tackle separately from this RFC.

I wasn't seeing the same behavior in a quick test to reproduce. I'll try some more, and if I can reliable reproduce I'll open an issue for later.

Note that I didn't end up adding anything as "considered", for now. I've directly added both DNS fields based on another discussion out of band. Since this only left geo.name to be considered, I've directly migrated it instead. Stage 2 is experimental, so we can walk this back if this is a problem.

Sounds good. 👍

I addressed the two items @webmat listed and also added http.request.referrer to the field definitions based on our agreement above.

One outstanding question:

From @leehinman's observation, should we edit: https:/ebeahan/ecs/blob/wildcard-rfc-stage-2/rfcs/text/0001/event.yml#L5 and remove the index: true?

@webmat
Copy link
Contributor

webmat commented Oct 2, 2020

@ebeahan Yes for now, let's remove index: true from the event.original field, but leave the "migration" to wildcard in place for the field. If folks actually want to index event.original for their use case, I do think wildcard would be the best recommendation.

However we will address the rendering of this subtlety in the docs separately.

@webmat
Copy link
Contributor

webmat commented Oct 2, 2020

Good 👁️ on adding http.request.referrer 👍

rfcs/text/0001/pe.yml Outdated Show resolved Hide resolved
@webmat
Copy link
Contributor

webmat commented Oct 2, 2020

Stumbled on a typo. Line 158 "Luence" => "Lucene"

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

Thanks everyone for the input!

@ebeahan ebeahan merged commit c7422e8 into elastic:master Oct 2, 2020
@ebeahan ebeahan deleted the wildcard-rfc-stage-2 branch October 2, 2020 20:11
@ebeahan ebeahan mentioned this pull request Nov 7, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants