| Message ID | 20221023195105.31714-1-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Ensure --auth-nocache is handled during renegotiation | expand |
Am 23.10.22 um 21:51 schrieb selva.nair@gmail.com: > From: Selva Nair <selva.nair@gmail.com> > > Currently, clearing auth_user_pass struct is delayed until > push-reply processing to support auth-token. This results in > username/password not purged after renegotiations that may > not accompany any pushed tokens -- say, when auth-token is not > in use. > > Fix by always clearing auth_user_pass soon after it is used, > instead of delaying the purge as in pre-token days. But, when > "pull" is true, retain the username in auth_token in anticipation > of a token that may or may not arrive later. > > Remove ssl_clean_user_pass() as there is no delayed purge any > longer -- auth-nocache handling is now done immediately after > writing username/password to the send-buffer. Acked-By: Arne Schwabe <arne@rfc2549.org I think the delaying and complicated logic, which thiss patch removes, is the last part of the attempt to try to use the same auth_user_pass struct for both auth token and user/password. Arne
Looking at the code, I'd say your commit description makes a lot of
sense, and explains the observed "funnies" quite well (which I could
reproduce). The code change looks reasonable, Arne ACKed this, and
I could not find a way to break it. Good :-)
One comment, though: when auth-tokens are in use, this call chain
if (auth_token.token_defined && auth_token.defined)
{
up = &auth_token;
}
/* save username for auth-token which may get pushed later */
if (session->opt->pull)
{
strncpynt(auth_token.username, up->username, USER_PASS_LEN);
}
.. will effectively copy auth_token.username to itself, which looks
a bit silly but *should* be well-defined. At least it did not cause
issues in my auth-token tests. If we think this is unwanted, having
a check like
if (session->opt->pull && up != &auth_token)
would avoid that ("copy only if not working on auth_token already").
Testing took quite a bit of test cases + renegotiations to catch
"the original problem" and verify that "the fix" does not break
related use cases (since this is a tangled mess of a-u-p and tokens)...
but it looks good!
old code (commit 7d48d31b8226d5, fairly recent "master", and also
with 2.5.6 / commit 7b1b100557):
* --auth-user-pass --auth-nocache --reneg-sec 150 (no tokens)
expected behaviour:
- u+p reauth every 150 seconds, prompt for username+password
result:
- asks on *first* renegotiation
- ran into a --ping-restart timeout
- reconnected with *cached* username + password <<< this is unwanted
- asks again on *first* reneg
- next 100 renegs with *cached* u+p <<< unwanted
-> so, clearly somewhat nondeterministic and not what was ordered
with "master + patch" and also "release/2.5 + patch":
* --auth-user-pass --reneg-sec 150
expected behaviour:
- u+p reauth every 150 seconds, no prompting on reneg
result: works fine (no prompts after the initial ones)
* --auth-user-pass --auth-nocache --reneg-sec 150
expected behaviour:
- u+p reauth every 150 seconds, prompt for username+password
result: works fine (prompts on every reneg)
* --auth-user-pass --reneg-sec 150 + server-pushed auth-token
expected behaviour:
- token reauth ever 150 sec
- u+p reauth when the token expires (600s here), no prompting
result: works fine (no prompts on reneg ever)
- terminate + restart server -> reauth with token, no prompting
result: works fine
NOTE: due to reasons outside of this patch, "renegotiate with UP without
prompting" only works if there is a continuous connection to the server
- if the server connection breaks, server_pushed_signal()->ssl_purge_auth()
happens, and UP is forgotten, so when the token expires, prompts ensue
(surprising behaviour, but not caused by this patch).
* --auth-user-pass --auth-nocache --reneg-sec 150 + server-pushed auth-token
expected behaviour:
- token reauth ever 150 sec
- u+p reauth when the token expires (600s), prompt for username+password
result: works fine (prompts at token expiry, *only*)
- terminate + restart server -> reauth with token, no prompting
result: works fine
(never using --auth-user-pass <file>, because that makes it hard to verify
without extra instrumentation whether it used the cached copy, or the
on-file copy)
Your patch has been applied to the master and release/2.5 branch.
commit 3a4fb17d103be37599d72d072bbee42cc121a39d (master)
commit 5ad4b4b374f072459ab2436ed372c92d3a42d65d (HEAD -> release/2.5)
Author: Selva Nair
Date: Sun Oct 23 15:51:05 2022 -0400
Ensure --auth-nocache is handled during renegotiation
Signed-off-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20221023195105.31714-1-selva.nair@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25452.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
On Thu, Oct 27, 2022 at 2:08 AM Gert Doering <gert@greenie.muc.de> wrote: > Looking at the code, I'd say your commit description makes a lot of > sense, and explains the observed "funnies" quite well (which I could > reproduce). The code change looks reasonable, Arne ACKed this, and > I could not find a way to break it. Good :-) > Thanks for the laborious testing! 2.4 is also affected as commit dfd624b52 was cherrypicked into it as well: (commit 607dfa9) But this patch won't apply due to some context changes. Would you like a resolved patch or is 2.4 out of "support" now? Selva
Hi, On Thu, Oct 27, 2022 at 11:18:07AM -0400, Selva Nair wrote: > On Thu, Oct 27, 2022 at 2:08 AM Gert Doering <gert@greenie.muc.de> wrote: > > > Looking at the code, I'd say your commit description makes a lot of > > sense, and explains the observed "funnies" quite well (which I could > > reproduce). The code change looks reasonable, Arne ACKed this, and > > I could not find a way to break it. Good :-) > Thanks for the laborious testing! We've been bitten *so* often already by --auth-nocache + --auth-token combinations on the path here that I've become very careful ;-) > 2.4 is also affected as commit dfd624b52 was cherrypicked into it as well: > (commit 607dfa9) > But this patch won't apply due to some context changes. > > Would you like a resolved patch or is 2.4 out of "support" now? It's in "semisupport", so I'd merge a patch for that - but it's not clear if we ever do a formal 2.4.13 release, and even if we do, we are *not* going to a full windows installer release (so, just tag + tarball, if needed). 2.3, no :-) (I do have 2.2 and 2.3 test clients for the "master" test, but actually getting these to build on modern systems with recent OpenSSL is painful...) gert
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index c48048a1..65a822e6 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1548,19 +1548,6 @@ initialization_sequence_completed(struct context *c, const unsigned int flags) /* If we delayed UID/GID downgrade or chroot, do it now */ do_uid_gid_chroot(c, true); - - /* - * In some cases (i.e. when receiving auth-token via - * push-reply) the auth-nocache option configured on the - * client is overridden; for this reason we have to wait - * for the push-reply message before attempting to wipe - * the user/pass entered by the user - */ - if (c->options.mode == MODE_POINT_TO_POINT) - { - ssl_clean_user_pass(); - } - /* Test if errors */ if (flags & ISC_ERRORS) { diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c index 50f7f975..d78106cd 100644 --- a/src/openvpn/misc.c +++ b/src/openvpn/misc.c @@ -504,19 +504,13 @@ set_auth_token(struct user_pass *up, struct user_pass *tk, const char *token) * --auth-token has no username, so it needs the username * either already set or copied from up, or later set by * --auth-token-user - * - * Do not overwrite the username if already set to avoid - * overwriting an username set by --auth-token-user + * If already set, tk is fully defined. */ - if (up->defined && !tk->defined) + if (strlen(tk->username)) { - strncpynt(tk->username, up->username, USER_PASS_LEN); tk->defined = true; } } - - /* Cleans user/pass for nocache */ - purge_user_pass(up, false); } void diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 4c0d78a1..765861b1 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -2180,20 +2180,13 @@ key_method_2_write(struct buffer *buf, struct tls_multi *multi, struct tls_sessi { goto error; } - /* if auth-nocache was specified, the auth_user_pass object reaches - * a "complete" state only after having received the push-reply - * message. The push message might contain an auth-token that needs - * the username of auth_user_pass. - * - * For this reason, skip the purge operation here if no push-reply - * message has been received yet. - * - * This normally happens upon first negotiation only. - */ - if (!session->opt->pull) + /* save username for auth-token which may get pushed later */ + if (session->opt->pull) { - purge_user_pass(&auth_user_pass, false); + strncpynt(auth_token.username, up->username, USER_PASS_LEN); } + /* respect auth-nocache */ + purge_user_pass(&auth_user_pass, false); } else { @@ -4092,9 +4085,3 @@ print_data: done: return BSTR(&out); } - -void -ssl_clean_user_pass(void) -{ - purge_user_pass(&auth_user_pass, false); -} diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index 9ae6ae8f..96de9ccc 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -538,12 +538,6 @@ void extract_x509_field_test(void); */ bool is_hard_reset_method2(int op); -/** - * Cleans the saved user/password unless auth-nocache is in use. - */ -void ssl_clean_user_pass(void); - - /* * Show the TLS ciphers that are available for us to use in the SSL * library with headers hinting their usage and warnings about usage.