-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Feature: TLS support for TCP input #7023
Conversation
// during handshake being limited, verify certificates in | ||
// postVerifyTLSConnection | ||
// VerifyCertificate | ||
VerifyFull = tlscommon.VerifyFull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported const VerifyFull should have comment (or a comment on this block) or be unexported
TLSVersion10 TLSVersion = tls.VersionTLS10 | ||
TLSVersion11 TLSVersion = tls.VersionTLS11 | ||
TLSVersion12 TLSVersion = tls.VersionTLS12 | ||
TLSVersionSSL30 = tlscommon.TLSVersionSSL30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported const TLSVersionSSL30 should have comment (or a comment on this block) or be unexported
libbeat/outputs/transport/tls.go
Outdated
|
||
type TLSVersion uint16 | ||
type TLSConfig = tlscommon.TLSConfig | ||
type TLSVersion = tlscommon.TLSVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported type TLSVersion should have comment or be unexported
libbeat/outputs/transport/tls.go
Outdated
} | ||
|
||
type TLSVersion uint16 | ||
type TLSConfig = tlscommon.TLSConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exported type TLSConfig should have comment or be unexported
thanks hound I will fix them :) |
…puts and support server options. When working on the TLS tcp it was a bit strange to actually import a package coming from the outputs, this commit addresses a few things: - Move the `outputs/tls.go` and `transport/tls.go` into the tlscommon under the transport folder. - Add shims to make sure we keep backward compatibility on anything that could be using theses classes. - Extract common logic code to be reusable. - Add inverse mapper for TLSVersion and tlsCiphersuite, to give a uint and get the human string. - Add a new `ServerConfig` config struct used for any tcl class and the appropriate helper to get a new `tls.Config`. *This is a light refactoring, mostly moving code and adding a few tests. Fixes: elastic#6079
We can now receive events on the TCP input with a TLS connection, the input uses existing type to make sure we have the same naming convention and code used by outputs that support TLS communication (Elasticsearch and Logstash). The configuration will look like this: ``` host: "localhost:9000" ssl.enabled: true ssl.verification_mode: full # default ssl.supported_protocols: [TLSv1.1] ssl.cipher_suites: [] ssl.certificate_authorities: ["/etc/cacert"] ssl.certificate: /etc/mycert.crt ssl.key: /etc/mycert.key ssl.client_authentification: required ``` One added configuration is `client_authentification`, this option is only used in the context of server and define how we will force the authentification, it support the three following options: - `required`: Assume that the client will provide a certificate and we will verify it. (default) - `optional`: If a certificate is given by the client. - `none`: We don't validate client certificate. *Note: This commit contains a script to generate certs from a self signed CA. Fixes: elastic#6873
7b6b3a5
to
8b16191
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I normally prefer one PR per commit is that CI would actually test if everything is green for each commit. Like this is only checks the summary of all commits AFAIK.
@@ -0,0 +1,86 @@ | |||
package tlscommon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you named it tlscommon
instead of tls
because of the name conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, I am open to better name.
@@ -215,7 +215,7 @@ Contains common fields available in all event types. | |||
required: True | |||
|
|||
The event log API type used to read the record. The possible values are "wineventlog" for the Windows Event Log API or "eventlogging" for the Event Logging API. | |||
The Event Logging API was designed for Windows Server 2003 or Windows 2000 operating systems. In Windows Vista, the event logging infrastructure was redesigned. On Windows Vista or later operating systems, the Windows Event Log API is used. Winlogbeat automatically detects which API to use for reading event logs. | |||
The Event Logging API was designed for Windows Server 2003, Windows XP, or Windows 2000 operating systems. In Windows Vista, the event logging infrastructure was redesigned. On Windows Vista or later operating systems, the Windows Event Log API is used. Winlogbeat automatically detects which API to use for reading event logs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how that got in again as I think @dedemorton removed it recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed I did: 33b4f9a#diff-5ef49766a1ba7710815be5733d346e9e
@dedemorton @ruflin I will rebase it, maybe I was missing a commits or something, I do not remember having a merge conflict at all. |
@ruflin concerning the multiple commits I was reading your mind and I will make multiple PRs for it. It just easier to develop it as a whole thing. |
@ph Fully agree that development is much easier tackling the full thing at once. |
This PR is composed of multiple commits that touch multiple parts of beats that were required, each commits message should be explicit.
We can now receive events on the TCP input with a TLS connection, the
input uses existing type to make sure we have the same naming convention
and code used by outputs that support TLS communication (Elasticsearch
and Logstash).
The configuration will look like this:
One added configuration is
client_authentification
, this option isonly used in the context of server and define how we will force the
authentification, it support the three following options:
required
: Assume that the client will provide a certificate and wewill verify it. (default)
optional
: If a certificate is given by the client.none
: We don't validate client certificate.*Note: This commit contains a script to generate certs from a self
signed CA.
Note to reviewers
Please use rebase not squash, each commit make sure beats was stable at that point.