[Openvpn-devel] Windows: Set interface IPv6 prefix length when configuring address

  Address prefix length defaults to /64 on Windows. This change allows using
Windows clients in setups that use a different prefix length.

Note: the ability to set the prefix length is documented in the netsh
'add address' command, but works on the 'set address' command as well.

Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
---
 src/openvpn/tun.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
  

Hi,

I'm on a reviewing spree (doing my penance), so here goes..

Thanks for the patch

On Tue, Jan 9, 2018 at 2:16 AM, Eyal Birger <eyal.birger@gmail.com> wrote:
> Address prefix length defaults to /64 on Windows. This change allows using
> Windows clients in setups that use a different prefix length.
>
> Note: the ability to set the prefix length is documented in the netsh
> 'add address' command, but works on the 'set address' command as well.

Aside:
If interactive service is in use, the ip helper API is used and setting
prefix already works.  Ideally I would like to see openvpn on Windows
used only with the interactive service, but we are not there yet --
instances started by the automatic service does not use it and there
are some users still running the GUI as admin for some inexplicable
reasons.

So we need to continue supporting these code paths.

>
> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> ---
>  src/openvpn/tun.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 25831ce..b2b4795 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -1561,15 +1561,16 @@ do_ifconfig(struct tuntap *tt,
>              }
>              else
>              {
> -                /* example: netsh interface ipv6 set address interface=42 2001:608:8003::d store=active */
> +                /* example: netsh interface ipv6 set address interface=42 2001:608:8003::d/64 store=active */
>                  char iface[64];
>                  openvpn_snprintf(iface, sizeof(iface), "interface=%lu", tt->adapter_index );
>                  argv_printf(&argv,
> -                            "%s%sc interface ipv6 set address %s %s store=active",
> +                            "%s%sc interface ipv6 set address %s %s/%d store=active",
>                              get_win_sys_path(),
>                              NETSH_PATH_SUFFIX,
>                              iface,
> -                            ifconfig_ipv6_local );
> +                            ifconfig_ipv6_local,
> +                            tt->netbits_ipv6);
>                  netsh_command(&argv, 4, M_FATAL);
>                  /* set ipv6 dns servers if any are specified */
>                  netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len, actual);

Works as expected and the code is good.

Currently, on setting the address, a default route gets set with
prefix /64 with gateway as OnLink (does not happen when iphelper api
is used). Although our explicit route to fe80::8 may override it, it
looks better to set the correct prefix in the address. So:

Acked-by: Selva Nair <selva.nair@gmail.com>

Selva

P.S. While going through this I noticed a bug in our route deletion code
for ipv6: only when using netsh (not the interactiveservice), so gone
unnoticed. Will report separately.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
  

Hi,

Going through patchworks noticed this.

Thankfully this never got committed so here goes a retraction.

On Sun, Jan 21, 2018 at 1:45 PM Selva Nair <selva.nair@gmail.com> wrote:

