[Openvpn-devel,3/3] Make key_state->authenticated more state machine like

Message ID 20200707121615.15736-5-arne@rfc2549.org
State Superseded
Headers show
Series
  • [Openvpn-devel] Add file to ignore reformatting changes
Related show

Commit Message

Arne Schwabe July 7, 2020, 12:16 p.m.
This order the states from unauthenticated to authenticated and also
changes the comparison for KS_AUTH_FALSE from != to >

Also remove a now obsolete comment and two obsolete ifdefs. While
keeping the ifdef in ssl_verify would save a few bytes of code,
this is too minor to justify keeping the ifdef

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl.c        |  6 +++---
 src/openvpn/ssl_common.h |  7 ++-----
 src/openvpn/ssl_verify.c | 15 ++++-----------
 3 files changed, 9 insertions(+), 19 deletions(-)

Comments

Antonio Quartulli July 7, 2020, 4:20 p.m. | #1
Hi,

On 07/07/2020 14:16, Arne Schwabe wrote:
> This order the states from unauthenticated to authenticated and also
> changes the comparison for KS_AUTH_FALSE from != to >
> 
> Also remove a now obsolete comment and two obsolete ifdefs. While
> keeping the ifdef in ssl_verify would save a few bytes of code,
> this is too minor to justify keeping the ifdef
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/ssl.c        |  6 +++---
>  src/openvpn/ssl_common.h |  7 ++-----
>  src/openvpn/ssl_verify.c | 15 ++++-----------
>  3 files changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 71565dd3..c73b51c3 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2465,7 +2465,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
>       */
>      if (session->opt->server && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0))
>      {
> -        if (ks->authenticated != KS_AUTH_FALSE)
> +        if (ks->authenticated > KS_AUTH_FALSE)
>          {
>              if (!tls_session_generate_data_channel_keys(session))
>              {
> @@ -2646,7 +2646,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
>      secure_memzero(up, sizeof(*up));
>  
>      /* Perform final authentication checks */
> -    if (ks->authenticated != KS_AUTH_FALSE)
> +    if (ks->authenticated > KS_AUTH_FALSE)
>      {
>          verify_final_auth_checks(multi, session);
>      }
> @@ -2671,7 +2671,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
>       * Call OPENVPN_PLUGIN_TLS_FINAL plugin if defined, for final
>       * veto opportunity over authentication decision.
>       */
> -    if ((ks->authenticated != KS_AUTH_FALSE)
> +    if ((ks->authenticated > KS_AUTH_FALSE)
>          && plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL))
>      {
>          key_state_export_keying_material(&ks->ks_ssl, session);
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index fdf589b5..7d841ffb 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -129,8 +129,8 @@ struct key_source2 {
>  
>  enum ks_auth_state {
>    KS_AUTH_FALSE,

I suggest to add comments here describing how states work and how new
states should be added.

I.e. something like: "any state before or equal to KS_AUTH_FALSE is
considered unauthorized". Not sure the terminology is right, but
something like this would help introducing new states, like Steffan
reported before.

Maybe he has additional comments too.


Regards,

> -  KS_AUTH_TRUE,
> -  KS_AUTH_DEFERRED
> +  KS_AUTH_DEFERRED,
> +  KS_AUTH_TRUE
>  };
>  
>  /**
> @@ -194,8 +194,6 @@ struct key_state
>      enum ks_auth_state authenticated;
>      time_t auth_deferred_expire;
>  
> -#ifdef ENABLE_DEF_AUTH
> -    /* If auth_deferred is true, authentication is being deferred */
>  #ifdef MANAGEMENT_DEF_AUTH
>      unsigned int mda_key_id;
>      unsigned int mda_status;
> @@ -205,7 +203,6 @@ struct key_state
>      time_t acf_last_mod;
>      char *auth_control_file;
>  #endif
> -#endif
>  };
>  
>  /** Control channel wrapping (--tls-auth/--tls-crypt) context */
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index e28f1f3a..6996d430 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -950,7 +950,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
>              if (DECRYPT_KEY_ENABLED(multi, ks))
>              {
>                  active = true;
> -                if (ks->authenticated != KS_AUTH_FALSE)
> +                if (ks->authenticated > KS_AUTH_FALSE)
>                  {
>  #ifdef ENABLE_DEF_AUTH
>                      unsigned int s1 = ACF_DISABLED;
> @@ -1414,17 +1414,10 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
>               */
>              send_push_reply_auth_token(multi);
>          }
> -#ifdef ENABLE_DEF_AUTH
>          msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s",
>              (ks->authenticated == KS_AUTH_DEFERRED) ? "deferred" : "succeeded",
>              up->username,
>              (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : "");
> -#else
> -        msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s",
> -            "succeeded",
> -            up->username,
> -            (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : "");
> -#endif
>      }
>      else
>      {
> @@ -1445,7 +1438,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session)
>      }
>  
>      /* Don't allow the CN to change once it's been locked */
> -    if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cn)
> +    if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cn)
>      {
>          const char *cn = session->common_name;
>          if (cn && strcmp(cn, multi->locked_cn))
> @@ -1461,7 +1454,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session)
>      }
>  
>      /* Don't allow the cert hashes to change once they have been locked */
> -    if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cert_hash_set)
> +    if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cert_hash_set)
>      {
>          const struct cert_hash_set *chs = session->cert_hash_set;
>          if (chs && !cert_hash_compare(chs, multi->locked_cert_hash_set))
> @@ -1475,7 +1468,7 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session)
>      }
>  
>      /* verify --client-config-dir based authentication */
> -    if (ks->authenticated != KS_AUTH_FALSE && session->opt->client_config_dir_exclusive)
> +    if (ks->authenticated > KS_AUTH_FALSE && session->opt->client_config_dir_exclusive)
>      {
>          struct gc_arena gc = gc_new();
>  
>
Steffan Karger July 8, 2020, 7:58 a.m. | #2
Hi,

On 07-07-2020 18:20, Antonio Quartulli wrote:
> On 07/07/2020 14:16, Arne Schwabe wrote:
>> This order the states from unauthenticated to authenticated and also
>> changes the comparison for KS_AUTH_FALSE from != to >
>>
>> Also remove a now obsolete comment and two obsolete ifdefs. While
>> keeping the ifdef in ssl_verify would save a few bytes of code,
>> this is too minor to justify keeping the ifdef
>>
>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>> ---
>>  src/openvpn/ssl.c        |  6 +++---
>>  src/openvpn/ssl_common.h |  7 ++-----
>>  src/openvpn/ssl_verify.c | 15 ++++-----------
>>  3 files changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
>> index 71565dd3..c73b51c3 100644
>> --- a/src/openvpn/ssl.c
>> +++ b/src/openvpn/ssl.c
>> @@ -2465,7 +2465,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
>>       */
>>      if (session->opt->server && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0))
>>      {
>> -        if (ks->authenticated != KS_AUTH_FALSE)
>> +        if (ks->authenticated > KS_AUTH_FALSE)
>>          {
>>              if (!tls_session_generate_data_channel_keys(session))
>>              {
>> @@ -2646,7 +2646,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
>>      secure_memzero(up, sizeof(*up));
>>  
>>      /* Perform final authentication checks */
>> -    if (ks->authenticated != KS_AUTH_FALSE)
>> +    if (ks->authenticated > KS_AUTH_FALSE)
>>      {
>>          verify_final_auth_checks(multi, session);
>>      }
>> @@ -2671,7 +2671,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
>>       * Call OPENVPN_PLUGIN_TLS_FINAL plugin if defined, for final
>>       * veto opportunity over authentication decision.
>>       */
>> -    if ((ks->authenticated != KS_AUTH_FALSE)
>> +    if ((ks->authenticated > KS_AUTH_FALSE)
>>          && plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL))
>>      {
>>          key_state_export_keying_material(&ks->ks_ssl, session);
>> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
>> index fdf589b5..7d841ffb 100644
>> --- a/src/openvpn/ssl_common.h
>> +++ b/src/openvpn/ssl_common.h
>> @@ -129,8 +129,8 @@ struct key_source2 {
>>  
>>  enum ks_auth_state {
>>    KS_AUTH_FALSE,
> 
> I suggest to add comments here describing how states work and how new
> states should be added.
> 
> I.e. something like: "any state before or equal to KS_AUTH_FALSE is
> considered unauthorized". Not sure the terminology is right, but
> something like this would help introducing new states, like Steffan
> reported before.

+1

Don't care much about the terminology. As long as the intention from the
quote from Antonio is immediately obvious from the code.

> Maybe he has additional comments too.

Nope, otherwise I think this looks good.

-Steffan

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 71565dd3..c73b51c3 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2465,7 +2465,7 @@  key_method_2_write(struct buffer *buf, struct tls_session *session)
      */
     if (session->opt->server && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0))
     {
-        if (ks->authenticated != KS_AUTH_FALSE)
+        if (ks->authenticated > KS_AUTH_FALSE)
         {
             if (!tls_session_generate_data_channel_keys(session))
             {
@@ -2646,7 +2646,7 @@  key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
     secure_memzero(up, sizeof(*up));
 
     /* Perform final authentication checks */
-    if (ks->authenticated != KS_AUTH_FALSE)
+    if (ks->authenticated > KS_AUTH_FALSE)
     {
         verify_final_auth_checks(multi, session);
     }
@@ -2671,7 +2671,7 @@  key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
      * Call OPENVPN_PLUGIN_TLS_FINAL plugin if defined, for final
      * veto opportunity over authentication decision.
      */
-    if ((ks->authenticated != KS_AUTH_FALSE)
+    if ((ks->authenticated > KS_AUTH_FALSE)
         && plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL))
     {
         key_state_export_keying_material(&ks->ks_ssl, session);
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index fdf589b5..7d841ffb 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -129,8 +129,8 @@  struct key_source2 {
 
 enum ks_auth_state {
   KS_AUTH_FALSE,
-  KS_AUTH_TRUE,
-  KS_AUTH_DEFERRED
+  KS_AUTH_DEFERRED,
+  KS_AUTH_TRUE
 };
 
 /**
@@ -194,8 +194,6 @@  struct key_state
     enum ks_auth_state authenticated;
     time_t auth_deferred_expire;
 
-#ifdef ENABLE_DEF_AUTH
-    /* If auth_deferred is true, authentication is being deferred */
 #ifdef MANAGEMENT_DEF_AUTH
     unsigned int mda_key_id;
     unsigned int mda_status;
@@ -205,7 +203,6 @@  struct key_state
     time_t acf_last_mod;
     char *auth_control_file;
 #endif
-#endif
 };
 
 /** Control channel wrapping (--tls-auth/--tls-crypt) context */
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index e28f1f3a..6996d430 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -950,7 +950,7 @@  tls_authentication_status(struct tls_multi *multi, const int latency)
             if (DECRYPT_KEY_ENABLED(multi, ks))
             {
                 active = true;
-                if (ks->authenticated != KS_AUTH_FALSE)
+                if (ks->authenticated > KS_AUTH_FALSE)
                 {
 #ifdef ENABLE_DEF_AUTH
                     unsigned int s1 = ACF_DISABLED;
@@ -1414,17 +1414,10 @@  verify_user_pass(struct user_pass *up, struct tls_multi *multi,
              */
             send_push_reply_auth_token(multi);
         }
-#ifdef ENABLE_DEF_AUTH
         msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s",
             (ks->authenticated == KS_AUTH_DEFERRED) ? "deferred" : "succeeded",
             up->username,
             (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : "");
-#else
-        msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for username '%s' %s",
-            "succeeded",
-            up->username,
-            (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN SET]" : "");
-#endif
     }
     else
     {
@@ -1445,7 +1438,7 @@  verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session)
     }
 
     /* Don't allow the CN to change once it's been locked */
-    if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cn)
+    if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cn)
     {
         const char *cn = session->common_name;
         if (cn && strcmp(cn, multi->locked_cn))
@@ -1461,7 +1454,7 @@  verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session)
     }
 
     /* Don't allow the cert hashes to change once they have been locked */
-    if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cert_hash_set)
+    if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cert_hash_set)
     {
         const struct cert_hash_set *chs = session->cert_hash_set;
         if (chs && !cert_hash_compare(chs, multi->locked_cert_hash_set))
@@ -1475,7 +1468,7 @@  verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session)
     }
 
     /* verify --client-config-dir based authentication */
-    if (ks->authenticated != KS_AUTH_FALSE && session->opt->client_config_dir_exclusive)
+    if (ks->authenticated > KS_AUTH_FALSE && session->opt->client_config_dir_exclusive)
     {
         struct gc_arena gc = gc_new();