GNOME Bugzilla – Bug 637257
g_tls_client_connection_gnutls_retrieve_function needs to be able to block
Last modified: 2013-10-28 08:51:28 UTC
I've been looking further over the GTlsConnection code, and I think I've bumped into a small problem: When an application uses client certificates, the application does not know which key to use until after the handshake has partially completed and we know the list of requested CA's received from the server. This is what g_tls_client_connection_gnutls_retrieve_function and gnutls_certificate_client_set_retrieve_function is about. So somehow the application code (or GTlsCertificateDb) needs to be able to select a key based on what comes from the server. And secondly the selection may involve prompting, and so we need to block returning from g_tls_client_connection_gnutls_retrieve_function. So g_tls_client_connection_gnutls_retrieve_function (and the handshake) may need to run in another thread.
Right, originally there was a signal emitted at this point (GTlsCertificate::need-certificate), but I removed it, because I didn't like any of the possibilities for dealing with this problem. The theory now (as explained by the docs) is that you have to call g_tls_connection_set_certificate() *before* handshaking, and if you don't, then you're saying you don't want to use a client-side certificate (and if the server requires one, then the connection will fail with %G_TLS_ERROR_CERTIFICATE_REQUIRED). So rather than: connect, start handshaking server requests certificate client figures out what certificate to use client sends certificate finish handshaking we do: connect, start handshaking server requests certificate client DOESN'T send certificate server rejects handshake disconnect client figures out what certificate to use connect, start handshaking server requests certificate client sends certificate finish handshaking and so no action within the handshake needs to be blocking/async, because the blocking/async part happens outside of the handshake Of course it's expected that the app would remember (at least for the duration of the session) that the site requires a certificate, and would set it automatically on all future connections. does this make sense? It's a little bit inefficient, but it seems to make the API/implementation much simpler
So almost every application using a certificate database for client auth would use this two connection model? Even when we can automatically select the certificate based on accepted-cas? It seems like we would punish the use of client certificates. Imagine an IMAP client opening 5 parallel TLS connections, now it's actually doing 10 TLS handshakes.
(In reply to comment #2) > It seems like we would punish the use of client certificates. Imagine an IMAP > client opening 5 parallel TLS connections, now it's actually doing 10 TLS > handshakes. No, you only fail if you don't know that the server is going to ask for a certificate. Once you know (either because you got the error once, or because the IMAP account configuration explicitly says so) then you just call g_tls_connection_set_certificate() on every new connection to that server *before handshaking*, and then it will already have the certificate ready to use at the point when gnutls asks for it. (Note: I'm willing to switch back to the old way if you think this is a problem; that's why I didn't mark this NOTABUG/WONTFIX before.)
Is there any plan for this to be fixed? If client certificate selection can be done inside handshake function (through GTlsInteraction), then libsoup does not need to do lookup_client_certificate or destory and re-create connection.
It is wrong to assume that you will get exactly the same parameters in the Certificate Request from one connection to the next. Especially if you are connecting to a "server" which is in fact a round-robin pool, it's entirely unsurprising if the servers are out of sync. And I've seen first-hand a situation where the admins actually change the *type* of certificate used for authentication, because they initially issued certs with a three-year lifetime, then decided they wanted it to be only three months, and didn't want the overhead of managing lots of revocations so they just issued new certs in parallel and then changed the servers to request the new cert types... one server at a time. So in the Certificate Request during the SSL handshake you may get *different* parameters each time. Your flow in comment #1 becomes connect (#1), start handshaking server #1 requests certificate client DOESN'T send certificate server #1 rejects handshake disconnect client figures out what certificate to use for connection #1 connect (#2), start handshaking server #2 requests certificate client sends certificate that was valid for connection #1 server #2 rejects wrong certificate Please don't do this. Make the callback *during* the connection.
Yes. I completely agree. This (and the blocking bug) is on my list of things to get to. But if either of you want to take a whack at it, I'd be happy to work with you on it.
BTW, after thinking about this more, asking for client certificates should probably implemented as a set of methods on GTlsInteraction.
Created attachment 230131 [details] [review] Add a request_certificate virtual method to GTlsInteraction This allows GTlsConnection implementations to request a certificate from the user.
Created attachment 230185 [details] [review] gnutls: Add support for requesting certificate via GTlsInteraction When gnutls asks us for to provide a certificate, on the client we ask our GTlsInteraction (if available) to provide that certificate. This allows us to provide a certificate during the handshake, rather than completing the handshake and forcing the caller to connect again this time with a certificate.
Comment on attachment 230131 [details] [review] Add a request_certificate virtual method to GTlsInteraction I'm assuming this is mostly just a copy+paste of the existing password stuff, and so is mostly known to work... >+ * g_tls_interaction_request_certificate: >+ * Derived subclasses usually implement a certificate selector, although they may >+ * also choose to provide a password from elsewhere. s/password/certificate/ >+ * The selected certificate will be returned. not true. (Likewise below when it talks about returning %NULL) >+ * Since: 2.34 2.36 now >+ * g_tls_interaction_request_certificate_async: >+ * Run asynchronous interaction to ask the user for a certificate to use with >+ * the connection. In general, g_tls_interaction_invoke_ask_password() should >+ * be used instead of this function. s/password/certificate/ >+GTlsInteractionResult g_tls_interaction_invoke_request_certificate (GTlsInteraction each of the new functions needs GLIB_AVAILABLE_IN_2_36 before it in the .h file
Comment on attachment 230185 [details] [review] gnutls: Add support for requesting certificate via GTlsInteraction >+ g_debug ("request certificate returned error: %s", error->message); remove this before committing >+ /* Propagate the error to the handshake */ >+ if (self->priv->reading && !self->priv->handshake_error) >+ g_propagate_error (&self->priv->handshake_error, error); I think it would be better to just return the error normally up the call chain and let the handshake code deal with putting it into priv->handshake_error. (Avoids any potential locking issues, etc.)
Created attachment 230244 [details] [review] Add a request_certificate virtual method to GTlsInteraction This allows GTlsConnection implementations to request a certificate from the user.
(In reply to comment #11) > (From update of attachment 230185 [details] [review]) > >+ /* Propagate the error to the handshake */ > >+ if (self->priv->reading && !self->priv->handshake_error) > >+ g_propagate_error (&self->priv->handshake_error, error); > > I think it would be better to just return the error normally up the call chain > and let the handshake code deal with putting it into priv->handshake_error. > (Avoids any potential locking issues, etc.) Unfortnutately this isn't possible because g_tls_client_connection_gnutls_retrieve_function() is in the call chain. Or am I misunderstanding?
Created attachment 230246 [details] [review] gnutls: Add support for requesting certificate via GTlsInteraction When gnutls asks us for to provide a certificate, on the client we ask our GTlsInteraction (if available) to provide that certificate. This allows us to provide a certificate during the handshake, rather than completing the handshake and forcing the caller to connect again this time with a certificate.
(In reply to comment #13) > (In reply to comment #11) > > (From update of attachment 230185 [details] [review] [details]) > > >+ /* Propagate the error to the handshake */ > > >+ if (self->priv->reading && !self->priv->handshake_error) > > >+ g_propagate_error (&self->priv->handshake_error, error); > > > > I think it would be better to just return the error normally up the call chain > > and let the handshake code deal with putting it into priv->handshake_error. > > (Avoids any potential locking issues, etc.) > > Unfortnutately this isn't possible because > g_tls_client_connection_gnutls_retrieve_function() is in the call chain. Or am > I misunderstanding? Ah, right. OK, in that case, it ought to use read_error (matching the earlier use of read_cancellable), which will get properly propagated to handshake_error later on if appropriate. Also, I don't think self->priv->reading could be FALSE there, since it will always be TRUE during a handshake, and this can only be called during a handshake. If you want any check, I'd say to return FALSE if !handshaking at the beginning, before even invoking the interation.
Comment on attachment 230244 [details] [review] Add a request_certificate virtual method to GTlsInteraction missing docs for g_tls_interaction_invoke_request_certificate()
Created attachment 237266 [details] [review] Add a request_certificate virtual method to GTlsInteraction This allows GTlsConnection implementations to request a certificate from the user.
Comment on attachment 237266 [details] [review] Add a request_certificate virtual method to GTlsInteraction I just realized there was an unreviewed patch here... >+ * Since: 2.36 2.38 now, and likewise with the GLIB_AVAILABLE_IN's also, docs for g_tls_interaction_invoke_request_certificate() are still missing. besides that, I think we can commit this?
Comment on attachment 230246 [details] [review] gnutls: Add support for requesting certificate via GTlsInteraction probably doesn't completely apply any more since I just ported everything to the preferred gnutls 3 APIs. but shouldn't be hard to fix. other than that, just the thing i said in comment 11
Created attachment 256812 [details] [review] Add a request_certificate virtual method to GTlsInteraction This allows GTlsConnection implementations to request a certificate from the user.
Created attachment 256813 [details] [review] fixup! Add a request_certificate virtual method to GTlsInteraction add missing docs, update versions
Created attachment 256814 [details] [review] gnutls: Add support for requesting certificate via GTlsInteraction When gnutls asks us for to provide a certificate, on the client we ask our GTlsInteraction (if available) to provide that certificate. This allows us to provide a certificate during the handshake, rather than completing the handshake and forcing the caller to connect again this time with a certificate.
Created attachment 256815 [details] [review] fixup! gnutls: Add support for requesting certificate via GTlsInteraction Putting the error directly into handshake_error broke things because the code assumes that handshake_error won't be set until after finish_handshake() happens. So I reorganized that. Also updated the new tests for the changes to support installed tests
So, is this ready to go?
Tested, and looked over the patches. Looks good to me.
OK, was about to land it but then noticed the: >+ * @unused_flags: FIXME Do you really think we need the flags? If so, it should be a flags type, not an int
Yes, I think we need to keep them there. I think we'll need tweaks to the virtual functions in the future, and since this is public API, need at least some flags to help with that. Are you squashing the patches before pushing? Do you already have those done? If you'd like me to make the change, can you post your squashed changes, and I'll tweak that? Otherwise I'm happy for you to add the empty flags enum.
I've pushed to wip/danw/clicert (in both glib and glib-networking), with the patches not yet squashed, and with GTlsCertificateRequestFlags added. If you're happy with it, squash the fixups and push it to master.
Attachment 256812 [details] pushed as 65af7c4 - Add a request_certificate virtual method to GTlsInteraction
Attachment 256814 [details] pushed as b3e2c99 - gnutls: Add support for requesting certificate via GTlsInteraction