| Message ID | 20220513093740.1091639-1-heiko@ist.eigentlich.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] signal --dns support in peer info | expand |
On 13/05/2022 11:37, Heiko Hund wrote: > Have clients set a bit in IV_PROTO, so that servers can make an informed > decision on whether to push --dns to the client. While unknown options > are ignored by clients when pushed, they generate a warning in the log. > That can be circumvented by server backends by checking if bit 7 is set. > > Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net> > --- > src/openvpn/ssl.c | 3 +++ > src/openvpn/ssl.h | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index 61dea996..24d7f3f4 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -1940,6 +1940,9 @@ push_peer_info(struct buffer *buf, struct tls_session *session) > /* support for P_DATA_V2 */ > int iv_proto = IV_PROTO_DATA_V2; > > + /* support for the --dns option */ > + iv_proto |= IV_PROTO_DNS_OPTION; > + > /* support for receiving push_reply before sending > * push request, also signal that the client wants > * to get push-reply messages without without requiring a round > diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h > index 0ba86d3e..c8802707 100644 > --- a/src/openvpn/ssl.h > +++ b/src/openvpn/ssl.h > @@ -93,6 +93,9 @@ > * result. */ > #define IV_PROTO_NCP_P2P (1<<5) > > +/** Supports the --dns option introduced in version 2.6 */ > +#define IV_PROTO_DNS_OPTION (1<<6) > + > /* Default field in X509 to be username */ > #define X509_USERNAME_FIELD_DEFAULT "CN" > Only glared on the code and compile tested. LGTM. Acked-By: David Sommerseth <davids@openvpn.net>
Am 13.05.22 um 13:22 schrieb David Sommerseth: > On 13/05/2022 11:37, Heiko Hund wrote: >> Have clients set a bit in IV_PROTO, so that servers can make an informed >> decision on whether to push --dns to the client. While unknown options >> are ignored by clients when pushed, they generate a warning in the log. >> That can be circumvented by server backends by checking if bit 7 is set. >> >> Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net> >> --- >> src/openvpn/ssl.c | 3 +++ >> src/openvpn/ssl.h | 3 +++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c >> index 61dea996..24d7f3f4 100644 >> --- a/src/openvpn/ssl.c >> +++ b/src/openvpn/ssl.c >> @@ -1940,6 +1940,9 @@ push_peer_info(struct buffer *buf, struct >> tls_session *session) >> /* support for P_DATA_V2 */ >> int iv_proto = IV_PROTO_DATA_V2; >> + /* support for the --dns option */ >> + iv_proto |= IV_PROTO_DNS_OPTION; >> + >> /* support for receiving push_reply before sending >> * push request, also signal that the client wants >> * to get push-reply messages without without requiring a round >> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h >> index 0ba86d3e..c8802707 100644 >> --- a/src/openvpn/ssl.h >> +++ b/src/openvpn/ssl.h >> @@ -93,6 +93,9 @@ >> * result. */ >> #define IV_PROTO_NCP_P2P (1<<5) >> +/** Supports the --dns option introduced in version 2.6 */ >> +#define IV_PROTO_DNS_OPTION (1<<6) >> + >> /* Default field in X509 to be username */ >> #define X509_USERNAME_FIELD_DEFAULT "CN" > > Only glared on the code and compile tested. LGTM. > > Acked-By: David Sommerseth <davids@openvpn.net> To be honest, I requested this flag but I don't think this is really what I want/need any more. I wanted to have a flag that tells me as a server that I can push --dns options instead of --dhcp-options and accept the client to evaluate them. But after some digging, I found that on platforms where dhcp-option is NOT parsed by openvpn itself (so anything but Android and Windows) and scripts are used to set DNS, these scripts will always use dhcp-options as they rely on foreign_option support. So they end up with no DNS configuration if only --dns is pushed and using --dhcp-option options if both are pushed unless the script is updated. I think having clear preference and not knowing will make debugging logs from 3rd parties that have both --dns and --dhcp-option in them quite tedious. So this flag doesn't really do what I expected it to promose (This client will accept --dns and use them) Arne
Hi, On Fri, May 13, 2022 at 01:40:07PM +0200, Arne Schwabe wrote: > So this flag doesn't really do what I expected it to promose (This > client will accept --dns and use them) So you want something that goes hand in hand with code to actually "do something with it", like +#if defined(TARGET_WINDOWS)||defined(TARGET_ANDROID) + /* we have full backend support for --dns options */ + iv_proto |= IV_PROTO_DNS_OPTION; +#endif and whenever a platform gets a "real" implementation, be it "openvpn provided --up script parsing the correct variables" or "NM plugin" or "directly implemented by magic", we add it to that list? gert
On 13/05/2022 13:40, Arne Schwabe wrote: > Am 13.05.22 um 13:22 schrieb David Sommerseth: >> On 13/05/2022 11:37, Heiko Hund wrote: >>> Have clients set a bit in IV_PROTO, so that servers can make an informed >>> decision on whether to push --dns to the client. While unknown options >>> are ignored by clients when pushed, they generate a warning in the log. >>> That can be circumvented by server backends by checking if bit 7 is set. >>> >>> Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net> [...snip...] >>> + /* support for the --dns option */ >>> + iv_proto |= IV_PROTO_DNS_OPTION; >>> + [...snip...] >>> +#define IV_PROTO_DNS_OPTION (1<<6) >>> + [...snip...] > To be honest, I requested this flag but I don't think this is really > what I want/need any more. I wanted to have a flag that tells me as a > server that I can push --dns options instead of --dhcp-options and > accept the client to evaluate them. > > But after some digging, I found that on platforms where dhcp-option is > NOT parsed by openvpn itself (so anything but Android and Windows) and > scripts are used to set DNS, these scripts will always use dhcp-options > as they rely on foreign_option support. So they end up with no DNS > configuration if only --dns is pushed and using --dhcp-option options if > both are pushed unless the script is updated. As I remember it, this wasn't initially about Android and Windows clients. It was about the server pushing both --dhcp-option DNS and --dns at the same time, and that OpenVPN 2.5 and older clients would complain about --dns option warnings. By signalling to a server who wants to avoid this (like the Access Server), the server can chose if it wants to send --dns or --dhcp-options, based on this flag. Right? That the client side isn't parsing --dns options properly is to my understanding a different issue, which also needs a solution. And if my memory isn't completely corrupted, we had some brief talks about v2.6 clients adding some kind of "conversion" from --dns to --dhcp-options *data* for scripts/plug-ins - but only in a simplistic best-effort approach. If the strict --dhcp-option behavior is needed, that can be used with v2.6 clients for the time being if --dns causes troubles. But if both --dns and --dhcp-options are used, and the --dhcp-option provides a setting --dns support, --dns should take precedence. This should also work fine, as the --dns option is far more flexible in what it supports, while --dhcp-options are just simpler by design - and designed around a lesser dynamic Internet world. -- kind regards, David Sommerseth OpenVPN Inc
We had the ACK for this since a long time, but then had discussions on
"is this what we really want", leading to the "DNS foreign option" patch
committed just now - so after checking with Arne, this is now making sense
for the server side intent ("if IV_PROTO contains the DNS bit, the server
can stop pushing dhcp-options, because --dns will do all the job, and
more, even for old-style platforms").
The patch itself is trivially correct "the bit is unique, and it is added" :-)
Your patch has been applied to the master branch.
commit 1f7f7d2a8943b5e33f86f208cce8f5d10c91a8f4
Author: Heiko Hund
Date: Fri May 13 11:37:40 2022 +0200
signal --dns support in peer info
Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>
Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <20220513093740.1091639-1-heiko@ist.eigentlich.net>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24350.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 61dea996..24d7f3f4 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1940,6 +1940,9 @@ push_peer_info(struct buffer *buf, struct tls_session *session) /* support for P_DATA_V2 */ int iv_proto = IV_PROTO_DATA_V2; + /* support for the --dns option */ + iv_proto |= IV_PROTO_DNS_OPTION; + /* support for receiving push_reply before sending * push request, also signal that the client wants * to get push-reply messages without without requiring a round diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index 0ba86d3e..c8802707 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -93,6 +93,9 @@ * result. */ #define IV_PROTO_NCP_P2P (1<<5) +/** Supports the --dns option introduced in version 2.6 */ +#define IV_PROTO_DNS_OPTION (1<<6) + /* Default field in X509 to be username */ #define X509_USERNAME_FIELD_DEFAULT "CN"
Have clients set a bit in IV_PROTO, so that servers can make an informed decision on whether to push --dns to the client. While unknown options are ignored by clients when pushed, they generate a warning in the log. That can be circumvented by server backends by checking if bit 7 is set. Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net> --- src/openvpn/ssl.c | 3 +++ src/openvpn/ssl.h | 3 +++ 2 files changed, 6 insertions(+)