| Message ID | 20220217182234.33850-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v3] Fix OpenVPN querying user/password if auth-token with user expires | expand |
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>
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>
Hi, On Thu, Aug 18, 2022 at 10:26:04AM +0200, Heiko Hund wrote: > 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. Actually, it shouldn't. The #ifdef is #ifdef ENABLE_MANAGEMENT auth_user_pass_setup(..., foo) #else auth_user_pass_setup(..., NULL) #endif so either the enable_auth_user_pass() goes before the #ifdef, or would need to be in both branches. So the patch is fine as it is, one can just not see this from the hunk alone. gert
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.
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(-)