Menu
▾
▴
openvpn-devel
|
From: d12fk (C. Review) <ge...@op...> - 2025-06-25 10:45:27
|
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/+/1066?usp=email
to review the following change.
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...>
---
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/1
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: 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
|
|
From: plaisthos (C. Review) <ge...@op...> - 2025-06-26 09:18:01
|
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 |
|
From: Gert D. <ge...@gr...> - 2025-06-26 09:20:13
|
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" |
|
From: Gert D. <ge...@gr...> - 2025-06-26 09:26:28
|
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
|
|
From: plaisthos (C. Review) <ge...@op...> - 2025-06-26 09:23:27
|
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 |
|
From: cron2 (C. Review) <ge...@op...> - 2025-06-26 09:26:46
|
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 |
|
From: cron2 (C. Review) <ge...@op...> - 2025-06-26 09:26:52
|
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 |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X