[Openvpn-devel,v2,2/3] Permit unlimited connection entries and remotes

Message ID 20210825210232.22509-2-selva.nair@gmail.com
State Superseded
Headers show
Series
  • [Openvpn-devel,v2,1/3] Add remote-count and remote-entry query via management
Related show

Commit Message

Selva Nair Aug. 25, 2021, 9:02 p.m.
From: Selva Nair <selva.nair@gmail.com>

Currently we allow a max of 64 connection entries and remotes.
A larger number would allow users with 100's of independent
config files for different end points of same provider to
consolidate them to connection entries.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 Changes.rst           |  2 ++
 src/openvpn/options.c | 34 ++++++++++++++++++++++++++++------
 src/openvpn/options.h |  6 ++++--
 3 files changed, 34 insertions(+), 8 deletions(-)

Comments

Selva Nair Sept. 7, 2021, 10:27 p.m. | #1
Hi,

While implementing this feature in the GUI, I have come across some
limitations of management-query-remote:

(i) Using a remote selected by the user from a list requires skipping
several remotes and accepting when the right one is reached. This is very
slow when there are more than a dozen remote entries: about ~1 second per
command.

Solution: Extend "remote SKIP" to take an optional parameter to skip "n"
entries by one command.

Use of "remote MOD" is not useful here as it does not support change of
protocol. It also has some "misbehaviour" due to the way resolved addresses
are cached.

(ii) remote-entry-get introduced here needs to be extended to support
requesting multiple or all remote entries in one command.

Please ignore the current version. v3 is coming.

Selva
<div dir="ltr"><div dir="ltr">Hi,</div><div dir="ltr"><br></div><div>While implementing this feature in the GUI, I have come across some limitations of management-query-remote:</div><div><br></div><div>(i) Using a remote selected by the user from a list requires skipping several remotes and accepting when the right one is reached. This is very slow when there are more than a dozen remote entries: about ~1 second per command.</div><div><br></div><div>Solution: Extend &quot;remote SKIP&quot; to take an optional parameter to skip &quot;n&quot; entries by one command.</div><div><br></div><div>Use of &quot;remote MOD&quot; is not useful here as it does not support change of protocol. It also has some &quot;misbehaviour&quot; due to the way resolved addresses are cached.</div><div><br></div><div>(ii) remote-entry-get introduced here needs to be extended to support requesting multiple or all remote entries in one command.</div><div><br></div><div>Please ignore the current version. v3 is coming.</div><div><br></div><div>Selva</div></div>
Selva Nair Oct. 5, 2021, 5:13 p.m. | #2
Hi,

As this patch set is not getting any attention, I guess the need for this
is not clear enough from the commit messages. To be blunt,
--management-query-remote is pretty useless the way it stands. Let me
explain:

(1) It queries the user with one remote at a time taking about 1 second per
query. With 100 remotes, if the user wants to select the 100th one, a UI
has to show 100 popups and take ~100 seconds.

So one wants two things in good UI
(i) show a list of all available remotes to the user
(ii) have a way to jump to a remote that user chooses

For (i) one could scan the config and make a list but I'll come to why that
doesn't work in a minute. Consider (ii) first: we currently have an option
to skip remotes one by one or send a MOD request where the host and port
can be changed. Skipping one by one is very slow, and MOD doesn't allow
change of protocol. So, if the 100th item that user chooses is TCP and all
other 99 are UDP, we have to wait until we get prompted with the right one.
There are other reasons why MOD doesn't work but I want to keep this mail
simple.

Thus, we want a way to skip "N" remotes by one command (say skip 99, get
prompted with the 100th which we accept). One of the three patches in this
set does that.

Now back to (i) about making a list. For "SKIP N" to work, the UI and the
core must have an identical view of the list -- especially the order of
entries. If remote-random is in use, the only way to assure that is by
making the list by querying the core. Even otherwise independently parsing
the list from the config is not the right approach when identity of content
is important.

This, hopefully, justifies the patch that allows the UI to query the core
for a list of remotes and their indices.

The third patch is to relax the restriction on the number of remote entries
and connection lists. Currently it's hard coded to 64, there is no reason
why we can't allow 100 or even 1000 remote entries.

From experience, I know that patches that languish stay like that for
months if not years. Please, either review and ACK or hate it at first
sight and NAK. Or ask for clarification. A patch for the GUI is waiting for
the fate of this.

Thanks,

Selva

On Tue, Sep 7, 2021 at 6:27 PM Selva Nair <selva.nair@gmail.com> wrote:

> Hi,
>
> While implementing this feature in the GUI, I have come across some
> limitations of management-query-remote:
>
> (i) Using a remote selected by the user from a list requires skipping
> several remotes and accepting when the right one is reached. This is very
> slow when there are more than a dozen remote entries: about ~1 second per
> command.
>
> Solution: Extend "remote SKIP" to take an optional parameter to skip "n"
> entries by one command.
>
> Use of "remote MOD" is not useful here as it does not support change of
> protocol. It also has some "misbehaviour" due to the way resolved addresses
> are cached.
>
> (ii) remote-entry-get introduced here needs to be extended to support
> requesting multiple or all remote entries in one command.
>
> Please ignore the current version. v3 is coming.
>
> Selva
>
<div dir="ltr">Hi,<div><br></div><div>As this patch set is not getting any attention, I guess the need for this is not clear enough from the commit messages. To be blunt, --management-query-remote is pretty useless the way it stands. Let me explain:</div><div><br></div><div>(1) It queries the user with one remote at a time taking about 1 second per query. With 100 remotes, if the user wants to select the 100th one, a UI has to show 100 popups and take ~100 seconds.</div><div><br></div><div>So one wants two things in good UI </div><div>(i) show a list of all available remotes to the user</div><div>(ii) have a way to jump to a remote that user chooses </div><div><br></div><div>For (i) one could scan the config and make a list but I&#39;ll come to why that doesn&#39;t work in a minute. Consider (ii) first: we currently have an option to skip remotes one by one or send a MOD request where the host and port can be changed. Skipping one by one is very slow, and MOD doesn&#39;t allow change of protocol. So, if the 100th item that user chooses is TCP and all other 99 are UDP, we have to wait until we get prompted with the right one. There are other reasons why MOD doesn&#39;t work but I want to keep this mail simple.</div><div><br></div><div>Thus, we want a way to skip &quot;N&quot; remotes by one command (say skip 99, get prompted with the 100th which we accept). One of the three patches in this set does that.</div><div><br></div><div>Now back to (i) about making a list. For &quot;SKIP N&quot; to work, the UI and the core must have an identical view of the list -- especially the order of entries. If remote-random is in use, the only way to assure that is by making the list by querying the core. Even otherwise independently parsing the list from the config is not the right approach when identity of content is important.</div><div><br></div><div>This, hopefully, justifies the patch that allows the UI to query the core for a list of remotes and their indices.</div><div><br></div><div>The third patch is to relax the restriction on the number of remote entries and connection lists. Currently it&#39;s hard coded to 64, there is no reason why we can&#39;t allow 100 or even 1000 remote entries.</div><div><br></div><div>From experience, I know that patches that languish stay like that for months if not years. Please, either review and ACK or hate it at first sight and NAK. Or ask for clarification. A patch for the GUI is waiting for the fate of this.</div><div><br></div><div>Thanks,</div><div><br>Selva</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 7, 2021 at 6:27 PM Selva Nair &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">Hi,</div><div dir="ltr"><br></div><div>While implementing this feature in the GUI, I have come across some limitations of management-query-remote:</div><div><br></div><div>(i) Using a remote selected by the user from a list requires skipping several remotes and accepting when the right one is reached. This is very slow when there are more than a dozen remote entries: about ~1 second per command.</div><div><br></div><div>Solution: Extend &quot;remote SKIP&quot; to take an optional parameter to skip &quot;n&quot; entries by one command.</div><div><br></div><div>Use of &quot;remote MOD&quot; is not useful here as it does not support change of protocol. It also has some &quot;misbehaviour&quot; due to the way resolved addresses are cached.</div><div><br></div><div>(ii) remote-entry-get introduced here needs to be extended to support requesting multiple or all remote entries in one command.</div><div><br></div><div>Please ignore the current version. v3 is coming.</div><div><br></div><div>Selva</div></div>
</blockquote></div>

