Menu
▾
▴
openvpn-devel
|
From: mrbff (C. Review) <ge...@op...> - 2025-09-24 09:30:35
|
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/+/1210?usp=email
to review the following change.
Change subject: redirect-gateway: Only redirect traffic through TUN if address families match
......................................................................
redirect-gateway: Only redirect traffic through TUN if address families match
Fixes an ifconfig push-reply bug where, if the remote is switched and the new
TUN has a different address family, the previous ifconfig options remain
assigned to the new TUN.
Adds a check in do_init_route_ipv6_list() to add default routes toward the TUN
only if the TUN has IPv6 addresses.
Change-Id: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c
---
M src/openvpn/init.c
M src/openvpn/options.c
2 files changed, 6 insertions(+), 1 deletion(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/10/1210/1
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 0d7a2ec..a5be243 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1518,7 +1518,7 @@
/* redirect (IPv6) gateway to VPN? if yes, add a few more specifics
*/
- if (options->routes_ipv6->flags & RG_REROUTE_GW)
+ if (options->routes_ipv6->flags & RG_REROUTE_GW && options->ifconfig_ipv6_local)
{
char *opt_list[] = { "::/3", "2000::/4", "3000::/4", "fc00::/7", NULL };
int i;
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 151a016..5972cb9 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -5464,6 +5464,11 @@
const msglvl_t msglevel = D_PUSH_ERRORS | M_OPTERR;
unsigned int update_options_found = 0;
+ options->ifconfig_local = NULL;
+ options->ifconfig_remote_netmask = NULL;
+ options->ifconfig_ipv6_local = NULL;
+ options->ifconfig_ipv6_netbits = 0;
+ options->ifconfig_ipv6_remote = NULL;
while (buf_parse(buf, ',', line, sizeof(line)))
{
char *p[MAX_PARMS + 1];
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1210?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: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c
Gerrit-Change-Number: 1210
Gerrit-PatchSet: 1
Gerrit-Owner: mrbff <ma...@ma...>
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: cron2 (C. Review) <ge...@op...> - 2025-09-24 09:56:11
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email ) Change subject: redirect-gateway: Only redirect traffic through TUN if address families match ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Is this about `push-reply`, as the commit message says, or about `PUSH_UPDATE`, as the code suggests? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1210?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: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c Gerrit-Change-Number: 1210 Gerrit-PatchSet: 1 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: cron2 <ge...@gr...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Wed, 24 Sep 2025 09:55:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment |
|
From: mrbff (C. Review) <ge...@op...> - 2025-09-24 10:10:30
|
Attention is currently required from: cron2, flichtenheld, plaisthos. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email ) Change subject: redirect-gateway: Only redirect traffic through TUN if address families match ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Is this about `push-reply`, as the commit message says, or about `PUSH_UPDATE`, as the code suggests […] No push-reply is correct, but actually is for push-update too. The bug can be triggered just adding a sequence of remote-ipv4 and remote-ipv6, so if server disconnect, the connection is switched to the next remote and if the family is not the same it can creates some problems. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1210?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: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c Gerrit-Change-Number: 1210 Gerrit-PatchSet: 1 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: cron2 <ge...@gr...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Wed, 24 Sep 2025 10:10:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Gerrit-MessageType: comment |
|
From: cron2 (C. Review) <ge...@op...> - 2025-09-24 10:11:54
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email ) Change subject: redirect-gateway: Only redirect traffic through TUN if address families match ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > No push-reply is correct, but actually is for push-update too. […] Can you link to an issue having a full log showing what happens and why this is a problem? I need to test this before merging, so need to know what to look for. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1210?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: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c Gerrit-Change-Number: 1210 Gerrit-PatchSet: 1 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: cron2 <ge...@gr...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Wed, 24 Sep 2025 10:11:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Comment-In-Reply-To: mrbff <ma...@ma...> Gerrit-MessageType: comment |
|
From: mrbff (C. Review) <ge...@op...> - 2025-09-25 13:07:39
|
Attention is currently required from: cron2, flichtenheld, plaisthos. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email ) Change subject: redirect-gateway: Only redirect traffic through TUN if address families match ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Can you link to an issue having a full log showing what happens and why this is a problem? I need t […] https://github.com/OpenVPN/openvpn/issues/850 Issue opened, hope it's clear enough. Anyway you can just watch the test logs provided in the issue. If you want to easily test by yourself i also provided some scripts i used. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1210?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: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c Gerrit-Change-Number: 1210 Gerrit-PatchSet: 1 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: cron2 <ge...@gr...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: cron2 <ge...@gr...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Thu, 25 Sep 2025 13:07:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Comment-In-Reply-To: mrbff <ma...@ma...> Gerrit-MessageType: comment |
|
From: mrbff (C. Review) <ge...@op...> - 2025-10-05 18:59:56
|
Attention is currently required from: cron2, flichtenheld, plaisthos.
Hello flichtenheld, plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email
to look at the new patch set (#2).
Change subject: redirect-gateway: Only redirect traffic through TUN if address families match
......................................................................
redirect-gateway: Only redirect traffic through TUN if address families match
Fixes an ifconfig push-reply bug where, if the remote is switched and the new
TUN has a different address family, the previous ifconfig options remain
assigned to the new TUN.
Adds a check in do_init_route_ipv6_list() to add default routes toward the TUN
only if the TUN has IPv6 addresses.
Change-Id: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c
---
M src/openvpn/init.c
M src/openvpn/options.c
2 files changed, 13 insertions(+), 1 deletion(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/10/1210/2
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index f8a0fee..aaa0573 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1523,7 +1523,7 @@
/* redirect (IPv6) gateway to VPN? if yes, add a few more specifics
*/
- if (options->routes_ipv6->flags & RG_REROUTE_GW)
+ if (options->routes_ipv6->flags & RG_REROUTE_GW && options->ifconfig_ipv6_local)
{
char *opt_list[] = { "::/3", "2000::/4", "3000::/4", "fc00::/7", NULL };
int i;
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index f35738d..185233f 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -5470,6 +5470,18 @@
const msglvl_t msglevel = D_PUSH_ERRORS | M_OPTERR;
unsigned int update_options_found = 0;
+ /* When receiving a PUSH_REPLY, reset the ifconfig options to prevent
+ * stale data conflicts. This could be necessary when the new address has a
+ * different address family than the previous one. */
+ if (!is_update)
+ {
+ options->ifconfig_local = NULL;
+ options->ifconfig_remote_netmask = NULL;
+ options->ifconfig_ipv6_local = NULL;
+ options->ifconfig_ipv6_netbits = 0;
+ options->ifconfig_ipv6_remote = NULL;
+ }
+
while (buf_parse(buf, ',', line, sizeof(line)))
{
char *p[MAX_PARMS + 1];
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c
Gerrit-Change-Number: 1210
Gerrit-PatchSet: 2
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: cron2 <ge...@gr...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Attention: flichtenheld <fr...@li...>
|
|
From: mrbff (C. Review) <ge...@op...> - 2025-10-06 09:11:08
|
Attention is currently required from: cron2, flichtenheld, plaisthos.
Hello flichtenheld, plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email
to look at the new patch set (#3).
Change subject: redirect-gateway: only redirect traffic through TUN if address families match
......................................................................
redirect-gateway: only redirect traffic through TUN if address families match
Fixes an ifconfig push-reply bug where, if the remote is switched and the new
TUN has a different address family, the previous ifconfig options remain
assigned to the new TUN.
Adds a check in do_init_route_ipv6_list() to add default routes toward the TUN
only if the TUN has IPv6 addresses.
It also resets the ifconfig and ifconfig-ipv6 options on every PUSH_REPLY and
whenever a valid ifconfig or ifconfig-ipv6 option is found in a PUSH_UPDATE
message to avoid stale data conflicts. This may be necessary when the new
address has a different address family than the old one.
Change-Id: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c
---
M src/openvpn/init.c
M src/openvpn/options.c
2 files changed, 62 insertions(+), 2 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/10/1210/3
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index f8a0fee..aaa0573 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1523,7 +1523,7 @@
/* redirect (IPv6) gateway to VPN? if yes, add a few more specifics
*/
- if (options->routes_ipv6->flags & RG_REROUTE_GW)
+ if (options->routes_ipv6->flags & RG_REROUTE_GW && options->ifconfig_ipv6_local)
{
char *opt_list[] = { "::/3", "2000::/4", "3000::/4", "fc00::/7", NULL };
int i;
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index f35738d..fc18931 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -5459,6 +5459,17 @@
}
}
+/* Reset all ifconfig and ifconfig-ipv6 related options to their default/NULL state */
+#define RESET_IFCONFIG_OPTIONS(opt) \
+ do \
+ { \
+ (opt)->ifconfig_local = NULL; \
+ (opt)->ifconfig_remote_netmask = NULL; \
+ (opt)->ifconfig_ipv6_local = NULL; \
+ (opt)->ifconfig_ipv6_netbits = 0; \
+ (opt)->ifconfig_ipv6_remote = NULL; \
+ } while (0)
+
bool
apply_push_options(struct context *c, struct options *options, struct buffer *buf,
unsigned int permission_mask, unsigned int *option_types_found,
@@ -5470,6 +5481,14 @@
const msglvl_t msglevel = D_PUSH_ERRORS | M_OPTERR;
unsigned int update_options_found = 0;
+ /* When receiving a PUSH_REPLY, reset the ifconfig options to prevent
+ * stale data conflicts. This could be necessary when the new address has a
+ * different address family than the previous one. */
+ if (!is_update)
+ {
+ RESET_IFCONFIG_OPTIONS(options);
+ }
+
while (buf_parse(buf, ',', line, sizeof(line)))
{
char *p[MAX_PARMS + 1];
@@ -6014,7 +6033,48 @@
const bool pull_mode = BOOL_CAST(permission_mask & OPT_P_PULL_MODE);
ASSERT(MAX_PARMS >= 7);
- if (streq(p[0], "route") && p[1] && !p[5])
+ if (streq(p[0], "ifconfig") && p[1] && p[2] && !p[3])
+ {
+ VERIFY_PERMISSION(OPT_P_UP);
+ if (ip_or_dns_addr_safe(p[1], options->allow_pull_fqdn)
+ && ip_or_dns_addr_safe(p[2], options->allow_pull_fqdn)) /* FQDN -- may be DNS name */
+ {
+ /* When receiving a PUSH_UPDATE containing a valid ifconfig option,
+ * we should reset the ifconfig and ifconfig-ipv6 options to prevent
+ * stale data conflicts. This could be necessary when the new address has a
+ * different address family than the previous one. Same is true when a valid
+ * ifconfig-ipv6 option is received. */
+ RESET_IFCONFIG_OPTIONS(options);
+ }
+ else
+ {
+ msg(msglevel, "ifconfig parms '%s' and '%s' must be valid addresses", p[1], p[2]);
+ goto err;
+ }
+ }
+ else if (streq(p[0], "ifconfig-ipv6") && p[1] && p[2] && !p[3])
+ {
+ unsigned int netbits;
+
+ VERIFY_PERMISSION(OPT_P_UP);
+ if (get_ipv6_addr(p[1], NULL, &netbits, msglevel) && ipv6_addr_safe(p[2]))
+ {
+ if (netbits < 64 || netbits > 124)
+ {
+ msg(msglevel, "ifconfig-ipv6: /netbits must be between 64 and 124, not '/%d'",
+ netbits);
+ goto err;
+ }
+
+ RESET_IFCONFIG_OPTIONS(options);
+ }
+ else
+ {
+ msg(msglevel, "ifconfig-ipv6 parms '%s' and '%s' must be valid addresses", p[1], p[2]);
+ goto err;
+ }
+ }
+ else if (streq(p[0], "route") && p[1] && !p[5])
{
if (!(*update_options_found & OPT_P_U_ROUTE))
{
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c
Gerrit-Change-Number: 1210
Gerrit-PatchSet: 3
Gerrit-Owner: mrbff <ma...@ma...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: cron2 <ge...@gr...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: plaisthos <arn...@rf...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Attention: flichtenheld <fr...@li...>
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-10-06 09:15:40
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. cron2 has posted comments on this change by mrbff. ( http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email ) Change subject: redirect-gateway: only redirect traffic through TUN if address families match ...................................................................... Patch Set 3: Code-Review-2 (1 comment) Patchset: PS3: This looks like it's not actually the correct way to do it - with the patch, a client that has a local `--ifconfig` configured will lose that on PUSH_REPLY, no matter what the server pushes. This is a valid use case and must not be broken. We do have the concept of "saved config data" which preserves what is in the client config - so `ifconfig` and `ifconfig-ipv6` need to be reset to "what is in the client config" before connecting out (PUSH_REPLY), otherwise we will break existing setups. For `PUSH_REPLY` I do not really care, as that's new functionality. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c Gerrit-Change-Number: 1210 Gerrit-PatchSet: 3 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> 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-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Mon, 06 Oct 2025 09:15:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes |
|
From: cron2 (C. Review) <ge...@op...> - 2025-10-06 09:18:01
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. cron2 has posted comments on this change by mrbff. ( http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email ) Change subject: redirect-gateway: only redirect traffic through TUN if address families match ...................................................................... Patch Set 3: (1 comment) Patchset: PS1: > https://github.com/OpenVPN/openvpn/issues/850 […] please do also link to the issue in the commit message (`Github: fixes OpenVPN/openvpn#850`) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1210?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: Ib3458a9ed2eb38e00184c4a92659b83b97fe476c Gerrit-Change-Number: 1210 Gerrit-PatchSet: 3 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: cron2 <ge...@gr...> 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-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Mon, 06 Oct 2025 09:17:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cron2 <ge...@gr...> Comment-In-Reply-To: mrbff <ma...@ma...> |
×
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