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