> Hi,
>
> I'm on a reviewing spree (doing my penance), so here goes..
>
> Thanks for the patch
>
> On Tue, Jan 9, 2018 at 2:16 AM, Eyal Birger <eyal.birger@gmail.com> wrote:
> > Address prefix length defaults to /64 on Windows. This change allows
> using
> > Windows clients in setups that use a different prefix length.
> >
> > Note: the ability to set the prefix length is documented in the netsh
> > 'add address' command, but works on the 'set address' command as well.
>
> Aside:
> If interactive service is in use, the ip helper API is used and setting
> prefix already works.  Ideally I would like to see openvpn on Windows
> used only with the interactive service, but we are not there yet --
> instances started by the automatic service does not use it and there
> are some users still running the GUI as admin for some inexplicable
> reasons.
>
> So we need to continue supporting these code paths.
>
> >
> > Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> > ---
> >  src/openvpn/tun.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> > index 25831ce..b2b4795 100644
> > --- a/src/openvpn/tun.c
> > +++ b/src/openvpn/tun.c
> > @@ -1561,15 +1561,16 @@ do_ifconfig(struct tuntap *tt,
> >              }
> >              else
> >              {
> > -                /* example: netsh interface ipv6 set address
> interface=42 2001:608:8003::d store=active */
> > +                /* example: netsh interface ipv6 set address
> interface=42 2001:608:8003::d/64 store=active */
> >                  char iface[64];
> >                  openvpn_snprintf(iface, sizeof(iface), "interface=%lu",
> tt->adapter_index );
> >                  argv_printf(&argv,
> > -                            "%s%sc interface ipv6 set address %s %s
> store=active",
> > +                            "%s%sc interface ipv6 set address %s %s/%d
> store=active",
> >                              get_win_sys_path(),
> >                              NETSH_PATH_SUFFIX,
> >                              iface,
> > -                            ifconfig_ipv6_local );
> > +                            ifconfig_ipv6_local,
> > +                            tt->netbits_ipv6);
> >                  netsh_command(&argv, 4, M_FATAL);
> >                  /* set ipv6 dns servers if any are specified */
> >                  netsh_set_dns6_servers(tt->options.dns6,
> tt->options.dns6_len, actual);
>
> Works as expected and the code is good.
>
> Currently, on setting the address, a default route gets set with
> prefix /64 with gateway as OnLink (does not happen when iphelper api
> is used). Although our explicit route to fe80::8 may override it, it
> looks better to set the correct prefix in the address. So:
>
> Acked-by: Selva Nair <selva.nair@gmail.com>
>

Though this works in my tests I want to retract this ACK.

Apart from possible issues due to the appearance of the onlink route in
some cases, I think the correct approach going forward is to stop using
netsh and use the IP helper API for such tasks. And do it in the same way
as done using the service.

Un-Acked-by: Selva Nair <selva.nair@gmail.com> :)

