Bug 38749

I fixed the bug in http://cgit.collabora.com/git/user/bari/wocky.git/log/?h=extra-certificate-identities and http://cgit.collabora.com/git/user/bari/telepathy-gabble.git/log/?h=extra-certificate-identities

In wocky/wocky-tls-handler.c there is this comment:
  /* When lenient, don't check the peername, set cert flags accordingly.
   * When 'strict', leave the flags at NORMAL and check the peername.
   * Under legacy SSL, the connect hostname is the preferred peername.
   * Under STARTTLS, we check the domain regardless of the connect server.
I'm not sure what the comment is talking about to be honest. Was it already outdated before my changes?

With this patches passing talk.google.com in extra-certificate-identities is not needed as it's already the server name. This seems to me like the right behaviour, what do you think?

Wōcky review:

> In this case the
> XMPP server (rightly) provides a certificate for talk.google.com, but
> the JID's domain part is the Google apps domain.

It’s not really “rightly”—the server is not really meant to behave like this. More like, “Rather than providing a certificate for the JID’s domain part, the server provides a certificate for talk.google.com; if the user has explicitly configured a ‘Google Talk’ account, it’s reasonable to accept certificates for this domain.”

Regarding the comment: I think the only part which was inaccurate before your changes was the use of “When lenient” and “When 'strict'” rather than “When ignore_ssl_errors is TRUE” and “Otherwise”. The rest is accurate (though confusingly-worded as if there were four cases rather than two orthogonal pairs of cases): when using legacy TLS you expect the certificate of the domain you connected to (because the server has to present a certificate before you tell it what your domain is); when using STARTTLS your initial, unencrypted <stream:stream> opening includes the domain of your JID so when you do <starttls/> the server knows which domain it should present a cert for (but of course Google ignores this information).

So I think it could do with a bit of rewording in any case, and of course needs updating in the face of these changes: if the user/application has explicitly specified other possible identities, then those are acceptable too.

+      peers = gnutls_certificate_get_peers (session->session, &cls);

'cls' is not a great name for the that variable, compared to something like ‘n_peers’. (“Certificate List Size” is the only expansion I can come up with that makes sense.) This is existing code, so I'm not going to demand it be fixed. (On closer inspection, it's unused in this function—only the bizzare construction ‘&peers[0]’ is used—but GnuTLS, that shining jewel of a library, doesn't let you pass NULL instead …)

-              DEBUG ("gnutls_openpgp_crt_check_hostname: %s -> %d",peername,rval);
+              DEBUG ("gnutls_openpgp_crt_check_hostname: %s -> %d",peername, rval);

I guess this is to fix the coding style by adding a space … could you add another space?

+                  rval = gnutls_openpgp_crt_check_hostname(opgp, peername);

Style: space before the paren plz.

(In reply to comment #2)
> > In this case the
> > XMPP server (rightly) provides a certificate for talk.google.com, but
> > the JID's domain part is the Google apps domain.
> 
> It’s not really “rightly”—the server is not really meant to behave like this.
> More like, “Rather than providing a certificate for the JID’s domain part, the
> server provides a certificate for talk.google.com; if the user has explicitly
> configured a ‘Google Talk’ account, it’s reasonable to accept certificates for
> this domain.”

Same comment on the Gabble commit message. Otherwise that branch looks fine!

Updated. Do you want to take another look?

both look fine, ship 'em!

Pushed to master.