| Message ID | 1573234655-905-1-git-send-email-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v3] wintun: add --windows-driver config option | expand |
Hi, On 08-11-2019 18:37, 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> > --- > > v3: > - added man page > - removed unneccesary assignment > - make method static > - removed unneeded _WIN32 define > > v2: > - rebased on top of latest master > > doc/openvpn.8 | 8 ++++++++ > src/openvpn/init.c | 4 ++++ > src/openvpn/options.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- > src/openvpn/options.h | 1 + > src/openvpn/tun.h | 1 + > 5 files changed, 58 insertions(+), 2 deletions(-) > > diff --git a/doc/openvpn.8 b/doc/openvpn.8 > index 11daa92..8a7e70f 100644 > --- a/doc/openvpn.8 > +++ b/doc/openvpn.8 > @@ -6147,6 +6147,14 @@ Note that pushing unknown options from server does not trigger > fatal errors. > .\"********************************************************* > .TP > +.B \-\-windows\-driver > +Specifies which tun driver to use. Values are > +.B tap-windows6 > +(default) and > +.B wintun. > +This is Windows-only option. > +.\"********************************************************* > +.TP > .B \-\-dhcp\-renew > Ask Windows to renew the TAP adapter lease on startup. > This option is normally unnecessary, as Windows automatically > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index ae7bd63..0bdb0a9 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); > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index 1838a69..a70a822 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -747,9 +747,10 @@ 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" > -#ifdef _WIN32 > + "--windows-driver : Which tun driver to use?\n" > + " tap-windows6 (default)\n" > + " wintun\n" > "--block-outside-dns : Block DNS on other network adapters to prevent DNS leaks\n" > -#endif > "Windows Standalone Options:\n" > "\n" > "--show-adapters : Show all TAP-Windows adapters.\n" > @@ -851,6 +852,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 +2996,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 +4047,33 @@ foreign_option(struct options *o, char *argv[], int len, struct env_set *es) > } > } > > +#ifdef _WIN32 > +/** > + * Parses --windows-driver config option > + * > + * @param str value of --windows-driver option > + * @param msglevel msglevel to report parsing error > + * @return bool true if --windows-driver is wintun, false otherwise > + */ > +static 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 +5316,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 */ > Thanks, this looks good to me now. I don't have a working Windows test setup available, so only did stare-at-code and verified that building with mingw still works for me. So, as far as this goes without real testing: Acked-by: Steffan Karger <steffan@karger.me> -Steffan
Your patch has been applied to the master branch.
(It needed a bit of manual massaging as the context in options.c:851
changed due to the merge of the VLAN patches)
I'm not totally happy with introducing a new function (parse_windows_driver())
to effectively map two constant strings to a boolan variable... but since
this got ACKed already, in it goes...
commit c0658271be5b45a201eb6aba1e4bdc5a729285be
Author: Lev Stipakov
Date: Fri Nov 8 19:37:35 2019 +0200
wintun: add --windows-driver config option
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Simon Rozman <simon@rozman.si>
Message-Id: <1573234655-905-1-git-send-email-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19046.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Hi, On Sat, Nov 09, 2019 at 03:49:51PM +0100, Gert Doering wrote: > wintun: add --windows-driver config option > > Signed-off-by: Lev Stipakov <lev@openvpn.net> > Acked-by: Simon Rozman <simon@rozman.si> Ooops. Misattributed the ACK, so this was really Steffan's ACK. I am not amending & re-pusing the commit. This will call more confusion. Sorry. gert
On 09/11/2019 16:00, Gert Doering wrote: > Hi, > > On Sat, Nov 09, 2019 at 03:49:51PM +0100, Gert Doering wrote: >> wintun: add --windows-driver config option >> >> Signed-off-by: Lev Stipakov <lev@openvpn.net> >> Acked-by: Simon Rozman <simon@rozman.si> > > Ooops. Misattributed the ACK, so this was really Steffan's ACK. > > I am not amending & re-pusing the commit. This will call more confusion. > > Sorry. Wise choice. However you can use another little git feature ... git notes <https://web.archive.org/web/20180121193320/http://git-scm.com/blog/2010/08/25/notes.html> <https://git-scm.com/docs/git-notes> Git notes does not change the commitish, but are more weakly tied to commits and does not have the same "change traceability" as commits does. So I recommend to PGP sign notes (which might need to be handled outside git) if it important to ensure the note is unchanged. But a note can be AFAIK, it doesn't push or fetch notes by default, so that might need to be handled separately, using: $ git push <remote> refs/notes/* To fetch remote notes: $ git fetch <remote> refs/notes/*:refs/notes/* We already have a few notes in our repo for similar situations or clarifications.
On 12/11/2019 19:39, David Sommerseth wrote: > On 09/11/2019 16:00, Gert Doering wrote: >> Hi, >> >> On Sat, Nov 09, 2019 at 03:49:51PM +0100, Gert Doering wrote: >>> wintun: add --windows-driver config option >>> >>> Signed-off-by: Lev Stipakov <lev@openvpn.net> >>> Acked-by: Simon Rozman <simon@rozman.si> >> >> Ooops. Misattributed the ACK, so this was really Steffan's ACK. >> >> I am not amending & re-pusing the commit. This will call more confusion. >> >> Sorry. > > Wise choice. However you can use another little git feature ... git notes > <https://web.archive.org/web/20180121193320/http://git-scm.com/blog/2010/08/25/notes.html> > <https://git-scm.com/docs/git-notes> > > Git notes does not change the commitish, but are more weakly tied to commits > and does not have the same "change traceability" as commits does. So I > recommend to PGP sign notes (which might need to be handled outside git) if it > important to ensure the note is unchanged. But a note can be Okay, lets complete that sentence ... But a note can be removed without making it too obvious, so it's not a totally secure extension to commit messages. But it helps clarify things, especially if the integrity of the note isn't critical.
Hi, I have been late to this wintun party (no time for anything fun, these days) and this has already been committed, it seems. But some concerns below.. > + /* 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; > + } > Looks like this will most likely break any dhcp-related options in the client config.. Say "dhcp-option DNS xxx". We currently require ip_win32_type to be adaptive or dynamic when such options are specified. I think we should set this to adaptive here, and then handle it in tun.c Although there is no dhcp when wintun is used, we'll still support dhcp-options such as DNS etc using netsh or service, right? Selva <div dir="ltr"><div>Hi,</div><div><br></div><div>I have been late to this wintun party (no time for anything fun, these days)</div><div>and this has already been committed, it seems. But some concerns below..</div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br> + /* for wintun kernel doesn't send DHCP requests, so use ipapi to set IP address and netmask */<br> + if (options->wintun)<br> + {<br> + options->tuntap_options.ip_win32_type = IPW32_SET_IPAPI;<br> + }<br></blockquote><div><br></div><div>Looks like this will most likely break any dhcp-related options</div><div>in the client config.. Say "dhcp-option DNS xxx". We currently require </div><div>ip_win32_type to be adaptive or dynamic when such options are specified.</div><div>I think we should set this to adaptive here, and then handle it in tun.c</div><div><br></div><div>Although there is no dhcp when wintun is used, we'll still support</div><div>dhcp-options such as DNS etc using netsh or service, right?</div><div><br></div><div>Selva</div></div></div>
Hi, On Mon, Nov 18, 2019 at 11:17:33PM -0500, Selva Nair wrote: > Looks like this will most likely break any dhcp-related options > in the client config.. Say "dhcp-option DNS xxx". We currently require > ip_win32_type to be adaptive or dynamic when such options are specified. > I think we should set this to adaptive here, and then handle it in tun.c > > Although there is no dhcp when wintun is used, we'll still support > dhcp-options such as DNS etc using netsh or service, right? I had the same concerns, and if I remember right (I haven't looked just now) one of the later patches adds the missing bits to the iservice. So we'd accept the DHCP options, pass them on, and iservice would IPAPI them in. I'll have another look... gert
Hello, ti 19. marrask. 2019 klo 9.37 Gert Doering (gert@greenie.muc.de) kirjoitti: > Looks like this will most likely break any dhcp-related options > > in the client config.. Say "dhcp-option DNS xxx". Oops, indeed. When I add those to client config, I got: > Options error: --dhcp-options requires --ip-win32 dynamic or adaptive > We currently require ip_win32_type to be adaptive or dynamic when such > options are specified. > That limitation has been presented since the very first commit in github: if (options->tuntap_options.dhcp_options && options->tuntap_options.ip_win32_type != IPW32_SET_DHCP_MASQ && options->tuntap_options.ip_win32_type != IPW32_SET_ADAPTIVE) { msg(M_USAGE, "--dhcp-options requires --ip-win32 dynamic or adaptive"); } I commented it out and client-side --dhcp-options worked for me. Any ideas why we have this code? > > I think we should set this to adaptive here, and then handle it in tun.c > Actually, in follow-up commit I have changed IPAPI to NETSH - if I remember correctly I could not set up routing using former, although it works when done by iserv. If we don't know why we have that check and we could apply dhcp options from client config without it, shouldn't we just get rid of it? > > > > Although there is no dhcp when wintun is used, we'll still support > > dhcp-options such as DNS etc using netsh or service, right? > That's correct. At the moment we use: - netsh when running openvpn without interactive service - iserv, which itself uses both IPAPI (for setting up IP address and routing) and netsh (for setting up DNS) I had the same concerns, and if I remember right (I haven't looked just now) > one of the later patches adds the missing bits to the iservice. > Yep, patch 6/7 https://patchwork.openvpn.net/patch/883/
Hi, On Tue, Nov 19, 2019 at 3:29 AM Lev Stipakov <lstipakov@gmail.com> wrote: > Hello, > > ti 19. marrask. 2019 klo 9.37 Gert Doering (gert@greenie.muc.de) > kirjoitti: > > > Looks like this will most likely break any dhcp-related options >> > in the client config.. Say "dhcp-option DNS xxx". > > > Oops, indeed. When I add those to client config, I got: > > > Options error: --dhcp-options requires --ip-win32 dynamic or adaptive > > >> We currently require ip_win32_type to be adaptive or dynamic when such >> options are specified. >> > > That limitation has been presented since the very first commit in github: > > if (options->tuntap_options.dhcp_options > && options->tuntap_options.ip_win32_type != IPW32_SET_DHCP_MASQ > && options->tuntap_options.ip_win32_type != IPW32_SET_ADAPTIVE) > { > msg(M_USAGE, "--dhcp-options requires --ip-win32 dynamic or > adaptive"); > } > > I commented it out and client-side --dhcp-options worked for me. Any ideas > why we have this code? > > >> > I think we should set this to adaptive here, and then handle it in tun.c >> > > Actually, in follow-up commit I have changed IPAPI to NETSH - if I > remember correctly > I could not set up routing using former, although it works when done by > iserv. > > If we don't know why we have that check and we could apply dhcp options > from client config > without it, shouldn't we just get rid of it? > > >> > >> > Although there is no dhcp when wintun is used, we'll still support >> > dhcp-options such as DNS etc using netsh or service, right? >> > > That's correct. At the moment we use: > > - netsh when running openvpn without interactive service > - iserv, which itself uses both IPAPI (for setting up IP address and > routing) and netsh (for setting up DNS) > The ip-win32 mess does need clenup but at least it would be nice to use the same API/method in iservice and the core. Can we figure out why IP helper API is not working for setting IP? Unfortunately there is no API for setting DNS (not that I know of) so there the use of netsh is unavoidable. Anyway, wintun + dhcp-option in client config is broken right now. Looks like it may work when dhcp-option is pushed (not tested) but there are cases where we do use it directly in the config. Selva <div dir="ltr"><div>Hi,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 19, 2019 at 3:29 AM Lev Stipakov <<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</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"><div dir="ltr"><div dir="ltr">Hello,<div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">ti 19. marrask. 2019 klo 9.37 Gert Doering (<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>) kirjoitti:</div><div dir="ltr" class="gmail_attr"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > Looks like this will most likely break any dhcp-related options<br> > in the client config.. Say "dhcp-option DNS xxx".</blockquote><div><br></div><div>Oops, indeed. When I add those to client config, I got:</div><div><br></div><div>> Options error: --dhcp-options requires --ip-win32 dynamic or adaptive</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">We currently require ip_win32_type to be adaptive or dynamic when such options are specified.<br></blockquote><div><br></div><div>That limitation has been presented since the very first commit in github:</div><div><br></div><div> if (options->tuntap_options.dhcp_options<br> && options->tuntap_options.ip_win32_type != IPW32_SET_DHCP_MASQ<br> && options->tuntap_options.ip_win32_type != IPW32_SET_ADAPTIVE)<br> {<br> msg(M_USAGE, "--dhcp-options requires --ip-win32 dynamic or adaptive");<br> }<br></div><div><br></div><div>I commented it out and client-side --dhcp-options worked for me. Any ideas why we have this code?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > I think we should set this to adaptive here, and then handle it in tun.c<br></blockquote><div><br></div><div>Actually, in follow-up commit I have changed IPAPI to NETSH - if I remember correctly</div><div>I could not set up routing using former, although it works when done by iserv.</div><div><br></div><div>If we don't know why we have that check and we could apply dhcp options from client config</div><div>without it, shouldn't we just get rid of it?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > <br> > Although there is no dhcp when wintun is used, we'll still support<br> > dhcp-options such as DNS etc using netsh or service, right?<br></blockquote><div><br></div><div>That's correct. At the moment we use:</div><div><br></div><div> - netsh when running openvpn without interactive service</div><div> - iserv, which itself uses both IPAPI (for setting up IP address and routing) and netsh (for setting up DNS)</div></div></div></blockquote><div><br></div><div>The ip-win32 mess does need clenup but at least it would be nice to use the same API/method in iservice and the core. Can we figure out why IP helper API is not working for setting IP? Unfortunately there is no API for setting DNS (not that I know of) so there the use of netsh is unavoidable.<br></div></div><div class="gmail_quote"><br></div><div class="gmail_quote">Anyway, wintun + dhcp-option in client config is broken right now. Looks like it may work when dhcp-option is pushed (not tested) but there are cases where we do use it directly in the config.<br></div><div><br></div><div>Selva<br></div></div>
diff --git a/doc/openvpn.8 b/doc/openvpn.8 index 11daa92..8a7e70f 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -6147,6 +6147,14 @@ Note that pushing unknown options from server does not trigger fatal errors. .\"********************************************************* .TP +.B \-\-windows\-driver +Specifies which tun driver to use. Values are +.B tap-windows6 +(default) and +.B wintun. +This is Windows-only option. +.\"********************************************************* +.TP .B \-\-dhcp\-renew Ask Windows to renew the TAP adapter lease on startup. This option is normally unnecessary, as Windows automatically diff --git a/src/openvpn/init.c b/src/openvpn/init.c index ae7bd63..0bdb0a9 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); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 1838a69..a70a822 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -747,9 +747,10 @@ 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" -#ifdef _WIN32 + "--windows-driver : Which tun driver to use?\n" + " tap-windows6 (default)\n" + " wintun\n" "--block-outside-dns : Block DNS on other network adapters to prevent DNS leaks\n" -#endif "Windows Standalone Options:\n" "\n" "--show-adapters : Show all TAP-Windows adapters.\n" @@ -851,6 +852,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 +2996,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 +4047,33 @@ foreign_option(struct options *o, char *argv[], int len, struct env_set *es) } } +#ifdef _WIN32 +/** + * Parses --windows-driver config option + * + * @param str value of --windows-driver option + * @param msglevel msglevel to report parsing error + * @return bool true if --windows-driver is wintun, false otherwise + */ +static 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 +5316,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 */