Bug 31474

Summary: [Patch] Make CA cert paths configurable more friendly
Product: Telepathy Reporter: Brian Pepple <bpepple>
Component: gabbleAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium CC: adconrad, danielle, nicolas, sjoerd
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=shortlog;h=refs/heads/gtls
See Also: https://bugzilla.gnome.org/show_bug.cgi?id=588189
Whiteboard:
i915 platform: i915 features:
Attachments: Patch to make ca-cert config more friendly

Description Brian Pepple 2010-11-08 09:03:58 UTC
Please find the attached patch to make the ca cert configuration a bit more friendly for distros that don't use /etc/ssl. In addition this patch was created to make empathy & tp-gabble use the same config code (which was borrowed from gio).

Empathy bug to make ca-cert configurable:
https://bugzilla.gnome.org/show_bug.cgi?id=634197
Comment 1 Brian Pepple 2010-11-08 09:06:50 UTC
Created attachment 40123 [details] [review]
Patch to make ca-cert config more friendly

This patch was made against the tp-gabble-0.10 branch, but I believe it should apply fine against master.
Comment 2 Nicolas Dufresne 2010-11-08 09:11:44 UTC
This is already solved, use --with-ca-certificates to set you CA certificate path.

*** This bug has been marked as a duplicate of bug 29000 ***
Comment 3 Nicolas Dufresne 2010-11-08 09:13:34 UTC
Oops, sorry, didn't notice you where trying to enhanced current implementation, let it go through normal review process then, again sorry ...
Comment 4 Danielle Madeley 2010-11-09 14:23:35 UTC
Marking with the 'patch' keyword.
Comment 5 Nicolas Dufresne 2010-11-09 14:36:01 UTC
Review of attachment 40123 [details] [review]:

I think I'd prefer keeping the option and define name we had originally used to avoid breaking packagers work.

::: configure.ac
@@ -99,4 +99,6 @@
-AC_ARG_WITH(ca-certificates,
-    AC_HELP_STRING([--with-ca-certificates],[path to CA certificates @<:@default=/etc/ssl/certs/ca-certificates.crt@:>@]),
-    ca_certificates_path="$withval", ca_certificates_path="/etc/ssl/certs/ca-certificates.crt")
-AC_DEFINE_UNQUOTED(CA_CERTIFICATES_PATH, ["${ca_certificates_path}"], [Path to CA certificates])
+# -----------------------------------------------------------
+# Make CA certificates path configurable
+# Stolen from GIO's TLS
... 3 more ...

I don't like renaming a compilation option, some packagers already uses --with-ca-certificates.

@@ -99,4 +99,30 @@
-AC_ARG_WITH(ca-certificates,
-    AC_HELP_STRING([--with-ca-certificates],[path to CA certificates @<:@default=/etc/ssl/certs/ca-certificates.crt@:>@]),
-    ca_certificates_path="$withval", ca_certificates_path="/etc/ssl/certs/ca-certificates.crt")
-AC_DEFINE_UNQUOTED(CA_CERTIFICATES_PATH, ["${ca_certificates_path}"], [Path to CA certificates])
+# -----------------------------------------------------------
+# Stolen from GIO's TLS
+else
... 27 more ...

I don't like using GLib like namespace in Gabble, also why do we change it from original name, CA_CERTIFICATES_PATH

::: src/connection.c
@@ +1967,2 @@
   /* system certs */
+  wocky_tls_handler_add_ca (tls_handler, GTLS_SYSTEM_CA_FILE);