Selva
<div dir="ltr"><div>Hi,<br><br></div><div>Going through patchworks noticed this.<br><br></div>Thankfully this never got committed so here goes a retraction.<br><div><div><br><div class="gmail_quote"><div dir="ltr">On Sun, Jan 21, 2018 at 1:45 PM Selva Nair &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
I&#39;m on a reviewing spree (doing my penance), so here goes..<br>
<br>
Thanks for the patch<br>
<br>
On Tue, Jan 9, 2018 at 2:16 AM, Eyal Birger &lt;<a href="mailto:eyal.birger@gmail.com" target="_blank">eyal.birger@gmail.com</a>&gt; wrote:<br>
&gt; Address prefix length defaults to /64 on Windows. This change allows using<br>
&gt; Windows clients in setups that use a different prefix length.<br>
&gt;<br>
&gt; Note: the ability to set the prefix length is documented in the netsh<br>
&gt; &#39;add address&#39; command, but works on the &#39;set address&#39; command as well.<br>
<br>
Aside:<br>
If interactive service is in use, the ip helper API is used and setting<br>
prefix already works.  Ideally I would like to see openvpn on Windows<br>
used only with the interactive service, but we are not there yet --<br>
instances started by the automatic service does not use it and there<br>
are some users still running the GUI as admin for some inexplicable<br>
reasons.<br>
<br>
So we need to continue supporting these code paths.<br>
<br>
&gt;<br>
&gt; Signed-off-by: Eyal Birger &lt;<a href="mailto:eyal.birger@gmail.com" target="_blank">eyal.birger@gmail.com</a>&gt;<br>
&gt; ---<br>
&gt;  src/openvpn/tun.c | 7 ++++---<br>
&gt;  1 file changed, 4 insertions(+), 3 deletions(-)<br>
&gt;<br>
&gt; diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c<br>
&gt; index 25831ce..b2b4795 100644<br>
&gt; --- a/src/openvpn/tun.c<br>
&gt; +++ b/src/openvpn/tun.c<br>
&gt; @@ -1561,15 +1561,16 @@ do_ifconfig(struct tuntap *tt,<br>
&gt;              }<br>
&gt;              else<br>
&gt;              {<br>
&gt; -                /* example: netsh interface ipv6 set address interface=42 2001:608:8003::d store=active */<br>
&gt; +                /* example: netsh interface ipv6 set address interface=42 2001:608:8003::d/64 store=active */<br>
&gt;                  char iface[64];<br>
&gt;                  openvpn_snprintf(iface, sizeof(iface), &quot;interface=%lu&quot;, tt-&gt;adapter_index );<br>
&gt;                  argv_printf(&amp;argv,<br>
&gt; -                            &quot;%s%sc interface ipv6 set address %s %s store=active&quot;,<br>
&gt; +                            &quot;%s%sc interface ipv6 set address %s %s/%d store=active&quot;,<br>
&gt;                              get_win_sys_path(),<br>
&gt;                              NETSH_PATH_SUFFIX,<br>
&gt;                              iface,<br>
&gt; -                            ifconfig_ipv6_local );<br>
&gt; +                            ifconfig_ipv6_local,<br>
&gt; +                            tt-&gt;netbits_ipv6);<br>
&gt;                  netsh_command(&amp;argv, 4, M_FATAL);<br>
&gt;                  /* set ipv6 dns servers if any are specified */<br>
&gt;                  netsh_set_dns6_servers(tt-&gt;options.dns6, tt-&gt;options.dns6_len, actual);<br>
<br>
Works as expected and the code is good.<br>
<br>
Currently, on setting the address, a default route gets set with<br>
prefix /64 with gateway as OnLink (does not happen when iphelper api<br>
is used). Although our explicit route to fe80::8 may override it, it<br>
looks better to set the correct prefix in the address. So:<br>
<br>
Acked-by: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;<br></blockquote><div><br></div><div>Though this works in my tests I want to retract this ACK.<br><br>Apart from possible issues due to the appearance of the onlink route in some cases, I think the correct approach going forward is to stop using netsh and use the IP helper API for such tasks. And do it in the same way as done using the service. <br><br></div><div>Un-Acked-by: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt; :)<br><br></div><div>Selva<br></div></div></div></div></div>
  

