GNOME Bugzilla – Bug 636572
GTlsCertificateDB
Last modified: 2011-08-11 20:47:19 UTC
currently there's an implicit concept of a certificate database lurking in gio tls (for CAs at least), but nothing exposed explicitly. We need to eventually have an explicit certdb, in particular so that apps can look up client certificates from it (which may involve using PKCS#11 to retrieve them from a smart card, or from gnome-keyring).
I've been thinking about this a bit... I just finished implementing the certificate chain lookups and trust assertion work in empathy and libgcr, so I'm full of the subject :) I think that following virtual methods something like the following would be needed on GTlsCertificateDB: void build_chain_async (GTlsCertificateDB *self, GTlsCertificate* input, const gchar *hostname, guint flags_reserved, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); @input contains the list of certificates recieved from the server. @hostname is used to lookup pinned certificates. gboolean build_chain_finish (GTlsCertificateDB *self, GAsyncResult *result, GTlsCertificate **chain, GPtrArray **anchors, GTlsCertificateChainStatus *status, GError *error); @chain contains a complete proper certificate chain, with the DB possibly filling in missing certificates. @anchors contains one or more anchor certificates (in the case of the /etc/ssl/certs based GTlsCertificateDB, it would return all the possible anchors, in the case of a smarter backend, it would return only the relevant anchor for @output). @status contains some flags as to what kind of chain is being returned. See #GcrCertificateChainStatus for an idea of the chain statuses. But basically the built certificate chain can be (anchored, pinned, explicitly distrusted, selfsigned). This is separate from then verifying the chain with gnutls. With the above style interface we get pinned certificates (ie: per-host certificate exceptions) and CRL support pretty much for free. In addition, it goes without saying that the GTlsCertificate::verify would need an async friend. WDYT? For more info about building certificate chains and chain statuses: http://people.collabora.co.uk/~stefw/gcr-docs/GcrCertificateChain.html And more on on how it relates to trust assertions: http://people.collabora.co.uk/~stefw/trust-assertions.html
(In reply to comment #1) > @input contains the list of certificates recieved from the server. @hostname is > used to lookup pinned certificates. Not hostname-and-port? Also, what about SRV lookups? I've been using GSocketConnectable to represent identity for that reason; it can be either a GNetworkAddress or a GNetworkService, and we can DTRT. (Trusting the http certificate on http://gmail.com is different from trusting the xmpp certificate at the server identified by SRV _xmpp-client._tcp.gmail.com.) > @anchors contains one or more anchor > certificates (in the case of the /etc/ssl/certs based GTlsCertificateDB, it > would return all the possible anchors, in the case of a smarter backend, it > would return only the relevant anchor for @output). There's no reason the "dumb" certdb needs to be that dumb though is there? It can look at the chain and see what CA it's signed by, and find that CA in its list and return just that. > In addition, it goes without saying that the GTlsCertificate::verify would need > an async friend. g_tls_certificate_verify() is pretty much a bad hack, and if we have a public GTlsCertificateDB type then it will probably go away. Anyway, I am not totally sure we need to split up the "build a chain" and "verify the chain" operations in the API. It's true that in the backend the first half would be implemented by talking to gnome-keyring and the second half by calling gnutls methods, but that doesn't matter to the caller. Is there any case where an application would need to do one but not the other? It seems like having a "verify this certificate without checking for CRLs" API is just inviting people to shoot themselves in the foot... So we just need g_tls_certificate_db_build_chain_and_verify{,_async,_finish}?
(In reply to comment #2) > Not hostname-and-port? Also, what about SRV lookups? I've been using > GSocketConnectable to represent identity for that reason; it can be either a > GNetworkAddress or a GNetworkService, and we can DTRT. (Trusting the http > certificate on http://gmail.com is different from trusting the xmpp certificate > at the server identified by SRV _xmpp-client._tcp.gmail.com.) I guess we need to decide what the to pin certificates on. But passing a GSocketConnectable here makes complete sense, and then let the db figure out which part to use. > There's no reason the "dumb" certdb needs to be that dumb though is there? It > can look at the chain and see what CA it's signed by, and find that CA in its > list and return just that. Well I figured we could have a "dumb" generic certdb in glib (rather than glib-networking). If you didn't want to 'do' ASN.1 and DER then it would need to be quite dumb. But I see below, you'd like to have the certdb do the verify step too (not only build) so then it would be tied to a particular ssl implementation like gnutls, and wouldn't need to be dumb at all. > Anyway, I am not totally sure we need to split up the "build a chain" and > "verify the chain" operations in the API. It's true that in the backend the > first half would be implemented by talking to gnome-keyring and the second half > by calling gnutls methods, but that doesn't matter to the caller. Is there any > case where an application would need to do one but not the other? It seems like > having a "verify this certificate without checking for CRLs" API is just > inviting people to shoot themselves in the foot... > > So we just need g_tls_certificate_db_build_chain_and_verify{,_async,_finish}? Yes, that makes sense.
Did a bunch of work and pushed it to the tls-database branches of glib and glib-networking. Added GTlsDatabase abstract base class. With the following virtual methods (and async cousins): GList* (*build_chain_and_verify) (GTlsDatabase *self, GList *input, GSocketConnectable *identity, gint flags, GCancellable *cancellable, GTlsCertificateFlags *verify_result, GError **error); GList* (*lookup_certificates) (GTlsDatabase *self, GTlsDatabaseLookup lookup_type, gconstpointer identifier, gsize n_identifier, GCancellable *cancellable, GError **error); Question, should we call "build_chain_and_verify" simply "verify_chain" with the side effect that it builds a more complete chain when and where necessary? As you'll notice the above use GList as a chain instead of GTlsCertificate::issuer based chains. It turns out that with the latter it is very hard to modify a chain, you essentially end up doing a deep copy of the certificates. Should we have an object that represents a chain, or just use GList? build_chain_and_verify returns the actual chain that was verified, which may be shorter or longer than the original. I wonder if the resulting chain should contain the anchor certificate or not. In RFC 5280 it says that the anchor certificate is not considered part of the chain. However I can't help but think that it would be useful to have the entire chain validated present for display purposes and the like. FWIW, the anchor is almost never sent from the server on the other end. However, when using gnutls_x509_crt_list_verify it's pretty hard to identify which anchor actually was the one that was used by the chain, when multiple anchor certificates have the same DN. This is of course bad practice, so it's a very small corner case. lookup_certificates() returns a list of certificates from the database. The content of @identifier depends on the @lookup_type parameter. When lookup issuer certificates @identifier will be set to the DER value of the issuer DN. When looking up private keys/certs it would be set to the DER value of the subject of the issuing CA. So it's a bit extensible in that regard. Also implemented abstract class GTlsDatabaseGnutls, which implements build_chain_and_verify(). It will be further subclassed by GTlsFileDatabaseGnutls and GTlsPkcs11DatabaseGnutls. Let me know if things are going in the right direction.
(In reply to comment #4) > Did a bunch of work and pushed it to the tls-database branches of glib and > glib-networking. cool. commenting without actually looking at the code yet... > GList* (*build_chain_and_verify) (GTlsDatabase *self, > GList *input, > GSocketConnectable *identity, > gint flags, > GCancellable *cancellable, > GTlsCertificateFlags *verify_result, > GError **error); "input" is a bad name, and "flags" (if it's going to be there) should be an enum type, even if the only value currently-defined is "NONE = 0". Do you have any specific ideas about flags that might be needed, or is it just a generic future-ass-covering move? > Question, should we call "build_chain_and_verify" simply "verify_chain" with > the side effect that it builds a more complete chain when and where necessary? Yes. Also, I think the verify_flags should be the return value, not the chain. > As you'll notice the above use GList as a chain instead of > GTlsCertificate::issuer based chains. It turns out that with the latter it is > very hard to modify a chain, you essentially end up doing a deep copy of the > certificates. Should we have an object that represents a chain, or just use > GList? I don't think having a GTlsCertificateChain object makes sense. We should either keep :issuer, or else drop it and use GLists everywhere. I avoided GList because it's less type-safe, especially with properties (although you *can* specify the element-type and memory management conventions of a GList property via introspection annotations, so maybe it doesn't really matter). Also, it's nice that you can treat GTlsConnection:peer-certificate as being either a single certificate, or a chain, depending on what you need. Since all the issuer-modification would happen internal to the backend, it could be done via private GTlsCertificateGnutls methods, leaving :issuer still read-only at the level of the public API... > However I can't help but think that it would be useful to have the entire chain > validated present for display purposes and the like. Agreed. > GList* (*lookup_certificates) (GTlsDatabase *self, > GTlsDatabaseLookup lookup_type, > gconstpointer identifier, > gsize > n_identifier, > GCancellable *cancellable, > GError **error); > Is @identifier an array, or is @n_identifer mis-named? (@identifier_length?) GTlsDatabaseLookup should be GTlsDatabaseLookupType or GTlsDatabaseIdentifierType or something. > lookup_certificates() returns a list of certificates from the database. The > content of @identifier depends on the @lookup_type parameter. When lookup > issuer certificates @identifier will be set to the DER value of the issuer DN. > When looking up private keys/certs it would be set to the DER value of the > subject of the issuing CA. So it's a bit extensible in that regard. ISTR we need to be using the serial number in one of these lookups as well? Will we want some sort of "find client certificates matching these CertificateRequest params (key type, algos, CAs)" option? I guess that would have to be a separate method probably. > Also implemented abstract class GTlsDatabaseGnutls, which implements > build_chain_and_verify(). It will be further subclassed by > GTlsFileDatabaseGnutls and GTlsPkcs11DatabaseGnutls. Btw we were talking about not being able to do full pkcs11 in gnutls 2.10, but if we've got our own code for talking to the pkcs11 backend, then does gnutls_sign_callback_set() get us the rest of the way? I know that was originally added for pkcs11-related reasons.
Made the following changes: * Rename build_chain_and_verify() to verify_chain() * Rename @input to @chain and modify @chain in verify_chain * Return verify result from verify_chain * Add typedef GTlsDatabaseVerifyFlags * GTlsDatabaseLookup -> GTlsDatabaseLookupType * Use GTlsCertificate as chain instead of GList > Do you have > any specific ideas about flags that might be needed, or is it just a generic > future-ass-covering move? The following flag has been floating around in my head, but yes ass covering is also a nice side-effect. G_TLS_DATABASE_VERIFY_IGNORE_PINNED /* eg: when used from GTlsServerConnectionGnutls */ > Since all the issuer-modification would happen internal to the backend, it > could be done via private GTlsCertificateGnutls methods, leaving :issuer still > read-only at the level of the public API... I chose this option. Nice and clean. > > lookup_certificates() returns a list of certificates from the database. The > > content of @identifier depends on the @lookup_type parameter. When lookup > > issuer certificates @identifier will be set to the DER value of the issuer DN. > > When looking up private keys/certs it would be set to the DER value of the > > subject of the issuing CA. So it's a bit extensible in that regard. > > ISTR we need to be using the serial number in one of these lookups as well? > > Will we want some sort of "find client certificates matching these > CertificateRequest params (key type, algos, CAs)" option? I guess that would > have to be a separate method probably. Hmmm, I was hoping we could cram all this into the single lookup_certificates() method with varying GTlsDatabaseLookupType values. However that concept only works when there's a single argument per lookup type (ie: the identifier). Should we just add a new method for each use case? > Btw we were talking about not being able to do full pkcs11 in gnutls 2.10, but > if we've got our own code for talking to the pkcs11 backend, then does > gnutls_sign_callback_set() get us the rest of the way? I know that was > originally added for pkcs11-related reasons. Interesting, I'll have to look into that. BTW, where were you thinking to have the GTlsDatabase pointer. A property on GTlsConnection or on GTlsBackend? And how would an application specify which database they wanted to use, since the app doesn't have the glib-networking headers? Should there be different backends registered like so? "gnutls+file" "gnutls+pkcs11"
I've implemented initial version of GTlsFileDatabaseGnutls in glib-networking tls-database branch. Not yet hooked up to anything.
G_TLS_DATABASE_VERIFY_NOFLAGS should be _NONE, and it should be defined in gioenums.h. fix up alignment of params, etc in both .h and .c Needs gtk-docs. Should g_tls_database_lookup_issuer() try calling g_tls_certificate_get_issuer() first? For the async data, don't use g_object_set_data(); use g_simple_async_result_set/get_op_res_gpointer(). (That's what it's there for.) I think we probably need a GTlsFileDatabase interface. And then add vmethods to GTlsBackend for get_default_database_type() and get_file_database_type(). Actually, maybe the former should just be get_default_database(), returning a GTlsDatabase, not a GType. gtlsfiledatabase.h would define "g_tls_file_database_new(const char *ca_file)", which would look up the correct type in the default GTlsBackend, and then g_object_new() a database of the correct type, passing in the ca_file as a property. (This would be used for situations like libsoup's SoupSession:ssl-ca-file property, where for backward compat you need to be able to allow the caller to specify an explicit file-based CA list rather than using the default system database.) GTlsConnection:use-system-certdb should become GTlsConnection:tlsdb or something, and have a GTlsDatabase value. It would be initialized to the default db, but callers could override it, setting it to a filedb, etc.
Thinking some more... I think we can have GTlsFileDatabase be non-backend-specific, and just defer to the default database for the hard parts. Eg, its verify_chain implementation would look something like: chain = build_chain_from_my_database (filedb, chain, &found_an_anchor); flags = g_tls_database_verify_chain (g_tls_database_get_default (), chain, identity, flags, cancellable, error); if (found_an_anchor) flags &= ~G_TLS_DATABASE_UNKNOWN_CA; else flags |= G_TLS_DATABASE_UNKNOWN_CA; return flags; (or maybe "flags | G_TLS_DATABASE_VERIFY_IGNORE_PINNED" ?) This would require that the "real" database implementation still checked expiration, algorithms, etc, even if it couldn't validate the CA, and we'd probably need to add a few methods like "g_tls_certificate_get_issuer_dn()" to be able to implement the helper methods. Code style note on method implementations: the wrapper function (eg, g_tls_database_verify_chain()) does all the g_return_if_fail()s. The implementations in each class (g_tls_database_gnutls_verify_chain()) do not need to.
(In reply to comment #8) > G_TLS_DATABASE_VERIFY_NOFLAGS should be _NONE, and it should be defined in > gioenums.h. Done. But FWIW, G_TLS_DATABASE_VERIFY_NONE looks like a flags that says no verification should be performed. > fix up alignment of params, etc in both .h and .c Done for glib, will work on glib-networking. > Needs gtk-docs. Done for glib. > Should g_tls_database_lookup_issuer() try calling > g_tls_certificate_get_issuer() first? I don't think so, as in this case we're asking the database to lookup the issuer, which may return a different certificate that may have been set as the issuer property on GTlsCertificate. The question remains as to whether it should *set* the issuer property of the passed in certificate or not. It currently doesn't. > For the async data, don't use g_object_set_data(); use > g_simple_async_result_set/get_op_res_gpointer(). (That's what it's there for.) Done, not sure what I was thinking here... > I think we probably need a GTlsFileDatabase interface. And then add vmethods to > GTlsBackend for get_default_database_type() and get_file_database_type(). > Actually, maybe the former should just be get_default_database(), returning a > GTlsDatabase, not a GType. gtlsfiledatabase.h would define > "g_tls_file_database_new(const char *ca_file)", which would look up the correct > type in the default GTlsBackend, and then g_object_new() a database of the > correct type, passing in the ca_file as a property. (This would be used for > situations like libsoup's SoupSession:ssl-ca-file property, where for backward > compat you need to be able to allow the caller to specify an explicit > file-based CA list rather than using the default system database.) > > GTlsConnection:use-system-certdb should become GTlsConnection:tlsdb or > something, and have a GTlsDatabase value. It would be initialized to the > default db, but callers could override it, setting it to a filedb, etc. Sounds like a good plan.
(In reply to comment #9) > Thinking some more... I think we can have GTlsFileDatabase be > non-backend-specific, That's what I tried initially too, but in order to do things like lookup the issuer we need to parse the certificates in the CA file. But with the right properties on GTlsCertificate it could work. > (or maybe "flags | G_TLS_DATABASE_VERIFY_IGNORE_PINNED" ?) Yes, or perhaps G_TLS_DATABASE_VERIFY_NO_LOOKUPS which would signify just to verify what's already in the chain, and not to try to do any database lookups. > This would require that the "real" database implementation still checked > expiration, algorithms, etc, even if it couldn't validate the CA, and we'd > probably need to add a few methods like "g_tls_certificate_get_issuer_dn()" to > be able to implement the helper methods. Right. > Code style note on method implementations: the wrapper function (eg, > g_tls_database_verify_chain()) does all the g_return_if_fail()s. The > implementations in each class (g_tls_database_gnutls_verify_chain()) do not > need to. Roger. Work on this may slow down a bit for the next few weeks, as I'm preparing to move, and away on a couple trips. But I'll see if I have a bit more time to squeeze in before that.
Pushed a bunch more code: * Fix preconditions in on virtual methods. In some cases preconditions have to stay in the implementation function, because we need to check things only the backend knows. * Hooked GTlsDatabase::verify_chain() into implementation and ran some basic tests to make sure it works with GFileDatabaseGnutls * GTlsConnection::database is a GTlsDatabase which defaults to backend default but can be overriden or set to NULL (for no database verification) * Dummy GTlsDatabase implementation. * Rough untested and not totally complete PKCS#11 implementation. * Created GFileDatabase interface with a single property. Open: * Need to move gnutls handshake stuff into a thread. * Didn't put a GFileDatabase implementation in glib yet. I don't really understand the reasoning of why we can't just have the implementation in the backend? Once we have key selection, signing callbacks, and other stuff using the backend then I'm not sure having GFileDatabase in glib will be so easy. But this is just a feeling. * Need to add GTlsCertificate::issuer-dn and GTlsCertificate::subject-dn * GFileDatabase needs to have multiple constructor functions. eg: a TLS server may want to specify a revocation list file. I noticed there's no real test suite that I can tell which exercises this stuff. I think that's worth investing some time in.
This will miss glib 2.28, but I believe the plan is to have glib 2.30 with GNOME 3.0.
Added lots of test today to the tls-database which exercise various parts of the functionality. And will be adding more. The tests are already paying off and among other things I found two memory leaks. One where GTlsConnection is essentially never freed. Created a new gnutls-pkcs11 backend which uses the GTlsPkcs11DatabaseGnutls by default. Did research on using gnutls_sign_callback_set() to support PKCS#11 based client certificates using the database. This however seems like a deadend. Not only is it not completely implemented (in gnutls), but future versions of gnutls deprecate support for gnutls_sign_callback_set() altogether. I believe that integration with the pkcs11 support in the upcoming gnutls 2.12 is where we should be headed instead.
As posted on the mailing list, I've completed the initial set of work for this. And I'll organize into patches for merging. Unless I hear back, I'll organize it like so: 1. - All glib GTlsDatabase abstract functionality. * http://cgit.collabora.com/git/user/stefw/glib.git/log/?h=tls-database - File database implementation in glib-networking. * http://cgit.collabora.com/git/user/stefw/glib-networking.git/log/?h=tls-database 2. - "gnutls-pkcs11" backend implementing PKCS#11 database lookup for trust assertions (like certificate authorities and pinned certificates). * Parts of: http://cgit.collabora.com/git/user/stefw/glib-networking.git/log/?h=tls-pkcs11 3. - Extending the "gnutls-pkcs11" backend to support client certificates in PKCS#11 (such as smart cards). * This depends on patches merged into gnutls and backported to gnutls 2.12, so may need to wait until later. I'll be working on that. * Parts of: http://cgit.collabora.com/git/user/stefw/glib-networking.git/log/?h=tls-pkcs11 I'll file new dependent bugs for the latter two parts on Monday. Does that sound like a good approach? And now some open questions: * The GTlsDatabase::unlock-required signal doesn't have the best name. The database emits this signal to prompt the user for PINs/passwords, which unlock things. But there's no 'unlocked' and 'locked' database states so this could get confusing. If we're being picky, we'd want to come up with a better name. * I don't like the name gtls_database_lookup_issued and friends. Might need a better name. Some comments: * The reason that GTlsUnlock is an object is for two reasons: - It can handle further extensions of information needed for the unlock prompts (such as perhaps the URI of the token being unlocked). - The memory for the password is allocated and deallocated by the signal handling code, so it can choose the right kind of free function (think non-pageable memory, if that's what's desired). * There's currently no provision for using more than one GTlsDatabase, although support for this could be added later if necessary. * We need to rework how the handshaking in the gnutls backend works so that it can block (while prompting for PINs or performing smart card operations). This is related to bug #637257, and the various blocking issues could be worked on together once this is merged. * There's the concept of a 'handle' string for a certificate that originated from a database. This allows client code to save away a reference to a certificate, in settings or pass it between apps where necessary. It's not mandatory that all databases support these handles. But more importantly these handles are what we use to pass references to keys and certificates that live on smart cards (in PKCS#11) to gnutls and tell it which ones to use.
(In reply to comment #15) > * The GTlsDatabase::unlock-required signal doesn't have the best name. or GTlsUnlock. I'd go for something like GTlsDatabase:password-required, and GTlsPasswordRequest. > * I don't like the name gtls_database_lookup_issued and friends. Might need a > better name. what is that API needed for anyway? > * The reason that GTlsUnlock is an object is for two reasons: > - It can handle further extensions of information needed for the unlock > prompts (such as perhaps the URI of the token being unlocked). see also bug 651277 > * There's currently no provision for using more than one GTlsDatabase, that seems fine by me > * There's the concept of a 'handle' string for a certificate that originated > from a database. This allows client code to save away a reference to a > certificate, in settings or pass it between apps where necessary. It's not > mandatory that all databases support these handles. It needs to be mandatory that all GTlsDatabase implementations support handles. Otherwise apps can't use the API for anything. > But more importantly > these handles are what we use to pass references to keys and certificates > that live on smart cards (in PKCS#11) to gnutls and tell it which ones to > use. Right, but that is entirely an internal implementation detail; from the app's point of view, a GTlsCertificate is a GTlsCertificate, whether it's backed by a file or by a hardware token.
notes on the glib bits: +typedef enum { + G_TLS_DATABASE_VERIFY_NONE = 0, +} GTlsDatabaseVerifyFlags; Are there any specific flags you expect to need in the future? If not, I'd drop this. - * GTlsConnection:use-system-certdb: + * GTlsConnection:database: You can't get rid of API that has already shipped. You have to just deprecate it and explain how it relates to the new API. eg, setting use-system-certdb to TRUE would be equivalent to setting database to g_tls_backend_get_default_database(), and setting it to FALSE would be equivalent to setting database to NULL. +/* GIO - GLib Input, Output and Certificateing Library oops. my bad originally in gtlscertificate.c, but you copied it into gtlsdatabase.c + * The most common operation with a %GTlsDatabase is using the + * g_tls_database_verify_chain () to build up and verify a chain of should probably say that most client applications will not directly interact with GTlsDatabase at all. + * Since: 2.28 fix those (some are correct, some say 2.28) +gchar* +g_tls_database_create_handle (GTlsDatabase *self, missing docs + * g_tls_database_lookup_handle: It's not looking up a handle, it's looking up a certificate, so it should be "lookup_certificate_by_handle" or "lookup_certificate". Also, the doc comment has two references to "issuer" presumably from cut+paste. +#define G_TLS_DATABASE_PURPOSE_SERVER_AUTH "1.3.6.1.5.5.7.3.1" +#define G_TLS_DATABASE_PURPOSE_CLIENT_AUTH "1.3.6.1.5.5.7.3.2" I find it confusing that the client uses "SERVER_AUTH" and the server uses "CLIENT_AUTH". "AUTHENTICATE_SERVER" and "AUTHENTICATE_CLIENT" would be less ambiguous. (I also wonder if having this be a string rather than an enum is overkill...) + * GTlsFileDatabase:anchor-file: We want to allow for the possibility of an "anchor directory" as well (bug 636571). So maybe just "anchor", and allow for the possibility that in the future it could be a directory name rather than a filename. +tls_tests = \ These should actually go in glib-networking, not glib, so that they can be run as part of "make check" without creating dependency loops.
(In reply to comment #16) > (In reply to comment #15) > > * The GTlsDatabase::unlock-required signal doesn't have the best name. > > or GTlsUnlock. I'd go for something like GTlsDatabase:password-required, and > GTlsPasswordRequest. A similar thing is GMountOperation that was originally for asking for passwords during mounting. This was later extended to also be used for unmount() (to show a list of processes using a volume). I doubt that you need something like this... but it could be that you want to add other authenticated operations later... and then it's annoying having to add new authentication objects just because you named the existing one after the "unlock" operation. On that basis, I'd suggest to call it GTlsOperation with GTlsDatabase:authentication-required which, thanks to being more abstract, can be extended to also be used in other operations. Also, it's probably not far fetched that you can use something else than a passphrase to unlock the secrets needed (think two-factor or other out-of-band authentication)... one way to do that is to add a conversation-style interface like the one we've already have in GMountOperation, see http://developer.gnome.org/gio/unstable/GMountOperation.html which, in my view is a bit too... concrete... There's also e.g. PolkitAgentSession http://hal.freedesktop.org/docs/polkit/PolkitAgentSession.html which basically just mirrors current PAM (not perfect either). FWIW, you could even use GMountOperation here and then you'd get all the UI for free... e.g. GtkMountOperation and there' also an implementation that speaks to the Shell process for nice shell dialogs, cf. http://blogs.gnome.org/cosimoc/2011/06/23/hotplug-hotness/ ... the naming isn't perfect though. For GLib 3.0 we might want to refactor all this into a GOperation type. I don't know.
Hmmm, very interesting. Good points. I like the way that GMountOperation doesn't block. That's a plus. On the other hand the name isn't very descriptive. But the way that its a caller implemented is a good idea. Yes there are other concrete use cases for user interaction: When using smart cards, you often want to ask the user to "Insert the Smart Card called 'XXXX'. The word 'authentication' means something completely different in TLS. Using it to mean user authentication would be confusing. So maybe I'll go with GTlsUserInteraction. We should probably take a cue from GMountOperation and fire appropriate signals on the GTlsUserInteraction object itself. The remaining open question is a choice: a) Have g_tls_database_set_user_interaction(). This amounts to having a global user interaction for all objects. But a single implementor can cover all TLS connections in a process. b) Have g_tls_connection_set_user_interaction(). This lets callers have a different user interaction per connection, but means more book keeping for callers, hard to configure a default or have one implementor just make all TLS connections user interaction work.
Btw, I forgot to mention GDBusAuthObserver which is sorta-kina similar API http://developer.gnome.org/gio/unstable/GDBusAuthObserver.html but doesn't really interact with the user (although it could).. anyway, that API-style was chosen because it's extensible and you could imagine querying the user for a pass-phrase. (In reply to comment #19) > Hmmm, very interesting. Good points. > > I like the way that GMountOperation doesn't block. That's a plus. On the other > hand the name isn't very descriptive. But the way that its a caller implemented > is a good idea. > > Yes there are other concrete use cases for user interaction: When using smart > cards, you often want to ask the user to "Insert the Smart Card called 'XXXX'. > > The word 'authentication' means something completely different in TLS. Using it > to mean user authentication would be confusing. So maybe I'll go with > GTlsUserInteraction. I like that name. > We should probably take a cue from GMountOperation and fire appropriate signals > on the GTlsUserInteraction object itself. > > The remaining open question is a choice: > > a) Have g_tls_database_set_user_interaction(). This amounts to having a global > user interaction for all objects. But a single implementor can cover all > TLS > connections in a process. > > b) Have g_tls_connection_set_user_interaction(). This lets callers have a > different user interaction per connection, but means more book keeping for > callers, hard to configure a default or have one implementor just make all > TLS connections user interaction work. I don't think a) works... imagine having two separate TLS library users in a app that don't know about each other.. one wanting to interact in a modal-dialog in for the app and one wanting to interact via the Shell interface. It's also not very good to have global state in a library... Regarding the default... I think we should just have a GtkTlsUserInteraction type in libgtk+ just like we already have a GtkMountOperation. We might also want a GnomeShellTlsUserInteraction type (similar to the GnomeShellMountOperation type) but I don't know where it would live...
Global state won't really work because of situations where you have multiple tls-using plugins that don't communicate with each other, inside an app that is itself tls-unaware (eg, pidgin used to be this way, not sure if it still is). > b) Have g_tls_connection_set_user_interaction(). This lets callers have a > different user interaction per connection, but means more book keeping for > callers, hard to configure a default or have one implementor just make all > TLS connections user interaction work. But it's only callers using client certificates that need to deal with this though, right? For libsoup, the way I imagine something like this working would be to have SoupSession have a "tls-user-interaction" property. So the application would create an appropriate GTlsUserInteraction object, pass it to soup_session_async_new(), and then libsoup would set it on any GTlsConnections it created. That's not too much bookkeeping. And thinking about that, it seems like GTlsUserInteraction should also cover the case of *selecting* a client certificate, not just unlocking it after it's been selected. And maybe also deciding whether or not to accept an otherwise-unacceptable peer certificate? (In which case it would actually be needed even by non-client-cert-using apps, but it still seems like a good idea.)
That makes sense. Thinking about it more, the problem is that it's not how TLS libraries generally work. They generally have somewhat global callbacks for prompting for PINs and stuff. Last time I looked NSS uses global user password prompting hooks globally. I've done some work on gnutls to try and make it so we can use more fine grained callbacks for prompting, but relying on this may make it hard to do other TLS backends. So this is a plus in favor of having (at least) the password interaction stuff on GTlsDatabase. It ends up modelling what's actually going on. As far as the certificate selector, I think it could make to put a default certificate selector in the gcr library, since it has certificate display widgets. A downside to having all these user interaction requests on the same object is that they all have to be implemented in the same place. Not a major problem as long as there don't end up being too many of these. When there's just a few, you could implement them like so: GTlsUserInteraction (abstract) |_ GtkTlsUserInteraction |_ GcrTlsUserInteraction BTW, I'd really like to see us moving away from dialog spam that is 'accept certificate?' dialogs. In general, I think that if there's a out of the ordinary certificate to be trusted, it should be configured as part of the account/connection configuration in the app. But I certainly agree with the point that we need more types of user interaction. On another topic, the GMountOperation has a really weird API. There must be a good reason, but I can't figure out why. A signal is emitted on the GMountOperation to make a request (like 'ask-password'), and then when the derived GMountOperation has a response, it replies with the 'reply' signal. This would seem to make it a non-blocking API, but for the fact that the default handler for the various request signals (like 'ask-password') sets up an idle handler which replies with G_MOUNT_OPERATION_UNHANDLED. So the signal handler can't return and then reply later from the main loop. Am I reading this wrong? In addition each signal can have multiple handlers, and we're not using accumulators in the GMountOperation signals, so there's no way to coordinate which handler is going to reply. Each request can easily have multiple reply signals. Quite strange, especially if multiple user interactions are needed for a single TLS connection or such Why not just use standard gio async IO? Unless there's some strange advantage to using signals like that, I won't do it that way in GTlsUserInteraction.
BTW, I just checked and NSS supports: * Per socket client certificate callback, both func and data are per socket (SSL_AuthCertificateHook) * Global PIN callback func with per socket data (PK11_SetPasswordFunc, SSL_SetPKCS11PinArg) So yeah we can probably cobble together something so that GTlsUserInteraction is per connection if like David suggested on IRC, we assume that Glib is the 'only' thing using NSS.
I've cleaned up the glib abstract implementation of this stuff based on feedback. I haven't yet made the matching changes to glib-networking. This glib part is ready for review again. As part of this I've rebased the branch on master. Still at the same place though. Will attach a squashed patch. http://cgit.collabora.com/git/user/stefw/glib.git/log/?h=tls-database (In reply to comment #16) > (In reply to comment #15) > > * The GTlsDatabase::unlock-required signal doesn't have the best name. > > or GTlsUnlock. I'd go for something like GTlsDatabase:password-required, and > GTlsPasswordRequest. As discussed above, I've redesigned this around GTlsInteraction. As David pointed out, the interaction isn't always with a user (although this is often the case). For example a caller could lookup passwords in some other storage, or cache them somehow. So that's why the class name doesn't have 'user' in there. The GTlsInteraction methods are be based around 'gasync' style stuff, slightly modified (eg: no GCancellable since that doesn't make sense). GTlsConnection has an interaction property, and each GTLsDatabase virtual method accepts an interaction argument. Then there's GTlsPassword which represents a password to be used with TLS. As noted above, having this as an object allows us to do two things: 1) extend it, hopefully we can add more attributes about what a password can be used for, which then could be used to lookup passwords where this is desired, 2) allow for stronger guarantees about where passwords are stored in memory, like non-pageable memory. The GTlsPassword can then be used for future stuff (like support SRP if those patches show up, bug 651277 and all that). > > * I don't like the name gtls_database_lookup_issued and friends. Might need a > > better name. > > what is that API needed for anyway? It's used to lookup certificates issued by an authority. Basically you take the output of g_tls_client_connection_get_accepted_cas() and use it to lookup certificates in the database that could be used. I've renamed the lookup functions in GTlsDatabase: - g_tls_database_create_certificate_handle() ... and friends. - g_tls_database_lookup_certificate_for_handle() ... and friends. - g_tls_database_lookup_certificate_issuer() ... and friends. - g_tls_database_lookup_certificates_issued_by() ... and friends. > > * There's the concept of a 'handle' string for a certificate that originated > > from a database. This allows client code to save away a reference to a > > certificate, in settings or pass it between apps where necessary. It's not > > mandatory that all databases support these handles. > > It needs to be mandatory that all GTlsDatabase implementations support handles. > Otherwise apps can't use the API for anything. True, although apps have to be aware that not any certificate can have a handle created for it. For example a certificate that didn't originate in the database, but was created from a file. > > > But more importantly > > these handles are what we use to pass references to keys and certificates > > that live on smart cards (in PKCS#11) to gnutls and tell it which ones to > > use. > > Right, but that is entirely an internal implementation detail; from the app's > point of view, a GTlsCertificate is a GTlsCertificate, whether it's backed by a > file or by a hardware token. Yes, true, with the above caveat. (In reply to comment #17) > notes on the glib bits: > > +typedef enum { > + G_TLS_DATABASE_VERIFY_NONE = 0, > +} GTlsDatabaseVerifyFlags; > > Are there any specific flags you expect to need in the future? If not, I'd drop > this. Yes, once we have an NSS backend I'd expect a flag to go here that says whether to use OSCP (and other remote host lookups). And probably other flags... > - * GTlsConnection:use-system-certdb: > + * GTlsConnection:database: > > You can't get rid of API that has already shipped. You have to just deprecate > it and explain how it relates to the new API. eg, setting use-system-certdb to > TRUE would be equivalent to setting database to > g_tls_backend_get_default_database(), and setting it to FALSE would be > equivalent to setting database to NULL. Yeah, sorry. Those commits were done before before 2.28 was released. Marked deprecated and fixed. > +/* GIO - GLib Input, Output and Certificateing Library > > oops. my bad originally in gtlscertificate.c, but you copied it into > gtlsdatabase.c Fixed. > + * The most common operation with a %GTlsDatabase is using the > + * g_tls_database_verify_chain () to build up and verify a chain of > > should probably say that most client applications will not directly interact > with GTlsDatabase at all. Done. > + * Since: 2.28 > > fix those (some are correct, some say 2.28) Should all be fixed. > +gchar* > +g_tls_database_create_handle (GTlsDatabase *self, > > missing docs Added. > + * g_tls_database_lookup_handle: > > It's not looking up a handle, it's looking up a certificate, so it should be > "lookup_certificate_by_handle" or "lookup_certificate". Also, the doc comment > has two references to "issuer" presumably from cut+paste. Changed name, as noted above. > +#define G_TLS_DATABASE_PURPOSE_SERVER_AUTH "1.3.6.1.5.5.7.3.1" > +#define G_TLS_DATABASE_PURPOSE_CLIENT_AUTH "1.3.6.1.5.5.7.3.2" > > I find it confusing that the client uses "SERVER_AUTH" and the server uses > "CLIENT_AUTH". "AUTHENTICATE_SERVER" and "AUTHENTICATE_CLIENT" would be less > ambiguous. Done. > (I also wonder if having this be a string rather than an enum is > overkill...) I'd like to leave this as a string, as I think that we'll need extensible purposes in the future. For example with stuff like XMPP E2E encryption, we probshould be using the a different purpose (the same on both sides) both sides. > + * GTlsFileDatabase:anchor-file: > > We want to allow for the possibility of an "anchor directory" as well (bug > 636571). So maybe just "anchor", and allow for the possibility that in the > future it could be a directory name rather than a filename. Good plan, renamed it to 'anchors'. > +tls_tests = \ > > These should actually go in glib-networking, not glib, so that they can be run > as part of "make check" without creating dependency loops. Alright, removed from glib, and will add to glib-networking.
Created attachment 191923 [details] [review] Updated patch for GTlsDatabase and related Squashed version of: http://cgit.collabora.com/git/user/stefw/glib.git/log/?h=tls-database
Created attachment 191940 [details] [review] Updated glib-networking patch to match glib changes Updated to match the glib changes. This contains the general gnutls TLS backend, but no PKCS#11 code or anything fancy like that. Also moved the tests here as requested above. However glib-networking needs to be installed in the prefix for the tests to work. This is because, I can't figure out how to have glib *only* load a certain module path for an extension point.
Comment on attachment 191923 [details] [review] Updated patch for GTlsDatabase and related > gtlsserverconnection.c \ >+ gtlspassword.c \ the unlock -> password rename left this misalphabetized (both .c and .h, and in gio.h) >+ * GTlsConnection:database: >+ * >+ * The certificate database to use when verifying this TLS connection. >+ * If no cerificate database is set, then the default database will be ^^ typo >+ g_return_if_fail (!database || G_IS_TLS_DATABASE (database)); "database != NULL", not "!database" in g_return_if_fail()s. (everywhere) >+/** >+ * g_tls_interaction_ask_password: It is probably worth pointing out explicitly that this function may be called in an arbitrary thread, and must invoke its callback from the same thread. I think there needs to be a synchronous version of this API. It would be somewhat awkward to use from a synchronous GTlsConnection operation otherwise... Are you planning to implement GtkTlsInteraction? It would be good to see at least a proof of concept of that, to make sure the sync/async/threads/contexts stuff will all work out right. >+/** >+ * GTlsPassword: 1) There does not appear to be anything TLS-specific about this... maybe give it a more generic name? 2) I'm not convinced the combination of flags and description really gives the UI code what it wants... eg, what exactly do you say in the case of G_TLS_PASSWORD_FINAL_TRY? "If you get this password wrong again then... something bad will happen... with... that place that is asking about the password...". Another reason it would be good to see an implementation of GtkTlsInteraction. (Of course, it is possible that fixing (2) will invalidate (1).) Is there a reason why it is an interface? It seems like it could be implemented as a concrete object in glib.
(In reply to comment #27) > (From update of attachment 191923 [details] [review]) > > gtlsserverconnection.c \ > >+ gtlspassword.c \ > > the unlock -> password rename left this misalphabetized (both .c and .h, and in > gio.h) Done. > > >+ * GTlsConnection:database: > >+ * > >+ * The certificate database to use when verifying this TLS connection. > >+ * If no cerificate database is set, then the default database will be > ^^ typo > > >+ g_return_if_fail (!database || G_IS_TLS_DATABASE (database)); > > "database != NULL", not "!database" in g_return_if_fail()s. (everywhere) Done, and fixed other uses of boolean logic on pointers. > >+/** > >+ * g_tls_interaction_ask_password: > > It is probably worth pointing out explicitly that this function may be called > in an arbitrary thread, and must invoke its callback from the same thread. Do you mean "must invoke its callback from the thread-default main context"? Essentially like what g_simple_context_async_complete_in_idle() does? > I think there needs to be a synchronous version of this API. It would be > somewhat awkward to use from a synchronous GTlsConnection operation > otherwise... Yes, true. I'm trying to understand how such a synchronous API would be implemented. It would need to account for whether its being called from within a mainloop iteration or on a different thread. Because in one case it would just run a modal dialog, and in the latter case it would need to block and call out to the real mainloop. Any suggestions? I guess that's why it's important to implement something like GtkTlsInteraciton. > 2) I'm not convinced the combination of flags and description really gives the > UI code what it wants... eg, what exactly do you say in the case of > G_TLS_PASSWORD_FINAL_TRY? "If you get this password wrong again then... > something bad will happen... with... that place that is asking about the > password...". Another reason it would be good to see an implementation of > GtkTlsInteraction. These are the sorts of messages you get from ATM machines and other smart card readers (in Europe at least). Generally after 3 incorrect PIN tries you card will no longer work. But I agree that it would be hard to write a good message at the higher layers. So I think that then perhaps we should have a warning-message property in addition to the flags. This could be much more user specific. > Is there a reason why it is an interface? It seems like it could be implemented > as a concrete object in glib. True.
(In reply to comment #28) > > "database != NULL", not "!database" in g_return_if_fail()s. (everywhere) > > Done, and fixed other uses of boolean logic on pointers. "if (!database)" is fine everywhere except g_return_if_fail()s. But the general convention is to spell the condition out explicitly in g_return_if_fail(), because that makes the error message when it fails clearer. > > It is probably worth pointing out explicitly that this function may be called > > in an arbitrary thread, and must invoke its callback from the same thread. > > Do you mean "must invoke its callback from the thread-default main context"? > Essentially like what g_simple_context_async_complete_in_idle() does? Yes, assuming that this method is only called for async connections. > > I think there needs to be a synchronous version of this API. It would be > > somewhat awkward to use from a synchronous GTlsConnection operation > > otherwise... > > Yes, true. I'm trying to understand how such a synchronous API would be > implemented. It would need to account for whether its being called from within > a mainloop iteration or on a different thread. Well, if an interactive app is making synchronous calls from the UI thread, then it deserves to lose. So I would say GtkTlsInteraction's synchronous ask_password method can do g_return_val_if_fail(current_thread != main_thread, G_TLS_INTERACTION_UNHANDLED) > These are the sorts of messages you get from ATM machines and other smart card > readers (in Europe at least). Generally after 3 incorrect PIN tries you card > will no longer work. Right, I just meant that the app doesn't know whether to say "you'll be locked out of your smart card" or "you'll be locked out of the GNOME keyring" or what.
Created attachment 192383 [details] [review] Fixed patch for GTlsDatabase and related, with GTlsPassword as concrete object. Made all the other changes, and did some more testing... Completed a GtkTlsInteraction example: http://cgit.collabora.com/git/user/stefw/gtls-frob.git/tree/ Also fixed up the tls-pkcs11 branch to match these changes: http://cgit.collabora.com/git/user/stefw/glib-networking.git/log/?h=tls-pkcs11
(In reply to comment #30) This is great! When can I expect it to be merged into the master? I am working with David Woodhouse to add SSL client certification support in libsoup. Do you think I can start implement it now based on your branch http://cgit.collabora.com/git/user/stefw/glib.git/log/?h=tls-database?
This has been fine tuned, and I think it's ready to go in. Hopefully Dan will sign off on it soon. Looking forward to this being in glib too. BTW, I'm hoping to also make some changes in the way that client certificates are requested from the application.
Rebased glib changes onto master. Fine tuned how GTlsFileDatabase handles look for the gnutls backend in glib-networking, added some tests: http://cgit.collabora.com/git/user/stefw/glib-networking.git/commit/?h=tls-database&id=fa77c107e8a26a680b12d3793d549d1f4587689b
>diff --git a/gio/giotypes.h b/gio/giotypes.h >+typedef struct _GTlsContext GTlsContext; That got removed in master (the type only ever existed in code that never got merged), but you accidentally resurrected it in your patch >diff --git a/gio/gtlsbackend.h b/gio/gtlsbackend.h To preserve ABI compat, get_default_database and get_file_database_type have to be added after the existing methods, and their wrapper functions have to deal with the possibility of them being NULL. >diff --git a/gio/gtlsinteraction.h b/gio/gtlsinteraction.h >+ /* Padding for future expansion */ >+ void (*_g_reserved1) (void); >+ void (*_g_reserved2) (void); Use an array (like in gtlspassword.h) >diff --git a/gio/gtlspassword.h b/gio/gtlspassword.h >+void g_tls_password_take_value (GTlsPassword *password, >+ guchar *value, >+ gssize length, >+ GDestroyNotify destroy); "take" generally means "free this in the usual way when you're done with it". This API is more "set"-like.
I think with those changes, this is ready to go in... I haven't really reviewed the glib-networking parts as much, but any bugs there can be fixed later.
Thanks. I'll make those changes. Dan, Matthias, should I squash this as I merge it, or bring it with all the fiddly development history?
Matthias is on vacation right now, but I'd say squash
Squashed and merged with master for both glib and glib-networking. 'make distcheck' works on both. I have a tls-pkcs11 branch ready as well. However it depends on unreleased changes to gnutls 2.12.x. I think those will be released shortly, and then I'll deal with getting the tls-pkcs11 stuff in.
PKCS#11 related work will be tracked in bug #656361