Created attachment 86074 [details] [review]
add support for certificate authentication

Add support for the existing certificate request and proposed data signing callbacks for gnutls.

The signing callback patch is here:

http://marc.info/?l=gnutls-dev&m=117614528128374&w=2

I've been successfully using this patch with evolution-exchange for some time now.

Cool.

Is the connector patch available anywhere? (note that bug 273869 is the
connector side of this bug).

Notes on the patch:
    - soup code generally avoids {}s around 1-line if bodies

    - Need gtkdocs (including for the new session signals)

    - Kill soup_ssl_set_authenticate_callback() and
      soup_ssl_set_sign_callback(): just put that all into
      soup_ssl_get_client_credentials() (making them share the same
      user_data and destroy notify).

    - IMHO in the nossl case we should just call the destroy notify
      immediately when the callbacks are set.

    - I think session_to_cred can be replaced with
      gnutls_session_set_ptr / gnutls_session_get_ptr

    - soup_ssl_authenticate_cb and soup_ssl_sign_cb do illegal things
      with GByteArrays. The actual size of a GByteArray is larger than
      sizeof(GByteArray), because glib keeps hidden bookkeeping data
      after the public part of the struct. So cert_ders_ret in
      SoupSSLAuthenticateCallback needs to be a "GByteArray ***"
      (pointer to array of GByteArray *) and sign_data in
      SoupSSLSignCallback needs to be "GByteArray **" (pointer to
      GByteArray *), and the individual GByteArrays need to be freed,
      not just their contents.

Hard questions:

    - Would the new signals/callbacks would work for other SSL libraries?
      (Sun occasionally threatens to submit patches to bring back NSS
      support as an alternative to GnuTLS.) Do they work for cert types
      other than X509?

    - How does this interact with the gnome-keyring/PKCS#11 stuff
      Nate Nielsen has been talking about on ddl?
      http://mail.gnome.org/archives/desktop-devel-list/2007-April/msg00086.html
      http://live.gnome.org/GnomeKeyring/Cryptoki

oops, jacob wasn't cc'ed

(see previous comment)

(In reply to comment #2)
> Is the connector patch available anywhere? (note that bug 273869 is the
> connector side of this bug).

I'm going to be cleaning it up soon (it's split across 4 or 5 patches each to eds and connector).  It is pretty convoluted as it proxies the cert and sign steps over bonobo to the cal and adbk listeners, but it seems to be working ok :/

>     - Kill soup_ssl_set_authenticate_callback() and
>       soup_ssl_set_sign_callback(): just put that all into
>       soup_ssl_get_client_credentials() (making them share the same
>       user_data and destroy notify).

Ok.  I was attempting to remain source compatible but if that's not necessary...

>     - IMHO in the nossl case we should just call the destroy notify
>       immediately when the callbacks are set.

Yes, that is definitely easier.

>     - I think session_to_cred can be replaced with
>       gnutls_session_set_ptr / gnutls_session_get_ptr

Ah; I hadn't noticed this.

>     - soup_ssl_authenticate_cb and soup_ssl_sign_cb do illegal things
>       with GByteArrays.

Only because GByteArrays are lame!  It makes me sad to have to do all of those extra de/allocations, but perhaps not having to make a new ptr/len data type is worth the trouble.

Also because I didn't see that GByteArrays behaved like this until it was a little too late.

> Hard questions:
> 
>     - Would the new signals/callbacks would work for other SSL libraries?
>       (Sun occasionally threatens to submit patches to bring back NSS
>       support as an alternative to GnuTLS.) Do they work for cert types
>       other than X509?

NSS supports an API similar to the current gnutls setup; your callback returns a cert and a privkey.  The problem is that the privkey we get from NSS for smart card certs is somewhat fake: it has a handle to get back to the smart card in order to have the card do the signing, and this doesn't translate to gnutls (or some generic abstraction) very well.

So, if we added a privkey_ret ** to the certificate-request signal, we could work with NSS and gnutls for normal certs, but to do gnutls smart cards we need the extra sign callback, until they have native support for smart cards.

