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

[Packetbeat] Output ECS-compatible TLS fields #15497

Merged
merged 10 commits into from
Jan 14, 2020

Conversation

adriansr
Copy link
Contributor

@adriansr adriansr commented Jan 13, 2020

Packetbeat now outputs TLS fields from ECS 1.3+:

  • The additional information not covered by ECS is nested under tls.detailed.
  • Fields already in ECS are removed from detailed to avoid bloat.
  • A new configuration flag tls.include_detailed_fields allows to
    toggle the inclusion of extra fields. It's enabled by default.

Caveats:

  • Originally it would output the top-level certificate in tls.server_certificate and the rest under tls.server_certificate_chain. ECS mandates that tls.server.certificate and tls.server.certificate_chain are mutually exclusive. To avoid confusion, a chain is always generated, even if it consists of a single certificate.
  • Same for tls.client certificates.
  • The behavior of the configuration options tls.send_certificates and tls.include_raw_certificates has slightly changed.

Non-populated TLS ECS fields:

  • tls.curve: Not implemented. Requires parsing the server key exchange.
  • tls.server.ja3s: JA3s is not implemented yet.

@adriansr adriansr added enhancement in progress Pull request is currently in progress. review Packetbeat needs_backport PR is waiting to be backported to other branches. needs_docs ecs labels Jan 13, 2020
@adriansr adriansr requested a review from a team as a code owner January 13, 2020 11:24
libbeat/beat/event.go Outdated Show resolved Hide resolved
Packetbeat now outputs TLS fields from ECS:
- The additional information not covered by ECS is nested under `tls.detailed`.
- Fields already in ECS are removed from detailed to avoid bloat.
- A new configuration flag `tls.include_detailed_information` allows to
  toggle the inclusion of extra fields. It's enabled by default.

Caveats:
- Originally it would output the top-level certificate in `tls.server_certificate`
  and the rest under `tls.server_certificate_chain`. ECS mandates that
  `tls.server.certificate` and `tls.server.certificate_chain` are
  mutually exclusive. To avoid confusion, a chain is always generated,
  even if it consists of a single certificate.
- Same for `tls.client` certificates.

Non-populated TLS ECS fields:
- `tls.curve`: Requires parsing the server key exchange.
- `tls.server.ja3s`: JA3s is not implemented yet.
WindowsError: [Error 206] The filename or extension is too long: 'C:\\Users\\jenkins\\workspace\\elastic+beats+pull-request+multijob-windows\\beat\\packetbeat\\label\\windows\\src\\github.com\\elastic\\beats\\packetbeat\\build\\system-tests\\run\\test_0099_golden_files.Test.test_golden_files_4_test_established_tls_not_certs_but_raw'
@adriansr
Copy link
Contributor Author

Docs build are failing because I added a reference to ECS docs that needs an update in the elastic/docs repo: elastic/docs#1696

@elasticmachine
Copy link
Collaborator

Pinging @elastic/siem (Team:SIEM)

Copy link
Contributor

@dcode dcode left a comment

Choose a reason for hiding this comment

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

I reviewed mostly the asciidoc when I realized I probably should have reviewed the fields.yml. In some places the documentation looks like it references detailed.* fields, but the JSON examples show in the places I would expect the data to appear. Mostly, we should make sure we populate tls.client.* and tls.server.* where available. I also think the tls.client.detailed is a good approach to provide applications specific data that packetbeat can provide.

packetbeat/docs/fields.asciidoc Outdated Show resolved Hide resolved
packetbeat/docs/fields.asciidoc Outdated Show resolved Hide resolved
@@ -9436,7 +9447,7 @@ type: array
The hello extensions provided by the client.


*`tls.client_hello.extensions.server_name_indication`*::
*`tls.detailed.client_hello.extensions.server_name_indication`*::
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be tls.client.server_name

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 is copied to the ECS field already. I just want to keep the extensions array verbatim with all the extensions present in the hello message.

@@ -9516,7 +9527,7 @@ type: keyword
--


*`tls.server_hello.version`*::
*`tls.detailed.server_hello.version`*::
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be parsed into tls.version for the numeric part and tls.version_protocol for the protocol name (e.g. ssl or tls)

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 is already (more or less, the logic is more complicated for TLS 1.3), I just keep the raw versions from the hello message in those fields.

@@ -9526,7 +9537,7 @@ type: keyword

--

