Mailing List Archive

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.

> 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

On Sun, 2014-02-16 at 11:15 +0000, David Woodhouse 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.

+1 from me; that's cleaner to deal with for NetworkManager-vpnc too, I
think.

Dan

> 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.
>
> > 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.)
>
> _______________________________________________
> vpnc-devel mailing list
> vpnc-devel@unix-ag.uni-kl.de
> https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
> http://www.unix-ag.uni-kl.de/~massar/vpnc/


_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Sun, Feb 16, 2014 at 7:15 PM, David Woodhouse <dwmw2@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
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Tue, 2014-02-18 at 15:39 +0800, Antonio Borneo wrote:
> 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");

I don't like this.

Firstly, it's turning the vpnc-script invocation from a
'fire-and-forget' into something more complicated, and it's more fragile
than it used to be.

Secondly, it'll confuse users who see the message about
unknown reason 'disconnect_destroy'
and will lead to more people asking questions about it on random web
fora where they never get good answers, instead of just sending mail to
the list like sensible people. Which always makes me sad.

I would much prefer to stick with 'disconnect', with a separate variable
set to indicate that there will be a subsequent 'destroy' call, and then
the promised 'destroy' call.

> 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.

(Hm, it's a shame those aren't the *same* damn executable.)

--
dwmw2

On Wed, Feb 19, 2014 at 5:26 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Tue, 2014-02-18 at 15:39 +0800, Antonio Borneo wrote:
>> 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");
>
> I don't like this.
>
> Firstly, it's turning the vpnc-script invocation from a
> 'fire-and-forget' into something more complicated, and it's more fragile
> than it used to be.
>
> Secondly, it'll confuse users who see the message about
> unknown reason 'disconnect_destroy'
> and will lead to more people asking questions about it on random web
> fora where they never get good answers, instead of just sending mail to
> the list like sensible people. Which always makes me sad.
>

Current script will print "unknown reason xyz" whatever new reason we add.
But we can silient it redirecting its stdout to /dev/null, using only
for the new reasons a code equivalent to
system("/path/to/vpnc-script > /dev/null");

In such case I would add to new scripts a further reason 'script-version'.
On old script it will fallback in the 'unknown reason' case, so exit 1
The new script will exit 0 and return the version. Or even a trivial
exit $version.

> I would much prefer to stick with 'disconnect', with a separate variable
> set to indicate that there will be a subsequent 'destroy' call, and then
> the promised 'destroy' call.
>

Just to be sure.
You mean that in new scripts:
- reason disconnect plus separate variable == alpha, means disconnect only;
- reason disconnect plus separate variable == beta, means destroy only.
Old script will receive twice call to disconnect (since ignores the
new variable) and we have to verify this double call does not messup
the network.
Correct?

Antonio

>> 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.
>
> (Hm, it's a shame those aren't the *same* damn executable.)
>
> --
> dwmw2
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Thu, 2014-02-20 at 23:31 +0800, Antonio Borneo wrote:
>
> Just to be sure.
> You mean that in new scripts:
> - reason disconnect plus separate variable == alpha, means disconnect only;
> - reason disconnect plus separate variable == beta, means destroy only.

I think I'd use a new reason=destroy for the latter.

> Old script will receive twice call to disconnect (since ignores the
> new variable) and we have to verify this double call does not messup
> the network.

The old script would receive one call to 'disconnect', which would both
disconnect and destroy. And then a call to 'destroy', which would do
nothing.

I concede this will still lead to a complaint from the script, and thus
to random questions on silly web fora, which I held up as a disadvantage
of your approach. But I still prefer it, because it's still
fire-and-forget from the POV of the VPN client.

--
dwmw2