| Message ID | 20190917121115.13966-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | None | expand |
On 17/09/2019 14:11, Arne Schwabe wrote: > From: Arne Schwabe <arne@openvpn.net> > > This allows OpenVPN 3 core to fall back to the original authentication > method. > > This commit changes man_def_auth_set_client_reason to > auth_set_client_reason since it now used in more contexts. > > Also remove a FIXME about client_reason not being freed, as it is freed > in tls_multi_free with auth_set_client_reason(multi, NULL); > > Patch V4: Rebase on master > Patch V7: Rebase on master > --- > src/openvpn/auth_token.c | 3 +++ > src/openvpn/ssl.c | 6 ++---- > src/openvpn/ssl_common.h | 10 +++++----- > src/openvpn/ssl_verify.c | 8 ++++---- > src/openvpn/ssl_verify.h | 15 ++++++++++----- > 5 files changed, 24 insertions(+), 18 deletions(-) Acked-By: David Sommerseth <davids@openvpn.net> This is identical to the previous version, just reference points for the changes are changed - exactly what I'd expect with a rebase.
Your patch has been applied to the master branch.
This breaks --disable-server (again), but since David sent a patch to
remedy this, I'm merging it nonetheless. I'll merge David's patch right
away and push both together to avoid needless buildbot fails.
The patch itself looks like a very nice approach to the problem with
"how can we communicate authentication fail reasons to the client?" issue
that had a few (fairly intrusiv) patch sets flying around... someone
needs to go through the open patches and see what has become obsolete.
commit fba0e8b8964d5a1f24182ff4c48b18923b5d7ba8
Author: Arne Schwabe
Date: Tue Sep 17 14:11:15 2019 +0200
Sent indication that a session is expired to clients
Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <20190917121115.13966-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18820.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
On 01/10/2019 13:11, Gert Doering wrote: > > The patch itself looks like a very nice approach to the problem with > "how can we communicate authentication fail reasons to the client?" issue > that had a few (fairly intrusiv) patch sets flying around... someone > needs to go through the open patches and see what has become obsolete. This patch should obsolete the older patches resolving the same issue. It's a simpler approach, storing the reason in a session context object which is then picked up when sending AUTH_FAILED. I had one more intrusive change, which changed the internal APIs fairly much, an approach not even I didn't really liked that much. I don't think there were any other proposals on the mailing list. I don't know what the various GUIs does, but they should also now be able to pick up these rejections via the management interface as well; even on 2.4.x clients connecting to a server with this patch.
diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c index 5568a144..1edc8069 100644 --- a/src/openvpn/auth_token.c +++ b/src/openvpn/auth_token.c @@ -15,6 +15,7 @@ #include "push.h" #include "integer.h" #include "ssl.h" +#include "ssl_verify.h" #include <inttypes.h> const char *auth_token_pem_name = "OpenVPN auth-token server key"; @@ -378,6 +379,8 @@ verify_auth_token(struct user_pass *up, struct tls_multi *multi, if (ret & AUTH_TOKEN_EXPIRED) { + /* Tell client that the session token is expired */ + auth_set_client_reason(multi, "SESSION: token expired"); msg(M_INFO, "--auth-token-gen: auth-token from client expired"); } return ret; diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 0d79f91e..4455ebb8 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1349,11 +1349,9 @@ tls_multi_free(struct tls_multi *multi, bool clear) ASSERT(multi); -#ifdef MANAGEMENT_DEF_AUTH - man_def_auth_set_client_reason(multi, NULL); - -#endif #if P2MP_SERVER + auth_set_client_reason(multi, NULL); + free(multi->peer_info); #endif diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index 4f4fd599..406601bc 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -526,16 +526,16 @@ struct tls_multi struct cert_hash_set *locked_cert_hash_set; #ifdef ENABLE_DEF_AUTH - /* - * An error message to send to client on AUTH_FAILED - */ - char *client_reason; - /* Time of last call to tls_authentication_status */ time_t tas_last; #endif #if P2MP_SERVER + /* + * An error message to send to client on AUTH_FAILED + */ + char *client_reason; + /* * A multi-line string of general-purpose info received from peer * over control channel. diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index 5e98fe8a..65188d23 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -803,9 +803,8 @@ cleanup: #define ACF_FAILED 3 #endif -#ifdef MANAGEMENT_DEF_AUTH void -man_def_auth_set_client_reason(struct tls_multi *multi, const char *client_reason) +auth_set_client_reason(struct tls_multi* multi, const char* client_reason) { if (multi->client_reason) { @@ -814,11 +813,12 @@ man_def_auth_set_client_reason(struct tls_multi *multi, const char *client_reaso } if (client_reason && strlen(client_reason)) { - /* FIXME: Last alloc will never be freed */ multi->client_reason = string_alloc(client_reason, NULL); } } +#ifdef MANAGEMENT_DEF_AUTH + static inline unsigned int man_def_auth_test(const struct key_state *ks) { @@ -1022,7 +1022,7 @@ tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id, con if (multi) { int i; - man_def_auth_set_client_reason(multi, client_reason); + auth_set_client_reason(multi, client_reason); for (i = 0; i < KEY_SCAN_SIZE; ++i) { struct key_state *ks = multi->key_scan[i]; diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h index 64f27efb..c54b89a6 100644 --- a/src/openvpn/ssl_verify.h +++ b/src/openvpn/ssl_verify.h @@ -224,18 +224,23 @@ struct x509_track #ifdef MANAGEMENT_DEF_AUTH bool tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id, const bool auth, const char *client_reason); -void man_def_auth_set_client_reason(struct tls_multi *multi, const char *client_reason); +#endif +#ifdef P2MP_SERVER +/** + * Sets the reason why authentication of a client failed. This be will send to the client + * when the AUTH_FAILED message is sent + * An example would be "SESSION: Token expired" + * @param multi The multi tls struct + * @param client_reason The string to send to the client as part of AUTH_FAILED + */ +void auth_set_client_reason(struct tls_multi* multi, const char* client_reason); #endif static inline const char * tls_client_reason(struct tls_multi *multi) { -#ifdef ENABLE_DEF_AUTH return multi->client_reason; -#else - return NULL; -#endif } /** Remove any X509_ env variables from env_set es */
From: Arne Schwabe <arne@openvpn.net> This allows OpenVPN 3 core to fall back to the original authentication method. This commit changes man_def_auth_set_client_reason to auth_set_client_reason since it now used in more contexts. Also remove a FIXME about client_reason not being freed, as it is freed in tls_multi_free with auth_set_client_reason(multi, NULL); Patch V4: Rebase on master Patch V7: Rebase on master --- src/openvpn/auth_token.c | 3 +++ src/openvpn/ssl.c | 6 ++---- src/openvpn/ssl_common.h | 10 +++++----- src/openvpn/ssl_verify.c | 8 ++++---- src/openvpn/ssl_verify.h | 15 ++++++++++----- 5 files changed, 24 insertions(+), 18 deletions(-)