Message ID | 20220520213250.3126372-3-arne@rfc2549.org |
---|---|
State | Accepted |
Headers | show |
Series | Implement exit notifcation via control channel and temporary AUTH_FAIL | expand |
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
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
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>
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
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 --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 + } /*