openvpn-devel — OpenVPN developers list

[Openvpn-devel] [XS] Change in openvpn[master]: packet_id: Fix build with --disable-debug

[flichtenheld (C. Review) <ge...@op...>]

Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1071?usp=email

to review the following change.


Change subject: packet_id: Fix build with --disable-debug
......................................................................

packet_id: Fix build with --disable-debug

Broken since commit
bc62a9a02cb7365a678bcd3f2faf537a420cc5a0
"Add methods to read/write packet ids for epoch data"

Change-Id: I3bed9c7aafee8e62ddae14c0d3e21cf4c146a37c
Signed-off-by: Frank Lichtenheld <fr...@li...>
---
M src/openvpn/packet_id.c
1 file changed, 2 insertions(+), 3 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/71/1071/1

diff --git a/src/openvpn/packet_id.c b/src/openvpn/packet_id.c
index c8dae32..76a81c6 100644
--- a/src/openvpn/packet_id.c
+++ b/src/openvpn/packet_id.c
@@ -673,6 +673,8 @@
     gc_free(&gc);
 }
 
+#endif /* ifdef ENABLE_DEBUG */
+
 uint16_t
 packet_id_read_epoch(struct packet_id_net *pin, struct buffer *buf)
 {
@@ -711,6 +713,3 @@
 
     return buf_write(buf, &net_id, sizeof(net_id));
 }
-
-
-#endif /* ifdef ENABLE_DEBUG */

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1071?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I3bed9c7aafee8e62ddae14c0d3e21cf4c146a37c
Gerrit-Change-Number: 1071
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-MessageType: newchange

[Openvpn-devel] [XS] Change in openvpn[master]: dns: create NRPT registry key if it doesn't exist

[cron2 (C. Review) <ge...@op...>]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email )

Change subject: dns: create NRPT registry key if it doesn't exist
......................................................................

dns: create NRPT registry key if it doesn't exist

Windows 2019 Server by default does not have the key where local system
NRPT rules are stored. Tests have determined that NRPT is actually
working when rules are created under the key. So, instead of failing if
the key doesn't exist, we create it, and things will start working.

Github: OpenVPN/openvpn#768
Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Signed-off-by: Heiko Hund <he...@is...>
Acked-by: Lev Stipakov <lst...@gm...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32001.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpnserv/interactive.c
1 file changed, 1 insertion(+), 1 deletion(-)




diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 3bd2722..628a96b 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -2662,7 +2662,7 @@
     if (err == ERROR_FILE_NOT_FOUND)
     {
         *gpol = FALSE;
-        err = RegOpenKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, KEY_ALL_ACCESS, &nrpt);
+        err = RegCreateKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, NULL, 0, KEY_ALL_ACCESS, NULL, &nrpt, NULL);
         if (err)
         {
             nrpt = INVALID_HANDLE_VALUE;

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Gerrit-Change-Number: 1069
Gerrit-PatchSet: 4
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: stipa <lst...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: merged

[Openvpn-devel] [XS] Change in openvpn[master]: dns: create NRPT registry key if it doesn't exist

[cron2 (C. Review) <ge...@op...>]

cron2 has uploaded a new patch set (#4) to the change originally created by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email )

The following approvals got outdated and were removed:
Code-Review+2 by stipa


Change subject: dns: create NRPT registry key if it doesn't exist
......................................................................

dns: create NRPT registry key if it doesn't exist

Windows 2019 Server by default does not have the key where local system
NRPT rules are stored. Tests have determined that NRPT is actually
working when rules are created under the key. So, instead of failing if
the key doesn't exist, we create it, and things will start working.

Github: OpenVPN/openvpn#768
Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Signed-off-by: Heiko Hund <he...@is...>
Acked-by: Lev Stipakov <lst...@gm...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg32001.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpnserv/interactive.c
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/69/1069/4

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 3bd2722..628a96b 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -2662,7 +2662,7 @@
     if (err == ERROR_FILE_NOT_FOUND)
     {
         *gpol = FALSE;
-        err = RegOpenKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, KEY_ALL_ACCESS, &nrpt);
+        err = RegCreateKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, NULL, 0, KEY_ALL_ACCESS, NULL, &nrpt, NULL);
         if (err)
         {
             nrpt = INVALID_HANDLE_VALUE;

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Gerrit-Change-Number: 1069
Gerrit-PatchSet: 4
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: stipa <lst...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: newpatchset

[Openvpn-devel] [PATCH applied] Re: dns: create NRPT registry key if it doesn't exist

[Gert D. <ge...@gr...>]

I have not tested this beyond "does it compile on a MinGW buildhost" (it
does), and "what does this function do?" (open a registry key if it exists,
and create a new key if it doesn't exist yet) - so this all seems to make
sense, and Lev confirms that it does what it wants to achieve ;-)

Your patch has been applied to the master branch.

commit df4863aa0e43544ea82ab9d98966a03a95c62334
Author: Heiko Hund
Date:   Fri Jun 27 10:24:53 2025 +0200

     dns: create NRPT registry key if it doesn't exist

     Signed-off-by: Heiko Hund <he...@is...>
     Acked-by: Lev Stipakov <lst...@gm...>
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg32001.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [M] Change in openvpn[master]: run forced --dns-updown without --script-security

[cron2 (C. Review) <ge...@op...>]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1065?usp=email )

Change subject: run forced --dns-updown without --script-security
......................................................................

run forced --dns-updown without --script-security

Due to a shortcut in the `--dns-updown force' implementation, running the
default dns-updown script required `--script-security 2'. This makes the
forced default script run without --script-security set.

Change-Id: I55940b78e35f0e3d74aa6cba14378afed97a444e
Signed-off-by: Heiko Hund <he...@is...>
Acked-by: Frank Lichtenheld <fr...@li...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg31994.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/dns.c
M src/openvpn/dns.h
M src/openvpn/options.c
3 files changed, 39 insertions(+), 12 deletions(-)




diff --git a/src/openvpn/dns.c b/src/openvpn/dns.c
index 939ae09..ea3d91b 100644
--- a/src/openvpn/dns.c
+++ b/src/openvpn/dns.c
@@ -264,7 +264,7 @@
     clone.servers = clone_dns_servers(o->servers, gc);
     clone.servers_prepull = clone_dns_servers(o->servers_prepull, gc);
     clone.updown = o->updown;
-    clone.user_set_updown = o->user_set_updown;
+    clone.updown_flags = o->updown_flags;
 
     return clone;
 }
@@ -580,7 +580,7 @@
     argv_printf(&argv, "%s", o->updown);
     argv_msg(M_INFO, &argv);
     int res;
-    if (o->user_set_updown)
+    if (dns_updown_user_set(o))
     {
         res = openvpn_run_script(&argv, es, S_EXITCODE, "dns updown");
     }
@@ -692,7 +692,7 @@
 run_up_down_command(bool up, struct options *o, const struct tuntap *tt, struct dns_updown_runner_info *updown_runner)
 {
     struct dns_options *dns = &o->dns_options;
-    if (!dns->updown || (o->up_script && !dns->user_set_updown))
+    if (!dns->updown || (o->up_script && !dns_updown_user_set(dns) && !dns_updown_forced(dns)))
     {
         return;
     }
diff --git a/src/openvpn/dns.h b/src/openvpn/dns.h
index 688daa7..d33f64e 100644
--- a/src/openvpn/dns.h
+++ b/src/openvpn/dns.h
@@ -42,13 +42,18 @@
     DNS_TRANSPORT_TLS
 };
 
+enum dns_updown_flags {
+    DNS_UPDOWN_NO_FLAGS,
+    DNS_UPDOWN_USER_SET,
+    DNS_UPDOWN_FORCED
+};
+
 struct dns_domain {
     struct dns_domain *next;
     const char *name;
 };
 
-struct dns_server_addr
-{
+struct dns_server_addr {
     union {
         struct in_addr a4;
         struct in6_addr a6;
@@ -103,7 +108,7 @@
     struct dns_server *servers;
     struct gc_arena gc;
     const char *updown;
-    bool user_set_updown;
+    enum dns_updown_flags updown_flags;
 };
 
 /**
@@ -195,4 +200,26 @@
  */
 void show_dns_options(const struct dns_options *o);
 
+/**
+ * Returns whether dns-updown is user defined
+ *
+ * @param   o           Pointer to the DNS options struct
+ */
+static inline bool
+dns_updown_user_set(const struct dns_options *o)
+{
+    return o->updown_flags == DNS_UPDOWN_USER_SET;
+}
+
+/**
+ * Returns whether dns-updown is forced to run
+ *
+ * @param   o           Pointer to the DNS options struct
+ */
+static inline bool
+dns_updown_forced(const struct dns_options *o)
+{
+    return o->updown_flags == DNS_UPDOWN_FORCED;
+}
+
 #endif /* ifndef DNS_H */
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7e26069..af097f8 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3593,7 +3593,7 @@
     struct gc_arena gc = gc_new();
     struct dns_options *dns = &o->dns_options;
 
-    if (dns->servers || dns->user_set_updown)
+    if (dns->servers || dns_updown_user_set(dns) || dns_updown_forced(dns))
     {
         /* Clean up env from --dhcp-option DNS config */
         struct buffer name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
@@ -3667,7 +3667,7 @@
             }
         }
     }
-    else if (o->up_script && !dns->user_set_updown)
+    else if (o->up_script && !dns_updown_user_set(dns) && !dns_updown_forced(dns))
     {
         /* Set foreign option env vars from --dns config */
         const char *p[] = { "dhcp-option", NULL, NULL };
@@ -8182,15 +8182,15 @@
         if (streq(p[1], "disable"))
         {
             dns->updown = NULL;
-            dns->user_set_updown = false;
+            dns->updown_flags = DNS_UPDOWN_NO_FLAGS;
         }
         else if (streq(p[1], "force"))
         {
             /* force dns-updown run, even if a --up script is defined */
-            if (dns->user_set_updown == false)
+            if (!dns_updown_user_set(dns))
             {
                 dns->updown = DEFAULT_DNS_UPDOWN;
-                dns->user_set_updown = true;
+                dns->updown_flags = DNS_UPDOWN_FORCED;
             }
         }
         else
@@ -8201,7 +8201,7 @@
                 dns->updown = NULL;
             }
             set_user_script(options, &dns->updown, p[1], p[0], false);
-            dns->user_set_updown = true;
+            dns->updown_flags = DNS_UPDOWN_USER_SET;
         }
     }
     else if (streq(p[0], "dns") && p[1])

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1065?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I55940b78e35f0e3d74aa6cba14378afed97a444e
Gerrit-Change-Number: 1065
Gerrit-PatchSet: 4
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: merged

[Openvpn-devel] [M] Change in openvpn[master]: run forced --dns-updown without --script-security

[cron2 (C. Review) <ge...@op...>]

cron2 has uploaded a new patch set (#4) to the change originally created by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1065?usp=email )

The following approvals got outdated and were removed:
Code-Review+2 by flichtenheld


Change subject: run forced --dns-updown without --script-security
......................................................................

run forced --dns-updown without --script-security

Due to a shortcut in the `--dns-updown force' implementation, running the
default dns-updown script required `--script-security 2'. This makes the
forced default script run without --script-security set.

Change-Id: I55940b78e35f0e3d74aa6cba14378afed97a444e
Signed-off-by: Heiko Hund <he...@is...>
Acked-by: Frank Lichtenheld <fr...@li...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg31994.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M src/openvpn/dns.c
M src/openvpn/dns.h
M src/openvpn/options.c
3 files changed, 39 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/65/1065/4

diff --git a/src/openvpn/dns.c b/src/openvpn/dns.c
index 939ae09..ea3d91b 100644
--- a/src/openvpn/dns.c
+++ b/src/openvpn/dns.c
@@ -264,7 +264,7 @@
     clone.servers = clone_dns_servers(o->servers, gc);
     clone.servers_prepull = clone_dns_servers(o->servers_prepull, gc);
     clone.updown = o->updown;
-    clone.user_set_updown = o->user_set_updown;
+    clone.updown_flags = o->updown_flags;
 
     return clone;
 }
@@ -580,7 +580,7 @@
     argv_printf(&argv, "%s", o->updown);
     argv_msg(M_INFO, &argv);
     int res;
-    if (o->user_set_updown)
+    if (dns_updown_user_set(o))
     {
         res = openvpn_run_script(&argv, es, S_EXITCODE, "dns updown");
     }
@@ -692,7 +692,7 @@
 run_up_down_command(bool up, struct options *o, const struct tuntap *tt, struct dns_updown_runner_info *updown_runner)
 {
     struct dns_options *dns = &o->dns_options;
-    if (!dns->updown || (o->up_script && !dns->user_set_updown))
+    if (!dns->updown || (o->up_script && !dns_updown_user_set(dns) && !dns_updown_forced(dns)))
     {
         return;
     }
diff --git a/src/openvpn/dns.h b/src/openvpn/dns.h
index 688daa7..d33f64e 100644
--- a/src/openvpn/dns.h
+++ b/src/openvpn/dns.h
@@ -42,13 +42,18 @@
     DNS_TRANSPORT_TLS
 };
 
+enum dns_updown_flags {
+    DNS_UPDOWN_NO_FLAGS,
+    DNS_UPDOWN_USER_SET,
+    DNS_UPDOWN_FORCED
+};
+
 struct dns_domain {
     struct dns_domain *next;
     const char *name;
 };
 
-struct dns_server_addr
-{
+struct dns_server_addr {
     union {
         struct in_addr a4;
         struct in6_addr a6;
@@ -103,7 +108,7 @@
     struct dns_server *servers;
     struct gc_arena gc;
     const char *updown;
-    bool user_set_updown;
+    enum dns_updown_flags updown_flags;
 };
 
 /**
@@ -195,4 +200,26 @@
  */
 void show_dns_options(const struct dns_options *o);
 
+/**
+ * Returns whether dns-updown is user defined
+ *
+ * @param   o           Pointer to the DNS options struct
+ */
+static inline bool
+dns_updown_user_set(const struct dns_options *o)
+{
+    return o->updown_flags == DNS_UPDOWN_USER_SET;
+}
+
+/**
+ * Returns whether dns-updown is forced to run
+ *
+ * @param   o           Pointer to the DNS options struct
+ */
+static inline bool
+dns_updown_forced(const struct dns_options *o)
+{
+    return o->updown_flags == DNS_UPDOWN_FORCED;
+}
+
 #endif /* ifndef DNS_H */
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7e26069..af097f8 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3593,7 +3593,7 @@
     struct gc_arena gc = gc_new();
     struct dns_options *dns = &o->dns_options;
 
-    if (dns->servers || dns->user_set_updown)
+    if (dns->servers || dns_updown_user_set(dns) || dns_updown_forced(dns))
     {
         /* Clean up env from --dhcp-option DNS config */
         struct buffer name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
@@ -3667,7 +3667,7 @@
             }
         }
     }
