client-side ALPN support

[elliefm]

No description provided.

[elliefm]

This needs #5097 to land first

[elliefm]

@ksmurchison Can I get your input on this please? Especially the "WIP various: add client-side ALPN maps for IANA registered protocols" commit. We have a bunch of things that act as http clients, but it's not clear to me which http protocol version(s) they support.

And for http_proxy.c, which looks like it supports 1.0, 1.1, and h2 on the backend -- I think I'll need to implement a check_availability callback for h2 like httpd.c does, but its callback relies on http2_init and http2_start_session, neither of which http_proxy.c ever calls, so I'm not sure.

Also, if you have any suggestions for imtest and imclient, I'm all ears. They both implement their own tls_init_clientengine() and tls_start_clienttls() instead of using the functions from tls.c. It's tempting to rip those out and have them use the tls.c functions, but I don't know if there's some good reason they have their own versions

[ksmurchison]

@ksmurchison Can I get your input on this please? Especially the "WIP various: add client-side ALPN maps for IANA registered protocols" commit. We have a bunch of things that act as http clients, but it's not clear to me which http protocol version(s) they support.

We don't have any client-side HTTP/2 code so any connection that we create will be only HTTP/1.1. I think HTTP/1.1 should be the only ALPN token for our client code/

And for http_proxy.c, which looks like it supports 1.0, 1.1, and h2 on the backend -- I think I'll need to implement a check_availability callback for h2 like httpd.c does, but its callback relies on http2_init and http2_start_session, neither of which http_proxy.c ever calls, so I'm not sure.

See above.

Also, if you have any suggestions for imtest and imclient, I'm all ears. They both implement their own tls_init_clientengine() and tls_start_clienttls() instead of using the functions from tls.c. It's tempting to rip those out and have them use the tls.c functions, but I don't know if there's some good reason they have their own versions

I'd use the stuff in tls.c if it works. Not sure why there are separate implementations.

[elliefm]

@ksmurchison I've added a tls_get_alpn_protocol() function to the TLS API that returns the protocol that was chosen by the negotiation, and extended my tests to make sure that when we negotiate a protocol we do actually make a sensible choice...

rfc7301 says:

It is expected that a server will have a list of protocols that it supports, in preference order, and will only select a protocol if the client supports it. In that case, the server SHOULD select the most highly preferred protocol that it supports and that is also advertised by the client.

From my reading, this is saying that the server chooses in order of its preference. That is, if the client supports ("foo", "bar") and the server supports ("bar", "foo"), the server should choose "bar". Our implementation processes them in client preference order and short-circuits at the first match, so it ends up choosing "foo" even though the server would prefer "bar" and both sides support it. The spec says "SHOULD", so this isn't technically wrong. But was this choice deliberate, and if so why? Or is it just something that's shaken out of how you implemented it? My test for this case currently fails, and I'm not sure whether I should change the implementation or just change the test to match.

Also, I was scratching my head over this in the next paragraph:

The server SHALL NOT respond with a selected protocol and subsequently use a different protocol for application data exchange.

The implication is that accepting a protocol in the TLS negotiation must also configure us to speak that protocol right from the start. At first I couldn't see how this happens for httpd (which is the only server that supports multiple protocols), but I eventually realised that the h2_is_available() callback calls http2_start_session(), and I think this is what sets the connection up for h2 if both parties support it. Is that right?

But I think this logic only works if we examine the client preferences first -- if we simply inverted the pair of loops to choose in the server's preferred order instead of the client's, then h2_is_available would set the connection up for h2 before we know whether the client even supports it. So maybe that's why you made it use the client's preference instead of the server's?

Given there's now a way to query the chosen protocol after the TLS session is established, what if we pull the http2_start_session call out of h2_is_available, and instead choose how to proceed based on tls_get_alpn_protocl() after tls_start_servertls() returns?

[elliefm]

Needs #5126 to land first

[ksmurchison]

After re-reading RFC 2595, I'm wondering if imapd, pop3d, etc should just call shutdown() if STARTTLS fails

[elliefm]

That's an interesting thought, giving that RFC a read now myself

[elliefm]

Hmm, it doesn't really give any clear direction about what to do when the TLS negotiation fails, does it. This paragraph is about as close as it gets:

Both the client and server MUST check the result of the STARTTLS command and subsequent TLS negotiation to see whether acceptable authentication or privacy was achieved. Ignoring this step completely invalidates using TLS for security. The decision about whether acceptable authentication or privacy was achieved is made locally, is implementation-dependent, and is beyond the scope of this document.

I think that's giving us permission to handle it however we like. I would argue that a client which has issued STARTTLS expects to get a secure connection, and may not correctly handle the case where the TLS negotiation fails and the connection remains unencrypted. A naive client might just send their password over the wire at this point. That being the case, the server should drop the connection rather than give them the opportunity to do so.

I'm not sure I'm comfortable bundling such a change into this PR though, though it is highlighting the need for it. Guess I'll add it to the pile.

[ksmurchison]

I can workin that if you like.

[elliefm]

Sure, I actually just wrote up a linear task for it. It's in the triage queue if you wanna grab it.

[ksmurchison]

@elliefm See #5134

[elliefm]

@ksmurchison Ignoring ALPN for a minute, how do we handle clients that only support HTTP/1.0? I'm thinking about if we can extend the do_h2 stuff after the starttls() call to also set up HTTP/1.0 vs HTTP/1.1 based on the ALPN negotiation.

[elliefm]

Looks like that's handled in txn->flags.ver, but that isn't available until cmdloop, hmm.

While I'm poking around, I notice we also call starttls() if (I think) the client requests to upgrade the connection to one using TLS. I guess this is the http equivalent of imap STARTTLS. Perhaps we need to check tls_get_alpn_protocol() there too.

[ksmurchison]

@elliefm With HTTP/1.x, the client determines the protocol used on the request line. If the client uses HTTP/1.0, the server will respond as such. If the client negotiates 1.0 via ALPN and uses 1.1 or vice-versa, I don't see that as a huge problem. The major difference is that HTTP/1.0 has non-persistent connections by default.

As far as upgrading from plaintext to TLS, this code is only used in a Murder. No existing HTTP clients that I'm aware of use the Upgrade mechanism for anything other than bootstrapping websockets. Even upgrading from HTTP/1.1 to h2 via Upgrade has been deprecated. I wouldn't worry about ALPN here.