Created attachment 348117 [details] [review]
tls-verifier: Handle GNUTLS_CERT_REVOKED

Created attachment 348118 [details] [review]
tls-verifier: Use GIO to verify the chain of TLS certificates

Created attachment 348119 [details] [review]
Remove the GnuTLS dependency

Review of attachment 348117 [details] [review]:

Seems right

Review of attachment 348118 [details] [review]:

::: libempathy/empathy-tls-verifier.c
@@ +205,3 @@
+
+  /* error is set only when there is an actual failure. It won't be
+   * set, if it successfully determined that the ceritificate was not

nitpick: "certificate"

@@ +223,3 @@
+    {
+      data = g_ptr_array_index (cert_data, idx);
+      cert = g_initable_new (tls_certificate_type,

Is there really no convenience API for constructing the certificates?

@@ +252,3 @@
+
+  tls_database = g_tls_backend_get_default_database (tls_backend);
+  identity = g_network_address_new (priv->hostname, 0);

Only having one identity here could be bad. The old code supported multiple reference identities, and I dimly remember that actually mattering for some XMPP servers (I think some of the big corporate/semi-proprietary ones would put hostname A in your JID but expected you to know that they were going to present a certificate for hostname B, and certainly if we had JID example@gmail.com but were explicitly connecting to talk.google.com we had to accept either).

It wouldn't surprise me if that had test coverage. Maybe that's what's failing?

(In reply to Debarshi Ray from comment #0)
> Note that empathy has some copy/pasted GnuTLS code to get the hostname out
> of the certificate (empathy_get_x509_certificate_hostname) for which there
> isn't any counterpart in GTlsCertificate. I suggest we drop this and wait
> for the corresponding accessor to be added to GTlsCertificate.

"Wait for it to be added" isn't a good plan. Either "drop this feature with the expectation that it isn't coming back" or else "file a glib bug with a patch adding the API we need". (Or at the very least, "file a glib bug *requesting* the API we need".)

(In reply to Simon McVittie from comment #5)
> +      cert = g_initable_new (tls_certificate_type,
> 
> Is there really no convenience API for constructing the certificates?

Not from an array of DER blobs. It's not an API you'd be likely to need, except when interfacing with a TLS-library-agnostic TLS-handshake-validation API.

> Only having one identity here could be bad. The old code supported multiple
> reference identities, and I dimly remember that actually mattering for some
> XMPP servers

You could deal with that by passing a NULL identity to g_tls_database_verify_chain_async() to verify everything except the identity, and then (assuming that passes), call g_tls_certificate_verify() (with a NULL trusted_ca) on each possible identity until you get one that works.

(In reply to Dan Winship from comment #6)
> (In reply to Debarshi Ray from comment #0)
> > Note that empathy has some copy/pasted GnuTLS code to get the hostname out
> > of the certificate (empathy_get_x509_certificate_hostname) for which there
> > isn't any counterpart in GTlsCertificate. I suggest we drop this and wait
> > for the corresponding accessor to be added to GTlsCertificate.
> 
> "Wait for it to be added" isn't a good plan. Either "drop this feature with
> the expectation that it isn't coming back" or else "file a glib bug with a
> patch adding the API we need". (Or at the very least, "file a glib bug
> *requesting* the API we need".)

We'll have to "wait" in the sense that this patch set can't require anything higher than glib-2.50 or GNOME 3.22 because I need it at least in Fedora 25. The glib API addition would be glib-2.54 or GNOME 3.26 material.

But, yes, I'll file a request for the API. Although, I was hoping to take a look at any other remaining gcr_certificate_chain_build users to see what else they might be needing to have a fuller picture before asking for anything.

(In reply to Dan Winship from comment #7)
> (In reply to Simon McVittie from comment #5)
> > +      cert = g_initable_new (tls_certificate_type,
> > 
> > Is there really no convenience API for constructing the certificates?
> 
> Not from an array of DER blobs. It's not an API you'd be likely to need,
> except when interfacing with a TLS-library-agnostic TLS-handshake-validation
> API.

Umm... wouldn't it be useful for other instances of code that was using GcrSimpleCertificate and gcr_certificate_chain_build and now wants to port to GIO? I guess we will have to actually dig up those instances and see.

> > Only having one identity here could be bad. The old code supported multiple
> > reference identities, and I dimly remember that actually mattering for some
> > XMPP servers
> 
> You could deal with that by passing a NULL identity to
> g_tls_database_verify_chain_async() to verify everything except the
> identity, and then (assuming that passes), call g_tls_certificate_verify()
> (with a NULL trusted_ca) on each possible identity until you get one that
> works.

Ok. I was thinking that I'd have to chain together multiple g_tls_database_verify_chain_async calls, one for each identity, but this is simpler. g_tls_certificate_verify appears to be sync-safe and hence more loop-friendly.

(In reply to Debarshi Ray from comment #9)
> > > Is there really no convenience API for constructing the certificates?
> > 
> > Not from an array of DER blobs. It's not an API you'd be likely to need,
> > except when interfacing with a TLS-library-agnostic TLS-handshake-validation
> > API.
> 
> Umm... wouldn't it be useful for other instances of code that was using
> GcrSimpleCertificate and gcr_certificate_chain_build and now wants to port
> to GIO? I guess we will have to actually dig up those instances and see.

Are there any? And it's people who want to port *only* their certificate validation code to gio, but *not* their network I/O code. GLib assumes you're only going to be using its TLS certificate validation API if you're also using its TLS connection API, which seems reasonably reasonable. If it turns out there are lots of people who have good reason to want to use one but not the other then more APIs can be added, but I'm guessing that won't be the case.

> Ok. I was thinking that I'd have to chain together multiple
> g_tls_database_verify_chain_async calls, one for each identity, but this is
> simpler. g_tls_certificate_verify appears to be sync-safe and hence more
> loop-friendly.

Yes, g_tls_database_verify_chain() is blocking-or-async because it might have to do I/O to a smart card. g_tls_certificate_verify() only looks at the certs you explicitly give it, so there are no worries.

Comment on attachment 348117 [details] [review]
tls-verifier: Handle GNUTLS_CERT_REVOKED

Pushed to master and gnome-3-12.

Created attachment 348321 [details] [review]
tls-verifier: Use GIO to verify the chain of TLS certificates

Created attachment 348322 [details] [review]
Remove the GnuTLS dependency

I have tested the combination of the three attached patches, on top of empathy 3.12.13

It appears to work correctly.

Connecting to google talk works, when the legacy CAs are removed.

I've performed a couple of additional tests, using a local XMPP server:

- valid certificate, but hostname mismatch:
      fails as expected

- valid certificate with missing intermediate CA:
      fails as expected

- self-signed server certificate:
      fails as expected

- both self-signed server certificate and hostname mismatch:
      fails as expected

Given that I was unable to connect to google talk without these patch, I conclude that these patches improve the certificate verification as requested, but common certificate errors are still correctly identified.

(In reply to Kai Engert from comment #14)
> I have tested the combination of the three attached patches, on top of
> empathy 3.12.13
> 
> It appears to work correctly.

Thanks for the extensive tests, Kai!

I just realized that unlike Gcr [1], GnuTLS doesn't seem to provide a way to load a PKCS#11 module that's built into the code, as opposed to being a shared object. This makes it hard for us to load our mock PKCS#11 module. I guess it won't hurt that much to disable the test case that relies on using PKCS#11 storage to complete the certificate chain. All that work should be happening inside GnuTLS anyway.

[1] https://developer.gnome.org/gck/unstable/GckModule.html

Created attachment 348350 [details] [review]
tests: Fix comment

Created attachment 348351 [details] [review]
tests: Actually test that hostnames of pinned certificates are verified

Created attachment 348352 [details] [review]
tests: Retain the PEM formatted root certificate

Created attachment 348353 [details] [review]
tls-verifier: Use GIO to verify the chain of TLS certificates

Created attachment 348354 [details] [review]
Remove the GnuTLS dependency

I pushed the test suite fixes to master and gnome-3-12.

Created attachment 348417 [details] [review]
tls-verifier: Use GIO to verify the chain of TLS certificates

Rebased against master.

I pushed the actual port GIO port only to master because it bumps the GLib requirement quite significantly. Downstreams wishing to backport should be able to do so pretty easily.

Dan, Simon and Kai: thanks for all the help on this so far! Let me know if something is wrong.