Created attachment 276094 [details] [review]
load certificate chains

(In reply to comment #0)
>    g_tls_certificate_new_from_file
>        (this one now just calls g_tls_certificate_new_from_pem)

just realized that g_tls_certificate_new_from_file() was already calling g_tls_certificate_new_from_pem(), as it should. so, forget this little note.

Ping.

I was thinking that if we don't modify the current behavior of the functions, we could have new functions:

   g_tls_certificate_chain_new_from_pem
   g_tls_certificate_chain_new_from_file
   g_tls_certificate_chain_new_from_files

But I think the modification I proposed makes sense. The only reason for keeping the old behavior is if you have a file with a bunch of certificate and you simply want the first one without having to load the rest (with g_tls_certificate_list_new_from_file). But I've never seen such a use case.

Comment on attachment 276094 [details] [review]
load certificate chains

OK, so I think we can do this, but the version in this patch isn't quite backward-compatible enough.

>They work as before if only one certificate is found in the file. If
>more than one certificate is found it will try to load the chain.
>
>It is assumed that the chain will be in the right order. If the chain
>cannot be verified an error will be returned.

You can't do that; the current documentation explicitly states that "If @data includes multiple certificates, only the first one will be parsed.". Meaning that we have to still allow passing files that contain multiple unrelated certificates, and just ignore the other certificates in that case.


> static GTlsCertificate *
> g_tls_certificate_new_internal (const gchar  *certificate_pem,
>-				const gchar  *private_key_pem,
>-				GError      **error)
>+				const gchar      *private_key_pem,
>+				GTlsCertificate  *issuer,
>+				GError          **error)

you need to adjust the spacing on the first line to line "*certificate_pem" up with the other lines too

>+static GTlsCertificate *
>+parse_and_create_certificate (const gchar *data,
>+                              gsize        data_len,
>+                              const gchar *key_pem,
>+                              GError      **error)

the first three lines need an extra space to line up the arguments correctly

>+              /* We need to free the loaded certificate as it has no
>+               * issuer and create a new one with the issuer. */

(Another style nitpick: put the "*/" of a comment on the next line.)

It seems like it would be easier to process the list of certs in reverse order; first create the last one, with no issuer, then create the second-to-last one, using the last one as its issuer, and so on. Then when you're done, see if it verifies, and if not, throw away the chain and reparse just the first certificate, with no issuer.

>+    g_list_free_full (queue.head, g_free);

There's g_queue_free_full(). (But if you do it in reverse order, you don't need a GQueue anyway.)


Also, test cases! (in glib-networking)

