> core: update for non-deprecated libreswan initialization flow

>+AC_USE_SYSTEM_EXTENSIONS
>+AC_GNU_SOURCE

That's redundant. The former implies the latter.

> 		PREFIX "/sbin/",
> 		PREFIX "/bin/",
>+		PREFIX "/libexec/ipsec/",
>+		PREFIX "/lib/ipsec/",

This seems a little weird; you have one program that you expect to find in sbin, and one you expect to find in libexec, but you search for both in both paths...

>+			success = do_spawn (&priv->pid, NULL, NULL, error, stackman_path, "start", NULL);
>+			if (success)
>+				priv->watch_id = g_child_watch_add (priv->pid, pluto_watch_cb, self);

Rename pluto_watch_cb?

>+		libreswan = !!(output && strcasestr (output, " Libreswan "));

&& in C always returns 0 or 1. The "!!()" is redundant.

However, maybe you should set priv->libreswan based on whether the additional binaries (_stackmanager, whack, etc) are present?



> fixup! core: update for non-deprecated libreswan initialization flow

remember to squash this obviously


> core: work around libreswan config-terminating oddity

compatibility FAIL! :-}


> core: wait for connection addition to be ready

The commit message talks about "whack", suggesting this is libreswan-specific, but the new step will get run for openswan too...


> core: build libreswan and openswan services from the same sources

The .desktop files are unused. Kill 'em.

But this approach seems wrong to me; it means if you upgrade from openswan to libreswan, then you need to create a new connection, which is identical to the old one other than using the "IPsec based VPN (libreswan)" type rather than the "IPsec based VPN (openswan)" type. Except that, no, actually you don't, because either plugin will run either binary if it's available? So you have two different plugins, one of which claims to be "openswan" and one which claims to be "libreswan", but they are otherwise completely identical in behavior?

(In reply to comment #1)
> But this approach seems wrong to me; it means if you upgrade from openswan to
> libreswan, then you need to create a new connection, which is identical to the
> old one other than using the "IPsec based VPN (libreswan)" type rather than the
> "IPsec based VPN (openswan)" type. Except that, no, actually you don't, because
> either plugin will run either binary if it's available? So you have two
> different plugins, one of which claims to be "openswan" and one which claims to
> be "libreswan", but they are otherwise completely identical in behavior?

Kinda.  It's a tough call; openswan and libreswan are expected to diverge in the future, but right now it's near-free to support both in the same code.  This also means we don't have to rename the whole thing right now, but might in the future or might fork it in the future.  I don't actually know.  But they aren't 100% compatible and aren't expected to be.

Also, during the switch-over in Fedora I told Avesh that we should use a new D-Bus service name for the libreswan stuff because it was unclear whether we'd fork the existing openswan plugin or whether we'd just use the existing sources and rename.  So anything F20+ and RHE7 already uses 'org.freedesktop.NetworkManager.libreswan' and we need some kind of contingency for that.

I could certainly change the nm-openswan-service binary to be built like the others, and only work with one specific service if that makes things cleaner?

>> core: make find_ipsec() generic

+    static const char *paths[] = {
+         PREFIX "/sbin/",
+         PREFIX "/bin/",
+         "/sbin/",
+         "/usr/sbin/",
+         "/usr/local/sbin/",
+         "/usr/bin/",
+         "/usr/local/bin/",
+    };

now also searches the /bin/ paths. That seems wrong.

You could pass "paths" as argument to find_helper() and pass different ones depending on which binary you search.



Rest LGTM

(In reply to comment #2)
> Kinda.  It's a tough call; openswan and libreswan are expected to diverge in
> the future

OK... but, "diverge" as in "openswan is expected to stay dead and libreswan is expected to keep being developed", right? Don't we expect everyone to eventually migrate to libreswan? In which case, is there any need to support having both openswan and libreswan support on the same machine? People/distros can just switch from openswan to libreswan at some point, and have all their old "openswan" connections just start using libreswan. (Though I guess we'll need to do something to migrate them from vpn.type=openswan to vpn.type=libreswan at some point.)

(In reply to comment #4)
> (In reply to comment #2)
> > Kinda.  It's a tough call; openswan and libreswan are expected to diverge in
> > the future
> 
> OK... but, "diverge" as in "openswan is expected to stay dead and libreswan is
> expected to keep being developed", right? Don't we expect everyone to

Yes, more or less.  openswan isn't quite dead, and Xelerance is continuing to develop it, but it doesn't have the momentum or pace that libreswan does.

> eventually migrate to libreswan? In which case, is there any need to support
> having both openswan and libreswan support on the same machine? People/distros

At this point, we don't expect this to happen because openswan/libreswan use the same file paths for everything, including whack/pluto/ipsec etc.  So you cannot parallel install them and they should conflict, but it's a simple 'yum remove/yum install' (or the equivalent dpkg) to get the other.  Note that in Fedora libreswan obsoletes openswan so we don't even ship openswan anymore, but that's not the case in other distros.

> can just switch from openswan to libreswan at some point, and have all their
> old "openswan" connections just start using libreswan. (Though I guess we'll
> need to do something to migrate them from vpn.type=openswan to
> vpn.type=libreswan at some point.)

That *might* work, but I wouldn't count on it if/when the configurations start to diverge.  There's also some things that we want from libreswan (like having them stop calling nm-libreswan-service-helper directly) that will make NM-libreswan diverge further.

Maybe the solution is to just fork NM-openswan, but it wasn't clear whether that was currently worth it...

(In reply to comment #2)
> So anything F20+ and RHE7 already uses
> 'org.freedesktop.NetworkManager.libreswan' and we need some kind of contingency
> for that.

NetworkManager-openswan-0.9.8.4-2.fc20.x86_64 still uses o.fd.NM.openswan...

(In reply to comment #5)
> > In which case, is there any need to support
> > having both openswan and libreswan support on the same machine? 
> 
> At this point, we don't expect this to happen because openswan/libreswan use
> the same file paths for everything, including whack/pluto/ipsec etc.  So you
> cannot parallel install them and they should conflict, but it's a simple 'yum
> remove/yum install' (or the equivalent dpkg) to get the other.

So, I think we should try to avoid:

  1. users upgrading their distro and finding that their VPN no longer
     works because the distro upgraded from openswan to libreswan

  2. users trying to create a new VPN and having to decide between
     two seemingly-identical IPsec options

Given that as of NM 1.0 we are not using any libreswan-specific functionality, it seems like the simplest solution is to just continue using the name "openswan" in all API-like situations (D-Bus, vpn.type, etc), but always compile in support for both openswan and libreswan (ie, no --enable flags for that) and be able to use either one at runtime.

We can change NMVPNPlugin:name to "openswan/libreswan" so that both names show up in the UI.

(In reply to comment #6)
> (In reply to comment #2)
> > So anything F20+ and RHE7 already uses
> > 'org.freedesktop.NetworkManager.libreswan' and we need some kind of contingency
> > for that.
> 
> NetworkManager-openswan-0.9.8.4-2.fc20.x86_64 still uses o.fd.NM.openswan...
> 
> (In reply to comment #5)
> > > In which case, is there any need to support
> > > having both openswan and libreswan support on the same machine? 
> > 
> > At this point, we don't expect this to happen because openswan/libreswan use
> > the same file paths for everything, including whack/pluto/ipsec etc.  So you
> > cannot parallel install them and they should conflict, but it's a simple 'yum
> > remove/yum install' (or the equivalent dpkg) to get the other.
> 
> So, I think we should try to avoid:
> 
>   1. users upgrading their distro and finding that their VPN no longer
>      works because the distro upgraded from openswan to libreswan
> 
>   2. users trying to create a new VPN and having to decide between
>      two seemingly-identical IPsec options
> 
> Given that as of NM 1.0 we are not using any libreswan-specific functionality,
> it seems like the simplest solution is to just continue using the name
> "openswan" in all API-like situations (D-Bus, vpn.type, etc), but always
> compile in support for both openswan and libreswan (ie, no --enable flags for
> that) and be able to use either one at runtime.
> 
> We can change NMVPNPlugin:name to "openswan/libreswan" so that both names show
> up in the UI.

I dunno. I would be find with building two (mostly) separate plugins out of the same source.

I think it's correct to treat libre- and openswan as entirely different plugins (requiring different vpn-types, and separate DBUS paths, etc.)

I feel it would be great if one package could provide both plugins at the same time (and choosing at runtime).
However, as dcbw says, that might not be worth the effort, because most users won't have both applications installed at the same time.
-- OTOH, it might be worth the effort, because it could simplify packaging.


The update path for users who migrate from openswan to libreswan could be improved by adding a function "import libreswan VPN config from existing openswan VPN" (and vice versa) to  NM-*swan-gnome
(which for now would simply copy the connection and resetting the vpn-type)

(In reply to comment #6)
> (In reply to comment #2)
> > So anything F20+ and RHE7 already uses
> > 'org.freedesktop.NetworkManager.libreswan' and we need some kind of contingency
> > for that.
> 
> NetworkManager-openswan-0.9.8.4-2.fc20.x86_64 still uses o.fd.NM.openswan...

I thought Avesh had done that...  Ugh.  So I guess it's only RHEL7.0 that uses 'org.fdo.NM.libreswan' right now, which is almost worse.

> (In reply to comment #5)
> > > In which case, is there any need to support
> > > having both openswan and libreswan support on the same machine? 
> > 
> > At this point, we don't expect this to happen because openswan/libreswan use
> > the same file paths for everything, including whack/pluto/ipsec etc.  So you
> > cannot parallel install them and they should conflict, but it's a simple 'yum
> > remove/yum install' (or the equivalent dpkg) to get the other.
> 
> So, I think we should try to avoid:
> 
>   1. users upgrading their distro and finding that their VPN no longer
>      works because the distro upgraded from openswan to libreswan

I tend to agree, but I'm concerned about a year from now when things diverge.  How long do we try to keep compatibility?

>   2. users trying to create a new VPN and having to decide between
>      two seemingly-identical IPsec options

I certainly agree on that.

> Given that as of NM 1.0 we are not using any libreswan-specific functionality,
> it seems like the simplest solution is to just continue using the name
> "openswan" in all API-like situations (D-Bus, vpn.type, etc), but always
> compile in support for both openswan and libreswan (ie, no --enable flags for
> that) and be able to use either one at runtime.
> 
> We can change NMVPNPlugin:name to "openswan/libreswan" so that both names show
> up in the UI.

The only issue I think we have is the RHEL7.0 issue.  We can (as upstream) do what you suggest, and then Avesh can continue patching the package in RHEL to rename everything?  That would work I suppose.

(In reply to comment #8)
> >   1. users upgrading their distro and finding that their VPN no longer
> >      works because the distro upgraded from openswan to libreswan
> 
> I tend to agree, but I'm concerned about a year from now when things diverge. 
> How long do we try to keep compatibility?

If we want to add libreswan-specific features, but we think that there will still also be openswan users at that point, then we could just make it so that that if you try to activate a libreswan-specific connection using the openswan binaries, it returns an activation error. (But as long as you don't use any of the new features, the openswan binaries continue to work fine).

Eventually, when we're satisfied that no one cares about openswan any more, we can drop support for it, and figure out how to rename everything without breaking existing connections.

> The only issue I think we have is the RHEL7.0 issue.  We can (as upstream) do
> what you suggest, and then Avesh can continue patching the package in RHEL to
> rename everything?  That would work I suppose.

And presumably by the time we reach RHEL8, we'll have renamed upstream as well, so there won't be any problems with the 7->8 upgrade path.

On fedora, libreswan package installs binaries such as:
  /usr/libexec/ipsec/pluto

that is missing if PREFIX!=/usr.

»···static const char *paths[] = {
»···»···PREFIX "/sbin/",
»···»···PREFIX "/bin/",
»···»···PREFIX "/libexec/ipsec/",
»···»···PREFIX "/lib/ipsec/",
»···»···"/sbin/",
»···»···"/usr/sbin/",
»···»···"/usr/local/sbin/",
»···»···"/usr/bin/",
»···»···"/usr/local/bin/",
»···»···"/usr/libexec/",
»···»···"/usr/local/libexec/ipsec/"
»···»···"/usr/lib/ipsec/",
»···»···"/usr/local/lib/ipsec/"
»···};



-     "/usr/libexec/",
+     "/usr/libexec/ipsec/",

Added a fixup

fixup! determine swan implementation prior to calling ipsec_stop()

The pty password chat needs change to selinux policy: https://github.com/selinux-policy/selinux-policy/pull/8

libreswan may not call the helper if there's not dns and banner. Fix here:
https://github.com/libreswan/libreswan/pull/22

The fix for the trailing \n issue is here, you may want to adjust the comment:
https://github.com/libreswan/libreswan/pull/20

Otherwise looks/works fine for me.

(In reply to comment #1)
> > core: update for non-deprecated libreswan initialization flow
> 
> >+AC_USE_SYSTEM_EXTENSIONS
> >+AC_GNU_SOURCE
> 
> That's redundant. The former implies the latter.

Fixed.

> > 		PREFIX "/sbin/",
> > 		PREFIX "/bin/",
> >+		PREFIX "/libexec/ipsec/",
> >+		PREFIX "/lib/ipsec/",
> 
> This seems a little weird; you have one program that you expect to find in
> sbin, and one you expect to find in libexec, but you search for both in both
> paths...

Split into a _bin and _libexec helper.

> >+			success = do_spawn (&priv->pid, NULL, NULL, error, stackman_path, "start", NULL);
> >+			if (success)
> >+				priv->watch_id = g_child_watch_add (priv->pid, pluto_watch_cb, self);
> 
> Rename pluto_watch_cb?

Done.

> >+		libreswan = !!(output && strcasestr (output, " Libreswan "));
> 
> && in C always returns 0 or 1. The "!!()" is redundant.

Done.

> However, maybe you should set priv->libreswan based on whether the additional
> binaries (_stackmanager, whack, etc) are present?

If they aren't present, then we can't do stuff, so I don't think falling back to the openswan codepaths at that point would be useful.

> > core: wait for connection addition to be ready
> 
> The commit message talks about "whack", suggesting this is libreswan-specific,
> but the new step will get run for openswan too...

Updated.

> > core: build libreswan and openswan services from the same sources
> 
> The .desktop files are unused. Kill 'em.

Kill.

> But this approach seems wrong to me; it means if you upgrade from openswan to
> libreswan, then you need to create a new connection, which is identical to the
> old one other than using the "IPsec based VPN (libreswan)" type rather than the
> "IPsec based VPN (openswan)" type. Except that, no, actually you don't, because
> either plugin will run either binary if it's available? So you have two
> different plugins, one of which claims to be "openswan" and one which claims to
> be "libreswan", but they are otherwise completely identical in behavior?

I have dropped that big copy & rename patch, and instead just symlinked the helper to nm-libreswan-service-helper so that we get called.  I'll let Avesh just sed the whole thing if he wants for Fedora/RHEL.

Repushed with fixups.

(In reply to comment #10)
> On fedora, libreswan package installs binaries such as:
>   /usr/libexec/ipsec/pluto
> 
> that is missing if PREFIX!=/usr.

See the updated commit "core: update for non-deprecated libreswan initialization flow" which does the change you suggest.

(In reply to comment #13)
> I have dropped that big copy & rename patch, and instead just symlinked the
> helper to nm-libreswan-service-helper so that we get called.

You need an uninstall hook to remove the symlink too, or distcheck will fail.

Everything else looks good.

Branch merged to master after adding uninstall-hook and passing distcheck.