There is another problem: in my current set of patches, the connector calls back to the evo client for the cert, which then handles all of the smart card work, including prompting the user for a pin and (in the future) for which certificate to use.  If gnutls or nss are being used to interface with the smart card directly, the pin needs to get to the connector (which is different from the user's password for that service).

Does this make any sense?

>     - How does this interact with the gnome-keyring/PKCS#11 stuff
>       Nate Nielsen has been talking about on ddl?

The application using soup could use the gnome-keyring PKCS#11 module via NSS just like any other smart card driver to look for certs and keys and do signing.

(In reply to comment #4)
> Ok.  I was attempting to remain source compatible but if that's not
> necessary...

soup-ssl.h is an internal API (it's not installed), so there's no problem.

> So, if we added a privkey_ret ** to the certificate-request signal, we could
> work with NSS

Hm... we'd still need some sort of ask-the-user-for-a-pin callback wouldn't
we? (Oh, I guess you said that later.)

> but to do gnutls smart cards we need
> the extra sign callback, until they have native support for smart cards.

And I see from the mailing list thread you linked to above that the gnutls
guy is planning to implement PKSS#11 soon...

And if gnome-keyring provides a PKCS#11 module for selecting user certs, that
should handle all of the non-smart-card client certificate cases without
needing any new signals from libsoup...

It seems like this patch is the best way to implement things *now*, but it
might not be the best way in the future... So maybe we should just declare
the new signals to be hacks for use by connector only, and not worry about
being clean/general purpose. In that case, you could just export the gnutls
types directly rather than needing to juggle GByteArrays and stuff, and get
rid of anything that isn't being used Right Now (SoupSSLCertificateType, the
destroy notifies, the sign_algos in the authenticate callback/signal, etc).

Then after gnutls and gnome-keyring implement pkcs#11 I can revisit this...

Created attachment 86260 [details] [review]
try #2

how about this.

Created attachment 86447 [details] [review]
patch to create a "SoupSSLCredentials" type (already committed)

Created attachment 86448 [details] [review]
updated patch to add cert support

Here's my version of your patch. This depends on the previous patch (which is
already committed to libsoup trunk), but all of the certificate-related bits
are in this patch.

If this works for you, this is what I'd like to commit.

Comment on attachment 86448 [details] [review]
updated patch to add cert support

>+typedef int (*SoupGnuTLSSignDataFunc) (gnutls_datum_t *cert_der,
>+				       const gnutls_datum_t *hash_data,
>+				       gnutls_datum_t *sign_data,
>+				       gpointer user_data);

cert_der should probably be const here as well.

otherwise i have things in evo building against this patch, and working sometimes, but i don't think the problems i'm hitting are with this patch.

Yes, the bugs were all in my other codes; i think this patch will work out ok.

Based on tonights Epiphany/Webkit/Soup discussion I'd like to know if this patch is still relevant. If so, can it be committed?

If it hasn't been committed, it should still be relevant, AFAIC.

gnutls_certificate_client_set_sign_function still does not exist in the released version of gnutls. Did the gnutls patch get rejected, or implemented some other way?

also, as discussed in comments 4 and 5, the right way to do this is with a more PKCS#11-like API, which would also be more portable to other SSL libraries (which continues to be a possible future issue).

however, if we added a more explicit ssl API as I just mentioned in bug 507801, we could have this just be a part of the SoupSSLClientGNUTLS, and then worry about possible generic implementations later.

This thread may have some more info: http://archive.netbsd.se/?ml=gnutls-dev&a=2007-04&t=3472348

Alon Bar-Lev put his version of gnutls-pkcs11 on http://alon.barlev.googlepages.com/gnutls-pkcs11

gnome-keyring now provides PKCS#11 services, a certificate keyring, and other facilities required to implement SSL/TLS client cert support for IMAP/MAPI/POP3/SMTP . Seahorse doesn't provide keyring management yet, but that appears to be being investigated.

NSS is adding support for a shared certificate store (see https://wiki.mozilla.org/NSS:Roadmap#SQLite-Based_Multiaccess_Certificate_and_Key_Databases) but this won't help GnuTLS-based apps.

Fedora is working on migrating all apps over to NSS, away from GnuTLS and OpenSSL:

http://fedoraproject.org/wiki/FedoraCryptoConsolidation

... so perhaps there's an opportunity for soup there.

GIO TLS (bug 588189) supports client certificates. That will go into glib 2.28 and libsoup will be ported to use it for 2.32/3.0.

(This doesn't yet support the sign_callback/cryptoki stuff, though that will come at some point.)

*** This bug has been marked as a duplicate of bug 634425 ***

unduping this from the generic "gio tls" bug

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

Created attachment 193515 [details] [review]
patch to add SSL client certificate

This is based on glib master code (commit 	d728c00a0488fb2bfbe69e95aba360a552a73d84), which has GTlsDatabase related API ready (see bug https://bugzilla.gnome.org/show_bug.cgi?id=636572#c38 for details). 

When handshake return certificate_requried error, libsoup will look up for the client certificate, destroy and recreate the connection.

However there is a open bug https://bugzilla.gnome.org/show_bug.cgi?id=637257 to change the way certificate request work. It might use GTlsInteraction within the handshake call, and when handshake done, the user already has selected the certificate and sent to the server. If this is implemented, then libsoup will just need add a GTlsInteraction, set it to the connection, and that is done.

The way this patch handle the async lookup certificate operations in a loop might need modification to make sure no concurrent issue. However, the basic idea is there. Please help review it to see if this is the right approach. Thanks!

Should SoupSession:ssl-ca-file be replace by GTlsInteraction? and soup_ssl also needs to modify to reflect the change?

(In reply to comment #21)
> Should SoupSession:ssl-ca-file be replace by GTlsInteraction? and soup_ssl also
> needs to modify to reflect the change?

No, those are different things. Unless you meant, should we replace ssl-ca-file with a GTlsDatabase, in which case, yes. (But we can't really replace it, we can only deprecate the old property and add a new one.)

> No, those are different things. Unless you meant, should we replace ssl-ca-file
> with a GTlsDatabase, in which case, yes. (But we can't really replace it, we
> can only deprecate the old property and add a new one.)
sorry, yes, I mean GTlsDatabase.

Comment on attachment 193515 [details] [review]
patch to add SSL client certificate

marking needs-work, since we do want to make the changes suggested in bug 637257

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

Created attachment 275570 [details] [review]
Add tls-interaction property to Soup{Connection,Session,Socket}

This can be used by applications to do client-side certificates via
the new g_tls_interaction_request_certificate().  Will be used by
OSTree at least.

I ended up landing the SoupSocketProperties work that I'd mentioned, so I had to rework the patch for that. (Though your original patch will still be useful if you're planning to backport this anywhere.)

pushed to master

We also need to be able to specify the client cert in  the form of a PKCS#11 URI. Which presumably depends on bug 750393.