Bug 31474

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.

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 ***

Oops, sorry, didn't notice you where trying to enhanced current implementation, let it go through normal review process then, again sorry ...

Marking with the 'patch' keyword.

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.

(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.

(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.)

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? :-)

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? :-)

I'll apply this when I have stable Internet again :-)

Same comment as I did on the Empathy bug :  --without-ca-file doesn't seem to be actually used.

--without-FOO is exactly equivalent to --with-FOO=no, and follows the same code paths, so I think the patch is fine.

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 :-)

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.

(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 ?

(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.

(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.