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.n...@gmail.com>
Acked-by: Arne Schwabe <a...@rfc2549.org>
Message-Id: <20221023195105.31714-1-selva.n...@gmail.com>
URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25452.html
Signed-off-by: Gert Doering <g...@greenie.muc.de>
--
kind regards,
Gert Doering
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
