Add an option to filter options received from server

Usage: --pull-filter accept|reject "option string"

Permit a client to selectively accept or reject options pushed by the server. May be used multiple times. The filters are applied in the order specified to each pushed option received. The filtering stops as soon as a match is found. Full regex support is not available but partial matching is used to allow some flexibility. Thus

pull-filter accept "ifconfig 10.9.0."
pull-filter reject "ifconfig "
pull-filter accept "route 10."
pull-filter reject "route "

will reject assigned ip unless its in the "10.9.0.0/24" range and all pushed routes except those starting with "10.". Note the space at the end of "route " to not reject "route-gateway", for example.

There is an implicit 'pull-filter accept ""' at the end so that all options not rejected by any filter are accepted.

A maximum of 64 filters are supported.

Acknowledges shameless imitation of --push-remove. Inspired by Trac #682.

---
 Changes.rst           |    4 ++
 doc/openvpn.8         |   34 +++++++++++++++++
 src/openvpn/options.c |   98 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/options.h |   19 ++++++++++
 4 files changed, 155 insertions(+)

index a6bb2a5..1ac3c2b 100644
@@ -5,6 +5,10 @@ Version 2.4.0
 New features
 ------------
 
+pull-filter
+    New option to explicitly allow or reject options pushed by the server.
+    May be used multiple times and is applied in the order specified.
+
 push-remove
     new option to remove options on a per-client basis from the "push" list
     (more fine-grained than "push-reset")

index e1dd7cd..aec37da 100644
@@ -3830,6 +3830,40 @@ in situations where you don't trust the server to have control
 over the client's routing table.
 .\"*********************************************************
 .TP
+.B \-\-pull\-filter accept|reject opt
+Filter options matching
+.B opt
+pushed by the server:
+.B accept
+allows the option,
+.B reject
+removes it. May be specified multiple times, and each filter is
+applied in the order it is specified. The filtering of each pushed
+option stops as soon as a match (accept or reject) is found.
+
+Partial matching is used so that
+.B \-\-pull\-filter reject route
+would remove all pushed options starting with
+.B route
+which would include, for example,
+.B route\-gateway.
+Enclose the option string in quotes to embed spaces.
+
+.nf
+.ft 3
+.in +4
+\-\-pull\-filter accept "route 192.168.1."
+\-\-pull\-filter reject "route "
+.in -4
+.ft
+.fi
+
+would reject all pushed routes that do not start with
+.B 192.168.1.
+
+A maximum of 64 filters are supported. May be used only on clients.
+.\"*********************************************************
+.TP
 .B \-\-auth\-user\-pass [up]
 Authenticate with server using username/password.
 .B up

index 2fa375f..b73bafc 100644
@@ -163,6 +163,23 @@ struct remote_host_store
   char port[RH_PORT_LEN];
 };
 
+#define PULL_FILTER_LIST_SIZE 64
+struct pull_filter
+{
+# define PUF_TYPE_UNDEF 0
+# define PUF_TYPE_ACCEPT 1
+# define PUF_TYPE_REJECT 2
+  int type;
+  int size;
+  char *filter;
+};
+
+struct pull_filter_list
+{
+  int len;
+  struct pull_filter *array[PULL_FILTER_LIST_SIZE];
+};
+
 /* Command line options */
 struct options
 {

@@ -597,6 +614,8 @@ struct options
   const char *keying_material_exporter_label;
   int keying_material_exporter_length;
 #endif
+
+  struct pull_filter_list *pull_filter_list;
 };
 
 #define streq(x, y) (!strcmp((x), (y)))

1.7.10.4

Am 31.05.16 um 06:22 schrieb Selva Nair:

We already have dymanic lists, used for ignore-unknown-options and --route, any reason not to use them here?

Arne

Hi,

I just thought 64 is good enough and kept it simple like the connection_list.

Will consider making it dynamic if we go for a v2.

Thanks,

Selva

I think a fixed list of 64 slots is more than good enough in this case. I even considered suggesting 32 slots, but 64 is reasonable as a maximum currently. An average openvpn config is probably roughly 10-15 lines all in all (not considering inline files). If someone pushes more than 50-60 options, then something more funky is probably going on as well.

*If* this limit gets too annoying, then I we can reconsider using a dynamic list.

--
kind regards,

David Sommerseth

This being filters its not really determined by how many options are being pushed..

That said, agreed 64 may be plenty to filter out almost anything. The paranoid can explicitly specify everything they want to keep as "accept" and end with a 'pull-filter reject ""' to reject everything else.

