GNOME Bugzilla – Bug 740345
libnm: clean up NMSecretAgent API
Last modified: 2014-11-21 17:24:27 UTC
Three things suck about secret agents right now.. ------ #1) Constructing the GHashTable that you pass back to NMSecretAgent sucks, and you have to pay attention to VPN vs. not VPN Clients have to return a complete dict-in-dict that represents the NMConnection but with only secrets. This requires creating GValues for the secrets because the returned hash is an old-style dict-of-dicts where the values are GValues. Nobody likes doing this :) Furthermore VPN secrets get treated slightly differently because to represent them in an NMConnection it's really a dict-of-dict-of-dict. Eww. Also for backwards compatibility libnm-glib/libnm would accept *either* a dict-in-dict (skeleton NMConnection) or just the NMSetting dict (skeleton NMSetting). We don't care about this in libnm at all so we can remove this possibility. See below for details about Request subobjects, but if we do that then we could track the updated secrets on those objects. Instead of creating some kind of hash that the Agents have to send back, we could do something like: nm_secret_agent_request_add_secret (obj, setting_name, secret value); and then NMSecretAgentRequest in libnm would be responsible for marshalling to the D-Bus format instead of the client. One of the gross things is handling VPN secrets because that's really a dict-dict-dict, but obviously if there's a specific function like nm_secret_agent_request_add_secret() we know exactly how to deal with that when 'setting_name="vpn". There may be future cases where we want to let agents return more than just secrets in the GetSecrets reply though, so we should ensure that we don't completely lock that possibility out. Abstracting the dict-dict thing preserves the ability to later push data items back to NM. Anyway it would get rid of code like if (g_strcmp0 (name, SHELL_KEYRING_SK_TAG) == 0) { gchar *secret_name = g_strdup (attribute); if (!closure->is_vpn) { GValue *secret_value = g_slice_new0 (GValue); g_value_init (secret_value, G_TYPE_STRING); g_value_set_string (secret_value, secret_value_get (secret, NULL)); g_hash_table_insert (closure->entries, secret_name, secret_value); } else g_hash_table_insert (closure->vpn_entries, secret_name, g_strdup (secret_value_get (secret, NULL))); secrets_found = TRUE; g_hash_table_unref (attributes); secret_value_unref (secret); break; } and make that into something like: if (g_strcmp0 (name, SHELL_KEYRING_SK_TAG) == 0) { gchar *setting_name = nm_secret_agent_request_get_setting_name (request); nm_secret_agent_request_add_secret (request, setting_name, attribute, secret_value_get (secret, NULL)); secrets_found = TRUE; secret_value_unref (secret); break; } ----- #2) Every client has to duplicate code to track secrets requests, and NMSecretAgent does the same thing internally too. NMSecretAgent creates an 'info' closure and all the subclasses of NMSecretAgent create their own closures too. Double the work. Perhaps a small NMSecretAgentRequest abstract object that callers could subclass? NMSecretAgent could grow a construct-only GType property for the request object type, and then g_object_new() that when a new request comes in. This would effectively replace the internal GetSecretsInfo struct. This object would get passed to the subclass in the get_secrets() class method where the subclass would then initialize the rest of the object as it sees fit. Same thing for Save/Delete, using the same request class? Or if not any of this, I'm just looking for some way to reduce the number of random structs for tracking every request since NMSecretAgent and the subclasses do the same things right now (WRT storing metadata and tracking for cancellation) but duplicate that code. ------ #3) The 'has_always_ask'/'is_connection_always_ask' bits in applet-agent.c are also used by the Shell in shell-network-agent.c. If we did do NMSecretAgentRequest then we could have a method on this object that would just do the same thing. Or just a general method to check the NMConnection that secrets requests have to see if it contains any always-ask secrets. ------ That's all I can think of right now. The major suckage is #1, and #2 would be a huge bonus. #3 is lower priority but pretty easy I think?
The other thing I want to do is switch it around to use gio-style async like everything else does now. So NMSecretAgentClass will have get_secrets_async() and get_secrets_finish() methods, rather than get_secrets() and a custom callback type. And GCancellables rather than a separate cancel method. Oh, also also, as discussed, we'd like the API to be able to handle device secrets later too, so I was going to change all of the NMConnections in the API to NMObjects, but note in the documentation that they're guaranteed to be connections, unless you set, eg, NM_SECRET_AGENT_CAPABILITY_DEVICE_SECRETS. So... void nm_secret_agent_get_secrets_async (NMSecretAgent *self, NMObject *object, NMSecretAgentRequest *request, NMSecretAgentGetSecretsFlags flags, GCancellable *cancellable, GAsyncReadyCallback callback, gpointer user_data); gboolean nm_secret_agent_get_secrets_finish (NMSecretAgent *self, GAsyncResult *result, GError **error); NMSecretAgentRequest would have some method that returned a char** of requested secrets, which would be like "802-11-wireless-security.psk" or "vpn.Xauth password". The API would just ignore the fact that VPN secrets work differently from other secrets and handle that entirely internally. Then the implementation would call nm_secret_agent_request_add_secret() or whatever for each secret, and then call the callback via a GAsyncResult, etc. In the future, @object might be an NMDevice rather than an NMRemoteConnection, and the secrets might be "gsm.pin" or whatever. Internally there'd have to be a bunch of differences, and code specific to each kind of secrets request. But externally, it could all look the same. I'm still thinking about #2...
(In reply to comment #1) > The other thing I want to do is switch it around to use gio-style async like > everything else does now. So NMSecretAgentClass will have get_secrets_async() > and get_secrets_finish() methods, rather than get_secrets() and a custom > callback type. And GCancellables rather than a separate cancel method. Yeah, that makes sense. > Oh, also also, as discussed, we'd like the API to be able to handle device > secrets later too, so I was going to change all of the NMConnections in the API > to NMObjects, but note in the documentation that they're guaranteed to be > connections, unless you set, eg, NM_SECRET_AGENT_CAPABILITY_DEVICE_SECRETS. Well, as further discussed on IRC, we may not actually do device secrets in NM, becuase the only case for that now is WWAN PIN, and things other than NM touch the modem (GPS, SMS apps, etc) and they also have to deal with the PIN. So given that PIN operations are broader than just networking, whatever UI/DE/Shell the user has should probably be handling the unlocking itself? But it doesn't hurt to make the request source an NMObject like you propose. > So... > > void nm_secret_agent_get_secrets_async (NMSecretAgent *self, > NMObject *object, > NMSecretAgentRequest *request, > NMSecretAgentGetSecretsFlags > flags, > GCancellable *cancellable, > GAsyncReadyCallback callback, > gpointer user_data); > gboolean nm_secret_agent_get_secrets_finish (NMSecretAgent *self, > GAsyncResult *result, > GError **error); > > NMSecretAgentRequest would have some method that returned a char** of requested > secrets, which would be like "802-11-wireless-security.psk" or "vpn.Xauth > password". The API would just ignore the fact that VPN secrets work differently > from other secrets and handle that entirely internally. Right. Although, I assume what you're talking about here is 'hints'. These don't have to be property names, and in fact we do use them for VPN plugin hints that get passed through to the auth dialogs. In that case they are prefixed with "x-vpn-message:". Should we even both to prefix the returned hints with the setting name though? We could just expose the setting name as a getter method and clients assume that anything not prefixed with "x-" is a a secret in that setting. I only ask this because otherwise the client has to do more work to split the hint into setting + property. > Then the implementation would call nm_secret_agent_request_add_secret() or > whatever for each secret, and then call the callback via a GAsyncResult, etc. Right. > In the future, @object might be an NMDevice rather than an NMRemoteConnection, > and the secrets might be "gsm.pin" or whatever. Internally there'd have to be a > bunch of differences, and code specific to each kind of secrets request. But > externally, it could all look the same. Yeah, that's about right. > I'm still thinking about #2... Yeah, the subclass of NMSecretAgentRequest thing was the best I could think of here, because then every client could have it's own implementation and wouldn't have to create yet another struct to do it. NMSecretAgent could (if needed) grow a method to search for a specific NMSecretAgentRequest too.
(In reply to comment #2) > Well, as further discussed on IRC, we may not actually do device secrets in NM, > becuase the only case for that now is WWAN PIN > But it doesn't hurt to make the request source an NMObject like you propose. Well... it does; it makes it less convenient for the user, since they have to cast it. And also, the NMConnection that's currently used there is an NMSimpleConnection (ie, not actually an NMObject), because we don't want to have to deal with the cache and/or the async object initialization to get an NMRemoteConnection. So if we're not *very* likely to eventually want non-connection objects using the same API, then sticking with NMConnection is a win. > We could just expose the setting name as a getter method and clients assume > that anything not prefixed with "x-" is a a secret in that setting. I only ask > this because otherwise the client has to do more work to split the hint into > setting + property. Hm... I was thinking the daemon could request secrets for multiple settings at once, but I guess it can't. And I was mixing up the underlying NMSecretAgent API with the NMSecretAgentSimple API. (And thus basically completely messing up the hints/secrets thing.)
After some discussion on IRC, I've pushed danw/secret-agent-old-bgo740345, which renames NMSecretAgent to NMSecretAgentOld, so we can fix NMSecretAgent later. (It does not rename NMSecretAgentGetSecretsFlags or NMSecretAgentError, because those are both part of the D-Bus API, so they can't change anyway.)
Looks good to me. I also pushed a commit: libnm: merge nm_secret_agent_simple_set_connection_path() into nm_secret_agent_simple_enable() that I'd been talking with jklimes about on IRC today.
pushed, with dcbw's additional commit