diff mbox series

[Openvpn-devel,v2,2/4] Cleanup receive_auth_failed and simplify method

Message ID 20220520213250.3126372-3-arne@rfc2549.org
State Accepted
Headers show
Series Implement exit notifcation via control channel and temporary AUTH_FAIL | expand

Commit Message

Arne Schwabe May 20, 2022, 11:32 a.m. UTC 
This simplifies the buffer handling in the method and adds a quick
return instead of wrapping the whole method in a if (pull) block

Patch V2: remove uncessary ifdef/endif and unnecassary block
---
 src/openvpn/push.c | 99 ++++++++++++++++++++++++----------------------
 1 file changed, 51 insertions(+), 48 deletions(-)

Comments

Frank Lichtenheld May 22, 2022, 10:26 p.m. UTC | #1 
Acked-By: Frank Lichtenheld <frank@lichtenheld.com>

Best viewed with "git show -w" ;)

AFAICT this is a good cleanup without any behavioral change.

> Arne Schwabe <arne@rfc2549.org> hat am 20.05.2022 23:32 geschrieben:
> This simplifies the buffer handling in the method and adds a quick
> return instead of wrapping the whole method in a if (pull) block
> 
> Patch V2: remove uncessary ifdef/endif and unnecassary block
> ---
>  src/openvpn/push.c | 99 ++++++++++++++++++++++++----------------------
>  1 file changed, 51 insertions(+), 48 deletions(-)
> 
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index fa0def7f8..1c4e637e4 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -53,64 +53,67 @@ receive_auth_failed(struct context *c, const struct buffer *buffer)
>      msg(M_VERB0, "AUTH: Received control message: %s", BSTR(buffer));
>      c->options.no_advance = true;
>  
> -    if (c->options.pull)
> +    if (!c->options.pull)
>      {
> -        /* Before checking how to react on AUTH_FAILED, first check if the
> -         * failed auth might be the result of an expired auth-token.
> -         * Note that a server restart will trigger a generic AUTH_FAILED
> -         * instead an AUTH_FAILED,SESSION so handle all AUTH_FAILED message
> -         * identical for this scenario */
> -        if (ssl_clean_auth_token())
> -        {
> -            c->sig->signal_received = SIGUSR1; /* SOFT-SIGUSR1 -- Auth failure error */
> -            c->sig->signal_text = "auth-failure (auth-token)";
> -        }
> -        else
> +        return;
> +    }
> +
> +    struct buffer buf = *buffer;
> +
> +    /* If the AUTH_FAIL message ends with a , it is an extended message that
> +     * contains further flags */
> +    bool authfail_extended = buf_string_compare_advance(&buf, "AUTH_FAILED,");
> +
> +    /* Before checking how to react on AUTH_FAILED, first check if the
> +     * failed auth might be the result of an expired auth-token.
> +     * Note that a server restart will trigger a generic AUTH_FAILED
> +     * instead an AUTH_FAILED,SESSION so handle all AUTH_FAILED message
> +     * identical for this scenario */
> +    if (ssl_clean_auth_token())
> +    {
> +        c->sig->signal_received = SIGUSR1;     /* SOFT-SIGUSR1 -- Auth failure error */
> +        c->sig->signal_text = "auth-failure (auth-token)";
> +    }
> +    else
> +    {
> +        switch (auth_retry_get())
>          {
> -            switch (auth_retry_get())
> -            {
> -                case AR_NONE:
> -                    c->sig->signal_received = SIGTERM; /* SOFT-SIGTERM -- Auth failure error */
> -                    break;
> +            case AR_NONE:
> +                c->sig->signal_received = SIGTERM;     /* SOFT-SIGTERM -- Auth failure error */
> +                break;
>  
> -                case AR_INTERACT:
> -                    ssl_purge_auth(false);
> +            case AR_INTERACT:
> +                ssl_purge_auth(false);
>  
> -                case AR_NOINTERACT:
> -                    c->sig->signal_received = SIGUSR1; /* SOFT-SIGUSR1 -- Auth failure error */
> -                    break;
> +            case AR_NOINTERACT:
> +                c->sig->signal_received = SIGUSR1;     /* SOFT-SIGUSR1 -- Auth failure error */
> +                break;
>  
> -                default:
> -                    ASSERT(0);
> -            }
> -            c->sig->signal_text = "auth-failure";
> +            default:
> +                ASSERT(0);
>          }
> +        c->sig->signal_text = "auth-failure";
> +    }
>  #ifdef ENABLE_MANAGEMENT
> -        if (management)
> -        {
> -            const char *reason = NULL;
> -            struct buffer buf = *buffer;
> -            if (buf_string_compare_advance(&buf, "AUTH_FAILED,") && BLEN(&buf))
> -            {
> -                reason = BSTR(&buf);
> -            }
> -            management_auth_failure(management, UP_TYPE_AUTH, reason);
> -        }
> -#endif
> -        /*
> -         * Save the dynamic-challenge text even when management is defined
> -         */
> +    if (management)
> +    {
> +        const char *reason = NULL;
> +        if (authfail_extended && BLEN(&buf))
>          {
> -#ifdef ENABLE_MANAGEMENT
> -            struct buffer buf = *buffer;
> -            if (buf_string_match_head_str(&buf, "AUTH_FAILED,CRV1:") && BLEN(&buf))
> -            {
> -                buf_advance(&buf, 12); /* Length of "AUTH_FAILED," substring */
> -                ssl_put_auth_challenge(BSTR(&buf));
> -            }
> -#endif
> +            reason = BSTR(&buf);
>          }
> +        management_auth_failure(management, UP_TYPE_AUTH, reason);
> +    }
> +    /*
> +     * Save the dynamic-challenge text even when management is defined
> +     */
> +    if (authfail_extended
> +        && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf))
> +    {
> +        ssl_put_auth_challenge(BSTR(&buf));
>      }
> +#endif
> +
>  }
>  
>  /*
> -- 

--
Frank Lichtenheld
Gert Doering Aug. 2, 2022, 3:26 a.m. UTC | #2 
As Frank said, "best viewed with diff -w" :-) - the cleanup bits are
also straightforward.

I do not have a good test case for this - but my usual client tests with
"expect AUTH_FAILED..." all pass.

Your patch has been applied to the master branch.

commit 88823adebac31958cee83572241cff9fc775a601
Author: Arne Schwabe
Date:   Fri May 20 23:32:48 2022 +0200

     Cleanup receive_auth_failed and simplify method

     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Message-Id: <20220520213250.3126372-3-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24412.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Heiko Hund Aug. 18, 2022, 4:39 a.m. UTC | #3 
On Freitag, 20. Mai 2022 23:32:48 CEST Arne Schwabe wrote:
> This simplifies the buffer handling in the method and adds a quick
> return instead of wrapping the whole method in a if (pull) block
> 
> Patch V2: remove uncessary ifdef/endif and unnecassary block

Acked-by: Heiko Hund <heiko@ist.eigentlich.net>
Gert Doering Aug. 18, 2022, 7:20 a.m. UTC | #4 
Hi,

On Thu, Aug 18, 2022 at 04:39:07PM +0200, Heiko Hund wrote:
> On Freitag, 20. Mai 2022 23:32:48 CEST Arne Schwabe wrote:
> > This simplifies the buffer handling in the method and adds a quick
> > return instead of wrapping the whole method in a if (pull) block
> > 
> > Patch V2: remove uncessary ifdef/endif and unnecassary block
> 
> Acked-by: Heiko Hund <heiko@ist.eigentlich.net>

Thanks for the extra ACK :-) - this got already merged as

commit 88823adebac31958cee83572241cff9fc775a601
Author: Arne Schwabe <arne@rfc2549.org>
Date:   Fri May 20 23:32:48 2022 +0200

    Cleanup receive_auth_failed and simplify method

with an ACK from Frank.

gert
Heiko Hund Aug. 18, 2022, 10:55 p.m. UTC | #5 
On Donnerstag, 18. August 2022 19:20:33 CEST Gert Doering wrote:
> On Thu, Aug 18, 2022 at 04:39:07PM +0200, Heiko Hund wrote:
> > On Freitag, 20. Mai 2022 23:32:48 CEST Arne Schwabe wrote:
> > > Patch V2: remove uncessary ifdef/endif and unnecassary block
> > 
> > Acked-by: Heiko Hund <heiko@ist.eigentlich.net>
> 
> Thanks for the extra ACK :-) - this got already merged as
> 
> commit 88823adebac31958cee83572241cff9fc775a601

Hm, was still showing in patchwork.
diff mbox series

Patch

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index fa0def7f8..1c4e637e4 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -53,64 +53,67 @@  receive_auth_failed(struct context *c, const struct buffer *buffer)
     msg(M_VERB0, "AUTH: Received control message: %s", BSTR(buffer));
     c->options.no_advance = true;
 
-    if (c->options.pull)
+    if (!c->options.pull)
     {
-        /* Before checking how to react on AUTH_FAILED, first check if the
-         * failed auth might be the result of an expired auth-token.
-         * Note that a server restart will trigger a generic AUTH_FAILED
-         * instead an AUTH_FAILED,SESSION so handle all AUTH_FAILED message
-         * identical for this scenario */
-        if (ssl_clean_auth_token())
-        {
-            c->sig->signal_received = SIGUSR1; /* SOFT-SIGUSR1 -- Auth failure error */
-            c->sig->signal_text = "auth-failure (auth-token)";
-        }
-        else
+        return;
+    }
+
+    struct buffer buf = *buffer;
+
+    /* If the AUTH_FAIL message ends with a , it is an extended message that
+     * contains further flags */
+    bool authfail_extended = buf_string_compare_advance(&buf, "AUTH_FAILED,");
+
+    /* Before checking how to react on AUTH_FAILED, first check if the
+     * failed auth might be the result of an expired auth-token.
+     * Note that a server restart will trigger a generic AUTH_FAILED
+     * instead an AUTH_FAILED,SESSION so handle all AUTH_FAILED message
+     * identical for this scenario */
+    if (ssl_clean_auth_token())
+    {
+        c->sig->signal_received = SIGUSR1;     /* SOFT-SIGUSR1 -- Auth failure error */
+        c->sig->signal_text = "auth-failure (auth-token)";
+    }
+    else
+    {
+        switch (auth_retry_get())
         {
-            switch (auth_retry_get())
-            {
-                case AR_NONE:
-                    c->sig->signal_received = SIGTERM; /* SOFT-SIGTERM -- Auth failure error */
-                    break;
+            case AR_NONE:
+                c->sig->signal_received = SIGTERM;     /* SOFT-SIGTERM -- Auth failure error */
+                break;
 
-                case AR_INTERACT:
-                    ssl_purge_auth(false);
+            case AR_INTERACT:
+                ssl_purge_auth(false);
 
-                case AR_NOINTERACT:
-                    c->sig->signal_received = SIGUSR1; /* SOFT-SIGUSR1 -- Auth failure error */
-                    break;
+            case AR_NOINTERACT:
+                c->sig->signal_received = SIGUSR1;     /* SOFT-SIGUSR1 -- Auth failure error */
+                break;
 
-                default:
-                    ASSERT(0);
-            }
-            c->sig->signal_text = "auth-failure";
+            default:
+                ASSERT(0);
         }
+        c->sig->signal_text = "auth-failure";
+    }
 #ifdef ENABLE_MANAGEMENT
