[Openvpn-devel] [PATCH] Remove --disable-def-auth configure argument

[Openvpn-devel] [PATCH] Remove --disable-def-auth configure argument

[Arne Schwabe]

With scripts, plugin and management interface now all supporting
deferred auth, maintaining support of --disbale-def-auth becomes more
of a burden and the few kilobyte in potential binary size do not
outweigh this. Also the code in ssl_verify is hard to hard because
all the ifdefs.

Especially for management interface there are so many features not
directly related to deferred that depend on MANAGEMENT_DEF_AUTH
(like client-kill) that supporting management without deferred auth
is not worth it anymore. And removing this remover a high number of
ifdefs in manage.c/h

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 config-msvc.h            |  1 -
 configure.ac             |  8 -------
 src/openvpn/forward.c    |  4 ----
 src/openvpn/init.c       |  4 ++--
 src/openvpn/manage.c     | 21 -----------------
 src/openvpn/manage.h     | 17 --------------
 src/openvpn/multi.c      | 48 ++++++++++++++++++-------------------
 src/openvpn/multi.h      |  4 ++--
 src/openvpn/openvpn.h    |  2 +-
 src/openvpn/options.c    |  6 +----
 src/openvpn/options.h    |  2 +-
 src/openvpn/push.c       |  2 +-
 src/openvpn/ssl.c        |  4 ++--
 src/openvpn/ssl_common.h |  8 ++-----
 src/openvpn/ssl_verify.c | 51 ++++++++++------------------------------
 src/openvpn/ssl_verify.h |  2 +-
 src/openvpn/syshead.h    | 15 +-----------
 17 files changed, 50 insertions(+), 149 deletions(-)

diff --git a/config-msvc.h b/config-msvc.h
index f199bb2c..6126ac05 100644
--- a/config-msvc.h
+++ b/config-msvc.h
@@ -2,7 +2,6 @@
 
 #define CONFIGURE_DEFINES "N/A"
 
-#define ENABLE_DEF_AUTH 1
 #define ENABLE_PF 1
 #define ENABLE_CRYPTO_OPENSSL 1
 #define ENABLE_DEBUG 1
diff --git a/configure.ac b/configure.ac
index ebb32204..1ab8fe59 100644
--- a/configure.ac
+++ b/configure.ac
@@ -156,13 +156,6 @@ AC_ARG_ENABLE(
 	[enable_iproute2="no"]
 )
 
