Created attachment 308746 [details]
server certificate

Created attachment 308747 [details]
full trust chain (from /etc/pki/ca-trust/source/anchors)

Hm, this design flaw is far-reaching. A lot of code in gtlsfiledatabase-gnutls.c and gtlsdatabase-gnutls-pkcs11.c seems to assume there can only be one certificate with a given subject.

Yet GnuTLS *has* a gnutls_certificate_get_issuer() function, and coherent integration with the system certificate database (which ought to work on Windows, etc.), and for some reason we decline to use it...

There were reasons we did it this way, though it was a long time ago and I'm not sure I remember all the details. Mostly it had to do with things that gnutls didn't do *at that time*:

  - we thought glib-networking was going to have fetching-certificates-
    out-of-gnome-keyring support before gnutls had pkcs#11 support, so in
    some cases we would need to assemble the cert chain ourselves

  - we needed to be able to validate certificates even when the server
    passed an incomplete chain, and IIRC at the time we couldn't get
    gnutls to do that unless we built the chain ourselves

  - we thought it would be important to be able to verify SRV-record-based
    chains in addition to hostname-based chains (though in fact this was
    never implemented, in either glib-networking or gnutls, and seems to
    have been abandoned as an idea in the internet at large)

  - gnutls_certificate_set_x509_system_trust() wasn't added to gnutls
    until a year after our code was written (and IIRC only in gnutls 3.x,
    which wasn't available on Fedora at the time)

But it certainly seems like this code is just hurting us now (bug 683266, bug 750457), so we should probably gut it. (Patches welcome...)

Yup agree with Dan. Now that GnuTLS does this stuff, and does PKCS#11 too ... this could is a prime candidate for removal/cleanup.

(In reply to Dan Winship from comment #4)
> There were reasons we did it this way, though it was a long time ago and I'm
> not sure I remember all the details. Mostly it had to do with things that
> gnutls didn't do *at that time*:

I can certainly sympathise with *all* of that. I have to admit I *still* have code in OpenConnect which validates a server hostname/IP address against its X.509 certificate. Which is utterly evil for all the same reasons. I must kill that now that OpenSSL 1.0.2 finally offers that functionality.

> But it certainly seems like this code is just hurting us now (bug 683266,
> bug 750457), so we should probably gut it. (Patches welcome...)

I might be able to take a look at gutting the offending code and just using GnuTLS, but it's not going to happen imminently. My "TO BE DONE BY LAST WEEK" list is long enough already.

I think we should probably mark those other bugs as duplicates of this, or of a new "we do this all for ourselves and we shouldn't" bug. Let's not just play whack-a-mole with the symptoms.

This is likely a duplicate of bug 750457

At first glance that appears to be a *different* symptom of the same underlying "we reimplemented the wheel and we got it wrong" problem. We may have papered over the cracks of *that* particular symptom, but I don't believe that patch addresses *this* symptom. And it certainly doesn't fix the real underlying problem.

Partly fixed in git: GTlsFileDatabaseGnutls now uses gnutls_x509_trust_list_add_trust_file() and gnutls_x509_trust_list_verify_crt2(), and none of the old hand-rolled verification code.

Remaining to be done:

  - Make the PKCS#11 code use gnutls_x509_trust_list_t as well, using
    gnutls's built-in PKCS#11 support.

  - Make GTlsFileDatabaseGnutls construct its internal list of CAs by
    iterating the gnutls_x509_trust_list_t rather than by re-parsing the
    file itself. (This depends on a recent-ish gnutls, so we might need
    to keep both codepaths around. Also, using the trust list iterating
    API on the PKCS#11 side should let us get rid of most of our direct
    p11-kit use as well.)

  - Make the default GTlsDatabase use
    gnutls_x509_trust_list_add_system_trust() rather than reading in a
    file.

      - Can only do this if gnutls is new enough to have the
        trust-list-iterating API, because otherwise we can't implement
        things like g_tls_database_lookup_certificates_issued_by().

      - This basically deprecates --with-ca-certificates.

*** Bug 636571 has been marked as a duplicate of this bug. ***

meh, broke the build (bug 761713), will re-fix tomorrow

(In reply to David Woodhouse from comment #0)
> It's also rather concerning that you choose to complain that the cert is
> *expired*. Because there's a far more serious issue here, which is that if
> you think that *other* 'Intel Intranet Basic Issuing CA 2A' was the issuer
> of the server's cert, that means the signautre on the server's certificate
> is completely invalid. Are you even *checking* the signatures from the
> alleged issuers?

For future reference, what actually happened here is that glib-networking constructed the correct chain, passed it to gnutls_x509_crt_list_verify(), determined that the chain was valid... and then shot itself in the foot by doing additional incorrect checks after that.

The reason glib-networking did additional checks after calling gnutls_x509_crt_list_verify() was because I had wanted g_tls_database_verify_chain() to return *all* of the errors associated with a bad chain, whereas gnutls usually only returns a single error. (Eg, if the CA is unknown and the certificate is expired, gnutls will only return GNUTLS_CERT_SIGNER_NOT_FOUND but I wanted to return G_TLS_CERTIFICATE_UNKNOWN_CA | G_TLS_CERTIFICATE_EXPIRED.)

But the additional-failure-checking code was (a) running even when gnutls_x509_crt_list_verify() returned success, and (b) checking the wrong list of certs.

Anyway, the new version of the code doesn't do this any more; in addition to letting gnutls construct the chain, it also just returns whatever error(s) gnutls returns rather than doing additional checks.


tl;dr: the old validation code generated false negatives, but not false positives

(In reply to Dan Winship from comment #12)

> The reason glib-networking did additional checks after calling
> gnutls_x509_crt_list_verify() was because I had wanted
> g_tls_database_verify_chain() to return *all* of the errors associated with
> a bad chain, whereas gnutls usually only returns a single error. (Eg, if the
> CA is unknown and the certificate is expired, gnutls will only return
> GNUTLS_CERT_SIGNER_NOT_FOUND but I wanted to return
> G_TLS_CERTIFICATE_UNKNOWN_CA | G_TLS_CERTIFICATE_EXPIRED.)

Hi Dan,
 Is there something gnutls can do better in that aspect to help verification? There is already (granted a quite limited) callback interface to assist in providing more information about verification (the one used by certtool -e to provide info on the chain). If you think there is something I can add or improve to that, I'd be happy to discuss that.

[0]. https://gitlab.com/gnutls/gnutls/blob/master/src/certtool.c#L2550

I don't think there's anything else we need from gnutls. The stuff you quoted was something I did because it seemed more correct to do it that way (return all relevant error codes rather than just one), but there's no real reason we *need* to do it that way (which is why I removed that code rather than continuing to try to maintain it).

Comment 9 lays out what remains to be done in glib-networking to finish fixing certificate validation. As far as I could tell from just inspecting the APIs, it ought to be possible to do everything we want just with the APIs that are already there.

(In reply to Dan Winship from comment #9)
> Remaining to be done:
> 
>   - Make the PKCS#11 code use gnutls_x509_trust_list_t as well, using
>     gnutls's built-in PKCS#11 support.

Bug #793280

>   - Make GTlsFileDatabaseGnutls construct its internal list of CAs by
>     iterating the gnutls_x509_trust_list_t rather than by re-parsing the
>     file itself. (This depends on a recent-ish gnutls, so we might need
>     to keep both codepaths around. Also, using the trust list iterating
>     API on the PKCS#11 side should let us get rid of most of our direct
>     p11-kit use as well.)

I'm planning to work on this soon.

>   - Make the default GTlsDatabase use
>     gnutls_x509_trust_list_add_system_trust() rather than reading in a
>     file.

This was easy to do, but it creates the odd result that the default GTlsFileDatabase may no longer based on a file at all. E.g. in Fedora, it will use this codepath in GnuTLS:

https://gitlab.com/gnutls/gnutls/blob/c0eb46d3463cd21b3f822ac377ff37f067f66b8d/lib/system/certs.c#L118

where gnutls_x509_trust_list_add_trust_file() is called with DEFAULT_TRUST_STORE_PKCS11, which, in Fedora, is "pkcs11:", see https://src.fedoraproject.org/cgit/rpms/gnutls.git/tree/gnutls.spec?id=68b93da7fa2889f0070f47bb6a7c754e8f97f88f#n154.

I think we should do exactly that, but I'm not sure I fully understand the consequences. Clearly PKCS#11 will now be used by default. But it's hard to find *understandable* information about PKCS#11 on the internet. I see it's a standard for managing smartcards and hardware security modules; that doesn't tell me what happens when a mysterious URI "pkcs11:" replaces a straightforward file like /etc/pki/tls/certs/ca-bundle.crt.

This change also breaks HTTPS inside Flatpak, because GnuTLS has no default trust store there. I've created https://github.com/flatpak/freedesktop-sdk-base/pull/16 for that.

PKCS#11 is just an API to access crypto operations, used mostly to access HSMs and smart cards.

The p11-kit trust store module, provides the PKCS#11 and allows accessing the system's CA trust store, in addition with local restrictions. That is, you can retrieve not only the trust CAs, but also additional restrictions on them (that's mainly the idea, in practice restrictions are mostly applied by gnutls [0]).

The PKCS#11 API was used because it provides certificate search functionality.

Example:
```
$ p11tool --list-all 'pkcs11:model=p11-kit-trust'

Object 1:
        URL: pkcs11:model=p11-kit-trust;manufacturer=PKCS%2311%20Kit;serial=1;token=System%20Trust;id=%0d%ac%19%70%ed%04%a7%5e%78%ab%fe%8b%a8%34%eb%21%26%c1%56%35;object=Local%20CA;type=cert
        Type: X.509 Certificate
        Label: Local CA
        Flags: CKA_CERTIFICATE_CATEGORY=CA; CKA_TRUSTED; 
        ID: 0d:ac:19:70:ed:04:a7:5e:78:ab:fe:8b:a8:34:eb:21:26:c1:56:35
        Attached extensions:2.5.29.30 
```

That shows a local CA on my system which has the name constraints extensions attached.

See also:
https://p11-glue.github.io/p11-glue/manual/trust-module.html

[0]. https://nikmav.blogspot.cz/2016/06/restricting-scope-of-ca-certificates.html

Patch incoming. I'll push it soon unless there are any objections.

Created attachment 368171 [details] [review]
Use the GnuTLS system trust by default

Get rid of the ca_certificates_path build flag. The default GTlsDatabase
will now use the GnuTLS system trust. GTlsFileDatabase now builds its
internal hash tables by iterating its gnutls_x509_trust_list_t, rather
than by parsing its certificate file manually.

There are some potential compatibility risks here:

 * The minimum required GnuTLS version is increased from 3.3.5 to 3.4.

 * If GnuTLS is not configured with a system trust, all certificate
   verification using the default GTlsDatabase will fail. I noticed that
   GNOME's flatpak runtime does not configure a system trust, so this
   breaks HTTPS there. This is sad for Epiphany Technology Preview, but
   we should do this anyway, and separately fix the GNOME runtime.

 * It was previously possible to configure glib-networking using
   --without-ca-certificates to ensure the default GTlsDatabase is
   empty (GNOME #727282). Apparently this was desirable on some embedded
   systems, though I'm not sure why. Such configuration is still
   possible by configuring GnuTLS with no system trust. Presumably,
   anybody relying on this behavior will notice that the
   --without-ca-certificates flag has disappeared during the build
   system change and investigate, so this seems unproblematic.

 * The default GTlsDatabase is a GTlsFileDatabase, but it might not
   actually correspond to a file anymore, since GnuTLS may be configured
   to use p11-kit for its default trust store. This is a bit weird.

 * If the anchors property of a GTlsFileDatabase is NULL, that
   previously indicated an empty GTlsFileDatabase, but now indicates
   that the system trust is used. NULL is documented to be an allowed
   value, but the effects of setting the property to NULL are not
   documented. We should document it. Arguably, it would be safer to
   create a new property to indicate that the system trust should be
   used. But I think we can get away with this, because this change only
   affects applications that were intentionally creating useless
   GTlsFileDatabases with no certificates, which seems unlikely.

 * Certificate handles created using the default GTlsDatabase will be
   different before and after this commit. This seems unlikely to cause
   problems in practice, since a quick Debian codesearch reveals zero
   applications using our certificate handles. But, if an application
   were to rely on handles generated by previous versions of
   glib-networking being valid in the new version, it would break.

So there are actually several risks here. Still, none of the above seem
*likely* to cause practical issues, once distributors ensure that GnuTLS
 is built properly, so I think we can proceed.

Created attachment 368190 [details] [review]
Use the GnuTLS system trust by default

With fixes suggested by Phillip

Created attachment 368194 [details] [review]
Use the GnuTLS system trust by default

Forgot to update the commit message to explain why the script is removed.

Created attachment 368195 [details] [review]
Use the GnuTLS system trust by default

Um, is Bugzilla having trouble still? Test test test....

For some reason git-bz or Bugzilla is no longer leaving comments along with the new attachments... but I attached an updated version.

Review of attachment 368171 [details] [review]:

I haven’t checked the TLS/GnuTLS specifics, but here are a few general C bugs.

::: find-ca-certificates
@@ -1,1 @@
-#!/usr/bin/env python3

The removal of this file should be mentioned in the commit message.

::: tls/gnutls/gtlsfiledatabase-gnutls.c
@@ +166,3 @@
+  else
+    {
+      uri_part = g_strdup ("system-trust:///");

What’s with the treble-slash (`///`)? URIs don’t need that; they just need a colon to separate the scheme.

https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax

@@ +203,2 @@
 static gboolean
+initialize_tables (gnutls_x509_trust_list_t  trust_list,

This can have a void return type now, since you’ve removed the GError parameter.

@@ +208,2 @@
 {
+  gnutls_x509_trust_list_iter_t iter = NULL;

The iter needs to be freed at the end of the function using gnutls_x509_trust_list_iter_deinit().

@@ +255,3 @@
       g_bytes_unref (issuer);
 
+      gnutls_x509_crt_deinit (cert);

I suspect `cert` is leaked on all the g_warning() paths above.

(In reply to Philip Withnall from comment #19)
> ::: find-ca-certificates
> @@ -1,1 @@
> -#!/usr/bin/env python3
> 
> The removal of this file should be mentioned in the commit message.

Sure.

> ::: tls/gnutls/gtlsfiledatabase-gnutls.c
> @@ +166,3 @@
> +  else
> +    {
> +      uri_part = g_strdup ("system-trust:///");
> 
> What’s with the treble-slash (`///`)? URIs don’t need that; they just need a
> colon to separate the scheme.
> 
> https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax

OK, I'll drop two of the slashes. It doesn't actually matter, since the handle does not have to be a URI.

> @@ +203,2 @@
>  static gboolean
> +initialize_tables (gnutls_x509_trust_list_t  trust_list,
> 
> This can have a void return type now, since you’ve removed the GError
> parameter.

Oops, yes.

> @@ +208,2 @@
>  {
> +  gnutls_x509_trust_list_iter_t iter = NULL;
> 
> The iter needs to be freed at the end of the function using
> gnutls_x509_trust_list_iter_deinit().

It's actually a documentation bug, I reported https://gitlab.com/gnutls/gnutls/merge_requests/597 for this.

> @@ +255,3 @@
>        g_bytes_unref (issuer);
>  
> +      gnutls_x509_crt_deinit (cert);
> 
> I suspect `cert` is leaked on all the g_warning() paths above.

Yes, good catch.

(In reply to Michael Catanzaro from comment #20)
> > ::: tls/gnutls/gtlsfiledatabase-gnutls.c
> > @@ +166,3 @@
> > +  else
> > +    {
> > +      uri_part = g_strdup ("system-trust:///");
> > 
> > What’s with the treble-slash (`///`)? URIs don’t need that; they just need a
> > colon to separate the scheme.
> > 
> > https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax
> 
> OK, I'll drop two of the slashes. It doesn't actually matter, since the
> handle does not have to be a URI.

Why leave any of the slashes? Unless you explicitly want a ‘root’ path in the URI? URIs can have a fragment without a path, I believe.

(In reply to Michael Catanzaro from comment #20)
> > @@ +255,3 @@
> >        g_bytes_unref (issuer);
> >  
> > +      gnutls_x509_crt_deinit (cert);
> > 
> > I suspect `cert` is leaked on all the g_warning() paths above.
> 
> Yes, good catch.

The GBytes were already leaking on these error paths; I'll fix that too.

So I wonder, is Bugzilla just not displaying new comments in this bug? I wonder if my chain of comments is going to show up later or not....

(In reply to Philip Withnall from comment #25)

> Why leave any of the slashes? Unless you explicitly want a ‘root’ path in
> the URI? URIs can have a fragment without a path, I believe.

I've removed the slashes. Got too used to web schemes.

(In reply to Michael Catanzaro from comment #27)
> So I wonder, is Bugzilla just not displaying new comments in this bug? I
> wonder if my chain of comments is going to show up later or not....

OK, looks like comments are back on.

I attached an updated patch. There's just no comment for it. It's magically appeared in the attachment section, though.

(In reply to Michael Catanzaro from comment #18)
>  * If GnuTLS is not configured with a system trust, all certificate
>    verification using the default GTlsDatabase will fail...
> 
>  * It was previously possible to configure glib-networking using
>    --without-ca-certificates to ensure the default GTlsDatabase is
>    empty (GNOME #727282). Apparently this was desirable on some embedded
>    systems, though I'm not sure why...

A workaround for both of these issues would be to keep --with-ca-certificates, and continue using a non-system-trust-based database if that was passed. (Though you'd want to do *something* to make sure people didn't keep using it when they *should* be using the system trust. So maybe keep the behavior but rename the flag?)

>  * The default GTlsDatabase is a GTlsFileDatabase, but it might not
>    actually correspond to a file anymore, since GnuTLS may be configured
>    to use p11-kit for its default trust store. This is a bit weird.
> 
>  * If the anchors property of a GTlsFileDatabase is NULL, that
>    previously indicated an empty GTlsFileDatabase, but now indicates
>    that the system trust is used.

If the database is not backed by a file, it shouldn't be a GTlsFileDatabase.

I think this is easily fixed though: just move 95% of the GTlsFileDatabaseGnutls code into its parent class, GTlsDatabaseGnutls, and then have the "system trust" database be a plain GTlsDatabaseGnutls rather than a GTlsFileDatabaseGnutls.

>  * Certificate handles created using the default GTlsDatabase will be
>    different before and after this commit. This seems unlikely to cause
>    problems in practice, since a quick Debian codesearch reveals zero
>    applications using our certificate handles.

Yeah, I'm not aware of anyone ever having used any of the GTlsDatabase lookup APIs.

(In reply to Dan Winship from comment #29)
> A workaround for both of these issues would be to keep
> --with-ca-certificates, and continue using a non-system-trust-based database
> if that was passed. (Though you'd want to do *something* to make sure people
> didn't keep using it when they *should* be using the system trust. So maybe
> keep the behavior but rename the flag?)

Well the flag is already renamed to ca_certificates_path, and the build will just fail if it's not specified. When creating the meson build system, neither Inigo nor myself considered that it was OK to just leave this unset.

I think it's perfectly reasonable to just inherit the system trust from GnuTLS.

But fixing Flatpak is actually a bit harder than I expected, because I haven't been able to successfully build the runtime. And you're right, keeping ca_certificates_path around for a little longer would help with that. But I think I'd rather patch that at the Flatpak runtime level, instead.

> >  * The default GTlsDatabase is a GTlsFileDatabase, but it might not
> >    actually correspond to a file anymore, since GnuTLS may be configured
> >    to use p11-kit for its default trust store. This is a bit weird.
> > 
> >  * If the anchors property of a GTlsFileDatabase is NULL, that
> >    previously indicated an empty GTlsFileDatabase, but now indicates
> >    that the system trust is used.
> 
> If the database is not backed by a file, it shouldn't be a GTlsFileDatabase.

Probably....

> I think this is easily fixed though: just move 95% of the
> GTlsFileDatabaseGnutls code into its parent class, GTlsDatabaseGnutls, and
> then have the "system trust" database be a plain GTlsDatabaseGnutls rather
> than a GTlsFileDatabaseGnutls.

Yeah... the reason I didn't do this was because of GTlsDatabaseGnutlsPkcs11, which I'd rather not touch or break. But I guess I need to learn how that works.

I think you could just make GTlsDatabaseGnutlsPkcs11 be a direct subclass of GTlsDatabase rather than being a subclass of GTlsDatabaseGnutls, and nothing would care/notice.

(In theory, if you have the trust-list-walking code now, GTlsDatabaseGnutlsPkcs11 can be turned into mostly a no-op, by using gnutls's built-in pkcs11 support rather than calling out to p11kit. But if you have no way of testing the end result then leaving the old code there and making it its own class is easier.)

Review of attachment 368195 [details] [review]:

This fixes all the review issues I had, but I’ll leave acking it to Dan, since he’s got a good idea of the architectural issues here (and I don’t).

(In reply to Philip Withnall from comment #32)
> Review of attachment 368195 [details] [review] [review]:
> 
> This fixes all the review issues I had, but I’ll leave acking it to Dan,
> since he’s got a good idea of the architectural issues here (and I don’t).

I need to update it in response to Dan's feedback.

Created attachment 369301 [details] [review]
Make GTlsDatabaseGnutlsPkcs11 inherit directly from GTlsDatabase

This is pretty weird, because GTlsDatabaseGnutlsPkcs11 is no longer a
GTlsDatabaseGnutls, but this is not meant to be the final state of
things. This is just to avoid breaking GTlsDatabaseGnutlsPkcs11 with the
massive changes that are about to land to GTlsDatabaseGnutls.

Created attachment 369302 [details] [review]
Use the GnuTLS system trust by default

Get rid of the ca_certificates_path build flag. The default GTlsDatabase
will now use the GnuTLS system trust. GTlsFileDatabase now builds its
internal hash tables by iterating its gnutls_x509_trust_list_t, rather
than by parsing its certificate file manually.

The find-ca-certificates script is removed, since it's no longer needed.

There are some potential compatibility risks here:

 * The minimum required GnuTLS version is increased from 3.3.5 to 3.4.

 * If GnuTLS is not configured with a system trust, all certificate
   verification using the default GTlsDatabase will fail. I noticed that
   GNOME's flatpak runtime does not configure a system trust, so this
   breaks HTTPS there. This is sad for Epiphany Technology Preview, but
   we should do this anyway, and separately fix the GNOME runtime.

 * It was previously possible to configure glib-networking using
   --without-ca-certificates to ensure the default GTlsDatabase is
   empty (GNOME #727282). Apparently this was desirable on some embedded
   systems, though I'm not sure why. Such configuration is still
   possible by configuring GnuTLS with no system trust. Presumably,
   anybody relying on this behavior will notice that the
   --without-ca-certificates flag has disappeared during the build
   system change and investigate, so this seems unproblematic.

 * The default GTlsDatabase is a GTlsFileDatabase, but it might not
   actually correspond to a file anymore, since GnuTLS may be configured
   to use p11-kit for its default trust store. This is a bit weird.

 * If the anchors property of a GTlsFileDatabase is NULL, that
   previously indicated an empty GTlsFileDatabase, but now indicates
   that the system trust is used. NULL is documented to be an allowed
   value, but the effects of setting the property to NULL are not
   documented. We should document it. Arguably, it would be safer to
   create a new property to indicate that the system trust should be
   used. But I think we can get away with this, because this change only
   affects applications that were intentionally creating useless
   GTlsFileDatabases with no certificates, which seems unlikely.

 * Certificate handles created using the default GTlsDatabase will be
   different before and after this commit. This seems unlikely to cause
   problems in practice, since a quick Debian codesearch reveals zero
   applications using our certificate handles. But, if an application
   were to rely on handles generated by previous versions of
   glib-networking being valid in the new version, it would break.

So there are actually several risks here. Still, none of the above seem
*likely* to cause practical issues, once distributors ensure that GnuTLS
 is built properly, so I think we can proceed.

Created attachment 369303 [details] [review]
Move most of GTlsFileDatabaseGnutls up into GTlsDatabaseGnutls

Now that we use the GnuTLS system trust by default, it is likely that
the default GTlsDatabase is no longer backed by a file at all, since the
GnuTLS system trust uses the p11-kit trust store on Red Hat
distributions. But it *might* still be a file, which is still the case on
Debian, and when using the Freedesktop 1.6 SDK.

As suggested by Dan Winship, we can avoid this semantic difficulty by
moving pretty much all of the GTlsFileDatabaseGnutls implementation up
to GTlsDatabaseGnutls, and allowing instantiation of GTlsDatabaseGnutls.
GTlsDatabaseGnutls will now be used directly for the default
GTlsDatabase. The remaining code in GTlsFileDatabaseGnutls is just
enough to deal with the anchor_file property.

I'll push these soon unless there are more review comments. I've already branched glib-2-56 and am not inclined to mess around anymore there, so we'll have the next six months to notice any regressions.

Review of attachment 369303 [details] [review]:

Looks reasonable to me, although as I said before, I know nothing of the right architecture here, and I haven’t done a line-by-line review of this patch.

::: tls/gnutls/gtlsbackend-gnutls.c
@@ +151,3 @@
       if (error)
         {
+          g_warning ("couldn't load TLS file database: %s", error->message);

Why rewrap this line? Seems like unnecessary noise in the patch.

Review of attachment 369302 [details] [review]:

Assuming there are no changes from the previous version of the patch (it would be helpful if you listed any), r+.

Review of attachment 369301 [details] [review]:

Are you planning to revert this at some point, or something?

Review of attachment 369303 [details] [review]:

::: tls/gnutls/gtlsbackend-gnutls.c
@@ +151,3 @@
       if (error)
         {
+          g_warning ("couldn't load TLS file database: %s", error->message);

It's just unnecessary noise; I can get rid of it.

(In reply to Philip Withnall from comment #39)
> Review of attachment 369302 [details] [review] [review]:
> 
> Assuming there are no changes from the previous version of the patch (it
> would be helpful if you listed any), r+.

No changes here. I should update the commit message before landing to indicate that the GNOME SDK has been fixed, though.

(In reply to Philip Withnall from comment #40)
> Review of attachment 369301 [details] [review] [review]:
> 
> Are you planning to revert this at some point, or something?

There are two possible futures:

 * Change GTlsDatabaseGnutlsPkcs11 to again inherit from GTlsDatabaseGnutls, removing redundant code and fixing the same problems outlined in this bug that were previously fixed for GTlsFileDatabaseGnutls.
 * Just get rid of the PKCS#11 backend, bug #793281.

The later seems reasonable because (a) I'm not aware of anyone using the PKCS#11 backend, which is never enabled unless a special environment variable is manually exported, and (b) after this change, the normal backend now supports PKCS#11 if enabled in GnuTLS.

(In reply to Michael Catanzaro from comment #42)
>  * Change GTlsDatabaseGnutlsPkcs11 to again inherit from GTlsDatabaseGnutls,
> removing redundant code and fixing the same problems outlined in this bug
> that were previously fixed for GTlsFileDatabaseGnutls.

That option is bug #793281

(In reply to Michael Catanzaro from comment #43)
> That option is bug #793281

Sorry: that option is bug #753260.

(In reply to Michael Catanzaro from comment #35)
>  * The default GTlsDatabase is a GTlsFileDatabase, but it might not
>    actually correspond to a file anymore, since GnuTLS may be configured
>    to use p11-kit for its default trust store. This is a bit weird.
> 
>  * If the anchors property of a GTlsFileDatabase is NULL, that
>    previously indicated an empty GTlsFileDatabase, but now indicates
>    that the system trust is used. NULL is documented to be an allowed
>    value, but the effects of setting the property to NULL are not
>    documented. We should document it. Arguably, it would be safer to
>    create a new property to indicate that the system trust should be
>    used. But I think we can get away with this, because this change only
>    affects applications that were intentionally creating useless
>    GTlsFileDatabases with no certificates, which seems unlikely.

I don't want to squash these commits together, because that would make it harder to see the effective changes in this patch. But I will update the commit message. These problems are avoided by the final commit in the series, so that should be mentioned and they don't need to be documented so extensively.

(In reply to Michael Catanzaro from comment #37)
> I'll push these soon unless there are more review comments. I've already
> branched glib-2-56 and am not inclined to mess around anymore there, so
> we'll have the next six months to notice any regressions.

OK, let's do this!

Attachment 369301 [details] pushed as 8f8f47b - Make GTlsDatabaseGnutlsPkcs11 inherit directly from GTlsDatabase
Attachment 369302 [details] pushed as f1c8fee - Use the GnuTLS system trust by default
Attachment 369303 [details] pushed as 75fd268 - Move most of GTlsFileDatabaseGnutls up into GTlsDatabaseGnutls