Message ID | 20210907223126.8440-1-selva.nair@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [Openvpn-devel,v3,1/3] Add remote-count and remote-entry query via management | expand |
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
diff --git a/Changes.rst b/Changes.rst index 637ed97a..fa5d5ffa 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`` + 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 diff --git a/doc/management-notes.txt b/doc/management-notes.txt index 84e3d04b..544caf46 100644 --- a/doc/management-notes.txt +++ b/doc/management-notes.txt @@ -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) -------------------------------------------- diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 386aee23..39dcfcef 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -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 diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index f86c87f2..f2a51d6c 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 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)) diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h index 6d6f2fb1..5de0a7da 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,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); }; /*