[Openvpn-devel,2/2] Allow repeated cycles through remotes when management-query-remote is in use

Message ID 1521522224-18829-2-git-send-email-selva.nair@gmail.com
State Superseded
Headers show
Series [Openvpn-devel,1/2] Persist management-query-remote and proxy prompts | expand

Commit Message

Selva Nair March 19, 2018, 6:03 p.m. UTC
From: Selva Nair <selva.nair@gmail.com>

(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.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/init.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

Selva Nair June 9, 2019, 9:33 a.m. UTC | #1
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
Gert Doering May 13, 2020, 6:36 a.m. UTC | #2
Hi,

On Sun, Jun 09, 2019 at 03:33:55PM -0400, Selva Nair wrote:
> Ref: https://patchwork.openvpn.net/project/openvpn2/list/?series=201
> 
> 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.

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
Selva Nair May 13, 2020, 7:44 a.m. UTC | #3
Hi,

On Wed, May 13, 2020 at 12:36 PM Gert Doering <gert@greenie.muc.de> wrote:
>
> Hi,
>
> On Sun, Jun 09, 2019 at 03:33:55PM -0400, Selva Nair wrote:
> > Ref: https://patchwork.openvpn.net/project/openvpn2/list/?series=201
> >
> > 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.
>
> 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 :) ).

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
Arne Schwabe May 18, 2020, 12:20 a.m. UTC | #4
Am 13.05.20 um 19:44 schrieb Selva Nair:
> Hi,
> 
> On Wed, May 13, 2020 at 12:36 PM Gert Doering <gert@greenie.muc.de> wrote:
>>
>> Hi,
>>
>> On Sun, Jun 09, 2019 at 03:33:55PM -0400, Selva Nair wrote:
>>> Ref: https://patchwork.openvpn.net/project/openvpn2/list/?series=201
>>>
>>> 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.
>>
>> 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 :) ).
> 
> 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.


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

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 413563a..c63dc67 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -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))