-    else if (o->up_script && !dns->user_set_updown)
+    else if (o->up_script && !dns_updown_user_set(dns) && !dns_updown_forced(dns))
     {
         /* Set foreign option env vars from --dns config */
         const char *p[] = { "dhcp-option", NULL, NULL };
@@ -8182,15 +8182,15 @@
         if (streq(p[1], "disable"))
         {
             dns->updown = NULL;
-            dns->user_set_updown = false;
+            dns->updown_flags = DNS_UPDOWN_NO_FLAGS;
         }
         else if (streq(p[1], "force"))
         {
             /* force dns-updown run, even if a --up script is defined */
-            if (dns->user_set_updown == false)
+            if (!dns_updown_user_set(dns))
             {
                 dns->updown = DEFAULT_DNS_UPDOWN;
-                dns->user_set_updown = true;
+                dns->updown_flags = DNS_UPDOWN_FORCED;
             }
         }
         else
@@ -8201,7 +8201,7 @@
                 dns->updown = NULL;
             }
             set_user_script(options, &dns->updown, p[1], p[0], false);
-            dns->user_set_updown = true;
+            dns->updown_flags = DNS_UPDOWN_USER_SET;
         }
     }
     else if (streq(p[0], "dns") && p[1])

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1065?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I55940b78e35f0e3d74aa6cba14378afed97a444e
Gerrit-Change-Number: 1065
Gerrit-PatchSet: 4
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: newpatchset

[Openvpn-devel] [PATCH applied] Re: run forced --dns-updown without --script-security

[Gert D. <ge...@gr...>]

Thanks for addressing this imbalance wrt --script-security - I have not
tested the various combinations, just --dns-updown $builddir/... 
in my t_client tests (still works, and correctly still requires
script-security).  A brief stare at the code also looks reasonable.

The message "I am going to run *this* script now" is still a bit barebones,
though ;-)

2025-06-28 18:18:00 ../distro/dns-scripts/dns-updown
2025-06-28 18:18:00 WARNING: External program may not be called unless '--script-security 2' or higher...

Your patch has been applied to the master branch.

commit cbf3621825c9e2f2542a370f4c049411c71d2329
Author: Heiko Hund
Date:   Thu Jun 26 11:30:00 2025 +0200

     run forced --dns-updown without --script-security

     Signed-off-by: Heiko Hund <he...@is...>
     Acked-by: Frank Lichtenheld <fr...@li...>
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg31994.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [M] Change in openvpn[master]: Define a .clang-format file for the project

[flichtenheld (C. Review) <ge...@op...>]

Attention is currently required from: cron2, plaisthos.

flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/790?usp=email )

Change subject: Define a .clang-format file for the project
......................................................................


Patch Set 16:

(1 comment)

File .pre-commit-config.yaml:

http://gerrit.openvpn.net/c/openvpn/+/790/comment/5597360b_bf2ebcec :
PS15, Line 9: 
> This is "take it or leave it". […]
See https://gerrit.openvpn.net/c/openvpn/+/1070



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/790?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I40f6af10c5ee2f5aed4185d783fc622a2e3c19ff
Gerrit-Change-Number: 790
Gerrit-PatchSet: 16
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Comment-Date: Fri, 27 Jun 2025 16:50:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: cron2 <ge...@gr...>
Comment-In-Reply-To: flichtenheld <fr...@li...>
Gerrit-MessageType: comment

[Openvpn-devel] [L] Change in openvpn[master]: Remove uncrustify config and reformat-all.sh, switch GHA

[flichtenheld (C. Review) <ge...@op...>]

Attention is currently required from: cron2, flichtenheld, plaisthos.

Hello cron2, plaisthos, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/830?usp=email

