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

Populate tls.Config.NextProtos for teleport specific ALPNs #43473

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

rosstimothy
Copy link
Contributor

@rosstimothy rosstimothy commented Jun 25, 2024

Any alpnproxy.HandlerDesc that forwards TLS or provides its own tls.Config is responsible for setting the tls.Config.NextProtos so that the negotiated ALPN is returned to clients. In all other cases the proxy web tls.Config is used which populates the next protos with all supported ALPN protocols. All of the existing places that the default tls.Config was being overridden were supplying the NextProtos. This updates them to populate the NextProtos with the correct values and updates the commentary on alpnproxy.HandlerDecs to indicate that NextProtos should be set if not using the default tls.Config.

Fixes #41500.

Changelog: Ensure the negotiated ALPN is returned correctly for connections to Teleport.

Copy link
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

Can we add a check for ConnectionState().NegotiatedProtocol in tests?

lib/service/service.go Outdated Show resolved Hide resolved
@rosstimothy
Copy link
Contributor Author

Can we add a check for ConnectionState().NegotiatedProtocol in tests?

@espadolini added a test in f605f8c that validates the negotiated protocols.

@rosstimothy rosstimothy force-pushed the tross/next_protos branch 2 times, most recently from 8105b09 to b1b4ed0 Compare June 26, 2024 15:17
@nklaassen
Copy link
Contributor

What is the problem with not returning the custom protocol? Just trying to understand if the way we use teleport-proxy-grpc is problematic too, it relies on the client requesting the protocol for routing, but IIRC we actually have to negotiate to h2 for grpc to work

@espadolini
Copy link
Contributor

espadolini commented Jun 26, 2024

What is the problem with not returning the custom protocol?

It breaks the ALPN contract; the entire point of ALPN is that the client presents a list of protocols and the server chooses one that's supported and makes the client aware, without adding roundtrips to the communication.

If we're choosing to do teleport-proxy-grpc we should select that and have both sides aware of it. If that wouldn't work with clients in the wild today then we have to rethink the approach a bit I guess. Maybe a separate teleport-proxy-grpc-but-this-time-we-actually-read-the-rfc nextproto that both servers and clients can ask and use?

@nklaassen
Copy link
Contributor

What is the problem with not returning the custom protocol?

It breaks the ALPN contract; the entire point of ALPN is that the client presents a list of protocols and the server chooses one that's supported and makes the client aware, without adding roundtrips to the communication.

If we're choosing to do teleport-proxy-grpc we should select that and have both sides aware of it. If that wouldn't work with clients in the wild today then we have to rethink the approach a bit I guess. Maybe a separate teleport-proxy-grpc-but-this-time-we-actually-read-the-rfc nextproto that both servers and clients can ask and use?

I meant like, is there an actual problem/bug/situation where this breaks, that we need to fix or else things aren't going to work?

Not sure I'd go so far as to say it breaks the ALPN contract for the client to advertise support for [teleport-proxy-grpc, h2], the server selects h2, and the actual protocol used between the two is h2. teleport-proxy-grpc is not a real protocol that's supported by any server, so I guess our client is lying a bit, but it is a very useful way to pass that routing information

@espadolini
Copy link
Contributor

I meant like, is there an actual problem/bug/situation where this breaks, that we need to fix or else things aren't going to work?

The unfortunate situation of our ALPN misuse results in the inability to understand if what's negotiated is actually the thing that we intended to reach or, for example, it's just a TLS terminator in front of the proxy, which means that anything involving a connection to the proxy needs either stored state (which might be out of date), prior knowledge of the cleanliness of the connection (which might be wrong), or a second roundtrip (which is slow). If we actually used the protocol as it's specced to do, we could've avoided the tls_routing_conn_upgrade_required thing, as any connection to the proxy public address with the appropriate ALPN could've just transparently used the connection upgrade fallback.

This also makes every new protocol incredibly flaky, as it's hard to actually know if the proxy understands the protocol we intended to use - unless one picks just the one protocol, and on failure a new connection needs to happen (which is half a tls handshake and a full tcp and tls handshake slower).

@rosstimothy rosstimothy removed the request for review from ibeckermayer July 8, 2024 19:18
Any alpnproxy.HandlerDesc that forwards TLS or provides its own
tls.Config is responsible for setting the tls.Config.NextProtos so
that the negotiated ALPN is returned to clients. In all other cases
the proxy web tls.Config is used which populates the next protos
with _all_ supported ALPN protocols. All of the existing places
that the default tls.Config was being overridden were supplying
the NextProtos. This updates them to populate the NextProtos with
the correct values and updates the commentary on alpnproxy.HandlerDecs
to indicate that NextProtos should be set if not using the default
tls.Config.

Fixes #41500.
@rosstimothy rosstimothy added this pull request to the merge queue Aug 13, 2024
Merged via the queue into master with commit 8d12347 Aug 13, 2024
36 checks passed
@rosstimothy rosstimothy deleted the tross/next_protos branch August 13, 2024 20:01
@public-teleport-github-review-bot

@rosstimothy See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Failed
branch/v16 Failed

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.

Most Teleport-specific ALPN protocols don't confirm the selected ALPN protocol
4 participants