[Openvpn-devel,v7,6/7] Sent indication that a session is expired to clients

Message ID 20190917121115.13966-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • Untitled series #543
Related show

Commit Message

Arne Schwabe Sept. 17, 2019, 12:11 p.m.
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(-)

Comments

David Sommerseth Sept. 18, 2019, 1:58 p.m. | #1
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.
Gert Doering Oct. 1, 2019, 11:11 a.m. | #2
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
David Sommerseth Oct. 1, 2019, 11:44 a.m. | #3
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.

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 */