Re: [Openvpn-devel] [PATCH] add support for --dns option
Sorry, this is [PATCH v2]. Somehow I managed for send-email to override my --subject. ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] add support for --dns option
As a first step towards DNS configuration in openvpn and a unified way to push DNS related settings to clients in v2 and v3, this commit adds support for parsing the new --dns option. Later commits will add support for setting up DNS on different platforms. For now, --dns and DNS related --dhcp-option can be used together for smoother transition. Settings from --dns will override ones --dhcp-option where applicable. For detailed information about the option consult the documentation in this commit. Signed-off-by: Heiko Hund --- doc/man-sections/client-options.rst | 59 doc/man-sections/script-options.rst | 19 ++ doc/man-sections/server-options.rst | 2 +- src/openvpn/Makefile.am | 1 + src/openvpn/dns.c | 495 src/openvpn/dns.h | 164 + src/openvpn/openvpn.vcxproj | 4 +- src/openvpn/openvpn.vcxproj.filters | 8 +- src/openvpn/options.c | 221 + src/openvpn/options.h | 7 + src/openvpn/push.c | 4 + src/openvpn/socket.c| 11 + src/openvpn/socket.h| 2 + 13 files changed, 994 insertions(+), 3 deletions(-) create mode 100644 src/openvpn/dns.c create mode 100644 src/openvpn/dns.h diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst index 92a02e28..2d7293c6 100644 --- a/doc/man-sections/client-options.rst +++ b/doc/man-sections/client-options.rst @@ -154,6 +154,65 @@ configuration. --connect-timeout n See ``--server-poll-timeout``. +--dns args + Client DNS configuration to be used with the connection. + + Valid syntaxes: + :: + + dns search-domains domain [domain ...] + dns server n address addr[:port] [addr[:port]] + dns server n resolve-domains|exclude-domains domain [domain ...] + dns server n dnssec yes|optional|no + dns server n transport DoH|DoT|plain + dns server n sni server-name + + The ``--dns search-domains`` directive takes one or more domain names + to be added as DNS domain suffixes. If it is repeated multiple times within + a configuration the domains are appended, thus e.g. domain names pushed by + a server will amend locally defined ones. + + The ``--dns server`` directive is used to configure DNS server ``n``. + The server id ``n`` must be a value between -128 and 127. For pushed + DNS server options it must be between 0 and 127. The server id is used + to group options and also for ordering the list of configured DNS servers; + lower numbers come first. DNS servers being pushed to a client replace + already configured DNS servers with the same server id. + + The ``address`` option configures the IPv4 and / or IPv6 address of + the DNS server. Optionally a port can be appended after a colon. IPv6 + addresses need to be enclosed in brackets if a port is appended. + + The ``resolve-domains`` and ``exclude-domains`` options take one or + more DNS domains which are explicitly resolved or explicitly not resolved + by a server. Only one of the options can be configured for a server. + ``resolve-domains`` is used to define a split-dns setup, where only + given domains are resolved by a server. ``exclude-domains`` is used to + define domains which will never be resolved by a server (e.g. domains + which can only be resolved locally). Systems which do not support fine + grained DNS domain configuration, will ignore these settings. + + The ``dnssec`` option is used to configure validation of DNSSEC records. + While the exact semantics may differ for resolvers on different systems, + ``yes`` likely makes validation mandatory, ``no`` disables it, and ``optional`` + uses it opportunistically. + + The ``transport`` option enables DNS-over-HTTPS (``DoH``) or DNS-over-TLS (``DoT``) + for a DNS server. The ``sni`` option can be used with them to specify the + ``server-name`` for TLS server name indication. + + Each server has to have at least one address configured for a configuration + to be valid. All the other options can be omitted. + + Note that not all options may be supported on all platforms. As soon support + for different systems is implemented, information will be added here how + unsupported options are treated. + + The ``--dns`` option will eventually obsolete the ``--dhcp-option`` directive. + Until then it will replace configuration at the places ``--dhcp-option`` puts it, + so that ``--dns`` overrides ``--dhcp-option``. Thus, ``--dns`` can be used today + to migrate from ``--dhcp-option``. + --explicit-exit-notify n In UDP client mode or point-to-point mode, send server/peer an exit notification if tunnel is restarted or OpenVPN process is exited. In diff --git a/doc/man-sections/script-options.rst b/doc/man-sections/script-options.rst index 22990f4f..c81efe9e 100644 --- a/doc/man-sections/script-options.rst +++ b/doc/man-sections/script-options.rst @@ -586,6 +586,25
Re: [Openvpn-devel] [PATCH] add support for --dns option
On Mittwoch, 9. März 2022 13:40:32 CET Arne Schwabe wrote: > Am 09.03.22 um 00:06 schrieb Heiko Hund: > > +bool dns_server_priority_parse(long *priority, const char *str, bool > > +[...] > > +void show_dns_options(const struct dns_options *o); > > These new functions are missing doxygen comments. We generally have the > doxygen comments in the header files rather than in the c files. Okay, I'll move the comments into the header file. > > +--dns args > > + Client DNS configuration to be used with the connection. > > This man page section sounds like all options are supported on all > platforms while currently that is not the case. An explicit warning etc > that says that some options like port, dnssec, doh etc are not supported > on certain platforms should be included and also what happens if they > are not. Noted. What happens will likely be determined when we have the implementation(s) that actually take this option and try to apply it. Think it is too early to add this info to the manual, as it may change. Some ideas exist, but they have to prove themselves and survive public discussion. So far I have the idea that options will be ignored if they are not supported, and split DNS is turned into all DNS redirected to the pushed server(s) if they are unsupported. Again, this is very theoretical at this point and may change once facing reality. > > The ``--dns`` option will eventually obsolete the ``--dhcp-option`` > > directive.> > > + Until then it will add configuration at the places ``--dhcp-option`` > > puts it + and will override it if both are given. > > Is dhcp-option ignored when dns is present by newer client or are they > mixed? This is a bit vague. Will clarify. > > + long priority; > > why a type that is 32 bit on some platforms and 64 bit on others? Either > go with just int if 32 bit is enough or use long long or int64_t if we > need 64 but long is a weird type. int8_t would be enough, just need an extra few bits for the conversation from text. It is a long because strtol(3) returns a long, and I went with that. Not sure if casting after checks will gain much. If we decide to change this I'd rather use int8_t however, or a future somebody will ponder why the extra bits there, when the priority is really a 8 bit value. IMHO it is not worth the extra code, that's why I went with long. Compilers might align an 8 bit value anyways. > > +name_ok &= openvpn_snprintf(env_name, sizeof(env_name), > > "dns_server_%d_address4", i); > bitwise and with a bool feels wrong. We should use && instead of & for > boolean logic. Since there is no &&= operator, I thought it's more readable with &=. The result is the same because of the way bool is defined. I'll convert it to logical operators for the v2 and then we'll see if it looks and feels better. > > + enum dns_security dnssec; > > + enum dns_server_transport transport; > > + char *sni; > > +}; > > That should be probably be a const char* instead of a modifable char* Right. > > gc_free(>gc); > > > > +gc_free(>dns_options.gc); > > having an extra gc in this way feels a bit strange. Since this is the > only time it is initialised you can just as well just use o->gc instead. Actually no. The --dns option is a bit special as it can partially replace local setting with pushed ones (i.e. override single dns servers and others not). The contained gc is used to allocate (and free) pulled options. If we'd use o->gc as well, the pulled options wouldn't be freed for a potentially long time, increasing the memory footprint over time. > > +struct gc_arena dns_gc = gc_new();; > > extra ; Eagle eyes! =) > > +/* Free DNS options and reset them to pre-pull state */ > > +gc_free(>dns_options.gc); > > There is an additional free for the gc here that has no matching > gc_init. If your goal is to have a gc for the life time of prepull etc, > then it is better to put it there as that is easier to see its lifetime > than to have it only for DNS where the lifetime/ownership is not so clear. The accompanying gc_init() is with the one for o->gc. Don't see how moving the gc into struct options_pre_connect would change anything substantially. The gc_init would still be done in init_options() then. > I see also that this code makes no attempt to remove/refactor the old > DHCP code. The only thing I see is the tuntap_options_copy_dns code. > What the supposed way forward here? That also raises the question about > platform specific code. Does that code now need two implementations? One > for installing options from the dhcp structs and one from the DNS struct? > > Some idea how this is going to go forward would be good. There are several ideas. Instead of having a #ifdef hell, like for other platform specific code, I would prefer some sort of "dns plugin / script" way, where platform specific code could reside in individual units and linked / called on a on-demand basis.
