| Message ID | 1573148729-27339-3-git-send-email-lstipakov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Wintun support | expand |
Hi, Thanks for looking into wintun support. Definitely feature-ack. On 07-11-2019 18:45, Lev Stipakov wrote: > From: Lev Stipakov <lev@openvpn.net> > > This allows to specify which tun driver openvpn should use, > tap-windows6 (default) or wintun. > > Note than wintun support will be added in follow-up patches. > > Signed-off-by: Lev Stipakov <lev@openvpn.net> > --- > src/openvpn/init.c | 7 +++++++ > src/openvpn/options.c | 37 +++++++++++++++++++++++++++++++++++++ > src/openvpn/options.h | 1 + > src/openvpn/tun.h | 1 + > 4 files changed, 46 insertions(+) Should there not be a manpage entry in this commit too? > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index ae7bd63..c6d4953 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -1733,6 +1733,10 @@ do_init_tun(struct context *c) > c->c2.es, > &c->net_ctx); > > +#ifdef _WIN32 > + c->c1.tuntap->wintun = c->options.wintun; > +#endif > + > init_tun_post(c->c1.tuntap, > &c->c2.frame, > &c->options.tuntap_options); > @@ -1775,6 +1779,9 @@ do_open_tun(struct context *c) > /* store (hide) interactive service handle in tuntap_options */ > c->c1.tuntap->options.msg_channel = c->options.msg_channel; > msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned int) c->options.msg_channel); > + > + c->c1.tuntap->wintun = c->options.wintun; > + Why do we have to set c->c2.tuntap->wintun in both do_init_tun and do_open_tun? Should one of them not suffice? > #endif > > /* allocate route list structure */ > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index 1838a69..5c5033e 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -747,6 +747,9 @@ static const char usage_message[] = > " optional parameter controls the initial state of ex.\n" > "--show-net-up : Show " PACKAGE_NAME "'s view of routing table and net adapter list\n" > " after TAP adapter is up and routes have been added.\n" > + "--windows-driver : Which tun driver to use?\n" > + " tap-windows6 (default)\n" > + " wintun\n" > #ifdef _WIN32 Should this --windows-driver option not be inside the #ifdef _WIN32 ? > "--block-outside-dns : Block DNS on other network adapters to prevent DNS leaks\n" > #endif > @@ -851,6 +854,7 @@ init_options(struct options *o, const bool init_gc) > o->tuntap_options.dhcp_masq_offset = 0; /* use network address as internal DHCP server address */ > o->route_method = ROUTE_METHOD_ADAPTIVE; > o->block_outside_dns = false; > + o->wintun = false; > #endif > o->vlan_accept = VLAN_ONLY_UNTAGGED_OR_PRIORITY; > o->vlan_pvid = 1; > @@ -2994,6 +2998,12 @@ options_postprocess_mutate_invariant(struct options *options) > options->ifconfig_noexec = false; > } > > + /* for wintun kernel doesn't send DHCP requests, so use ipapi to set IP address and netmask */ > + if (options->wintun) > + { > + options->tuntap_options.ip_win32_type = IPW32_SET_IPAPI; > + } > + > remap_redirect_gateway_flags(options); > #endif > > @@ -4039,6 +4049,26 @@ foreign_option(struct options *o, char *argv[], int len, struct env_set *es) > } > } > > +#ifdef _WIN32 > +bool > +parse_windows_driver(const char *str, const int msglevel) I think this should be a static function. Also a short (doxygen) comment explaining what the return value means would be nice. > +{ > + if (streq(str, "tap-windows6")) > + { > + return false; > + } > + else if (streq(str, "wintun")) > + { > + return true; > + } > + else > + { > + msg(msglevel, "--windows-driver must be tap-windows6 or wintun"); > + return false; > + } > +} > +#endif > + > /* > * parse/print topology coding > */ > @@ -5281,6 +5311,13 @@ add_option(struct options *options, > VERIFY_PERMISSION(OPT_P_GENERAL); > options->dev_type = p[1]; > } > +#ifdef _WIN32 > + else if (streq(p[0], "windows-driver") && p[1] && !p[2]) > + { > + VERIFY_PERMISSION(OPT_P_GENERAL); > + options->wintun = parse_windows_driver(p[1], M_FATAL); > + } > +#endif > else if (streq(p[0], "dev-node") && p[1] && !p[2]) > { > VERIFY_PERMISSION(OPT_P_GENERAL); > diff --git a/src/openvpn/options.h b/src/openvpn/options.h > index ff7a5bb..0a24e5e 100644 > --- a/src/openvpn/options.h > +++ b/src/openvpn/options.h > @@ -632,6 +632,7 @@ struct options > bool show_net_up; > int route_method; > bool block_outside_dns; > + bool wintun; Did you consider using an enum instead? I think it would make the code easier to read. E.g. with typedef enum { WINDRV_TAP6, WINDRV_WINTUN } windrv_t; in tun.h or so, we'd get options->windrv = WINDRV_TAP6 instead of options->wintun = false. > #endif > > bool use_peer_id; > diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h > index 5a0a933..df935f6 100644 > --- a/src/openvpn/tun.h > +++ b/src/openvpn/tun.h > @@ -175,6 +175,7 @@ struct tuntap > * ~0 if undefined */ > DWORD adapter_index; > > + bool wintun; /* true if wintun is used instead of tap-windows6 */ > int standby_iter; > #else /* ifdef _WIN32 */ > int fd; /* file descriptor for TUN/TAP dev */ > Thanks, -Steffan
Hi, Should there not be a manpage entry in this commit too? > Indeed. Added to manpage. Why do we have to set c->c2.tuntap->wintun in both do_init_tun and > do_open_tun? Should one of them not suffice? > Just to be sure that it is assigned :) Removed unneeded second assignment. Should this --windows-driver option not be inside the #ifdef _WIN32 ? > In fact it is already in _WIN32, but the next option has #ifdef _WIN32 again, so I removed it. > > +#ifdef _WIN32 > > +bool > > +parse_windows_driver(const char *str, const int msglevel) > > I think this should be a static function. Also a short (doxygen) comment > explaining what the return value means would be nice. > Done. > > + bool wintun; > > Did you consider using an enum instead? I think it would make the code > easier to read. > Honestly I do not see to much value in converting bool to enum, it is unlikely that we'll have more tun drivers in the near future. Also changing it here would break follow-up patches. As verbally agreed, I'll look into it afterwards. v3 is on its way.
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index ae7bd63..c6d4953 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1733,6 +1733,10 @@ do_init_tun(struct context *c) c->c2.es, &c->net_ctx); +#ifdef _WIN32 + c->c1.tuntap->wintun = c->options.wintun; +#endif + init_tun_post(c->c1.tuntap, &c->c2.frame, &c->options.tuntap_options); @@ -1775,6 +1779,9 @@ do_open_tun(struct context *c) /* store (hide) interactive service handle in tuntap_options */ c->c1.tuntap->options.msg_channel = c->options.msg_channel; msg(D_ROUTE, "interactive service msg_channel=%u", (unsigned int) c->options.msg_channel); + + c->c1.tuntap->wintun = c->options.wintun; + #endif /* allocate route list structure */ diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 1838a69..5c5033e 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -747,6 +747,9 @@ static const char usage_message[] = " optional parameter controls the initial state of ex.\n" "--show-net-up : Show " PACKAGE_NAME "'s view of routing table and net adapter list\n" " after TAP adapter is up and routes have been added.\n" + "--windows-driver : Which tun driver to use?\n" + " tap-windows6 (default)\n" + " wintun\n" #ifdef _WIN32 "--block-outside-dns : Block DNS on other network adapters to prevent DNS leaks\n" #endif @@ -851,6 +854,7 @@ init_options(struct options *o, const bool init_gc) o->tuntap_options.dhcp_masq_offset = 0; /* use network address as internal DHCP server address */ o->route_method = ROUTE_METHOD_ADAPTIVE; o->block_outside_dns = false; + o->wintun = false; #endif o->vlan_accept = VLAN_ONLY_UNTAGGED_OR_PRIORITY; o->vlan_pvid = 1; @@ -2994,6 +2998,12 @@ options_postprocess_mutate_invariant(struct options *options) options->ifconfig_noexec = false; } + /* for wintun kernel doesn't send DHCP requests, so use ipapi to set IP address and netmask */ + if (options->wintun) + { + options->tuntap_options.ip_win32_type = IPW32_SET_IPAPI; + } + remap_redirect_gateway_flags(options); #endif @@ -4039,6 +4049,26 @@ foreign_option(struct options *o, char *argv[], int len, struct env_set *es) } } +#ifdef _WIN32 +bool +parse_windows_driver(const char *str, const int msglevel) +{ + if (streq(str, "tap-windows6")) + { + return false; + } + else if (streq(str, "wintun")) + { + return true; + } + else + { + msg(msglevel, "--windows-driver must be tap-windows6 or wintun"); + return false; + } +} +#endif + /* * parse/print topology coding */ @@ -5281,6 +5311,13 @@ add_option(struct options *options, VERIFY_PERMISSION(OPT_P_GENERAL); options->dev_type = p[1]; } +#ifdef _WIN32 + else if (streq(p[0], "windows-driver") && p[1] && !p[2]) + { + VERIFY_PERMISSION(OPT_P_GENERAL); + options->wintun = parse_windows_driver(p[1], M_FATAL); + } +#endif else if (streq(p[0], "dev-node") && p[1] && !p[2]) { VERIFY_PERMISSION(OPT_P_GENERAL); diff --git a/src/openvpn/options.h b/src/openvpn/options.h index ff7a5bb..0a24e5e 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -632,6 +632,7 @@ struct options bool show_net_up; int route_method; bool block_outside_dns; + bool wintun; #endif bool use_peer_id; diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h index 5a0a933..df935f6 100644 --- a/src/openvpn/tun.h +++ b/src/openvpn/tun.h @@ -175,6 +175,7 @@ struct tuntap * ~0 if undefined */ DWORD adapter_index; + bool wintun; /* true if wintun is used instead of tap-windows6 */ int standby_iter; #else /* ifdef _WIN32 */ int fd; /* file descriptor for TUN/TAP dev */