Re: [Openvpn-devel] [PATCH] add support for --dns option
Hi Gert
On Donnerstag, 17. März 2022 11:41:22 CET Gert Doering wrote:
> I wonder why we bother to actually *do* this? As in "we already know
> that this can never overflow here" (because all strings involved are
> known, and the max width of %d is known, too), but *if* it ever did,
> calling the subsequent setenv_str() wouldn't be a good idea to do - no?
It's in the spirit of defensive programming. It cannot overflow now, but will
likely overflow some time. However I thought that getting the value into env
might still be worthwhile in some scenarios, even if the name is not complete,
e.g. if the env is logged somewhere. The error message in the openvpn log, will
lead to cause then.
> Can we make transform these into a miniwrapper that does
> like
>
>for (i = 1, d = o->search_domains; d != NULL; i++, d = d->next)
>{
> _do_dns_env_thing( "dns_search_domain_%d", i, d->name );
>}
I actually had such a wrapper in place, but dumped it because I didn't like the
fact that the value would have to come before the name in the argument list,
because of the var-args. Since we have a format string with two %d (and there
might be more / other formatting specifiers in the future), the wrapper would
have to deal with ..., or we'd end up with two wrappers (or more if this is
extended in the future). No matter what:
>const char* format = s->domain_type == DNS_RESOLVE_DOMAINS ?
>"dns_server_%d_resolve_domain_%d" :
> "dns_server_%d_exclude_domain_%d";
requires it to be:
_do_dns_env_thing(d->name, "dns_server_%d_resolve__domain_%d", i, j);
right now already. And that lead me to not making things wrapped.
However I could be talked into removing the checks for openvpn_snprintf()'s
return value if there's a consensus, as it wouldn't introduce an actual issue.
Just maybe a little harder to debug issues in future code possibly. Also not
setting the env if sprintf fails sounds good, would do this rather:
if (openvpn_snprinf())
setenv_str();
else
name_ok = false;
Iff the value is really not useful in the env, when the name is incomplete.
Bonus: no &=.
Heiko
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] add support for --dns option
Hi,
(this is about v2)
On Sat, Mar 12, 2022 at 02:58:10PM +0100, Heiko Hund wrote:
> 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.
[..]
> +for (i = 1, d = o->search_domains; d != NULL; i++, d = d->next)
> +{
> +name_ok = openvpn_snprintf(env_name, sizeof(env_name),
> "dns_search_domain_%d", i) && name_ok;
> +setenv_str(es, env_name, d->name);
> +}
With the "&& name_ok" this code does not really improve, even if it
was requested in the v1 review...
I wonder why we bother to actually *do* this? As in "we already know
that this can never overflow here" (because all strings involved are
known, and the max width of %d is known, too), but *if* it ever did,
calling the subsequent setenv_str() wouldn't be a good idea to do - no?
Can we make transform these into a miniwrapper that does
- openvpn_snprintf()
- check result, warn if overflow
- call setenv_str() otherwise
like
for (i = 1, d = o->search_domains; d != NULL; i++, d = d->next)
{
_do_dns_env_thing( "dns_search_domain_%d", i, d->name );
}
so, shorter lines, clearer code, actual error checking, and no && ugliness :-)
(As far as I could see, it's all "setenv_str()" anyway)
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] add support for --dns option
On Samstag, 12. März 2022 14:58:10 CET Heiko Hund wrote: > +name_ok = openvpn_snprintf(env_name, sizeof(env_name), > "dns_search_domain_%d", i) && name_ok; With some distance, I still like the &= version better from a readability standpoint. Even though it's a bit unclean. Heiko ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
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.
Re: [Openvpn-devel] [PATCH] add support for --dns option
Am 09.03.22 um 00:06 schrieb Heiko Hund: + +bool dns_server_priority_parse(long *priority, const char *str, bool pulled); +struct dns_server* dns_server_get(struct dns_server **entry, long priority, struct gc_arena *gc); +void dns_domain_list_append(struct dns_domain **entry, char **domains, struct gc_arena *gc); +bool dns_server_addr_parse(struct dns_server *server, const char *addr); +bool dns_options_verify(int msglevel, const struct dns_options *o); +struct dns_options clone_dns_options(const struct dns_options o, struct gc_arena *gc); +void dns_options_preprocess_pull(struct dns_options *o); +void dns_options_postprocess_pull(struct dns_options *o); +void setenv_dns_options(const struct dns_options *o, struct env_set *es); +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. +--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. 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. + 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. +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. + enum dns_security dnssec; + enum dns_server_transport transport; + char *sni; +}; That should be probably be a const char* instead of a modifable char* 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. +struct gc_arena dns_gc = gc_new();; extra ; +/* 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. 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. Arne ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] add support for --dns option
Acked-By: Frank Lichtenheld I have reviewed this on Github already and had no further issues. > Heiko Hund hat am 09.03.2022 00:06 geschrieben: > > > 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 Grüße, -- Frank Lichtenheld ___ 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 | 55 +++ doc/man-sections/script-options.rst | 19 + doc/man-sections/server-options.rst | 2 +- src/openvpn/Makefile.am | 1 + src/openvpn/dns.c | 561 src/openvpn/dns.h | 89 + 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, 981 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..bdfe6aac 100644 --- a/doc/man-sections/client-options.rst +++ b/doc/man-sections/client-options.rst @@ -154,6 +154,61 @@ 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. + + 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. So, ``--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 @@ instances. netsh.exe calls which sometimes just do not work right with interface names). Set prior to ``--up`` or ``--down`` script execution. +:code:`dns_*` +The ``--dns`` configuration options will
