[Openvpn-devel,v3] Fix OpenVPN querying user/password if auth-token with user expires

Message ID 20220217182234.33850-1-arne@rfc2549.org
State New
Headers show
Series [Openvpn-devel,v3] Fix OpenVPN querying user/password if auth-token with user expires | expand

Commit Message

Arne Schwabe Feb. 17, 2022, 6:22 p.m. UTC
The problematic behaviour happens when start a profile without
auth-user-pass and connect to a server that pushes auth-token
When the auth token expires OpenVPN asks for auth User and password
again.

The problem is that the auth_user_pass_setup sets
auth_user_pass_enabled = true; This function is called from two places.
In ssl.c it is only called with an auth-token present or that
variable already set. The other one is init_query_passwords.

Move setting auth_user_pass_enabled to the second place to ensure it is
only set if we really want passwords.

Patch v2: Remove unrelated code change
Patch v3: Rebase to master

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/init.c | 1 +
 src/openvpn/ssl.c  | 7 ++++++-
 src/openvpn/ssl.h  | 3 +++
 3 files changed, 10 insertions(+), 1 deletion(-)

Comments

David Sommerseth June 27, 2022, 8:19 p.m. UTC | #1
On 17/02/2022 19:22, Arne Schwabe wrote:
> The problematic behaviour happens when start a profile without

That sentence can be improved slightly; can be done commit time.  I propose:

   The problematic behaviour happens when a profile is started without

> auth-user-pass and connect to a server that pushes auth-token
> When the auth token expires OpenVPN asks for auth User and password
> again.
> 
> The problem is that the auth_user_pass_setup sets
> auth_user_pass_enabled = true; This function is called from two places.
> In ssl.c it is only called with an auth-token present or that
> variable already set. The other one is init_query_passwords.
> 
> Move setting auth_user_pass_enabled to the second place to ensure it is
> only set if we really want passwords.
> 
> Patch v2: Remove unrelated code change
> Patch v3: Rebase to master
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org
I've done several attempts to reproduce the ill behavior with
OpenVPN 2.5.7, both local server (using the --management interface), 
OpenVPN Cloud and OpenVPN Access Server (auto-login profile) without 
being able to trigger this behavior.  Maybe my testing just didn't run 
long enough.  Connections was restarted, servers restarted to wipe 
state, etc.  But it just reconnected again without any issues.  So 
behavior this patch fixes is clearly in the "corner case" area of bugs.

That said, the code looks fine and sane.  I can understand the code path 
and I can see that username/passwords should - in theory - not be asked 
for when the auth-token expires with this fix; and that it would ask for 
it without this fix.

Acked-By: David Sommerseth <davids@openvpn.net>
Heiko Hund Aug. 18, 2022, 8:26 a.m. UTC | #2
On Donnerstag, 17. Februar 2022 19:22:34 CEST Arne Schwabe wrote:

> @@ -590,6 +590,7 @@ init_query_passwords(const struct context *c)
>      /* Auth user/pass input */
>      if (c->options.auth_user_pass_file)
>      {
> +        enable_auth_user_pass();
>  #ifdef ENABLE_MANAGEMENT
>          auth_user_pass_setup(c->options.auth_user_pass_file,
> &c->options.sc_info); #else

This should be inside the #ifdef to do exactly the same as before, i.e. 
doesn't introduce side effects potentially. But then we want to get rid of 
ENABLE_MANAGEMENT soonish, thus it may not matter.

Acked-by: Heiko Hund <heiko@ist.eigentlich.net>

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 21adc3cf..e5fba621 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -590,6 +590,7 @@  init_query_passwords(const struct context *c)
     /* Auth user/pass input */
     if (c->options.auth_user_pass_file)
     {
+        enable_auth_user_pass();
 #ifdef ENABLE_MANAGEMENT
         auth_user_pass_setup(c->options.auth_user_pass_file, &c->options.sc_info);
 #else
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 14a943a7..b68708b0 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -398,9 +398,14 @@  static char *auth_challenge; /* GLOBAL */
 #endif
 
 void
-auth_user_pass_setup(const char *auth_file, const struct static_challenge_info *sci)
+enable_auth_user_pass()
 {
     auth_user_pass_enabled = true;
+}
+
+void
+auth_user_pass_setup(const char *auth_file, const struct static_challenge_info *sci)
+{
     if (!auth_user_pass.defined && !auth_token.defined)
     {
 #ifdef ENABLE_MANAGEMENT
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index cf754ad2..76d8a7dc 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -436,6 +436,9 @@  void tls_post_encrypt(struct tls_multi *multi, struct buffer *buf);
  */
 void pem_password_setup(const char *auth_file);
 
+/* Enables the use of user/password authentication */
+void enable_auth_user_pass();
+
 /*
  * Setup authentication username and password. If auth_file is given, use the
  * credentials stored in the file.