GNOME Bugzilla – Bug 664335
Implement support for shell mode in vpn auth dialogs
Last modified: 2012-02-27 20:02:37 UTC
As described in bug 658484, we want gnome-shell to show native dialogs for VPN auth. To make this possible, I implemented a new version of the protocol between the secret agent and the auth helper. This is a tracker bug to host patches to the various VPN plugins, starting from openvpn, which is what I used for testing.
Created attachment 201666 [details] [review] auth-dialog: introduce gnome-shell mode In this mode, we don't actually prompt for secrets. Instead, we emit a sequence of key/values that describe the password entries and are used to create a shell styled dialog.
Created attachment 201668 [details] [review] auth-dialog: introduce gnome-shell mode In this mode, we don't actually prompt for secrets. Instead, we emit a sequence of key/values that describe the password entries and are used to create a shell styled dialog. This time for VPNC. Not tested, but the patch is pretty similar.
Instead of "shell mode" I'd rather call it something like "other ui" to keep it a bit generic. One thing to note is that this isn't going to work with openconnect (which is a plugin that connects to newer Cisco ASA VPN concentrators) since it does a bunch of work in the auth dialog. The other VPNs are pretty simple. We probably want to get Dave Woodhouse's opinion on what to do WRT openconnect. Other than that, I have no issues with the patches or the protocol. At some point we should fundamentally redesign the way the VPN UI plugin communication works here, but that doesn't have to be today.
Created attachment 202664 [details] [review] auth-dialog: introduce external-ui mode (vpnc) In this mode, we don't actually prompt for secrets. Instead, we emit a sequence of key/values that describe the password entries. This will be used by gnome-shell to create a shell styled dialog.
Created attachment 202665 [details] [review] auth-dialog: introduce external-ui mode (openvpn) In this mode, we don't actually prompt for secrets. Instead, we emit a sequence of key/values that describe the password entries. This is used by gnome-shell to create a shell styled dialog. for openvpn
Created attachment 202666 [details] [review] auth-dialog: introduce external-ui mode (pptp) In this mode, we don't actually prompt for secrets. Instead, we emit a sequence of key/values that describe the password entries. This is used by gnome-shell to create a shell styled dialog. For PPTP
I briefly looked up openconnect code, and it definitely seems to need more than just a sequence of entries. As far as implementation is concerned, the protocol is backward compatible (if the shell doesn't see the appropriate indication in the keyfile, or if VERSION=2 is not emitted, the plugin is assumed to be for the old version), so we can just port the others and skip openconnect for now. If we want to have openconnect in the shell, we would need better integration (someone in the other bug proposed a shell extension). On the other hand, if complex configuration is really necessary, maybe a Gtk dialog is still appropriate, and therefore we should think of moving it in gnome-control-center (like we did with wifi).
Comment on attachment 202664 [details] [review] auth-dialog: introduce external-ui mode (vpnc) >+ gboolean other_ui_mode, make it "external" rather than "other", for consistency >+ if (!get_secrets (vpn_uuid, vpn_name, >+ retry, allow_interaction, other_ui_mode, > g_hash_table_lookup (secrets, NM_VPNC_KEY_XAUTH_PASSWORD), NM's indentation style is not 100% consistent, but the ideal is "tabs for indentation, spaces for alignment". (So the added line should have 1 tab followed by 18 spaces, like the line below it.) >+supports-shell-mode=true "supports-external-ui-mode" Also, one of the alternate UIs we might want to support later is nmcli, so you should change the gtk_init() call to gtk_init_check(), and then later bail out if that returned FALSE and the user didn't pass --external-ui-mode.
Comment on attachment 202665 [details] [review] auth-dialog: introduce external-ui mode (openvpn) basically exactly the same comments, except that the misindented lines are different
Comment on attachment 202666 [details] [review] auth-dialog: introduce external-ui mode (pptp) likewise
(In reply to comment #7) > I briefly looked up openconnect code, and it definitely seems to need more than > just a sequence of entries. Yeah, and given that none of us currently have access to testing hardware, and IIRC Cisco doesn't even make that hardware any more anyway, I think we can just leave it using the old style for now.
Created attachment 207459 [details] [review] auth-dialog: introduce external-ui mode (vpnc) In this mode, we don't actually prompt for secrets. Instead, we emit a GKeyFile that describes the needed password entries. This will be used by gnome-shell to create a shell styled dialog.
Created attachment 207460 [details] [review] auth-dialog: introduce external-ui mode (openvpn) In this mode, we don't actually prompt for secrets. Instead, we emit a GKeyFile that describes the needed password entries. This is used by gnome-shell to create a shell styled dialog.
Created attachment 207461 [details] [review] auth-dialog: introduce external-ui mode (pptp) In this mode, we don't actually prompt for secrets. Instead, we emit a GKeyFile that describes the needed password entries. This is used by gnome-shell to create a shell styled dialog.
code looks basically fine, though maybe we'd want to move some of it into nm-vpn-plugin-utils in libnm-glib... dcbw, does the approach in these pathes work for you?
(In reply to comment #11) > (In reply to comment #7) > > I briefly looked up openconnect code, and it definitely seems to need more than > > just a sequence of entries. > > Yeah, and given that none of us currently have access to testing hardware, and > IIRC Cisco doesn't even make that hardware any more anyway, I think we can just > leave it using the old style for now. Not quite; openconnect/SSL is the *new* way of doing things with Cisco VPN hardware, vpnc/IPSec is the old way. So over time we'd expect vpnc usage to decrease and SSL-type VPNs to become more common.
(In reply to comment #16) > (In reply to comment #11) > > (In reply to comment #7) > > > I briefly looked up openconnect code, and it definitely seems to need more than > > > just a sequence of entries. > > > > Yeah, and given that none of us currently have access to testing hardware, and > > IIRC Cisco doesn't even make that hardware any more anyway, I think we can just > > leave it using the old style for now. > > Not quite; openconnect/SSL is the *new* way of doing things with Cisco VPN > hardware, vpnc/IPSec is the old way. So over time we'd expect vpnc usage to > decrease and SSL-type VPNs to become more common. Given the amount of work the auth dialog does (and the fact that, differently from the other plugins, it must be called every time one connects), the only way to support this decently in gnome-shell is an extension, which in turn requires to wrap some of the openconnect stuff in a GObject private library. What do you think of it? (If you want, it wouldn't be difficult to also do the fallback UI in gjs+gtk, so that all openconnect stuff goes through the same code in the private library)
Review of attachment 207460 [details] [review]: Small nit: gnome_keyring_memory_free() is already NULL-safe so we don't need to check for NULL before calling it.
Review of attachment 207461 [details] [review]: I don't think we want this change: - if (pw && !(pw_flags & NM_SETTING_SECRET_FLAG_NOT_SAVED)) + if (pw) vpn_password_dialog_set_password (dialog, pw); Since we'll get here for (allow_interaction == TRUE || retry) and in that case, we still don't want to show the old password if it's not supposed to be saved. Also same comment for gnome_keyring_memory_free(); it's NULL-safe, so we might as well just fix that even though the original code didn't think it was.
Review of attachment 207459 [details] [review]: Looks good.
General comments for all the patches: One nit, can we change 'external-ui-mode' in the .name files to 'supports-external-auth-ui' to more accurately reflect what it actually does? I'm fine with that arg name in the vpn auth dialog code itself. Also, what's the motivation for passing "allow_interaction" to keyfile_add_entry_info()? The shell already knows that interaction isn't allowed, so even if the ShouldAsk key is "true" the shell shouldn't be asking for that item. In fact, in the allow_interaction=false case, the ShouldAsk option is entirely redundant. But as the patch stands it won't hurt things to keep it as-is.
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #11) > > > (In reply to comment #7) > > > > I briefly looked up openconnect code, and it definitely seems to need more than > > > > just a sequence of entries. > > > > > > Yeah, and given that none of us currently have access to testing hardware, and > > > IIRC Cisco doesn't even make that hardware any more anyway, I think we can just > > > leave it using the old style for now. > > > > Not quite; openconnect/SSL is the *new* way of doing things with Cisco VPN > > hardware, vpnc/IPSec is the old way. So over time we'd expect vpnc usage to > > decrease and SSL-type VPNs to become more common. > > Given the amount of work the auth dialog does (and the fact that, differently > from the other plugins, it must be called every time one connects), the only > way to support this decently in gnome-shell is an extension, which in turn > requires to wrap some of the openconnect stuff in a GObject private library. > What do you think of it? > (If you want, it wouldn't be difficult to also do the fallback UI in gjs+gtk, > so that all openconnect stuff goes through the same code in the private > library) danw said that if external-ui-mode wasn't supported the shell implemented the nm-applet communication protocol? If so, then we're fine, it just won't look great. If not, then maybe we should. There's also out-of-tree VPN daemons around that we need to fix up. I forgot about network-manager-openswan too; it's ugly but it's grudgingly supported so we should get a patch for it too. It's based off the nm-vpnc code so it should be pretty straightforward.
(In reply to comment #21) > General comments for all the patches: > > One nit, can we change 'external-ui-mode' in the .name files to > 'supports-external-auth-ui' to more accurately reflect what it actually does? > I'm fine with that arg name in the vpn auth dialog code itself. > > Also, what's the motivation for passing "allow_interaction" to > keyfile_add_entry_info()? The shell already knows that interaction isn't > allowed, so even if the ShouldAsk key is "true" the shell shouldn't be asking > for that item. In fact, in the allow_interaction=false case, the ShouldAsk > option is entirely redundant. But as the patch stands it won't hurt things to > keep it as-is. In the openvpn patch, there is an example of a secret whose ShouldAsk is always false, irrespective of allow_interaction. It is meant to give plugins the flexibility of storing secrets that are not actually asked to the user. (In reply to comment #22) > (In reply to comment #17) > > > > Given the amount of work the auth dialog does (and the fact that, differently > > from the other plugins, it must be called every time one connects), the only > > way to support this decently in gnome-shell is an extension, which in turn > > requires to wrap some of the openconnect stuff in a GObject private library. > > What do you think of it? > > (If you want, it wouldn't be difficult to also do the fallback UI in gjs+gtk, > > so that all openconnect stuff goes through the same code in the private > > library) > > danw said that if external-ui-mode wasn't supported the shell implemented the > nm-applet communication protocol? If so, then we're fine, it just won't look > great. If not, then maybe we should. There's also out-of-tree VPN daemons > around that we need to fix up. Yes, we fallback on the nm-applet protocol, although it probably needs testing after the major rework to use GKeyFile for new-style plugins. As for out-of-tree, how many of them do we have? What are their requirements? > I forgot about network-manager-openswan too; it's ugly but it's grudgingly > supported so we should get a patch for it too. It's based off the nm-vpnc code > so it should be pretty straightforward. If it's supported, it should be added to jhbuild, it's not in gnome-world-3.4. Anyway, it's next on the list.
Ok, patches have been pushed to the respective repositories for openvpn, vpnc, pptp, as well as gnome-shell fix for "supports-external-ui-mode".
Created attachment 207663 [details] [review] auth-dialog: introduce external-ui mode (openswan) In this mode, we don't actually prompt for secrets. Instead, we emit a GKeyFile that describes the needed password entries. This is used by gnome-shell to create a shell styled dialog.
Review of attachment 207663 [details] [review]: Looks good, thanks.