Re: [Openvpn-devel] [PATCH v2] Use adapter index instead of name

[Gert Doering]

Hi,

On Fri, Dec 11, 2015 at 10:29:33PM +0200, Lev Stipakov wrote:
> Thanks, I think (I think!) I got it now.
> 
> 1) Since we have tt->adapter_index (which temporarily disappeared from 
> my perception of reality), no need to add new member to tuntap or 
> tuntap_options.
> 
> 2) tt->adapter_index has nothing to do with rgi->adapter_index, first 
> one is windows adapter index (which we pass to netsh), last one is for 
> routing (which we pass to netsh too in special case).
> 
> 3) In add/del_route* we (try to) use tt->adapter_index.

Right :-)

> Regarding master's "special case" code. Should it be something like this:
> 
>if ( r6->adapter_index )   /* vpn server special route */
>  {
>struct buffer out = alloc_buf_gc (64, );
>buf_printf (, "interface=%d", r6->adapter_index );
>device = buf_bptr();
>gateway_needed = true;
>  }
>else
>  {
>/* device = interface=tt->adapter_index */
>  }

Already committed ;-) - but yes, exactly like this.

So if we need to have route that points elsewhere (/128 to existing
default gateway to enable overlapping v6-in-v6), r6->adapter_index is
set.  If not, the tap index is used.

... and it works (except that my win7 vm started to really misbehave
regarding IPv6 today, so my tests took much longer than planned... grrr)

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

Re: [Openvpn-devel] [PATCH v2] Use adapter index instead of name

[Lev Stipakov]

Thanks, I think (I think!) I got it now.

1) Since we have tt->adapter_index (which temporarily disappeared from 
my perception of reality), no need to add new member to tuntap or 
tuntap_options.


2) tt->adapter_index has nothing to do with rgi->adapter_index, first 
one is windows adapter index (which we pass to netsh), last one is for 
routing (which we pass to netsh too in special case).


3) In add/del_route* we (try to) use tt->adapter_index.

Regarding master's "special case" code. Should it be something like this:

  if ( r6->adapter_index )   /* vpn server special route */
{
  struct buffer out = alloc_buf_gc (64, );
  buf_printf (, "interface=%d", r6->adapter_index );
  device = buf_bptr();
  gateway_needed = true;
}
  else
{
  /* device = interface=tt->adapter_index */
}

-Lev

Re: [Openvpn-devel] [PATCH v2] Use adapter index instead of name

[Gert Doering]

Hi,

On Fri, Dec 11, 2015 at 09:41:56PM +0200, Lev Stipakov wrote:
> Hi,
> 
> >> So, if we want to use index also for "add/del route", I'd gently modify
> >> add/del_route_ipv6 and make it use "interface=" (without breaking
> >> "vpn server special route" case).
> >
> > For consistency, I think we should do that.  What I'd avoid is to do
> > the adapter_index lookup for every single route - which would imply adding
> > the index to struct tuntap_options or something like that which is
> > available in add/del_route[_ipv6]() already.  But this is less elegant
> > than I hoped for, and might have to look different for 2.3 and master.
> 
> Adding the index - you probably meant to "struct tuntap"?

Sort of.  Struct tuntap has a sub-struct "struct tuntap_options options"
which is mostly empty for all platforms but WIN32, and on WIN32 contains
stuff like

  /* --ip-win32 options */
  bool ip_win32_defined;
  int ip_win32_type;

already (tun.h).  So adding "tuntap_adapter_index" or something like this
here would be fully in line with the existing style of packing windows
specific bits nicely away, but having them available where needed...

route_add() gets passed a "struct tuntap *tt", so the option would be
in tt->options.tuntap_adapter_index...

> Master has member "adapter_index", which seems to be used for "special 
> case", release/2.3 does not have that.

Now that's an interesting finding.  I never noticed that, but it basically
*is* what I propose above - not in tuntap_options but in the "main"
struct tuntap - and I had no idea that this one existed (I tend to not
look very closely at #ifdef WIN32 bits unless I have to).

It *is* in 2.3, btw :-) - as per "git blame tun.h" it was introduced 
into James' svn repo before we even had a git repo...


