[Openvpn-devel,for,2.5/2.6] Purge auth-token as well while purging passwords

From: Selva Nair <selva.nair@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.

Purging caused by --auth-nocache is not affected
(auth-token is retained in that case as it should be).

Use case:
For Pre-Logon access and persistent connections on Windows, use of
"forget-passwords" before disconnect is probably the only way to
ensure that no credentials are left behind. Note that openvpn.exe
continues to run after disconnect in these cases.

Also, the original intent of "forget-passwords" appears to be to
clear all "passwords" that can be used to reconnect.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/ssl.c | 1 +
 1 file changed, 1 insertion(+)

Am 20.10.22 um 00:46 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@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.
> 
> Purging caused by --auth-nocache is not affected
> (auth-token is retained in that case as it should be).
> 
> Use case:
> For Pre-Logon access and persistent connections on Windows, use of
> "forget-passwords" before disconnect is probably the only way to
> ensure that no credentials are left behind. Note that openvpn.exe
> continues to run after disconnect in these cases.
> 
> Also, the original intent of "forget-passwords" appears to be to
> clear all "passwords" that can be used to reconnect.

I had to first figure out what forget-password actually but yes this 
commit is correct for that obscure feature.

Acked-By: Arne Schwabe <arne@rfc2549.org>

Hi,

On Wed, Oct 19, 2022 at 06:46:42PM -0400, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@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

Hi,

Thanks for the detailed analysis!

On Wed, Oct 26, 2022 at 8:45 AM Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Wed, Oct 19, 2022 at 06:46:42PM -0400, selva.nair@gmail.com wrote:
> > From: Selva Nair <selva.nair@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

Hi,

On Wed, Oct 26, 2022 at 10:18:19AM -0400, Selva Nair wrote:
> > 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?

If you have a number of servers under common management, and they share
the auth-token secret key, a token received from "server A" will be valid
on "server B".  I'd consider that a feature :-)

If the token is refused on server B, the client will come back and look
at user+password credentials (which are now flushed, so will ask).


> > 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.

I can see the reasoning for "forget-passwords".

I'm not sure I understand what the intended use of 
--management-forget-disconnect *is* - if it's "disconnected by a button
press", the GUI can send "forget-passwords", and we do not need this
option in the first place.

If it's "disconnected by a network hickup", why would one want to 
flush the token here, which is intended to seamlessly reconnect 
after the hickup?

Is anyone actually *using* --management-forget-disconnect?  Just asking :-)


(This whole token thing is really great when it works, but it is
obvious that it wasn't there from day one, and not all combinations
of use cases, options and "events" seemt o really make sense)

gert

Hi


>
> I'm not sure I understand what the intended use of
> --management-forget-disconnect *is* - if it's "disconnected by a button
> press", the GUI can send "forget-passwords", and we do not need this
> option in the first place.
>
> If it's "disconnected by a network hickup", why would one want to
> flush the token here, which is intended to seamlessly reconnect
> after the hickup?
>

Here "disconnect" does not mean disconnect from the server, but disconnect
the management client from the daemon. I like to refer to it as "detach"
from the management interface. When the client detaches it allows another
"client" (may be an interactive  telnet session) to connect.

So the use case I have is this: the daemon is started by automatic service
and is kept running all the time. But the connection requires interactive
password entry so the daemon is waiting for password via management i/f. A
user connects to the management i/f, say, via telnet, enters password, and
completes the connection. When done, the user can stop the tunnel using
management-hold and SIGHUP and detach from the management.

A second user can now connect to the management interface and restart the
tunnel. --management-forget-disconnect is a safety feature in such cases to
ensure the password entered by the first user was forgotten when the user
detached from the interface. Including any cached token.  Sending
"forget-passwords" would also work in this case, but then the user has to
remember each time to send it before detaching.

The actual use case is for the GUI handling persistent connections. In this
case, you are right, we can send forget-passwords whenever we detach from a
persistent or PLAP connection. If users do not want that behaviour by
default, we can make it a configurable option in the GUI. But then
--management-forget-passwords is such a "user-configurable forget-passwords
on detach".

Still not convincing?

Selva

Hi,

On Wed, Oct 26, 2022 at 11:54:57AM -0400, Selva Nair wrote:
> > I'm not sure I understand what the intended use of
> > --management-forget-disconnect *is* - if it's "disconnected by a button
> > press", the GUI can send "forget-passwords", and we do not need this
> > option in the first place.
> >
> > If it's "disconnected by a network hickup", why would one want to
> > flush the token here, which is intended to seamlessly reconnect
> > after the hickup?
> 
> Here "disconnect" does not mean disconnect from the server, but disconnect
> the management client from the daemon. I like to refer to it as "detach"
> from the management interface. When the client detaches it allows another
> "client" (may be an interactive  telnet session) to connect.

Ah!  I was misunderstanding this option.  Yes, in that case, "forget
everything, including tokens" seems reasonable.

> Still not convincing?

Now it is, thanks :-) - wrong "disconnect".

gert