Selva

Hi,

This is not to be *pushed* but local sanity control for pushed stuff :-)

ValdikSS is pushing well over 1000 routes, so "60" is really not much in that regard...

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Clumsy wording on my side. Yeah, I understand it is a local filter. And that one filter may filter out far more than just a single pushed option.

My point was more that if more than 50-60 filters are needed, then you have a bigger challenge than just these filters.

kind regards,

David Sommerseth

Hi,

Thinking about this a bit more, without having studied the code - what do you think about having a third category

pull-filter accept "route 10."
pull-filter reject "route 195.30."
pull-filter ignore "route "

"ignore" would be "if that option shows up, log the fact that we ignored it, and go on", while "reject" would be "if this shows up, something bad is happening, so complain loudly, abort the connection and try the next alternative".

We briefly discussed this yesterday - 64 should be plenty for about everything, but a dynamic list might be the way to cope for the *one* user who then comes up with the 65th rule...

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

Interesting. At the very least it may be worth doing a v2 to make adding new action types easier.

Suppose we do make "reject" to not just remove the option but reject the server. What to do if no alternative remotes exist --- just SIGHUP and hope that next try will succeed or something like management-hold will keep the connection waiting for user intervention? I would avoid issuing or causing a SIGTERM.

I didn't make it a linked-list mainly because of the need of adding to the tail of the list which means some extra extra book-keeping and if-elses.

For now the 65th filter will trigger an early exit at the config parsing stage with a message.

Selva

Hi,

[..]

Do you foresee more different action types? My imagination ends at "accept, ignore, refuse-with-error"...

I have a use case where I would like to silently *ignore* something one of my servers is sending me (one route that is sent conflicts with local stuff, but that is not malicious or neglicience on their end, but something on my end). So v1 would do that for me, changing it to "you can only have accept ore reject-the-server" would make this option not work for me (right now I use route-nopull and manually set route statements).

But anyway - I think SIGHUP is good enough. If there is no other server, it will loop, like in any other uncorrectable error (like, server cert invalid), but there is not much we can do about it.

Was more thinking about realloc()ing the array, not linked lists - but let's keep it static for now, and just tell people "your config is too big".

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

I'm holding back on version 2 of patch due to an issue seen while testing the proposed change (accept, ignore or reject with error by triggering a restart) -- I'm using SIGUSR1 restart so that the next remote gest tried.

Testing shows that during such a failure-restart cycle caused by the reject, a SIGTERM received could be lost sometimes depending on the timing of the signal. This is particularly bad for the windows-GUI which uses the exit-event for SIGTERM --- when missed it leaves openvpn.exe process running after the GUI exits causing all kinds of issues.

The problem appears to be present only when explicit-exit-notify is used so it seems this is what happens:

SIGTERM triggers exit-notify and initializes the struct c->c2.explicit_exit_notification_interval with params that controls the timer. Any SIGUSR1 or SIGHUP after this can break the loop in
tunnel_point_to_point() by any of the P2P_CHECK_SIG()s. Then the instance
gets restarted. At the re-entry to tunnel_point_to_point() we have

context_clear_2 (c);

which clears the previously set elements of the c->c2.explicit_exit_notification_interval struct and the SIGTERM is lost.

Now, I recall a discussion in the past about SIGTERM/SIGINT being ignored during the resolve-failure -- how was that resolved?

Selva

Hi,

Version 2 of the patch is in the next email.

There is also an accompanying patch that addresses the restart/SIGTERM issue raised in my previous email and Trac 687. I'm posting itinto this thread as the first would benefit from the second.

That said, I.m not sure this is the best/correct way to handle the "lost SIGTERM" problem.

Selva

v2 changes:
- Add the flag "ignore" and have "reject" trigger a restart.
- Unlimited number of filters: yes, going against the consensus, but the code looks simpler and cleaner this way.
- New commit message to reflect the changes.

Usage: --pull-filter accept|ignore|reject "option"

Permit a client to selectively accept, ignore or reject options pushed by the server. May be used multiple times. The filters are applied in the order specified to each pushed option received. The filtering stops as soon as a match is found. The action "ignore" removes the option and continues processing the next option, while "reject" flags an error and restarts the connection with SIGUSR1.

Prefix matching is used so that all options starting with the specified "option" string are filtered.

Example:

