| Message ID | 1515482173-29447-1-git-send-email-eyal.birger@gmail.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [Openvpn-devel] Windows: Set interface IPv6 prefix length when configuring address | expand |
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 <<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>> 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'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 <<a href="mailto:eyal.birger@gmail.com" target="_blank">eyal.birger@gmail.com</a>> wrote:<br> > Address prefix length defaults to /64 on Windows. This change allows using<br> > Windows clients in setups that use a different prefix length.<br> ><br> > Note: the ability to set the prefix length is documented in the netsh<br> > 'add address' command, but works on the 'set address' 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> ><br> > Signed-off-by: Eyal Birger <<a href="mailto:eyal.birger@gmail.com" target="_blank">eyal.birger@gmail.com</a>><br> > ---<br> > src/openvpn/tun.c | 7 ++++---<br> > 1 file changed, 4 insertions(+), 3 deletions(-)<br> ><br> > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c<br> > index 25831ce..b2b4795 100644<br> > --- a/src/openvpn/tun.c<br> > +++ b/src/openvpn/tun.c<br> > @@ -1561,15 +1561,16 @@ do_ifconfig(struct tuntap *tt,<br> > }<br> > else<br> > {<br> > - /* example: netsh interface ipv6 set address interface=42 2001:608:8003::d store=active */<br> > + /* example: netsh interface ipv6 set address interface=42 2001:608:8003::d/64 store=active */<br> > char iface[64];<br> > openvpn_snprintf(iface, sizeof(iface), "interface=%lu", tt->adapter_index );<br> > argv_printf(&argv,<br> > - "%s%sc interface ipv6 set address %s %s store=active",<br> > + "%s%sc interface ipv6 set address %s %s/%d store=active",<br> > get_win_sys_path(),<br> > NETSH_PATH_SUFFIX,<br> > iface,<br> > - ifconfig_ipv6_local );<br> > + ifconfig_ipv6_local,<br> > + tt->netbits_ipv6);<br> > netsh_command(&argv, 4, M_FATAL);<br> > /* set ipv6 dns servers if any are specified */<br> > netsh_set_dns6_servers(tt->options.dns6, tt->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 <<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>><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 <<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>> :)<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 <<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>> 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> > Going through patchworks noticed this.<br> > <br> > Thankfully this never got committed so here goes a retraction.<br> > <br> > On Sun, Jan 21, 2018 at 1:45 PM Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>> wrote:<br> > <br> > > Hi,<br> > ><br> > > I'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 <<a href="mailto:eyal.birger@gmail.com" target="_blank">eyal.birger@gmail.com</a>> wrote:<br> > > > Address prefix length defaults to /64 on Windows. This change allows<br> > > using<br> > > > Windows clients in setups that use a different prefix length.<br> <br> I had this on "I need to do more testing with this", which is why it<br> never proceeded - there might be unintended side effects, so I wanted<br> to do more detailed testing with "netsh" and with "iservice" 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't answer (it<br> will only answer fe80::8) thus causing a "black hole route" 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->netbits_ipv6 is passed as prefix_len to the ip helper API),<br>don't we? <br><br></div><div>Selva<br></div></div></div></div></div>
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);
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(-)