GNOME Bugzilla – Bug 781578
gnutls: too aggressive session ID caching prevents client cert changing
Last modified: 2017-10-27 01:46:42 UTC
Created attachment 350193 [details] test case I've been chasing a bug in Cockpit [1] that is related to TLS certificate authentication to an Openshift server. Openshift does not enforce validation of client certs during TLS handshake, as it also supports user/password authentication (this is simplified, but sufficient for this bug report), but does use client certs to authorize users. Now, when first using an invalid certificate, Openshift responds with "Unauthorized". But after updating the certificate to a "good" one, Openshift *still* responds "Unauthorized", and only accepts it after *either* restarting the client (Cockpit) *or* the server (Openshift). I found [2] that this is due to the GnuTLS module's session caching. The session ID is built only from the address and port. So when attempting to do a new client connection with a different certificate to the same server, it will hit the session cache and use the *old* credentials/cert for connecting to the server. g_tls_backend_gnutls_remove_session() as well as the entire session ID is purely internal to glib-networking's tls module and not exposed to GIO. It seems we also can't set a session ID externally. Thus the whole session caching is an internal implementation detail, so I don't see a workaround. Things work if I disable session caching: --- a/tls/gnutls/gtlsclientconnection-gnutls.c +++ b/tls/gnutls/gtlsclientconnection-gnutls.c @@ -333,7 +334,7 @@ g_tls_client_connection_gnutls_begin_handshake (GTlsConnectionGnutls *conn) g_bytes_get_data (gnutls->priv->session_data, NULL), g_bytes_get_size (gnutls->priv->session_data)); } - else if (gnutls->priv->session_id) + else if (FALSE && gnutls->priv->session_id) { GBytes *session_data; But of course this is only a proof that this is the problem, not an actual solution. For that I see two possibilities: 1) Make the client cert part of the session ID, e. g. by creating a short hash/checksum and appending it. 32 bits should certainly suffice, one usually doesn't have a gazillion certs. Adding an SHA256 hash will completely be on the safe side. This has the advantage that it is completely generic and keeps this an internal implementation detail. Or 2) Provide an external API to influence the session cache. E. g. a "session-id" property on the connection object, and a method to terminate it. I don't like this much as clients then need to be aware of this problem, and this is by no means specific to Cockpit or Openshift. I extended the /tls/connection/client-auth test to demonstrate this problem (patch attached): After what it currently does, it creates a second connection with a different client cert to the same server, and ensures that it actually uses that new cert. This fails with GLib-Net:ERROR:connection.c:987:test_client_auth_connection: assertion failed: (g_tls_certificate_is_same (peer, cert)) as it still uses the cert from the previous connection (i. e. this bug). When applying the "disable session cache" hack from above, the test passes, but then /tls/connection/client-auth-rehandshake fails as this assumes that the re-handshake does not actually renegotiate the connection: GLib-Net:ERROR:connection.c:908:on_notify_accepted_cas: assertion failed: (*changed == FALSE) But this is of course just a side effect from that blunt hack. Solution 1) from above ought to make both tests happy. Do you have an opinion how this should be fixed cleanly? Thanks in advance! [1] https://github.com/cockpit-project/cockpit/issues/6359 [2] https://github.com/cockpit-project/cockpit/issues/6359#issuecomment-295781763
> 1) Make the client cert part of the session ID Ah, I don't think that will work -- the session ID needs to be created before the client cert is set/known. So perhaps receiving a client cert should remove the session cache entry?
Created attachment 350306 [details] [review] gnutls: Fix using different client certs for different connections > 1) Make the client cert part of the session ID This does work after all, the session ID just needs to (and can) be computed later on. This patch implements the proposed change, by appending the client cert hash to the session ID, if present. This now satisfies all tests, and the original cockpit bug is fixed too.
Review of attachment 350306 [details] [review]: I can't approve this patch since I don't know enough, but the approach looks sane to me and it's great that you added a nice test and even updated create-files.sh. I just wanted to point out... ::: tls/gnutls/gtlsclientconnection-gnutls.c @@ +136,3 @@ + * that different connections to the same server can use different + * certificates. */ + g_object_get (G_OBJECT (gnutls), "certificate", &cert, NULL); ...you forgot to unref the GTlsCertificate you get here.
Created attachment 351512 [details] [review] gnutls: Fix using different client certs for different connections So I did, thanks for spotting! Fixed patch attached.
(In reply to Martin Pitt from comment #1) > > 1) Make the client cert part of the session ID > > Ah, I don't think that will work -- the session ID needs to be created > before the client cert is set/known. So perhaps receiving a client cert > should remove the session cache entry? Hm... I think we have to refuse to cache connections if the server even *requests* a certificate (whether or not the caller provides one). Otherwise, it breaks the case where a client first connects with no cert, then disconnects and reconnects and wants to provide a cert the second time. Right? And is that sufficient? Are there other cases that could break? It had never occurred to me before that the session state included the client certificate, so things might be arbitrarily broken here :-/
Review of attachment 351512 [details] [review]: ::: tls/tests/connection.c @@ +961,3 @@ + g_object_unref (test->client_connection); + + /* Now start a new connection to the same server with a diffent client cert */ Nitpick: ‘different’ @@ +963,3 @@ + /* Now start a new connection to the same server with a diffent client cert */ + client = g_socket_client_new (); + connection = G_IO_STREAM (g_socket_client_connect (client, G_SOCKET_CONNECTABLE (test->address), `connection` is leaked at the end of the block. @@ +971,3 @@ + g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (test->client_connection), + 0); + cert = g_tls_certificate_new_from_file (tls_test_file_path ("client2-and-key.pem"), &error); `cert` is leaked at the end of the block.
(In reply to Dan Winship from comment #5) > Hm... I think we have to refuse to cache connections if the server even > *requests* a certificate (whether or not the caller provides one). > Otherwise, it breaks the case where a client first connects with no cert, > then disconnects and reconnects and wants to provide a cert the second time. > Right? AFAICS this should work as the session ID for both cases is now different, so both connections will be cached independently. I'll try to add a test case for that. > And is that sufficient? Are there other cases that could break? Indeed there might be other things in the session data that cause breakage, but it's not very obvious what goes in there. (In reply to Philip Withnall from comment #6) > Review of attachment 351512 [details] [review] [review]: Thanks for spotting these! Fixed locally, I'll attach an updated patch once I'm done with the above new test case.
Created attachment 352079 [details] [review] gnutls: Fix using different client certs for different connections Updated patch with the new test case and leak/typo fixes from above.
Review of attachment 352079 [details] [review]: Looks good to me. Dan can give final a-c-n on it.
(In reply to Philip Withnall from comment #9) > Review of attachment 352079 [details] [review] [review]: > > Looks good to me. Dan can give final a-c-n on it. Ping, Dan?
I don't see any problems. Fix is elegant and tests look excellent. Thanks. (In reply to Michael Catanzaro from comment #3) > Review of attachment 350306 [details] [review] [review]: > > I can't approve this patch Times change....
Attachment 352079 [details] pushed as 8da92fd - gnutls: Fix using different client certs for different connections
The following fix has been pushed: 35a4038 Properly install all the new test files
Created attachment 361831 [details] [review] Properly install all the new test files
@Michael: Thanks for landing this, and the Makefile.am fix!
Released in 2.54.1. Should make its way to F27 soon, probably as a zero-day update.