-AC_ARG_ENABLE(
-	[def-auth],
-	[AS_HELP_STRING([--disable-def-auth], [disable deferred authentication @<:@default=yes@:>@])],
-	,
-	[enable_def_auth="yes"]
-)
-
 AC_ARG_ENABLE(
 	[pf],
 	[AS_HELP_STRING([--disable-pf], [disable internal packet filter @<:@default=yes@:>@])],
@@ -1221,7 +1214,6 @@ test "${enable_debug}" = "yes" && AC_DEFINE([ENABLE_DEBUG], [1], [Enable debuggi
 test "${enable_small}" = "yes" && AC_DEFINE([ENABLE_SMALL], [1], [Enable smaller executable size])
 test "${enable_fragment}" = "yes" && AC_DEFINE([ENABLE_FRAGMENT], [1], [Enable internal fragmentation support])
 test "${enable_port_share}" = "yes" && AC_DEFINE([ENABLE_PORT_SHARE], [1], [Enable TCP Server port sharing])
-test "${enable_def_auth}" = "yes" && AC_DEFINE([ENABLE_DEF_AUTH], [1], [Enable deferred authentication])
 test "${enable_pf}" = "yes" && AC_DEFINE([ENABLE_PF], [1], [Enable internal packet filter])
 test "${enable_strict_options}" = "yes" && AC_DEFINE([ENABLE_STRICT_OPTIONS_CHECK], [1], [Enable strict options check between peers])
 
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 7ed8d0d7..958246c4 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -880,9 +880,7 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo
         if (management)
         {
             management_bytes_in(management, c->c2.buf.len);
-#ifdef MANAGEMENT_DEF_AUTH
             management_bytes_server(management, &c->c2.link_read_bytes, &c->c2.link_write_bytes, &c->c2.mda_context);
-#endif
         }
 #endif
     }
@@ -1642,9 +1640,7 @@ process_outgoing_link(struct context *c)
                 if (management)
                 {
                     management_bytes_out(management, size);
-#ifdef MANAGEMENT_DEF_AUTH
                     management_bytes_server(management, &c->c2.link_read_bytes, &c->c2.link_write_bytes, &c->c2.mda_context);
-#endif
                 }
 #endif
             }
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 034edba0..dd7daa49 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2943,7 +2943,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
 
     to.plugins = c->plugins;
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     to.mda_context = &c->c2.mda_context;
 #endif
 
@@ -4495,7 +4495,7 @@ close_instance(struct context *c)
         /* close TUN/TAP device */
         do_close_tun(c, false);
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
         if (management)
         {
             management_notify_client_close(management, &c->c2.mda_context, NULL);
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index ac142177..17694d04 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -100,7 +100,6 @@ man_help(void)
     msg(M_CLIENT, "pkcs11-id-count        : Get number of available PKCS#11 identities.");
     msg(M_CLIENT, "pkcs11-id-get index    : Get PKCS#11 identity at index.");
 #endif
-#ifdef MANAGEMENT_DEF_AUTH
     msg(M_CLIENT, "client-auth CID KID    : Authenticate client-id/key-id CID/KID (MULTILINE)");
     msg(M_CLIENT, "client-auth-nt CID KID : Authenticate client-id/key-id CID/KID");
     msg(M_CLIENT, "client-deny CID KID R [CR] : Deny auth client-id/key-id CID/KID with log reason");
@@ -111,7 +110,6 @@ man_help(void)
     msg(M_CLIENT, "env-filter [level]     : Set env-var filter level");
 #ifdef MANAGEMENT_PF
     msg(M_CLIENT, "client-pf CID          : Define packet filter for client CID (MULTILINE)");
-#endif
 #endif
     msg(M_CLIENT, "rsa-sig                : Enter a signature in response to >RSA_SIGN challenge");
     msg(M_CLIENT, "                         Enter signature base64 on subsequent lines followed by END");
@@ -483,8 +481,6 @@ man_bytecount_output_client(struct management *man)
     man->connection.bytecount_last_update = now;
 }
 
-#ifdef MANAGEMENT_DEF_AUTH
-
 void
 man_bytecount_output_server(struct management *man,
                             const counter_type *bytes_in_total,
@@ -500,8 +496,6 @@ man_bytecount_output_server(struct management *man,
     mdac->bytecount_last_update = now;
 }
 
-#endif
-
 static void
 man_kill(struct management *man, const char *victim)
 {
@@ -880,10 +874,8 @@ in_extra_reset(struct man_connection *mc, const int mode)
         if (mode != IER_NEW)
         {
             mc->in_extra_cmd = IEC_UNDEF;
-#ifdef MANAGEMENT_DEF_AUTH
             mc->in_extra_cid = 0;
             mc->in_extra_kid = 0;
-#endif
         }
         if (mc->in_extra)
         {
@@ -902,7 +894,6 @@ in_extra_dispatch(struct management *man)
 {
     switch (man->connection.in_extra_cmd)
     {
-#ifdef MANAGEMENT_DEF_AUTH
         case IEC_CLIENT_AUTH:
             if (man->persist.callback.client_auth)
             {
@@ -930,7 +921,6 @@ in_extra_dispatch(struct management *man)
             }
             break;
 
-#endif /* ifdef MANAGEMENT_DEF_AUTH */
 #ifdef MANAGEMENT_PF
         case IEC_CLIENT_PF:
             if (man->persist.callback.client_pf)
@@ -973,8 +963,6 @@ in_extra_dispatch(struct management *man)
     in_extra_reset(&man->connection, IER_RESET);
 }
 
-#ifdef MANAGEMENT_DEF_AUTH
-
 static bool
 parse_cid(const char *str, unsigned long *cid)
 {
@@ -1153,7 +1141,6 @@ man_client_pf(struct management *man, const char *cid_str)
 }
 
 #endif /* MANAGEMENT_PF */
-#endif /* MANAGEMENT_DEF_AUTH */
 
 static void
 man_pk_sig(struct management *man, const char *cmd_name)
@@ -1337,7 +1324,6 @@ man_dispatch_command(struct management *man, struct status_output *so, const cha
     {
         msg(M_CLIENT, "SUCCESS: pid=%d", platform_getpid());
     }
-#ifdef MANAGEMENT_DEF_AUTH
     else if (streq(p[0], "nclients"))
     {
         man_client_n_clients(man);
@@ -1351,7 +1337,6 @@ man_dispatch_command(struct management *man, struct status_output *so, const cha
         }
         man_env_filter(man, level);
     }
-#endif
     else if (streq(p[0], "signal"))
     {
         if (man_need(man, p, 1, 0))
@@ -1551,7 +1536,6 @@ man_dispatch_command(struct management *man, struct status_output *so, const cha
             man_bytecount(man, atoi(p[1]));
         }
     }
-#ifdef MANAGEMENT_DEF_AUTH
     else if (streq(p[0], "client-kill"))
     {
         if (man_need(man, p, 1, MN_AT_LEAST))
@@ -1596,7 +1580,6 @@ man_dispatch_command(struct management *man, struct status_output *so, const cha
         }
     }
 #endif
-#endif /* ifdef MANAGEMENT_DEF_AUTH */
     else if (streq(p[0], "rsa-sig"))
     {
         man_pk_sig(man, "rsa-sig");
@@ -2905,8 +2888,6 @@ management_notify_generic(struct management *man, const char *str)
     msg(M_CLIENT, "%s", str);
 }
 
-#ifdef MANAGEMENT_DEF_AUTH
-
 static void
 man_output_peer_info_env(struct management *man, const struct man_def_auth_context *mdac)
 {
@@ -3025,8 +3006,6 @@ management_learn_addr(struct management *management,
     gc_free(&gc);
 }
 
-#endif /* MANAGEMENT_DEF_AUTH */
-
 void
 management_echo(struct management *man, const char *string, const bool pull)
 {
diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h
index 881bfb14..a3364644 100644
--- a/src/openvpn/manage.h
+++ b/src/openvpn/manage.h
@@ -40,7 +40,6 @@
 /*
  * Management-interface-based deferred authentication
  */
-#ifdef MANAGEMENT_DEF_AUTH
 struct man_def_auth_context {
     unsigned long cid;
 
@@ -53,7 +52,6 @@ struct man_def_auth_context {
 
     time_t bytecount_last_update;
 };
-#endif
 
 /*
  * Manage build-up of command line
@@ -165,7 +163,6 @@ struct management_callback
     void (*delete_event) (void *arg, event_t event);
     int (*n_clients) (void *arg);
     bool (*send_cc_message) (void *arg, const char *message, const char *parameter);
-#ifdef MANAGEMENT_DEF_AUTH
     bool (*kill_by_cid)(void *arg, const unsigned long cid, const char *kill_msg);
     bool (*client_auth) (void *arg,
                          const unsigned long cid,
@@ -178,7 +175,6 @@ struct management_callback
                                  const unsigned long cid,
                                  const char *url);
     char *(*get_peer_info) (void *arg, const unsigned long cid);
-#endif
 #ifdef MANAGEMENT_PF
     bool (*client_pf)(void *arg,
                       const unsigned long cid,
@@ -287,10 +283,8 @@ struct man_connection {
 #define IEC_PK_SIGN     5
     int in_extra_cmd;
     struct buffer_list *in_extra;
-#ifdef MANAGEMENT_DEF_AUTH
     unsigned long in_extra_cid;
     unsigned int in_extra_kid;
-#endif
 #define EKS_UNDEF   0
 #define EKS_SOLICIT 1
 #define EKS_INPUT   2
@@ -339,9 +333,7 @@ struct management *management_init(void);
 #define MF_SIGNAL            (1<<3)
 #define MF_FORGET_DISCONNECT (1<<4)
 #define MF_CONNECT_AS_CLIENT (1<<5)
-#ifdef MANAGEMENT_DEF_AUTH
 #define MF_CLIENT_AUTH       (1<<6)
-#endif
 #ifdef MANAGEMENT_PF
 #define MF_CLIENT_PF         (1<<7)
 #endif
@@ -415,7 +407,6 @@ void management_notify(struct management *man, const char *severity, const char
 
 void management_notify_generic(struct management *man, const char *str);
 
-#ifdef MANAGEMENT_DEF_AUTH
 void management_notify_client_needing_auth(struct management *management,
                                            const unsigned int auth_id,
                                            struct man_def_auth_context *mdac,
@@ -439,8 +430,6 @@ void management_notify_client_cr_response(unsigned mda_key_id,
                                           const struct env_set *es,
                                           const char *response);
 
-#endif /* ifdef MANAGEMENT_DEF_AUTH */
-
 char *management_query_pk_sig(struct management *man, const char *b64_data,
                               const char *algorithm);
 
@@ -478,13 +467,11 @@ management_enable_pf(const struct management *man)
 }
 #endif
 
-#ifdef MANAGEMENT_DEF_AUTH
 static inline bool
 management_enable_def_auth(const struct management *man)
 {
     return man && BOOL_CAST(man->settings.flags & MF_CLIENT_AUTH);
 }
-#endif
 
 /*
  * OpenVPN tells the management layer what state it's in
@@ -582,8 +569,6 @@ management_bytes_in(struct management *man, const int size)
     }
 }
 
-#ifdef MANAGEMENT_DEF_AUTH
-
 void man_bytecount_output_server(struct management *man,
                                  const counter_type *bytes_in_total,
                                  const counter_type *bytes_out_total,
@@ -603,8 +588,6 @@ management_bytes_server(struct management *man,
     }
 }
 
-#endif /* MANAGEMENT_DEF_AUTH */
-
 #endif /* ifdef ENABLE_MANAGEMENT */
 
 /**
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a5862020..9becb2b2 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -69,7 +69,7 @@ id(struct multi_instance *mi)
 }
 #endif
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
 static void
 set_cc_config(struct multi_instance *mi, struct buffer_list *cc_config)
 {
@@ -252,7 +252,7 @@ reap_buckets_per_pass(int n_buckets)
     return constrain_int(n_buckets / REAP_DIVISOR, REAP_MIN, REAP_MAX);
 }
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
 
 static uint32_t
 cid_hash_function(const void *key, uint32_t iv)
@@ -342,7 +342,7 @@ multi_init(struct multi_context *m, struct context *t, bool tcp_mode, int thread
                         mroute_addr_hash_function,
                         mroute_addr_compare_function);
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     m->cid_hash = hash_init(t->options.real_hash_size,
                             0,
                             cid_hash_function,
@@ -592,7 +592,7 @@ multi_client_disconnect_script(struct multi_instance *mi)
         openvpn_run_script(&argv, mi->context.c2.es, 0, "--client-disconnect");
         argv_free(&argv);
     }
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     if (management)
     {
         management_notify_client_close(management, &mi->context.c2.mda_context, mi->context.c2.es);
@@ -637,7 +637,7 @@ multi_close_instance(struct multi_context *m,
         {
             ASSERT(hash_remove(m->iter, &mi->real));
         }
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
         if (mi->did_cid_hash)
         {
             ASSERT(hash_remove(m->cid_hash, &mi->context.c2.mda_context.cid));
@@ -675,7 +675,7 @@ multi_close_instance(struct multi_context *m,
         mbuf_dereference_instance(m->mbuf, mi);
     }
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     set_cc_config(mi, NULL);
 #endif
     if (mi->context.c2.context_auth == CAS_SUCCEEDED)
@@ -731,7 +731,7 @@ multi_uninit(struct multi_context *m)
             hash_free(m->hash);
             hash_free(m->vhash);
             hash_free(m->iter);
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
             hash_free(m->cid_hash);
 #endif
             m->hash = NULL;
@@ -813,7 +813,7 @@ multi_create_instance(struct multi_context *m, const struct mroute_addr *real)
     }
     mi->did_iter = true;
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     do
     {
         mi->context.c2.mda_context.cid = m->cid_counter++;
@@ -944,7 +944,7 @@ multi_print_status(struct multi_context *m, struct status_output *so, const int
                 if (!mi->halt)
                 {
                     status_printf(so, "CLIENT_LIST%c%s%c%s%c%s%c%s%c" counter_format "%c" counter_format "%c%s%c%u%c%s%c"
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
                                   "%lu"
 #else
                                   ""
@@ -959,7 +959,7 @@ multi_print_status(struct multi_context *m, struct status_output *so, const int
                                   sep, time_string(mi->created, 0, false, &gc),
                                   sep, (unsigned int)mi->created,
                                   sep, tls_username(mi->context.c2.tls_multi, false),
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
                                   sep, mi->context.c2.mda_context.cid,
 #else
                                   sep,
@@ -1252,7 +1252,7 @@ multi_learn_in_addr_t(struct multi_context *m,
 
     {
         struct multi_instance *owner = multi_learn_addr(m, mi, &addr, 0);
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
         if (management && owner)
         {
             management_learn_addr(management, &mi->context.c2.mda_context, &addr, primary);
@@ -1285,7 +1285,7 @@ multi_learn_in6_addr(struct multi_context *m,
 
     {
         struct multi_instance *owner = multi_learn_addr(m, mi, &addr, 0);
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
         if (management && owner)
         {
             management_learn_addr(management, &mi->context.c2.mda_context, &addr, primary);
@@ -1716,7 +1716,7 @@ multi_client_connect_mda(struct multi_context *m,
     /* We never return CC_RET_DEFERRED */
     ASSERT(!deferred);
     enum client_connect_return ret = CC_RET_SKIPPED;
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     if (mi->cc_config)
     {
         struct buffer_entry *be;
@@ -1742,7 +1742,7 @@ multi_client_connect_mda(struct multi_context *m,
 
         ret = CC_RET_SUCCEEDED;
     }
-#endif /* ifdef MANAGEMENT_DEF_AUTH */
+#endif /* ifdef ENABLE_MANAGEMENT */
     return ret;
 }
 
@@ -2699,7 +2699,7 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
     update_mstat_n_clients(m->n_clients);
     --mi->n_clients_delta;
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     if (management)
     {
         management_connection_established(management,
@@ -2922,7 +2922,7 @@ multi_schedule_context_wakeup(struct multi_context *m, struct multi_instance *mi
                        compute_wakeup_sigma(&mi->context.c2.timeval));
 }
 
-#if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH)
+#if defined(ENABLE_ASYNC_PUSH)
 static void
 add_inotify_file_watch(struct multi_context *m, struct multi_instance *mi,
                        int inotify_fd, const char *file)
@@ -2946,7 +2946,7 @@ add_inotify_file_watch(struct multi_context *m, struct multi_instance *mi,
         msg(M_NONFATAL | M_ERRNO, "MULTI: inotify_add_watch error");
     }
 }
-#endif /* if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) */
+#endif /* if defined(ENABLE_ASYNC_PUSH) */
 
 /*
  * Figure instance-specific timers, convert
@@ -2962,7 +2962,7 @@ 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)
+#if defined(ENABLE_ASYNC_PUSH)
         bool was_unauthenticated = true;
         struct key_state *ks = NULL;
         if (mi->context.c2.tls_multi)
@@ -2976,7 +2976,7 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns
          * to_link packets (such as ping or TLS control) */
         pre_select(&mi->context);
 
-#if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH)
+#if defined(ENABLE_ASYNC_PUSH)
         /*
          * if we see the state transition from unauthenticated to deferred
          * and an auth_control_file, we assume it got just added and add
@@ -2999,7 +2999,7 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns
             {
                 multi_connection_established(m, mi);
             }
-#if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH)
+#if defined(ENABLE_ASYNC_PUSH)
             if (is_cas_pending(mi->context.c2.context_auth)
                 && mi->client_connect_defer_state.deferred_ret_file)
             {
@@ -3111,7 +3111,7 @@ multi_process_float(struct multi_context *m, struct multi_instance *mi)
     ASSERT(hash_add(m->hash, &mi->real, mi, false));
     ASSERT(hash_add(m->iter, &mi->real, mi, false));
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     ASSERT(hash_add(m->cid_hash, &mi->context.c2.mda_context.cid, mi, true));
 #endif
 
@@ -3885,7 +3885,7 @@ management_delete_event(void *arg, event_t event)
 
 #endif /* ifdef ENABLE_MANAGEMENT */
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
 
 static struct multi_instance *
 lookup_by_cid(struct multi_context *m, const unsigned long cid)
@@ -3999,7 +3999,7 @@ management_get_peer_info(void *arg, const unsigned long cid)
     return ret;
 }
 
-#endif /* ifdef MANAGEMENT_DEF_AUTH */
+#endif /* ifdef ENABLE_MANAGEMENT */
 
 #ifdef MANAGEMENT_PF
 static bool
@@ -4040,12 +4040,10 @@ init_management_callback_multi(struct multi_context *m)
         cb.kill_by_addr = management_callback_kill_by_addr;
         cb.delete_event = management_delete_event;
         cb.n_clients = management_callback_n_clients;
-#ifdef MANAGEMENT_DEF_AUTH
         cb.kill_by_cid = management_kill_by_cid;
         cb.client_auth = management_client_auth;
         cb.client_pending_auth = management_client_pending_auth;
         cb.get_peer_info = management_get_peer_info;
-#endif
 #ifdef MANAGEMENT_PF
         cb.client_pf = management_client_pf;
 #endif
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 40e808ab..7669508c 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -123,7 +123,7 @@ struct multi_instance {
 
     bool did_real_hash;
     bool did_iter;
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     bool did_cid_hash;
     struct buffer_list *cc_config;
 #endif
@@ -185,7 +185,7 @@ struct multi_context {
     int status_file_version;
     int n_clients; /* current number of authenticated clients */
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     struct hash *cid_hash;
     unsigned long cid_counter;
 #endif
diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index a7b59774..4ca89ba9 100644
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -479,7 +479,7 @@ struct context_2
     struct pf_context pf;
 #endif
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     struct man_def_auth_context mda_context;
 #endif
 
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 4e19d7cb..21f8d494 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -390,11 +390,9 @@ static const char usage_message[] =
     "--management-client-group g : When management interface is a unix socket, only\n"
     "                              allow connections from group g.\n"
 #endif
-#ifdef MANAGEMENT_DEF_AUTH
     "--management-client-auth : gives management interface client the responsibility\n"
     "                           to authenticate clients after their client certificate\n"
     "			      has been verified.\n"
-#endif
 #ifdef MANAGEMENT_PF
     "--management-client-pf : management interface clients must specify a packet\n"
     "                         filter file for each connecting client.\n"
@@ -5438,14 +5436,12 @@ add_option(struct options *options,
         options->management_flags |= MF_EXTERNAL_CERT;
         options->management_certificate = p[1];
     }
-#endif /* ifdef ENABLE_MANAGEMENT */
-#ifdef MANAGEMENT_DEF_AUTH
     else if (streq(p[0], "management-client-auth") && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->management_flags |= MF_CLIENT_AUTH;
     }
-#endif
+#endif /* ifdef ENABLE_MANAGEMENT */
 #ifdef MANAGEMENT_PF
     else if (streq(p[0], "management-client-pf") && !p[1])
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 5d977793..5b6d9441 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -722,7 +722,7 @@ struct options
 #define PLUGIN_OPTION_LIST(opt) (NULL)
 #endif
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
 #define MAN_CLIENT_AUTH_ENABLED(opt) ((opt)->management_flags & MF_CLIENT_AUTH)
 #else
 #define MAN_CLIENT_AUTH_ENABLED(opt) (false)
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 17bba948..19004077 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -219,7 +219,7 @@ receive_cr_response(struct context *c, const struct buffer *buffer)
     {
         m = BSTR(&buf);
     }
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
     struct man_def_auth_context *mda = session->opt->mda_context;
     struct env_set *es = session->opt->es;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 87b51d96..fb1edd6e 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -937,7 +937,7 @@ key_state_init(struct tls_session *session, struct key_state *ks)
 
     ks->crypto_options.pid_persist = NULL;
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     ks->mda_key_id = session->opt->mda_context->mda_key_id_counter++;
 #endif
 }
@@ -1021,7 +1021,7 @@ tls_session_user_pass_enabled(struct tls_session *session)
 {
     return (session->opt->auth_user_pass_verify_script
             || plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY)
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
             || management_enable_def_auth(management)
 #endif
             );
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 53f74cac..810aba95 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -206,15 +206,13 @@ struct key_state
     enum ks_auth_state authenticated;
     time_t auth_deferred_expire;
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     unsigned int mda_key_id;
     unsigned int mda_status;
 #endif
-#ifdef PLUGIN_DEF_AUTH
     unsigned int auth_control_status;
     time_t acf_last_mod;
     char *auth_control_file;
-#endif
 };
 
 /** Control channel wrapping (--tls-auth/--tls-crypt) context */
@@ -353,7 +351,7 @@ struct tls_options
 #define SSLF_TLS_VERSION_MAX_MASK     0xF  /* (uses bit positions 10 to 13) */
     unsigned int ssl_flags;
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     struct man_def_auth_context *mda_context;
 #endif
 
@@ -536,10 +534,8 @@ struct tls_multi
     char *locked_username;
     struct cert_hash_set *locked_cert_hash_set;
 
-#ifdef ENABLE_DEF_AUTH
     /* Time of last call to tls_authentication_status */
     time_t tas_last;
-#endif
 
     /*
      * 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 2d7abdde..acc788fc 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -829,14 +829,12 @@ cleanup:
 * user/password authentication.
 *************************************************************************** */
 
-#ifdef ENABLE_DEF_AUTH
 /* 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
-#endif
 
 void
 auth_set_client_reason(struct tls_multi *multi, const char *client_reason)
@@ -852,7 +850,7 @@ auth_set_client_reason(struct tls_multi *multi, const char *client_reason)
     }
 }
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
 
 static inline unsigned int
 man_def_auth_test(const struct key_state *ks)
@@ -866,9 +864,8 @@ man_def_auth_test(const struct key_state *ks)
         return ACF_DISABLED;
     }
 }
-#endif /* ifdef MANAGEMENT_DEF_AUTH */
+#endif /* ifdef ENABLE_MANAGEMENT */
 
-#ifdef PLUGIN_DEF_AUTH
 
 /*
  * auth_control_file functions
@@ -931,8 +928,6 @@ key_state_test_auth_control_file(struct key_state *ks)
     return ACF_DISABLED;
 }
 
-#endif /* ifdef PLUGIN_DEF_AUTH */
-
 /*
  * Return current session authentication state.  Return
  * value is TLS_AUTHENTICATION_x.
@@ -945,7 +940,6 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
     bool success = false;
     bool active = false;
 
-#ifdef ENABLE_DEF_AUTH
     static const unsigned char acf_merge[] =
     {
         ACF_UNDEFINED, /* s1=ACF_UNDEFINED s2=ACF_UNDEFINED */
@@ -965,19 +959,16 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
         ACF_FAILED,  /* s1=ACF_FAILED    s2=ACF_DISABLED */
         ACF_FAILED   /* s1=ACF_FAILED    s2=ACF_FAILED */
     };
-#endif /* ENABLE_DEF_AUTH */
 
     if (multi)
     {
         int i;
 
-#ifdef ENABLE_DEF_AUTH
         if (latency && multi->tas_last && multi->tas_last + latency >= now)
         {
             return TLS_AUTHENTICATION_UNDEFINED;
         }
         multi->tas_last = now;
-#endif /* ENABLE_DEF_AUTH */
 
         for (i = 0; i < KEY_SCAN_SIZE; ++i)
         {
@@ -987,15 +978,12 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
                 active = true;
                 if (ks->authenticated > KS_AUTH_FALSE)
                 {
-#ifdef ENABLE_DEF_AUTH
                     unsigned int s1 = ACF_DISABLED;
                     unsigned int s2 = ACF_DISABLED;
-#ifdef PLUGIN_DEF_AUTH
                     s1 = key_state_test_auth_control_file(ks);
-#endif /* PLUGIN_DEF_AUTH */
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
                     s2 = man_def_auth_test(ks);
-#endif /* MANAGEMENT_DEF_AUTH */
+#endif
                     ASSERT(s1 < 4 && s2 < 4);
                     switch (acf_merge[(s1<<2) + s2])
                     {
@@ -1019,9 +1007,6 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
                         default:
                             ASSERT(0);
                     }
-#else /* !ENABLE_DEF_AUTH */
-                    success = true;
-#endif /* ENABLE_DEF_AUTH */
                 }
             }
         }
@@ -1045,7 +1030,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
     }
 }
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
 /*
  * For deferred auth, this is where the management interface calls (on server)
  * to indicate auth failure/success.
@@ -1070,7 +1055,7 @@ tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id, con
     }
     return ret;
 }
-#endif /* ifdef MANAGEMENT_DEF_AUTH */
+#endif /* ifdef ENABLE_MANAGEMENT */
 
 
 /* ****************************************************************************
@@ -1159,14 +1144,11 @@ verify_user_pass_plugin(struct tls_session *session, struct tls_multi *multi,
                         const struct user_pass *up)
 {
     int retval = OPENVPN_PLUGIN_FUNC_ERROR;
-#ifdef PLUGIN_DEF_AUTH
     struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
-#endif
 
     /* set password in private env space */
     setenv_str(session->opt->es, "password", up->password);
 
-#ifdef PLUGIN_DEF_AUTH
     /* generate filename for deferred auth control file */
     if (!key_state_gen_auth_control_file(ks, session->opt))
     {
@@ -1174,18 +1156,15 @@ verify_user_pass_plugin(struct tls_session *session, struct tls_multi *multi,
             "could not create deferred auth control file", __func__);
         return retval;
     }
-#endif
 
     /* call command */
     retval = plugin_call(session->opt->plugins, OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, NULL, NULL, session->opt->es);
 
-#ifdef PLUGIN_DEF_AUTH
     /* purge auth control filename (and file itself) for non-deferred returns */
     if (retval != OPENVPN_PLUGIN_FUNC_DEFERRED)
     {
         key_state_rm_auth_control_file(ks);
     }
-#endif
 
     setenv_del(session->opt->es, "password");
 
@@ -1193,9 +1172,9 @@ verify_user_pass_plugin(struct tls_session *session, struct tls_multi *multi,
 }
 
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
 /*
- * MANAGEMENT_DEF_AUTH internal ssl_verify.c status codes
+ * management deferred internal ssl_verify.c status codes
  */
 #define KMDA_ERROR   0
 #define KMDA_SUCCESS 1
@@ -1224,7 +1203,7 @@ verify_user_pass_management(struct tls_session *session,
 
     return retval;
 }
-#endif /* ifdef MANAGEMENT_DEF_AUTH */
+#endif /* ifdef ENABLE_MANAGEMENT */
 
 static bool
 set_verify_user_pass_env(struct user_pass *up, struct tls_multi *multi,
@@ -1269,7 +1248,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
     bool s2 = true;
     struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
 
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
     int man_def_auth = KMDA_UNDEF;
 
     if (management_enable_def_auth(management))
@@ -1336,7 +1315,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
     /* call plugin(s) and/or script */
     if (!skip_auth)
     {
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
         if (man_def_auth==KMDA_DEF)
         {
             man_def_auth = verify_user_pass_management(session, multi, up);
@@ -1364,23 +1343,19 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
     }
     /* auth succeeded? */
     if ((s1 == OPENVPN_PLUGIN_FUNC_SUCCESS
-#ifdef PLUGIN_DEF_AUTH
          || s1 == OPENVPN_PLUGIN_FUNC_DEFERRED
-#endif
          ) && s2
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
         && man_def_auth != KMDA_ERROR
 #endif
         && tls_lock_username(multi, up->username))
     {
         ks->authenticated = KS_AUTH_TRUE;
-#ifdef PLUGIN_DEF_AUTH
         if (s1 == OPENVPN_PLUGIN_FUNC_DEFERRED)
         {
             ks->authenticated = KS_AUTH_DEFERRED;
         }
-#endif
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
         if (man_def_auth != KMDA_UNDEF)
         {
             ks->authenticated = KS_AUTH_DEFERRED;
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index b1ced956..d913f102 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -221,7 +221,7 @@ struct x509_track
 /*
  * TODO: document
  */
-#ifdef MANAGEMENT_DEF_AUTH
+#ifdef ENABLE_MANAGEMENT
 bool tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id, const bool auth, const char *client_reason);
 
 #endif
diff --git a/src/openvpn/syshead.h b/src/openvpn/syshead.h
index 8342eae0..2ad5afc2 100644
--- a/src/openvpn/syshead.h
+++ b/src/openvpn/syshead.h
@@ -530,19 +530,6 @@ socket_defined(const socket_descriptor_t sd)
 #define PORT_SHARE 0
 #endif
 
-/*
- * Enable deferred authentication?
- */
-#if defined(ENABLE_DEF_AUTH) && defined(ENABLE_PLUGIN)
-#define PLUGIN_DEF_AUTH
-#endif
-#if defined(ENABLE_DEF_AUTH) && defined(ENABLE_MANAGEMENT)
-#define MANAGEMENT_DEF_AUTH
-#endif
-#if !defined(PLUGIN_DEF_AUTH) && !defined(MANAGEMENT_DEF_AUTH)
-#undef ENABLE_DEF_AUTH
-#endif
-
 #ifdef ENABLE_CRYPTO_MBEDTLS
 #define ENABLE_PREDICTION_RESISTANCE
 #endif /* ENABLE_CRYPTO_MBEDTLS */
@@ -553,7 +540,7 @@ socket_defined(const socket_descriptor_t sd)
 #if defined(ENABLE_PF) && defined(ENABLE_PLUGIN) && defined(HAVE_STAT)
 #define PLUGIN_PF
 #endif
-#if defined(ENABLE_PF) && defined(MANAGEMENT_DEF_AUTH)
+#if defined(ENABLE_PF) && defined(ENABLE_MANAGEMENT)
 #define MANAGEMENT_PF
 #endif
 #if !defined(PLUGIN_PF) && !defined(MANAGEMENT_PF)
-- 
2.26.2

[Openvpn-devel] [PATCH 2/8] Replace key_scan array of static points with inline function

[Arne Schwabe]

The key_scan array is an array that is setup as a reference to members
of itself that have static offsets. Replace this pointer indirection
with an inline function. This has also the advantage that the compiler
can inline the function and just just a direct offset into the struct.

Replacing the implicit indirection with the pointer array with an
explicit indirection with the inline function also makes the code a
bit easier to follow.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/ssl.c        | 20 +++++++-------------
 src/openvpn/ssl_common.h | 26 +++++++++++++++++++++-----
 src/openvpn/ssl_verify.c |  4 ++--
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index fb1edd6e..618cc9cc 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -832,7 +832,7 @@ print_key_id(struct tls_multi *multi, struct gc_arena *gc)
 
     for (int i = 0; i < KEY_SCAN_SIZE; ++i)
     {
-        struct key_state *ks = multi->key_scan[i];
+        struct key_state *ks = get_key_scan(multi, i);
         buf_printf(&out, " [key#%d state=%s id=%d sid=%s]", i,
                    state_name(ks->state), ks->key_id,
                    session_id_print(&ks->session_id_remote, gc));
@@ -1229,12 +1229,6 @@ tls_multi_init(struct tls_options *tls_options)
     /* get command line derived options */
     ret->opt = *tls_options;
 
-    /* set up list of keys to be scanned by data channel encrypt and decrypt routines */
-    ASSERT(SIZE(ret->key_scan) == 3);
-    ret->key_scan[0] = &ret->session[TM_ACTIVE].key[KS_PRIMARY];
-    ret->key_scan[1] = &ret->session[TM_ACTIVE].key[KS_LAME_DUCK];
-    ret->key_scan[2] = &ret->session[TM_LAME_DUCK].key[KS_LAME_DUCK];
-
     /* By default not use P_DATA_V2 */
     ret->use_peer_id = false;
 
@@ -3212,9 +3206,9 @@ tls_multi_process(struct tls_multi *multi,
      */
     if (error)
     {
-        for (int i = 0; i < (int) SIZE(multi->key_scan); ++i)
+        for (int i = 0; i < KEY_SCAN_SIZE; ++i)
         {
-            if (multi->key_scan[i]->state >= S_ACTIVE)
+            if (get_key_scan(multi, i)->state >= S_ACTIVE)
             {
                 goto nohard;
             }
@@ -3229,9 +3223,9 @@ nohard:
         const int throw_level = GREMLIN_CONNECTION_FLOOD_LEVEL(multi->opt.gremlin);
         if (throw_level)
         {
-            for (int i = 0; i < (int) SIZE(multi->key_scan); ++i)
+            for (int i = 0; i < KEY_SCAN_SIZE; ++i)
             {
-                if (multi->key_scan[i]->state >= throw_level)
+                if (get_key_scan(multi, i)->state >= throw_level)
                 {
                     ++multi->n_hard_errors;
                     ++multi->n_soft_errors;
@@ -3269,7 +3263,7 @@ handle_data_channel_packet(struct tls_multi *multi,
     /* data channel packet */
     for (int i = 0; i < KEY_SCAN_SIZE; ++i)
     {
-        struct key_state *ks = multi->key_scan[i];
+        struct key_state *ks = get_key_scan(multi, i);
 
         /*
          * This is the basic test of TLS state compatibility between a local OpenVPN
@@ -3878,7 +3872,7 @@ tls_pre_encrypt(struct tls_multi *multi,
     struct key_state *ks_select = NULL;
     for (int i = 0; i < KEY_SCAN_SIZE; ++i)
     {
-        struct key_state *ks = multi->key_scan[i];
+        struct key_state *ks = get_key_scan(multi, i);
         if (ks->state >= S_ACTIVE
             && (ks->authenticated == KS_AUTH_TRUE)
             && ks->crypto_options.key_ctx_bi.initialized
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 810aba95..c07c58ac 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -501,11 +501,6 @@ struct tls_multi
     /* const options and config info */
     struct tls_options opt;
 
-    struct key_state *key_scan[KEY_SCAN_SIZE];
-    /**< List of \c key_state objects in the
-     *   order they should be scanned by data
-     *   channel modules. */
-
     /*
      * used by tls_pre_encrypt to communicate the encrypt key
      * to tls_post_encrypt()
@@ -585,4 +580,25 @@ struct tls_multi
      *   sessions with the remote peer. */
 };
 
+/**  gets an item  of \c key_state objects in the
+ *   order they should be scanned by data
+ *   channel modules. */
+static inline struct key_state *
+get_key_scan(struct tls_multi *multi, int index)
+{
+    switch (index)
+    {
+    case 0:
+        return &multi->session[TM_ACTIVE].key[KS_PRIMARY];
+    case 1:
+        return &multi->session[TM_ACTIVE].key[KS_LAME_DUCK];
+    case 2:
+        return &multi->session[TM_LAME_DUCK].key[KS_LAME_DUCK];
+    default:
+        ASSERT(false);
+    }
+
+};
+
+
 #endif /* SSL_COMMON_H_ */
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index acc788fc..862a6f56 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -972,7 +972,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
 
         for (i = 0; i < KEY_SCAN_SIZE; ++i)
         {
-            struct key_state *ks = multi->key_scan[i];
+            struct key_state *ks = get_key_scan(multi, i);
             if (DECRYPT_KEY_ENABLED(multi, ks))
             {
                 active = true;
@@ -1045,7 +1045,7 @@ tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id, con
         auth_set_client_reason(multi, client_reason);
         for (i = 0; i < KEY_SCAN_SIZE; ++i)
         {
-            struct key_state *ks = multi->key_scan[i];
+            struct key_state *ks = get_key_scan(multi, i);
             if (ks->mda_key_id == mda_key_id)
             {
                 ks->mda_status = auth ? ACF_SUCCEEDED : ACF_FAILED;
-- 
2.26.2

[Openvpn-devel] [PATCH 3/8] Add more documentation about our internal TLS functions

[Arne Schwabe]

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/ssl.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 618cc9cc..98ce38f9 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1109,7 +1109,10 @@ tls_session_init(struct tls_multi *multi, struct tls_session *session)
  * @param session      - A pointer to the \c tls_session structure to be
  *                       cleaned up.
  * @param clear        - Whether the memory allocated for the \a session
- *                       object should be overwritten with 0s.
+ *                       object should be overwritten with 0s. This
+ *                       implicitly sets many states to 0/false,
+ *                       e.g. the validity of the keys in the structure
+ *
  */
 static void
 tls_session_free(struct tls_session *session, bool clear)
@@ -1118,6 +1121,9 @@ tls_session_free(struct tls_session *session, bool clear)
 
     for (size_t i = 0; i < KS_SIZE; ++i)
     {
+        /* we don't need clear=true for this call since
+         * the structs are part of session and get cleared
+         * as part of session */
         key_state_free(&session->key[i], false);
     }
 
-- 
2.26.2

[Openvpn-devel] [PATCH 4/8] Improve keys out of sync message

[Arne Schwabe]

The current message basically lacks the information to actually figure
out why the keys are out of sync. This adds the missing information to
that diagnostic message.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/ssl.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 98ce38f9..79ad322a 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -763,6 +763,22 @@ state_name(int state)
     }
 }
 
+static const char *
+ks_auth_name(enum ks_auth_state auth)
+{
+    switch(auth)
+    {
+    case KS_AUTH_TRUE:
+        return "KS_AUTH_TRUE";
+    case KS_AUTH_DEFERRED:
+        return "KS_AUTH_DEFERRED";
+    case KS_AUTH_FALSE:
+        return "KS_AUTH_FALSE";
+    default:
+        return "KS_????";
+    }
+}
+
 static const char *
 packet_opcode_name(int op)
 {
@@ -833,8 +849,9 @@ print_key_id(struct tls_multi *multi, struct gc_arena *gc)
     for (int i = 0; i < KEY_SCAN_SIZE; ++i)
     {
         struct key_state *ks = get_key_scan(multi, i);
-        buf_printf(&out, " [key#%d state=%s id=%d sid=%s]", i,
-                   state_name(ks->state), ks->key_id,
+        buf_printf(&out, " [key#%d state=%s auth=%s id=%d sid=%s]", i,
+                   state_name(ks->state), ks_auth_name(ks->authenticated),
+                   ks->key_id,
                    session_id_print(&ks->session_id_remote, gc));
     }
 
@@ -3331,8 +3348,10 @@ handle_data_channel_packet(struct tls_multi *multi,
     }
 
     msg(D_TLS_ERRORS,
-        "TLS Error: local/remote TLS keys are out of sync: %s [%d]",
-        print_link_socket_actual(from, &gc), key_id);
+        "TLS Error: local/remote TLS keys are out of sync: %s "
+        "(received key id: %d, known key ids: %s)",
+        print_link_socket_actual(from, &gc), key_id,
+        print_key_id(multi, &gc));
 
 done:
     tls_clear_error();
-- 
2.26.2

[Openvpn-devel] [PATCH 5/8] Clean up tls_authentication_status and document it

[Arne Schwabe]

The gain of the used optimisation approach of using a array with a
calculated index in favour of simple ifs is questionable with modern
compilers and the readability of the function suffers.

Also change the return type from simple int to an enum and add comments
and doxygen documentation.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/push.c       |   3 +-
 src/openvpn/ssl.c        |   2 +-
 src/openvpn/ssl_verify.c | 108 +++++++++++++++------------------------
 src/openvpn/ssl_verify.h |  35 +++++++++----
 4 files changed, 70 insertions(+), 78 deletions(-)

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 19004077..26a6201f 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -736,7 +736,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 || c->c2.context_auth == CAS_FAILED)
+    if ((c->c2.tls_multi && tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED)
+        || c->c2.context_auth == CAS_FAILED)
     {
         const char *client_reason = tls_client_reason(c->c2.tls_multi);
         send_auth_failed(c, client_reason);
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 79ad322a..e59dba31 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -3196,7 +3196,7 @@ tls_multi_process(struct tls_multi *multi,
 
     update_time();
 
-    int tas = tls_authentication_status(multi, TLS_MULTI_AUTH_STATUS_INTERVAL);
+    enum tls_auth_status tas = tls_authentication_status(multi, TLS_MULTI_AUTH_STATUS_INTERVAL);
 
     /*
      * If lame duck session expires, kill it.
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 862a6f56..4172e2fd 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -928,86 +928,56 @@ key_state_test_auth_control_file(struct key_state *ks)
     return ACF_DISABLED;
 }
 
-/*
- * Return current session authentication state.  Return
- * value is TLS_AUTHENTICATION_x.
- */
-
-int
+enum tls_auth_status
 tls_authentication_status(struct tls_multi *multi, const int latency)
 {
     bool deferred = false;
+
+    /* at least one valid key has successfully completed authentication */
     bool success = false;
-    bool active = false;
-
-    static const unsigned char acf_merge[] =
-    {
-        ACF_UNDEFINED, /* s1=ACF_UNDEFINED s2=ACF_UNDEFINED */
-        ACF_UNDEFINED, /* s1=ACF_UNDEFINED s2=ACF_SUCCEEDED */
-        ACF_UNDEFINED, /* s1=ACF_UNDEFINED s2=ACF_DISABLED */
-        ACF_FAILED,  /* s1=ACF_UNDEFINED s2=ACF_FAILED */
-        ACF_UNDEFINED, /* s1=ACF_SUCCEEDED s2=ACF_UNDEFINED */
-        ACF_SUCCEEDED, /* s1=ACF_SUCCEEDED s2=ACF_SUCCEEDED */
-        ACF_SUCCEEDED, /* s1=ACF_SUCCEEDED s2=ACF_DISABLED */
-        ACF_FAILED,  /* s1=ACF_SUCCEEDED s2=ACF_FAILED */
-        ACF_UNDEFINED, /* s1=ACF_DISABLED  s2=ACF_UNDEFINED */
-        ACF_SUCCEEDED, /* s1=ACF_DISABLED  s2=ACF_SUCCEEDED */
-        ACF_DISABLED, /* s1=ACF_DISABLED  s2=ACF_DISABLED */
-        ACF_FAILED,  /* s1=ACF_DISABLED  s2=ACF_FAILED */
-        ACF_FAILED,  /* s1=ACF_FAILED    s2=ACF_UNDEFINED */
-        ACF_FAILED,  /* s1=ACF_FAILED    s2=ACF_SUCCEEDED */
-        ACF_FAILED,  /* s1=ACF_FAILED    s2=ACF_DISABLED */
-        ACF_FAILED   /* s1=ACF_FAILED    s2=ACF_FAILED */
-    };
 
-    if (multi)
-    {
-        int i;
+    /* at least one key is enabled for decryption */
+    int active = 0;
 
-        if (latency && multi->tas_last && multi->tas_last + latency >= now)
-        {
-            return TLS_AUTHENTICATION_UNDEFINED;
-        }
-        multi->tas_last = now;
+    if (latency && multi->tas_last + latency >= now)
+    {
+        return TLS_AUTHENTICATION_UNDEFINED;
+    }
+    multi->tas_last = now;
 
-        for (i = 0; i < KEY_SCAN_SIZE; ++i)
+    for (int i = 0; i < KEY_SCAN_SIZE; ++i)
+    {
+        struct key_state *ks = get_key_scan(multi, i);
+        if (DECRYPT_KEY_ENABLED(multi, ks))
         {
-            struct key_state *ks = get_key_scan(multi, i);
-            if (DECRYPT_KEY_ENABLED(multi, ks))
+            active++;
+            if (ks->authenticated > KS_AUTH_FALSE)
             {
-                active = true;
-                if (ks->authenticated > KS_AUTH_FALSE)
-                {
-                    unsigned int s1 = ACF_DISABLED;
-                    unsigned int s2 = ACF_DISABLED;
-                    s1 = key_state_test_auth_control_file(ks);
+                unsigned int s1 = ACF_DISABLED;
+                unsigned int s2 = ACF_DISABLED;
+                s1 = key_state_test_auth_control_file(ks);
 #ifdef ENABLE_MANAGEMENT
-                    s2 = man_def_auth_test(ks);
+                s2 = man_def_auth_test(ks);
 #endif
-                    ASSERT(s1 < 4 && s2 < 4);
-                    switch (acf_merge[(s1<<2) + s2])
+                ASSERT(s1 < 4 && s2 < 4);
+
+                if (s1 == ACF_FAILED || s2 == ACF_FAILED)
+                {
+                    ks->authenticated = KS_AUTH_FALSE;
+                }
+                else if (s1 == ACF_UNDEFINED || s2 == ACF_UNDEFINED)
+                {
+                    if (now < ks->auth_deferred_expire)
                     {
-                        case ACF_SUCCEEDED:
-                        case ACF_DISABLED:
-                            success = true;
-                            ks->authenticated = KS_AUTH_TRUE;
-                            break;
-
-                        case ACF_UNDEFINED:
-                            if (now < ks->auth_deferred_expire)
-                            {
-                                deferred = true;
-                            }
-                            break;
-
-                        case ACF_FAILED:
-                            ks->authenticated = KS_AUTH_FALSE;
-                            break;
-
-                        default:
-                            ASSERT(0);
+                        deferred = true;
                     }
                 }
+                else
+                {
+                    /* s1 and s2 are either ACF_DISABLED or ACF_SUCCEDED */
+                    success = true;
+                    ks->authenticated = KS_AUTH_TRUE;
+                }
             }
         }
     }
@@ -1020,12 +990,16 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
     {
         return TLS_AUTHENTICATION_SUCCEEDED;
     }
-    else if (!active || deferred)
+    else if (active == 0 || deferred)
     {
+        /* We have a deferred authentication and no currently active key
+         * (first auth, no renegotiation)  */
         return TLS_AUTHENTICATION_DEFERRED;
     }
     else
     {
+        /* at least one key is active but none is fully authenticated (!success)
+         * and all active are either failed authed or expired deferred auth */
         return TLS_AUTHENTICATION_FAILED;
     }
 }
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index d913f102..b3fe25d2 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -65,18 +65,35 @@ struct cert_hash_set {
 #define VERIFY_X509_SUBJECT_RDN         2
 #define VERIFY_X509_SUBJECT_RDN_PREFIX  3
 
-#define TLS_AUTHENTICATION_SUCCEEDED  0
-#define TLS_AUTHENTICATION_FAILED     1
-#define TLS_AUTHENTICATION_DEFERRED   2
-#define TLS_AUTHENTICATION_UNDEFINED  3
+enum tls_auth_status
+{
+    TLS_AUTHENTICATION_SUCCEEDED=0,
+    TLS_AUTHENTICATION_FAILED=1,
+    TLS_AUTHENTICATION_DEFERRED=2,
+    TLS_AUTHENTICATION_UNDEFINED=3
+};
 
-/*
- * Return current session authentication state.  Return
- * value is TLS_AUTHENTICATION_x.
+/**
+ * Return current session authentication state of the tls_multi structure
+ * This will return TLS_AUTHENTICATION_SUCCEEDED only if the session is
+ * fully authenicated, i.e. VPN traffic is allowed over it.
+ *
+ * Checks the status of all active keys and checks if the deferred
+ * authentication has succeeded.
+ *
+ * As a side effect this function will also transition ks->authenticated
+ * 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   multi       the tls_multi struct to operate on
  *
- * TODO: document this function
+ * @return              Current authentication status of the tls_multi
  */
-int tls_authentication_status(struct tls_multi *multi, const int latency);
+enum tls_auth_status
+tls_authentication_status(struct tls_multi *multi, const int latency);
 
 /** Check whether the \a ks \c key_state is ready to receive data channel
  *   packets.
-- 
2.26.2

[Openvpn-devel] [PATCH 6/8] Rename DECRYPT_KEY_ENABLED to TLS_AUTHENTICATED

[Arne Schwabe]

The macro's name suggests that the key is enabled and being used. But
the macro actually something different but similar enough that the name
was probably right at some point.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/ssl.c        |  6 +++---
 src/openvpn/ssl_verify.c |  2 +-
 src/openvpn/ssl_verify.h | 13 ++++++++++---
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index e59dba31..e4f43a86 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -3216,7 +3216,7 @@ tls_multi_process(struct tls_multi *multi,
      * verification failed.  A semi-trusted session can forward data on the
      * TLS control channel but not on the tunnel channel.
      */
-    if (DECRYPT_KEY_ENABLED(multi, &multi->session[TM_UNTRUSTED].key[KS_PRIMARY]))
+    if (TLS_AUTHENTICATED(multi, &multi->session[TM_UNTRUSTED].key[KS_PRIMARY]))
     {
         move_session(multi, TM_ACTIVE, TM_UNTRUSTED, true);
         msg(D_TLS_DEBUG_LOW, "TLS: tls_multi_process: untrusted session promoted to %strusted",
@@ -3301,7 +3301,7 @@ handle_data_channel_packet(struct tls_multi *multi,
          * passive side is the server which only listens for the connections, the
          * active side is the client which initiates connections).
          */
-        if (DECRYPT_KEY_ENABLED(multi, ks)
+        if (TLS_AUTHENTICATED(multi, ks)
             && key_id == ks->key_id
             && (ks->authenticated == KS_AUTH_TRUE)
             && (floated || link_socket_actual_match(from, &ks->remote_addr)))
@@ -3628,7 +3628,7 @@ tls_pre_decrypt(struct tls_multi *multi,
          * Remote is requesting a key renegotiation
          */
         if (op == P_CONTROL_SOFT_RESET_V1
-            && DECRYPT_KEY_ENABLED(multi, ks))
+            && TLS_AUTHENTICATED(multi, ks))
         {
             if (!read_control_auth(buf, &session->tls_wrap, from,
                                    session->opt))
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 4172e2fd..98985c51 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -948,7 +948,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
     for (int i = 0; i < KEY_SCAN_SIZE; ++i)
     {
         struct key_state *ks = get_key_scan(multi, i);
-        if (DECRYPT_KEY_ENABLED(multi, ks))
+        if (TLS_AUTHENTICATED(multi, ks))
         {
             active++;
             if (ks->authenticated > KS_AUTH_FALSE)
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index b3fe25d2..7e8b9710 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -95,15 +95,22 @@ enum tls_auth_status
 enum tls_auth_status
 tls_authentication_status(struct tls_multi *multi, const int latency);
 
-/** Check whether the \a ks \c key_state is ready to receive data channel
- *   packets.
+/** Check whether the \a ks \c key_state has finished the key exchange part
+ *  of the OpenVPN hand shake. This is that the key_method_2read/write
+ *  handshakes have been completed and certificate verification have
+ *  been completed.
+ *
+ * connect/deferred auth might still pending. Also data-channel keys might
+ * not have been created since they are delayed until PUSH_REPLY for NCP
+ * clients.
+ *
  *   @ingroup data_crypto
  *
  *   If true, it is safe to assume that this session has been authenticated
  *   by TLS.
  *
  *   @note This macro only works if S_SENT_KEY + 1 == S_GOT_KEY. */
-#define DECRYPT_KEY_ENABLED(multi, ks) ((ks)->state >= (S_GOT_KEY - (multi)->opt.server))
+#define TLS_AUTHENTICATED(multi, ks) ((ks)->state >= (S_GOT_KEY - (multi)->opt.server))
 
 /**
  * Remove the given key state's auth control file, if it exists.
-- 
2.26.2

[Openvpn-devel] [PATCH 7/8] Send AUTH_FAILED message to clients on renegotiation failures

[Arne Schwabe]

This changes the exit in server mode on renegotiation to an exit that
also sends an AUTH_FAILED to the client. Any previously set failed auth
reason is passed to the client.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/forward.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 958246c4..67615a6b 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -155,7 +155,14 @@ check_tls(struct context *c)
         }
         else if (tmp_status == TLSMP_KILL)
         {
-            register_signal(c, SIGTERM, "auth-control-exit");
+            if (c->options.mode == MODE_SERVER)
+            {
+                send_auth_failed(c, c->c2.tls_multi->client_reason);
+            }
+            else
+            {
+                register_signal(c, SIGTERM, "auth-control-exit");
+            }
         }
 
         interval_future_trigger(&c->c2.tmp_int, wakeup);
-- 
2.26.2

[Openvpn-devel] [PATCH 8/8] Make any auth failure tls_authentication_status return auth failed

[Arne Schwabe]

Previously tls_authentication_status only return
TLS_AUTHENTICATION_FAILED if there is no usable key at all. This
behaviour allows continuing using the still valid keys
(see --tran-window). However, the OpenVPN protocol lacks a way of
communicating that key is not useable to client once it reached
the TLS authenticated status (eg cert checks pass but connect or
user-pass verify fail). To avoid these desynchronisation issues
during deferred auth and renegotiation OpenVPN quietly only starts
using a new key after the hand-window has passed.

With this change any failure on a renogiation will lead to a
deauthentication of a client. This also fixes a number of bugs that
expiring auth-token and failed deferred auth is leading to key desync
or unexpected continuation of the VPN session.

The behaviour of deauthentication of all keys on deferred auth failure
has been already been used for years if authentication is done via
management interface. This commit also aligns the code paths for both.

A side effect might be that we also deauth clients earlier in some
other corner cases but the behaviour of continuing using an old
authenticated session while we already a failed authentication for the
client is most times unexpected behaviour from the user (admin).

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/multi.c      | 12 ++----------
 src/openvpn/ssl_verify.c | 25 +++++++++++++++++++++----
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 9becb2b2..401dfa8e 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3963,17 +3963,9 @@ management_client_auth(void *arg,
                     cc_config_owned = false;
                 }
             }
-            else
+            else if (reason)
             {
-                if (reason)
-                {
-                    msg(D_MULTI_LOW, "MULTI: connection rejected: %s, CLI:%s", reason, np(client_reason));
-                }
-                if (!is_cas_pending(mi->context.c2.context_auth))
-                {
-                    send_auth_failed(&mi->context, client_reason); /* mid-session reauth failed */
-                    multi_schedule_context_wakeup(m, mi);
-                }
+                msg(D_MULTI_LOW, "MULTI: connection rejected: %s, CLI:%s", reason, np(client_reason));
             }
         }
     }
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 98985c51..a4538d38 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -939,6 +939,9 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
     /* at least one key is enabled for decryption */
     int active = 0;
 
+    /* at least one key already failed authentication */
+    bool failed_auth = false;
+
     if (latency && multi->tas_last + latency >= now)
     {
         return TLS_AUTHENTICATION_UNDEFINED;
@@ -951,7 +954,11 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
         if (TLS_AUTHENTICATED(multi, ks))
         {
             active++;
-            if (ks->authenticated > KS_AUTH_FALSE)
+            if (ks->authenticated == KS_AUTH_FALSE)
+            {
+                failed_auth = true;
+            }
+            else
             {
                 unsigned int s1 = ACF_DISABLED;
                 unsigned int s2 = ACF_DISABLED;
@@ -964,6 +971,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
                 if (s1 == ACF_FAILED || s2 == ACF_FAILED)
                 {
                     ks->authenticated = KS_AUTH_FALSE;
+                    failed_auth = true;
                 }
                 else if (s1 == ACF_UNDEFINED || s2 == ACF_UNDEFINED)
                 {
@@ -983,10 +991,19 @@ tls_authentication_status(struct tls_multi *multi, const int latency)
     }
 
 #if 0
-    dmsg(D_TLS_ERRORS, "TAS: a=%d s=%d d=%d", active, success, deferred);
+    dmsg(D_TLS_ERRORS, "TAS: a=%d s=%d d=%d f=%d", active, success, deferred, failed_auth);
 #endif
-
-    if (success)
+    if (failed_auth)
+    {
+        /* We have at least one session that failed authentication. There
+         * might be still another session with valid keys.
+         * Although our protocol allows keeping the VPN session alive
+         * with the other session (and we actually did that in earlier
+         * version, this behaviour is really strange from a user (admin)
+         * experience */
+        return TLS_AUTHENTICATION_FAILED;
+    }
+    else if (success)
     {
         return TLS_AUTHENTICATION_SUCCEEDED;
     }
-- 
2.26.2

[Openvpn-devel] [PATCH applied] Re: Remove --disable-def-auth configure argument

[Gert Doering]

Acked-by: Gert Doering <gert@...1296...>

Looked at the changes (look good), saw that a few #ifdefs are now
"ENABLE_MANAGEMENT", so compiled with default settings and with
--disable-management.  Both passed compile + t_client tests.

I have not specifically tested management functionality - I do not
have a server testbed for that yet, and I was too lazy to compile
a windows client to test the client side.  I do assume that you
tested that part on the Android client already.

Your patch has been applied to the master branch.

commit 99d217b20064e7fef90dfa49bdcbab23ea7fbcb3
Author: Arne Schwabe
Date:   Fri Oct 23 13:32:44 2020 +0200

     Remove --disable-def-auth configure argument

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20201023113244.26295-1-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21214.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Replace key_scan array of static points with inline function

[Gert Doering]

Acked-by: Gert Doering <gert@...1296...>

Not sure if I ever understood these shortcuts (there are 3*2 keys
in session[].keys[], but the shortcuts only reference [0][0],
[0][1] and [2][1]) but from a "mechanically comparing before/after"
point of view, the change looks sane.  

Plus, it passes client side tests, with short reneg timeouts (20s).

This change, of course, makes it more interesting to do backports
of ssl.c-related changes from master/2.6 to 2.5...  so let's get
out 2.6 quickly :-)

Uncrustify complains about indent of the new switch/case block,
and that there is a trailing semicolon.  Fixed.

*Also*, this patch needed to be (manually) rebased due to context 
conflicts after 0d4ca79d4f.  Done.

"points" -> "pointers", as requested.

Your patch has been applied to the master branch.

commit cc5a71637139557a7eaa024251ff75a0acb22bc8
Author: Arne Schwabe
Date:   Fri Oct 23 14:02:53 2020 +0200

     Replace key_scan array of static pointers with inline function

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20201023120259.29783-1-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21225.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Add more documentation about our internal TLS functions

[Gert Doering]

Acked-by: Gert Doering <gert@...1296...>

"This was an easy one" - documentation is good.

Your patch has been applied to the master branch.

commit 8292102b102ff62d6b7ed1254076b822cb113162
Author: Arne Schwabe
Date:   Fri Oct 23 14:02:54 2020 +0200

     Add more documentation about our internal TLS functions

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20201023120259.29783-2-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21220.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Improve keys out of sync message

[Gert Doering]

Acked-by: Gert Doering <gert@...1296...>

I have no test bed that will trigger this, but the change is
not hard to understand (extend print_key_id() with a 
"auth=make_string(ks->authenticated)", append the result to
the "TLS Error:" message.

All other users of print_key_id() are all "just debug output",
so the new format will not confuse anything.

Compile tested on the client.

As with 2/8, uncrustify does not like the indenting of the
new switch/case block.  Adjusted.

Your patch has been applied to the master branch.

commit f1f0f074bf6e7b91673bfa8cb08b3be44ebda76b
Author: Arne Schwabe
Date:   Fri Oct 23 14:02:55 2020 +0200

     Improve keys out of sync message

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20201023120259.29783-3-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21226.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Clean up tls_authentication_status and document it

[Gert Doering]

Acked-by: Gert Doering <gert@...1296...>

This is not for the faint of heard... so I've excercised this
on the server side test framework (which has various "fail auth"
tests).

The changes in push.c and ssl.c are self-explanatory, though I
wonder why you didn't go for an "early exit if (!multi)" in
tls_authentication_status() to avoid the extra condition.

The change to ssl_verify.c is somewhat harder to grok - but after
staring at the table for a while, the combinations and priority
of evaluation becomes clear ("if either is ACF_FAILED, the result
is ACF_FAILED.  Then, if either is UNDEFINED, ...") - and the new
code reflects that, with WAY less magic, and more comments.  Yay.

Your patch has been applied to the master branch.

commit f9d3fbf9bc87ae6c05fc592712f610491a77d78b
Author: Arne Schwabe
Date:   Fri Oct 23 14:02:56 2020 +0200

     Clean up tls_authentication_status and document it

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20201023120259.29783-4-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21224.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Rename DECRYPT_KEY_ENABLED to TLS_AUTHENTICATED

[Gert Doering]

Acked-by: Gert Doering <gert@...1296...>

code wise, this is just mechanical search-and-replace, so easily
tested :-) - "understanding the code" wise this makes sense,
and even brings in extra documentation!

Your patch has been applied to the master branch.

commit 3ac8e5923a12390f68aa901e04ab3204e326d243
Author: Arne Schwabe
Date:   Fri Oct 23 14:02:57 2020 +0200

     Rename DECRYPT_KEY_ENABLED to TLS_AUTHENTICATED

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20201023120259.29783-5-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21221.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Send AUTH_FAILED message to clients on renegotiation failures

[Gert Doering]

Acked-by: Gert Doering <gert@...1296...>

I actually have a test case for this...

 - auth-gen-token 600
 - reneg-sec 30
 - sync plugin-auth-pam

then it will happily renegotiate every 30 seconds, and after 
10 minutes it will "fail without noticing" - the server logs

2020-11-26 15:10:30 us=755319 cron2-freebsd-tc-amd64/2001:608:0:814::f000:21 --auth-token-gen: auth-token from client expired
2020-11-26 15:10:30 us=755355 cron2-freebsd-tc-amd64/2001:608:0:814::f000:21 TLS: Username/auth-token authentication failed for username 'fbsd-tc-master'

(but never tells the client).

Eventually the keys time out:

2020-11-26 15:10:50 us=604558 cron2-freebsd-tc-amd64/2001:608:0:814::f000:21 TLS Error: local/remote TLS keys are out of sync: [AF_INET6]2001:608:0:814::f000:21:42838 (received key id: 7, known key ids:  [key#0 state=S_ACTIVE auth=KS_AUTH_FALSE id=7 sid=4ead8bbc 11847581] [key#1 state=S_ACTIVE auth=KS_AUTH_TRUE id=6 sid=4ead8bbc 11847581] [key#2 state=S_UNDEF auth=KS_AUTH_FALSE id=0 sid=00000000 00000000])

.. so pings start failing from here.

2020-11-26 15:11:00 us=968564 cron2-freebsd-tc-amd64/2001:608:0:814::f000:21 SIGTERM[soft,auth-control-exit] received, client-instance exiting

.. and on the next reneg-interval, the MI is closed.

The client runs into *ping* timeout eventually...  (but is never told by the server 
that the server instance went away):

2020-11-26 15:11:00 TLS: soft reset sec=30/30 bytes=7869/-1 pkts=61/0
2020-11-26 15:11:24 [server] Inactivity timeout (--ping-restart), restarting

retries after 5s:

2020-11-26 15:11:31 us=7470 2001:608:0:814::f000:21 SENT CONTROL [cron2-freebsd-tc-amd64]: 'AUTH_FAILED,SESSION: token expired' (status=1)

here the client *is* told, and re-tries 5 seconds later, with the 
"non-token" auth:

2020-11-26 15:11:36 us=98591 2001:608:0:814::f000:21 TLS: Username/Password authentication succeeded for username 'fbsd-tc-master' 

(failure of 51 seconds in here)


*With* the patch, there still is silliness involved

2020-11-26 15:42:30 us=587138 cron2-freebsd-tc-amd64/2001:608:0:814::f000:21 --auth-token-gen: auth-token from client expired
2020-11-26 15:42:30 us=587168 cron2-freebsd-tc-amd64/2001:608:0:814::f000:21 TLS: Username/auth-token authentication failed for username 'fbsd-tc-master'
2020-11-26 15:42:30 us=591020 cron2-freebsd-tc-amd64/2001:608:0:814::f000:21 Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, 2048 bit RSA
2020-11-26 15:42:45 us=175486 cron2-freebsd-tc-amd64/2001:608:0:814::f000:21 TLS Error: local/remote TLS keys are out of sync: [AF_INET6]2001:608:0:814::f000:21:29307 (received key id: 7, known key ids:  [key#0 state=S_ACTIVE auth=KS_AUTH_FALSE id=7 sid=028b5663 63c9dc4d] [key#1 state=S_ACTIVE auth=KS_AUTH_TRUE id=6 sid=028b5663 63c9dc4d] [key#2 state=S_UNDEF auth=KS_AUTH_FALSE id=0 sid=00000000 00000000])

.. and pings fail from 15:42:45 onwards, without telling the client.

The improvement bit happens then:

2020-11-26 15:43:00 TLS: soft reset sec=30/30 bytes=7869/-1 pkts=61/0
2020-11-26 15:43:00 AUTH: Received control message: AUTH_FAILED,SESSION: token expired
2020-11-26 15:43:00 Restart pause, 5 second(s)
2020-11-26 15:43:05 [server] Peer Connection Initiated with [AF_INET6]2001:608:0:814::f000:11:51199

So on the next renegotiation the client will receive a proper
error, and can reconnect right away.

Ping failure time is down from 51s to 23s, so "improvement" :-)


Your patch has been applied to the master branch.

commit 55d5eaa3e021a21b9537a474c46636d4c2dcdac5
Author: Arne Schwabe
Date:   Fri Oct 23 14:02:58 2020 +0200

     Send AUTH_FAILED message to clients on renegotiation failures

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20201023120259.29783-6-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21222.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Make any auth failure tls_authentication_status return auth failed

[Gert Doering]

Acked-by: Gert Doering <gert@...1296...>

Stared at the code for a bit, and it looks simple and straightforward
enough.  With 1/8...7/8 reviewed, I think I even understand what it
does :-)

The bit about "ks->authenticated == KS_AUTH_FALSE" becomes clear when
you look ks_auth_state - FALSE is 0, everything else is > FALSE, so
the "else" is basically the same condition as before.


Testing (only script backend + pam auth + token):

This improves the "token expired on renegotiation" even further,
though it still shows some interesting timing aspects.

Case 1: auth-gen-token 600, sync plugin auth-pam

2020-11-29 10:30:16 us=100840 --auth-token-gen: auth-token from client expired
2020-11-29 10:30:16 us=100868 TLS: Username/auth-token authentication failed for username 'fbsd-tc-master'
2020-11-29 10:30:16 us=101534 Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, 2048 bit RSA
2020-11-29 10:30:32 us=478267 Delayed exit in 5 seconds
2020-11-29 10:30:32 us=478374 SENT CONTROL [cron2-freebsd-tc-amd64]: 'AUTH_FAILED,SESSION: token expired' (status=1)

I wonder what it is doing here, between 10:30:16 and 10:30:32?  Expiring
the active key?

On the client side, we see renegotiation, TLS handshake, then a pause
(in which the connection still works fine(!)), then AUTH FAIL, and then
reconnect with user/password credentials:

2020-11-29 10:30:16 TLS: soft reset sec=151/151 bytes=89537/-1 pkts=705/0
2020-11-29 10:30:16 Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, 2048 bit RSA
2020-11-29 10:30:32 AUTH: Received control message: AUTH_FAILED,SESSION: token expired

2020-11-29 10:30:32 Restart pause, 5 second(s)
..
2020-11-29 10:30:37 TLS: Initial packet from ...
..
2020-11-29 10:30:37 PUSH: Received control message: 'PUSH_REPLY,...,xauth-tokenSESS_ID,...
..
2020-11-29 10:30:38 Initialization Sequence Completed


The server confirms:

2020-11-29 10:30:37 us=551426 TLS: Username/Password authentication succeeded for username 'fbsd-tc-master' 


So - this is definitely very nice.



Test case 2: auth-gen-token 600, deferred plugin auth-pam with 16s delay

- initial connect takes 18s (which is expected)
- initial TLS-reconnect with token takes 0.002s (which is very nice :) )
- TLS reconnect after 10 minute, with expired token...

2020-11-29 10:50:05 us=911127 --auth-token-gen: auth-token from client expired
2020-11-29 10:50:05 us=911147 TLS: Username/auth-token authentication failed for username 'fbsd-tc-master'
2020-11-29 10:50:05 us=912088 Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, 2048 bit RSA
2020-11-29 10:50:21 us=533468 Delayed exit in 5 seconds
2020-11-29 10:50:21 us=533597 SENT CONTROL [cron2-freebsd-tc-amd64]: 'AUTH_FAILED,SESSION: token expired' (status=1)

.. does the right thing now!

The client understands, and reconnects with password:

2020-11-29 10:50:26 us=601574 Re-using SSL/TLS context
2020-11-29 10:50:26 us=614341 PLUGIN AUTH-PAM: do deferred auth '/tmp/openvpn_acf_3131079bbfa3c5c756b7e374b8328dfc.tmp'
2020-11-29 10:50:26 us=627822 TLS: Username/Password authentication deferred for username 'fbsd-tc-master' 
2020-11-29 10:50:41 us=641229 PLUGIN AUTH-PAM: BACKGROUND: fbsd-tc-master: deferred auth: PAM succeeded
2020-11-29 10:50:42 us=814877 SENT CONTROL [cron2-freebsd-tc-amd64]: 'PUSH_REPLY,...,auth-tokenSESS_ID,...,key-derivation tls-ekm' (status=1)


And things are back to "it pings".  Of course the "no-ping" time is longer in 
this case, due to client reconnect (5s) *plus* 16s AUTH-PAM backgrounding, 
but this is exactly what is to be expected.

(This is the case where OpenVPN would previously get totally confused and 
never tell the client "hey, you're dead, reconnect!" - so, bug fixed, great 
stuff)


Your patch has been applied to the master branch (because it needs all
the prerequisite refactoring patches).  Now, how to get the bugfix part
into 2.5.1?

commit 88dc4276485bf2a4cecae3cff55d2e146dcd31ca
Author: Arne Schwabe
Date:   Fri Oct 23 14:02:59 2020 +0200

     Make any auth failure tls_authentication_status return auth failed

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20201023120259.29783-7-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21223.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering