Extend push-remove to also handle 'ifconfig'.

PATCH In review Gert Doering 2 participants 5 messages Jun 29, 2018 – Jul 2, 2018

Share & tools

Gert Doering

author

2018-06-29 17:45 UTC #0 lore raw

PATCH 1/1 Extend push-remove to also handle 'ifconfig'.

Push-remove (introduced in commit 970312f1850) did not handle "ifconfig" yet, as both "ifconfig" and "ifconfig-ipv6" are handled differently from all other pushed options. Since there was no valid use-case to not-push "ifconfig" (no support on the client side for running IPv6-only) this was not an issue so far - but with the recent commits to enable ipv6-only operation it can be a desirable feature.

The implementation is similar to "push-remove ifconfig-ipv6" - namely, flagging via a new context option (c->options.push_ifconfig_ipv4_blocked) and then not creating the push statement in "send_push_reply()".

While not truly elegant, it's much less invasive than the alternatives (storing the list of "push-remove" statements somewhere and then checking in push_option_ex())

Trac: #1072

Signed-off-by Gert Doering <[email protected]...>

---
 src/openvpn/options.h |  1 +
 src/openvpn/push.c    | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

src/openvpn/options.h +1 -0

index f7d0145..3a6c33f 100644
@@ -425,6 +425,7 @@ struct options
     bool push_ifconfig_constraint_defined;
     in_addr_t push_ifconfig_constraint_network;
     in_addr_t push_ifconfig_constraint_netmask;
+    bool push_ifconfig_ipv4_blocked;                    /* IPv4 */
     bool push_ifconfig_ipv6_defined;                    /* IPv6 */
     struct in6_addr push_ifconfig_ipv6_local;           /* IPv6 */
     int push_ifconfig_ipv6_netbits;                     /* IPv6 */

src/openvpn/push.c +9 -1

