| Message ID | 20221010071229.7935-1-gert@greenie.muc.de |
|---|---|
| State | New |
| Delegated to: | Arne Schwabe |
| Headers | show |
| Series | [Openvpn-devel] TLS: do not lock empty usernames | expand |
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
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) { multi->locked_username = string_alloc(username, NULL); }
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(-)