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] [SIEM] Fileset for Cisco FTD logs #13286

Merged
merged 16 commits into from Aug 28, 2019

Conversation

adriansr
Copy link
Contributor

@adriansr adriansr commented Aug 19, 2019

This adds a new ftd fileset to the cisco module for parsing Firepower Threat Defense logs.

As the FTD logs are a superset of the Cisco ASA logs, this PR introduces a shared ingest pipeline that is used both by the new ftd and the existing asa filesets.

As a side effect of this, it improves the existing ASA fileset so that it uses custom syslog message decoding instead of relying on Filebeat's syslog input, which has caused compatibility problems with some ASA devices.

Closes #12690

@adriansr adriansr requested a review from a team as a code owner August 19, 2019 15:48
@adriansr adriansr force-pushed the feature_fb_cisco_ftd branch 4 times, most recently from 0f63699 to 5244bf3 Compare August 19, 2019 17:23
@adriansr adriansr requested review from a team and removed request for a team August 19, 2019 17:24
@elasticmachine
Copy link
Collaborator

Pinging @elastic/siem

@adriansr adriansr added enhancement x-pack Issues and pull requests for X-Pack features. labels Aug 19, 2019
@@ -1,8 +1,8 @@
{{ if eq .input "syslog" }}

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 left the input configuration as syslog in order to not break existing installs. Does it makes sense, or it should align with the underlying input name?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's a good decision for existing users and consistent with other modules.

x-pack/filebeat/module/cisco/ftd/_meta/fields.yml Outdated Show resolved Hide resolved
@adriansr adriansr requested a review from a team as a code owner August 23, 2019 11:24
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.

Having reviewed the ECS 1.1 import, I knew which files to skip in this PR, so I started reviewing it already. I haven't had time to finish my review yet, but thought I'd share the two things I've seen so far.

I'll continue my review on Monday :-)

"input.type": "log",
"log.level": "debug",
"log.offset": 0,
"log.original": "ApplicationProtocol: http, Client: webserver, DstIP: 10.8.12.47, SrcIP: 10.1.123.45, Message: Intrusion attempt",
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, there's confusion between log.original and event.original 🤦‍♂

Based on elastic/ecs#127, I think this should go to event.original.

cc @ruflin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed, but will keep log.original for 7.x to avoid introducing a breaking change in the ASA fileset.

Copy link
Contributor

Choose a reason for hiding this comment

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

This module is in Beta, correct? I think it's worth preserving for 7.3.x if it was already out in 7.3, but I wouldn't keep log.original until 8.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that this FTD module builds on top of the ASA module which came out in 7.2. Both FTD and ASA share the same pipeline because FTD logs are just ASA logs with 5 extra message types.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand it came out in a previous version.

But there's often confusion between log.original and event.original. Even I am struggling with the purpose of log.original, as it seems very wasteful to keep two versions of the full original event, one with slight modifications vs the other.

So I'm afraid if we keep log.original for the life of 7.x (roughly another year), it will just help carry the confusion forward.

That's why I was bringing up the fact that since it's in Beta, perhaps we can introduce this breaking change and try to keep alignment on using event.original.

x-pack/filebeat/module/cisco/ftd/_meta/fields.yml Outdated Show resolved Hide resolved
@adriansr adriansr force-pushed the feature_fb_cisco_ftd branch 3 times, most recently from 6c25f50 to 19740f2 Compare August 27, 2019 01:01
@adriansr adriansr requested a review from webmat August 27, 2019 13:26
@adriansr
Copy link
Contributor Author

@webmat can you have another look?

@webmat
Copy link
Contributor

webmat commented Aug 27, 2019

Yes, today. This has been rebased over the ECS 1.1.0 import?

@adriansr
Copy link
Contributor Author

Yes, it's rebased.

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.

Ok, I have a lot of comments. Not 100% need to be addressed, there's a few instances of calling out to ECS contributors.

There's also a few places where I may be wishing for something to be parsed out in more detail, but perhaps it's too deep in the message to be extracted at this time. This is fine.

But I've still noticed many things I think should be modified.

I'm not 100% done with this review. Note to self:

  • asa-ftd-pipeline.yml
  • security-mappings.csv

Speaking of mappings, I only realized the mappings were generated based on the CSV very late in my review. So there's a lot of comments on the mappings scattered around the PR (docs, expected JSON, etc). Sorry :-\

"event.duration": "0",
"event.module": "cisco",
"event.original": "%FTD-1-430003: AccessControlRuleAction: Allow, SrcIP: 10.0.1.20, DstIP: 8.8.8.8, SrcPort: 57379, DstPort: 53, Protocol: udp, IngressInterface: inside, EgressInterface: outside, IngressZone: input-zone, EgressZone: output-zone, ACPolicy: default, AccessControlRuleName: Intrusion-Rule, Prefilter Policy: Default Prefilter Policy, User: No Authentication Required, Client: DNS client, ApplicationProtocol: DNS, ConnectionDuration: 0, InitiatorPackets: 1, ResponderPackets: 1, InitiatorBytes: 93, ResponderBytes: 145, NAPPolicy: Balanced Security and Connectivity, DNSQuery: elastic.co, DNSRecordType: a host address, DNS_TTL: 70",
"event.outcome": "allow",
Copy link
Contributor

Choose a reason for hiding this comment

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

event.outcome is reserved. This is risky, as it may change.

But since the module is still in Beta, I suppose it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's bad. event.outcome is in use by plenty of modules in SIEM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I know. I pushed back on reserving fields that everybody would want to use for that reason. When that didn't fly, I proposed we introduce the equivalent 4 fields for the raw value at the same time, so that people could record these values there, and I got push back on that. 🤷‍♂

I'll open a PR for the raw fields soon after this FF and try to push it through. It won't change anything for this PR and the modules using the reserved fields, but at least there will be a sensible way to resolve this from then on.

At that time we can figure out what we do with all this.

"cisco-ftd"
],
"user.id": "No Authentication Required",
"user.name": "No Authentication Required"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would skip the mapping to user.[id|name] when it's a generic value like this.

Can be fixed later, though.

filebeat/docs/modules/cisco.asciidoc Outdated Show resolved Hide resolved
filebeat/docs/modules/cisco.asciidoc Outdated Show resolved Hide resolved
"dns.response_code": "NOERROR",
"event.action": "connection-started",
"event.code": "430002",
"event.dataset": "cisco.ftd",
Copy link
Contributor

Choose a reason for hiding this comment

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

With so many different types of logs emitted by Cisco FTD, I'm actually wondering if "event.dataset": "cisco.ftd" is granular enough...

Not a blocker for this PR, but @ruflin what would you think about having the Cisco FTD datasets being more granular?

  • "cisco.ftd.dns" for x-pack/filebeat/module/cisco/ftd/test/dns.log
  • "cisco.ftd.management" for x-pack/filebeat/module/cisco/ftd/test/firepower-management.log
  • "cisco.ftd.intrusion" for x-pack/filebeat/module/cisco/ftd/test/intrusion.log
  • "cisco.ftd.connection" for x-pack/filebeat/module/cisco/ftd/test/security-connection.log
  • "cisco.ftd.malware" for x-pack/filebeat/module/cisco/ftd/test/security-file-malware.log

@adriansr
Copy link
Contributor Author

Thanks @webmat! you've found plenty of important issues.

I think I've addressed all your comments

This adds a new `ftd` fileset to the `cisco` module for parsing
Firepower Threat Defense logs.

As the FTD logs are a superset of the Cisco ASA logs, this PR
introduces a shared ingest pipeline that is used both by the new `ftd`
and the existing `asa` filesets.

As a side effect of this, it improves the existing ASA fileset so that
it uses custom syslog message decoding instead of relying on Filebeat's
syslog input, which has caused compatibility problems with some ASA
devices.

Closes elastic#12690
This is a breaking change so it will be included in master but not in
7.4.
Perform this mappings in the CSV instead of a final processor in the
pipeline so that the docs get the right mappings.
It was directly using the value from ConnectionDuration, which is
seconds not nanos.

Fixed and now populates event.{start,end} too.
Append, don't set, to error.message
don't remove _temp_
@MikePaquette
Copy link

Nice work with this review @webmat and @adriansr

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.

Ok, I think the two big outstanding issues are outside of the scope of this PR.

  • I didn't realize the reserved event.outcome was already being populated by multiple modules already. No need to change anything around this, for this PR, this will have to be resolved outside of this PR. discussion
  • I think the confusion between log.original and event.original can be resolved in a fix after FF, if we decide to make this breaking change. Let's not block this PR because of that. The Cisco module is already using log.original before this PR, in any case. discussion

Last point I wonder if we could tweak is making event.dataset more fine grained (discussion). But since in general this is being set by Filebeat directly, perhaps this is out of scope for this PR. I do think a more fine grained event.dataset would be useful for users, though. WDYT?

If that last one's out of scope, all of my other comments have been addressed. Great work, @adriansr!

@@ -1,8 +1,8 @@
{{ if eq .input "syslog" }}

Copy link
Member

Choose a reason for hiding this comment

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

I think that's a good decision for existing users and consistent with other modules.

@adriansr adriansr merged commit 2a6c58f into elastic:master Aug 28, 2019
@adriansr
Copy link
Contributor Author

Oh dear, I forgot to git push before merge.

I will send a new PR with the extra comment to the generator and the section rename in the docs.

@webmat
Copy link
Contributor

webmat commented Aug 28, 2019

Yeah I think #13379 covers everything

adriansr added a commit that referenced this pull request Aug 28, 2019
* Rename docs section to Field Mappings

* Add explanatory comment to the generator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Filebeat Filebeat review x-pack Issues and pull requests for X-Pack features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Filebeat] Module to Cisco Firepower Threat Defense Logs
5 participants