[Openvpn-devel] Extend push-remove to also handle 'ifconfig'.

Message ID 20180629174529.19915-1-gert@greenie.muc.de
State Changes Requested
Headers show
Series
  • [Openvpn-devel] Extend push-remove to also handle 'ifconfig'.
Related show

Commit Message

Gert Doering June 29, 2018, 5:45 p.m.
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 <gert@greenie.muc.de>
---
 src/openvpn/options.h |  1 +
 src/openvpn/push.c    | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Antonio Quartulli June 30, 2018, 1:02 p.m. | #1
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 :)

On 30/06/18 01:45, Gert Doering wrote:
> 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 <gert@greenie.muc.de>
> ---
>  src/openvpn/options.h |  1 +
>  src/openvpn/push.c    | 10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index f7d0145..3a6c33f 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -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 */
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 6a30e47..86ec7ea 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -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).

> +        !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.

> +    {
> +        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,

Patch

diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index f7d0145..3a6c33f 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -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 */
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 6a30e47..86ec7ea 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -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" ))
     {