Hi,
Thanks for the detailed analysis!
On Wed, Oct 26, 2022 at 8:45 AM Gert Doering <g...@greenie.muc.de> wrote:
> 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).
>
I think the intent of the original code is to purge all credentials by
default in case of RESTART --- [RESTART,P] was required to preserve
credentials as you note below. But somehow we did not keep that behaviour
when auth-token was introduced.
But, I agree it may be useful to preserve the token in this case, though it
will fail if restart moves to the next server, isn't it?
> 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.
>
IMO, management-forget-disconnect should forget the token along with
passwords. The patch is meant to address this option and
"forget-passwords". The use case: if you stop the tunnel and put the
connection on management-hold and then detach from the management
interface, another user connecting to it would be able to connect to it
without entering a password. --management-forget-disconnect can protect
against this.
> 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().
>
I'm okay with this. Will send an amended patch.
Selva
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
