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

[Filebeat] Replace Suricata/Eve fields with aliases to ECS fields #10377

Merged
merged 10 commits into from
Jan 29, 2019

Conversation

adriansr
Copy link
Contributor

@adriansr adriansr commented Jan 28, 2019

This PR avoids duplicate data in documents ingested via the Suricata Eve fileset by replacing a few fields with aliases to ECS fields.

This allows to maintain the full set of fields Suricata users may expect while at the same time reducing the size of the events.

The aliased fields are:

Aliased field Target ECS field
suricata.eve.alert.action event.outcome
suricata.eve.alert.severity event.severity
suricata.eve.app_proto network.protocol
suricata.eve.dest_ip destination.ip
suricata.eve.dest_port destination.port
suricata.eve.event_type event.type
suricata.eve.fileinfo.filename file.path
suricata.eve.fileinfo.size file.size
suricata.eve.flow.start event.start
suricata.eve.flow.bytes_toclient destination.bytes
suricata.eve.flow.bytes_toserver source.bytes
suricata.eve.flow.pkts_toclient destination.packets
suricata.eve.flow.pkts_toserver source.packets
suricata.eve.http.hostname url.domain
suricata.eve.http.http_method http.request.method
suricata.eve.http.http_refer http.request.referrer
suricata.eve.http.http_user_agent user_agent.original
suricata.eve.http.length http.response.body.bytes
suricata.eve.http.status http.response.status_code
suricata.eve.proto network.transport
suricata.eve.src_ip source.ip
suricata.eve.src_port source.port
suricata.eve.timestamp @timestamp

Also, the following non-related fixes are performed:

  • "http.response.status_code" was being set from a string and not an integer.
  • "user_agent.original" was being inadvertedly removed by the user_agent processor.
  • Reformatted the ingest pipeline with standard JSON formatting.

@adriansr adriansr added the in progress Pull request is currently in progress. label Jan 28, 2019
@adriansr adriansr requested a review from a team as a code owner January 28, 2019 14:37
@adriansr adriansr requested a review from a team as a code owner January 28, 2019 15:57
Aliased fields don't seem to be handled in dashboards.
This field cannot be aliased to `message` as we need this data in a keyword field.
There is no suitable field for that in ECS.
@adriansr adriansr changed the title [WIP] Alias redundant fields in the Suricata module [Filebeat] Replace Suricata/Eve fields with aliases to ECS fields Jan 28, 2019
@adriansr adriansr added review Filebeat Filebeat SecOps ecs and removed in progress Pull request is currently in progress. labels Jan 28, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/secops

@adriansr adriansr requested a review from ruflin January 28, 2019 19:26
@adriansr
Copy link
Contributor Author

adriansr commented Jan 28, 2019

Note to reviewers: To review the ingest pipeline it's easier if you look at the changes added in every individual commit in the PR.

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.

This is almost ready to go. Good work!

I've noticed one missing alias, for url.path. That's the only change required in my review.

I also have one optional suggestion to make the IN code a little more straightforward.

@@ -76,6 +58,7 @@
"user_agent.major": "7",
"user_agent.minor": "58",
"user_agent.name": "curl",
"user_agent.original": "curl/7.58.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

"suricata.eve.timestamp"
],
"ignore_missing": true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For most of these fields, you could actually get rid of the convert { type: string} and do a straight field rename. Would look much more like the other modules, and would remove the need to have this huge remove operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are completely right. There's a lot of cleanup that can be done

"suricata.eve.http.protocol": "HTTP/1.1",
"suricata.eve.http.status": 200,
"suricata.eve.http.url": "/dd.xml",
Copy link
Contributor

Choose a reason for hiding this comment

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

suricata.eve.http.url => url.path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not so sure about that one, as we will be losing information if s.e.http.url has query and/or fragment part (those go into url.query and url.fragment).

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps then url.original?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, in this case url.original should be used. This is for the value as observed.

Then optionally (out of scope here) the other fields could be populated to break it down (url.path, url.query, url.fragment), or rebuild full context (url.full).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I overlooked url.original. Done!

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM (after addressing review from Mat). I mostly focused on the change in the golden files.

"suricata.eve.flow_id": 2191386088856669,
"suricata.eve.http.hostname": "example.net",
"suricata.eve.http.http_content_type": "text/html",
Copy link
Contributor

Choose a reason for hiding this comment

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

@webmat This and the one below could be something for ECS in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I took a note to add content_type, when reviewing this PR, indeed :-)

What's the other field you're referring to? The field below is method LOL. And .hostname => .domain ;-)

@ruflin
Copy link
Contributor

ruflin commented Jan 29, 2019

I initially thought these should not go into the ecs-migration.yml file but now I think it would make sense to add them there. This has two advantages:

  • In case you missed renaming one of the fields above in the dashboards, the migration script will catch it.
  • We can show a full list of files that we migrated including this out of ecs-migration.yml file.

@adriansr
Copy link
Contributor Author

@ruflin I've updated the ecs-migration.yml, please have a look. I'm unsure if I used the right set of flags. Is there a way to test this migration script?

@ruflin
Copy link
Contributor

ruflin commented Jan 29, 2019

@adriansr The migration script for the dashboards can be found here: #9998 I still have to create the script to generate a nice changelog entry.

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

I'm good with getting this in as is and have follow up PR's if needed.

"type": "string",
"ignore_missing": true
}
},
{
"convert": {
"field": "suricata.eve.http.http_refer",
Copy link
Contributor

Choose a reason for hiding this comment

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

This one's straightforward as well. Any reason why you're not renaming it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, just skipped it by mistake. Good catch

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 as well

One last nit about doing a straight rename for http_refer.

@adriansr
Copy link
Contributor Author

jenkins, test this

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