[Openvpn-devel,v2,5/9] Extracting key_state deferred auth status update into function

Message ID 20210520151148.2565578-5-arne@rfc2549.org
State New
Delegated to: Antonio Quartulli
Headers show
Series
  • [Openvpn-devel,v2,1/9] Move auth_token_state from multi to key_state
Related show

Commit Message

Arne Schwabe May 20, 2021, 3:11 p.m.
This extract the update of a deferred key status into into own
function.

Patch v2: Do not ignore auth_deferred_expire. Minor format changes.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl_verify.c | 96 ++++++++++++++++++++++++++--------------
 1 file changed, 62 insertions(+), 34 deletions(-)

Comments

Antonio Quartulli June 14, 2021, 1:06 a.m. | #1
Hi,

On 20/05/2021 17:11, Arne Schwabe wrote:
> This extract the update of a deferred key status into into own
> function.
> 
> Patch v2: Do not ignore auth_deferred_expire. Minor format changes.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/ssl_verify.c | 96 ++++++++++++++++++++++++++--------------
>  1 file changed, 62 insertions(+), 34 deletions(-)
> 
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index 7992a6eb9..455a5cd1b 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -1073,6 +1073,60 @@ key_state_test_auth_control_file(struct auth_deferred_status *ads, bool cached)
>      return ACF_DISABLED;
>  }
>  
> +/**
> + * This method takes a key_state and if updates the state
> + * of the key if it is deferred.
> + * @param cached    If auth control files should be tried to be opened or th
> + *                  cached results should be used
> + * @param ks        The key_state to update
> + */
> +static void
> +update_key_auth_status(bool cached, struct key_state *ks)
> +{
> +    if (ks->authenticated == KS_AUTH_FALSE)
> +    {
> +        return;
> +    }
> +    else
> +    {
> +        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
> +        ASSERT(auth_plugin < 4 && auth_script < 4 && auth_man < 4);
> +
> +        if (auth_plugin == ACF_FAILED || auth_script == ACF_FAILED
> +           || auth_man == ACF_FAILED)
> +        {
> +            ks->authenticated = KS_AUTH_FALSE;
> +            return;
> +        }
> +        else if (auth_plugin == ACF_PENDING || auth_script == ACF_PENDING
> +                 || auth_man == ACF_PENDING)
> +        {
> +            if (now > ks->auth_deferred_expire)

To be 100% semantically the same of the code we have now this comparison
should be '>=' - just for consistency I'd change it.

> +            {
> +                /* Window to authenticate the key has expired, mark
> +                 * the key as unauthenticated */
> +                ks->authenticated = KS_AUTH_FALSE;
> +            }

If the if-condition above is false, we assume that authenticated is
KS_AUTH_DEFERRED. Can we safely rely on this assumption?
I could not find anywhere in the code an explicit link between
KS_AUTH_DEFERRED and ACF_PENDING (actually I could not find anywhere
setting the value ACF_PENDING).


> +        }
> +        else
> +        {
> +            /* all auth states (auth_plugin, auth_script, auth_man)
> +             * are either ACF_DISABLED or ACF_SUCCEDED now, which
> +             * translates to "not checked" or "auth succeeded"
> +             */
> +            ks->authenticated = KS_AUTH_TRUE;
> +        }
> +    }
> +}
> +
> +
>  /**
>   * The minimum times to have passed to update the cache. Older versions
>   * of OpenVPN had code path that did not do any caching, so we start
> @@ -1115,46 +1169,20 @@ tls_authentication_status(struct tls_multi *multi)
>          if (TLS_AUTHENTICATED(multi, ks))
>          {
>              active++;
> +            update_key_auth_status(cached, ks);
> +
>              if (ks->authenticated == KS_AUTH_FALSE)
>              {
>                  failed_auth = true;
>              }
> -            else
> +            else if (ks->authenticated == KS_AUTH_DEFERRED)
>              {
> -                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
> -                ASSERT(auth_plugin < 4 && auth_script < 4 && auth_man < 4);
>  
> -                if (auth_plugin == ACF_FAILED || auth_script == ACF_FAILED
> -                   || auth_man == ACF_FAILED)
> -                {
> -                    ks->authenticated = KS_AUTH_FALSE;
> -                    failed_auth = true;
> -                }
> -                else if (auth_plugin == ACF_PENDING
> -                         || auth_script == ACF_PENDING
> -                         || auth_man == ACF_PENDING)
> -                {
> -                    if (now < ks->auth_deferred_expire)
> -                    {
> -                        deferred = true;
> -                    }
> -                }
> -                else
> -                {
> -                    /* all auth states (auth_plugin, auth_script, auth_man)
> -                     * are either ACF_DISABLED or ACF_SUCCEDED now, which
> -                     * translates to "not checked" or "auth succeeded"
> -                     */
> -                    success = true;
> -                    ks->authenticated = KS_AUTH_TRUE;
> -                }
> +                deferred = true;
> +            }
> +            else if (ks->authenticated == KS_AUTH_TRUE)
> +            {
> +                success = true;
>              }
>          }
>      }
> 

