RFC: [PATCH] vpnc-script: split disconnect and destroy

[Antonio Borneo]

On Sun, Feb 16, 2014 at 7:15 PM, David Woodhouse <dwmw2 at infradead.org> wrote:
> On Sun, 2014-02-16 at 12:48 +0800, Antonio Borneo wrote:
>> Hi David,
>>
>> I'm reworking a patch I have received for vpnc
>> http://lists.unix-ag.uni-kl.de/pipermail/vpnc-devel/2013-December/004037.html
>> in a set of new patches that I'm going to apply soon.
>>
>> The key point is the split, in vpnc-script, of do_disconnect()
>> by creating a new do_destroy().
>> I have isolated this part in the patch in attachment.
>>
>> For backward compatibility I kept the original behavior for
>> reason=disconnect, but I consider this name as incorrect and
>> obsolete so added synonym reason=disconnect_destroy.
>
> For me the most interesting case of backward compatibility has been
> using an old script with newer VPN client. At least in the past with
> various systems shipping vpnc-script from the vpnc distribution while
> OpenConnect required various new features, that's the one I've had to
> deal with more.
>
> So I'm wondering if a better approach would be to continue to use
> 'disconnect', but add a new environment variable which indicates that
> this is a new VPN client that supports a separate 'destroy' invocation.
>
> So instead of the plethora of
> 'disconnect/disconnect_only/disconnect_destroy' methods, we just have
> 'disconnect' with a modifier in a separate variable. And 'destroy'.
>
> That way, all forms of compatibility should work. New clients invoking
> an old script will call 'disconnect', which will do the destroy
> operation too, and then call the unsupported 'destroy' method.
>
> Old clients invoking a new script will calls 'disconnect' but without
> that extra 'disconnect=disconnect_only' environment variable (or
> whatever we choose to call it), and thus the new script will do what
> they expect.

Well, my idea was to first patch the scripts in vpnc and your
repository, then modify the clients (vpnc and if needed also
openconnect).

Anyway with my proposal it's easy to make new clients working with old
(actually current) script.
When the client calls any of the the new methods, the old script prints
   unknown reason 'destroy'. Maybe vpnc-script is out of date
end exit 1.
Would be up to us to handle this case in the client.

A new client should do something like:
  ret = system("vpnc-script disconnect_only");
  if (ret == 1) {
    printf("additional warning about old script\n");
    system("vpnc-script disconnect");
  }
  .. whatever other stuff ..
  if (ret == 0)
    system("vpnc-script destroy");

For Dan.
This would be transparent to NetworkManager-vpnc.
The helper in src/nm-vpnc-service-vpnc-helper.c line 331
only handles reason=="connect". Exit 0 in the other cases.

Same for NetworkManager-openconnect.
Helper src/nm-openconnect-service-openconnect-helper.c line 489
only handles reason=="connect". Exit 0 in the other cases.

Antonio

>
>> Please notice that destroy_tun_device() has code for *BSD only.
>> If we suppose that on these OS the packages are built with proper
>> dependencies, we can be more aggressive with a cleaner patch
>> that introduces only the reason destroy.
>
> We'd have to look again at that. In the past they've mostly been
> shipping an old version from vpnc which has been problematic for
> openconnect. I think FreeBSD at least did switch to a separate
> vpnc-script package which both their vpnc and openconnect packages
> depend on. And it seems to be up to date as of about two days ago.
> Despite that change being Windows-only :)
>
> The OpenConnect configure script does check for existence of vpnc-script
> in the default location. It could be augmented to check whether the
> script supports the 'destroy' method, perhaps.
>
> But in practice I think it's easier and safer to just ensure
> compatibility in both directions, as described above.
>
>> In this case reason destroy have to be called independently by
>> openconnect and vpnc.
>
> Right.
>
> Did I ever give you access to the vpnc-scripts repository, btw? If you
> mail me a SSH public key (and preferred username), I can.
>
> I have changes to vpnc-script-win.js in that repository, which probably
> don't affect you yet since you don't support IPv6 yet. But there may be
> more changes coming (it doesn't support full-tunnel mode in Legacy IP,
> and should probably *remove* all IP addresses before setting up the
> tunnel and in 'disconnect', etc.).
>
> (Btw, once vpnc is in git as its primary version control, it should be
> simple enough just to do 'git pull' to pull in vpnc-script changes.)
>
> --
> dwmw2