On 16/10/18 23:48, Selva Nair wrote:
> Hi,
> 
> Going through patchworks noticed this.
> 
> Thankfully this never got committed so here goes a retraction.
> 
> On Sun, Jan 21, 2018 at 1:45 PM Selva Nair <selva.nair@gmail.com
> <mailto:selva.nair@gmail.com>> wrote:
> 
>     Hi,
> 
>     I'm on a reviewing spree (doing my penance), so here goes..
> 
>     Thanks for the patch
> 
>     On Tue, Jan 9, 2018 at 2:16 AM, Eyal Birger <eyal.birger@gmail.com
>     <mailto:eyal.birger@gmail.com>> wrote:
>     > Address prefix length defaults to /64 on Windows. This change allows using
>     > Windows clients in setups that use a different prefix length.
>     >
>     > Note: the ability to set the prefix length is documented in the netsh
>     > 'add address' command, but works on the 'set address' command as well.
> 
>     Aside:
>     If interactive service is in use, the ip helper API is used and setting
>     prefix already works.  Ideally I would like to see openvpn on Windows
>     used only with the interactive service, but we are not there yet --
>     instances started by the automatic service does not use it and there
>     are some users still running the GUI as admin for some inexplicable
>     reasons.
> 
>     So we need to continue supporting these code paths.
> 
>     >
>     > Signed-off-by: Eyal Birger <eyal.birger@gmail.com
>     <mailto:eyal.birger@gmail.com>>
>     > ---
>     >  src/openvpn/tun.c | 7 ++++---
>     >  1 file changed, 4 insertions(+), 3 deletions(-)
>     >
>     > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
>     > index 25831ce..b2b4795 100644
>     > --- a/src/openvpn/tun.c
>     > +++ b/src/openvpn/tun.c
>     > @@ -1561,15 +1561,16 @@ do_ifconfig(struct tuntap *tt,
>     >              }
>     >              else
>     >              {
>     > -                /* example: netsh interface ipv6 set address
>     interface=42 2001:608:8003::d store=active */
>     > +                /* example: netsh interface ipv6 set address
>     interface=42 2001:608:8003::d/64 store=active */
>     >                  char iface[64];
>     >                  openvpn_snprintf(iface, sizeof(iface), "interface=%lu",
>     tt->adapter_index );
>     >                  argv_printf(&argv,
>     > -                            "%s%sc interface ipv6 set address %s %s
>     store=active",
>     > +                            "%s%sc interface ipv6 set address %s %s/%d
>     store=active",
>     >                              get_win_sys_path(),
>     >                              NETSH_PATH_SUFFIX,
>     >                              iface,
>     > -                            ifconfig_ipv6_local );
>     > +                            ifconfig_ipv6_local,
>     > +                            tt->netbits_ipv6);
>     >                  netsh_command(&argv, 4, M_FATAL);
>     >                  /* set ipv6 dns servers if any are specified */
>     >                  netsh_set_dns6_servers(tt->options.dns6,
>     tt->options.dns6_len, actual);
> 
>     Works as expected and the code is good.
> 
>     Currently, on setting the address, a default route gets set with
>     prefix /64 with gateway as OnLink (does not happen when iphelper api
>     is used). Although our explicit route to fe80::8 may override it, it
>     looks better to set the correct prefix in the address. So:
> 
>     Acked-by: Selva Nair <selva.nair@gmail.com <mailto:selva.nair@gmail.com>>
> 
> 
> Though this works in my tests I want to retract this ACK.
> 
> Apart from possible issues due to the appearance of the onlink route in some
> cases, I think the correct approach going forward is to stop using netsh and
> use the IP helper API for such tasks. And do it in the same way as done using
> the service.
> 
> Un-Acked-by: Selva Nair <selva.nair@gmail.com <mailto:selva.nair@gmail.com>> :)

That was close .... I looked at this yesterday, but since the contributor was
a new person and changing details in code paths I'm not that deep into, I
wanted to have a much closer look before applying it - luckily I didn't have
enough brainpower yesterday to dive into this one.  And since you had ACKed
it, I considered it generally being safe enough for further processing.

So thanks for having a closer look again and notify us instantly :)

I'll ensure this patch is tagged as rejected in patchwork.
  

Hi,

On Tue, Oct 16, 2018 at 05:48:29PM -0400, Selva Nair wrote:
> Going through patchworks noticed this.
> 
> Thankfully this never got committed so here goes a retraction.
> 
> On Sun, Jan 21, 2018 at 1:45 PM Selva Nair <selva.nair@gmail.com> wrote:
> 
> > Hi,
> >
> > I'm on a reviewing spree (doing my penance), so here goes..
> >
> > Thanks for the patch
> >
> > On Tue, Jan 9, 2018 at 2:16 AM, Eyal Birger <eyal.birger@gmail.com> wrote:
> > > Address prefix length defaults to /64 on Windows. This change allows
> > using
> > > Windows clients in setups that use a different prefix length.

I had this on "I need to do more testing with this", which is why it
never proceeded - there might be unintended side effects, so I wanted
to do more detailed testing with "netsh" and with "iservice" backends,
and different prefix lengths etc.

Also, I had the suspicion that if we actually set a prefix length, it
will lead to local ND traffic which the tap driver won't answer (it
will only answer fe80::8) thus causing a "black hole route" for the
tap subnet...


