[Openvpn-devel,v2] signal --dns support in peer info

  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(+)
  

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