diff mbox series

[Openvpn-devel] Allow DNS autoconf by passing hostname by IV variables when using push-peer-info

Message ID CAJXQW9Vzf_ypQGSez8X96imiy2Hat0buoY2fp3gmsYs1EXdcpg@mail.gmail.com
State Changes Requested
Headers
Series [Openvpn-devel] Allow DNS autoconf by passing hostname by IV variables when using push-peer-info |

Commit Message

Ricardo Manriquez Sept. 17, 2022, 3:23 p.m. UTC 
  Author: Ricardo Manríquez <ricardo.manriquez@gmail.com>

To enable the possibility of DNS autoconfiguration the IP address and
hostname of the client are needed to register at the DNS level, this patch
adds this information when using push-peer-info.

The motivation is that the domain name is as intrusive as the MAC address
and DNS autoconfiguration is helpful to be able to communicate back to the
clients, this generates a problem when the client connects to the network
directly and then uses the VPN connection, now the DNS records do not match
and when using remote assistance or remote management tools the benefits of
DNS are negated.

Signed-off-by: Ricardo Manríquez <ricardo.manriquez@gmail.com>

---
 src/openvpn/ssl.c | 5 +++++
 1 file changed, 5 insertions(+)

 #if defined(_WIN32)
             buf_printf(&out, "IV_PLAT_VER=%s\n",
win32_version_string(&gc, false));

Comments

Gert Doering Sept. 18, 2022, 12:46 a.m. UTC | #1 
Hi,

On Sun, Sep 18, 2022 at 10:23:14AM +0900, Ricardo Manriquez wrote:
> +
> +            char hostname[64];
> +            gethostname(hostname, 63);
> +            buf_printf(&out, "IV_HOSTNAME=%s\n", hostname );

Without entering the discussion if this is a useful addition, the
implementation definitely lacks error handling - gethostname() can
fail, and then we have an unintialized buffer passed to buf_printf().

gert
Arne Schwabe Sept. 18, 2022, 12:51 a.m. UTC | #2 
Am 18.09.2022 um 03:23 schrieb Ricardo Manriquez:
> Author: Ricardo Manríquez <ricardo.manriquez@gmail.com>
>
> To enable the possibility of DNS autoconfiguration the IP address and 
> hostname of the client are needed to register at the DNS level, this 
> patch adds this information when using push-peer-info.
>
> The motivation is that the domain name is as intrusive as the MAC 
> address and DNS autoconfiguration is helpful to be able to communicate 
> back to the clients, this generates a problem when the client connects 
> to the network directly and then uses the VPN connection, now the DNS 
> records do not match and when using remote assistance or remote 
> management tools the benefits of DNS are negated.

Could you expain why this needs to be in OpenVPN itself and cannot be 
done with something like starting openvpn with an additional parameter 
like --setenv UV_HOSTNAME "$(hostname)" or derived from another 
parameter/variable from the client like CN, username etc? Space in the 
packet carrying IV_/UV_ variables is already limited and I am not sure 
if spending another 64 for the hostname is a good thing.


> ---
>   src/openvpn/ssl.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 80e0d5acb4..3031566585 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2321,6 +2321,11 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
>               {
>                   buf_printf(&out, "IV_HWADDR=%s\n", format_hex_ex(rgi.hwaddr, 6, 0, 1, ":", &gc));
>               }
> +
> +            char hostname[64];
> +            gethostname(hostname, 63);
> +            buf_printf(&out, "IV_HOSTNAME=%s\n", hostname );
Isn't there a MAX_HOSTNAME define or similar instead of hardcoding 64 here?

The handling of the string of hostname is not very well here. The man 
page of the function (gethostname(2) - Linux manual page (man7.org) 
<https://man7.org/linux/man-pages/man2/sethostname.2.html>) says null 
termination is not guaranteed for long hostnames.


> +
>               buf_printf(&out, "IV_SSL=%s\n", get_ssl_library_version() );
>   #if defined(_WIN32)
>               buf_printf(&out, "IV_PLAT_VER=%s\n", win32_version_string(&gc, false));
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
diff mbox series

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 80e0d5acb4..3031566585 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2321,6 +2321,11 @@  push_peer_info(struct buffer *buf, struct
tls_session *session)
             {
                 buf_printf(&out, "IV_HWADDR=%s\n",
format_hex_ex(rgi.hwaddr, 6, 0, 1, ":", &gc));
             }
+
+            char hostname[64];
+            gethostname(hostname, 63);
+            buf_printf(&out, "IV_HOSTNAME=%s\n", hostname );
+
             buf_printf(&out, "IV_SSL=%s\n", get_ssl_library_version() );