*`tls.server_hello.selected_cipher`*::
*`tls.detailed.server_hello.selected_cipher`*::
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be stored in tls.cipher using the IANA names, which Packetbeat already uses, I think. (see https://ciphersuite.info/cs/TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384/). @webmat maybe we should add IANA to the ECS specification so that the field data lines up?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dcode Good idea. I opened this issue to track this elastic/ecs#720

Copy link
Contributor Author

@adriansr adriansr Jan 13, 2020

Choose a reason for hiding this comment

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

Another field I forgot to remove.

I don't think I follow the discussion, though. Afaik ECS docs already use an IANA name as an example (TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256).

packetbeat/docs/fields.asciidoc Outdated Show resolved Hide resolved
packetbeat/docs/fields.asciidoc Outdated Show resolved Hide resolved
packetbeat/docs/fields.asciidoc Outdated Show resolved Hide resolved
@@ -10049,7 +10096,7 @@ type: keyword

--

*`tls.server_certificate_chain`*::
*`tls.detailed.server_certificate_chain`*::
Copy link
Contributor

Choose a reason for hiding this comment

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

how is this different than tls.server.certificate_chain? I see the code referencing both.

@@ -10058,7 +10105,7 @@ type: array

--

*`tls.client_certificate_chain`*::
*`tls.detailed.client_certificate_chain`*::
Copy link
Contributor

Choose a reason for hiding this comment

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

how is this different than tls.client.certificate_chain? I see the code referencing both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an array of objects, same fields as tls.detailed.client.certificate, while the ECS counterpart just stores the base64 PEM.

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 getting this started, @adriansr!

Moving everything that's not ECS under tls.detailed.* will be clearer indeed. But isn't that a breaking change? Is there a mitigating factor? For example, is TLS still in beta in Packetbeat?

using a client certificate.

- name: client_hello
- name: detailed
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving everything that's not ECS under tls.detailed.* will be clearer indeed. But isn't that a breaking change?

@@ -9526,7 +9537,7 @@ type: keyword

--

*`tls.server_hello.selected_cipher`*::
*`tls.detailed.server_hello.selected_cipher`*::
Copy link
Contributor

Choose a reason for hiding this comment

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

@dcode Good idea. I opened this issue to track this elastic/ecs#720

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.

These changes LGTM. I like the new golden tests for Packetbeat. Since some fields were renamed (breaking change) I think we need to add aliases when we merge this into 7.x that point to the new fields (I think this is just the tls.detailed fields that will have aliases pointing to them).

libbeat/beat/event.go Outdated Show resolved Hide resolved
@adriansr adriansr merged commit fd5fd3d into elastic:master Jan 14, 2020
adriansr added a commit to adriansr/beats that referenced this pull request Jan 14, 2020
Packetbeat now outputs TLS fields from ECS 1.3+:

- The additional information not covered by ECS is nested under tls.detailed.
- Fields already in ECS are removed from detailed to avoid bloat.
- A new configuration flag tls.include_detailed_fields allows to
    toggle the inclusion of extra fields. It's enabled by default.

Caveats:
- Originally it would output the top-level certificate in tls.server_certificate 
  and the rest under   tls.server_certificate_chain. ECS mandates that tls.server.certificate
  and tls.server.certificate_chain are mutually exclusive. To avoid confusion, a chain 
  is always generated, even if it consists of a single certificate.
- Same for tls.client certificates.
- The behavior of the configuration options tls.send_certificates and  
   tls.include_raw_certificates has slightly changed.

Non-populated TLS ECS fields:

- tls.curve: Not implemented. Requires parsing the server key exchange.
- tls.server.ja3s: JA3s is not implemented yet.

(cherry picked from commit fd5fd3d)
@adriansr adriansr added v7.6.0 and removed needs_backport PR is waiting to be backported to other branches. labels Jan 14, 2020
adriansr added a commit that referenced this pull request Jan 14, 2020
…lds (#15529)

Packetbeat now outputs TLS fields from ECS 1.3+:

- The additional information not covered by ECS is nested under tls.detailed.
- Fields already in ECS are removed from detailed to avoid bloat.
- A new configuration flag tls.include_detailed_fields allows to
    toggle the inclusion of extra fields. It's enabled by default.

Caveats:
- Originally it would output the top-level certificate in tls.server_certificate 
  and the rest under   tls.server_certificate_chain. ECS mandates that tls.server.certificate
  and tls.server.certificate_chain are mutually exclusive. To avoid confusion, a chain 
  is always generated, even if it consists of a single certificate.
- Same for tls.client certificates.
- The behavior of the configuration options tls.send_certificates and  
   tls.include_raw_certificates has slightly changed.

Non-populated TLS ECS fields:

- tls.curve: Not implemented. Requires parsing the server key exchange.
- tls.server.ja3s: JA3s is not implemented yet.

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

None yet

6 participants