GNOME Bugzilla – Bug 679854
libsecret migration
Last modified: 2012-08-20 16:59:40 UTC
Created attachment 218713 [details] [review] WIP patch to migrate to libsecret libsecret is a new client for the Secret Service DBus API. The Secret Service allows storage of passwords in a common way on the desktop. Supported by gnome-keyring and ksecretservice. libsecret solves many problems with libgnome-keyring. Relevant to gvfs: it solves threading issues, uses GDBus instead of dbus-glib, and makes cancellation cleaner. A future GNOME goal will be to migrate away from libgnome-keyring to libsecret: https://live.gnome.org/GnomeGoals/LibsecretMigration I've done a rough WIP patch in order to make sure that the libsecret API covered all the use cases. I'll attach that patch here. I hope the patch is a help for the migration, but I don't plan to iterate on it at the current time. Some notes about the patch: * gnome-keyring-daemon is autostarted, so I've g_vfs_keyring_is_available() is now hard coded. * I chose an arbitrary schema name for the luks password, you may want to change it to something better: org.gnome.GVfs.Luks.Password * I haven't tested the patch. I'm not familiar with how to trigger all the various code paths and logic. * Since the luks passwords are passwords that shouldn't be paged to disk I've used secret_password_free() on them, this clears the memory before freeing them. * secret_password_remove() removes all unlocked matching passwords as opposed to gnome_keyring_delete_password() which only removed an arbitrary one. I think this is the effect the code wanted to achieve: No invalid results returned from secret_password_store(). But if not, you can use secret_service_search() and secret_item_delete() to effect the old behavior. Note that the patch uses the unstable 'advanced' parts of the libsecret API. In particular, it's not "best practice" to use gnome-keyring as a place to store account or connection details. In an ideal world gnome-keyring would be used just to store secrets, and attributes are used to lookup those secrets. However libsecret supports this use case, but that functionality is not yet absolutely stable. I'm aiming to get most of this stable by GNOME 3.8, but if you do migrate to libsecret before then, I would patch network-manager-applet for any API changes that come up.
er, "I would patch gvfs for any API changes that come up."
The LUKS stuff should probably use the same schema name as the patch in bug #679861. Note that this isn't a hard dependency. With SECRET_SCHEMA_DONT_MATCH_NAME the two can interoperate even if mismatched, or one of the projects is still using libgnome-keyring.
Per r-t: Targetting GNOME 3.6
Created attachment 218792 [details] [review] WIP patch to migrate to libsecret Updated for last minute API change. The above comment should say: * secret_password_clear() removes all unlocked matching passwords as opposed to gnome_keyring_delete_password() which only removed an arbitrary one. I think this is the effect the code wanted to achieve: No invalid results returned from secret_password_store(). But if not, you can use secret_service_search() and secret_item_delete() to effect the old behavior.
BTW, just a heads up: Please look at the patch critically. I did the patch as a way to try out the API, and it's not ready to commit. There may be memory leaks or other logic errors. Most libsecret getters return data that must be unreferenced or freed.
Review of attachment 218792 [details] [review]: ::: daemon/gvfskeyring.c @@ +33,3 @@ { #ifdef HAVE_KEYRING + return TRUE; Is there no way to check at runtime if the daemon is available too? @@ +73,3 @@ + s = g_string_new (NULL); + if (user != NULL) + g_string_append_printf (s, "%s@", user); Maybe these things should be url escaped (for instance, its common in some setups to have usernames that are email addresses (i.e. have @ in them). @@ +102,1 @@ gchar **password_out) At some point we should make this use the appropriate APIs to pass the password securely. @@ +123,3 @@ return FALSE; /* We use the first result, which is the least specific match */ Is this still true with libsecret? I.e. are result ordered by least specific first? @@ +141,2 @@ if (username_out) + *username_out = g_hash_table_lookup (attributes, "user"); Should probably check for attributes != NULL here? @@ +144,2 @@ if (domain_out) + *domain_out = g_hash_table_lookup (attributes, "domain"); And here? @@ +172,3 @@ return FALSE; + keyring = (flags == G_PASSWORD_SAVE_FOR_SESSION) ? SECRET_COLLECTION_SESSION : NULL; Shouldn't this be SECRET_COLLECTION_DEFAULT instead of NULL? ::: monitor/udisks2/gvfsudisks2volume.c @@ +817,3 @@ +#ifdef HAVE_KEYRING + secret_password_free (data->passphrase); + secret_password_free (data->passphrase_from_keyring); This doesn't seem right? We're not looking up via lookup_nonpageable(). Should this not be g_free?
(In reply to comment #6) > Review of attachment 218792 [details] [review]: > > ::: daemon/gvfskeyring.c > @@ +33,3 @@ > { > #ifdef HAVE_KEYRING > + return TRUE; > > Is there no way to check at runtime if the daemon is available too? libsecret supports autostarting the daemon. So you're not sure if it's available until you try. If you really want to see if a secret service daemon is running, you could setup a watch for the well known name: org.freedesktop.secrets > @@ +73,3 @@ > + s = g_string_new (NULL); > + if (user != NULL) > + g_string_append_printf (s, "%s@", user); > > Maybe these things should be url escaped (for instance, its common in some > setups to have usernames that are email addresses (i.e. have @ in them). What would actually be even better is to build a human readable label, rather than building this pseudo-uri thing. > @@ +102,1 @@ > gchar **password_out) > > At some point we should make this use the appropriate APIs to pass the password > securely. http://developer.gnome.org/gcr/stable/GcrSecretExchange.html > @@ +123,3 @@ > return FALSE; > > /* We use the first result, which is the least specific match */ > > Is this still true with libsecret? I.e. are result ordered by least specific > first? Good catch. Not true anymore. We return the most recently modified match first. > @@ +141,2 @@ > if (username_out) > + *username_out = g_hash_table_lookup (attributes, "user"); > > Should probably check for attributes != NULL here? You could, although attributes will only be null if a precondition fails. secret_item_get_attributes() doesn't have 'allow-none' on its return value. And then g_hash_table_lookup() also has a precondition check for NULL. So it's not a big deal to check IMO. There's actually a line further up in the patch that checks whether attributes is NULL, that could be removed. > @@ +172,3 @@ > return FALSE; > > + keyring = (flags == G_PASSWORD_SAVE_FOR_SESSION) ? SECRET_COLLECTION_SESSION > : NULL; > > Shouldn't this be SECRET_COLLECTION_DEFAULT instead of NULL? Sure. That makes it clearer. NULL works as well as SECRET_COLLECTION_DEFAULT in this case though. The @collection parameter of secret_password_storev_sync() is 'allow-none'. > ::: monitor/udisks2/gvfsudisks2volume.c > @@ +817,3 @@ > +#ifdef HAVE_KEYRING > + secret_password_free (data->passphrase); > + secret_password_free (data->passphrase_from_keyring); > > This doesn't seem right? We're not looking up via lookup_nonpageable(). Should > this not be g_free? You can pass both non-pageable memory and normal strings to secret_password_free(). The benefit of using secret_password_free() with passwords in normal memory is that it'll clear them before freeing.
> libsecret supports autostarting the daemon. So you're not sure if it's > available until you try. If you really want to see if a secret service daemon > is running, you could setup a watch for the well known name: > org.freedesktop.secrets That might be nicer, as we then won't ask to store a password when its not possible. */ >> Is this still true with libsecret? I.e. are result ordered by least specific >> first? > Good catch. Not true anymore. We return the most recently modified match first. Then we should probably sort the results manually. The idea is that if you have e.g. passwords for ftp://host.com:666 and ftp://host.com in your keyring, then a match for ftp://host.com would match both (as matching is on a subset to allow e.g. username fill-in), but its likely that what you wanted was the later only. > You can pass both non-pageable memory and normal strings to > secret_password_free(). The benefit of using secret_password_free() with > passwords in normal memory is that it'll clear them before freeing. How does it know what free method to use?
(In reply to comment #8) > > libsecret supports autostarting the daemon. So you're not sure if it's > > available until you try. If you really want to see if a secret service daemon > > is running, you could setup a watch for the well known name: > > org.freedesktop.secrets > > That might be nicer, as we then won't ask to store a password when its not > possible. Right, but you don't actually know if it's possible or not until you try. I guess you could try to autostart the daemon *and* have a watch. > >> Is this still true with libsecret? I.e. are result ordered by least specific > >> first? > > Good catch. Not true anymore. We return the most recently modified match first. Actually to clarify, unlocked results would be returned secret_service_search_sync() and friends. And within the unlocked results, most recently modified is first. > Then we should probably sort the results manually. If you do this make sure that you specify the secret_service_search_sync() SECRET_SEARCH_ALL flag to get all the results, and not just the first (usually unlocked one). > The idea is that if you have e.g. passwords for ftp://host.com:666 and > ftp://host.com in your keyring, then a match for ftp://host.com would match > both (as matching is on a subset to allow e.g. username fill-in), but its > likely that what you wanted was the later only. That's not how gnome_keyring_find_network_password_sync() was called in gvfs. It was called with all the fields set. But you can change the behavior during the port to libsecret to whatever makes the most sense, I guess. Just make sure you sync up the secret_password_store() and secret_password_clear() calls in gvfs, so you store/clear the right items, and don't end up with a user in a frustrated loop because a different password item is stored than is used. > > You can pass both non-pageable memory and normal strings to > > secret_password_free(). The benefit of using secret_password_free() with > > passwords in normal memory is that it'll clear them before freeing. > > How does it know what free method to use? It can tell whether memory is allocated from the non-pageable libsecret allocator. Basically uses the equivalent of gcr_memory_is_secure() internally. http://developer.gnome.org/gcr/unstable/gcr-Non-pageable-Memory.html#gcr-secure-memory-is-secure
>> The idea is that if you have e.g. passwords for ftp://host.com:666 and >> ftp://host.com in your keyring, then a match for ftp://host.com would match >> both (as matching is on a subset to allow e.g. username fill-in), but its >> likely that what you wanted was the later only. > >That's not how gnome_keyring_find_network_password_sync() was called in gvfs. >It was called with all the fields set. Not sure what you mean by that. All arguments are passed in whatever was g_vfs_keyring_lookup_password was passed. Which depends. For instance, take the ftp one: if (g_vfs_keyring_lookup_password (ftp->user, g_network_address_get_hostname (addr), NULL, "ftp", NULL, NULL, port == 21 ? 0 : port, &username, NULL, &password) && It explicitly passes a 0 for the port if its the default of 21 in order to preserve this behaviour.
(In reply to comment #10) > >> The idea is that if you have e.g. passwords for ftp://host.com:666 and > >> ftp://host.com in your keyring, then a match for ftp://host.com would match > >> both (as matching is on a subset to allow e.g. username fill-in), but its > >> likely that what you wanted was the later only. > > > >That's not how gnome_keyring_find_network_password_sync() was called in gvfs. > >It was called with all the fields set. > > Not sure what you mean by that. All arguments are passed in whatever was > g_vfs_keyring_lookup_password was passed. Which depends. For instance, take the > ftp one: > > if (g_vfs_keyring_lookup_password (ftp->user, > g_network_address_get_hostname (addr), > NULL, > "ftp", > NULL, > NULL, > port == 21 ? 0 : port, > &username, > NULL, > &password) && > > > It explicitly passes a 0 for the port if its the default of 21 in order to > preserve this behaviour. Alright, makes sense. Then I guess you want to update build_network_attributes() to not include the attributes in the GHashTable which are either NULL or 0.
Pushed to master