pull-filter accept "route 192.168."
pull-filter ignore "route "
pull-filter accept "ifconfig 10.9.0."
pull-filter reject "ifconfig "

will ignore all pushed routes except those starting with "192.168." and reject the assigned ip unless its in the "10.9.0.0/24" range. A match of the reject filter will trigger a restart. SIGUSR1 restart is used instead of SIGHUP so as to try the next remote for reconnection.

Note the space at the end of "route " to not reject "route-gateway", for example. All options not matched by any filter are accepted.

Acknowledges shameless imitation of --push-remove. Inspired by Trac #682.

---
 Changes.rst           |    4 ++
 doc/openvpn.8         |   51 +++++++++++++++++
 src/openvpn/options.c |  146 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/options.h |    2 +
 4 files changed, 203 insertions(+)

index a6bb2a5..1ac3c2b 100644
@@ -5,6 +5,10 @@ Version 2.4.0
 New features
 ------------
 
+pull-filter
+    New option to explicitly allow or reject options pushed by the server.
+    May be used multiple times and is applied in the order specified.
+
 push-remove
     new option to remove options on a per-client basis from the "push" list
     (more fine-grained than "push-reset")

index 2fa375f..514511b 100644
@@ -597,6 +597,8 @@ struct options
   const char *keying_material_exporter_label;
   int keying_material_exporter_length;
 #endif
+
+  struct pull_filter_list *pull_filter_list;
 };
 
 #define streq(x, y) (!strcmp((x), (y)))

1.7.10.4

When exit-notify is specified, SIGTERM could be lost if a SIGUSR1/SIGHUP restart happens during the exit notification interval. This is particularly apparent when pull-filter reject causes a repeated SIGUSR1 restart cycle.

- Save the explicit_exit_notification_interval and time_wait across restarts.
- Check pending SIGTERM (with exit notification) in the SIGUSR1/SIGHUP loop and cause an exit instead of restart.

Trac #687

---
 src/openvpn/init.c    |   16 ++++++++++++++++
 src/openvpn/openvpn.c |    8 ++++++++
 2 files changed, 24 insertions(+)

index 42baf97..5a69a13 100644
@@ -76,7 +76,23 @@ context_clear_1 (struct context *c)
 void
 context_clear_2 (struct context *c)
 {
+#ifdef ENABLE_OCC
+  /* If we are in the middle of an exit notification, save the timer info */
+  struct event_timeout saved_interval = {0};
+  time_t saved_time_wait = 0;
+  if (event_timeout_defined(&c->c2.explicit_exit_notification_interval))
+    {
+      saved_interval = c->c2.explicit_exit_notification_interval;
+      saved_time_wait = c->c2.explicit_exit_notification_time_wait;
+    }
+#endif
+
   CLEAR (c->c2);
+
+#ifdef ENABLE_OCC
+  c->c2.explicit_exit_notification_interval = saved_interval;
+  c->c2.explicit_exit_notification_time_wait = saved_time_wait;
+#endif
 }
 
 void

index 823c3dd..6beb224 100644
@@ -281,6 +281,14 @@ openvpn_main (int argc, char *argv[])
 	      /* indicates first iteration -- has program-wide scope */
 	      c.first_time = false;
 
+# ifdef ENABLE_OCC
+              /* If we are in the middle of exit notification, exit now */
+              if (event_timeout_defined(&c.c2.explicit_exit_notification_interval))
+                {
+                  register_signal (&c, SIGTERM, "exit-with-notification");
+                }
+# endif
+
 	      /* any signals received? */
 	      if (IS_SIG (&c))
 		print_signal (c.sig, NULL, M_INFO);

1.7.10.4

Hi,

NAK on this one, as I think the later patch you sent for this is "better" and easier to understand. Just for the sake of the archives.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

ACK. Stared at the code, tested, works.

I like the way using a linked list actually made this code more simple (which hints that the previous version might not have been ideal, but anyway).

I'm not sure I agree with the style used in apply_pull_filter() - I might have coded it as "first do the strncmp(), and if that matches, use a switch/case block for the rest" (avoid repeating the strnmcp(), and it's actually minimally more effective as the current code always tests the filter type *and* strncmp()'s, while it could skip the filter type on no match) - but that is going very much into personal coding style land.

Your patch has been applied to the master branch.

commit 7f74c27e105a365d278181d00708c55a299398a0

Author: Selva Nair
Date:   Sun Jun 5 17:41:23 2016 -0400

     Add an option to filter options received from server

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11808
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering