Return cached result in tls_authentication_status

tls_authentication_status does caching to avoid file I/O more than
every TLS_MULTI_AUTH_STATUS_INTERVAL (10s) per connection. But
counter-intuitively it does not return the cached result but rather
TLS_AUTHENTICATION_UNDEFINED if the cache is not refreshed by the call.

This is workarounded by forcing a refresh in some areas of the code
(latency = 0).

This patch changes the behaviour by always returning the last known
status and only updating the file status when the i/o timeout for the
caches is reached.

The old logic in send_auth_failed is fragile in the sense that if
it is called again while an exit is scheduled it will reset the timer
to 5s again. Since we now always report the status from
tls_authentication_status() instead only every 10s, this caused OpenVPN
to infinitively reset the timer. Fix this by only setting the status
if no exit is scheduled. The function is still called multiple times but
since it is with coarse timer frequency, the 4 extra calls (1 per second)
are better than to add more extra code to avoid these calls.

The patch also changes the DEFINE enum into a real enum.

Patch v2: only update tas_cache_last_udpate when actually updating the cache.
Patch v3: avoid rearming timer

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20210506141259.309741-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22318.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 32e2821d3fb43db4c0f1fcb9d414c3f11a3c8898..c97bc20ba41cd2c7de40b63aaf57b752c85b8eb3 100644 (file)
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2595,7 +2595,7 @@ static const multi_client_connect_handler client_connect_handlers[] = {
 static void
 multi_connection_established(struct multi_context *m, struct multi_instance *mi)
 {
-    if (tls_authentication_status(mi->context.c2.tls_multi, 0)
+    if (tls_authentication_status(mi->context.c2.tls_multi, TLS_MULTI_AUTH_STATUS_INTERVAL)
         != TLS_AUTHENTICATION_SUCCEEDED)
     {
         return;

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 99aaa3e3e886573dc789a160ac5cdd523d962184..ad83c597db7d32844360bbc9eef1f2aa55de30a2 100644 (file)
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -331,10 +331,18 @@ __attribute__ ((format(__printf__, 4, 5)))
 
 /*
  * Send auth failed message from server to client.
+ *
+ * Does nothing if an exit is already scheduled
  */
 void
 send_auth_failed(struct context *c, const char *client_reason)
 {
+    if (event_timeout_defined(&c->c2.scheduled_exit))
+    {
+        msg(D_TLS_DEBUG, "exit already scheduled for context");
+        return;
+    }
+
     struct gc_arena gc = gc_new();
     static const char auth_failed[] = "AUTH_FAILED";
     size_t len;
@@ -851,7 +859,8 @@ process_incoming_push_request(struct context *c)
 {
     int ret = PUSH_MSG_ERROR;
 
-    if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED
+
+    if (tls_authentication_status(c->c2.tls_multi, TLS_MULTI_AUTH_STATUS_INTERVAL) == TLS_AUTHENTICATION_FAILED
         || c->c2.tls_multi->multi_state == CAS_FAILED)
     {
         const char *client_reason = tls_client_reason(c->c2.tls_multi);

diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 4ce6983c511dc91bc4dead7086cd9b43b53671dd..11d4c70404446c234886ef1114128cfa85b73c29 100644 (file)
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -152,6 +152,15 @@ struct auth_deferred_status
     unsigned int auth_control_status;
 };
 
+/* key_state_test_auth_control_file return values, these specify the
+ * current status of a deferred authentication */
+enum auth_deferred_result {
+    ACF_PENDING,      /**< deferred auth still pending */
+    ACF_SUCCEEDED,    /**< deferred auth has suceeded */
+    ACF_DISABLED,     /**< deferred auth is not used */
+    ACF_FAILED        /**< deferred auth has failed */
+};
+
 /**
  * Security parameter state of one TLS and data channel %key session.
  * @ingroup control_processor
@@ -217,7 +226,7 @@ struct key_state
 
 #ifdef ENABLE_MANAGEMENT
     unsigned int mda_key_id;
-    unsigned int mda_status;
+    enum auth_deferred_result mda_status;
 #endif
     time_t acf_last_mod;
 
@@ -552,8 +561,9 @@ struct tls_multi
     char *locked_username;
     struct cert_hash_set *locked_cert_hash_set;
 
-    /* Time of last call to tls_authentication_status */
-    time_t tas_last;
+    /* Time of last when we updated the cached state of
+     * tls_authentication_status deferred files */
+    time_t tas_cache_last_update;
 
     /*
      * An error message to send to client on AUTH_FAILED

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 98ec0e4eafeb0655c8779a21b85616456b28e543..0bcc03f361e05922825555d6ad903d79e2338425 100644 (file)
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -845,13 +845,6 @@ cleanup:
 * user/password authentication.
 *************************************************************************** */
 
-/* key_state_test_auth_control_file return values,
- * NOTE: acf_merge indexing depends on these values */
-#define ACF_UNDEFINED 0
-#define ACF_SUCCEEDED 1
-#define ACF_DISABLED  2
-#define ACF_FAILED    3
-
 void
 auth_set_client_reason(struct tls_multi *multi, const char *client_reason)
 {
@@ -866,7 +859,7 @@ auth_set_client_reason(struct tls_multi *multi, const char *client_reason)
 
 #ifdef ENABLE_MANAGEMENT
 
-static inline unsigned int
+static inline enum auth_deferred_result
 man_def_auth_test(const struct key_state *ks)
 {
     if (management_enable_def_auth(management))
@@ -1041,13 +1034,23 @@ key_state_gen_auth_control_files(struct auth_deferred_status *ads,
     return (acf && apf);
 }
 
-static unsigned int
-key_state_test_auth_control_file(struct auth_deferred_status *ads)
+/**
+ * Checks the auth control status from a file. The function will try 
+ * to read and update the cached status if the status is still pending 
+ * and the parameter cached is false. 
+ * The function returns the most recent known status.
+ *
+ * @param ads       deferred status control structure
+ * @param cached    Return only cached status
+ * @return          ACF_* as per enum
+ */
+static enum auth_deferred_result
+key_state_test_auth_control_file(struct auth_deferred_status *ads, bool cached)
 {
     if (ads->auth_control_file)
     {
         unsigned int ret = ads->auth_control_status;
-        if (ret == ACF_UNDEFINED)
+        if (ret == ACF_PENDING && !cached)
         {
             FILE *fp = fopen(ads->auth_control_file, "r");
             if (fp)
@@ -1084,11 +1087,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
     /* at least one key already failed authentication */
     bool failed_auth = false;
 
-    if (latency && multi->tas_last + latency >= now)
-    {
-        return TLS_AUTHENTICATION_UNDEFINED;
-    }
-    multi->tas_last = now;
+    bool cached = multi->tas_cache_last_update + latency >= now;
 
     for (int i = 0; i < KEY_SCAN_SIZE; ++i)
     {
@@ -1102,11 +1101,11 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
             }
             else
             {
-                unsigned int auth_plugin = ACF_DISABLED;
-                unsigned int auth_script = ACF_DISABLED;
-                unsigned int auth_man = ACF_DISABLED;
-                auth_plugin = key_state_test_auth_control_file(&ks->plugin_auth);
-                auth_script = key_state_test_auth_control_file(&ks->script_auth);
+                enum auth_deferred_result auth_plugin = ACF_DISABLED;
+                enum auth_deferred_result auth_script = ACF_DISABLED;
+                enum auth_deferred_result auth_man = ACF_DISABLED;
+                auth_plugin = key_state_test_auth_control_file(&ks->plugin_auth, cached);
+                auth_script = key_state_test_auth_control_file(&ks->script_auth, cached);
 #ifdef ENABLE_MANAGEMENT
                 auth_man = man_def_auth_test(ks);
 #endif
@@ -1118,9 +1117,9 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
                     ks->authenticated = KS_AUTH_FALSE;
                     failed_auth = true;
                 }
-                else if (auth_plugin == ACF_UNDEFINED
-                         || auth_script == ACF_UNDEFINED
-                         || auth_man == ACF_UNDEFINED)
+                else if (auth_plugin == ACF_PENDING
+                         || auth_script == ACF_PENDING
+                         || auth_man == ACF_PENDING)
                 {
                     if (now < ks->auth_deferred_expire)
                     {
@@ -1140,6 +1139,12 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
         }
     }
 
+    /* we did not rely on a cached result, remember the cache update time */
+    if (!cached)
+    {
+        multi->tas_cache_last_update = now;
+    }
+
 #if 0
     dmsg(D_TLS_ERRORS, "TAS: a=%d s=%d d=%d f=%d", active, success, deferred, failed_auth);
 #endif

diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index 874d2dcba2a06b8d6241bb50b169cf1a025309f0..8de5ee12241a8942784dff4a530de4960a5f4588 100644 (file)
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -69,8 +69,7 @@ enum tls_auth_status
 {
     TLS_AUTHENTICATION_SUCCEEDED=0,
     TLS_AUTHENTICATION_FAILED=1,
-    TLS_AUTHENTICATION_DEFERRED=2,
-    TLS_AUTHENTICATION_UNDEFINED=3
+    TLS_AUTHENTICATION_DEFERRED=2
 };
 
 /**
@@ -85,9 +84,9 @@ enum tls_auth_status
  * from KS_AUTH_DEFERRED to KS_AUTH_FALSE/KS_AUTH_TRUE if the deferred
  * authentication has succeeded after last call.
  *
- * @param   latency     if not null,  return TLS_AUTHENTICATION_UNDEFINED if
- *                      the last call for this multi struct has been less
- *                      than latency seconds ago
+ * @param   latency     if not null, return a cached result from the last
+ *                      call if the last call for this multi struct has 
+ *                      been less than latency seconds ago
  * @param   multi       the tls_multi struct to operate on
  *
  * @return              Current authentication status of the tls_multi