GNOME Bugzilla – Bug 656361
gnutls-pkcs11 based backend for glib-networking
Last modified: 2011-11-04 16:20:17 UTC
We should have a gnutls-pkcs11 based backend for glib-networking. This allows integration with gnome-keyring, smart cards, and even NSS (!) until we have a proper NSS backend in glib. I have a branch and will cleanup/post a patch shortly.
Rebased and available here: http://cgit.collabora.com/git/user/stefw/glib-networking.git/log?h=tls-pkcs11 This adds a dependency on p11-kit (which is already in jhbuild). Possible remaining work: Make GTlsDatabaseGnutlsPkcs11 derive from GTlsFileDatabaseGnutls so that you can use both files and pkcs11 at the same time.
there's no tls-pkcs11 branch in that repo...
Created attachment 194827 [details] [review] Squashed tls-pkcs11 patch Sorry Dan. I must have rebased, and then forgot to push the new one back. It should be there now. The mock stuff doesn't have perfect glib style formatting as it came from another project. I did one or two passes to make it look mostly like glib. Let me know if it's necessary to do the remaining formatting passes. Thanks for looking at all this.
Review of attachment 194827 [details] [review]: OK, I didn't look too close at most of the gpkcs11 stuff, or the tests. Basically this looks good. ::: Makefile.decl @@ +79,3 @@ $(LTP) --directory $(top_builddir) --capture --output-file glib-lcov.info --test-name GLIB_PERF --no-checksum --compat-libtool LANG=C $(LTP_GENHTML) --prefix $(top_builddir) --output-directory glib-lcov --title "GLib Code Coverage" --legend --show-details glib-lcov.info + @echo "file://$(abs_top_builddir)/glib-lcov/index.html" unrelated? ::: configure.ac @@ +17,3 @@ AM_PROG_CC_C_O AC_PROG_CPP +AM_PROG_CC_C_O that check already exists, 2 lines up ::: tls/gnutls/Makefile.am @@ +32,3 @@ +P11_CFLAGS = \ + $(PKCS11_CFLAGS) \ + -DP11_KIT_API_SUBJECT_TO_CHANGE urm... I don't think it's legitimate to have glib depending on an unstable API. Well, I guess this is glib-networking, so maybe it's a *little* better, but... is it possible to declare the current API stable (or at least, supported-but-likely-to-be-deprecated)? ::: tls/gnutls/gtlscertificate-gnutls.c @@ -226,2 +231,2 @@ status = gnutls_x509_privkey_import (gnutls->priv->key, &data, - GNUTLS_X509_FMT_PEM); + GNUTLS_X509_FMT_PEM); gratuitous mysterious tabs-to-spaces change ::: tls/gnutls/gtlsconnection-gnutls.c @@ +510,3 @@ + st->cert.x509[0] = g_tls_certificate_gnutls_copy_cert (gnutlscert, + gnutls->priv->interaction_id); + st->key.x509 = g_tls_certificate_gnutls_copy_key (gnutlscert, maybe it would be better to just have g_tls_certificate_gnutls_copy() that takes a gnutls_retr2_st and fills in the whole thing. Then you don't have the oddity of setting key.x509 to something that's not a gnutls_x509_privkey_t, and relying on the union to make it work. ::: tls/gnutls/gtlsdatabase-gnutls-pkcs11.c @@ +1,3 @@ +/* GIO - GLib Input, Output and Streaming Library + * + * Copyright © 2010 Collabora, Ltd 2011. Likewise in some other files. (Random note: the "©" is legally irrelevant and can be left off. Yes, I know most of the existing files have it.) There are a bunch of places in this file with non-gnu-style braces (ie, "if () {" rather than "if ()\n {"), and a few places where bodies are indented 4 spaces instead of 2. ::: tls/pkcs11/gpkcs11array.c @@ +60,3 @@ + + rarray->attrs = g_realloc (rarray->attrs, + (rarray->len + 1) * sizeof (CK_ATTRIBUTE)); You can use g_renew() there. But really, is there any reason to not just use GArray? (That would change the length field from long to int, but I assume that shouldn't be a problem.) In addition to getting rid of a bunch of code, it would be more memory-efficient when adding several elements, since doesn't realloc every time. ::: tls/pkcs11/gpkcs11pin.c @@ +106,3 @@ + + if (flags & G_TLS_PASSWORD_FINAL_TRY) + return _("This is the last chance to ether the PIN correctly before the token is locked."); s/ether/enter/ ::: tls/tests/mock-pkcs11.c @@ +1,2 @@ +/* + * gnome-keyring no :)
Thanks for the review. Made all the changes, with the notes and exception below. Is this ready to merge once we get the gnutls dependency bumped? (In reply to comment #4) > ::: Makefile.decl > @@ +79,3 @@ > $(LTP) --directory $(top_builddir) --capture --output-file glib-lcov.info > --test-name GLIB_PERF --no-checksum --compat-libtool > LANG=C $(LTP_GENHTML) --prefix $(top_builddir) --output-directory > glib-lcov --title "GLib Code Coverage" --legend --show-details glib-lcov.info > + @echo "file://$(abs_top_builddir)/glib-lcov/index.html" > > unrelated? Moved to another bug. > ::: tls/gnutls/Makefile.am > @@ +32,3 @@ > +P11_CFLAGS = \ > + $(PKCS11_CFLAGS) \ > + -DP11_KIT_API_SUBJECT_TO_CHANGE > > urm... I don't think it's legitimate to have glib depending on an unstable API. > Well, I guess this is glib-networking, so maybe it's a *little* better, but... > > is it possible to declare the current API stable (or at least, > supported-but-likely-to-be-deprecated)? This was an artifact of an old version of p11-kit, back when it was unstable. This define is no longer used. > ::: tls/gnutls/gtlscertificate-gnutls.c > @@ -226,2 +231,2 @@ > status = gnutls_x509_privkey_import (gnutls->priv->key, &data, > - GNUTLS_X509_FMT_PEM); > + GNUTLS_X509_FMT_PEM); > > gratuitous mysterious tabs-to-spaces change > > ::: tls/gnutls/gtlsconnection-gnutls.c > @@ +510,3 @@ > + st->cert.x509[0] = g_tls_certificate_gnutls_copy_cert (gnutlscert, > + > gnutls->priv->interaction_id); > + st->key.x509 = g_tls_certificate_gnutls_copy_key (gnutlscert, > > maybe it would be better to just have g_tls_certificate_gnutls_copy() that > takes a gnutls_retr2_st and fills in the whole thing. Then you don't have the > oddity of setting key.x509 to something that's not a gnutls_x509_privkey_t, and > relying on the union to make it work. True. Good plan. Much cleaner. Changed. In GTlsCertificateGnutlsPkcs11 we call through to the base class to allow it to fill in as much as possible, and then add the PKCS#11 key onto that. > ::: tls/pkcs11/gpkcs11array.c > @@ +60,3 @@ > + > + rarray->attrs = g_realloc (rarray->attrs, > + (rarray->len + 1) * sizeof (CK_ATTRIBUTE)); > > You can use g_renew() there. Done. > But really, is there any reason to not just use > GArray? (That would change the length field from long to int, but I assume that > shouldn't be a problem.) In addition to getting rid of a bunch of code, it > would be more memory-efficient when adding several elements, since doesn't > realloc every time. Yes, because the various array elements own 'deep' copies of the attribute data. So using GArray would mean replacing most of the g_array_ functions we'd use anyway.
Created attachment 194954 [details] [review] Updated with Dan's changes
(In reply to comment #5) > Yes, because the various array elements own 'deep' copies of the attribute > data. So using GArray would mean replacing most of the g_array_ functions we'd > use anyway. ah, basically we would need a GArray equivalent of g_ptr_array_new_with_free_func(). This may be worth adding in the future. For now though, we can stick with the current code.
hey, you gonna commit this at some point? :)
Errr, yes been meaning to... I need to make one last change so that we can work with trust anchors stored in the NSS trust object style as well.
Merged into master. Will file separate bugs for more testing and tweaks.