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

Add tls.handshake_match.version matcher #155

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

agittins
Copy link

@agittins agittins commented Nov 4, 2023

  • creates a new matcher, tls.handshake_match.version
  • based heavily on the alpn matcher.

@agittins
Copy link
Author

agittins commented Nov 4, 2023

Fixes #119

Copy link
Owner

@mholt mholt 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 this!

Since the TLS handshake hasn't been completed yet, a version hasn't actually been chosen. This only matches versions the client advertises support for. Is that intentional? If so, we should probably update the godoc to clarify.

Normally I suppose we'd just put this in the tls app package directly, but that's not strictly required and this is the only request I've had for it, so maybe keeping it in this separate module is better for now.

@agittins
Copy link
Author

agittins commented Nov 4, 2023

That's a good point - the idea is to match against versions the client supports (and especially those which the tls app doess not support), but it should probably be more specific - in my case I have clients that only support TLS 1.0, so that's what I was looking for - but yes the more general case is that clients offer a max version or a list.

I think even for my use-case I should be checking for "clients that support a max_version of tls1.0", since I don't want to redirect old browsers etc that support newer versions but still offer support of 1.0.

I might give this a bit more thought but if you have time to offer some insight it would be more than welcome.

@agittins
Copy link
Author

agittins commented Nov 6, 2023

Ah. So two issues I have.

  1. The current PR would match any client that supports down to tls 1.0, as well as higher. I am trying to target old clients, so I should be matching a "max" of tls 1.0. The clients I was observing only advertise tls1.0 (which is I think common for older clients, they treat their adv as their max), but modern clients report a list of supported protos. This would be easy enough to fix by scanning the supported versions and only checking if the max is <= tls1.0. This would mean the match rule is now effectively "client version not higher than" rather than "version".
  2. After testing I think this strategy isn't going to work for me anyway. I've noticed that the actual TLS ClientHello record layer that the client sends is... SSLv2! So I think Caddy is rejecting the connection before we get any further (I haven't checked that myself yet). Inside that is a tlsv1.0 version field(?).

Here's what the legacy client's first post-ACK packet looks like:
image

Is Caddy going to reject this already, or will I need to perhaps find a way to inspect the CHI within the matcher directly, since the tls libs don't have SSLv2 in the list of known versions?

@mholt
Copy link
Owner

mholt commented Nov 9, 2023

Huh, that's super weird. I admit I don't know. Go doesn't support SSLv2; maybe our parser will still decode it, but I'd be surprised if it worked.

@mholt
Copy link
Owner

mholt commented Mar 7, 2024

I wonder if we'll need a slightly more robust version matcher, using a struct instead of a simple []string. It seems like we'll need a min version, max version, or a specific set of versions that the user can specify.

And then we'll probably need a way to switch between comparing the version the client advertises versus the version that is actually negotiated. (Or maybe the latter isn't actually useful, I dunno.)

@legobeat
Copy link

legobeat commented Jul 3, 2024

I wonder if we'll need a slightly more robust version matcher, using a struct instead of a simple []string.

Sounds like a great idea. Different APIs sometimes using subtly different naming schemes is can already be a source of confusion and removing that source of potential misconfiguration seems nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants