After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 636258 - Certificate verification should use keyring
Certificate verification should use keyring
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Auth client
unspecified
Other Linux
: Normal normal
: ---
Assigned To: empathy-maint
: 634489 635586 636260 (view as bug list)
Depends on: 636257
Blocks: 634489
 
 
Reported: 2010-12-01 22:26 UTC by Stef Walter
Modified: 2010-12-24 14:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
diff for review (57.46 KB, patch)
2010-12-13 10:46 UTC, Guillaume Desmottes
reviewed Details | Review
Updated patch with review changes (58.06 KB, patch)
2010-12-13 20:40 UTC, Stef Walter
none Details | Review

Description Stef Walter 2010-12-01 22:26:55 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).
Comment 1 Stef Walter 2010-12-11 05:17:31 UTC
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.
Comment 2 Guillaume Desmottes 2010-12-13 10:46:47 UTC
Created attachment 176329 [details] [review]
diff for review

Ideally I would have prefer smaller bug/branches but that will do it.
Comment 3 Guillaume Desmottes 2010-12-13 11:06:52 UTC
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
Comment 4 Guillaume Desmottes 2010-12-13 15:31:12 UTC
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.
Comment 5 Stef Walter 2010-12-13 20:40:00 UTC
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?
Comment 6 Stef Walter 2010-12-13 20:40:34 UTC
*** Bug 634489 has been marked as a duplicate of this bug. ***
Comment 7 Stef Walter 2010-12-13 20:45:21 UTC
*** Bug 636260 has been marked as a duplicate of this bug. ***
Comment 8 Stef Walter 2010-12-13 21:11:16 UTC
*** Bug 635586 has been marked as a duplicate of this bug. ***
Comment 9 Guillaume Desmottes 2010-12-14 07:02:17 UTC
(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.
Comment 10 Stef Walter 2010-12-14 14:45:31 UTC
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?
Comment 11 Guillaume Desmottes 2010-12-14 14:53:07 UTC
We usually merge it as it, atomic commits are cool. :)
Comment 12 Travis Reitter 2010-12-23 01:57:14 UTC
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.
Comment 13 Travis Reitter 2010-12-23 02:02:41 UTC
(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.
Comment 14 Stef Walter 2010-12-23 03:39:37 UTC
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.
Comment 15 Guillaume Desmottes 2010-12-24 08:30:33 UTC
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?
Comment 16 Guillaume Desmottes 2010-12-24 10:45:24 UTC
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.
Comment 17 Guillaume Desmottes 2010-12-24 11:03:27 UTC
I just noticed that empathy's configure still check for gnome-keyring-1. Is that still needed? If not, feel free to remove it.
Comment 18 Stef Walter 2010-12-24 14:08:20 UTC
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.