Hi, On Wed, Oct 19, 2022 at 06:46:42PM -0400, selva.n...@gmail.com wrote: > From: Selva Nair <selva.n...@gmail.com> > > Starting from commit e61b401a auth-token is saved in a separate struct > from auth-user-pass and is not cleared when ssl_purge_auth() is called. > This makes "forget-passwords" sent to the management > interface or "--management-forget-disconnect" option not to work > as expected.
I am not sure I can go along with Arne's ACK here - the reason is that
ssl_purge_auth() is not only called from man_forget_passwords() (where
I'm totally fine with the change) but also on received EEN from the
server ("RESTART"), and this patch changes behaviour here.
Without patch:
- server pushes token (secret-based --auth-gen-token)
- server is ctrl-c'ed by admin, sends RESTART
- client calls server_pushed_signal() -> ssl_purge_auth()
forgets username + password, but not token
- server is restarted, same token secret
- client reconnects, sends token, is allowed in
With patch:
- server pushes token (secret-based --auth-gen-token)
- server is ctrl-c'ed by admin, sends RESTART
- client calls server_pushed_signal() -> ssl_purge_auth()
forgets username + password, but not token
- server is restarted, same token secret
- client reconnects...
2022-10-26 14:38:58 SIGUSR1[soft,server-pushed-connection-reset]
received, process restarting
2022-10-26 14:38:58 Restart pause, 5 second(s)
...
Enter Auth Username:
... and asks for a username.
I think this is not what this patch is expected to do, and would irk
a lot of my 2FA users (that are currently happy that I can restart the
openvpn server without them all having to do 2FA again).
There is a theoretical way to avoid this (server needs to send RESTART,P),
but the --explicit-exit-notify code on the server has no way to trigger
this, so it's not helping.
This also affects --management-forget-disconnect (obscure option of
the day), which, I think, should not forget tokens either - but I do
not have a strong opinion here. I think OpenVPN should retain tokens
on disconnect due to "network change" or whatever, but then, I have
no idea when we'd want to use this option in the first place.
The two other places where this is called (SIGUSR1 AUTH FAIL with
"auth-retry interact" and man_forget_passwords() should be fine with
forgetting tokens - I think "AUTH FAIL" already does this (in some
other code path).
I agree with the intent on the patch (Feature-ACK), and would suggest
to call "ssl_clean_auth_token()" directly from "man_forget_passwords()"
instead. This will catch your (valid) use-case without impacting
other users of ssl_purge_auth().
May I also suggest to add a comment to ssl_purge_auth() to clearly
state that "this function never clears auth tokens" so we remember
next time?
thanks,
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
