| Message ID | 20200924064452.1001-2-simon@rozman.si |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,1/3] netsh: Specify interfaces by index rather than name | expand |
Hi, > When there are no IPv6 DNS published, the adapter state is not > sanitized and might contain IPv6 DNS server from a previous session. In this case, shouldn't the "set dns" call below overwrite the previous value? > netsh_ifconfig_options() clears DNS servers for IPv4 already. Agreed, let's do it for consistency. > * The list of dns servers currently set on the interface > * are cleared first. Either existing comment lied or "implicit clear" was assumed because of the "set dns" call. But now it is fine, since we have real clearing. Stared and the code, built and tested on MSVC/Win10. Acked-by: Lev Stipakov <lstipakov@gmail.com>
Your patch has been applied to the master and release/2.5 branch.
In my win7 tests this does not seem to be *needed* - I ran a test
with an IPv6 DNS server, and then the same test without, and the
"ipconfig /all" output showed "no more IPv6 DNS server" (before
applying this patch) - but this might have an effect if no IPv4
servers are there.
I was a bit worried that this might break in the "ip-win32 netsh" case,
so I tested that one as well. It seems to do the right thing, though,
decoupling IPv4 DNS servers and IPv6 DNS servers (and not deleting
"just all" with the delete command).
Incidentially I tested "netsh interface ip set address 17 dhcp" here
as well (works).
commit dd754221024cf60226ebaa679ec65ccc23f4e402 (master)
commit 77c62003c263304f8b411d664cf56179f8d4df08 (release/2.5)
Author: Simon Rozman via Openvpn-devel
Date: Thu Sep 24 08:44:51 2020 +0200
netsh: Clear existing IPv6 DNS servers before configuring new ones
Signed-off-by: Simon Rozman <simon@rozman.si>
Acked-by: Lev Stipakov <lstipakov@gmail.com>
Message-Id: <20200924064452.1001-2-simon@rozman.si>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21078.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Hi, On Thu, Sep 24, 2020 at 4:57 AM Lev Stipakov <lstipakov@gmail.com> wrote: > Hi, > > > When there are no IPv6 DNS published, the adapter state is not > > sanitized and might contain IPv6 DNS server from a previous session. > > In this case, shouldn't the "set dns" call below overwrite the previous > value? > > netsh_ifconfig_options() clears DNS servers for IPv4 already. > > Agreed, let's do it for consistency. > > > * The list of dns servers currently set on the interface > > * are cleared first. > > Either existing comment lied or "implicit clear" was assumed because > of the "set dns" call. > The clearing happened only if at least one DNS address was specified using --dhcp-option DNS. > But now it is fine, since we have real clearing. > Now that this has been merged, should we make DNS settings using iservice consistent with this? Currently, when using the service, we do not touch any existing dns servers if none are specified using --dhcp-option. Same approach was taken in my patch for setting the domain suffix. The original logic was that any statically set values should not be overwritten unless explicitly asked for. At the same time, we do delete all addresses for v4 (not v6) while closing tun. But, if we want to ensure a clean state for the adapter, as argued here, we should be clearing current values regardless of whether new one's are being set or not. Selva > Stared and the code, built and tested on MSVC/Win10. > > Acked-by: Lev Stipakov <lstipakov@gmail.com> > > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel > <div dir="ltr"><div>Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Sep 24, 2020 at 4:57 AM Lev Stipakov <<a href="mailto:lstipakov@gmail.com" target="_blank">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">Hi,<br> <br> > When there are no IPv6 DNS published, the adapter state is not<br> > sanitized and might contain IPv6 DNS server from a previous session.<br> <br> In this case, shouldn't the "set dns" call below overwrite the previous value? </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > netsh_ifconfig_options() clears DNS servers for IPv4 already.<br> <br> Agreed, let's do it for consistency.<br> <br> > * The list of dns servers currently set on the interface<br> > * are cleared first.<br> <br> Either existing comment lied or "implicit clear" was assumed because<br> of the "set dns" call.<br></blockquote><div><br></div><div>The clearing happened only if at least one DNS address was specified using --dhcp-option DNS.</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"> But now it is fine, since we have real clearing.<br></blockquote><div><br></div><div>Now that this has been merged, should we make DNS settings using iservice consistent with this? Currently, when using the service, we do not touch any existing dns servers if none are specified using --dhcp-option. Same approach was taken in my patch for setting the domain suffix. The original logic was that any statically set values should not be overwritten unless explicitly asked for. At the same time, we do delete all addresses for v4 (not v6) while closing tun.</div><div><br></div><div>But, if we want to ensure a clean state for the adapter, as argued here, we should be clearing current values regardless of whether new one's are being set or not. </div><div><br></div><div>Selva</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"> <br> Stared and the code, built and tested on MSVC/Win10.<br> <br> Acked-by: Lev Stipakov <<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>><br> <br> <br> _______________________________________________<br> Openvpn-devel mailing list<br> <a href="mailto:Openvpn-devel@lists.sourceforge.net" target="_blank">Openvpn-devel@lists.sourceforge.net</a><br> <a href="https://lists.sourceforge.net/lists/listinfo/openvpn-devel" rel="noreferrer" target="_blank">https://lists.sourceforge.net/lists/listinfo/openvpn-devel</a><br> </blockquote></div></div>
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 8fd3229f..b1cd7a1b 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -5281,7 +5281,6 @@ ip_addr_member_of(const in_addr_t addr, const IP_ADDR_STRING *ias) * Set the ipv6 dns servers on the specified interface. * The list of dns servers currently set on the interface * are cleared first. - * No action is taken if number of addresses (addr_len) < 1. */ static void netsh_set_dns6_servers(const struct in6_addr *addr_list, @@ -5291,6 +5290,13 @@ netsh_set_dns6_servers(const struct in6_addr *addr_list, struct gc_arena gc = gc_new(); struct argv argv = argv_new(); + /* delete existing DNS settings from TAP interface */ + argv_printf(&argv, "%s%s interface ipv6 delete dns %lu all", + get_win_sys_path(), + NETSH_PATH_SUFFIX, + adapter_index); + netsh_command(&argv, 2, M_FATAL); + for (int i = 0; i < addr_len; ++i) { const char *fmt = (i == 0) ?
When there are no IPv6 DNS published, the adapter state is not sanitized and might contain IPv6 DNS server from a previous session. netsh_ifconfig_options() clears DNS servers for IPv4 already. Signed-off-by: Simon Rozman <simon@rozman.si> --- src/openvpn/tun.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)