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?

[ksmurchison]

Yeah. Are you thinking something like this:

diff --git a/imap/httpd.c b/imap/httpd.c
index 1f7fe7478..2caf8a341 100644
--- a/imap/httpd.c
+++ b/imap/httpd.c
@@ -1045,15 +1045,23 @@ int service_main(int argc __attribute__((unused)),
 
     /* we were connected on https port so we should do
        TLS negotiation immediately */
+    int do_h2 = 0;
     if (https == 1) {
         starttls(&http_conn, 180 /* timeout */);
+
+        /* Check negotiated protocol */
+        char *alpn = tls_get_alpn_protocol(http_conn.tls_ctx);
+        do_h2 = !strcmpsafe(alpn, "h2");
+        free(alpn);
     }
-    else if (http2_preface(&http_conn)) {
+    else {
         /* HTTP/2 client connection preface */
-        if (http2_start_session(NULL, &http_conn) != 0)
-            fatal("Failed initializing HTTP/2 session", EX_TEMPFAIL);
+        do_h2 = http2_preface(&http_conn);
     }
 
+    if (do_h2 && http2_start_session(NULL, &http_conn) != 0)
+        fatal("Failed initializing HTTP/2 session", EX_TEMPFAIL);
+
     /* Setup the signal handler for keepalive heartbeat */
     httpd_keepalive = config_getduration(IMAPOPT_HTTPKEEPALIVE, 's');
     if (httpd_keepalive < 0) httpd_keepalive = 0;
@@ -1248,7 +1256,7 @@ EXPORTED void fatal(const char* s, int code)
 
 static unsigned h2_is_available(void *http_conn)
 {
-    return (http2_enabled && http2_start_session(NULL, http_conn) == 0);
+    return http2_enabled;
 }

[ksmurchison]

And invert the loops in tls_alpn_select() as you said.

[elliefm]

Yeah something like that, thanks! Wasn't sure if there was some good reason you weren't already doing something like that which I just wasn't seeing, other than not yet being able to interrogate the ALPN choice after it's been made.

[ksmurchison]

Nope. Probably a combination of laziness and shortsightedness

[elliefm]

Needs #5126 to land first