[..]
> Though this works in my tests I want to retract this ACK.
> 
> Apart from possible issues due to the appearance of the onlink route in
> some cases, I think the correct approach going forward is to stop using
> netsh and use the IP helper API for such tasks. And do it in the same way
> as done using the service.
> 
> Un-Acked-by: Selva Nair <selva.nair@gmail.com> :)

Thanks :-)

We'll definitely need to look into this more closely.

gert
  

Hi,

On Wed, Oct 17, 2018 at 8:07 AM Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Tue, Oct 16, 2018 at 05:48:29PM -0400, Selva Nair wrote:
> > Going through patchworks noticed this.
> >
> > Thankfully this never got committed so here goes a retraction.
> >
> > On Sun, Jan 21, 2018 at 1:45 PM Selva Nair <selva.nair@gmail.com> wrote:
> >
> > > Hi,
> > >
> > > I'm on a reviewing spree (doing my penance), so here goes..
> > >
> > > Thanks for the patch
> > >
> > > On Tue, Jan 9, 2018 at 2:16 AM, Eyal Birger <eyal.birger@gmail.com>
> wrote:
> > > > Address prefix length defaults to /64 on Windows. This change allows
> > > using
> > > > Windows clients in setups that use a different prefix length.
>
> I had this on "I need to do more testing with this", which is why it
> never proceeded - there might be unintended side effects, so I wanted
> to do more detailed testing with "netsh" and with "iservice" backends,
> and different prefix lengths etc.
>
> Also, I had the suspicion that if we actually set a prefix length, it
> will lead to local ND traffic which the tap driver won't answer (it
> will only answer fe80::8) thus causing a "black hole route" for the
> tap subnet...
>

Aha, that explains why it lingered in that state for long :)

However, we do currently set the prefix-length while using the
service (that->netbits_ipv6 is passed as prefix_len to the ip helper API),
don't we?

Selva
<div dir="ltr"><div dir="ltr">Hi,<br><div><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 17, 2018 at 8:07 AM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On Tue, Oct 16, 2018 at 05:48:29PM -0400, Selva Nair wrote:<br>
&gt; Going through patchworks noticed this.<br>
&gt; <br>
&gt; Thankfully this never got committed so here goes a retraction.<br>
&gt; <br>
&gt; On Sun, Jan 21, 2018 at 1:45 PM Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt; wrote:<br>
&gt; <br>
&gt; &gt; Hi,<br>
&gt; &gt;<br>
&gt; &gt; I&#39;m on a reviewing spree (doing my penance), so here goes..<br>
&gt; &gt;<br>
&gt; &gt; Thanks for the patch<br>
&gt; &gt;<br>
&gt; &gt; On Tue, Jan 9, 2018 at 2:16 AM, Eyal Birger &lt;<a href="mailto:eyal.birger@gmail.com" target="_blank">eyal.birger@gmail.com</a>&gt; wrote:<br>
&gt; &gt; &gt; Address prefix length defaults to /64 on Windows. This change allows<br>
&gt; &gt; using<br>
&gt; &gt; &gt; Windows clients in setups that use a different prefix length.<br>
<br>
I had this on &quot;I need to do more testing with this&quot;, which is why it<br>
never proceeded - there might be unintended side effects, so I wanted<br>
to do more detailed testing with &quot;netsh&quot; and with &quot;iservice&quot; backends,<br>
and different prefix lengths etc.<br>
<br>
Also, I had the suspicion that if we actually set a prefix length, it<br>
will lead to local ND traffic which the tap driver won&#39;t answer (it<br>
will only answer fe80::8) thus causing a &quot;black hole route&quot; for the<br>
tap subnet...<br></blockquote><div><br></div><div>Aha, that explains why it lingered in that state for long :)<br><br></div><div>However, we do currently set the prefix-length while using the<br></div><div>service (that-&gt;netbits_ipv6 is passed as prefix_len to the ip helper API),<br>don&#39;t we? <br><br></div><div>Selva<br></div></div></div></div></div>