NetworkManager 1.2 support

[lkundrak]

Please note that this has a build time dependency on a version not yet
released; you probably don't want to pull it as it is. This is just so that review
and discussion can start early if there's anything objectionable here.

Hello,

The branch contains changes that are necessary for the properties
plugin to continue working with upcoming nm-connection-editor 1.2 and
more. The details are explained in the "nm: split the plugin into two halves"
commit message.

Please note that I just did a buildcheck; I don't have a necessary
setup to test this (= I'm probably a lazy fuck). Given I've done
analogous changes to other plugins and they worked I wouldn't expect it
to be too broken or broken at all. We'll eventually test this
downstream for Fedora 24. Should you try it out and discover any
issues, please let me know.

NetworkManager 1.2 RC is expected to be released towards the end of this
year, followed by a release in the Q1 2016.

Shortlog:

Lubomir Rintel (11):
      nm: move the plugin metadata to charon-nm
      nm: set full path to the connection editor plugin
      nm: drop some unneeded dependencies
      nm: drop useless calls to AC_SUBST
      nm: split the plugin into two halves
      nm: check for libnm
      nm: port to libnm
      nm: replace libgnomeui with libnma for password dialog
      nm: replace libgnomekeyring with libsecret
      nm: bump to GTK+ 3.0
      nm: bump minor version to 1.4.0

Thank you,
Lubo

[lkundrak]

Any opinions here?

[tobiasbrunner]

Generally looks great. Thanks a lot for your efforts.

I only did a review of the code changes and haven't tested it yet as I don't have a setup to do so right now.

Some questions:

• Why did you write "split the plugin into two halves"? Doesn't this rather create two versions of the same plugin, just linked against different libraries?
• Would it be possible to maintain compatibility with older NM releases? If not, we might want to split these changes into two branches/PRs, one with some general cleanups (plus e.g. the move to libsecret, which already caused some problems on recent distros) and one with the migration to NM 1.2.
• As you commented, the move to libnma removes the possibility to remember the entered password. Do you have some pointers on how this feature could be replicated?
• Is placing the .name file in two locations a problem?
• Not directly related, but perhaps you don't mind my asking your opinion: There seems to be an issue with the plugin's GUI (#1013 on our issue tracker). Did you see similar issues when you migrated other plugins? Does the patch posted there make sense to you (in particular if different frontends have to be supported)?

[lkundrak]

Sorry for the delay. NetworkManager 1.2 is nearing a release (beta3 out now), so this should be moved forward a bit.

Why did you write "split the plugin into two halves"? Doesn't this rather create two versions of the same plugin, just linked against different libraries?

Well, that's possibly a poor wording on my side. Will fix that.

This indeed essentially creates two plugins from a single source. The libnm and libnm-glib API is not entirely compatible, so this essentially is a libnm port with some #define trickery to make it build with libnm-glib. It's less code to maintain and still provides compatibility.

But yeah, I should probably improve the wording.

Would it be possible to maintain compatibility with older NM releases? If not, we might want to split these changes into two branches/PRs, one with some general cleanups (plus e.g. the move to libsecret, which already caused some problems on recent distros) and one with the migration to NM 1.2.

Done: #39

As you commented, the move to libnma removes the possibility to remember the entered password. Do you have some pointers on how this feature could be replicated?

Someone would need to add that functionality to libnma (the password dialog API is generally modelled after the gnomeui one).

But my take on the issue is that it's generally a waste of time. The password dialog during the attempt to connect is definitely not the correct place to store a permanent password. It should go into the preferences dialog instead.

Now i notice it's even not in the preferences dialog at all. That's not good; I'll look into that...

Is placing the .name file in two locations a problem?

No.

Not directly related, but perhaps you don't mind my asking your opinion: There seems to be an issue with the plugin's GUI (#1013 on our issue tracker). Did you see similar issues when you migrated other plugins? Does the patch posted there make sense to you (in particular if different frontends have to be supported)?

This is not related to a migration to a new version of NetworkManager and doesn't affect any other plugin. I've checked the history -- no other plugin ever had a top-level GtkWindow widget around the dialog and having that one is certainly a mistake (probably an unfortunate click in glade or something). Perhaps older versions of Gtk were more tolerant?

[lkundrak]

Now i notice it's even not in the preferences dialog at all. That's not good; I'll look into that...

Okay, seems like it's straightforward enough. Will follow up with a patch tomorrow, got to go now

[tobiasbrunner]

Thanks a lot. I rebased these changes to the nm-updates branch and pushed them to the nm-1.2 branch. I have yet to try them out on a newer system.

I wonder if the secret_password_lookup_sync() call in the auth dialog is now still the correct way to retrieve the password. Or does the libnma password storage use the same keystore?

[lkundrak]

I wonder if the secret_password_lookup_sync() call in the auth dialog is now still the correct way to retrieve the password. Or does the libnma password storage use the same keystore?

It's not. I've added a fixup to address that.

I don't think that code path is ever taken now, since if the secrets are wrong since charon_nm fails the connection on wrong credentials without asking for different ones.

NB: I've done some testing with PSK now and it seems to work well. Turned out we've broken charon_nm compatibility in NM; fixed that now but a very new (post-rc2) code is needed to test this.

We're probably doing a NetworkManager 1.2 release next Tuesday (April 19th).

[tobiasbrunner]

Thanks for the update.

I don't think that code path is ever taken now, since if the secrets are wrong since charon_nm fails the connection on wrong credentials without asking for different ones.

I see. I don't think we changed anything. So did NM retry to initiate the connection in earlier releases (e.g. based on NM_VPN_PLUGIN_FAILURE_LOGIN_FAILED)? Or was that always the case?

Anyway, I guess there is not really a point in retrieving a stored password then? Because if it is stored with the connection the dialog would not show up in the first place. And if it was not stored, trying to get it is kinda pointless. Or can we change it to get a password that was previously stored via libgnomekeyring (so existing connections work after an NM upgrade without having to re-enter the password or changing the settings)?

I noticed that you dropped the commit cf826a5 ("nm: Set full path to the connection editor plugin") that was in the nm-updates branch. Any particular reason for this?

[lkundrak]

So, I got the last update horribly wrong. It worked well before: the properties dialog and the agent now use the networkmanager schema to store the VPN secrets. In case it is used, the auth-helper doesn't get in the way: the agent deals with getting and storing the passwo
rd in the correct schema in keyring.

Only in case the agent can't retrieve the password it calls the auth helper for help. That happens in case the password is not stored at all or if it's in the old schema (be it during the activation or edit attempt). Thus, the auth helper really just needs to use the old "network" schema and everything works well. If the user attempts to edit the connection, the agent calls the helper to fetch the password from the old schema and saves it into the new one.

I've just removed fixup -- it worked well and I broke it.

I don't think that code path is ever taken now, since if the secrets are wrong since charon_nm fails the connection on wrong credentials without asking for different ones.
I see. I don't think we changed anything. So did NM retry to initiate the connection in earlier releases (e.g. based on NM_VPN_PLUGIN_FAILURE_LOGIN_FAILED)? Or was that always the case?

Anyway, I guess there is not really a point in retrieving a stored password then? Because if it is stored with the connection the dialog would not show up in the first place. And if it was not stored, trying to get it is kinda pointless. Or can we change it to get a password that was previously stored via libgnomekeyring (so existing connections work after an NM upgrade without having to re-enter the password or changing the settings)?

I dont think NM ever did that. That said; I was wrong saying that the code path is not taken. That was under the wrong assumption that the auth dialog is only used to get the interactive secrets. That is not true -- the agent calls the helper whenever it needs the password -- on editing the connection with the old schema as well as on activating it,

Sorry for the confusion; I was not terribly familiar with how the auth helpers are used since for most plugins they are not needed now (the services now ask the secrets with hints describing what the secret needed is for, so a generic VPN password dialogs can be implmented). charon_nm should eventually do that too; but as for now I'm interested in making sure we're not breaking the essential functionality.

I noticed that you dropped the commit cf826a5 ("nm: Set full path to the connection editor plugin") that was in the nm-updates branch. Any particular reason for this?

Yes. That was not a particularly clever idea: hardcoding a lib64 path in a file in lib directory wouldn't play well with multilib. It would likely make packageers unhappy; rpmlint and lintian both complained.

I've tested this patchset with PSK and old-schema password (had to insert the PSK to keyring by calling secret_password_store_sync(), neither seahorse nor secret-tool seem to be able to create network schema secrets).

[tobiasbrunner]

OK, thanks for the updates and tests. I dropped that commit and added the one fix regarding the second password field in the dialog and pushed the updated branches.

One issue I noticed (just tested the nm-updates branch so far) concerns the path to the auth-dialog that's set in nm-strongswan-service.name, which is now based on the libexecdir configured while building charon-nm (usually <prefix>/libexec). This was different previously as in the debian/rules file of the NM plugin libexecdir is currently set to <prefix>/lib/NetworkManager, which was then also used in the .name file.
Because the path is hardcoded in the .name file I assumed it wouldn't matter where the executable is actually located but that doesn't seem to be the case. On my system (Ubuntu 14.04 with NM 0.9.8.8) NM always tries to execute /usr/lib/NetworkManager/nm-strongswan-auth-dialog . So should that path actually be absolute (in which case we should probably change it from $(libexecdir) to e.g. $(prefix)/lib/NetworkManager) or should we just configure the name of the executable (analogous to the properties file)?

I was not terribly familiar with how the auth helpers are used since for most plugins they are not needed now (the services now ask the secrets with hints describing what the secret needed is for, so a generic VPN password dialogs can be implmented).

Interesting, is this documented somewhere?

---

Update: I now tested the nm-1.2 branch in an Ubuntu 16.04 VM that comes with NM 1.1.93. The problem with the paths applies there similarly, but here at least the path in the .name file is used. So as long as libexecdir used to build the plugin (e.g. in debian/rules) is the same as that used in the charon-nm build it should work (i.e. it shouldn't be required anymore to actually set libexecdir explicitly). On the other hand, as long as the auth-dialog is placed in /usr/lib/NetworkManager (or an architecture specific lib path) no absolute path seems to be required. So we could also just get rid of the path in the .name file and configure libexecdir accordingly (while libdir is probably set to /usr/lib/x86_64-linux-gnu on 64-bit Ubuntus, where packages are currently only installing plugins, auth-dialogs are apparently placed in /usr/lib/NetworkManager). Not using an absolute path for the auth-dialog seems to work fine with older NM releases too, so maybe this is the way to go.

However, with this particular NM version an absolute path to the libnm plugin is actually required:

(nm-connection-editor:28678): WARNING **: vpn: (strongswan,/etc/NetworkManager/VPN/nm-strongswan-service.name) could not load plugin: path is not absolute (libnm-vpn-plugin-strongswan.so)

So I guess even though it is not nice to refer to architecture specific libraries from a file in /usr/lib there is no way around that (it's also the case for the pptp plugin that is installed by default on Ubuntu).

There is also a strange issue regarding the password: It is not stored if I select to only store it for the current user, if I select to store it for all users it works (nm_setting_vpn_add_secret is called in both cases, but the flags set with nm_setting_set_secret_flags are obviously different). What's the reason for this? Is this because of missing functionality in the plugin (i.e. related to what you mentioned above)?
I've also added the 20 character minimum length check for PSKs that we currently have in the auth-dialog.

[lkundrak]

OK, thanks for the updates and tests. I dropped that commit and added the one fix regarding the second password field in the dialog and pushed the updated branches.

One issue I noticed (just tested the nm-updates branch so far) concerns the path to the auth-dialog that's set in nm-strongswan-service.name, which is now based on the libexecdir configured while building charon-nm (usually /libexec). This was different previously as in the debian/rules file of the NM plugin libexecdir is currently set to /lib/NetworkManager, which was then also used in the .name file.
Because the path is hardcoded in the .name file I assumed it wouldn't matter where the executable is actually located but that doesn't seem to be the case. On my system (Ubuntu 14.04 with NM 0.9.8.8) NM always tries to execute /usr/lib/NetworkManager/nm-strongswan-auth-dialog . So should that path actually be absolute (in which case we should probably change it from $(libexecdir) to e.g. $(prefix)/lib/NetworkManager) or should we just configure the name of the executable (analogous to the properties file)?

Well, there's more agents than just the nm-applet; notably the GNOME Shell and I'm almost sure it at some point required the absolute paths. Now it seems to look in libexec too.

It's unfortunate now that the sources for the service and the plugins are separate, but share the same metadata. It's perhaps most wise to just use what NetworkManager was configured bit. It's not too nice, but is going to be play well with what distributions do.

Added a commit that does that & build-install-tested on Ubuntu 16.04.

I was not terribly familiar with how the auth helpers are used since for most plugins they are not needed now (the services now ask the secrets with hints describing what the secret needed is for, so a generic VPN password dialogs can be implmented).

Interesting, is this documented somewhere?

Sorry, I confused that with the external UI mode; that still uses the auth helper, but delegates the renering of the dialog to the DE. It's basically a cosmetic thing. It would be nice to have this to get native look in GNOME Shell, but not terribly important.

Update: I now tested the nm-1.2 branch in an Ubuntu 16.04 VM that comes with NM 1.1.93. The problem with the paths applies there similarly, but here at least the path in the .name file is used. So as long as libexecdir used to build the plugin (e.g. in debian/rules) is the same as that used in the charon-nm build it should work (i.e. it shouldn't be required anymore to actually set libexecdir explicitly). On the other hand, as long as the auth-dialog is placed in /usr/lib/NetworkManager (or an architecture specific lib path) no absolute path seems to be required. So we could also just get rid of the path in the .name file and configure libexecdir accordingly (while libdir is probably set to /usr/lib/x86_64-linux-gnu on 64-bit Ubuntus, where packages are currently only installing plugins, auth-dialogs are apparently placed in /usr/lib/NetworkManager). Not using an absolute path for the auth-dialog seems to work fine with older NM releases too, so maybe this is the way to go.

The auth dialogs are used outside the nm-applet's agent. A quick look at the gnome-shell code suggests that it falls back to libexecdir it was configured with which is likely a different one than the plugin was configured with on Debian...

However, with this particular NM version an absolute path to the libnm plugin is actually required:

(nm-connection-editor:28678): WARNING **: vpn: (strongswan,/etc/NetworkManager/VPN/nm-strongswan-service.name) could not load plugin: path is not absolute (libnm-vpn-plugin-strongswan.so)

So I guess even though it is not nice to refer to architecture specific libraries from a file in /usr/lib there is no way around that (it's also the case for the pptp plugin that is installed by default on Ubuntu).

This is a bit unfortunate; we messed up a bit. Ubuntu included a Git snapshot before we realized the mistake and corrected it. Thus all other plugins need absolute paths on Ubuntu. The other plugins have a configure-time option for using the absolute paths now, but it's default off. That's not too optimal for users that build from source. I'm including a commit that adds analogous option. Maybe you may want to enable them from the debian/ build script if you care about it being buildable on current Ubunutu.

There is also a strange issue regarding the password: It is not stored if I select to only store it for the current user, if I select to store it for all users it works (nm_setting_vpn_add_secret is called in both cases, but the flags set with nm_setting_set_secret_flags are obviously different). What's the reason for this? Is this because of missing functionality in the plugin (i.e. related to what you mentioned above)?

That's interesting. It worked well for me; and I've used both the GNOME Shell agent (which seems to stop responding when NM is restarted a couple of times, resulting in a behavior similar to what you describe) and nm-applet too (had no problems with that one) and didn't notice that. I'll re-check.

I've also added the 20 character minimum length check for PSKs that we currently have in the auth-dialog.

If the external UI mode is implemented the secret length can't be validated in the auth dialog. If you want to disallow <= 20 character psk in all cases, then probably the service should reject it (unless it already does).

[lkundrak]

There is also a strange issue regarding the password: It is not stored if I select to only store it for the current user, if I select to store it for all users it works (nm_setting_vpn_add_secret is called in both cases, but the flags set with nm_setting_set_secret_flags are obviously different). What's the reason for this? Is this because of missing functionality in the plugin (i.e. related to what you mentioned above)?

That's interesting. It worked well for me; and I've used both the GNOME Shell agent (which seems to stop responding when NM is restarted a couple of times, resulting in a behavior similar to what you describe) and nm-applet too (had no problems with that one) and didn't notice that. I'll re-check.

Could not reproduce this. Is the secret agent running in your session? Is it GNOME Shell, nm-applet or something else? Which versions?

[tobiasbrunner]

It's unfortunate now that the sources for the service and the plugins are separate, but share the same metadata.

It was maybe a bit easier before moving the metadata to charon-nm as only the path to that had to be configured when building the plugin. The paths to the plugin itself and the auth-dialog were "automatically" set to the actual locations of the installed files (and these could be changed freely without affecting the regular build). Could we keep the .name file where it was and only move the D-Bus policy?

The other plugins have a configure-time option for using the absolute paths now, but it's default off. That's not too optimal for users that build from source. I'm including a commit that adds analogous option.

I see. You used pkg-config to determine libexecdir (or rather libgnome_serverdir), could we use the same to determine plugindir instead of making this depend on libdir?

I've also added the 20 character minimum length check for PSKs that we currently have in the auth-dialog.

If the external UI mode is implemented the secret length can't be validated in the auth dialog. If you want to disallow <= 20 character psk in all cases, then probably the service should reject it (unless it already does).

OK, good to know. I don't think the service currently enforces it, but we should probably add that if the number of ways to enter a PSK increase.

Could not reproduce this. Is the secret agent running in your session? Is it GNOME Shell, nm-applet or something else? Which versions?

I tried again on a new snapshot of a freshly installed Ubuntu 16.04 with nm-applet 1.1.93, doesn't work correctly. When I watch the Passwords/Login section in Seahorse I see that the password entry is created when I save the config. However, when I initiate the connection (or open the connection editor again) the entry disappears.

[lkundrak]

(Sorry for no response here; I haven't forgotten about this, just am busy with other things.)

[petski]

FWIW: NetworkManager 1.2 just hit Ubuntu 16.04 LTS. (https://bugs.launchpad.net/ubuntu/+source/network-manager-strongswan/+bug/1578193)

[AlexC]

Is there any update on this? Currently broken in Ubuntu 16.04 because of this

[lkundrak]

@AlexC sorry, super-busy now. Help (perhaps someone who checks the branch on Ubuntu and checks what's left to fix) more than welcome!

[AlexC]

@lkundrak Let me see if I can compile this and report back

[tobiasbrunner]

Besides the issues I brought up in my last message this is pretty much ready.

[tobiasbrunner]

I've rebased the nm-1.2 branch to the current master. Some questions: In "nm: Install the .name file into /usr/lib/NetworkManager/VPN" couldn't we set nm_plugindir to pkg-config --variable=plugindir NetworkManager and use the same for the frontend? And can't nmvpnservicedir just be set to $(nm_libexecdir)/VPN? This would allow testing the NM plugin on dev machines without having to set --prefix or --libdir (which affect all the other executables/libraries). Similarly, in "nm: Port to libnm" is PLUGINDIR (I readded the old definition in the Makefile) not the same as $(nm_plugindir) (at least in case absolute paths are used)?

As I mentioned before, all this might be clearer if the .name file is not moved from the frontend, which is configurable freely to match NM's paths (or they could be determined automatically). The only "moving target" is charon-nm, whose location is/was configurable.
I attempted to do this in the nm-1.2-name branch. What do you think? Does the path to the libnm plugin (see "nm: Port to libnm") always have to be absolute? Or is this like the other plugin?

In both branches the path in the comment on the first line of the .name file in /etc is not yet printed correctly (NM_VPN_SERVICE_DIR is not defined).

The problem with storing the password only for the current user is still apparent on a fully updated Ubuntu 16.04 (nm-applet 1.2.0-0ubuntu0.16.04.3).

FYI, I'll be away next week.

[AlexC]

@tobiasbrunner Struggling to recompile the Ubuntu packages with this patch. Did you recompile the packages or just compile this from source and install that? Any further progress on this?

[tobiasbrunner]

@AlexC I just compiled from source. What problems did you have when recompiling the packages?

[tobiasbrunner]

To get this moving, I fixed that password storing issue in 16.04 by updating the auth dialog and merged the branches that don't move the .name file. I'll upload some tarballs for the plugin so package maintainers can hopefully update the packages.