to look at the new patch set (#11).


Change subject: Remove uncrustify config and reformat-all.sh, switch GHA
......................................................................

Remove uncrustify config and reformat-all.sh, switch GHA

Replaced with clang-format and pre-commit.

Add a README file that explains how to use pre-commit
and how to combine this with the old hook.

Old hook does not get removed and will be updated
to be compatible with manually installed clang-format
in a separate commit.

Change-Id: I15d4946800cbfaead67a73450ff3b12193814e54
Signed-off-by: Frank Lichtenheld <fr...@li...>
---
M .github/workflows/build.yaml
D dev-tools/reformat-all.sh
D dev-tools/special-files.lst
D dev-tools/uncrustify.conf
4 files changed, 12 insertions(+), 241 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/30/830/11

diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
index d4fdc9d..37c09fe 100644
--- a/.github/workflows/build.yaml
+++ b/.github/workflows/build.yaml
@@ -5,35 +5,26 @@
   pull_request:
 
 jobs:
-  checkuncrustify:
-    name: "Check code style with Uncrustify"
-    # Ubuntu 22.04 has uncrustify 0.72_f
-    runs-on: ubuntu-22.04
+  clang-format:
+    name: Check code style with clang-format
+    runs-on: ubuntu-24.04
     steps:
       - name: Install dependencies
-        run: sudo apt update && sudo apt install -y uncrustify
+        run: |
+          sudo apt update && sudo apt install -y python3-pip
+          pip3 install pre-commit
       - name: Checkout OpenVPN
         uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
-        with:
-          path: openvpn
-      - name: Show uncrustify version
-        run: uncrustify --version
-      - name: Run uncrustify
-        run: ./dev-tools/reformat-all.sh
-        working-directory: openvpn
+      - name: Run clang-format
+        run: pre-commit run -a --show-diff-on-failure || true
       - name: Check for changes
-        run: git diff --output=uncrustify-changes.patch
-        working-directory: openvpn
-      - name: Show changes on standard output
-        run: git diff
-        working-directory: openvpn
+        run: git diff --output=format-changes.patch
       - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
         with:
-          name: uncrustify-changes.patch
-          path: 'openvpn/uncrustify-changes.patch'
+          name: format-changes.patch
+          path: format-changes.patch
       - name: Set job status
-        run: test ! -s uncrustify-changes.patch
-        working-directory: openvpn
+        run: test ! -s format-changes.patch
 
   android:
     strategy:
diff --git a/dev-tools/reformat-all.sh b/dev-tools/reformat-all.sh
deleted file mode 100755
index 02421c1..0000000
--- a/dev-tools/reformat-all.sh
+++ /dev/null
@@ -1,136 +0,0 @@
-#!/bin/sh
-# reformat-all.sh - Reformat all git files in the checked out
-#                   git branch using uncrustify.
-#
-# Copyright (C) 2016-2025 - David Sommerseth <da...@op...>
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License
-# as published by the Free Software Foundation; either version 2
-# of the License.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
-#
-
-tstamp="$(date +%Y%m%d-%H%M%S)"
-files="$(pwd)/reformat-all_files-$tstamp.lst"
-log="$(pwd)/reformat-all_log-$tstamp.txt"
-
-srcroot="$(git rev-parse --show-toplevel)"
-cfg="$srcroot/dev-tools/uncrustify.conf"
-specialfiles="$srcroot/dev-tools/special-files.lst"
-
-export gitfiles=0
-export procfiles=0
-
-# Go to the root of the source tree
-cd "$srcroot"
-
-{
-    echo -n "** Starting $0: "
-    date
-
-    # Find all C source/header files
-    git ls-files | grep -E ".*\.[ch](\.in$|$)" > "${files}.git"
-
-    # Manage files which needs special treatment
-    awk -F\# '{gsub("\n| ", "", $1); print $1}' "$specialfiles" > "${files}.sp"
-    while read srcfile
-    do
-        res=$(grep "$srcfile" "${files}.sp" 2>/dev/null)
-        if [ $? -ne 0 ]; then
-            # If grep didn't find the file among special files,
-            # process it normally
-            echo "$srcfile" >> "$files"
-        else
-            mode=$(echo "$res" | cut -d:  -f1)
-            case "$mode" in
-                E)
-                    echo "** INFO **  Excluding '$srcfile'"
-                    ;;
-                P)
-                    echo "** INFO **  Pre-patching '$srcfile'"
-                    patchfile="${srcroot}"/dev-tools/reformat-patches/before_$(echo "$srcfile" | tr "/" "_").patch
-                    if [ -r "$patchfile" ]; then
-                        git apply "$patchfile"
-                        if [ $? -ne 0 ]; then
-                            echo "** ERROR **  Failed to apply pre-patch file: $patchfile"
-                            exit 2
-                        fi
-                    else
-                        echo "** WARN ** Pre-patch file for $srcfile is missing: $patchfile"
-                    fi
-                    echo "$srcfile" >> "${files}.postpatch"
-                    echo "$srcfile" >> "$files"
-                    ;;
-                *)
-                    echo "** WARN ** Unknown mode '$mode' for file '$srcfile'"
-                    ;;
-            esac
-        fi
-    done < "${files}.git"
-    rm -f "${files}.git" "${files}.sp"
-
-    # Kick off uncrustify
-    echo
-    echo "** INFO ** Running: uncrustify -c $cfg --no-backup -l C -F $files"
-    uncrustify -c "$cfg" --no-backup -l C -F "$files" 2>&1
-    res=$?
-    echo "** INFO ** Uncrustify completed (exit code $res)"
-} | tee "${log}-1"  # Log needs to be closed here, to be processed in next block
-
-{
-    # Check the results
-    gitfiles=$(wc -l "$files" | cut -d\  -f1)
-    procfiles=$(grep "Parsing: " "${log}-1" | wc -l)
-    echo
-    echo "C source/header files checked into git: $gitfiles"
-    echo "Files processed by uncrustify:          $procfiles"
-    echo
-
-    # Post-Patch files modified after we uncrustify have adjusted them
-    if [ -r "${files}.postpatch" ]; then
-        while read srcfile;
-        do
-            patchfile="${srcroot}"/dev-tools/reformat-patches/after_$(echo "$srcfile" | tr "/" "_").patch
-            if [ -r "$patchfile" ]; then
-                echo "** INFO **  Post-patching '$srcfile'"
-                git apply "$patchfile"
-                if [ $? -ne 0 ]; then
-                    echo "** WARN ** Failed to apply $patchfile"
-                fi
-            else
-                echo "** WARN ** Post-patch file for $srcfile is missing: $patchfile"
-            fi
-        done < "${files}.postpatch"
-        rm -f "${files}.postpatch"
-    fi
-} | tee "${log}-2" # Log needs to be closed here, to be processed in next block
-
-cat "${log}-1" "${log}-2" > "$log"
-
-{
-    ec=1
-    echo
-    if [ "$gitfiles" -eq "$procfiles" ]; then
-        echo "Reformatting completed successfully"
-        ec=0
-    else
-        last=$(tail -n1 "${log}-1")
-        echo "** ERROR ** Reformating failed to process all files."
-        echo "            uncrustify exit code: $res"
-        echo "            Last log line: $last"
-        echo
-    fi
-    rm -f "${log}-1" "${log}-2"
-} | tee -a "$log"
-rm -f "${files}"
-
-exit $ec
diff --git a/dev-tools/special-files.lst b/dev-tools/special-files.lst
deleted file mode 100644
index e5f2fc2..0000000
--- a/dev-tools/special-files.lst
+++ /dev/null
@@ -1,5 +0,0 @@
-E:doc/doxygen/doc_key_generation.h     # @verbatim section gets mistreated, exclude it
-E:src/compat/compat-lz4.c              # Preserve LZ4 upstream formatting
-E:src/compat/compat-lz4.h              # Preserve LZ4 upstream formatting
-E:src/openvpn/ovpn_dco_linux.h         # Preserve ovpn-dco upstream formatting
-E:src/openvpn/ovpn_dco_win.h           # Preserve ovpn-dco-win upstream formatting
diff --git a/dev-tools/uncrustify.conf b/dev-tools/uncrustify.conf
deleted file mode 100644
index 325f310..0000000
--- a/dev-tools/uncrustify.conf
+++ /dev/null
@@ -1,79 +0,0 @@
-# Use Allman-style
-indent_columns=4
-indent_braces=false
-indent_else_if=false
-indent_switch_case=4
-indent_label=1
-nl_if_brace=add
-nl_brace_else=add
-nl_elseif_brace=add
-nl_else_brace=add
-nl_else_if=remove
-nl_for_brace=add
-nl_while_brace=add
-nl_switch_brace=add
-nl_fdef_brace=add
-nl_do_brace=add
-sp_func_proto_paren=Remove
-sp_func_def_paren=Remove
-sp_func_call_paren=Remove
-sp_sizeof_paren=Remove
-
-# No tabs, spaces only
-indent_with_tabs=0
-align_with_tabs=false
-cmt_convert_tab_to_spaces=true
-
-# Do not put spaces between the # and preprocessor statements
-pp_space=remove
-
-# Various whitespace fiddling
-sp_assign=add
-sp_before_sparen=add
-sp_inside_sparen=remove
-sp_cond_colon=add
-sp_cond_question=add
-sp_bool=add
-sp_else_brace=add
-sp_brace_else=add
-sp_after_comma=add
-pos_arith=Lead
-pos_bool=Lead
-nl_func_type_name=add
-nl_before_case=true
-nl_assign_leave_one_liners=true
-nl_enum_leave_one_liners=true
-nl_brace_fparen=add
-nl_max=4
-nl_after_func_proto=2
-nl_end_of_file_min=1
-nl_end_of_file=force
-
-# Always use scoping braces for conditionals
-mod_full_brace_if=add
-mod_full_brace_if_chain=false
-mod_full_brace_while=add
-mod_full_brace_for=add
-mod_full_brace_do=add
-
-# Annotate #else and #endif statements
-mod_add_long_ifdef_endif_comment=20
-mod_add_long_ifdef_else_comment=5
-
-# Misc cleanup
-mod_remove_extra_semicolon=true
-
-# leave blank at end of empty for() statements
-sp_after_semi_for_empty=Add
-
-# Use C-style comments (/* .. */)
-cmt_c_nl_end=true
-cmt_star_cont=true
-cmt_cpp_to_c=true
-
-# Use "char **a"-style pointer stars/dereferences
-sp_before_ptr_star=Add
-sp_between_ptr_star=Remove
-sp_after_ptr_star=Remove
-sp_before_byref=Add
-sp_after_byref=Remove

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/830?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I15d4946800cbfaead67a73450ff3b12193814e54
Gerrit-Change-Number: 830
Gerrit-PatchSet: 11
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-MessageType: newpatchset

[Openvpn-devel] [S] Change in openvpn[master]: Update git-pre-commit-uncrustify.sh to handle clang-format

[flichtenheld (C. Review) <ge...@op...>]

Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1070?usp=email

to review the following change.


Change subject: Update git-pre-commit-uncrustify.sh to handle clang-format
......................................................................

Update git-pre-commit-uncrustify.sh to handle clang-format

Rename it as well, since it is not specific to
uncrustify anymore.

Change-Id: I03195c21807cdef0a2f903f424982ec29a555103
Signed-off-by: Frank Lichtenheld <fr...@li...>
---
R dev-tools/git-pre-commit-format.sh
1 file changed, 32 insertions(+), 14 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/70/1070/1

diff --git a/dev-tools/git-pre-commit-uncrustify.sh b/dev-tools/git-pre-commit-format.sh
similarity index 81%
rename from dev-tools/git-pre-commit-uncrustify.sh
rename to dev-tools/git-pre-commit-format.sh
index 9851c21..6e1ac71 100755
--- a/dev-tools/git-pre-commit-uncrustify.sh
+++ b/dev-tools/git-pre-commit-format.sh
@@ -2,6 +2,7 @@
 
 # Copyright (c) 2015, David Martin
 #               2022, Heiko Hund
+#               2025, Frank Lichtenheld
 # All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
@@ -26,10 +27,12 @@
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 
-# git pre-commit hook that runs an Uncrustify stylecheck.
+# git pre-commit hook that runs a stylecheck.
 # Features:
 #  - abort commit when commit does not comply with the style guidelines
 #  - create a patch of the proposed style changes
+#  - use clang-format or uncrustify depending on presence of .clang-format
+#    config file
 #
 # More info on Uncrustify: http://uncrustify.sourceforge.net/
 
@@ -77,27 +80,42 @@
     against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
 fi
 
-UNCRUSTIFY=$(command -v uncrustify)
-UNCRUST_CONFIG="$(git rev-parse --show-toplevel)/dev-tools/uncrustify.conf"
+TOPDIR="$(git rev-parse --show-toplevel)"
+if [ -e "${TOPDIR}/.clang-format" ]; then
+    TOOL=clang-format
+    TOOL_BIN=$(command -v clang-format)
+    TOOL_CMD="$TOOL_BIN"
 
-# make sure the config file and executable are correctly set
-if [ ! -f "$UNCRUST_CONFIG" ] ; then
-    printf "Error: uncrustify config file not found.\n"
-    printf "Expected to find it at $UNCRUST_CONFIG.\n"
-    printf "Aborting commit.\n"
-    exit 1
+    # Allow to use in parallel with pre-commit
+    if [ $(basename "$0") = "pre-commit.legacy" ]; then
+       echo "Skipping clang-format check in favor of pre-commit"
+       exit 0
+    fi
+else
+    TOOL=uncrustify
+    TOOL_BIN=$(command -v uncrustify)
+    UNCRUST_CONFIG="${TOPDIR}/dev-tools/uncrustify.conf"
+    TOOL_CMD="$TOOL_BIN -q -l C -c $UNCRUST_CONFIG"
+
+    # make sure the config file is correctly set
+    if [ ! -f "$UNCRUST_CONFIG" ] ; then
+        printf "Error: uncrustify config file not found.\n"
+        printf "Expected to find it at $UNCRUST_CONFIG.\n"
+        printf "Aborting commit.\n"
+        exit 1
+    fi
 fi
 
-if [ -z "$UNCRUSTIFY" ] ; then
-    printf "Error: uncrustify executable not found.\n"
+if [ -z "$TOOL_BIN" ] ; then
+    printf "Error: $TOOL executable not found.\n"
     printf "Is it installed and in your \$PATH?\n"
     printf "Aborting commit.\n"
     exit 1
 fi
 
 # create a filename to store our generated patch
-patch=$(mktemp /tmp/ovpn-fmt-XXXXXX)
-tmpout=$(mktemp /tmp/uncrustify-XXXXXX)
+patch=$(mktemp /tmp/ovpn-fmt-patch-XXXXXX)
+tmpout=$(mktemp /tmp/ovpn-fmt-tmp-XXXXXX)
 
 # create one patch containing all changes to the files
 # sed to remove quotes around the filename, if inserted by the system
@@ -131,7 +149,7 @@
     #    +++ $tmpout timestamp
     # to both lines working on the same file and having a a/ and b/ prefix.
     # Else it can not be applied with 'git apply'.
-    git show ":$file" | "$UNCRUSTIFY" -q -l C -c "$UNCRUST_CONFIG" -o "$tmpout"
+    git show ":$file" | $TOOL_CMD > "$tmpout"
     git show ":$file" | diff -u -- - "$tmpout" | \
         sed -e "1s|--- -|--- \"b/$file_escaped_target\"|" -e "2s|+++ $tmpout|+++ \"a/$file_escaped_target\"|" >> "$patch"
 done

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1070?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I03195c21807cdef0a2f903f424982ec29a555103
Gerrit-Change-Number: 1070
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-MessageType: newchange

[Openvpn-devel] [PATCH v3] dns: create NRPT registry key if it doesn't exist

[Gert D. <ge...@gr...>]

From: Heiko Hund <he...@is...>

Windows 2019 Server by default does not have the key where local system
NRPT rules are stored. Tests have determined that NRPT is actually
working when rules are created under the key. So, instead of failing if
the key doesn't exist, we create it, and things will start working.

Github: OpenVPN/openvpn#768
Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Signed-off-by: Heiko Hund <he...@is...>
Acked-by: Lev Stipakov <lst...@gm...>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1069
This mail reflects revision 3 of this Change.

Acked-by according to Gerrit (reflected above):
Lev Stipakov <lst...@gm...>

        
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 3bd2722..628a96b 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -2662,7 +2662,7 @@
     if (err == ERROR_FILE_NOT_FOUND)
     {
         *gpol = FALSE;
-        err = RegOpenKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, KEY_ALL_ACCESS, &nrpt);
+        err = RegCreateKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, NULL, 0, KEY_ALL_ACCESS, NULL, &nrpt, NULL);
         if (err)
         {
             nrpt = INVALID_HANDLE_VALUE;

[Openvpn-devel] [XS] Change in openvpn[master]: dns: create NRPT registry key if it doesn't exist

[stipa (C. Review) <ge...@op...>]

Attention is currently required from: d12fk, flichtenheld, plaisthos.

stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email )

Change subject: dns: create NRPT registry key if it doesn't exist
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

Patchset:

PS3: 
Tested with DnsPolicyConfig key presented and not presented, works as expected - a subkey for NRPT rule got created.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Gerrit-Change-Number: 1069
Gerrit-PatchSet: 3
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: stipa <lst...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: d12fk <he...@op...>
Gerrit-Comment-Date: Thu, 26 Jun 2025 13:00:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [XS] Change in openvpn[master]: dns: create NRPT registry key if it doesn't exist

[d12fk (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos, stipa.

d12fk has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email )

Change subject: dns: create NRPT registry key if it doesn't exist
......................................................................


Patch Set 2:

(1 comment)

File src/openvpnserv/interactive.c:

http://gerrit.openvpn.net/c/openvpn/+/1069/comment/e370619e_43d723cb :
PS2, Line 2665:         err = RegCreateKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, NULL, 0, 0, NULL, &nrpt, NULL);
> Doesn't work for me - the DnsPolicyConfig is created but no subkeys are present. […]
MSDN being wrong again ...



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Gerrit-Change-Number: 1069
Gerrit-PatchSet: 2
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: stipa <lst...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: stipa <lst...@gm...>
Gerrit-Comment-Date: Thu, 26 Jun 2025 12:29:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: stipa <lst...@gm...>
Gerrit-MessageType: comment

[Openvpn-devel] [XS] Change in openvpn[master]: dns: create NRPT registry key if it doesn't exist

