GNOME Bugzilla – Bug 719815
[PATCH] shell-network-agent: fix handling of initial secrets requests and VPN always-ask secrets
Last modified: 2013-12-13 22:26:46 UTC
Created attachment 263470 [details] [review] simplify secrets lookup code n_found isn't used anywhere as an integer value; nothing cares about how many secrets are found, just that some are found or some are not found. Simplify that. (the reason I found this was that for some reason, possibly compiler optimization related though just with -O2 -Wp,-D_FORTIFY_SOURCE=2, n_found was never actually touched by the code. Yes, even after "n_found += 1", printing out the value of n_found was still zero!!?? This caused the shell to always ask for secrets even when it had some in the keyring. The simplification fixes that by side-effect)
If somebody could test-compile this just to be sure (for some reason I need to update some deps before I can check it) that would be great too...
Review of attachment 263470 [details] [review]: Please try to attach a 'git-format-patch'-formatted patch next time. git-bz is a useful tool for this. ::: src/shell-network-agent.c @@ +328,3 @@ g_hash_table_insert (closure->vpn_entries, secret_name, g_strdup (secret_value_get (secret, NULL))); + secrets_found = TRUE; Looks like you can remove strv_has now. @@ +341,3 @@ g_list_free_full (items, g_object_unref); + if (secrets_found == FALSE && !secrets_found
Got the real cause; patch attached. Hints can come in from dbus as a strv with one NULL element, but the code didn't treat that as "no hints".
Created attachment 263483 [details] [review] 0001-NetworkAgent-handle-empty-hints-correctly.patch
Review of attachment 263483 [details] [review]: I'm not too familiar with this code, but I haven't figured out what "hints" is. Should an empty list behave differently on purpose?
Created attachment 263517 [details] [review] 0001-NetworkAgent-handle-empty-hints-and-VPN-hints-correc.patch
(In reply to comment #5) > Review of attachment 263483 [details] [review]: > > I'm not too familiar with this code, but I haven't figured out what "hints" is. > Should an empty list behave differently on purpose? The API docs for 'hints' say: --- Array of strings of key names in the requested setting for which NetworkManager thinks a secrets may be required, and/or well-known identifiers and data that may be useful to the client in processing the secrets request. Note that it's not always possible to determine which secret is required, so in some cases no hints may be given. The Agent should return any secrets it has, or that it thinks are required, regardless of what hints NetworkManager sends in this request. --- Basically, if NM has any information that could help out, it might pass that information. But it may also pass nothing at all. Unfortunately, get_secrets_keyring_cb() mishandled some of that; more details in the latest patch that I've attached. The latest patch also fixes the VPN always-ask case, which the previous patch broke by side-effect. Since the shell agent doesn't know anything about the specific VPN properties (because those are different for every VPN plugin and we can't hardcode anything there) it doesn't know about which VPN secrets are always-ask or not. But the VPN auth-dialogs know about that. For this reason, nm-applet always calls the auth dialogs for VPN secrets requests; they already handle the request 'flags' and should do the right thing for interactive/request-new.
Created attachment 263544 [details] [review] 0001-NetworkAgent-handle-empty-hints-and-VPN-secrets-corr.patch
Review of attachment 263544 [details] [review]: ::: src/shell-network-agent.c @@ +332,3 @@ nm_connection_update_secrets (closure->connection, closure->setting_name, closure->entries, NULL); request_secrets_from_ui (closure); I'm confused about this. Looking through networkAgent.js, this will always pop up a dialog, even if we already have the secrets stored in the keyring. What's wrong with letting NM send a REQUEST_NEW request if it doesn't get the secrets it wants?
Actually it won't pop up the dialog unless the VPN plugins tell the shell to do so via the "ShouldAsk" key of the ExternalUiMode keyfile they write to the shell. They know what to do based on the secret flags and the -i and -n flags passed from the shell based on the GetSecrets() request flags NM sends. Arguably, the shell shouldn't even bother reading keyring secrets for the VPN plugins anyway, since they already do all of that, and thus the shell doing it is redundant. Only the auth dialogs know *exactly* what they require for secrets because the VPN key/value pairs are known only to the VPN plugins, since they are all different, and that cannot be standardized. The shell has no idea whether the VPN is "always ask" or "not required" or what, which is what triggered the VPN parts of this patch. Originally, since the shell failed to detect that the VPN was always ask, it never hit that is_connection_always_ask() part below. Then it read the keyring, and because of the n_found/hints bug, would always call the VPN auth dialog. Now, with the n_found/hints bug fixed, it no longer always calls the VPN auth dialog if it did read stuff from the keyring. This patch fixes that. > What's wrong with letting NM send a REQUEST_NEW request if it doesn't get the secrets it wants? Because NM doesn't currently set REQUEST_NEW for VPN connections because there's no way to know that a given password is wrong and that a completely new one is required; VPN services typically do not give such information. Plus, you may have multiple passwords (in the vpnc case, a group secret and a user secret) and in most cases, only one of those would be wrong and need to be re-requested. But all that is beside the point: the VPN auth dialogs know what to do, and the shell cannot know what to do, so they VPN auth dialogs need to be part of the process here.
Review of attachment 263544 [details] [review]: I'll trust your judgement on this one, then.
Thanks, pushed.