diff mbox series

[Openvpn-devel,1/2] Add remote-count and remote-entry query via management

Message ID 20210825040122.14244-1-selva.nair@gmail.com
State Superseded
Headers show
Series [Openvpn-devel,1/2] Add remote-count and remote-entry query via management | expand

Commit Message

Selva Nair Aug. 24, 2021, 6:01 p.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

Selecting the remote host via the management iterface
(management-query-remote) provides a restrictive user
experience as there is no easy way to tabulate all available
remote entries and show a list to the user to choose from.
Fix that.

Two new commands for querying the management interface are added:
(i) remote-entry-count : returns the number of remotes specified
    in the config file. Example result:
    >REMOTE-ENTRY-COUNT:10

(ii) remote-entry-get index : returns the remote entry at index in
     the form index,host,port,protocol. Example result for index = 2:
     >REMOTE-ENTRY:2,ovpn.example.com,1194,udp

See also management-notes.txt

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 Changes.rst              |  5 ++++
 doc/management-notes.txt | 22 +++++++++++++++++
 src/openvpn/init.c       | 36 ++++++++++++++++++++++++++++
 src/openvpn/manage.c     | 52 ++++++++++++++++++++++++++++++++++++++++
 src/openvpn/manage.h     |  3 ++-
 5 files changed, 117 insertions(+), 1 deletion(-)

Comments

Arne Schwabe Aug. 25, 2021, 4:24 a.m. UTC | #1 
>  
> +static bool
> +management_callback_remote_entry(void *arg, unsigned *count, char **remote)
> +{
> +    assert(arg);
> +    assert(count);
> +
> +    struct context *c = (struct context *) arg;
> +    struct connection_list *l = c->options.connection_list;
> +    bool ret = true;
> +
> +    if (!remote) /* query is for the count of entries */
> +    {
> +        *count = l->len;
> +    }
> +    else if (*count < l->len) /* the query is for entry with index = count */
> +    {
> +        struct connection_entry *ce = l->array[*count];
> +        const char *proto = proto2ascii(ce->proto, ce->af, false);
> +
> +        /* space for output including 2 commas and a nul */
> +        int len = strlen(ce->remote) + strlen(ce->remote_port) + strlen(proto) + 2 + 1;
> +        char *out = malloc(len);
> +        check_malloc_return(out);
> +
> +        openvpn_snprintf(out, len, "%s,%s,%s", ce->remote, ce->remote_port, proto);
> +        *remote = out;
> +    }
> +    else
> +    {
> +        ret = false;
> +        msg(M_WARN, "Invalid arguments in management query for remote entry: count = %u", *count);
> +    }
> +    return ret;
> +}
> +

I am not sure mixing two related but different functions into one
function has any real advantages. A function that just returns the count
would simplify that function and this function.
> +static void
> +man_remote_entry_count(struct management *man)
> +{
> +    unsigned count = 0;
> +    if (man->persist.callback.remote_entry)
> +    {
> +        (*man->persist.callback.remote_entry)(man->persist.callback.arg, &count, NULL);

The return value is not used here, this might upset some compilers.

> +        msg(M_CLIENT, ">REMOTE-ENTRY-COUNT:%u", count);
> +    }
> +    else
> +    {
> +        msg(M_CLIENT, "ERROR: The remote-entry-count command is not supported by the current daemon mode");
> +    }
> +}

At some point we should do a helper function that does this boilerplate
code that we copy&paste here over and over.


Arne
Selva Nair Aug. 25, 2021, 6:10 a.m. UTC | #2 
Hi

On Wed, Aug 25, 2021 at 10:24 AM Arne Schwabe <arne@rfc2549.org> wrote:

