[Openvpn-devel,v3,1/3] Add remote-count and remote-entry query via management
Commit Message
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:
10
END
(ii) remote-entry-get i [j]: returns the remote entry at index i
in the form index,host,port,protocol. Or, if j is present
all entries from index i to j-1 are returned, one per line.
Example result for i = 2:
2,ovpn.example.com,1194,udp
END
Example result for i = 2, j = 4
2,ovpn.example.com,1194,udp
3,ovpn.example.com,443,tcp-client
END
remote-entry-get all: returns all remote entries.
v2: use independent callback functions for the two commands
v3: return results as 0 or more lines terminated by END, as done
for all other similar commands. v1 was fashioned after
pkcs11-id-count and pkcs11-id-get which uses a format not
consistent with the rest of the management commands.
See also management-notes.txt
Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
Changes.rst | 5 +++
doc/management-notes.txt | 60 ++++++++++++++++++++++++++++++++++
src/openvpn/init.c | 44 +++++++++++++++++++++++++
src/openvpn/manage.c | 70 ++++++++++++++++++++++++++++++++++++++++
src/openvpn/manage.h | 4 ++-
5 files changed, 182 insertions(+), 1 deletion(-)
Comments
Am 08.09.21 um 00:31 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@gmail.com>
>
> Selecting the remote host via the management iterface
*sigh* I reviewed the v2 instead of the v3 but the typo is still here :P
> +static void
> +man_remote_entry_count(struct management *man)
> +{
> + unsigned count = 0;
> + if (man->persist.callback.remote_entry_count)
> + {
> + count = (*man->persist.callback.remote_entry_count)(man->persist.callback.arg);
We could use C99 style here.
> + msg(M_CLIENT, "%u", count);
> + msg(M_CLIENT, "END");
> + }
> + else
> + {
> + msg(M_CLIENT, "ERROR: The remote-entry-count command is not supported by the current daemon mode");
> + }
> +}
> +
> +#define min(a,b) ((a) < (b) ? (a) : (b))
We already have min_int and min_uint in integer.h. For consistency I
would prefer using those instead of adding a macro.
> +
> +static void
> +man_remote_entry_get(struct management *man, const char *p1, const char *p2)
> +{
> + ASSERT(p1);
> +
> + if (man->persist.callback.remote_entry_get
> + && man->persist.callback.remote_entry_count)
> + {
> + bool res;
> + unsigned int from, to;
> + unsigned int count = (*man->persist.callback.remote_entry_count)(man->persist.callback.arg);
> +
> + from = (unsigned int) atoi(p1);
> + to = p2? (unsigned int) atoi(p2) : from + 1;
> +
We probably want a space after p2.
I know this has been very long and we are very close to 2.6 so
Acked-By: Arne Schwabe <arne@rfc2549.org>
Maybe we can do a follow up patch to fix the mentioned small issues.
Hi
On Mon, Dec 26, 2022 at 2:45 PM Arne Schwabe <arne@rfc2549.org> wrote:
> Am 08.09.21 um 00:31 schrieb selva.nair@gmail.com:
> > From: Selva Nair <selva.nair@gmail.com>
> >
> > Selecting the remote host via the management iterface
>
> *sigh* I reviewed the v2 instead of the v3 but the typo is still here :P
>
> > +static void
> > +man_remote_entry_count(struct management *man)
> > +{
> > + unsigned count = 0;
> > + if (man->persist.callback.remote_entry_count)
> > + {
> > + count =
> (*man->persist.callback.remote_entry_count)(man->persist.callback.arg);
>
> We could use C99 style here.
>
> > + msg(M_CLIENT, "%u", count);
> > + msg(M_CLIENT, "END");
> > + }
> > + else
> > + {
> > + msg(M_CLIENT, "ERROR: The remote-entry-count command is not
> supported by the current daemon mode");
> > + }
> > +}
> > +
> > +#define min(a,b) ((a) < (b) ? (a) : (b))
>
> We already have min_int and min_uint in integer.h. For consistency I
> would prefer using those instead of adding a macro.
>
>
> > +
> > +static void
> > +man_remote_entry_get(struct management *man, const char *p1, const char
> *p2)
> > +{
> > + ASSERT(p1);
> > +
> > + if (man->persist.callback.remote_entry_get
> > + && man->persist.callback.remote_entry_count)
> > + {
> > + bool res;
> > + unsigned int from, to;
> > + unsigned int count =
> (*man->persist.callback.remote_entry_count)(man->persist.callback.arg);
> > +
> > + from = (unsigned int) atoi(p1);
> > + to = p2? (unsigned int) atoi(p2) : from + 1;
> > +
>
> We probably want a space after p2.
>
> I know this has been very long and we are very close to 2.6 so
>
> Acked-By: Arne Schwabe <arne@rfc2549.org>
>
Thanks..
>
> Maybe we can do a follow up patch to fix the mentioned small issues.
>
Will do as a followup or in the rebased pachc depending what Gert asks for.
Would it be too much to ask for a review of this one too ?
https://patchwork.openvpn.net/project/openvpn2/patch/20210907223614.8574-1-selva.nair@gmail.com/
Selva
I had conflicts in doc/management-notes.txt (the context leading to
this in the patch is way different, and not in my tree?!), and uncrustify
had some whitespace complaints - fixed the conflict, and the complaints.
Stare-at-code looks reasonable ("and it has an ACK from Arne").
I have tested this (with a small .ovpn with only a handful of remotes in),
and it seems to do what it says on the lid.
One thing that confuses me slightly - either I am holding this wrong, or
I don't understand how this is supposed to work...
- I run the client with --management-hold
- connect to the management interface
- try remote-entry-count ...
>INFO:OpenVPN Management Interface Version 4 -- type 'help' for more info
>HOLD:Waiting for hold release:0
remote-entry-count
ERROR: The remote-entry-count command is not supported by the current daemon mode
- do a hold release (openvpn connects to the first remote now), then try
again...
hold release
SUCCESS: hold release succeeded
remote-entry-count
5
END
- looking at the code confirms that cb.remote_entry_count + *get is
only initialized (init_management_callback_p2p()) when OpenVPN starts,
well, initializing the actual P2P instance - so the code does what it
is told, but I wonder if this feature should not help the GUI select
one of the configs *before* OpenVPN starts connecting to the first
remote?
Your patch has been applied to the master and release/2.6 branch.
commit 125263804701f9e62a5a27587e4ea6afdb21f54d ( master)
commit 5503b45e6666cc7a3f10dd1e6fb406ca487489bd (HEAD -> release/2.6)
Author: Selva Nair
Date: Tue Sep 7 18:31:24 2021 -0400
Add remote-count and remote-entry query via management
Signed-off-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20210907223126.8440-1-selva.nair@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22815.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Hi,
On Mon, Dec 26, 2022 at 05:30:23PM -0500, Selva Nair wrote:
> > > +
> > > +#define min(a,b) ((a) < (b) ? (a) : (b))
> >
> > We already have min_int and min_uint in integer.h. For consistency I
> > would prefer using those instead of adding a macro.
Unfortunately, this is more than "just an extra macro" - it breaks our
MSVC builds due to the windows equivalent of -Werr and macro
redefinition...
https://github.com/OpenVPN/openvpn/actions/runs/3786194652/jobs/6436868937
D:\a\openvpn\openvpn\src\openvpn\manage.c(862,1): warning C4005: 'min': macro redefinition [D:\a\openvpn\openvpn\src\openvpn\openvpn.vcxproj]
D:\a\openvpn\openvpn\src\openvpn\manage.c(862,1): error C2220: the following warning is treated as an error [D:\a\openvpn\openvpn\src\openvpn\openvpn.vcxproj]
(not sure why it calls out "the following warning" when it talks about the
one before it? but anyway)
this came a bit unexpected, so it's been pushed out already...
gert
Hi,
On Mon, Dec 26, 2022 at 05:30:23PM -0500, Selva Nair wrote:
> > > > +
> > > > +#define min(a,b) ((a) < (b) ? (a) : (b))
> > >
> > > We already have min_int and min_uint in integer.h. For consistency I
> > > would prefer using those instead of adding a macro.
Unfortunately, this is more than "just an extra macro" - it breaks our
> MSVC builds due to the windows equivalent of -Werr and macro
> redefinition...
https://github.com/OpenVPN/openvpn/actions/runs/3786194652/jobs/6436868937
Sorry about that -- I did not check MSVC build.
Looks like a fix is already in. Thanks Arne.
One thing that confuses me slightly - either I am holding this wrong, or
> I don't understand how this is supposed to work...
>
> - I run the client with --management-hold
> - connect to the management interface
> - try remote-entry-count ...
>
> >INFO:OpenVPN Management Interface Version 4 -- type 'help' for more
> info
> >HOLD:Waiting for hold release:0
> remote-entry-count
> ERROR: The remote-entry-count command is not supported by the current
> daemon mode
>
> - do a hold release (openvpn connects to the first remote now), then try
> again...
>
> hold release
> SUCCESS: hold release succeeded
> remote-entry-count
> 5
> END
>
My test implementation in the GUI constructs the list when first >REMOTE
query is received. If there is no >REMOTE from the daemon, the feature is
not used. That works.
That said, making these commands work from management-hold would have been
nice, but looked harder to implement -- we need a callback to get the
context in management.c, but callbacks are initialized late.
Selva
Hi,
On Tue, Dec 27, 2022 at 01:18:02PM -0500, Selva Nair wrote:
> My test implementation in the GUI constructs the list when first >REMOTE
> query is received. If there is no >REMOTE from the daemon, the feature is
> not used. That works.
So what's the magic --option to make openvpn query for >REMOTE?
(My understanding of the management interface details is limited... I just
happen to stumble across things here and there :-) )
> That said, making these commands work from management-hold would have been
> nice, but looked harder to implement -- we need a callback to get the
> context in management.c, but callbacks are initialized late.
Yes, definitely looked like it. But if OpenVPN queries for >REMOTE
voluntarily ("if more than one option exists"), that sounds good enough.
gert
On Tue, Dec 27, 2022 at 1:25 PM Gert Doering <gert@greenie.muc.de> wrote:
> Hi,
>
> On Tue, Dec 27, 2022 at 01:18:02PM -0500, Selva Nair wrote:
> > My test implementation in the GUI constructs the list when first >REMOTE
> > query is received. If there is no >REMOTE from the daemon, the feature is
> > not used. That works.
>
> So what's the magic --option to make openvpn query for >REMOTE?
>
--management-query-remote
> (My understanding of the management interface details is limited... I just
> happen to stumble across things here and there :-) )
>
> > That said, making these commands work from management-hold would have
> been
> > nice, but looked harder to implement -- we need a callback to get the
> > context in management.c, but callbacks are initialized late.
>
> Yes, definitely looked like it. But if OpenVPN queries for >REMOTE
> voluntarily ("if more than one option exists"), that sounds good enough.
>
Yet undecided whether we should include this option from the GUI, or
require the user to add this to the config. Unlike
management-query-passwords adding this option always has the side effect of
querying for remote. That may annoy/burden some users who want to leave it
to the daemon via remote-random or use the first that works or whatever.
Or, we could always add this option, and just accept the first prompt if
the user does not want this feature.
Selva
@@ -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``
+ commands from the management interface to get the number of
+ remote entries and the entries themselves.
+
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
@@ -897,6 +897,66 @@ 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:
+
+ Management interface client sends:
+
+ remote-entry-count
+
+ OpenVPN daemon responds with
+
+ 5
+ END
+
+COMMAND -- remote-entry-get (OpenVPN 2.6+ management version > 3)
+------------------------------------------------------------------
+
+ remote-entry-get <start> [<end>]
+
+Retrieve remote entry (host, port and protocol) for index
+<start> or indices from <start> to <end>+1. Alternatively
+<start> = "all" retrieves all remote entries.
+
+Example 1:
+
+ Management interface client sends:
+
+ remote-entry-get 1
+
+ OpenVPN daemon responds with
+
+ 1,vpn.example.com,1194,udp
+ END
+
+Example 2:
+
+ Management interface client sends:
+
+ remote-entry-get 1 3
+
+ OpenVPN daemon responds with
+
+ 1,vpn.example.com,1194,udp
+ 2,vpn.example.net,443,tcp-client
+ END
+
+Example 3:
+ Management interface client sends:
+
+ remote-entry-get all
+
+ OpenVPN daemon with 3 connection entries responds with
+
+ 1,vpn.example.com,1194,udp
+ 2,vpn.example.com,443,tcp-client
+ 3,vpn.example.net,443,udp
+ END
+
COMMAND -- remote (OpenVPN AS 2.1.5/OpenVPN 2.3 or higher)
--------------------------------------------
@@ -330,6 +330,48 @@ management_callback_send_cc_message(void *arg,
return status;
}
+static unsigned int
+management_callback_remote_entry_count(void *arg)
+{
+ assert(arg);
+ struct context *c = (struct context *) arg;
+ struct connection_list *l = c->options.connection_list;
+
+ return l->len;
+}
+
+static bool
+management_callback_remote_entry_get(void *arg, unsigned int index, char **remote)
+{
+ assert(arg);
+ assert(remote);
+
+ struct context *c = (struct context *) arg;
+ struct connection_list *l = c->options.connection_list;
+ bool ret = true;
+
+ if (index < l->len)
+ {
+ struct connection_entry *ce = l->array[index];
+ 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, "Out of bounds index in management query for remote entry: index = %u", index);
+ }
+
+ return ret;
+}
+
static bool
management_callback_remote_cmd(void *arg, const char **p)
{
@@ -3944,6 +3986,8 @@ init_management_callback_p2p(struct context *c)
#ifdef TARGET_ANDROID
cb.network_change = management_callback_network_change;
#endif
+ cb.remote_entry_count = management_callback_remote_entry_count;
+ cb.remote_entry_get = management_callback_remote_entry_get;
management_set_callback(management, &cb);
}
#endif
@@ -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 i|all [j]: Get remote entry at index = i to to j-1 or all.");
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,63 @@ 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_count)
+ {
+ count = (*man->persist.callback.remote_entry_count)(man->persist.callback.arg);
+ msg(M_CLIENT, "%u", count);
+ msg(M_CLIENT, "END");
+ }
+ else
+ {
+ msg(M_CLIENT, "ERROR: The remote-entry-count command is not supported by the current daemon mode");
+ }
+}
+
+#define min(a,b) ((a) < (b) ? (a) : (b))
+
+static void
+man_remote_entry_get(struct management *man, const char *p1, const char *p2)
+{
+ ASSERT(p1);
+
+ if (man->persist.callback.remote_entry_get
+ && man->persist.callback.remote_entry_count)
+ {
+ bool res;
+ unsigned int from, to;
+ unsigned int count = (*man->persist.callback.remote_entry_count)(man->persist.callback.arg);
+
+ from = (unsigned int) atoi(p1);
+ to = p2? (unsigned int) atoi(p2) : from + 1;
+
+ if (!strcmp(p1, "all"))
+ {
+ from = 0;
+ to = count;
+ }
+
+ for (unsigned int i = from; i < min(to, count); i++)
+ {
+ char *remote = NULL;
+ res = (*man->persist.callback.remote_entry_get)(man->persist.callback.arg, i, &remote);
+ if (res && remote)
+ {
+ msg(M_CLIENT, "%u,%s", i, remote);
+ }
+ free(remote);
+ }
+ msg(M_CLIENT, "END");
+ }
+ else
+ {
+ msg(M_CLIENT, "ERROR: The remote-entry command is not supported by the current daemon mode");
+ }
+}
+
static void
man_hold(struct management *man, const char *cmd)
{
@@ -1601,6 +1660,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, MN_AT_LEAST))
+ {
+ man_remote_entry_get(man, p[1], p[2]);
+ }
+ }
else if (streq(p[0], "proxy"))
{
if (man_need(man, p, 1, MN_AT_LEAST))
@@ -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,8 @@ struct management_callback
#ifdef TARGET_ANDROID
int (*network_change)(void *arg, bool samenetwork);
#endif
+ unsigned int (*remote_entry_count)(void *arg);
+ bool (*remote_entry_get)(void *arg, unsigned int index, char **remote);
};
/*