| Message ID | 20220518093212.2802495-3-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Implement exit notifcation via control channel and temporary AUTH_FAIL | expand |
Patch looks reasonable when looking at it with -w. But I have some suggestions to further simplify the function. See below. > Arne Schwabe <arne@rfc2549.org> hat am 18.05.2022 11: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 > --- > src/openvpn/push.c | 96 ++++++++++++++++++++++++---------------------- > 1 file changed, 51 insertions(+), 45 deletions(-) > > diff --git a/src/openvpn/push.c b/src/openvpn/push.c > index 51b8bd521..be118831d 100644 > --- a/src/openvpn/push.c > +++ b/src/openvpn/push.c [...] > #ifdef ENABLE_MANAGEMENT > - if (management) > + if (management) > + { > + const char *reason = NULL; > + if (authfail_extended && BLEN(&buf)) > { > - 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); > + reason = BSTR(&buf); > } > + management_auth_failure(management, UP_TYPE_AUTH, reason); > + } > #endif > - /* > - * Save the dynamic-challenge text even when management is defined > - */ > - { > + /* > + * Save the dynamic-challenge text even when management is defined > + */ > + { This additional block has no meaning now that you don't define a variable anymore. > #ifdef ENABLE_MANAGEMENT This ifdef can be merged with the previous ifdef to further simplify the function. > - 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 > + if (authfail_extended > + && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf)) > + { > + ssl_put_auth_challenge(BSTR(&buf)); > } > +#endif > } > } So something like this on top of this patch: diff --git a/src/openvpn/push.c b/src/openvpn/push.c index be118831..6b8bc0e1 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -104,19 +104,15 @@ receive_auth_failed(struct context *c, const struct buffer *buffer) } management_auth_failure(management, UP_TYPE_AUTH, reason); } -#endif /* * Save the dynamic-challenge text even when management is defined */ + if (authfail_extended + && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf)) { -#ifdef ENABLE_MANAGEMENT - if (authfail_extended - && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf)) - { - ssl_put_auth_challenge(BSTR(&buf)); - } -#endif + ssl_put_auth_challenge(BSTR(&buf)); } +#endif } /* flichtenheld@MININT-961QE0A:~/openvpn/community/openvpn (master *)$ git --no-pager diff diff --git a/src/openvpn/push.c b/src/openvpn/push.c index be118831..6b8bc0e1 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -104,19 +104,15 @@ receive_auth_failed(struct context *c, const struct buffer *buffer) } management_auth_failure(management, UP_TYPE_AUTH, reason); } -#endif /* * Save the dynamic-challenge text even when management is defined */ + if (authfail_extended + && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf)) { -#ifdef ENABLE_MANAGEMENT - if (authfail_extended - && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf)) - { - ssl_put_auth_challenge(BSTR(&buf)); - } -#endif + ssl_put_auth_challenge(BSTR(&buf)); } +#endif } /* Regards, -- Frank Lichtenheld
> Frank Lichtenheld <frank@lichtenheld.com> hat am 18.05.2022 12:40 geschrieben: [...] > So something like this on top of this patch: Somehow managed to double paste the patch. They seem to be identical, so simply ignore either one ;) > diff --git a/src/openvpn/push.c b/src/openvpn/push.c > index be118831..6b8bc0e1 100644 > --- a/src/openvpn/push.c > +++ b/src/openvpn/push.c > @@ -104,19 +104,15 @@ receive_auth_failed(struct context *c, const struct buffer *buffer) > } > management_auth_failure(management, UP_TYPE_AUTH, reason); > } > -#endif > /* > * Save the dynamic-challenge text even when management is defined > */ > + if (authfail_extended > + && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf)) > { > -#ifdef ENABLE_MANAGEMENT > - if (authfail_extended > - && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf)) > - { > - ssl_put_auth_challenge(BSTR(&buf)); > - } > -#endif > + ssl_put_auth_challenge(BSTR(&buf)); > } > +#endif > } > > /* > flichtenheld@MININT-961QE0A:~/openvpn/community/openvpn (master *)$ git --no-pager diff > diff --git a/src/openvpn/push.c b/src/openvpn/push.c > index be118831..6b8bc0e1 100644 > --- a/src/openvpn/push.c > +++ b/src/openvpn/push.c > @@ -104,19 +104,15 @@ receive_auth_failed(struct context *c, const struct buffer *buffer) > } > management_auth_failure(management, UP_TYPE_AUTH, reason); > } > -#endif > /* > * Save the dynamic-challenge text even when management is defined > */ > + if (authfail_extended > + && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf)) > { > -#ifdef ENABLE_MANAGEMENT > - if (authfail_extended > - && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf)) > - { > - ssl_put_auth_challenge(BSTR(&buf)); > - } > -#endif > + ssl_put_auth_challenge(BSTR(&buf)); > } > +#endif > } > > /* -- Frank Lichtenheld
diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 51b8bd521..be118831d 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -53,63 +53,69 @@ 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) + if (management) + { + const char *reason = NULL; + if (authfail_extended && BLEN(&buf)) { - 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); + reason = BSTR(&buf); } + management_auth_failure(management, UP_TYPE_AUTH, reason); + } #endif - /* - * Save the dynamic-challenge text even when management is defined - */ - { + /* + * Save the dynamic-challenge text even when management is defined + */ + { #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 + if (authfail_extended + && buf_string_match_head_str(&buf, "CRV1:") && BLEN(&buf)) + { + ssl_put_auth_challenge(BSTR(&buf)); } +#endif } }