>
> >
> > +static bool
> > +management_callback_remote_entry(void *arg, unsigned *count, char
> **remote)
> > +{
> > +    assert(arg);
> > +    assert(count);
> > +
> > +    struct context *c = (struct context *) arg;
> > +    struct connection_list *l = c->options.connection_list;
> > +    bool ret = true;
> > +
> > +    if (!remote) /* query is for the count of entries */
> > +    {
> > +        *count = l->len;
> > +    }
> > +    else if (*count < l->len) /* the query is for entry with index =
> count */
> > +    {
> > +        struct connection_entry *ce = l->array[*count];
> > +        const char *proto = proto2ascii(ce->proto, ce->af, false);
> > +
> > +        /* space for output including 2 commas and a nul */
> > +        int len = strlen(ce->remote) + strlen(ce->remote_port) +
> strlen(proto) + 2 + 1;
> > +        char *out = malloc(len);
> > +        check_malloc_return(out);
> > +
> > +        openvpn_snprintf(out, len, "%s,%s,%s", ce->remote,
> ce->remote_port, proto);
> > +        *remote = out;
> > +    }
> > +    else
> > +    {
> > +        ret = false;
> > +        msg(M_WARN, "Invalid arguments in management query for remote
> entry: count = %u", *count);
> > +    }
> > +    return ret;
> > +}
> > +
>
> I am not sure mixing two related but different functions into one
> function has any real advantages. A function that just returns the count
> would simplify that function and this function.
>

Makes sense. v2 is coming.


> > +static void
> > +man_remote_entry_count(struct management *man)
> > +{
> > +    unsigned count = 0;
> > +    if (man->persist.callback.remote_entry)
> > +    {
> > +
> (*man->persist.callback.remote_entry)(man->persist.callback.arg, &count,
> NULL);
>
> The return value is not used here, this might upset some compilers.
>

The above will fix that too.


>
> > +        msg(M_CLIENT, ">REMOTE-ENTRY-COUNT:%u", count);
> > +    }
> > +    else
> > +    {
> > +        msg(M_CLIENT, "ERROR: The remote-entry-count command is not
> supported by the current daemon mode");
> > +    }
> > +}
>
> At some point we should do a helper function that does this boilerplate
> code that we copy&paste here over and over.
>

Actually this else could be possibly eliminated as, in this case, the
callback is not conditionally compiled in. Unlike things like pkcs11-id
support. Will check and simplify.

Thanks,

Selva
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Aug 25, 2021 at 10:24 AM Arne Schwabe &lt;<a href="mailto:arne@rfc2549.org">arne@rfc2549.org</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"><br>
&gt;  <br>
&gt; +static bool<br>
&gt; +management_callback_remote_entry(void *arg, unsigned *count, char **remote)<br>
&gt; +{<br>
&gt; +    assert(arg);<br>
&gt; +    assert(count);<br>
&gt; +<br>
&gt; +    struct context *c = (struct context *) arg;<br>
&gt; +    struct connection_list *l = c-&gt;options.connection_list;<br>
&gt; +    bool ret = true;<br>
&gt; +<br>
&gt; +    if (!remote) /* query is for the count of entries */<br>
&gt; +    {<br>
&gt; +        *count = l-&gt;len;<br>
&gt; +    }<br>
&gt; +    else if (*count &lt; l-&gt;len) /* the query is for entry with index = count */<br>
&gt; +    {<br>
&gt; +        struct connection_entry *ce = l-&gt;array[*count];<br>
&gt; +        const char *proto = proto2ascii(ce-&gt;proto, ce-&gt;af, false);<br>
&gt; +<br>
&gt; +        /* space for output including 2 commas and a nul */<br>
&gt; +        int len = strlen(ce-&gt;remote) + strlen(ce-&gt;remote_port) + strlen(proto) + 2 + 1;<br>
&gt; +        char *out = malloc(len);<br>
&gt; +        check_malloc_return(out);<br>
&gt; +<br>
&gt; +        openvpn_snprintf(out, len, &quot;%s,%s,%s&quot;, ce-&gt;remote, ce-&gt;remote_port, proto);<br>
&gt; +        *remote = out;<br>
&gt; +    }<br>
&gt; +    else<br>
&gt; +    {<br>
&gt; +        ret = false;<br>
&gt; +        msg(M_WARN, &quot;Invalid arguments in management query for remote entry: count = %u&quot;, *count);<br>
&gt; +    }<br>
&gt; +    return ret;<br>
&gt; +}<br>
&gt; +<br>
<br>
I am not sure mixing two related but different functions into one<br>
function has any real advantages. A function that just returns the count<br>
would simplify that function and this function.<br></blockquote><div><br></div><div>Makes sense. v2 is coming.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
&gt; +static void<br>
&gt; +man_remote_entry_count(struct management *man)<br>
&gt; +{<br>
&gt; +    unsigned count = 0;<br>
&gt; +    if (man-&gt;persist.callback.remote_entry)<br>
&gt; +    {<br>
&gt; +        (*man-&gt;persist.callback.remote_entry)(man-&gt;persist.callback.arg, &amp;count, NULL);<br>
<br>
The return value is not used here, this might upset some compilers.<br></blockquote><div><br></div><div>The above will fix that too.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
&gt; +        msg(M_CLIENT, &quot;&gt;REMOTE-ENTRY-COUNT:%u&quot;, count);<br>
&gt; +    }<br>
&gt; +    else<br>
&gt; +    {<br>
&gt; +        msg(M_CLIENT, &quot;ERROR: The remote-entry-count command is not supported by the current daemon mode&quot;);<br>
&gt; +    }<br>
&gt; +}<br>
<br>
At some point we should do a helper function that does this boilerplate<br>
code that we copy&amp;paste here over and over.<br></blockquote><div><br></div><div>Actually this else could be possibly eliminated as, in this case, the callback is not conditionally compiled in. Unlike things like pkcs11-id support. Will check and simplify.<br></div><div><br></div><div>Thanks,</div><div><br></div><div>Selva</div></div></div>
Arne Schwabe Aug. 25, 2021, 8:20 a.m. UTC | #3 
> 
> 
> Actually this else could be possibly eliminated as, in this case, the
> callback is not conditionally compiled in. Unlike things like pkcs11-id
> support. Will check and simplify.

I think in client vs server mode the management interface is still
different enough that many of these are actually needed.

Arne
diff mbox series

Patch

diff --git a/Changes.rst b/Changes.rst
index 0323a7f7..e5ac8098 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -4,6 +4,11 @@  Overview of changes in 2.6
 
 New features
 ------------
+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
+    remote entries and the entry at index respectively.
+
 Keying Material Exporters (RFC 5705) based key generation
     As part of the cipher negotiation OpenVPN will automatically prefer
     the RFC5705 based key material generation to the current custom
diff --git a/doc/management-notes.txt b/doc/management-notes.txt
index 84e3d04b..f7a0fe1f 100644
--- a/doc/management-notes.txt
+++ b/doc/management-notes.txt
@@ -897,6 +897,28 @@  the 10.0.0.0/8 netblock is allowed: 10.10.0.1.  Also, the client
 may not interact with external IP addresses using an "unknown"
 protocol (i.e. one that is not IPv4 or ARP).
 
+COMMAND -- remote-entry-count (OpenVPN 2.6+ management version > 3)
+-------------------------------------------------------------------
+
+Retrieve available number of remote host/port entries
+
+Example:
+
+     remote-entry-count
+     >REMOTE-ENTRY-COUNT:5
+
+COMMAND -- remote-entry-get  (OpenVPN 2.6+ management version > 3)
+------------------------------------------------------------------
+
+Retrieve remote entry (host, port and protocol) by index.
+
+Example:
+
+     remote-entgry-get 1
+     REMOTE-ENTRY:1,vpn.example.com,1194,udp
+
+The protocol could be tcp-client or udp on client.
+
 COMMAND -- remote  (OpenVPN AS 2.1.5/OpenVPN 2.3 or higher)
 --------------------------------------------
 
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 386aee23..3c98a408 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -330,6 +330,41 @@  management_callback_send_cc_message(void *arg,
     return status;
 }
 
+static bool
+management_callback_remote_entry(void *arg, unsigned *count, char **remote)
+{
+    assert(arg);
+    assert(count);
+
+    struct context *c = (struct context *) arg;
+    struct connection_list *l = c->options.connection_list;
+    bool ret = true;
+
+    if (!remote) /* query is for the count of entries */
+    {
+        *count = l->len;
+    }
+    else if (*count < l->len) /* the query is for entry with index = count */
+    {
+        struct connection_entry *ce = l->array[*count];
+        const char *proto = proto2ascii(ce->proto, ce->af, false);
+
+        /* space for output including 2 commas and a nul */
+        int len = strlen(ce->remote) + strlen(ce->remote_port) + strlen(proto) + 2 + 1;
+        char *out = malloc(len);
+        check_malloc_return(out);
+
+        openvpn_snprintf(out, len, "%s,%s,%s", ce->remote, ce->remote_port, proto);
+        *remote = out;
+    }
+    else
+    {
+        ret = false;
+        msg(M_WARN, "Invalid arguments in management query for remote entry: count = %u", *count);
+    }
+    return ret;
+}
+
 static bool
 management_callback_remote_cmd(void *arg, const char **p)
 {
@@ -3944,6 +3979,7 @@  init_management_callback_p2p(struct context *c)
 #ifdef TARGET_ANDROID
         cb.network_change = management_callback_network_change;
 #endif
+        cb.remote_entry = management_callback_remote_entry;
         management_set_callback(management, &cb);
     }
 #endif
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index f86c87f2..c2eb699f 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -94,6 +94,8 @@  man_help(void)
     msg(M_CLIENT, "net                    : (Windows only) Show network info and routing table.");
     msg(M_CLIENT, "password type p        : Enter password p for a queried OpenVPN password.");
     msg(M_CLIENT, "remote type [host port] : Override remote directive, type=ACCEPT|MOD|SKIP.");
+    msg(M_CLIENT, "remote-entry-count     : Get number of available remote entries.");
+    msg(M_CLIENT, "remote-entry-get index : Get remote entry at index.");
     msg(M_CLIENT, "proxy type [host port flags] : Enter dynamic proxy server info.");
     msg(M_CLIENT, "pid                    : Show process ID of the current OpenVPN process.");
 #ifdef ENABLE_PKCS11
@@ -829,6 +831,45 @@  man_pkcs11_id_get(struct management *man, const int index)
 
 #endif /* ifdef ENABLE_PKCS11 */
 
+static void
+man_remote_entry_count(struct management *man)
+{
+    unsigned count = 0;
+    if (man->persist.callback.remote_entry)
+    {
+        (*man->persist.callback.remote_entry)(man->persist.callback.arg, &count, NULL);
+        msg(M_CLIENT, ">REMOTE-ENTRY-COUNT:%u", count);
+    }
+    else
+    {
+        msg(M_CLIENT, "ERROR: The remote-entry-count command is not supported by the current daemon mode");
+    }
+}
+
+static void
+man_remote_entry_get(struct management *man, unsigned index)
+{
+    char *remote = NULL;
+
+    if (man->persist.callback.remote_entry)
+    {
+        const bool status = (*man->persist.callback.remote_entry)(man->persist.callback.arg, &index, &remote);
+        if (status)
+        {
+            msg(M_CLIENT, ">REMOTE-ENTRY:%u,%s", index, remote);
+        }
+        else
+        {
+            msg(M_CLIENT, ">REMOTE-ENTRY:%u", index);
+        }
+    }
+    else
+    {
+        msg(M_CLIENT, "ERROR: The remote-entry command is not supported by the current daemon mode");
+    }
+    free(remote);
+}
+
 static void
 man_hold(struct management *man, const char *cmd)
 {
@@ -1601,6 +1642,17 @@  man_dispatch_command(struct management *man, struct status_output *so, const cha
         }
     }
 #endif
+    else if (streq(p[0], "remote-entry-count"))
+    {
+        man_remote_entry_count(man);
+    }
+    else if (streq(p[0], "remote-entry-get"))
+    {
+        if (man_need(man, p, 1, 0))
+        {
+            man_remote_entry_get(man, atoi(p[1]));
+        }
+    }
     else if (streq(p[0], "proxy"))
     {
         if (man_need(man, p, 1, MN_AT_LEAST))
diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h
index 6d6f2fb1..b7fcb86c 100644
--- a/src/openvpn/manage.h
+++ b/src/openvpn/manage.h
@@ -31,7 +31,7 @@ 
 #include "socket.h"
 #include "mroute.h"
 
-#define MANAGEMENT_VERSION                      3
+#define MANAGEMENT_VERSION                      4
 #define MANAGEMENT_N_PASSWORD_RETRIES           3
 #define MANAGEMENT_LOG_HISTORY_INITIAL_SIZE   100
 #define MANAGEMENT_ECHO_BUFFER_SIZE           100
@@ -186,6 +186,7 @@  struct management_callback
 #ifdef TARGET_ANDROID
     int (*network_change)(void *arg, bool samenetwork);
 #endif
+    bool (*remote_entry) (void *arg, unsigned *count, char **remote);
 };
 
 /*