Patch

diff --git a/Changes.rst b/Changes.rst
index e5ac8098..6e3c535e 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -4,6 +4,8 @@  Overview of changes in 2.6
 
 New features
 ------------
+Support unlimited number of connection entries and remote entries
+
 New management commands to enumerate and list remote entries
     Use ``remote-entry-count`` and ``remote-entry-get index``
     commands from the management interface to get the number of
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 0e398c0f..5d0aa8af 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -889,6 +889,14 @@  init_options(struct options *o, const bool init_gc)
 void
 uninit_options(struct options *o)
 {
+    if (o->connection_list) {
+        free(o->connection_list->array);
+        CLEAR(*o->connection_list);
+    }
+    if (o->remote_list) {
+        free(o->remote_list->array);
+        CLEAR(*o->remote_list);
+    }
     if (o->gc_owned)
     {
         gc_free(&o->gc);
@@ -1947,10 +1955,17 @@  alloc_connection_entry(struct options *options, const int msglevel)
     struct connection_list *l = alloc_connection_list_if_undef(options);
     struct connection_entry *e;
 
-    if (l->len >= CONNECTION_LIST_SIZE)
+    if (l->len == l->capacity)
     {
-        msg(msglevel, "Maximum number of 'connection' options (%d) exceeded", CONNECTION_LIST_SIZE);
-        return NULL;
+        int capacity = l->capacity + CONNECTION_LIST_SIZE;
+        struct connection_entry **ce = realloc(l->array, capacity*sizeof(struct connection_entry *));
+        if (ce == NULL)
+        {
+            msg(msglevel, "Unable to process more connection options: out of memory. Number of entries = %d", l->len);
+            return NULL;
+        }
+        l->array = ce;
+        l->capacity = capacity;
     }
     ALLOC_OBJ_GC(e, struct connection_entry, &options->gc);
     l->array[l->len++] = e;
@@ -1973,10 +1988,17 @@  alloc_remote_entry(struct options *options, const int msglevel)
     struct remote_list *l = alloc_remote_list_if_undef(options);
     struct remote_entry *e;
 
-    if (l->len >= CONNECTION_LIST_SIZE)
+    if (l->len == l->capacity)
     {
-        msg(msglevel, "Maximum number of 'remote' options (%d) exceeded", CONNECTION_LIST_SIZE);
-        return NULL;
+        int capacity = l->capacity + CONNECTION_LIST_SIZE;
+        struct remote_entry **re = realloc(l->array, capacity*sizeof(struct remote_entry *));
+        if (re == NULL)
+        {
+            msg(msglevel, "Unable to process more remote options: out of memory. Number of entries = %d", l->len);
+            return NULL;
+        }
+        l->array = re;
+        l->capacity = capacity;
     }
     ALLOC_OBJ_GC(e, struct remote_entry, &options->gc);
     l->array[l->len++] = e;
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index b0e40cb7..98977d41 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -168,15 +168,17 @@  struct remote_entry
 
 struct connection_list
 {
+    int capacity;
     int len;
     int current;
-    struct connection_entry *array[CONNECTION_LIST_SIZE];
+    struct connection_entry **array;
 };
 
 struct remote_list
 {
+    int capacity;
     int len;
-    struct remote_entry *array[CONNECTION_LIST_SIZE];
+    struct remote_entry **array;
 };
 
 enum vlan_acceptable_frames