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

Populate more ECS fields in the Suricata module #10006

Merged
merged 17 commits into from
Jan 22, 2019

Conversation

adriansr
Copy link
Contributor

A few more ECS fields are populated by the ingest pipeline that enriches Suricata's eve.json events.

Additions:

  • http.request.referrer (from suricata.eve.http.http_refer)

  • event.action (from suricata.eve.alert.category)
    describes the action that caused the event.
    Examples: "Attempted Denial of Service", "Successful Administrator Privilege Gain"

  • event.outcome (from suricata.eve.alert.action)
    Possible values: "allowed", "blocked"

  • event.severity (from suricata.eve.alert.severity)
    Possible values: 1, 2 or 3.

  • network.transport (from suricata.eve.proto)
    Examples: "tcp", "udp", "ipv6-icmp"

@elasticmachine
Copy link
Collaborator

Pinging @elastic/secops

@adriansr adriansr added ecs Filebeat Filebeat needs_backport PR is waiting to be backported to other branches. labels Jan 10, 2019
@ruflin ruflin mentioned this pull request Jan 11, 2019
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

There are few more fields that could be populated (not sure what the degree of difficulty will be for some of them w.r.t. doing this with ingest node).

  • source.bytes
  • source.packets
  • destination.bytes
  • destination.packets
  • destination.domain = suricata.eve.http.hostname
  • event.start = suricata.eve.flow.start
  • event.end = suricata.eve.timestamp (assuming this is marking the end)
  • event.duration = end - start
  • network.protocol = app_proto
  • url.path = suricata.eve.http.url (not sure if this also includes the query string though)
  • http.request.method should be lowercased (I think that’s what the ECS docs say, double check).
  • network.type - ipv4/ipv6
  • network.bytes
  • network.packets

Copy link
Member

@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.

This will also need a changelog entry and the ecs-migration.yml field should be updated.

I would assume also some more migration aliases have to be added to the fields.yml?

@@ -93,7 +100,39 @@
, "value": "{{suricata.eve.event_type}}"
}
}

, {"convert":
Copy link
Member

Choose a reason for hiding this comment

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

Do we need type conversion? Otherwise we could just use the rename processor: https://www.elastic.co/guide/en/elasticsearch/reference/current/rename-processor.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I wonder why it was done this way. Did I do it this way initially? (If so, I'm sorry, this was my very first module LOL)

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, it just occurred to me that this was the simplest way to copy the data out (instead of renaming).

We had decided that since Suricata had a JSON format which is already very familiar to people, we would start for now by leaving the full untouched event in place, and only copy out data, instead of renaming the fields.

This is different from most modules, but most modules have field names determined by the grok patterns (us), not determined by the tool creator (and familiar to the tool's community).

(My first coffee just kicked in, haha)

Copy link
Contributor Author

@adriansr adriansr Jan 11, 2019

Choose a reason for hiding this comment

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

Back in the day convert was the only way we found to copy a field and failing silently if the source field doesn't exist.

That, or an ugly gigantic piece of painless.

Original discussion: #8550 (comment)

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.

Agree with Andrew's suggestions.

Note that you can also use url.original to store whatever URL Suricata transmits.

@webmat
Copy link
Contributor

webmat commented Jan 11, 2019

A few additional suggestions

  • http.suricata.eve.http.length should go into either http.response.body.content or http.request.body.content

And btw @ruflin for Suricata, data is copied out, not moved out of the event. So no need for aliases at this time.

@ruflin
Copy link
Member

ruflin commented Jan 14, 2019

@webmat I think also in suricata we should follow the same logic as we do in all other modules, meaning using alias instead of copy. There are quite a few other modules where we have JSON docs and we converted them all to ECS. copy_to increases the data size and I rather have everyone rely on ECS instead of using the "old" fields.

Perhaps later on we can even allow to turn on alias for a single module if the "original" fields are needed, but let's not start to duplicate data.

@adriansr
Copy link
Contributor Author

jenkins, test this.

@adriansr
Copy link
Contributor Author

@webmat
I've followed your suggestion of using suricata.eve.http.length. After looking at suricata's source I found out that it is only populated with the response body length so I copied it to http.response.body.bytes.

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.

Left a few comments for things to fix, or code blocks to cut out.

One of the big Painless blocks could have been done with grok, I think (unless I'm missing something?). The other one for network stats is bugging me a bit, but it seems to work fine, so ignore my whining there ;-)

x-pack/filebeat/module/suricata/eve/ingest/pipeline.json Outdated Show resolved Hide resolved
x-pack/filebeat/module/suricata/eve/ingest/pipeline.json Outdated Show resolved Hide resolved
x-pack/filebeat/module/suricata/eve/ingest/pipeline.json Outdated Show resolved Hide resolved
}
, { "script":
{ "lang": "painless"
, "source": "long g(def map, def key) { if(map!=null && map[key]!=null) { return map[key]; } return 0; } def n=ctx['network']; if (n==null){n=new HashMap(); ctx['network']=n;}def s=ctx['source'], d=ctx['destination']; def sp=g(s,'packets'), sb=g(s,'bytes'), dp=g(d,'packets'), db=g(d,'bytes'); if(sb+db>0)n['bytes']=sb+db; if(sp+dp>0)n['packets']=sp+dp;"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of this much compact Painless. If only we could do indentation here, or use saved scripts :-)

I'm surprised that this code didn't create an empty network block for the stats event. It doesn't look like this code guards against running when there's no bytes or packets involved.

In other words, this should fine after all, despite my Spidey sense tingling :-)

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 surprised that this code didn't create an empty network block"

Me too. I rewrote it a bit.

x-pack/filebeat/module/suricata/eve/ingest/pipeline.json Outdated Show resolved Hide resolved
@webmat
Copy link
Contributor

webmat commented Jan 14, 2019

@ruflin I'd be curious to know which tools have JSON logs this way, that we are translating instead of copying out. Note that I'm not counting the tools from the Elastic Stack, since for those, the logs are bound to move to ECS eventually at the source (so I have no problem butchering those into shape ;-) )

The reason why we decided to translate Suricata this way is that it has quite a rich community of users and tools, used to the EVE log format. So having 30 to 50% of the fields moved out of the original event would look weird. So we decided to only copy out the data, and keep the EVE event untouched.

But I'm on the fence. I wouldn't make it a requirement for this PR, but eventually we could do straight renames, or introduce an option to get rid of the original fields that have been translated.

I'd like to get some user feedback about this. @MikePaquette Are you aware of anyone who's been waiting for this module, who'd be willing to give us feedback about this?

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@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.

For users that like the old mapping, we can have migration aliases or perhaps a similar feature in the future. But we should not duplicate data because of ECS. If we keep all the old fields around we don't push users in the direction of ECS and to learn ECS which decreases the overall benefit.

For now there are mainly the stack logs that are in json format, but I expect more to come. And this here would set a precedent I prefer not to have.

If we think the fields are really that important, we could introduce them as constant alias but that is something we can still add later if needed as it is not breaking.

@ruflin
Copy link
Member

ruflin commented Jan 15, 2019

@andrewkroh @tsg @MikePaquette @webmat Could you chime in here? My suggestion is to follow with the suricata module the same migration path as we do for all the other modules and have migration aliases.

@tsg
Copy link
Contributor

tsg commented Jan 15, 2019

From my understanding of the discussion, the Suricata module already did copying, and it's released in 6.5, so we are already on that path.

Note that this is not so much about ECS migration, but more that we consider that we might want to keep the duplication long term. The reason is to make the life of the Suricata users a bit easier (they might be confused that the suricata category is called event.action and that the suricata action is called event.outcome, etc). We could also consider using aliases, in this case, I'm not sure.

Another thought that I had was that in some cases we might want to do some data manipulation when copying, say, from suricata.category to event.action. For example, Attempted Denial of Service could become attempted-denial-of-service or just denial-of-service to better match our ECS conversions.

I don't feel strongly about it, but I'd favour copying in this particular case.

@ruflin
Copy link
Member

ruflin commented Jan 15, 2019

@tsg Thanks for chiming in. If the team wants to really keep the original suricata names I would suggest we then use alias for it (without the migration: true flag). When the data is different, we can copy it but I hope that will be the exception.

To make sure we talk about the same version: My assumption is the above should happen for 7.0 and not be backported.

@andrewkroh
Copy link
Member

For new users (users not using Filebeat+Suricata in 6.x) I don't really see much benefit to adding aliases from a usability perspective. They don't show up in Kibana so they're only useful to someone that knows to search for suricata.eve.+original_field_name.

So that leaves the decision as whether or not we should duplicate fields or remove the original Suricata field when something is copied. Users familiar with the Suricata fields will find is useful if we keep the original Suricata fields. So I'd keep the fields the around.

Some users (like me) that are more familiar with meaning behind ECS fields might like an option to drop the EVE fields that have been coped elsewhere.

@ruflin
Copy link
Member

ruflin commented Jan 16, 2019

I'm introducing alias support in Kibana / Index patterns here: #10075 This should solve the issue around not seeing the fields in Kibana. If we have that, are you ok with just having the aliases?

@tsg
Copy link
Contributor

tsg commented Jan 16, 2019

Interesting, I'd say if that works well, we're good with aliases. Is the Kibana team on board with having index pattern entries for aliases? Just thinking if there will be any unexpected side effects.

@ruflin
Copy link
Member

ruflin commented Jan 16, 2019

If the index pattern is generated automatically by Kibana, it's what Kibana generates. I did some testing around it and there is no issue with it.

In the future I hope Kibana adds support for an alias type in the index pattern as it would allow a bit more magic: elastic/kibana#28570

@tsg
Copy link
Contributor

tsg commented Jan 16, 2019

Thanks for the details @ruflin. Good from my side. Any concerns @andrewkroh?

@adriansr
Copy link
Contributor Author

jenkins, test this

This assumes client=source server=destination.

Populates
- source.{packets|bytes}
- destination.{packets|bytes}
- network.{packets|bytes}
Replace ugly painless code.
Lowercase processor can have a target field so its not neccesary to copy
the field in a previous step.
@adriansr adriansr merged commit f64a6a2 into elastic:master Jan 22, 2019
adriansr added a commit to adriansr/beats that referenced this pull request Feb 4, 2019
* Populate more ECS fields in the Suricata module

A few more ECS fields are populated by the ingest pipeline that enriches
Suricata's eve.json events.

Additions:

- http.request.referrer (from suricata.eve.http.http_refer)

- event.action (from suricata.eve.alert.category)
  describes the action that caused the event.
  Examples: "Attempted Denial of Service", "Successful Administrator Privilege Gain"

- event.outcome (from suricata.eve.alert.action)
  Possible values: "allowed", "blocked"

- event.severity (from suricata.eve.alert.severity)
  Possible values: 1, 2 or 3.

- network.transport (from suricata.eve.proto)
  Examples: "tcp", "udp", "ipv6-icmp"

* Use message for suricata.eve.alert.category

Instead of event.action, which is expected to have a fixed set of
enumeration values.

* Populate destination.domain

When http.hostname is present.

* Populate event.{start,end,duration}

* populate network.protocol

* url.hostname is url.domain

* Populate url.path, url.fragment, url.query

From http.url

* Lowercase http request method

* Source/Destination and aggregated counters

This assumes client=source server=destination.

Populates
- source.{packets|bytes}
- destination.{packets|bytes}
- network.{packets|bytes}

* Updated golden files

* Populate ECS field `http.response.body.bytes`

* Use grok pattern to parse url fields

Replace ugly painless code.

* Avoid pairs of convert/lowercase

Lowercase processor can have a target field so its not neccesary to copy
the field in a previous step.

* Cleanup painless script

* Fix golden data

* Fix golden data (2)

* Copy timestamp to event.end instead of parsing date again

(cherry picked from commit 184149f4a18b4162b0d6c89adba3bb924a2db0b8)
@adriansr adriansr added v6.7.0 and removed needs_backport PR is waiting to be backported to other branches. labels Feb 4, 2019
adriansr added a commit to adriansr/beats that referenced this pull request Feb 5, 2019
adriansr added a commit that referenced this pull request Feb 5, 2019
#10006 was merged without a CHANGELOG entry.
adriansr added a commit that referenced this pull request Feb 5, 2019
…odule (#10537)

Cherry-pick of PR #10006 to 6.x branch. Original message: 

A few more ECS fields are populated by the ingest pipeline that enriches Suricata's eve.json events.

Additions:

- http.request.referrer (from suricata.eve.http.http_refer)

- event.action (from suricata.eve.alert.category)
  describes the action that caused the event.
  Examples: "Attempted Denial of Service", "Successful Administrator Privilege Gain"

- event.outcome (from suricata.eve.alert.action)
  Possible values: "allowed", "blocked"

- event.severity (from suricata.eve.alert.severity)
  Possible values: 1, 2 or 3.

- network.transport (from suricata.eve.proto)
  Examples: "tcp", "udp", "ipv6-icmp"

(cherry picked from commit 184149f4a18b4162b0d6c89adba3bb924a2db0b8)
adriansr added a commit to adriansr/beats that referenced this pull request Feb 5, 2019
adriansr added a commit to adriansr/beats that referenced this pull request Feb 5, 2019
Previous PR (elastic#10565) put this entry in the wrong section.
It's an added feature, not a breaking change.
adriansr added a commit that referenced this pull request Feb 5, 2019
Previous PR (#10565) put this entry in the wrong section.
It's an added feature, not a breaking change.
adriansr added a commit to adriansr/beats that referenced this pull request Feb 5, 2019
Previous PR (elastic#10565) put this entry in the wrong section.
It's an added feature, not a breaking change.

(cherry picked from commit 220d053)
adriansr added a commit that referenced this pull request Feb 6, 2019
Previous PR (#10565) put this entry in the wrong section.
It's an added feature, not a breaking change.

(cherry picked from commit 220d053)
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.

7 participants