diff mbox series

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

Message ID 20210722162409.231265-1-arne@rfc2549.org
State Superseded
Headers
Series [Openvpn-devel] Fix OpenVPN querying user/password if auth-token with user expires |

Commit Message

Arne Schwabe July 22, 2021, 6:24 a.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.
---
 src/openvpn/init.c | 2 ++
 src/openvpn/ssl.c  | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Gert Doering July 22, 2021, 8:40 p.m. UTC | #1 
Hi,

On Thu, Jul 22, 2021 at 06:24:09PM +0200, Arne Schwabe wrote:
> @@ -3116,6 +3117,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
>      to.auth_token_generate = options->auth_token_generate;
>      to.auth_token_lifetime = options->auth_token_lifetime;
>      to.auth_token_call_auth = options->auth_token_call_auth;
> +    to.auth_token_user_common_name = options->auth_token_user_common_name;
>      to.auth_token_key = c->c1.ks.auth_token_key;
>  
>      to.x509_track = options->x509_track;

That hunk looks unrelated...?

gert
Arne Schwabe July 22, 2021, 11:37 p.m. UTC | #2 
Am 23.07.21 um 08:40 schrieb Gert Doering:
> Hi,
> 
> On Thu, Jul 22, 2021 at 06:24:09PM +0200, Arne Schwabe wrote:
>> @@ -3116,6 +3117,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
>>      to.auth_token_generate = options->auth_token_generate;
>>      to.auth_token_lifetime = options->auth_token_lifetime;
>>      to.auth_token_call_auth = options->auth_token_call_auth;
>> +    to.auth_token_user_common_name = options->auth_token_user_common_name;
>>      to.auth_token_key = c->c1.ks.auth_token_key;
>>  
>>      to.x509_track = options->x509_track;
> 
> That hunk looks unrelated...?
> 
> gert
> 
Oops :/

Is there anything wrong or should I send a v2 just with that change?

Arne
Gert Doering July 22, 2021, 11:41 p.m. UTC | #3 
Hi,

On Fri, Jul 23, 2021 at 11:37:37AM +0200, Arne Schwabe wrote:
> Am 23.07.21 um 08:40 schrieb Gert Doering:
> > On Thu, Jul 22, 2021 at 06:24:09PM +0200, Arne Schwabe wrote:
> >> @@ -3116,6 +3117,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
> >>      to.auth_token_generate = options->auth_token_generate;
> >>      to.auth_token_lifetime = options->auth_token_lifetime;
> >>      to.auth_token_call_auth = options->auth_token_call_auth;
> >> +    to.auth_token_user_common_name = options->auth_token_user_common_name;
> >>      to.auth_token_key = c->c1.ks.auth_token_key;
> >>  
> >>      to.x509_track = options->x509_track;
> > 
> > That hunk looks unrelated...?
> > 
> Oops :/
> 
> Is there anything wrong or should I send a v2 just with that change?

Well, current master does not have an "auth_token_user_common_name" structure
element anyway, so the introduction of it should not be part of this bugfix
(and it wouldn't compile) :-)

So, "v2, *without* that change" sounds like a plan.

gert
Antonio Quartulli July 27, 2021, 3:42 a.m. UTC | #4 
Hi,

On 22/07/2021 18:24, Arne Schwabe wrote:
> 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.
> ---
>  src/openvpn/init.c | 2 ++
>  src/openvpn/ssl.c  | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index a1401e805..f9083e69c 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -595,6 +595,7 @@ init_query_passwords(const struct context *c)
>      /* Auth user/pass input */
>      if (c->options.auth_user_pass_file)
>      {
> +        auth_user_pass_enabled = true;
>  #ifdef ENABLE_MANAGEMENT
>          auth_user_pass_setup(c->options.auth_user_pass_file, &c->options.sc_info);

init.c: In function ‘init_query_passwords’:
init.c:597:9: error: ‘auth_user_pass_enabled’ undeclared (first use in
this function); did you mean ‘auth_user_pass_setup’?
  597 |         auth_user_pass_enabled = true;
      |         ^~~~~~~~~~~~~~~~~~~~~~
      |         auth_user_pass_setup


anything wrong with my master branch?
Antonio Quartulli July 27, 2021, 3:44 a.m. UTC | #5 
Hi,

On 27/07/2021 15:42, Antonio Quartulli wrote:
> Hi,
> 
> On 22/07/2021 18:24, Arne Schwabe wrote:
>> 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.
>> ---
>>  src/openvpn/init.c | 2 ++
>>  src/openvpn/ssl.c  | 1 -
>>  2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
>> index a1401e805..f9083e69c 100644
>> --- a/src/openvpn/init.c
>> +++ b/src/openvpn/init.c
>> @@ -595,6 +595,7 @@ init_query_passwords(const struct context *c)
>>      /* Auth user/pass input */
>>      if (c->options.auth_user_pass_file)
>>      {
>> +        auth_user_pass_enabled = true;
>>  #ifdef ENABLE_MANAGEMENT
>>          auth_user_pass_setup(c->options.auth_user_pass_file, &c->options.sc_info);
> 
> init.c: In function ‘init_query_passwords’:
> init.c:597:9: error: ‘auth_user_pass_enabled’ undeclared (first use in
> this function); did you mean ‘auth_user_pass_setup’?
>   597 |         auth_user_pass_enabled = true;
>       |         ^~~~~~~~~~~~~~~~~~~~~~
>       |         auth_user_pass_setup
> 
> 
> anything wrong with my master branch?

this reply was meant for PATCHv2.

Cheers,


> 
> 
>
diff mbox series

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index a1401e805..f9083e69c 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -595,6 +595,7 @@  init_query_passwords(const struct context *c)
     /* Auth user/pass input */
     if (c->options.auth_user_pass_file)
     {
+        auth_user_pass_enabled = true;
 #ifdef ENABLE_MANAGEMENT
         auth_user_pass_setup(c->options.auth_user_pass_file, &c->options.sc_info);
 #else
@@ -3116,6 +3117,7 @@  do_init_crypto_tls(struct context *c, const unsigned int flags)
     to.auth_token_generate = options->auth_token_generate;
     to.auth_token_lifetime = options->auth_token_lifetime;
     to.auth_token_call_auth = options->auth_token_call_auth;
+    to.auth_token_user_common_name = options->auth_token_user_common_name;
     to.auth_token_key = c->c1.ks.auth_token_key;
 
     to.x509_track = options->x509_track;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index aa8cb3b27..c2dc36019 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -396,7 +396,6 @@  static char *auth_challenge; /* GLOBAL */
 void
 auth_user_pass_setup(const char *auth_file, const struct static_challenge_info *sci)
 {
-    auth_user_pass_enabled = true;
     if (!auth_user_pass.defined && !auth_token.defined)
     {
 #ifdef ENABLE_MANAGEMENT