Persist management-query-remote and proxy prompts

From: Selva Nair <[email protected]...>

Currently this prompt is only output once, not re-written to the management interface when the management client connects. It is thus not seen by a client that connects after the prompt is output or one that disconnects and reconnects. This leads to a deadlock: the daemon waiting for the "remote" command from the client, the latter not aware of it.

Resolve by adding the ">REMOTE" and ">PROXY" prompt to man.persist.special_state_msg as done for other persisted prompts such as ">PASSWORD"

---
This is a bug fix so could go into 2.4 and master.

 src/openvpn/init.c | 4 ++++
 1 file changed, 4 insertions(+)

index 36c1a4c..413563a 100644
@@ -269,6 +269,7 @@ ce_management_query_proxy(struct context *c)
             buf_printf(&out, ">PROXY:%u,%s,%s", (l ? l->current : 0) + 1,
                        (proto_is_udp(ce->proto) ? "UDP" : "TCP"), np(ce->remote));
             management_notify_generic(management, BSTR(&out));
+            management->persist.special_state_msg = BSTR(&out);
         }
         ce->flags |= CE_MAN_QUERY_PROXY;
         while (ce->flags & CE_MAN_QUERY_PROXY)

@@ -280,6 +281,7 @@ ce_management_query_proxy(struct context *c)
                 break;
             }
         }
+        management->persist.special_state_msg = NULL;
         gc_free(&gc);
     }
 

@@ -349,6 +351,7 @@ ce_management_query_remote(struct context *c)
         buf_printf(&out, ">REMOTE:%s,%s,%s", np(ce->remote), ce->remote_port,
                    proto2ascii(ce->proto, ce->af, false));
         management_notify_generic(management, BSTR(&out));
+        management->persist.special_state_msg = BSTR(&out);
 
         ce->flags &= ~(CE_MAN_QUERY_REMOTE_MASK << CE_MAN_QUERY_REMOTE_SHIFT);
         ce->flags |= (CE_MAN_QUERY_REMOTE_QUERY << CE_MAN_QUERY_REMOTE_SHIFT);

@@ -362,6 +365,7 @@ ce_management_query_remote(struct context *c)
                 break;
             }
         }
+        management->persist.special_state_msg = NULL;
     }
     gc_free(&gc);
 

2.6.2

From: Selva Nair <[email protected]...>

(i) Let the management-client predictably cycle through remote entries. This is done by not aborting after two cycles. The client can abort or restart the connection using signals (USR/HUP/TERM) as necessary.

In the current behaviour, the daemon can unexpectedly exit when the last remote is skipped. When management-query-remote is not in use, the behaviour is unchanged.

(ii) Do not count skipping a remote as an unsuccessful connection attempt. As the latter count is used for backoff it should count only failed attempts.

---
 src/openvpn/init.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

index 413563a..c63dc67 100644
@@ -468,14 +468,6 @@ next_connection_entry(struct context *c)
                         c->c1.link_socket_addr.remote_list;
                 }
 
-                /*
-                 * Increase the number of connection attempts
-                 * If this is connect-retry-max * size(l)
-                 * OpenVPN will quit
-                 */
-
-                c->options.unsuccessful_attempts++;
-
                 if (++l->current >= l->len)
                 {
 

@@ -501,6 +493,9 @@ next_connection_entry(struct context *c)
         {
             /* allow management interface to override connection entry details */
             ce_defined = ce_management_query_remote(c);
+
+            /* ignore cycles when management-query-remote is in use */
+            n_cycles = 0;
             if (IS_SIG(c))
             {
                 break;

@@ -517,6 +512,7 @@ next_connection_entry(struct context *c)
 #endif
     } while (!ce_defined);
 
+    c->options.unsuccessful_attempts++;
     /* Check if this connection attempt would bring us over the limit */
     if (c->options.connect_retry_max > 0
         && c->options.unsuccessful_attempts > (l->len  * c->options.connect_retry_max))

2.6.2

Ref: https://patchwork.openvpn.net/project/openvpn2/list/?series=201

Hi,

These patches were meant to help implement choosing the remote through the GUI. I may not find time for that but the patches by themselves are still relevant.

If there is some interest I'll rebase to master.

Selva

Hi,

I'm working my way through the patch queue these days, and now I'm at this one :-)

Can you elaborate a bit how this would work, and how much work on the GUI side would be needed? (And, yes, a rebased patch :) ).

gert

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...

Hi,

From what I can recall...

Two points to note:

(i) With multiple remotes, openvpn exits if no successful connection could be made after two cycles through all remotes (undocumented?) . (ii) When --management-query-remote is used, the core presents one remote at a time and the user has to make a choice to skip, accept or replace without knowing which remotes are available.

Now, for a user-friendly implementation of selecting the remote from a GUI dialog, the plan is to silently cycle through all remotes, make a list and then allow the user to make a selection from the list. This will be aided by having a safe way to cycle through all remotes multiple times without the core exiting --- arguably, one cycle is enough to make a list and the list building is complete when the second cycle starts. But it would be much easier to do this without having to worry about the core exiting unexpectedly. The GUI knows how to restart or terminate the core exit if need be.

The behaviour is unchanged if management-query-remote is not in use.

The patch also changes the way failed connections are counted: skipped remotes should not be counted as failed as that count is used in the back-off logic.

Selva

Am 13.05.20 um 19:44 schrieb Selva Nair:

I do not like this change. If you want to list all remotes adding an option to the management interface that list all remotes is a better idea than to piggyback on skipping and implmenenting this as hack to list them all. Something like like-remotes.

Also I want skip to count a failure since a UI might have some knowlege that certain remotes cannot work and skipping therefore should also count as "have tried this remote".

Arne

From: Selva Nair <[email protected]...>

(i) Let the management-client predictably cycle through remote entries. This is done by not aborting after two cycles. The client can abort or restart the connection using signals (USR/HUP/TERM) as necessary.

In the current behaviour, the daemon can unexpectedly exit when the last remote is skipped. When management-query-remote is not in use, the behaviour is unchanged.

(ii) Do not count skipping a remote as an unsuccessful connection attempt. As the latter count is used for backoff it should count only failed attempts.

---
- rebased to master, no changes

 src/openvpn/init.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

index 70cd493..1bb39ee 100644
@@ -471,14 +471,6 @@ next_connection_entry(struct context *c)
                         c->c1.link_socket_addr.remote_list;
                 }
 
-                /*
-                 * Increase the number of connection attempts
-                 * If this is connect-retry-max * size(l)
-                 * OpenVPN will quit
-                 */
-
-                c->options.unsuccessful_attempts++;
-
                 if (++l->current >= l->len)
                 {
 

@@ -504,6 +496,9 @@ next_connection_entry(struct context *c)
         {
             /* allow management interface to override connection entry details */
             ce_defined = ce_management_query_remote(c);
+
+            /* ignore cycles when management-query-remote is in use */
+            n_cycles = 0;
             if (IS_SIG(c))
             {
                 break;

@@ -520,6 +515,7 @@ next_connection_entry(struct context *c)
 #endif
     } while (!ce_defined);
 
+    c->options.unsuccessful_attempts++;
     /* Check if this connection attempt would bring us over the limit */
     if (c->options.connect_retry_max > 0
         && c->options.unsuccessful_attempts > (l->len  * c->options.connect_retry_max))

2.1.4

From: Selva Nair <[email protected]...>

Currently this prompt is only output once, not re-written to the management interface when the management client connects. It is thus not seen by a client that connects after the prompt is output or one that disconnects and reconnects. This leads to a deadlock: the daemon waiting for the "remote" command from the client, the latter not aware of it.

Resolve by adding the ">REMOTE" and ">PROXY" prompt to man.persist.special_state_msg as done for other persisted prompts such as ">PASSWORD"

---
v2: bump and rebase to master

 src/openvpn/init.c | 4 ++++
 1 file changed, 4 insertions(+)

index 1cfffbb..b4781a2 100644
@@ -271,6 +271,7 @@ ce_management_query_proxy(struct context *c)
             buf_printf(&out, ">PROXY:%u,%s,%s", (l ? l->current : 0) + 1,
                        (proto_is_udp(ce->proto) ? "UDP" : "TCP"), np(ce->remote));
             management_notify_generic(management, BSTR(&out));
+            management->persist.special_state_msg = BSTR(&out);
         }
         ce->flags |= CE_MAN_QUERY_PROXY;
         while (ce->flags & CE_MAN_QUERY_PROXY)

@@ -282,6 +283,7 @@ ce_management_query_proxy(struct context *c)
                 break;
             }
         }
+        management->persist.special_state_msg = NULL;
         gc_free(&gc);
     }
 

@@ -351,6 +353,7 @@ ce_management_query_remote(struct context *c)
         buf_printf(&out, ">REMOTE:%s,%s,%s", np(ce->remote), ce->remote_port,
                    proto2ascii(ce->proto, ce->af, false));
         management_notify_generic(management, BSTR(&out));
+        management->persist.special_state_msg = BSTR(&out);
 
         ce->flags &= ~(CE_MAN_QUERY_REMOTE_MASK << CE_MAN_QUERY_REMOTE_SHIFT);
         ce->flags |= (CE_MAN_QUERY_REMOTE_QUERY << CE_MAN_QUERY_REMOTE_SHIFT);

@@ -364,6 +367,7 @@ ce_management_query_remote(struct context *c)
                 break;
             }
         }
+        management->persist.special_state_msg = NULL;
     }
     gc_free(&gc);
 

2.1.4

I won't claim to understand management, but the patch does what it says, and it does so in the same way as querying for >PASSWORD is done. Selva understands management way better than I do, so if he says this is needed, and the code is in line with what we currently have (manage.c, management_query_user_pass(), management_hold(), management_query_multiline()...) and does not look like "mem leak" or "overflow" - which it doesn't - good enough for me.

The code *looks* as if a reference to "out" is going out of scope, but that one is allocated in &gc, so it's fine. Just hard to read.

These functions could do with a bit of C99 modernizing, getting rid of extra nesting levels that are just there to enable local variables... but that's for a different round of refactoring.

Your patch has been applied to the master branch.

commit 93ba6ccddafcc87f336f50dadde144ea4f6178ad

Author: Selva Nair
Date:   Thu Feb 20 22:00:28 2020 -0500

     Persist management-query-remote and proxy prompts

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg19497.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Hi,

*and* to release/2.4, because "bugfix". Forgot about that comment when I was done with review and testing...

commit 38b46e6bf65489c2c5d75da1c02a3a1c33e6da88 (HEAD -> release/2.4)

Author: Selva Nair <[email protected]...>
Date:   Thu Feb 20 22:00:28 2020 -0500

Persist management-query-remote and proxy prompts

applies fine, tests fine.

gert

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...