[d12fk (C. Review) <ge...@op...>]

Attention is currently required from: d12fk, flichtenheld, plaisthos.

Hello flichtenheld, plaisthos, stipa, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email

to look at the new patch set (#3).


Change subject: dns: create NRPT registry key if it doesn't exist
......................................................................

dns: create NRPT registry key if it doesn't exist

Windows 2019 Server by default does not have the key where local system
NRPT rules are stored. Tests have determined that NRPT is actually
working when rules are created under the key. So, instead of failing if
the key doesn't exist, we create it, and things will start working.

Github: OpenVPN/openvpn#768
Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Signed-off-by: Heiko Hund <he...@is...>
---
M src/openvpnserv/interactive.c
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/69/1069/3

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 3bd2722..628a96b 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -2662,7 +2662,7 @@
     if (err == ERROR_FILE_NOT_FOUND)
     {
         *gpol = FALSE;
-        err = RegOpenKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, KEY_ALL_ACCESS, &nrpt);
+        err = RegCreateKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, NULL, 0, KEY_ALL_ACCESS, NULL, &nrpt, NULL);
         if (err)
         {
             nrpt = INVALID_HANDLE_VALUE;

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Gerrit-Change-Number: 1069
Gerrit-PatchSet: 3
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: stipa <lst...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: d12fk <he...@op...>
Gerrit-MessageType: newpatchset

[Openvpn-devel] [XS] Change in openvpn[master]: dns: create NRPT registry key if it doesn't exist

[stipa (C. Review) <ge...@op...>]

Attention is currently required from: d12fk, flichtenheld, plaisthos.

stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email )

Change subject: dns: create NRPT registry key if it doesn't exist
......................................................................


Patch Set 2: Code-Review-2

(1 comment)

File src/openvpnserv/interactive.c:

http://gerrit.openvpn.net/c/openvpn/+/1069/comment/4e9b30f4_a9c94189 :
PS2, Line 2665:         err = RegCreateKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, NULL, 0, 0, NULL, &nrpt, NULL);
Doesn't work for me - the DnsPolicyConfig is created but no subkeys are present. Also in Event Log there is:

openvpnserv error:  
   DeleteNrptRules: could not open NRPT base key (5)



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Gerrit-Change-Number: 1069
Gerrit-PatchSet: 2
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-Reviewer: stipa <lst...@gm...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: d12fk <he...@op...>
Gerrit-Comment-Date: Thu, 26 Jun 2025 11:01:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [XS] Change in openvpn[master]: dns: create NRPT registry key if it doesn't exist

