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

Message ID 20200708071750.13213-1-arne@rfc2549.org
State Superseded
Headers show
Series [Openvpn-devel,v2] Make key_state->authenticated more state machine like | expand

Commit Message

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

It also add comments and documents part using the state machine
better.

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/multi.c      | 12 +++++++++---
 src/openvpn/ssl.c        |  7 ++++---
 src/openvpn/ssl_common.h | 14 ++++++++------
 src/openvpn/ssl_verify.c | 18 +++++++-----------
 4 files changed, 28 insertions(+), 23 deletions(-)

Comments

Antonio Quartulli July 7, 2020, 11:54 p.m. UTC | #1
Hi,

On 08/07/2020 09:17, Arne Schwabe wrote:
> This order the states from unauthenticated to authenticated and also
> changes the comparison for KS_AUTH_FALSE from != to >
> 
> It also add comments and documents part using the state machine
> better.
> 
> 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/multi.c      | 12 +++++++++---
>  src/openvpn/ssl.c        |  7 ++++---
>  src/openvpn/ssl_common.h | 14 ++++++++------
>  src/openvpn/ssl_verify.c | 18 +++++++-----------
>  4 files changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index f1ced9b7..f1332c8d 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2352,12 +2352,12 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns
>      if (!IS_SIG(&mi->context) && ((flags & MPP_PRE_SELECT) || ((flags & MPP_CONDITIONAL_PRE_SELECT) && !ANY_OUT(&mi->context))))
>      {
>  #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH)
> -        bool was_authenticated = false;
> +        bool was_unauthenticated = true;
>          struct key_state *ks = NULL;
>          if (mi->context.c2.tls_multi)
>          {
>              ks = &mi->context.c2.tls_multi->session[TM_ACTIVE].key[KS_PRIMARY];
> -            was_authenticated = ks->authenticated;
> +            was_unauthenticated = (ks->authenticated == KS_AUTH_FALSE);
>          }
>  #endif
>  
> @@ -2366,7 +2366,13 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns
>          pre_select(&mi->context);
>  
>  #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH)
> -        if (ks && ks->auth_control_file && ks->auth_deferred && !was_authenticated)
> +        /*
> +         * if we see the state transition from unauthenticated to deferred
> +         * and a auth_control_file, we assume it got just added and add
> +         * inotify watch to that file
> +         */
> +        if (ks && ks->auth_control_file && was_unauthenticated
> +            && (ks->authenticated == KS_AUTH_DEFERRED))
>          {
>              /* watch acf file */
>              long watch_descriptor = inotify_add_watch(m->top.c2.inotify_fd, ks->auth_control_file, IN_CLOSE_WRITE | IN_ONESHOT);
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 71565dd3..94fb0c5b 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);
> @@ -2702,6 +2702,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
>      return true;
>  
>  error:
> +    ks->authenticated = KS_AUTH_FALSE;
>      secure_memzero(ks->key_src, sizeof(*ks->key_src));
>      if (up)
>      {
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index fdf589b5..91e55c7a 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -128,9 +128,14 @@ struct key_source2 {
>  };
>  

I suggest adding a comment on top of this enum declaration. I'd suggest
something like this:

/*
 * The following enum represents the status of the key-state object
 * during its life-cycle. Based on the step taken by the server code,
 * the status will move forward by one (or more) values. This allows
 * us to make more flexible status checks by using the ">" or "<"
 * operator
 */

>  enum ks_auth_state {
> -  KS_AUTH_FALSE,
> -  KS_AUTH_TRUE,
> -  KS_AUTH_DEFERRED
> +  KS_AUTH_FALSE,              /**< Key state is not authenticated  */
> +  KS_AUTH_DEFERRED,           /**< Key state authentication is being deferred,
> +                                * by async auth */
> +  KS_AUTH_TRUE                /**< Key state is authenticated. TLS and user/pass
> +                                * succeeded. This include AUTH_PENDING/OOB
> +                                * authentication as those hold the
> +                                * connection artifically in KS_AUTH_DEFERRED
> +                                */
>  };
>  
>  /**
> @@ -194,8 +199,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 +208,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..990fba99 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;
> @@ -1249,6 +1249,9 @@ verify_user_pass_management(struct tls_session *session,
>  
>  /*
>   * Main username/password verification entry point
> + *
> + * Will set session->ks[KS_PRIMARY].authenticated according to
> + * result of the username/password verifcation
>   */
>  void
>  verify_user_pass(struct user_pass *up, struct tls_multi *multi,
> @@ -1414,17 +1417,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 +1441,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 +1457,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 +1471,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();
>  
> 

The rest looks good!

I don't have a super strong opinion about that comment I proposed, but I
think it can help to better understand how the state enum is used and
how to modify it in the future.

I'd prefer to have it, but in any case:

Acked-by: Antonio Quartulli <a@unstable.cc>

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index f1ced9b7..f1332c8d 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2352,12 +2352,12 @@  multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns
     if (!IS_SIG(&mi->context) && ((flags & MPP_PRE_SELECT) || ((flags & MPP_CONDITIONAL_PRE_SELECT) && !ANY_OUT(&mi->context))))
     {
 #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH)
-        bool was_authenticated = false;
+        bool was_unauthenticated = true;
         struct key_state *ks = NULL;
         if (mi->context.c2.tls_multi)
         {
             ks = &mi->context.c2.tls_multi->session[TM_ACTIVE].key[KS_PRIMARY];
-            was_authenticated = ks->authenticated;
+            was_unauthenticated = (ks->authenticated == KS_AUTH_FALSE);
         }
 #endif
 
@@ -2366,7 +2366,13 @@  multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns
         pre_select(&mi->context);
 
 #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH)
-        if (ks && ks->auth_control_file && ks->auth_deferred && !was_authenticated)
+        /*
+         * if we see the state transition from unauthenticated to deferred
+         * and a auth_control_file, we assume it got just added and add
+         * inotify watch to that file
+         */
+        if (ks && ks->auth_control_file && was_unauthenticated
+            && (ks->authenticated == KS_AUTH_DEFERRED))
         {
             /* watch acf file */
             long watch_descriptor = inotify_add_watch(m->top.c2.inotify_fd, ks->auth_control_file, IN_CLOSE_WRITE | IN_ONESHOT);
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 71565dd3..94fb0c5b 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);
@@ -2702,6 +2702,7 @@  key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
     return true;
 
 error:
+    ks->authenticated = KS_AUTH_FALSE;
     secure_memzero(ks->key_src, sizeof(*ks->key_src));
     if (up)
     {
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index fdf589b5..91e55c7a 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -128,9 +128,14 @@  struct key_source2 {
 };
 
 enum ks_auth_state {
-  KS_AUTH_FALSE,
-  KS_AUTH_TRUE,
-  KS_AUTH_DEFERRED
+  KS_AUTH_FALSE,              /**< Key state is not authenticated  */
+  KS_AUTH_DEFERRED,           /**< Key state authentication is being deferred,
+                                * by async auth */
+  KS_AUTH_TRUE                /**< Key state is authenticated. TLS and user/pass
+                                * succeeded. This include AUTH_PENDING/OOB
+                                * authentication as those hold the
+                                * connection artifically in KS_AUTH_DEFERRED
+                                */
 };
 
 /**
@@ -194,8 +199,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 +208,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..990fba99 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;
@@ -1249,6 +1249,9 @@  verify_user_pass_management(struct tls_session *session,
 
 /*
  * Main username/password verification entry point
+ *
+ * Will set session->ks[KS_PRIMARY].authenticated according to
+ * result of the username/password verifcation
  */
 void
 verify_user_pass(struct user_pass *up, struct tls_multi *multi,
@@ -1414,17 +1417,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 +1441,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 +1457,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 +1471,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();