Other that that, the patch looks good and passed all my tests.
So, despite my doubts as reported above, I could not hit any issue after
applying this patch.

Tested with deferred auth, client/server and p2p.


Regards,
Antonio Quartulli June 14, 2021, 1:23 a.m. | #2
Hi,

On 14/06/2021 03:06, Antonio Quartulli wrote:
> Hi,
> 
> On 20/05/2021 17:11, Arne Schwabe wrote:
>> This extract the update of a deferred key status into into own
>> function.
>>
>> Patch v2: Do not ignore auth_deferred_expire. Minor format changes.
>>
>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>> ---
>>  src/openvpn/ssl_verify.c | 96 ++++++++++++++++++++++++++--------------
>>  1 file changed, 62 insertions(+), 34 deletions(-)
>>
>> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
>> index 7992a6eb9..455a5cd1b 100644
>> --- a/src/openvpn/ssl_verify.c
>> +++ b/src/openvpn/ssl_verify.c
>> @@ -1073,6 +1073,60 @@ key_state_test_auth_control_file(struct auth_deferred_status *ads, bool cached)
>>      return ACF_DISABLED;
>>  }
>>  
>> +/**
>> + * This method takes a key_state and if updates the state
>> + * of the key if it is deferred.
>> + * @param cached    If auth control files should be tried to be opened or th
>> + *                  cached results should be used
>> + * @param ks        The key_state to update
>> + */
>> +static void
>> +update_key_auth_status(bool cached, struct key_state *ks)
>> +{
>> +    if (ks->authenticated == KS_AUTH_FALSE)
>> +    {
>> +        return;
>> +    }
>> +    else
>> +    {
>> +        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
>> +        ASSERT(auth_plugin < 4 && auth_script < 4 && auth_man < 4);
>> +
>> +        if (auth_plugin == ACF_FAILED || auth_script == ACF_FAILED
>> +           || auth_man == ACF_FAILED)
>> +        {
>> +            ks->authenticated = KS_AUTH_FALSE;
>> +            return;
>> +        }
>> +        else if (auth_plugin == ACF_PENDING || auth_script == ACF_PENDING
>> +                 || auth_man == ACF_PENDING)
>> +        {
>> +            if (now > ks->auth_deferred_expire)
> 
> To be 100% semantically the same of the code we have now this comparison
> should be '>=' - just for consistency I'd change it.
> 
>> +            {
>> +                /* Window to authenticate the key has expired, mark
>> +                 * the key as unauthenticated */
>> +                ks->authenticated = KS_AUTH_FALSE;
>> +            }
> 
> If the if-condition above is false, we assume that authenticated is
> KS_AUTH_DEFERRED. Can we safely rely on this assumption?
> I could not find anywhere in the code an explicit link between
> KS_AUTH_DEFERRED and ACF_PENDING (actually I could not find anywhere
> setting the value ACF_PENDING).

self-answer: when ks is initialized it is zero'd. ACF_PENDING is zero so
it's implicitly assigned until further change.

For this reason the assumption here is correct.


@Gert: could you apply the '>' -> '>=' ont he fly?


For me the rest looks good.

Acked-by: Antonio Quartulli <antonio@openvpn.net>

Patch

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 7992a6eb9..455a5cd1b 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1073,6 +1073,60 @@  key_state_test_auth_control_file(struct auth_deferred_status *ads, bool cached)
     return ACF_DISABLED;
 }
 
+/**
+ * This method takes a key_state and if updates the state
+ * of the key if it is deferred.
+ * @param cached    If auth control files should be tried to be opened or th
+ *                  cached results should be used
+ * @param ks        The key_state to update
+ */
+static void
+update_key_auth_status(bool cached, struct key_state *ks)
+{
+    if (ks->authenticated == KS_AUTH_FALSE)
+    {
+        return;
+    }
+    else
+    {
+        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
+        ASSERT(auth_plugin < 4 && auth_script < 4 && auth_man < 4);
+
+        if (auth_plugin == ACF_FAILED || auth_script == ACF_FAILED
+           || auth_man == ACF_FAILED)
+        {
+            ks->authenticated = KS_AUTH_FALSE;
+            return;
+        }
+        else if (auth_plugin == ACF_PENDING || auth_script == ACF_PENDING
+                 || auth_man == ACF_PENDING)
+        {
+            if (now > ks->auth_deferred_expire)
+            {
+                /* Window to authenticate the key has expired, mark
+                 * the key as unauthenticated */
+                ks->authenticated = KS_AUTH_FALSE;
+            }
+        }
+        else
+        {
+            /* all auth states (auth_plugin, auth_script, auth_man)
+             * are either ACF_DISABLED or ACF_SUCCEDED now, which
+             * translates to "not checked" or "auth succeeded"
+             */
+            ks->authenticated = KS_AUTH_TRUE;
+        }
+    }
+}
+
+
 /**
  * The minimum times to have passed to update the cache. Older versions
  * of OpenVPN had code path that did not do any caching, so we start
@@ -1115,46 +1169,20 @@  tls_authentication_status(struct tls_multi *multi)
         if (TLS_AUTHENTICATED(multi, ks))
         {
             active++;
+            update_key_auth_status(cached, ks);
+
             if (ks->authenticated == KS_AUTH_FALSE)
             {
                 failed_auth = true;
             }
-            else
+            else if (ks->authenticated == KS_AUTH_DEFERRED)
             {
-                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
-                ASSERT(auth_plugin < 4 && auth_script < 4 && auth_man < 4);
 
-                if (auth_plugin == ACF_FAILED || auth_script == ACF_FAILED
-                   || auth_man == ACF_FAILED)
-                {
-                    ks->authenticated = KS_AUTH_FALSE;
-                    failed_auth = true;
-                }
-                else if (auth_plugin == ACF_PENDING
-                         || auth_script == ACF_PENDING
-                         || auth_man == ACF_PENDING)
-                {
-                    if (now < ks->auth_deferred_expire)
-                    {
-                        deferred = true;
-                    }
-                }
-                else
-                {
-                    /* all auth states (auth_plugin, auth_script, auth_man)
-                     * are either ACF_DISABLED or ACF_SUCCEDED now, which
-                     * translates to "not checked" or "auth succeeded"
-                     */
-                    success = true;
-                    ks->authenticated = KS_AUTH_TRUE;
-                }
+                deferred = true;
+            }
+            else if (ks->authenticated == KS_AUTH_TRUE)
+            {
+                success = true;
             }
         }
     }