This is not required if you keep original name.
Comment 6 Guillaume Desmottes 2010-11-09 23:58:38 UTC
(In reply to comment #5)
> Review of attachment 40123 [details] [review]:
> 
> I think I'd prefer keeping the option and define name we had originally used to
> avoid breaking packagers work.

I don't think it's that bad if we change it only in the dev branch and clearly
document it in NEWS. Having the same option in gio, gabble and empathy seems a
worthy goal.
Comment 7 Simon McVittie 2010-11-15 04:36:34 UTC
(In reply to comment #6)
> I don't think it's that bad if we change it only in the dev branch and clearly
> document it in NEWS. Having the same option in gio, gabble and empathy seems a
> worthy goal.

I agree.

(The extended auto-detection here also covers both Debian-derived and Red-Hat-derived distros, although with my Debian packager hat on, I'd generally override automagic choices like that with the known-correct Debian path in the packaging, for better robustness.)
Comment 8 Simon McVittie 2010-11-15 05:04:20 UTC
Review of attachment 40123 [details] [review]:

I'm inclined to merge this (but only in 0.11, and with a pointer in NEWS).

::: configure.ac
@@ -99,4 +99,30 @@
-AC_ARG_WITH(ca-certificates,
-    AC_HELP_STRING([--with-ca-certificates],[path to CA certificates @<:@default=/etc/ssl/certs/ca-certificates.crt@:>@]),
-    ca_certificates_path="$withval", ca_certificates_path="/etc/ssl/certs/ca-certificates.crt")
-AC_DEFINE_UNQUOTED(CA_CERTIFICATES_PATH, ["${ca_certificates_path}"], [Path to CA certificates])
+# -----------------------------------------------------------
+# Make CA certificates path configurable
+# Stolen from GIO's TLS
... 27 more ...

I'm a bit suspicious about using a name that looks as though it "belongs to" GLib too, but I don't think it's a big deal, and having exactly the same check as GIO TLS seems like a win. It's not as if this is API or anything.

The new name does make it clearer that this is a CA-cert-bundle file, as opposed to a directory of hashed symlinks (e.g. Debian's /etc/ssl/certs/) or a $PATH-like search path, which I think is good.

Nicolas, if the current GIO TLS work doesn't already make this unnecessary, could you add g_get_system_ca_file() or something so that long-term we can just delegate this to GIO? :-)
Comment 9 Simon McVittie 2010-11-15 05:04:26 UTC
Review of attachment 40123 [details] [review]:

I'm inclined to merge this (but only in 0.11, and with a pointer in NEWS).

::: configure.ac
@@ -99,4 +99,30 @@
-AC_ARG_WITH(ca-certificates,
-    AC_HELP_STRING([--with-ca-certificates],[path to CA certificates @<:@default=/etc/ssl/certs/ca-certificates.crt@:>@]),
-    ca_certificates_path="$withval", ca_certificates_path="/etc/ssl/certs/ca-certificates.crt")
-AC_DEFINE_UNQUOTED(CA_CERTIFICATES_PATH, ["${ca_certificates_path}"], [Path to CA certificates])
+# -----------------------------------------------------------
+# Make CA certificates path configurable
+# Stolen from GIO's TLS
... 27 more ...

I'm a bit suspicious about using a name that looks as though it "belongs to" GLib too, but I don't think it's a big deal, and having exactly the same check as GIO TLS seems like a win. It's not as if this is API or anything.

The new name does make it clearer that this is a CA-cert-bundle file, as opposed to a directory of hashed symlinks (e.g. Debian's /etc/ssl/certs/) or a $PATH-like search path, which I think is good.

Nicolas, if the current GIO TLS work doesn't already make this unnecessary, could you add g_get_system_ca_file() or something so that long-term we can just delegate this to GIO? :-)
Comment 10 Simon McVittie 2010-11-15 05:05:43 UTC
I'll apply this when I have stable Internet again :-)
Comment 11 Guillaume Desmottes 2010-11-15 05:10:24 UTC
Same comment as I did on the Empathy bug :  --without-ca-file doesn't seem to be actually used.
Comment 12 Simon McVittie 2010-11-15 06:14:42 UTC
--without-FOO is exactly equivalent to --with-FOO=no, and follows the same code paths, so I think the patch is fine.
Comment 13 Simon McVittie 2010-11-16 04:14:45 UTC
Fixed in git for 0.11.1, thanks.

WONTFIX for 0.10.x, to avoid behaviour changes on a stable branch - please keep using --with-ca-certificates in spec files for that branch.

(If you use both options, it'll provoke a warning but is otherwise harmless, so you can use both if that makes it easier :-)
Comment 14 Simon McVittie 2010-11-18 07:35:41 UTC
Looks like Nicolas was right all along...

Adam points out that the configure check fails if the CA file isn't present on the build system. In a minimal build environment, or when cross-compiling, or whatever, we don't want to require that - it's fine for the auto-detection to fail in these cases, but if the user specifies a location, we should believe that they will arrange for it to be present on the host system.

Fixed in http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=f6468dd3e47958de879708ea83caaefa2e2fd3f5

Meanwhile, Sjoerd points out that when using the OpenSSL backend, it's conventional and more efficient if the CA location is a directory (/etc/ssl/certs on Debian) containing fingerprint-based symlinks (e.g. /etc/ssl/certs/00673b5b.0 -> thawte_Primary_Root_CA.pem). Again, it's OK if auto-detection doesn't handle this case, but if the user forces it, that should be respected.

This makes the name --with-ca-file misleading, so I reverted to --with-ca-certificates in http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=3badae31eb4c05015b17a7f759b7281f46a814bf

Reviewers? I'd particularly value input from Nicolas on getting this synced into GIO.
Comment 15 Nicolas Dufresne 2010-11-18 11:05:23 UTC
(In reply to comment #14)
> Reviewers? I'd particularly value input from Nicolas on getting this synced
> into GIO.

First Patch:
I totally agree, and you made me remembered that OpenSSL can work with directories (I had forgotten). The only remaining thing that I don't like is the fact that you use GTLS_, which I found stands for GNUTLS. This is misleading, not a big deal since all this will go away when we'll move to GLib TLS ;)

Second Patch:
On glib-networking side it's already fixed. They fail if path was auto-detected, and only warn if file/dir does not exist when it is being set manually. Maybe you should do the same and warn instead of not checking it ?
Comment 16 Simon McVittie 2010-11-19 04:26:59 UTC
(In reply to comment #15)
> I totally agree, and you made me remembered that OpenSSL can work with
> directories (I had forgotten). The only remaining thing that I don't like is
> the fact that you use GTLS_, which I found stands for GNUTLS. This is
> misleading, not a big deal since all this will go away when we'll move to GLib
> TLS ;)

I've asked on the GLib bug whether they'd consider renaming the option; it'd be good to have all these projects in sync. If they insist on --with-ca-file even though it's factually incorrect, it's probably better to follow them.

> On glib-networking side it's already fixed. They fail if path was
> auto-detected, and only warn if file/dir does not exist when it is being set
> manually. Maybe you should do the same and warn instead of not checking it ?

Branch updated to do so.
Comment 17 Simon McVittie 2010-11-19 08:41:41 UTC
(In reply to comment #16)
> I've asked on the GLib bug whether they'd consider renaming the option; it'd be
> good to have all these projects in sync. If they insist on --with-ca-file even
> though it's factually incorrect, it's probably better to follow them.

Dan agreed with --with-ca-certificates, so I've changed it back.

> Branch updated to do so.

r+ from wjt on IRC, merged.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.