(In reply to comment #5)

thanks for all the comments! i just started vacations today and i'll be out for a week and a half or so. i'll rework the patch just went i get back.

if you could have a look to the other patches while i'm out it would be really great, so i could work on them too if needed.

thanks again!

Created attachment 282188 [details] [review]
add support for certificate chains

This patch should fix all your comments. The old behaviour of the functions has been kept. The only change is that it now tries to load a certificate chain first.

Created attachment 282189 [details] [review]
certificate chain unit tests

Added new intermediate CA and server certificates plus unit tests to verify the new certificate chain loading.

Any comments on this one? I'm ready to commit ;-).

Comment on attachment 282188 [details] [review]
add support for certificate chains

>+g_tls_certificate_new_internal (const gchar      *certificate_pem,

>+			 "issuer", issuer,
> 			 NULL);
>+
>+  if (issuer)
>+    g_object_unref (issuer);

No, don't do that from the new() function; that's contrary to how other GLib APIs work. You need to have create_certificate_chain_from_list() unref it itself after calling g_tls_certificate_new_internal().

>+      cert_pem = parse_next_pem_certificate (&p, end, required, error);
>+
>+      if (error && *error)

You can't do that; that means the behavior of the function will differ depending on whether the caller passed a GError** or not.

Also, this still has the wrong semantics in the case where a file contains one valid certificate and then half of an invalid certificate; parse_next_pem_certificate() will return an error ("Could not parse PEM-encoded certificate") rather than ignoring the bad second cert.

What you want to do is:

  - if parse_next_pem_certificate() returns NULL on the first certificate, return NULL
    (and error will be set if the caller passed in a GError, but you don't need to check
    that).

  - After parsing the first certificate, don't pass error to the remaining
    parse_next_pem_certificate() calls (since if they return an error, you don't want to
    propagate it to the caller). Also, if you get a failure after the first cert, you
    want to return a list containing just the first certificate, NOT the list of all
    certs up to that point.

>+      /* last will point to the issuer of the first certificate. */
>+      last = cert;

That comment is wrong.

>+      cert = g_tls_certificate_new_internal (pem->data, key, cert, error);
>+
>+      if (error && *error)

Again, don't depend on whether the caller provided a GError**. Check whether cert is NULL instead.

>+      /* We couldn't verify the certificate chain but we still need to
>+       * return the first certificate in the file.
>+       */

Given that the function is called "create_certificate_chain_from_list()", it seems like it would make more sense to return NULL here if the certificate doesn't verify, and then have the fallback in parse_and_create_certificate().

>+ * @data. If @data contains more certifcates it will try to load a

typo "certifcates" (in all three functions)

Comment on attachment 282189 [details] [review]
certificate chain unit tests

skimming quickly, this looks good. you might want to add a case for the bug I pointed out in the previous comment (eg, read the file in, truncate the data right before the final "END CERTIFICATE" tag, and make sure that parsing it with g_tls_certificate_new_from_pem() returns the first certificate, not an error.)

Created attachment 284971 [details] [review]
add support for certificate chains fixup

Hi Dan. As usual, thanks for the great review. Here's a new patch with all the comments address, I think. Let me know how it looks now.

Created attachment 284972 [details] [review]
certificate chain unit tests fixup

Updated UT with a the new test to check for truncated PEM file.

Created attachment 284973 [details] [review]
add support for certificate chains fixup 2

Didn't save a last change (forgot to free a list). Updated patch attached.

Comment on attachment 284973 [details] [review]
add support for certificate chains fixup 2

Looks good; just two comments about comments:

>+  /* Verify the certificate chain. If it doesn't verify, we will have to
>+     return the first certificate.
>+   */

That comment is wrong now; you're returning NULL.

>+ * The returned certificate will be the first certificate found in
>+ * @data. If @data contains more certificates it will try to load a
>+ * certificate chain. All certificates will be verified in the order

I should have thought of this before, but this should say "As of GLib 2.42, if @data contains more certificates, ..." to make it clear that this is new behavior which can't be relied on if you need to support older releases. Likewise in the other doc comments.

Comment on attachment 284972 [details] [review]
certificate chain unit tests fixup

good

So I'm not sure about committing this at this point; it's effectively new API (though GLib is technically not subject to API freeze), and could *theoretically* break existing code that was for some reason expecting to get back a GTlsCertificate with no issuer property in this case.

Matthias?

(If this doesn't get committed now, then the comments about mentioning "GLib 2.42" in the doc comments should obviously be updated to "GLib 2.44" instead.)

Created attachment 285581 [details] [review]
add support for certificate chains fixup 3

Thank you Dan. This patch updates the comments as suggested. By now, I have assumed GLib 2.42.

Created attachment 286927 [details] [review]
add support for certificate chains glib 2.44

GLib 2.42 was released, so just changed the documentation to mention 2.44.

Pushed.

glib:

commit da053e345b4729a8a166eca54da257ae9accc7b1
Author: Aleix Conchillo Flaqué <aconchillo@gmail.com>
Date:   Mon Oct 6 10:19:48 2014 -0700

    tlscertificate: add support for certificate chains
    
    This patch changes the behavior of the following functions:
    
       g_tls_certificate_new_from_pem
       g_tls_certificate_new_from_file
       g_tls_certificate_new_from_files
    
    If more than one certificate is found it will try to load the chain.
    
    It is assumed that the chain will be in the right order (top-level
    certificate will be the last one in the file). If the chain cannot be
    verified, the first certificate in the file will be returned as before.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=729739


glib-networking:

commit dc1ffeee2e166c6129285127124bc50878c17815
Author: Aleix Conchillo Flaqué <aconchillo@gmail.com>
Date:   Mon Oct 6 10:22:32 2014 -0700

    tests: added certificate chain unit tests
    
    We add a new intermediate CA and a new server certificate signed by this
    new CA. Unit tests verify that the chain is loaded successfully and that
    the old behavior is kept by loading a file with an invalid chain.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=729739

Review of attachment 284972 [details] [review]:

Pushed.

Review of attachment 286927 [details] [review]:

Pushed.

Ah, Aleix, Dan, are you sure it's a good idea to verify the chain of certificates within create_certificate_chain_from_list()? Isn't it likely that the caller would expect to receive the full chain of certificates and then verify the chain himself if desired? It sure would be nice to have a set of functions in glib that load an arbitrary certificate chain without doing certificate verification. It's not very intuitive that these functions return one certificate that may or may not validate, followed by others if and only if the entire chain validates, and I don't see how receiving one unverified certificate is better than receiving a chain of unverified certificates when attempting to load a chain that doesn't check out.

So I would just cut out that one step....

If the user is expecting a chain he can use the new behavior without having to do anything else. I would say this is the most common use.

If the user just wants the list of certificate in the file, then he just uses g_tls_certificate_list_new_from_file.

My initial idea was to return NULL in the case the chain could not be verified. This would solve you concern. But Dan made the point of not breaking the old functionality.

In any case, if the user is expecting a chain and the chain is not verified, he can check for the issuer to be present. I agree this is not very intuitive though.

I still think returning NULL makes sense, even breaking old behavior. I don't think it's very common to have a file with multiple certificates and only load the first one (which is what the old function did). May be I'm wrong here.

Dan?

Hm... verification is perhaps a bit too strong. What I wanted is just to make sure that the certificates actually do form a chain (ie, it's not just a random group of certificates stored in the file together). But we should return the whole chain even if, say, one of the certs is expired.

So that would mean changing the g_tls_certificate_verify() part of create_certificate_chain_from_list() to only fail if flags contains G_TLS_CERTIFICATE_UNKNOWN_CA; other verification errors would just be ignored at this step.

Does that make sense?

> I still think returning NULL makes sense, even breaking old behavior.

We can't break old behavior

(In reply to comment #24)
> If the user is expecting a chain he can use the new behavior without having to
> do anything else. I would say this is the most common use.

Well g_tls_certificate_verify() is going to verify the entire chain when used, right?

(In reply to comment #25)
> So that would mean changing the g_tls_certificate_verify() part of
> create_certificate_chain_from_list() to only fail if flags contains
> G_TLS_CERTIFICATE_UNKNOWN_CA; other verification errors would just be ignored
> at this step.

Even this seems arbitrary to me. The function returns a certificate list on Thursday, but then on Friday your system gets a ca-certificates update that removes a root, now the function returns only a single cert with the same file? I don't see why an import function should be performing any verification at all.

OK, so use case: I want to be able to load whatever PEM I want, representing multiple certificates that may or may not form an ordered chain and certainly may not verify, into a single GTlsCertificate, so that it can be sent to a client via GTlsServerConnection. g_tls_certificate_list_new_from_file() returns a GList, which isn't helpful because GTlsServerConnection sends a single GTlsCertificate.

(In reply to comment #26)
> (In reply to comment #24)
> > If the user is expecting a chain he can use the new behavior without having to
> > do anything else. I would say this is the most common use.
> 
> Well g_tls_certificate_verify() is going to verify the entire chain when used,
> right?
> 

Yes, that would do it.

> (In reply to comment #25)
> > So that would mean changing the g_tls_certificate_verify() part of
> > create_certificate_chain_from_list() to only fail if flags contains
> > G_TLS_CERTIFICATE_UNKNOWN_CA; other verification errors would just be ignored
> > at this step.

Yes, this seems confusing to me too.

> 
> Even this seems arbitrary to me. The function returns a certificate list on
> Thursday, but then on Friday your system gets a ca-certificates update that
> removes a root, now the function returns only a single cert with the same file?
> I don't see why an import function should be performing any verification at
> all.
> 

The problem is that GTlsCertifacte issuer property says:

  "A GTlsCertificate representing the entity that issued this certificate. If 
   NULL, this means that the certificate is either self-signed, or else the 
   certificate of the issuer is not available."

So, if we remove the internal verify, we would return something that is not right according to the documentation.

Probably, g_tls_certificate_verify was meant to verify a GTlsCertificate against the database, assuming that its internal chain would be correct. This is just a guess though.

> OK, so use case: I want to be able to load whatever PEM I want, representing
> multiple certificates that may or may not form an ordered chain and certainly
> may not verify, into a single GTlsCertificate, so that it can be sent to a
> client via GTlsServerConnection. g_tls_certificate_list_new_from_file() returns
> a GList, which isn't helpful because GTlsServerConnection sends a single
> GTlsCertificate.

Because of what I said above. In this case you would need to build an "invalid" GTlsCertificate on your own with the GList that you got from g_tls_certificate_list_new_from_file.

So, I think it is fine like it is now. Except, sorry for being stubborn here, that I'd return NULL in case a chain doesn't verify (may be in 3.0?). Completely agree it's not possible now. I guess in 3.0 we can break things a bit? Don't even know when this will happen. This would follow the documentation of GTlsCertificate and it would not be arbitrary any more.

If a user wants to build a GTlsCertificate with an unordered chain or even unrelated chain, she can still do it writing some bit of extra code.

> (In reply to comment #25)
> > So that would mean changing the g_tls_certificate_verify() part of
> > create_certificate_chain_from_list() to only fail if flags contains
> > G_TLS_CERTIFICATE_UNKNOWN_CA; other verification errors would just be ignored
> > at this step.
> 
> Even this seems arbitrary to me. The function returns a certificate list on
> Thursday, but then on Friday your system gets a ca-certificates update that
> removes a root, now the function returns only a single cert with the same file?
> I don't see why an import function should be performing any verification at
> all.

No, g_tls_certificate_verify() doesn't verify against the system ca-certificates. (You need a GTlsDatabase function for that.) It just considers the "trusted_ca" argument you give it to be a valid CA, and checks if the "cert" argument is (directly or indirectly) signed by it. So as I said before, we'd only be checking that the certificates actually form a chain, but not that the chain is valid or complete.

> OK, so use case: I want to be able to load whatever PEM I want, representing
> multiple certificates that may or may not form an ordered chain and certainly
> may not verify, into a single GTlsCertificate, so that it can be sent to a
> client via GTlsServerConnection.

Why would you want to do that? And at any rate, that's not what a GTlsCertificate is. If you want to have multiple unrelated certificates, you have to have either a list/array of certificates, or else a GTlsDatabase.

(In reply to comment #28)
> Why would you want to do that? 

To write tests for bug #683266.

(In reply to comment #27)
> Because of what I said above. In this case you would need to build an "invalid"
> GTlsCertificate on your own with the GList that you got from
> g_tls_certificate_list_new_from_file.

Good point. That should work for me.

(In reply to comment #25)
> So that would mean changing the g_tls_certificate_verify() part of
> create_certificate_chain_from_list() to only fail if flags contains
> G_TLS_CERTIFICATE_UNKNOWN_CA; other verification errors would just be ignored
> at this step.

^ So this is the only action item left, correct?

Created attachment 289538 [details] [review]
GTlsCertificate: fix loading of bad certificate chains

g_tls_certificate_new_from_file() was only loading the complete chain
if it was fully valid, but we only meant to be validating that it
formed an actual chain (since the caller may be planning to ignore
other errors).

(In reply to comment #31)
> Created an attachment (id=289538) [details] [review]
> GTlsCertificate: fix loading of bad certificate chains
> 
> g_tls_certificate_new_from_file() was only loading the complete chain
> if it was fully valid, but we only meant to be validating that it
> formed an actual chain (since the caller may be planning to ignore
> other errors).

Looks good to me.

Attachment 289538 [details] pushed as 982d0e1 - GTlsCertificate: fix loading of bad certificate chains