[d12fk (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

Hello flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email

to look at the new patch set (#2).


Change subject: dns: create NRPT registry key if it doesn't exist
......................................................................

dns: create NRPT registry key if it doesn't exist

Windows 2019 Server by default does not have the key where local system
NRPT rules are stored. Tests have determined that NRPT is actually
working when rules are created under the key. So, instead of failing if
the key doesn't exist, we create it, and things will start working.

Github: OpenVPN/openvpn#768
Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Signed-off-by: Heiko Hund <he...@is...>
---
M src/openvpnserv/interactive.c
1 file changed, 1 insertion(+), 1 deletion(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/69/1069/2

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 3bd2722..0a00a6a 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -2662,7 +2662,7 @@
     if (err == ERROR_FILE_NOT_FOUND)
     {
         *gpol = FALSE;
-        err = RegOpenKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, KEY_ALL_ACCESS, &nrpt);
+        err = RegCreateKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, NULL, 0, 0, NULL, &nrpt, NULL);
         if (err)
         {
             nrpt = INVALID_HANDLE_VALUE;

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Gerrit-Change-Number: 1069
Gerrit-PatchSet: 2
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-MessageType: newpatchset

[Openvpn-devel] [XS] Change in openvpn[master]: dns: create NRPT registry key if it doesn't exist

[d12fk (C. Review) <ge...@op...>]

Attention is currently required from: flichtenheld, plaisthos.

Hello plaisthos, flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email

to review the following change.


Change subject: dns: create NRPT registry key if it doesn't exist
......................................................................

dns: create NRPT registry key if it doesn't exist

Windows 2019 Server by default does not have the key where local system
NRPT rules are stored. Tests have determined that NRPT is actually
working when rules are created under the key. So, instead of failing if
the key doesn't exist, we create it, and things will start working.

Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Signed-off-by: Heiko Hund <he...@is...>
---
M src/openvpnserv/interactive.c
1 file changed, 1 insertion(+), 1 deletion(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/69/1069/1

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 3bd2722..0a00a6a 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -2662,7 +2662,7 @@
     if (err == ERROR_FILE_NOT_FOUND)
     {
         *gpol = FALSE;
-        err = RegOpenKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, KEY_ALL_ACCESS, &nrpt);
+        err = RegCreateKeyExA(HKEY_LOCAL_MACHINE, sys_key, 0, NULL, 0, 0, NULL, &nrpt, NULL);
         if (err)
         {
             nrpt = INVALID_HANDLE_VALUE;

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1069?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I46132ebaf3bf3b16798b6f2416f7bf7272f5646b
Gerrit-Change-Number: 1069
Gerrit-PatchSet: 1
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-MessageType: newchange

[Openvpn-devel] [PATCH v2] run forced --dns-updown without --script-security

[Gert D. <ge...@gr...>]

From: Heiko Hund <he...@is...>

Due to a shortcut in the `--dns-updown force' implementation, running the
default dns-updown script required `--script-security 2'. This makes the
forced default script run without --script-security set.

Change-Id: I55940b78e35f0e3d74aa6cba14378afed97a444e
Signed-off-by: Heiko Hund <he...@is...>
Acked-by: Frank Lichtenheld <fr...@li...>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1065
This mail reflects revision 2 of this Change.

Acked-by according to Gerrit (reflected above):
Frank Lichtenheld <fr...@li...>

        
diff --git a/src/openvpn/dns.c b/src/openvpn/dns.c
index 939ae09..ea3d91b 100644
--- a/src/openvpn/dns.c
+++ b/src/openvpn/dns.c
@@ -264,7 +264,7 @@
     clone.servers = clone_dns_servers(o->servers, gc);
     clone.servers_prepull = clone_dns_servers(o->servers_prepull, gc);
     clone.updown = o->updown;
-    clone.user_set_updown = o->user_set_updown;
+    clone.updown_flags = o->updown_flags;
 
     return clone;
 }
@@ -580,7 +580,7 @@
     argv_printf(&argv, "%s", o->updown);
     argv_msg(M_INFO, &argv);
     int res;
-    if (o->user_set_updown)
+    if (dns_updown_user_set(o))
     {
         res = openvpn_run_script(&argv, es, S_EXITCODE, "dns updown");
     }
@@ -692,7 +692,7 @@
 run_up_down_command(bool up, struct options *o, const struct tuntap *tt, struct dns_updown_runner_info *updown_runner)
 {
     struct dns_options *dns = &o->dns_options;
-    if (!dns->updown || (o->up_script && !dns->user_set_updown))
+    if (!dns->updown || (o->up_script && !dns_updown_user_set(dns) && !dns_updown_forced(dns)))
     {
         return;
     }
diff --git a/src/openvpn/dns.h b/src/openvpn/dns.h
index 688daa7..d33f64e 100644
--- a/src/openvpn/dns.h
+++ b/src/openvpn/dns.h
@@ -42,13 +42,18 @@
     DNS_TRANSPORT_TLS
 };
 
+enum dns_updown_flags {
+    DNS_UPDOWN_NO_FLAGS,
+    DNS_UPDOWN_USER_SET,
+    DNS_UPDOWN_FORCED
+};
+
 struct dns_domain {
     struct dns_domain *next;
     const char *name;
 };
 
-struct dns_server_addr
-{
+struct dns_server_addr {
     union {
         struct in_addr a4;
         struct in6_addr a6;
@@ -103,7 +108,7 @@
     struct dns_server *servers;
     struct gc_arena gc;
     const char *updown;
-    bool user_set_updown;
+    enum dns_updown_flags updown_flags;
 };
 
 /**
@@ -195,4 +200,26 @@
  */
 void show_dns_options(const struct dns_options *o);
 
+/**
+ * Returns whether dns-updown is user defined
+ *
+ * @param   o           Pointer to the DNS options struct
+ */
+static inline bool
+dns_updown_user_set(const struct dns_options *o)
+{
+    return o->updown_flags == DNS_UPDOWN_USER_SET;
+}
+
+/**
+ * Returns whether dns-updown is forced to run
+ *
+ * @param   o           Pointer to the DNS options struct
+ */
+static inline bool
+dns_updown_forced(const struct dns_options *o)
+{
+    return o->updown_flags == DNS_UPDOWN_FORCED;
+}
+
 #endif /* ifndef DNS_H */
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7e26069..af097f8 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3593,7 +3593,7 @@
     struct gc_arena gc = gc_new();
     struct dns_options *dns = &o->dns_options;
 
-    if (dns->servers || dns->user_set_updown)
+    if (dns->servers || dns_updown_user_set(dns) || dns_updown_forced(dns))
     {
         /* Clean up env from --dhcp-option DNS config */
         struct buffer name = alloc_buf_gc(OPTION_PARM_SIZE, &gc);
@@ -3667,7 +3667,7 @@
             }
         }
     }
-    else if (o->up_script && !dns->user_set_updown)
+    else if (o->up_script && !dns_updown_user_set(dns) && !dns_updown_forced(dns))
     {
         /* Set foreign option env vars from --dns config */
         const char *p[] = { "dhcp-option", NULL, NULL };
@@ -8182,15 +8182,15 @@
         if (streq(p[1], "disable"))
         {
             dns->updown = NULL;
-            dns->user_set_updown = false;
+            dns->updown_flags = DNS_UPDOWN_NO_FLAGS;
         }
         else if (streq(p[1], "force"))
         {
             /* force dns-updown run, even if a --up script is defined */
-            if (dns->user_set_updown == false)
+            if (!dns_updown_user_set(dns))
             {
                 dns->updown = DEFAULT_DNS_UPDOWN;
-                dns->user_set_updown = true;
+                dns->updown_flags = DNS_UPDOWN_FORCED;
             }
         }
         else
@@ -8201,7 +8201,7 @@
                 dns->updown = NULL;
             }
             set_user_script(options, &dns->updown, p[1], p[0], false);
-            dns->user_set_updown = true;
+            dns->updown_flags = DNS_UPDOWN_USER_SET;
         }
     }
     else if (streq(p[0], "dns") && p[1])

[Openvpn-devel] [XS] Change in openvpn[master]: fix macOS dns-updown handling of parallel full redirects

[cron2 (C. Review) <ge...@op...>]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1066?usp=email )

Change subject: fix macOS dns-updown handling of parallel full redirects
......................................................................

fix macOS dns-updown handling of parallel full redirects

The script didn't handle scenarios well where two or more parallel VPN
connections want to replace the default DNS server. The DNS configuration
has a chance to get broken by the connections going down in a different
order than they came up in.

Disallowing all but the first connection to modify the default DNS server
will effectively prevent this issue. While it may break DNS for the
latter connections, it is the best we can do without knowing specifics
about the configurations.

Change-Id: I7b413578a8fc0c65fca26f72b901a9f7bc34b137
Signed-off-by: Heiko Hund <he...@is...>
Acked-by: Arne Schwabe <arn...@rf...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg31988.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M distro/dns-scripts/macos-dns-updown.sh
1 file changed, 9 insertions(+), 0 deletions(-)




diff --git a/distro/dns-scripts/macos-dns-updown.sh b/distro/dns-scripts/macos-dns-updown.sh
index 89d6882..c15abaa 100644
--- a/distro/dns-scripts/macos-dns-updown.sh
+++ b/distro/dns-scripts/macos-dns-updown.sh
@@ -30,6 +30,7 @@
 
 itf_dns_key="State:/Network/Service/openvpn-${dev}/DNS"
 dns_backup_key="State:/Network/Service/openvpn-${dev}/DnsBackup"
+dns_backup_key_pattern="State:/Network/Service/openvpn-.*/DnsBackup"
 
 function primary_dns_key {
     local uuid=$(echo "show State:/Network/Global/IPv4" | /usr/sbin/scutil | grep "PrimaryService" | cut -d: -f2 | xargs)
@@ -166,6 +167,11 @@
         echo -e "${cmds}" | /usr/sbin/scutil
         set_search_domains "$search_domains"
     else
+        echo list ${dns_backup_key_pattern} | /usr/sbin/scutil | grep -q 'no key' || {
+            echo "setting DNS failed, already redirecting to another tunnel"
+            exit 1
+        }
+
         local cmds=""
         cmds+="get $(primary_dns_key)\n"
         cmds+="set ${dns_backup_key}\n"
@@ -200,6 +206,9 @@
         echo "remove ${itf_dns_key}" | /usr/sbin/scutil
         unset_search_domains "$search_domains"
     else
+        # Do not unset if this tunnel did not set/backup DNS before
+        echo list ${dns_backup_key} | /usr/sbin/scutil | grep -qv 'no key' || return
+
         local cmds=""
         cmds+="get ${dns_backup_key}\n"
         cmds+="set $(primary_dns_key)\n"

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1066?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I7b413578a8fc0c65fca26f72b901a9f7bc34b137
Gerrit-Change-Number: 1066
Gerrit-PatchSet: 3
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: merged

[Openvpn-devel] [XS] Change in openvpn[master]: fix macOS dns-updown handling of parallel full redirects

[cron2 (C. Review) <ge...@op...>]

cron2 has uploaded a new patch set (#3) to the change originally created by d12fk. ( http://gerrit.openvpn.net/c/openvpn/+/1066?usp=email )

The following approvals got outdated and were removed:
Code-Review+2 by plaisthos


Change subject: fix macOS dns-updown handling of parallel full redirects
......................................................................

fix macOS dns-updown handling of parallel full redirects

The script didn't handle scenarios well where two or more parallel VPN
connections want to replace the default DNS server. The DNS configuration
has a chance to get broken by the connections going down in a different
order than they came up in.

Disallowing all but the first connection to modify the default DNS server
will effectively prevent this issue. While it may break DNS for the
latter connections, it is the best we can do without knowing specifics
about the configurations.

Change-Id: I7b413578a8fc0c65fca26f72b901a9f7bc34b137
Signed-off-by: Heiko Hund <he...@is...>
Acked-by: Arne Schwabe <arn...@rf...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg31988.html
Signed-off-by: Gert Doering <ge...@gr...>
---
M distro/dns-scripts/macos-dns-updown.sh
1 file changed, 9 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/66/1066/3

diff --git a/distro/dns-scripts/macos-dns-updown.sh b/distro/dns-scripts/macos-dns-updown.sh
index 89d6882..c15abaa 100644
--- a/distro/dns-scripts/macos-dns-updown.sh
+++ b/distro/dns-scripts/macos-dns-updown.sh
@@ -30,6 +30,7 @@
 
 itf_dns_key="State:/Network/Service/openvpn-${dev}/DNS"
 dns_backup_key="State:/Network/Service/openvpn-${dev}/DnsBackup"
+dns_backup_key_pattern="State:/Network/Service/openvpn-.*/DnsBackup"
 
 function primary_dns_key {
     local uuid=$(echo "show State:/Network/Global/IPv4" | /usr/sbin/scutil | grep "PrimaryService" | cut -d: -f2 | xargs)
@@ -166,6 +167,11 @@
         echo -e "${cmds}" | /usr/sbin/scutil
         set_search_domains "$search_domains"
     else
+        echo list ${dns_backup_key_pattern} | /usr/sbin/scutil | grep -q 'no key' || {
+            echo "setting DNS failed, already redirecting to another tunnel"
+            exit 1
+        }
+
         local cmds=""
         cmds+="get $(primary_dns_key)\n"
         cmds+="set ${dns_backup_key}\n"
@@ -200,6 +206,9 @@
         echo "remove ${itf_dns_key}" | /usr/sbin/scutil
         unset_search_domains "$search_domains"
     else
+        # Do not unset if this tunnel did not set/backup DNS before
+        echo list ${dns_backup_key} | /usr/sbin/scutil | grep -qv 'no key' || return
+
         local cmds=""
         cmds+="get ${dns_backup_key}\n"
         cmds+="set $(primary_dns_key)\n"

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1066?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I7b413578a8fc0c65fca26f72b901a9f7bc34b137
Gerrit-Change-Number: 1066
Gerrit-PatchSet: 3
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-MessageType: newpatchset

[Openvpn-devel] [PATCH applied] Re: fix macOS dns-updown handling of parallel full redirects

[Gert D. <ge...@gr...>]

I have just stared a bit at the code ("looks reasonable"), thanks to
Arne for confirming that it fixes the observed problem ("two VPN
connections active at the same time, both trying to redirect all DNS
queries").  Basically this will do nothing but print an error for the
second VPN to come up - and there is not much else we can do in this
scenario.

Your patch has been applied to the master branch.

commit 7a2b814fee06ab1edeb5f9ad104880f0fef5b0ba
Author: Heiko Hund
Date:   Thu Jun 26 11:19:52 2025 +0200

     fix macOS dns-updown handling of parallel full redirects

     Signed-off-by: Heiko Hund <he...@is...>
     Acked-by: Arne Schwabe <arn...@rf...>
     Message-Id: <202...@gr...>
     URL: https://www.mail-archive.com/ope...@li.../msg31988.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [XS] Change in openvpn[master]: fix macOS dns-updown handling of parallel full redirects

[plaisthos (C. Review) <ge...@op...>]

Attention is currently required from: d12fk, flichtenheld.

plaisthos has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1066?usp=email )

Change subject: fix macOS dns-updown handling of parallel full redirects
......................................................................


Patch Set 2:

(1 comment)

Patchset:

PS2: 
I tested it and it worked.

2025-06-26 11:15:51 /Users/arne/oss/openvpn-git/distro/dns-scripts/macos-dns-updown.sh
setting DNS failed, already redirecting to another tunnel
2025-06-26 11:15:51 dns up command exited with status 1



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1066?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I7b413578a8fc0c65fca26f72b901a9f7bc34b137
Gerrit-Change-Number: 1066
Gerrit-PatchSet: 2
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: d12fk <he...@op...>
Gerrit-Comment-Date: Thu, 26 Jun 2025 09:18:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Openvpn-devel] [M] Change in openvpn[master]: run forced --dns-updown without --script-security

[flichtenheld (C. Review) <ge...@op...>]

Attention is currently required from: d12fk, plaisthos.

flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1065?usp=email )

Change subject: run forced --dns-updown without --script-security
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

Patchset:

PS2: 
Tested, looks good



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1065?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I55940b78e35f0e3d74aa6cba14378afed97a444e
Gerrit-Change-Number: 1065
Gerrit-PatchSet: 2
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: d12fk <he...@op...>
Gerrit-Comment-Date: Thu, 26 Jun 2025 09:21:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Openvpn-devel] [PATCH v2] fix macOS dns-updown handling of parallel full redirects

[Gert D. <ge...@gr...>]

From: Heiko Hund <he...@is...>

The script didn't handle scenarios well where two or more parallel VPN
connections want to replace the default DNS server. The DNS configuration
has a chance to get broken by the connections going down in a different
order than they came up in.

Disallowing all but the first connection to modify the default DNS server
will effectively prevent this issue. While it may break DNS for the
latter connections, it is the best we can do without knowing specifics
about the configurations.

Change-Id: I7b413578a8fc0c65fca26f72b901a9f7bc34b137
Signed-off-by: Heiko Hund <he...@is...>
Acked-by: Arne Schwabe <arn...@rf...>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1066
This mail reflects revision 2 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <arn...@rf...>

        
diff --git a/distro/dns-scripts/macos-dns-updown.sh b/distro/dns-scripts/macos-dns-updown.sh
index 89d6882..c15abaa 100644
--- a/distro/dns-scripts/macos-dns-updown.sh
+++ b/distro/dns-scripts/macos-dns-updown.sh
@@ -30,6 +30,7 @@
 
 itf_dns_key="State:/Network/Service/openvpn-${dev}/DNS"
 dns_backup_key="State:/Network/Service/openvpn-${dev}/DnsBackup"
+dns_backup_key_pattern="State:/Network/Service/openvpn-.*/DnsBackup"
 
 function primary_dns_key {
     local uuid=$(echo "show State:/Network/Global/IPv4" | /usr/sbin/scutil | grep "PrimaryService" | cut -d: -f2 | xargs)
@@ -166,6 +167,11 @@
         echo -e "${cmds}" | /usr/sbin/scutil
         set_search_domains "$search_domains"
     else
+        echo list ${dns_backup_key_pattern} | /usr/sbin/scutil | grep -q 'no key' || {
+            echo "setting DNS failed, already redirecting to another tunnel"
+            exit 1
+        }
+
         local cmds=""
         cmds+="get $(primary_dns_key)\n"
         cmds+="set ${dns_backup_key}\n"
@@ -200,6 +206,9 @@
         echo "remove ${itf_dns_key}" | /usr/sbin/scutil
         unset_search_domains "$search_domains"
     else
+        # Do not unset if this tunnel did not set/backup DNS before
+        echo list ${dns_backup_key} | /usr/sbin/scutil | grep -qv 'no key' || return
+
         local cmds=""
         cmds+="get ${dns_backup_key}\n"
         cmds+="set $(primary_dns_key)\n"

[Openvpn-devel] [XS] Change in openvpn[master]: fix macOS dns-updown handling of parallel full redirects

[plaisthos (C. Review) <ge...@op...>]

Attention is currently required from: d12fk, flichtenheld.

plaisthos has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1066?usp=email )

Change subject: fix macOS dns-updown handling of parallel full redirects
......................................................................


Patch Set 2: Code-Review+2


-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1066?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I7b413578a8fc0c65fca26f72b901a9f7bc34b137
Gerrit-Change-Number: 1066
Gerrit-PatchSet: 2
Gerrit-Owner: d12fk <he...@op...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Attention: d12fk <he...@op...>
Gerrit-Comment-Date: Thu, 26 Jun 2025 09:17:52 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment