Bug 38749 - Accept extra certificate identities without relying on an external channel handler
Summary: Accept extra certificate identities without relying on an external channel ha...
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Marco Barisione
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/ba...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2011-06-28 08:10 UTC by Marco Barisione
Modified: 2011-07-07 06:39 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Marco Barisione 2011-06-28 08:10:55 UTC
If you connect to a Google Talk account with a non-gmail.com domain (for instance because you are using Google Apps) then the server will give you the certificate for the normal Google Talk server and not for the alternative domain.

Gabble has a connection parameter, called extra-certificate-identities, that allow you to specify alternative hostnames that we should accept as valid, but the current implementation relies on an external channel handler to do that. We should do it directly in Gabble.
Comment 1 Marco Barisione 2011-06-28 08:18:28 UTC
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?
Comment 2 Will Thompson 2011-06-28 09:52:15 UTC
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.
Comment 3 Will Thompson 2011-06-28 10:07:47 UTC
(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!
Comment 4 Marco Barisione 2011-07-05 10:04:09 UTC
Updated. Do you want to take another look?
Comment 5 Will Thompson 2011-07-05 10:08:16 UTC
both look fine, ship 'em!
Comment 6 Marco Barisione 2011-07-07 06:39:16 UTC
Pushed to master.


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.