-
Notifications
You must be signed in to change notification settings - Fork 22
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
RFC: implement TLS auto-detection #372
base: main
Are you sure you want to change the base?
Conversation
05738ce
to
ad7b0a4
Compare
19a319d
to
a2bc880
Compare
7f4e008
to
7c9a46a
Compare
case LPAS_SUCCESS: | ||
self->tls_detected = TRUE; | ||
/* this is a TLS handshake! let's switch to TLS */ | ||
if (log_transport_stack_switch(&self->super.transport_stack, LOG_TRANSPORT_TLS)) |
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.
TLS auto-detection sounds like a security issue for me. TLS provides encryption and authentication.
If we want to allow non-TLS and TLS traffic on the same port, we should notify the user with a big warning that neither encryption nor authentication is guaranteed. We could probably do this when peer-verify(off)
is set, but even then we should warn the user that encryption is also optional so vulnerable to "downgrade attack".
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 agree with that in general, however I think the syslog over TLS use-case is slightly different.
- everything is unauthenticated and plain text by default (unfortunately)
- the sensitive data is always on the client side and syslog itself is uni-directional, the server does not respond to anything
- there's no negotiation, the syslog client ultimately determines whether the syslog traffic is using TLS or not.
- the auto detection only applies to the server, not on the client
With those in mind:
-
Downgrade of the client: this is not possible, as the client has TLS enabled or it doesn't. If we had an active attacker, and this would be merged, the attacker could fool the server to accept plain text instead of TLS. But this does not change how the client is configured: the client would require TLS anyway with both encryption and authentication.
-
Downgrade of the server: this is indeed possible, client talking TLS while the server receives plain text. Going back to the first case, if the client has TLS configured and authenticates the server, the attacker would need to have a trusted X.509 certificate. In which case they don't even need to talk to the server.
And some more context:
- with a patch like this, it actually becomes a lot easier to deploy TLS on the clients, no need to reconfigure firewalls, load balancers or change ports on the client. You "just" configure tls() on the server on port 514 with PKI issued certificate and then you can incrementally enable the use of TLS on any of the clients, without any further configuration. Which means that you could end up with TLS encryption of log data, which is a big win.
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 agree that this is a useful feature, I would like to just add some security measures of how we enable such functionality. From the server's perspective, encrypted and non-encrypted, authenticated and non-authenticated data will flow from the same source and the user has very little influence on how to distinguish between the two (same source -> same log paths, same destinations, and we only have a few TLS-related macros to filter for authenticated and/or encrypted data).
My suggestion would be the following:
transport(auto)
should allow everything that does not have security implicationstransport(auto) + optional-tls(yes/no)
should enable TLS auto-detection (I'm open to rename the option to anything else). In the documentation of this option, we should mention how users can filter for TLS-encrypted/authenticated data using macros.
What do you think?
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 agree completely. That's exactly what we discussed in the past couple of days. That's why the patch is in RFC yet.
So, yes, I agree we need a way to specify require-tls(yes). And I think we should also revisit information about a message being encrypted or not once we receive it
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.
@MrAnno I was thinking about the option name and some feedback would be appreciated.
This is how this would work:
- transport(auto) + no TLS block: TLS is explicitly rejected if detected with an error message, so that we don't consume binary data as log messages.
- transport(auto) + tls() block: TLS is required
- transport(auto) + tls(peer-verify(optional-trusted) or peer-verify(optional-untrusted)): we accept plain text, we accept, verification of the client certificate is either trusted or untrusted based on the peer-verify() argument.
peer-verify(yes) also would require TLS.
What do you think?
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 think peer-verify
is about authentication, which is only a subset of what TLS offers. Encryption without peer verification can also be useful, so making encryption optional depending on a peer verification option sounds confusing to me.
My ideas are the following:
transport(auto,tls?) tls()
transport(auto) tls(... optional(yes))
transport(auto+optional-tls) tls()
transport(auto) optional-tls(ca-dir(), ...)
transport(auto) optional-tls(yes) tls()
Signed-off-by: Balazs Scheidler <[email protected]>
…citly If we are to receive an SSL connection attempt with impossible client-side settings, we might end up with an SSL alert instead of a ClienHello. Handle that case with an explicit action, instead of pulling in binary data from the socket. Signed-off-by: Balazs Scheidler <[email protected]>
Signed-off-by: Balazs Scheidler <[email protected]>
7c9a46a
to
3563cae
Compare
Add TLS/haproxy auto-detection in case we have transport(auto)
With this we auto-detect: