[Openvpn-devel] TLS: do not lock empty usernames

  We do not permit username changes on renegotiation (= username is
"locked" after successful initial authentication).

Unfortunately the way this is written this gets in the way of using
auth-user-pass-optional + pushing "auth-token-user" from client-connect
(and most likely also "from management") because we'll lock an empty
username, and on renegotiation, refuse the client with

   TLS Auth Error: username attempted to change from
            '' to 'MyTokenUser' -- tunnel disabled

Fix: extend "is username a valid pointer" to "... and points to a
     non-empty string" before locking.
---
 src/openvpn/ssl_verify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Hi,

On 10/10/2022 09:12, Gert Doering wrote:
> We do not permit username changes on renegotiation (= username is
> "locked" after successful initial authentication).
> 
> Unfortunately the way this is written this gets in the way of using
> auth-user-pass-optional + pushing "auth-token-user" from client-connect
> (and most likely also "from management") because we'll lock an empty
> username, and on renegotiation, refuse the client with
> 
>     TLS Auth Error: username attempted to change from
>              '' to 'MyTokenUser' -- tunnel disabled
> 
> Fix: extend "is username a valid pointer" to "... and points to a
>       non-empty string" before locking.
> ---
>   src/openvpn/ssl_verify.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index 76cb9f19..4206cf9c 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -166,7 +166,7 @@ tls_lock_username(struct tls_multi *multi, const char *username)
>       }
>       else
>       {
> -        if (username)
> +        if (username && *username)

super uber nitpick (bike shadding level):

I think in other places we perform the very same check using the format: 
username[0] instead of *username.

That's because username is a sequence of chars, therefore it is a bit 
more logical for our brains to "check the first character in the 
sequence" rather than "dereference this pointer".

[or if we want to go the clean way, we should use strlen() == 0, but I 
understand that may be overkill]

my 3 cents.

Cheers,

>           {
>               multi->locked_username = string_alloc(username, NULL);
>           }
  

Hi,

On Mon, Oct 10, 2022 at 10:00:37AM +0200, Antonio Quartulli wrote:
> > -        if (username)
> > +        if (username && *username)
> 
> super uber nitpick (bike shadding level):
> 
> I think in other places we perform the very same check using the format: 
> username[0] instead of *username.

I can adjust that in a v2, but first I need to hear what Arne thinks
about this - it's all his code, so I want to be sure that this is not
introducing unexpected side effects.

gert
  

Hi,

On Mon, Oct 10, 2022 at 3:14 AM Gert Doering <gert@greenie.muc.de> wrote:

> We do not permit username changes on renegotiation (= username is
> "locked" after successful initial authentication).
>
> Unfortunately the way this is written this gets in the way of using
> auth-user-pass-optional + pushing "auth-token-user" from client-connect
> (and most likely also "from management") because we'll lock an empty
> username, and on renegotiation, refuse the client with
>

Why is it not okay to support change of username?. I have a situation
where username change looks legitimate:

With our new PLAP (start from login screen) and "persistent connection"
support on Windows,
a connection may be started by one user and then "renegotiated" by another
who may enter
a different username and password from the initial connection (say
auth-nocache or 2FA
is in use).

In this case, the server will disable the tunnel (username tried to
change), the client will retry
asking the user for username/password input again. On inputting the same
credentials a
second time, the connection will succeed. This leads to poor UX.

If we can conjure up usernames (like with empty --> token-user) why not
allow other username
changes too?

Selva
  

Am 19.10.2022 um 01:01 schrieb Selva Nair:
> Hi,
>
> On Mon, Oct 10, 2022 at 3:14 AM Gert Doering <gert@greenie.muc.de> wrote:
>
>     We do not permit username changes on renegotiation (= username is
>     "locked" after successful initial authentication).
>
>     Unfortunately the way this is written this gets in the way of using
>     auth-user-pass-optional + pushing "auth-token-user" from
>     client-connect
>     (and most likely also "from management") because we'll lock an empty
>     username, and on renegotiation, refuse the client with
>
>
> Why is it not okay to support change of username?. I have a situation
> where username change looks legitimate:
>
> With our new PLAP (start from login screen) and "persistent 
> connection" support on Windows,
> a connection may be started by one user and then "renegotiated" by 
> another who may enter
> a different username and password from the initial connection (say 
> auth-nocache or 2FA
> is in use).
>
> In this case, the server will disable the tunnel (username tried to 
> change), the client will retry
> asking the user for username/password input again. On inputting the 
> same credentials a
> second time, the connection will succeed. This leads to poor UX.
>
> If we can conjure up usernames (like with empty --> token-user) why 
> not allow other username
> changes too?

In general the current authentication system in OpenVPN is ill equipped 
to handle them. On renegotiation we only do auth but no read in ccd or 
other other user specific data. So allowing a username change could in 
many instances give the new user the permissions/IP etc of the old user. 
There can be situations where this is okay and we can add options or 
auth results that explicitly allow. In general a username change 
probably leads authorisation problems. For the auth-token-user the idea 
there is that you get a username on the first auth assigned that you 
should use in the future but no actual user change.

Arne
  

On Wed, Oct 19, 2022 at 1:05 AM Arne Schwabe <arne@rfc2549.org> wrote:

>
>
> If we can conjure up usernames (like with empty --> token-user) why not
> allow other username
> changes too?
>
> In general the current authentication system in OpenVPN is ill equipped to
> handle them. On renegotiation we only do auth but no read in ccd or other
> other user specific data. So allowing a username change could in many
> instances give the new user the permissions/IP etc of the old user. There
> can be situations where this is okay and we can add options or auth results
> that explicitly allow. In general a username change probably leads
> authorisation problems. For the auth-token-user the idea there is that you
> get a username on the first auth assigned that you should use in the future
> but no actual user change.
>
That makes a lot of sense. Even I have setups where what is pushed to the
client depends on the username in addition to the commonname.

This has implications when we have interactively authenticated, but long
running connections which multiple users may be able to use though it will
appear to be associated with one user from the server's pov (so-called
Persistent connections in Windows GUI). Same with PLAP.  Requiring a new
tunnel on user name change alleviates this a bit, but still there will be a
duration until next reauth or token expiry when userA is using a tunnel
started by userB.  Instead of wading into an OT discussion, I will raise
this issue elsewhere / a new thread

Selva
  

Hi,

On Mon, Oct 10, 2022 at 09:12:29AM +0200, Gert Doering wrote:
> We do not permit username changes on renegotiation (= username is
> "locked" after successful initial authentication).
> 
> Unfortunately the way this is written this gets in the way of using
> auth-user-pass-optional + pushing "auth-token-user" from client-connect
> (and most likely also "from management") because we'll lock an empty
> username, and on renegotiation, refuse the client with
> 
>    TLS Auth Error: username attempted to change from
>             '' to 'MyTokenUser' -- tunnel disabled
> 
> Fix: extend "is username a valid pointer" to "... and points to a
>      non-empty string" before locking.

FTR, this patch was superseded by Arne's "--override-username" patch
which just now landed in master.

commit ebd433bd1e40917793903f76883d114d820e992d 
Author: Arne Schwabe <arne@rfc2549.org>
Date:   Tue Mar 11 16:59:04 2025 +0100

    Implement override-username


Also FTR, this is also https://github.com/OpenVPN/openvpn/issues/299

gert