> Do you think it is safe to store adapter index there (would have to add 
> it to 2.3) without breaking "special case"? I personally not so sure, 
> since that "adapter_index" is obtained via "adapter_index_of_ip" in 
> tun.c, which looks quite different from "get_adapter_index_flexible".
> 
> We might have a semantic problem here.

adapter_index_of_ip() seems to be a "find connect routes" thing, not
tied to the tuntap adapter - but that doesn't seem to be connected to
tt->adapter_index.

Don't confuse with "rgi->adapter_index" - *that* is for routing information,
and might point elsewhere.

tt->adapter_index seems to be set only in two places, namely

tun.c:  tt->adapter_index = TUN_ADAPTER_INDEX_INVALID;

and

tun.c:  tt->adapter_index = get_adapter_index (device_guid);
  (in open_tun())

... and that one seems to be sort of like "get_adapter_index_flexible()
lite" - the latter can search for guid *or name*, while the former only
searches by guid.


So... I think we can just *use* that in add/del_route*(), and be happy
everafter :-) - it's not available when we do "netsh... set address",
unfortunately, as open_tun() happens *after* do_ifconfig() on windows...

Definitely worth a try.

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

Re: [Openvpn-devel] [PATCH v2] Use adapter index instead of name

[Lev Stipakov]

Hi,


So, if we want to use index also for "add/del route", I'd gently modify
add/del_route_ipv6 and make it use "interface=" (without breaking
"vpn server special route" case).


For consistency, I think we should do that.  What I'd avoid is to do
the adapter_index lookup for every single route - which would imply adding
the index to struct tuntap_options or something like that which is
available in add/del_route[_ipv6]() already.  But this is less elegant
than I hoped for, and might have to look different for 2.3 and master.


Adding the index - you probably meant to "struct tuntap"?

Master has member "adapter_index", which seems to be used for "special 
case", release/2.3 does not have that.


Do you think it is safe to store adapter index there (would have to add 
it to 2.3) without breaking "special case"? I personally not so sure, 
since that "adapter_index" is obtained via "adapter_index_of_ip" in 
tun.c, which looks quite different from "get_adapter_index_flexible".


We might have a semantic problem here.

-Lev

Re: [Openvpn-devel] [PATCH v2] Use adapter index instead of name

[Gert Doering]

Hi,

On Wed, Nov 11, 2015 at 10:05:11AM +0200, Lev Stipakov wrote:
> >
> > It should actually be not very hard - we should be able to set "tt->actual"
> > to read "interface=nnn", and then it should work automagically without even
> > touching route.c at all
> 
> Setting "interface=" to "tt->actual_name" will affect all code 
> branches which use that value, for example "netsh_enable_dhcp". In this 
> particular case we cannot use index and must use interface name, 
> according to MS 
> (https://technet.microsoft.com/en-us/library/cc731521(v=ws.10).aspx#BKMK_setaddress)

Uh.  Thanks for investigating.  Why, oh why... it was naive of me to expect
consistency here, I should have known better :-(

[..]
> So, if we want to use index also for "add/del route", I'd gently modify 
> add/del_route_ipv6 and make it use "interface=" (without breaking 
> "vpn server special route" case).

For consistency, I think we should do that.  What I'd avoid is to do
the adapter_index lookup for every single route - which would imply adding
the index to struct tuntap_options or something like that which is 
available in add/del_route[_ipv6]() already.  But this is less elegant
than I hoped for, and might have to look different for 2.3 and master.

So maybe make this a separate patch?

> > What does surprise me, though, is that it works for you with just specifying
> > the interface index, without "IF" or "interface=" before it.
> >
> 
> MS says "[[ interface=] String] Specifies an interface name or index" 
> for "set address" and "add route". If I read it right, "interface" 
> prefix can be omitted. But let's use it for consistency with existing code.

... and make this ("interface=") a v3 patch, only for "set address"
for now, to move forward.

thanks,

gert

-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

Re: [Openvpn-devel] [PATCH v2] Use adapter index instead of name

[Lev Stipakov]

Hi,



It should actually be not very hard - we should be able to set "tt->actual"
to read "interface=nnn", and then it should work automagically without even
touching route.c at all


Setting "interface=" to "tt->actual_name" will affect all code 
branches which use that value, for example "netsh_enable_dhcp". In this 
particular case we cannot use index and must use interface name, 
according to MS 
(https://technet.microsoft.com/en-us/library/cc731521(v=ws.10).aspx#BKMK_setaddress)


Same documentation claims that "set/delete address" accepts interface 
index only for ipv6. That said, "add/delete route" accepts index for 
both ipv4/ipv6. Besides, ipv4 syntax for "set address" is "[ name =] 
InterfaceName" and ipv6 is "[[ interface=] String]".


So, if we want to use index also for "add/del route", I'd gently modify 
add/del_route_ipv6 and make it use "interface=" (without breaking 
"vpn server special route" case).



What does surprise me, though, is that it works for you with just specifying
the interface index, without "IF" or "interface=" before it.



MS says "[[ interface=] String] Specifies an interface name or index" 
for "set address" and "add route". If I read it right, "interface" 
prefix can be omitted. But let's use it for consistency with existing code.


-Lev

Re: [Openvpn-devel] [PATCH v2] Use adapter index instead of name

[Gert Doering]

Hi,

On Mon, Nov 09, 2015 at 04:35:09PM +0200, Lev Stipakov wrote:
> v2:
>  * Remove netsh call which uses adapter name. After thoughtful testing
> turns out that "adapter name" code branch is never used.

Not there yet :-) - but thanks for picking this up.

As I said for v1, if we change this for the "set address" part of the
code, because "it will sometimes not work otherwise", we should also
change it for the "set route" code - consistent usage of netsh, easier
to understand when reading the log, etc.

It should actually be not very hard - we should be able to set "tt->actual"
to read "interface=nnn", and then it should work automagically without even 
touching route.c at all

> - /* example: netsh interface ipv6 set address MyTap 2001:608:8003::d 
> store=active */
> + /* example: netsh interface ipv6 set address 42 2001:608:8003::d 
> store=active */

What does surprise me, though, is that it works for you with just specifying
the interface index, without "IF" or "interface=" before it.

When setting IPv6 routes for non-tun/tap gateways (route.c, line 1770, 
in git master) I use "interface=" and that is what the online
help told me to use.

  struct buffer out = alloc_buf_gc (64, );
  buf_printf (, "interface=%d", r6->adapter_index );
  device = buf_bptr();
...
  argv_printf (, "%s%sc interface ipv6 add route %s/%d %s",
   get_win_sys_path(),
   NETSH_PATH_SUFFIX,
   network,
   r6->netbits,
   device);

is the syntax different for "set address"?

(IPv4 code uses "IF "... hrmph)

gert


-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

[Openvpn-devel] [PATCH v2] Use adapter index instead of name

[Lev Stipakov]

v2:
 * Remove netsh call which uses adapter name. After thoughtful testing
turns out that "adapter name" code branch is never used.

Some windows machines get weird issues with netsh when using
adapter name on "netsh.exe interface ipv6 set address" command.

Changed logic to get adapter index and use it instead of adapter
name for netsh set address command. if unable to get adapter index,
try with adapter name.

Signed-off-by: Olli Mannisto 
Signed-off-by: Lev Stipakov 
---
 src/openvpn/tun.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 24a61ec..0347a52 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -1301,18 +1301,18 @@ do_ifconfig (struct tuntap *tt,
 if ( do_ipv6 )
   {
char * saved_actual;
+   const DWORD idx = get_adapter_index_flexible(actual);

if (!strcmp (actual, "NULL"))
  msg (M_FATAL, "Error: When using --tun-ipv6, if you have more than 
one TAP-Windows adapter, you must also specify --dev-node");

-   /* example: netsh interface ipv6 set address MyTap 2001:608:8003::d 
store=active */
+   /* example: netsh interface ipv6 set address 42 2001:608:8003::d 
store=active */
argv_printf (,
-   "%s%sc interface ipv6 set address %s %s store=active",
+"%s%sc interface ipv6 set address %u %s store=active",
 get_win_sys_path(),
 NETSH_PATH_SUFFIX,
-actual,
-ifconfig_ipv6_local );
-
+idx,
+ifconfig_ipv6_local);
netsh_command (, 4);

/* explicit route needed */
-- 
1.9.1