| Message ID | 20220704025840.2558-1-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Fix auth-token usage with management-def-auth | expand |
Looks good. Shortly after my last email about the issue I found this bit of code and also came to the conclusion it was faulty. Thanks for the patch Selva.
Am 04.07.22 um 04:58 schrieb selva.nair@gmail.com: > From: Selva Nair <selva.nair@gmail.com> > > When auth-token verify succeeds during a reauth, other auth > methods (plugin, script, management) are skipped unless > external-auth is in effect (skip_auth gets set to true). > > However, in this case, the status of management-def-auth > (ks->mda_satus) stays at its default value of ACF_PENDING ks->mda_status > and will never change. This causes TLS keys to go out of sync > and an eventual client disconnect. > > Further, a message saying username/password authentication is > "deferred" gets logged which is misleading. > For example: > > test/127.0.0.1:35874 TLS: Username/auth-token authentication > succeeded for username 'test' > > followed by > > test/127.0.0.1:35874 TLS: Username/Password authentication > deferred for username 'test' [CN SET] > > Fix by setting ks->mda_status to ACF_DISABLED, and do not > set ks->authenticated = KS_AUTH_DEFERRED when skip_auth is true. > > Also log a warning message when token is marked as expired on > missing the reneg window. > > Reported by: Connor Edwards <connor.edwards@b2c2.com> > Acked-By: Arne Schwabe <arne@rfc2549.org> Note that you need have management enabled for this bug to trigger. If you go through all the effort to talk to management like this, you probably want to use external-auth anyway. Basically currently AS is the only management+server mode application that active developer use, bugs like this tend to slip. Arne
Hi On Mon, Jul 4, 2022 at 5:50 AM Arne Schwabe <arne@rfc2549.org> wrote: > Am 04.07.22 um 04:58 schrieb selva.nair@gmail.com: > > From: Selva Nair <selva.nair@gmail.com> > > > > When auth-token verify succeeds during a reauth, other auth > > methods (plugin, script, management) are skipped unless > > external-auth is in effect (skip_auth gets set to true). > > > > However, in this case, the status of management-def-auth > > (ks->mda_satus) stays at its default value of ACF_PENDING > > ks->mda_status > > > and will never change. This causes TLS keys to go out of sync > > and an eventual client disconnect. > > > > Further, a message saying username/password authentication is > > "deferred" gets logged which is misleading. > > For example: > > > > test/127.0.0.1:35874 TLS: Username/auth-token authentication > > succeeded for username 'test' > > > > followed by > > > > test/127.0.0.1:35874 TLS: Username/Password authentication > > deferred for username 'test' [CN SET] > > > > Fix by setting ks->mda_status to ACF_DISABLED, and do not > > set ks->authenticated = KS_AUTH_DEFERRED when skip_auth is true. > > > > Also log a warning message when token is marked as expired on > > missing the reneg window. > > > > Reported by: Connor Edwards <connor.edwards@b2c2.com> > > > > Acked-By: Arne Schwabe <arne@rfc2549.org> > > Note that you need have management enabled for this bug to trigger. If > you go through all the effort to talk to management like this, you > probably want to use external-auth anyway. > I agree. This kind of fiddling with flags like mda_status is not clean and easy to break again. I use management-def-auth but do not use auth-gen-token -- instead the management script keeps track of reauth, lifetime of 2FA etc. If I were to "modernize" that setup I would use auth-gen-token with external-auth as well. That said, for 2.5, an easy fix like this is good enough? Selva <div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jul 4, 2022 at 5:50 AM Arne Schwabe <<a href="mailto:arne@rfc2549.org">arne@rfc2549.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Am 04.07.22 um 04:58 schrieb <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>:<br> > From: Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>><br> > <br> > When auth-token verify succeeds during a reauth, other auth<br> > methods (plugin, script, management) are skipped unless<br> > external-auth is in effect (skip_auth gets set to true).<br> > <br> > However, in this case, the status of management-def-auth<br> > (ks->mda_satus) stays at its default value of ACF_PENDING<br> <br> ks->mda_status<br> <br> > and will never change. This causes TLS keys to go out of sync<br> > and an eventual client disconnect.<br> > <br> > Further, a message saying username/password authentication is<br> > "deferred" gets logged which is misleading.<br> > For example:<br> > <br> > test/<a href="http://127.0.0.1:35874" rel="noreferrer" target="_blank">127.0.0.1:35874</a> TLS: Username/auth-token authentication<br> > succeeded for username 'test'<br> > <br> > followed by<br> > <br> > test/<a href="http://127.0.0.1:35874" rel="noreferrer" target="_blank">127.0.0.1:35874</a> TLS: Username/Password authentication<br> > deferred for username 'test' [CN SET]<br> > <br> > Fix by setting ks->mda_status to ACF_DISABLED, and do not<br> > set ks->authenticated = KS_AUTH_DEFERRED when skip_auth is true.<br> > <br> > Also log a warning message when token is marked as expired on<br> > missing the reneg window.<br> > <br> > Reported by: Connor Edwards <<a href="mailto:connor.edwards@b2c2.com" target="_blank">connor.edwards@b2c2.com</a>><br> > <br> <br> Acked-By: Arne Schwabe <<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>><br> <br> Note that you need have management enabled for this bug to trigger. If <br> you go through all the effort to talk to management like this, you <br> probably want to use external-auth anyway.<br></blockquote><div><br></div><div>I agree. This kind of fiddling with flags like mda_status is not clean and easy to break again. I use management-def-auth but do not use auth-gen-token -- instead the management script keeps track of reauth, lifetime of 2FA etc. If I were to "modernize" that setup I would use auth-gen-token with external-auth as well.</div><div><br></div><div>That said, for 2.5, an easy fix like this is good enough?</div><div><br></div><div>Selva</div></div></div>
Sorry for leaving this ACKed-but-unmerged since July. It got heaped-over
by DCO patches...
Anyway. I have no current test rig where I could recreate and test this,
but if Arne as resident management interface expert is happy with this,
this is fine for me. (The comment / message updates are easy to review
and make sense).
I have test compiled it :-) - and lightly tested, knowing that the
interesting code path is not excercised. Just to be sure.
Cherrypicking to 2.5 caused a funny conflict, as the variable is called
"in_renog_time" there, and got renamed to "in_renegotiation_time" somewhere
later... but solved easily enough.
Your patch has been applied to the master and release/2.5 branch.
commit ddbe6a6fc26586d09f5a9105f13124c479b4d993 (master)
commit 5b178f591c882a6600414104a77a9240f7a29331 (release/2.5)
Author: Selva Nair
Date: Sun Jul 3 22:58:40 2022 -0400
Fix auth-token usage with management-def-auth
Signed-off-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <20220704025840.2558-1-selva.nair@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24627.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c index 096edc75..b5f9f6dd 100644 --- a/src/openvpn/auth_token.c +++ b/src/openvpn/auth_token.c @@ -346,20 +346,22 @@ verify_auth_token(struct user_pass *up, struct tls_multi *multi, return 0; } - /* Accept session tokens that not expired are in the acceptable range - * for renogiations */ + /* Accept session tokens only if their timestamp is in the acceptable range + * for renegotiations */ bool in_renegotiation_time = now >= timestamp && now < timestamp + 2 * session->opt->renegotiate_seconds; if (!in_renegotiation_time) { + msg(M_WARN, "Timestamp (%" PRIu64 ") of auth-token is out of the renegotiation window", + timestamp); ret |= AUTH_TOKEN_EXPIRED; } /* Sanity check the initial timestamp */ if (timestamp < timestamp_initial) { - msg(M_WARN, "Initial timestamp (%" PRIu64 " in token from client earlier than " + msg(M_WARN, "Initial timestamp (%" PRIu64 ") in token from client earlier than " "current timestamp %" PRIu64 ". Broken/unsynchronised clock?", timestamp_initial, timestamp); ret |= AUTH_TOKEN_EXPIRED; diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index c01841fa..45eaf8ed 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -1599,7 +1599,14 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, #ifdef ENABLE_MANAGEMENT if (man_def_auth != KMDA_UNDEF) { - ks->authenticated = KS_AUTH_DEFERRED; + if (skip_auth) + { + ks->mda_status = ACF_DISABLED; + } + else + { + ks->authenticated = KS_AUTH_DEFERRED; + } } #endif if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME))