GNOME Bugzilla – Bug 636258
Certificate verification should use keyring
Last modified: 2010-12-24 14:08:20 UTC
This bug will track: * Loading of Anchor CA certificates from keyring. * Storing + loading of anchor CA certificates in keyring. The above will use PKCS#11 to provide the loose coupling, via a specification (in progress) for trust storage via PKCS#11. Rather than using PKCS#11 directly we use libgcr's new trust assertion functions (in the trust-store branch).
The auth client uses libgcr to lookup trust anchors and pinned certificates. Checking the 'Remember' box stores a pinned certificate trust assertion via libgcr. Initially implemented building of the certificate chain in empathy. However once you factor in the fact that it needs to be async, it can get a bit complicated. So instead I've added a GcrCertificateChain class to libgcr which empathy now uses to build the cert chain. The cert chain is then validated by gnutls. Added test program tests/empathy-tls-test which tests various scenarios for certificate validation. For the tests which require PKCS#11, it loads the modules from the standalone gnome-keyring modules. All of this is in this branch: http://git.collabora.co.uk/?p=user/stefw/empathy.git;a=shortlog;h=refs/heads/trust-assertions Guillaume, should I squash this into one commit? Or break into multiple bugs? If so, let me know.
Created attachment 176329 [details] [review] diff for review Ideally I would have prefer smaller bug/branches but that will do it.
Review of attachment 176329 [details] [review]: Does the branch pass "make check" and "make distcheck"? Did you valgrind your tests? That's generally a good thing to do to detect leaks. ::: configure.ac @@ +37,3 @@ GTK_REQUIRED=2.91.3 KEYRING_REQUIRED=2.26.0 +GCR_REQUIRED=2.91.4 Does this version contain everything we need? (ie, we don't depend on unreleased API?) @@ -202,2 @@ # ----------------------------------------------------------- -# Make CA certificates path configurable Does libgcr providee exactly the same option? We tried to unify it in glib, telepathy-gabble and empathy. ::: libempathy/empathy-tls-verifier.c @@ +114,3 @@ +static void +build_certificate_list_for_gnutls (GcrCertificateChain *chain, + gnutls_x509_crt_t **list, guint *n_list, one arg per line; please fix this in all the places where you introduced new functions. @@ +533,3 @@ + EmpathyTLSVerifierPriv *priv = GET_PRIV (self); + + g_object_get (priv->certificate, "cert-data", &certs, NULL); certs is leaked
Does this branch fix bug #635586 as well? If yes, can you please close it as a dup? While you're on it, it would be cool if you could update the status of others "Auth client" bugs, if needed.
Created attachment 176364 [details] [review] Updated patch with review changes (In reply to comment #2) > Ideally I would have prefer smaller bug/branches but that will do it. Okay, will keep that in mind in the future. (In reply to comment #3) > Does the branch pass "make check" and "make distcheck"? Yup both pass properly. > Did you valgrind your tests? That's generally a good thing to do to detect > leaks. Good plan. Done. > ::: configure.ac > @@ +37,3 @@ > GTK_REQUIRED=2.91.3 > KEYRING_REQUIRED=2.26.0 > +GCR_REQUIRED=2.91.4 > > Does this version contain everything we need? (ie, we don't depend on > unreleased API?) No, this is an unreleased, unmerged API in the trust-store branch of gnome-keyring. I'm trying to drum up some reviews there, but it's a big branch. There's good participation on the specification. I will hopefully be merging it this week. This bug depends on the relevant gnome-keyring bug. > @@ -202,2 @@ > # ----------------------------------------------------------- > -# Make CA certificates path configurable > > Does libgcr providee exactly the same option? We tried to unify it in glib, > telepathy-gabble and empathy. The option is in gnome-keyring and has been there for several years. It may not be so advantageous to change it at this point. WDYT? > one arg per line; please fix this in all the places where you introduced new > functions. Good to know. Fixed. > @@ +533,3 @@ > + EmpathyTLSVerifierPriv *priv = GET_PRIV (self); > + > + g_object_get (priv->certificate, "cert-data", &certs, NULL); > > certs is leaked Fixed, and renamed various variables to make it clearer what is what. Using g_boxed_free(). Is that correct?
*** Bug 634489 has been marked as a duplicate of this bug. ***
*** Bug 636260 has been marked as a duplicate of this bug. ***
*** Bug 635586 has been marked as a duplicate of this bug. ***
(In reply to comment #5) > > @@ -202,2 @@ > > # ----------------------------------------------------------- > > -# Make CA certificates path configurable > > > > Does libgcr providee exactly the same option? We tried to unify it in glib, > > telepathy-gabble and empathy. > > The option is in gnome-keyring and has been there for several years. It may not > be so advantageous to change it at this point. WDYT? I guess that's fine then. > > @@ +533,3 @@ > > + EmpathyTLSVerifierPriv *priv = GET_PRIV (self); > > + > > + g_object_get (priv->certificate, "cert-data", &certs, NULL); > > > > certs is leaked > > Fixed, and renamed various variables to make it clearer what is what. Using > g_boxed_free(). Is that correct? yep. Branch looks good. Feel free to merge once your gnome-keyring branch has been merged and released. It's probably a good idea to rebase the branch on top of master from time to time to avoid divering to much.
Okay will do. Do we generally squash merge this stuff in the empathy project? Or just bring in all the commits to show development progress?
We usually merge it as it, atomic commits are cool. :)
Is this still waiting on a gnome-keyring release? Please apply as soon as possible, since anyone keeping up with gcr-3 in git can get wedged, as the internal gcr-simple-certificate.[ch] conflict with the one shipped in gcr-3. (And the patch no longer applies cleanly to Empathy master, so I can't just apply it in my empathy work branch) I'm not sure why this wasn't a problem earlier - we've bundled gcr-simple-certificate.[ch] for a few months. Maybe I'm not seeing recent removal of build/pre-processor conditionals.
(In reply to comment #12) > I'm not sure why this wasn't a problem earlier - we've bundled > gcr-simple-certificate.[ch] for a few months. Maybe I'm not seeing recent > removal of build/pre-processor conditionals. Looks like the issue is that gcr_simple_certificate_new() broke API a few months ago.
I'm planning on working on merging this tomorrow. I just released gnome-keyring 2.91.4 today. If empathy now builds on gtk+ master, then I should be able to get it done in short order.
Latest Empathy release can't be build with latest gnome-keyring release. That's pretty critical and the GNOME is going to hate us if we don't fix that ASAP. Could you please cherry-pick only the bits of your branch fixing that problem and merge it?
So, I cherry picked this patch from your branch in order to fix this issue: commit 89ed5197c3cf61971b8425b176ce71073add7360 Author: Stef Walter <stefw@collabora.co.uk> Date: Thu Dec 2 17:21:38 2010 +0000 libempathy-gtk: No need to 'egg' gcr-simple-certificate.[ch] These are now properly distributed by libgcr. I also record this trivial patch to make it build fine: commit 8f4bd49edcf38951072b9f12ce50ba9aaddac83d Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Fri Dec 24 11:42:57 2010 +0100 just include gcr.h Just rebuild your branch on top of master, the conflict should be trivial.
I just noticed that empathy's configure still check for gnome-keyring-1. Is that still needed? If not, feel free to remove it.
I spent most of yesterday trying to fix GTK+ 3 and trying to get all the new dependencies to build, but have completed that now. I've now merged the trust-assertions branch (rebased and then merged). There's a bit more fine tuning to be done on the gnome-keyring side. The check for gnome-keyring-l is for libgnome-keyring which is used to store passwords.