index 6a30e47..86ec7ea 100644
@@ -342,7 +342,8 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
 
     /* ipv4 */
     if (c->c2.push_ifconfig_defined && c->c2.push_ifconfig_local
-        && c->c2.push_ifconfig_remote_netmask)
+        && c->c2.push_ifconfig_remote_netmask &&
+        !o->push_ifconfig_ipv4_blocked)
     {
         in_addr_t ifconfig_local = c->c2.push_ifconfig_local;
         if (c->c2.push_ifconfig_local_alias)

@@ -602,6 +603,13 @@ push_remove_option(struct options *o, const char *p)
 {
     msg(D_PUSH_DEBUG, "PUSH_REMOVE searching for: '%s'", p);
 
+    /* ifconfig is special, as not part of the push list */
+    if (streq( p, "ifconfig" ))
+    {
+        o->push_ifconfig_ipv4_blocked = true;
+        return;
+    }
+
     /* ifconfig-ipv6 is special, as not part of the push list */
     if (streq( p, "ifconfig-ipv6" ))
     {

Signature · 1 line

2.7.3

Antonio Quartulli

2018-06-30 13:02 UTC #1 lore raw

Hi,

thanks for taking care of this!
IPv4 settings should all be treated like others and never "assumed to be there all the time" anymore :)

Quoted 40 lines · Gert Doering

Push-remove (introduced in commit 970312f1850) did not handle "ifconfig" yet, as both "ifconfig" and "ifconfig-ipv6" are handled differently from all other pushed options. Since there was no valid use-case to not-push "ifconfig" (no support on the client side for running IPv6-only) this was not an issue so far - but with the recent commits to enable ipv6-only operation it can be a desirable feature.
The implementation is similar to "push-remove ifconfig-ipv6" - namely, flagging via a new context option (c->options.push_ifconfig_ipv4_blocked) and then not creating the push statement in "send_push_reply()".
While not truly elegant, it's much less invasive than the alternatives (storing the list of "push-remove" statements somewhere and then checking in push_option_ex())
Trac: #1072
Signed-off-by Gert Doering <[email protected]...>
---
 src/openvpn/options.h |  1 +
 src/openvpn/push.c    | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)
src/openvpn/options.h +1 -0
index f7d0145..3a6c33f 100644
@@ -425,6 +425,7 @@ struct options
     bool push_ifconfig_constraint_defined;
     in_addr_t push_ifconfig_constraint_network;
     in_addr_t push_ifconfig_constraint_netmask;
+    bool push_ifconfig_ipv4_blocked;                    /* IPv4 */
     bool push_ifconfig_ipv6_defined;                    /* IPv6 */
     struct in6_addr push_ifconfig_ipv6_local;           /* IPv6 */
     int push_ifconfig_ipv6_netbits;                     /* IPv6 */
src/openvpn/push.c +0 -0
index 6a30e47..86ec7ea 100644

src/openvpn/push.c prepare_push_reply(struct context *c, struct gc_arena *gc, · line 345

Quoted 5 lines

src/openvpn/push.c +1 -1

@@ -342,7 +342,8 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
 
     /* ipv4 */
     if (c->c2.push_ifconfig_defined && c->c2.push_ifconfig_local
-        && c->c2.push_ifconfig_remote_netmask)
+        && c->c2.push_ifconfig_remote_netmask &&

wrong style: && should go on the next line (unless you can put also the following condition on the same line).

src/openvpn/push.c push_remove_option(struct options *o, const char *p) · line 607

Quoted 10 lines

src/openvpn/push.c +3 -0

+        !o->push_ifconfig_ipv4_blocked)
     {
         in_addr_t ifconfig_local = c->c2.push_ifconfig_local;
         if (c->c2.push_ifconfig_local_alias)

@@ -602,6 +603,13 @@ push_remove_option(struct options *o, const char *p)
 {
     msg(D_PUSH_DEBUG, "PUSH_REMOVE searching for: '%s'", p);
 
+    /* ifconfig is special, as not part of the push list */
+    if (streq( p, "ifconfig" ))

I know you have simply copy/pasted the code from below, but since at it, please use the right style and remove useless spaces after of before the parentheses.

src/openvpn/push.c push_remove_option(struct options *o, const char *p) · line 615

Quoted 9 lines

src/openvpn/push.c +5 -0

+    {
+        o->push_ifconfig_ipv4_blocked = true;
+        return;
+    }
+
     /* ifconfig-ipv6 is special, as not part of the push list */
     if (streq( p, "ifconfig-ipv6" ))
     {

Once the small style glitches above, I can give this patch my ACK.

I tested with my small environment and it does what it says without breaking other features.

While, I think it is ok to take this patch as it is, as a follow up it would probably make sense to treat ifconfig and ifconfig-ipv6 like the other push-remove directives and allow for partial matching. This would make it identical to pull-filter (I think).

Cheers,

Signature · 1 line

Antonio Quartulli

Gert Doering

author

2018-07-01 19:59 UTC #2 lore raw

PATCH 1/1 Extend push-remove to also handle 'ifconfig'.

While not truly elegant, it's much less invasive than the alternatives (storing the list of "push-remove" statements somewhere and then checking in push_option_ex())

Trac: #1072

Signed-off-by Gert Doering <[email protected]...>

---
v2: style changes, manpage note about exact match

---
 doc/openvpn.8         |  5 +++++
 src/openvpn/options.h |  1 +
 src/openvpn/push.c    | 10 +++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

doc/openvpn.8 +5 -0

index 0e5d467..46ea58b 100644
@@ -3045,6 +3045,11 @@ an option,
 can be used to first remove the old value, and then add a new
 .B \-\-push
 option with the new value.
+
+NOTE2: due to implementation details, 'ifconfig' and 'ifconfig-ipv6'
+can only be removed with an exact match on the option ("push-remove ifconfig"),
+no substring matching and no matching on the IPv4/IPv6 address argument
+is possible.
 .\"*********************************************************
 .TP
 .B \-\-push\-peer\-info

src/openvpn/options.h +1 -0

index f7d0145..3a6c33f 100644
@@ -425,6 +425,7 @@ struct options
     bool push_ifconfig_constraint_defined;
     in_addr_t push_ifconfig_constraint_network;
     in_addr_t push_ifconfig_constraint_netmask;
+    bool push_ifconfig_ipv4_blocked;                    /* IPv4 */
     bool push_ifconfig_ipv6_defined;                    /* IPv6 */
     struct in6_addr push_ifconfig_ipv6_local;           /* IPv6 */
     int push_ifconfig_ipv6_netbits;                     /* IPv6 */

src/openvpn/push.c +9 -1

index 6a30e47..a7ec4dd 100644
@@ -342,7 +342,8 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
 
     /* ipv4 */
     if (c->c2.push_ifconfig_defined && c->c2.push_ifconfig_local
-        && c->c2.push_ifconfig_remote_netmask)
+        && c->c2.push_ifconfig_remote_netmask
+        && !o->push_ifconfig_ipv4_blocked)
     {
         in_addr_t ifconfig_local = c->c2.push_ifconfig_local;
         if (c->c2.push_ifconfig_local_alias)

@@ -602,6 +603,13 @@ push_remove_option(struct options *o, const char *p)
 {
     msg(D_PUSH_DEBUG, "PUSH_REMOVE searching for: '%s'", p);
 
+    /* ifconfig is special, as not part of the push list */
+    if (streq(p, "ifconfig"))
+    {
+        o->push_ifconfig_ipv4_blocked = true;
+        return;
+    }
+
     /* ifconfig-ipv6 is special, as not part of the push list */
     if (streq( p, "ifconfig-ipv6" ))
     {

Signature · 1 line

2.7.3

Antonio Quartulli

Acked-by on v1 1/1

2018-07-02 02:12 UTC #3 lore raw

Hi,

Quoted 15 lines · Gert Doering

Push-remove (introduced in commit 970312f1850) did not handle "ifconfig" yet, as both "ifconfig" and "ifconfig-ipv6" are handled differently from all other pushed options. Since there was no valid use-case to not-push "ifconfig" (no support on the client side for running IPv6-only) this was not an issue so far - but with the recent commits to enable ipv6-only operation it can be a desirable feature.
The implementation is similar to "push-remove ifconfig-ipv6" - namely, flagging via a new context option (c->options.push_ifconfig_ipv4_blocked) and then not creating the push statement in "send_push_reply()".
While not truly elegant, it's much less invasive than the alternatives (storing the list of "push-remove" statements somewhere and then checking in push_option_ex())

Quoted 6 lines

Trac: #1072
Signed-off-by Gert Doering <[email protected]...>
---
v2: style changes, manpage note about exact match

Acked-by Antonio Quartulli <[email protected]...>

Signature · 1 line

Antonio Quartulli

Gert Doering

author

2018-07-02 06:29 UTC #4 lore raw

In reply to Gert Doering

Your patch has been applied to the master branch.

commit 6ae2f19d891e8cedccffdb1760b9774b9feff140

Author: Gert Doering
Date:   Sun Jul 1 21:59:38 2018 +0200

     Extend push-remove to also handle 'ifconfig'.

     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Antonio Quartulli <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17169.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering