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.

Sign up for GitHub

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

Jump to bottom

[Filebeat] Add ECS tls fields to zeek and aws modules #15936

Merged
merged 4 commits into from
Feb 3, 2020
Merged

[Filebeat] Add ECS tls fields to zeek and aws modules #15936

merged 4 commits into from
Feb 3, 2020

Conversation

leehinman
Copy link
Contributor

  • zeek/smtp
    • tls.established
  • zeek/rdp
    • tls.established
  • zeek/ssl
    • tls.cipher
    • tls.curve
    • tls.client.issuer
    • tls.established
    • tls.resumed
    • tls.server.issuer
    • tls.version
    • tls.version_protocol
  • aws/elb
    • tls.cipher
    • tls.version
    • tls.version_protocol
  • aws/s3access
    • tls.cipher
    • tls.version
    • tls.version_protocol

Closes #15757

@leehinman leehinman added enhancement Filebeat Filebeat needs_backport PR is waiting to be backported to other branches. Team:SIEM labels Jan 29, 2020
@leehinman leehinman requested a review from a team as a code owner January 29, 2020 17:17
@elasticmachine
Copy link
Collaborator

Pinging @elastic/siem (Team:SIEM)

@leehinman
Add ECS tls fields to zeek and aws modules
895dc36 
- zeek/smtp
  + tls.established
- zeek/rdp
  + tls.established
- zeek/ssl
  + tls.cipher
  + tls.curve
  + tls.client.issuer
  + tls.established
  + tls.resumed
  + tls.server.issuer
  + tls.version
  + tls.version_protocol
- aws/elb
  + tls.cipher
  + tls.version
  + tls.version_protocol
- aws/s3access
  + tls.cipher
  + tls.version
  + tls.version_protocol

Closes elastic#15757
@leehinman leehinman force-pushed the 15757_add_tls_mappings branch from 39dd032 to 895dc36 Compare January 29, 2020 17:18
@leehinman
Copy link
Contributor Author

jenkins, test this

webmat
webmat reviewed Jan 30, 2020
View reviewed changes
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.

Looking great :-)

Nitpick: I would suggest converting booleans from string to boolean datatype.

andrewkroh
andrewkroh approved these changes Jan 30, 2020
View reviewed changes
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. Just the one question on Painless stuff.

x-pack/filebeat/module/aws/elb/ingest/pipeline.yml Outdated
- set:
field: tls.cipher
value: '{{aws.elb.ssl_cipher}}'
if: ctx.aws?.elb.ssl_cipher != null
Copy link
Member

Choose a reason for hiding this comment

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

Does this work safely if the ssl_cipher field does not exist?
Is this the same as if: ctx.aws?.elb?.ssl_cipher != null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does work if ssl_cipher doesn't exist, the http logs are good examples.

That being said it might be safer to use null safe operator on both

This doc would fails with current if: ctx.aws?.elb.ssl_cipher != null

{
  "aws.elb.ip": "10.0.0.1"
}

and this doc succeeds

{
  "aws": {
    "elb": {
      "ip": "10.0.0.1"
    }
  }
}

both work with if: ctx.aws?.elb?.ssl_cipher != null

Copy link
Contributor

Choose a reason for hiding this comment

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

This doc would fails...

This is to be expected, Elasticsearch ingest pipelines don't support dotted keys (other than the dot expander processor). So I'd be surprised if Painless (the if clause) supported it.

@leehinman leehinman merged commit b6baae0 into elastic:master Feb 3, 2020
leehinman added a commit to leehinman/beats that referenced this pull request Feb 3, 2020
@leehinman
[Filebeat] Add ECS tls fields to zeek and aws modules (elastic#15936)
e8abb0a 
* Add ECS tls fields to zeek and aws modules

- zeek/smtp
  + tls.established
- zeek/rdp
  + tls.established
- zeek/ssl
  + tls.cipher
  + tls.curve
  + tls.client.issuer
  + tls.established
  + tls.resumed
  + tls.server.issuer
  + tls.version
  + tls.version_protocol
- aws/elb
  + tls.cipher
  + tls.version
  + tls.version_protocol
- aws/s3access
  + tls.cipher
  + tls.version
  + tls.version_protocol

Closes elastic#15757

(cherry picked from commit b6baae0)
leehinman added a commit that referenced this pull request Feb 5, 2020
@leehinman
[Filebeat] Add ECS tls fields to zeek and aws modules (#15936) (#16043)
064f4ee 
* Add ECS tls fields to zeek and aws modules

- zeek/smtp
  + tls.established
- zeek/rdp
  + tls.established
- zeek/ssl
  + tls.cipher
  + tls.curve
  + tls.client.issuer
  + tls.established
  + tls.resumed
  + tls.server.issuer
  + tls.version
  + tls.version_protocol
- aws/elb
  + tls.cipher
  + tls.version
  + tls.version_protocol
- aws/s3access
  + tls.cipher
  + tls.version
  + tls.version_protocol

Closes #15757

(cherry picked from commit b6baae0)
@leehinman leehinman deleted the 15757_add_tls_mappings branch March 27, 2020 14:56
@andrewkroh andrewkroh removed the needs_backport PR is waiting to be backported to other branches. label May 6, 2020
@andrewkroh
Copy link
Member

Backported in #16043.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webmat webmat webmat left review comments

@andrewkroh andrewkroh andrewkroh approved these changes

Assignees
No one assigned
Labels
enhancement Filebeat Filebeat
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Filebeat] Add ECS TLS fields to existing filesets
4 participants
@leehinman @elasticmachine @andrewkroh @webmat