-        if (management)
-        {
-            const char *reason = NULL;
-            struct buffer buf = *buffer;
-            if (buf_string_compare_advance(&buf, "AUTH_FAILED,") && BLEN(&buf))
-            {
-                reason = BSTR(&buf);
-            }
-            management_auth_failure(management, UP_TYPE_AUTH, reason);
-        }
-#endif
-        /*
-         * Save the dynamic-challenge text even when management is defined
-         */
+    if (management)
+    {
+        const char *reason = NULL;
+        if (authfail_extended && BLEN(&buf))
         {
-#ifdef ENABLE_MANAGEMENT
-            struct buffer buf = *buffer;
-            if (buf_string_match_head_str(&buf, "AUTH_FAILED,CRV1:") && BLEN(&buf))
-            {
-                buf_advance(&buf, 12); /* Length of "AUTH_FAILED," substring */
-                ssl_put_auth_challenge(BSTR(&buf));
-            }
-#endif
+            reason = BSTR(&buf);
         }
+        management_auth_failure(management, UP_TYPE_AUTH, reason);
+    }
+    /*
+     * Save the dynamic-challenge text even when management is defined
+     */
+    if (authfail_extended
+        && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf))
+    {
+        ssl_put_auth_challenge(BSTR(&buf));
     }
+#endif
+
 }
 
 /*