openvpn-devel — OpenVPN developers list

[Openvpn-devel] [PATCH v4 10/13] client-connect: Also use inotify for the deferred client-connect status file

[Arne S. <ar...@rf...>]

As we never do client-connect and authentication at the same time
it is safe to reuse the existing fields for client-connect return
status file

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 src/openvpn/multi.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 63d9db05..09974357 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2505,8 +2505,10 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
 
 #ifdef ENABLE_ASYNC_PUSH
 /*
- * Called when inotify event is fired, which happens when acf file is closed or deleted.
- * Continues authentication and sends push_reply.
+ * Called when inotify event is fired, which happens when acf
+ * or connect-status file is closed or deleted.
+ * Continues authentication and sends push_reply
+ * (or be deferred again by client-connect)
  */
 void
 multi_process_file_closed(struct multi_context *m, const unsigned int mpp_flags)
@@ -2782,7 +2784,15 @@ 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 (mi->client_connect_status != CC_STATUS_ESTABLISHED
+                && mi->client_connect_defer_state.deferred_ret_file)
+            {
+                add_inotify_file_watch(m, mi, m->top.c2.inotify_fd,
+                                       mi->client_connect_defer_state.
+                                       deferred_ret_file);
+            }
+#endif
             /* tell scheduler to wake us up at some point in the future */
             multi_schedule_context_wakeup(m, mi);
         }
-- 
2.19.1

[Openvpn-devel] [PATCH v4 08/13] client-connect: Add deferred support to the client-connect script handler

[Arne S. <ar...@rf...>]

From: Fabian Knittel <fab...@le...>

This patch introduces the concept of a return value file for the client-connect
handlers. (This is very similar to the auth value file used during deferred
authentication.)  The file name is stored in the client_connect_state struct.

In addition, the patch also allows the storage of the client config file name
in struct client_connect_state.

Both changes are used by the client-connect script handler to support deferred
client-connection handling.  The deferred return value file (deferred_ret_file)
is passed to the actual script via the environment.  If the script succeeds and
writes the value for deferral into the deferred_ret_file, the handler knows to
indicate deferral.  Later on, the deferred handler checks whether the value of
the deferred_ret_file has been updated to success or failure.

Signed-off-by: Fabian Knittel <fab...@le...>
Signed-off-by: Arne Schwabe <ar...@rf...>
---
 src/openvpn/multi.c | 232 +++++++++++++++++++++++++++++++++++++++++---
 src/openvpn/multi.h |  12 +++
 2 files changed, 229 insertions(+), 15 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index e333476d..df6e4610 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1773,6 +1773,168 @@ multi_client_connect_setenv(struct multi_context *m,
     gc_free(&gc);
 }
 
+/**
+ * Delete the temporary file for the return value of client connect
+ * It also removes it from it from client_connect_defer_state and
+ * environment
+ */
+static void
+ccs_delete_deferred_ret_file(struct multi_instance *mi)
+{
+    struct client_connect_defer_state *ccs = &(mi->client_connect_defer_state);
+    if (ccs->deferred_ret_file)
+    {
+        setenv_del(mi->context.c2.es, "client_connect_deferred_file");
+        if (!platform_unlink(ccs->deferred_ret_file))
+        {
+            msg(D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
+                ccs->deferred_ret_file);
+        }
+        free(ccs->deferred_ret_file);
+        ccs->deferred_ret_file = NULL;
+    }
+}
+
+/**
+ * Create a temporary file for the return value of client connect
+ * and puts it into the client_connect_defer_state and environment
+ * as "client_connect_deferred_file"
+ *
+ * @return boolean value if creation was successfull
+ */
+static bool
+ccs_gen_deferred_ret_file(struct multi_instance *mi)
+{
+    struct client_connect_defer_state *ccs = &(mi->client_connect_defer_state);
+    struct gc_arena gc = gc_new();
+    const char *fn;
+
+    if (ccs->deferred_ret_file)
+    {
+        ccs_delete_deferred_ret_file(mi);
+    }
+
+    fn = platform_create_temp_file(mi->context.options.tmp_dir, "ccr", &gc);
+    if (!fn)
+    {
+        gc_free(&gc);
+        return false;
+    }
+    ccs->deferred_ret_file = string_alloc(fn, NULL);
+
+    setenv_str(mi->context.c2.es, "client_connect_deferred_file",
+               ccs->deferred_ret_file);
+
+    gc_free(&gc);
+    return true;
+}
+
+/**
+ * Tests whether the deferred return value file exists and returns the
+ * contained return value.
+ *
+ * @return CC_RET_SKIPPED if the file does not exist or is empty.
+ *         CC_RET_DEFERRED, CC_RET_SUCCEEDED or CC_RET_FAILED depending on
+ *         the value stored in the file.
+ */
+static enum client_connect_return
+ccs_test_deferred_ret_file(struct multi_instance *mi)
+{
+    struct client_connect_defer_state *ccs = &(mi->client_connect_defer_state);
+    enum client_connect_return ret = CC_RET_SKIPPED;
+    FILE *fp = fopen(ccs->deferred_ret_file, "r");
+    if (fp)
+    {
+        const int c = fgetc(fp);
+        switch (c)
+        {
+            case '0':
+                ret = CC_RET_FAILED;
+                break;
+
+            case '1':
+                ret = CC_RET_SUCCEEDED;
+                break;
+
+            case '2':
+                ret = CC_RET_DEFERRED;
+                break;
+
+            case EOF:
+                if (feof(fp))
+                {
+                    ret = CC_RET_SKIPPED;
+                    break;
+                }
+
+            /* Not EOF, but other error fall through to error state */
+            default:
+                /* We received an unknown/unexpected value.  Assume failure. */
+                msg(M_WARN, "WARNING: Unknown/unexcepted value in deferred"
+                    "client-connect resultfile");
+                ret = CC_RET_FAILED;
+        }
+        fclose(fp);
+    }
+    return ret;
+}
+
+/**
+ * Deletes the temporary file for the config directives of the  client connect
+ * script and removes it into the client_connect_defer_state and environment
+ *
+ */
+static void
+ccs_delete_config_file(struct multi_instance *mi)
+{
+    struct client_connect_defer_state *ccs = &(mi->client_connect_defer_state);
+    if (ccs->config_file)
+    {
+        setenv_del(mi->context.c2.es, "client_connect_config_file");
+        if (!platform_unlink(ccs->config_file))
+        {
+            msg(D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
+                ccs->config_file);
+        }
+        free(ccs->config_file);
+        ccs->config_file = NULL;
+    }
+}
+
+/**
+ * Create a temporary file for the config directives of the  client connect
+ * script and puts it into the client_connect_defer_state and environment
+ * as "client_connect_config_file"
+ *
+ * @return boolean value if creation was successfull
+ */
+static bool
+ccs_gen_config_file(struct multi_instance *mi)
+{
+    struct client_connect_defer_state *ccs = &(mi->client_connect_defer_state);
+    struct gc_arena gc = gc_new();
+    const char *fn;
+
+    if (ccs->config_file)
+    {
+        ccs_delete_config_file(mi);
+    }
+
+    fn = platform_create_temp_file(mi->context.options.tmp_dir, "cc", &gc);
+    if (!fn)
+    {
+        gc_free(&gc);
+        return false;
+    }
+    ccs->config_file = string_alloc(fn, NULL);
+
+    setenv_str(mi->context.c2.es, "client_connect_config_file",
+               ccs->config_file);
+
+    gc_free(&gc);
+    return true;
+}
+
 static enum client_connect_return
 multi_client_connect_call_plugin_v1(struct multi_context *m,
                                     struct multi_instance *mi,
@@ -1862,8 +2024,6 @@ multi_client_connect_call_plugin_v2(struct multi_context *m,
     return ret;
 }
 
-
-
 /**
  * Runs the --client-connect script if one is defined.
  */
@@ -1873,47 +2033,89 @@ multi_client_connect_call_script(struct multi_context *m,
                                  unsigned int *option_types_found)
 {
     enum client_connect_return ret = CC_RET_SKIPPED;
+    struct client_connect_defer_state *ccs = &(mi->client_connect_defer_state);
+
     if (mi->context.options.client_connect_script)
     {
         struct argv argv = argv_new();
         struct gc_arena gc = gc_new();
-        const char *dc_file = NULL;
 
         setenv_str(mi->context.c2.es, "script_type", "client-connect");
 
-        dc_file = platform_create_temp_file(mi->context.options.tmp_dir,
-                                            "cc", &gc);
-        if (!dc_file)
+        if (!ccs_gen_config_file(mi)
+            || !ccs_gen_deferred_ret_file(mi))
         {
             ret = CC_RET_FAILED;
             goto cleanup;
         }
 
         argv_parse_cmd(&argv, mi->context.options.client_connect_script);
-        argv_printf_cat(&argv, "%s", dc_file);
+        argv_printf_cat(&argv, "%s", ccs->config_file);
 
         if (openvpn_run_script(&argv, mi->context.c2.es, 0, "--client-connect"))
         {
-            multi_client_connect_post(m, mi, dc_file, option_types_found);
-            ret = CC_RET_SUCCEEDED;
+            if (ccs_test_deferred_ret_file(mi) == CC_RET_DEFERRED)
+            {
+                ret = CC_RET_DEFERRED;
+            }
+            else
+            {
+                multi_client_connect_post(m, mi, ccs->config_file,
+                                          option_types_found);
+                ret = CC_RET_SUCCEEDED;
+            }
         }
         else
         {
             ret = CC_RET_FAILED;
         }
-
-        if (!platform_unlink(dc_file))
+cleanup:
+        if (ret != CC_RET_DEFERRED)
         {
-            msg(D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
-                dc_file);
+            ccs_delete_config_file(mi);
+            ccs_delete_deferred_ret_file(mi);
         }
-cleanup:
         argv_reset(&argv);
         gc_free(&gc);
     }
     return ret;
 }
 
+static enum client_connect_return
+multi_client_handle_deferred(struct multi_context *m,
+                             struct multi_instance *mi,
+                             unsigned int *option_types_found)
+{
+    ASSERT(mi);
+    ASSERT(option_types_found);
+    struct client_connect_defer_state *ccs = &(mi->client_connect_defer_state);
+    enum client_connect_return ret = CC_RET_SKIPPED;
+
+    ret = ccs_test_deferred_ret_file(mi);
+
+    if (ret == CC_RET_SKIPPED)
+    {
+        /*
+         * Skipped and deferred are equivalent in this context.
+         * skipped means that the called program has not yet
+         * written a return status implicitly needing more time
+         * while deferred is the explicit notifcation that it
+         * needs more time
+         */
+        ret = CC_RET_DEFERRED;
+    }
+
+    if (ret != CC_RET_DEFERRED)
+    {
+        ccs_delete_deferred_ret_file(mi);
+        multi_client_connect_post(m, mi, ccs->config_file,
+                                  option_types_found);
+        ccs_delete_config_file(mi);
+    }
+    return ret;
+}
+
+
 static void
 multi_client_connect_late_setup(struct multi_context *m,
                                 struct multi_instance *mi,
@@ -2140,7 +2342,7 @@ static const struct client_connect_handlers client_connect_handlers[] = {
     },
     {
         .main = multi_client_connect_call_script,
-        .deferred = multi_client_connect_fail
+        .deferred = multi_client_handle_deferred
     },
     {
         .main = multi_client_connect_mda,
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 252b1d3d..c83323ce 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -74,6 +74,18 @@ struct client_connect_defer_state
     /* Remember which option classes where processed for delayed option
      * handling. */
     unsigned int option_types_found;
+
+    /**
+     * The temporrary file name that contains the return status of the
+     * client-connect script if it exits with defer as status
+     */
+    char *deferred_ret_file;
+
+    /**
+     * The temporary file name that contains the config directives
+     * returned by the client-connect script
+     */
+    char *config_file;
 };
 
 enum client_connect_status
-- 
2.19.1

[Openvpn-devel] [PATCH v4 04/13] client-connect: Refactor to use return values instead of modifying a passed-in flag

[Arne S. <ar...@rf...>]

From: Fabian Knittel <fab...@le...>

This patch changes the way the client-connect helper functions communicate with
the main function.  Instead of updating cc_succeeded and cc_succeeded_count,
they now return either CC_RET_SUCCEEDED, CC_RET_FAILED or CC_RET_SKIPPED.

In addition, the client-connect helpers are now called in completely identical
ways.  This is in preparation of handling the helpers as simple call-backs.

Signed-off-by: Fabian Knittel <fab...@le...>
Signed-off-by: Arne Schwabe <ar...@rf...>
---
 src/openvpn/multi.c | 126 +++++++++++++++++++++++++++-----------------
 src/openvpn/multi.h |  10 ++++
 2 files changed, 87 insertions(+), 49 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index bc9fddce..4182b553 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1706,20 +1706,21 @@ multi_client_connect_post_plugin(struct multi_context *m,
 
 #endif /* ifdef ENABLE_PLUGIN */
 
-#ifdef MANAGEMENT_DEF_AUTH
+
 
 /*
  * Called to load management-derived client-connect config
  */
-static void
+enum client_connect_return
 multi_client_connect_mda(struct multi_context *m,
                          struct multi_instance *mi,
                          unsigned int *option_types_found)
 {
+    enum client_connect_return ret = CC_RET_SKIPPED;
+#ifdef MANAGEMENT_DEF_AUTH
     if (mi->cc_config)
     {
         struct buffer_entry *be;
-
         for (be = mi->cc_config->head; be != NULL; be = be->next)
         {
             const char *opt = BSTR(&be->buf);
@@ -1739,10 +1740,12 @@ multi_client_connect_mda(struct multi_context *m,
          */
         multi_select_virtual_addr(m, mi);
         multi_set_virtual_addr_env(m, mi);
-    }
-}
 
+        ret = CC_RET_SUCCEEDED;
+    }
 #endif /* ifdef MANAGEMENT_DEF_AUTH */
+    return ret;
+}
 
 static void
 multi_client_connect_setenv(struct multi_context *m,
@@ -1769,19 +1772,16 @@ multi_client_connect_setenv(struct multi_context *m,
     gc_free(&gc);
 }
 
-static void
+static enum client_connect_return
 multi_client_connect_call_plugin_v1(struct multi_context *m,
                                     struct multi_instance *mi,
-                                    unsigned int *option_types_found,
-                                    int *cc_succeeded,
-                                    int *cc_succeeded_count)
+                                    unsigned int *option_types_found)
 {
+    enum client_connect_return ret = CC_RET_SKIPPED;
 #ifdef ENABLE_PLUGIN
     ASSERT(m);
     ASSERT(mi);
     ASSERT(option_types_found);
-    ASSERT(cc_succeeded);
-    ASSERT(cc_succeeded_count);
 
     /* deprecated callback, use a file for passing back return info */
     if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
@@ -1793,7 +1793,7 @@ multi_client_connect_call_plugin_v1(struct multi_context *m,
 
         if (!dc_file)
         {
-            cc_succeeded = false;
+            ret = CC_RET_FAILED;
             goto cleanup;
         }
 
@@ -1803,12 +1803,12 @@ multi_client_connect_call_plugin_v1(struct multi_context *m,
             != OPENVPN_PLUGIN_FUNC_SUCCESS)
         {
             msg(M_WARN, "WARNING: client-connect plugin call failed");
-            *cc_succeeded = false;
+            ret = CC_RET_FAILED;
         }
         else
         {
             multi_client_connect_post(m, mi, dc_file, option_types_found);
-            (*cc_succeeded_count)++;
+            ret = CC_RET_SUCCEEDED;
         }
 
         {
@@ -1821,21 +1821,19 @@ cleanup:
         gc_free(&gc);
     }
 #endif /* ifdef ENABLE_PLUGIN */
+    return ret;
 }
 
-static void
+static enum client_connect_return
 multi_client_connect_call_plugin_v2(struct multi_context *m,
                                     struct multi_instance *mi,
-                                    unsigned int *option_types_found,
-                                    int *cc_succeeded,
-                                    int *cc_succeeded_count)
+                                    unsigned int *option_types_found)
 {
+    enum client_connect_return ret = CC_RET_SKIPPED;
 #ifdef ENABLE_PLUGIN
     ASSERT(m);
     ASSERT(mi);
     ASSERT(option_types_found);
-    ASSERT(cc_succeeded);
-    ASSERT(cc_succeeded_count);
 
     /* V2 callback, use a plugin_return struct for passing back return info */
     if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT_V2))
@@ -1849,17 +1847,18 @@ multi_client_connect_call_plugin_v2(struct multi_context *m,
             != OPENVPN_PLUGIN_FUNC_SUCCESS)
         {
             msg(M_WARN, "WARNING: client-connect-v2 plugin call failed");
-            *cc_succeeded = false;
+            ret = CC_RET_FAILED;
         }
         else
         {
             multi_client_connect_post_plugin(m, mi, &pr, option_types_found);
-            (*cc_succeeded_count)++;
+            ret = CC_RET_SUCCEEDED;
         }
 
         plugin_return_free(&pr);
     }
 #endif /* ifdef ENABLE_PLUGIN */
+    return ret;
 }
 
 
@@ -1867,13 +1866,12 @@ multi_client_connect_call_plugin_v2(struct multi_context *m,
 /**
  * Runs the --client-connect script if one is defined.
  */
-static void
+static enum client_connect_return
 multi_client_connect_call_script(struct multi_context *m,
                                  struct multi_instance *mi,
-                                 unsigned int *option_types_found,
-                                 int *cc_succeeded,
-                                 int *cc_succeeded_count)
+                                 unsigned int *option_types_found)
 {
+    enum client_connect_return ret = CC_RET_SKIPPED;
     if (mi->context.options.client_connect_script)
     {
         struct argv argv = argv_new();
@@ -1886,7 +1884,7 @@ multi_client_connect_call_script(struct multi_context *m,
                                             "cc", &gc);
         if (!dc_file)
         {
-            *cc_succeeded = false;
+            ret = CC_RET_FAILED;
             goto cleanup;
         }
 
@@ -1896,11 +1894,11 @@ multi_client_connect_call_script(struct multi_context *m,
         if (openvpn_run_script(&argv, mi->context.c2.es, 0, "--client-connect"))
         {
             multi_client_connect_post(m, mi, dc_file, option_types_found);
-            (*cc_succeeded_count)++;
+            ret = CC_RET_SUCCEEDED;
         }
         else
         {
-            *cc_succeeded = false;
+            ret = CC_RET_FAILED;
         }
 
         if (!platform_unlink(dc_file))
@@ -1912,6 +1910,7 @@ cleanup:
         argv_reset(&argv);
         gc_free(&gc);
     }
+    return ret;
 }
 
 static void
@@ -2046,18 +2045,18 @@ multi_client_connect_early_setup(struct multi_context *m,
     multi_select_virtual_addr(m, mi);
 
     multi_client_connect_setenv(m, mi);
-
 }
 
 /**
  * Try to source a dynamic config file from the
  * --client-config-dir directory.
  */
-static void
+enum client_connect_return
 multi_client_connect_source_ccd(struct multi_context *m,
                                 struct multi_instance *mi,
                                 unsigned int *option_types_found)
 {
+    enum client_connect_return ret = CC_RET_SKIPPED;
     if (mi->context.options.client_config_dir)
     {
         struct gc_arena gc = gc_new();
@@ -2099,9 +2098,35 @@ multi_client_connect_source_ccd(struct multi_context *m,
             multi_select_virtual_addr(m, mi);
 
             multi_client_connect_setenv(m, mi);
+
+            ret = CC_RET_SUCCEEDED;
         }
         gc_free(&gc);
     }
+    return ret;
+}
+
+static inline bool
+cc_check_return(int *cc_succeeded_count,
+                enum client_connect_return ret)
+{
+    if (ret == CC_RET_SUCCEEDED)
+    {
+        (*cc_succeeded_count)++;
+        return true;
+    }
+    else if (ret == CC_RET_FAILED)
+    {
+        return false;
+    }
+    else if (ret == CC_RET_SKIPPED)
+    {
+        return true;
+    }
+    else
+    {
+        ASSERT(0);
+    }
 }
 
 /*
@@ -2123,38 +2148,41 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
 
         int cc_succeeded = true; /* client connect script status */
         int cc_succeeded_count = 0;
+        enum client_connect_return ret;
 
         multi_client_connect_early_setup(m, mi);
 
-        multi_client_connect_source_ccd(m, mi, &option_types_found);
-
-        multi_client_connect_call_plugin_v1(m, mi, &option_types_found,
-                                            &cc_succeeded,
-                                            &cc_succeeded_count);
+        ret = multi_client_connect_source_ccd(m, mi, &option_types_found);
+        cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
 
+        if (cc_succeeded)
+        {
+            ret = multi_client_connect_call_plugin_v1(m, mi,
+                                                      &option_types_found);
+            cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
+        }
 
-        multi_client_connect_call_plugin_v2(m, mi, &option_types_found,
-                                            &cc_succeeded,
-                                            &cc_succeeded_count);
+        if (cc_succeeded)
+        {
+            ret = multi_client_connect_call_plugin_v2(m, mi,
+                                                      &option_types_found);
+            cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
+        }
 
         /*
          * Check for client-connect script left by management interface client
          */
-
         if (cc_succeeded)
         {
-            multi_client_connect_call_script(m, mi, &option_types_found,
-                                             &cc_succeeded,
-                                             &cc_succeeded_count);
-
+            ret = multi_client_connect_call_script(m, mi, &option_types_found);
+            cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
         }
-#ifdef MANAGEMENT_DEF_AUTH
-        if (cc_succeeded && mi->cc_config)
+
+        if (cc_succeeded)
         {
-            multi_client_connect_mda(m, mi, &option_types_found);
-            cc_succeeded_count++;
+            ret = multi_client_connect_mda(m, mi, &option_types_found);
+            cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
         }
-#endif
 
         /*
          * Check for "disable" directive in client-config-dir file
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 489c073a..0a1ab2c3 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -191,6 +191,16 @@ struct multi_context {
     struct deferred_signal_schedule_entry deferred_shutdown_signal;
 };
 
+/**
+ * Return values used by the client connect call-back functions.
+ */
+enum client_connect_return
+{
+  CC_RET_FAILED,
+  CC_RET_SUCCEEDED,
+  CC_RET_SKIPPED
+};
+
 /*
  * Host route
  */
-- 
2.19.1

[Openvpn-devel] [PATCH v4 01/13] client-connect: Split multi_connection_established into separate functions

[Arne S. <ar...@rf...>]

From: Fabian Knittel <fab...@le...>

This patch splits up the multi_connection_established() function.  Each new
helper function does a specific job.  Functions that do a similar job receive a
similar calling interface.

The patch tries not to reindent code, so that the real changes are as clearly
visible as possible.  (A follow-up patch will only do indentation changes.)

Signed-off-by: Fabian Knittel <fab...@le...>

PATCH v3: Since the code has changed enough from the time the original patch
to the current master, the splitting has been redone from the current code.
Also some style and minor code changes have been added doing this patch.
This elimininates and the big reformatting done before eliminates the follow
up patch with indentation changes.

The original patch already replaces some instances of option_permission_mask
with CLIENT_CONNECT_OPT_MASK. The V3 version does this more consequently.

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 src/openvpn/multi.c | 597 +++++++++++++++++++++++++-------------------
 src/openvpn/multi.h |   4 +-
 2 files changed, 346 insertions(+), 255 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 8440f311..a4b62151 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1638,7 +1638,6 @@ static void
 multi_client_connect_post(struct multi_context *m,
                           struct multi_instance *mi,
                           const char *dc_file,
-                          unsigned int option_permissions_mask,
                           unsigned int *option_types_found)
 {
     /* Did script generate a dynamic config file? */
@@ -1647,7 +1646,7 @@ multi_client_connect_post(struct multi_context *m,
         options_server_import(&mi->context.options,
                               dc_file,
                               D_IMPORT_ERRORS|M_OPTERR,
-                              option_permissions_mask,
+                              CLIENT_CONNECT_OPT_MASK,
                               option_types_found,
                               mi->context.c2.es);
 
@@ -1671,7 +1670,6 @@ static void
 multi_client_connect_post_plugin(struct multi_context *m,
                                  struct multi_instance *mi,
                                  const struct plugin_return *pr,
-                                 unsigned int option_permissions_mask,
                                  unsigned int *option_types_found)
 {
     struct plugin_return config;
@@ -1689,7 +1687,7 @@ multi_client_connect_post_plugin(struct multi_context *m,
                 options_string_import(&mi->context.options,
                                       config.list[i]->value,
                                       D_IMPORT_ERRORS|M_OPTERR,
-                                      option_permissions_mask,
+                                      CLIENT_CONNECT_OPT_MASK,
                                       option_types_found,
                                       mi->context.c2.es);
             }
@@ -1716,21 +1714,19 @@ multi_client_connect_post_plugin(struct multi_context *m,
 static void
 multi_client_connect_mda(struct multi_context *m,
                          struct multi_instance *mi,
-                         const struct buffer_list *config,
-                         unsigned int option_permissions_mask,
                          unsigned int *option_types_found)
 {
-    if (config)
+    if (mi->cc_config)
     {
         struct buffer_entry *be;
 
-        for (be = config->head; be != NULL; be = be->next)
+        for (be = mi->cc_config->head; be != NULL; be = be->next)
         {
             const char *opt = BSTR(&be->buf);
             options_string_import(&mi->context.options,
                                   opt,
                                   D_IMPORT_ERRORS|M_OPTERR,
-                                  option_permissions_mask,
+                                  CLIENT_CONNECT_OPT_MASK,
                                   option_types_found,
                                   mi->context.c2.es);
         }
@@ -1773,215 +1769,386 @@ multi_client_connect_setenv(struct multi_context *m,
     gc_free(&gc);
 }
 
-/*
- * Called as soon as the SSL/TLS connection authenticates.
- *
- * Instance-specific directives to be processed:
- *
- *   iroute start-ip end-ip
- *   ifconfig-push local remote-netmask
- *   push
- */
 static void
-multi_connection_established(struct multi_context *m, struct multi_instance *mi)
+multi_client_connect_call_plugin_v1(struct multi_context *m,
+                                    struct multi_instance *mi,
+                                    unsigned int *option_types_found,
+                                    int *cc_succeeded,
+                                    int *cc_succeeded_count)
 {
-    if (tls_authentication_status(mi->context.c2.tls_multi, 0) == TLS_AUTHENTICATION_SUCCEEDED)
+#ifdef ENABLE_PLUGIN
+    ASSERT(m);
+    ASSERT(mi);
+    ASSERT(option_types_found);
+    ASSERT(cc_succeeded);
+    ASSERT(cc_succeeded_count);
+
+    /* deprecated callback, use a file for passing back return info */
+    if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
     {
+        struct argv argv = argv_new();
         struct gc_arena gc = gc_new();
-        unsigned int option_types_found = 0;
+        const char *dc_file =
+            platform_create_temp_file(mi->context.options.tmp_dir, "cc", &gc);
 
-        const unsigned int option_permissions_mask =
-            OPT_P_INSTANCE
-            | OPT_P_INHERIT
-            | OPT_P_PUSH
-            | OPT_P_TIMER
-            | OPT_P_CONFIG
-            | OPT_P_ECHO
-            | OPT_P_COMP
-            | OPT_P_SOCKFLAGS;
+        if (!dc_file)
+        {
+            cc_succeeded = false;
+            goto cleanup;
+        }
 
-        int cc_succeeded = true; /* client connect script status */
-        int cc_succeeded_count = 0;
+        argv_printf(&argv, "%s", dc_file);
+        if (plugin_call(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT,
+                        &argv, NULL, mi->context.c2.es)
+            != OPENVPN_PLUGIN_FUNC_SUCCESS)
+        {
+            msg(M_WARN, "WARNING: client-connect plugin call failed");
+            *cc_succeeded = false;
+        }
+        else
+        {
+            multi_client_connect_post(m, mi, dc_file, option_types_found);
+            (*cc_succeeded_count)++;
+        }
+
+        {
+            msg(D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
+                dc_file);
+        }
+
+cleanup:
+        argv_reset(&argv);
+        gc_free(&gc);
+    }
+#endif /* ifdef ENABLE_PLUGIN */
+}
 
-        ASSERT(mi->context.c1.tuntap);
+static void
+multi_client_connect_call_plugin_v2(struct multi_context *m,
+                                    struct multi_instance *mi,
+                                    unsigned int *option_types_found,
+                                    int *cc_succeeded,
+                                    int *cc_succeeded_count)
+{
+#ifdef ENABLE_PLUGIN
+    ASSERT(m);
+    ASSERT(mi);
+    ASSERT(option_types_found);
+    ASSERT(cc_succeeded);
+    ASSERT(cc_succeeded_count);
 
-        /* lock down the common name and cert hashes so they can't change during future TLS renegotiations */
-        tls_lock_common_name(mi->context.c2.tls_multi);
-        tls_lock_cert_hash_set(mi->context.c2.tls_multi);
+    /* V2 callback, use a plugin_return struct for passing back return info */
+    if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT_V2))
+    {
+        struct plugin_return pr;
 
-        /* generate a msg() prefix for this client instance */
-        generate_prefix(mi);
+        plugin_return_init(&pr);
 
-        /* delete instances of previous clients with same common-name */
-        if (!mi->context.options.duplicate_cn)
+        if (plugin_call(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT_V2,
+                        NULL, &pr, mi->context.c2.es)
+            != OPENVPN_PLUGIN_FUNC_SUCCESS)
         {
-            multi_delete_dup(m, mi);
+            msg(M_WARN, "WARNING: client-connect-v2 plugin call failed");
+            *cc_succeeded = false;
+        }
+        else
+        {
+            multi_client_connect_post_plugin(m, mi, &pr, option_types_found);
+            (*cc_succeeded_count)++;
         }
 
-        /* reset pool handle to null */
-        mi->vaddr_handle = -1;
+        plugin_return_free(&pr);
+    }
+#endif /* ifdef ENABLE_PLUGIN */
+}
 
-        /*
-         * Try to source a dynamic config file from the
-         * --client-config-dir directory.
-         */
-        if (mi->context.options.client_config_dir)
+
+
+/**
+ * Runs the --client-connect script if one is defined.
+ */
+static void
+multi_client_connect_call_script(struct multi_context *m,
+                                 struct multi_instance *mi,
+                                 unsigned int *option_types_found,
+                                 int *cc_succeeded,
+                                 int *cc_succeeded_count)
+{
+    if (mi->context.options.client_connect_script)
+    {
+        struct argv argv = argv_new();
+        struct gc_arena gc = gc_new();
+        const char *dc_file = NULL;
+
+        setenv_str(mi->context.c2.es, "script_type", "client-connect");
+
+        dc_file = platform_create_temp_file(mi->context.options.tmp_dir,
+                                            "cc", &gc);
+        if (!dc_file)
         {
-            const char *ccd_file;
+            *cc_succeeded = false;
+            goto cleanup;
+        }
 
-            ccd_file = platform_gen_path(mi->context.options.client_config_dir,
-                                         tls_common_name(mi->context.c2.tls_multi,
-                                                         false),
-                                         &gc);
+        argv_parse_cmd(&argv, mi->context.options.client_connect_script);
+        argv_printf_cat(&argv, "%s", dc_file);
 
-            /* try common-name file */
-            if (platform_test_file(ccd_file))
-            {
-                options_server_import(&mi->context.options,
-                                      ccd_file,
-                                      D_IMPORT_ERRORS|M_OPTERR,
-                                      option_permissions_mask,
-                                      &option_types_found,
-                                      mi->context.c2.es);
-            }
-            else /* try default file */
-            {
-                ccd_file = platform_gen_path(mi->context.options.client_config_dir,
-                                             CCD_DEFAULT,
-                                             &gc);
+        if (openvpn_run_script(&argv, mi->context.c2.es, 0, "--client-connect"))
+        {
+            multi_client_connect_post(m, mi, dc_file, option_types_found);
+            (*cc_succeeded_count)++;
+        }
+        else
+        {
+            *cc_succeeded = false;
+        }
 
-                if (platform_test_file(ccd_file))
-                {
-                    options_server_import(&mi->context.options,
-                                          ccd_file,
-                                          D_IMPORT_ERRORS|M_OPTERR,
-                                          option_permissions_mask,
-                                          &option_types_found,
-                                          mi->context.c2.es);
-                }
-            }
+        if (!platform_unlink(dc_file))
+        {
+            msg(D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
+                dc_file);
         }
+cleanup:
+        argv_reset(&argv);
+        gc_free(&gc);
+    }
+}
 
-        /*
-         * Select a virtual address from either --ifconfig-push in --client-config-dir file
-         * or --ifconfig-pool.
-         */
-        multi_select_virtual_addr(m, mi);
+static void
+multi_client_connect_late_setup(struct multi_context *m,
+                                struct multi_instance *mi,
+                                const unsigned int option_types_found)
+{
+    struct gc_arena gc = gc_new();
+    /*
+     * Process sourced options.
+     */
+    do_deferred_options(&mi->context, option_types_found);
 
-        /* do --client-connect setenvs */
-        multi_client_connect_setenv(m, mi);
+    /*
+     * make sure we got ifconfig settings from somewhere
+     */
+    if (!mi->context.c2.push_ifconfig_defined)
+    {
+        msg(D_MULTI_ERRORS, "MULTI: no dynamic or static remote"
+            "--ifconfig address is available for %s",
+            multi_instance_string(mi, false, &gc));
+    }
+
+    /*
+     * make sure that ifconfig settings comply with constraints
+     */
+    if (!ifconfig_push_constraint_satisfied(&mi->context))
+    {
+        const char *ifconfig_constraint_network =
+            print_in_addr_t(mi->context.options.push_ifconfig_constraint_network, 0, &gc);
+        const char *ifconfig_constraint_netmask =
+            print_in_addr_t(mi->context.options.push_ifconfig_constraint_netmask, 0, &gc);
+
+        /* JYFIXME -- this should cause the connection to fail */
+        msg(D_MULTI_ERRORS, "MULTI ERROR: primary virtual IP for %s (%s)"
+            "violates tunnel network/netmask constraint (%s/%s)",
+            multi_instance_string(mi, false, &gc),
+            print_in_addr_t(mi->context.c2.push_ifconfig_local, 0, &gc),
+            ifconfig_constraint_network, ifconfig_constraint_netmask);
+    }
+
+    /*
+     * For routed tunnels, set up internal route to endpoint
+     * plus add all iroute routes.
+     */
+    if (TUNNEL_TYPE(mi->context.c1.tuntap) == DEV_TYPE_TUN)
+    {
+        if (mi->context.c2.push_ifconfig_defined)
+        {
+            multi_learn_in_addr_t(m, mi,
+                                  mi->context.c2.push_ifconfig_local,
+                                  -1, true);
+            msg(D_MULTI_LOW, "MULTI: primary virtual IP for %s: %s",
+                multi_instance_string(mi, false, &gc),
+                print_in_addr_t(mi->context.c2.push_ifconfig_local, 0, &gc));
+        }
+
+        if (mi->context.c2.push_ifconfig_ipv6_defined)
+        {
+            multi_learn_in6_addr(m, mi,
+                                 mi->context.c2.push_ifconfig_ipv6_local,
+                                 -1, true);
+            /* TODO: find out where addresses are "unlearned"!! */
+            const char *ifconfig_local_ipv6 =
+                print_in6_addr(mi->context.c2.push_ifconfig_ipv6_local, 0, &gc);
+            msg(D_MULTI_LOW, "MULTI: primary virtual IPv6 for %s: %s",
+                multi_instance_string(mi, false, &gc),
+                ifconfig_local_ipv6);
+        }
+
+        /* add routes locally, pointing to new client, if
+         * --iroute options have been specified */
+        multi_add_iroutes(m, mi);
 
-#ifdef ENABLE_PLUGIN
         /*
-         * Call client-connect plug-in.
+         * iroutes represent subnets which are "owned" by a particular
+         * client.  Therefore, do not actually push a route to a client
+         * if it matches one of the client's iroutes.
          */
+        remove_iroutes_from_push_route_list(&mi->context.options);
+    }
+    else if (mi->context.options.iroutes)
+    {
+        msg(D_MULTI_ERRORS, "MULTI: --iroute options rejected for %s -- iroute"
+            "only works with tun-style tunnels",
+            multi_instance_string(mi, false, &gc));
+    }
 
-        /* deprecated callback, use a file for passing back return info */
-        if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
-        {
-            struct argv argv = argv_new();
-            const char *dc_file = platform_create_temp_file(mi->context.options.tmp_dir,
-                                                            "cc", &gc);
+    /* set our client's VPN endpoint for status reporting purposes */
+    mi->reporting_addr = mi->context.c2.push_ifconfig_local;
+    mi->reporting_addr_ipv6 = mi->context.c2.push_ifconfig_ipv6_local;
 
-            if (!dc_file)
-            {
-                cc_succeeded = false;
-                goto script_depr_failed;
-            }
+    /* set context-level authentication flag */
+    mi->context.c2.context_auth = CAS_SUCCEEDED;
 
-            argv_printf(&argv, "%s", dc_file);
-            if (plugin_call(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT, &argv, NULL, mi->context.c2.es) != OPENVPN_PLUGIN_FUNC_SUCCESS)
-            {
-                msg(M_WARN, "WARNING: client-connect plugin call failed");
-                cc_succeeded = false;
-            }
-            else
-            {
-                multi_client_connect_post(m, mi, dc_file, option_permissions_mask, &option_types_found);
-                ++cc_succeeded_count;
-            }
+#ifdef ENABLE_ASYNC_PUSH
+    /* authentication complete, send push reply */
+    if (mi->context.c2.push_request_received)
+    {
+        process_incoming_push_request(&mi->context);
+    }
+#endif
+    gc_free(&gc);
+}
 
-            if (!platform_unlink(dc_file))
-            {
-                msg(D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
-                    dc_file);
-            }
 
-script_depr_failed:
-            argv_reset(&argv);
-        }
+static void
+multi_client_connect_early_setup(struct multi_context *m,
+                                 struct multi_instance *mi)
+{
+    ASSERT(mi->context.c1.tuntap);
+    /*
+     * lock down the common name and cert hashes so they can't change
+     * during future TLS renegotiations
+     */
+    tls_lock_common_name(mi->context.c2.tls_multi);
+    tls_lock_cert_hash_set(mi->context.c2.tls_multi);
 
-        /* V2 callback, use a plugin_return struct for passing back return info */
-        if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT_V2))
-        {
-            struct plugin_return pr;
+    /* generate a msg() prefix for this client instance */
+    generate_prefix(mi);
 
-            plugin_return_init(&pr);
+    /* delete instances of previous clients with same common-name */
+    if (!mi->context.options.duplicate_cn)
+    {
+        multi_delete_dup(m, mi);
+    }
 
-            if (plugin_call(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT_V2, NULL, &pr, mi->context.c2.es) != OPENVPN_PLUGIN_FUNC_SUCCESS)
-            {
-                msg(M_WARN, "WARNING: client-connect-v2 plugin call failed");
-                cc_succeeded = false;
-            }
-            else
-            {
-                multi_client_connect_post_plugin(m, mi, &pr, option_permissions_mask, &option_types_found);
-                ++cc_succeeded_count;
-            }
+    /* reset pool handle to null */
+    mi->vaddr_handle = -1;
+}
 
-            plugin_return_free(&pr);
-        }
-#endif /* ifdef ENABLE_PLUGIN */
+/**
+ * Try to source a dynamic config file from the
+ * --client-config-dir directory.
+ */
+static void
+multi_client_connect_source_ccd(struct multi_context *m,
+                                struct multi_instance *mi,
+                                unsigned int *option_types_found)
+{
+    if (mi->context.options.client_config_dir)
+    {
+        struct gc_arena gc = gc_new();
+        const char *ccd_file;
 
-        /*
-         * Run --client-connect script.
-         */
-        if (mi->context.options.client_connect_script && cc_succeeded)
-        {
-            struct argv argv = argv_new();
-            const char *dc_file = NULL;
+        ccd_file = platform_gen_path(mi->context.options.client_config_dir,
+                                     tls_common_name(mi->context.c2.tls_multi,
+                                                     false),
+                                     &gc);
 
-            setenv_str(mi->context.c2.es, "script_type", "client-connect");
+        /* try common-name file */
+        if (platform_test_file(ccd_file))
+        {
+            options_server_import(&mi->context.options,
+                                  ccd_file,
+                                  D_IMPORT_ERRORS|M_OPTERR,
+                                  CLIENT_CONNECT_OPT_MASK,
+                                  option_types_found,
+                                  mi->context.c2.es);
+        }
+        else /* try default file */
+        {
+            ccd_file = platform_gen_path(mi->context.options.client_config_dir,
+                                         CCD_DEFAULT,
+                                         &gc);
 
-            dc_file = platform_create_temp_file(mi->context.options.tmp_dir,
-                                                "cc", &gc);
-            if (!dc_file)
+            if (platform_test_file(ccd_file))
             {
-                cc_succeeded = false;
-                goto script_failed;
+                options_server_import(&mi->context.options,
+                                      ccd_file,
+                                      D_IMPORT_ERRORS|M_OPTERR,
+                                      CLIENT_CONNECT_OPT_MASK,
+                                      option_types_found,
+                                      mi->context.c2.es);
             }
+        }
+        gc_free(&gc);
+    }
+}
+
+/*
+ * Called as soon as the SSL/TLS connection authenticates.
+ *
+ * Instance-specific directives to be processed:
+ *
+ *   iroute start-ip end-ip
+ *   ifconfig-push local remote-netmask
+ *   push
+ */
+static void
+multi_connection_established(struct multi_context *m, struct multi_instance *mi)
+{
+    if (tls_authentication_status(mi->context.c2.tls_multi, 0)
+        == TLS_AUTHENTICATION_SUCCEEDED)
+    {
+        unsigned int option_types_found = 0;
 
-            argv_parse_cmd(&argv, mi->context.options.client_connect_script);
-            argv_printf_cat(&argv, "%s", dc_file);
+        int cc_succeeded = true; /* client connect script status */
+        int cc_succeeded_count = 0;
 
-            if (openvpn_run_script(&argv, mi->context.c2.es, 0, "--client-connect"))
-            {
-                multi_client_connect_post(m, mi, dc_file, option_permissions_mask, &option_types_found);
-                ++cc_succeeded_count;
-            }
-            else
-            {
-                cc_succeeded = false;
-            }
+        multi_client_connect_early_setup(m, mi);
 
-            if (!platform_unlink(dc_file))
-            {
-                msg(D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
-                    dc_file);
-            }
+        multi_client_connect_source_ccd(m, mi, &option_types_found);
 
-script_failed:
-            argv_reset(&argv);
-        }
+        /*
+         * Select a virtual address from either --ifconfig-push in
+         * --client-config-dir file or --ifconfig-pool.
+         */
+        multi_select_virtual_addr(m, mi);
+
+        /* do --client-connect setenvs */
+        multi_client_connect_setenv(m, mi);
+
+        multi_client_connect_call_plugin_v1(m, mi, &option_types_found,
+                                            &cc_succeeded,
+                                            &cc_succeeded_count);
+
+        multi_client_connect_call_plugin_v2(m, mi, &option_types_found,
+                                            &cc_succeeded,
+                                            &cc_succeeded_count);
 
         /*
          * Check for client-connect script left by management interface client
          */
+
+        if (cc_succeeded)
+        {
+            multi_client_connect_call_script(m, mi, &option_types_found,
+                                             &cc_succeeded,
+                                             &cc_succeeded_count);
+
+        }
 #ifdef MANAGEMENT_DEF_AUTH
         if (cc_succeeded && mi->cc_config)
         {
-            multi_client_connect_mda(m, mi, mi->cc_config, option_permissions_mask, &option_types_found);
-            ++cc_succeeded_count;
+            multi_client_connect_mda(m, mi, &option_types_found);
+            cc_succeeded_count++;
         }
 #endif
 
@@ -1991,99 +2158,21 @@ script_failed:
          */
         if (mi->context.options.disable)
         {
-            msg(D_MULTI_ERRORS, "MULTI: client has been rejected due to 'disable' directive");
+            msg(D_MULTI_ERRORS, "MULTI: client has been rejected due to"
+                "'disable' directive");
             cc_succeeded = false;
             cc_succeeded_count = 0;
         }
 
         if (cc_succeeded)
         {
-            /*
-             * Process sourced options.
-             */
-            do_deferred_options(&mi->context, option_types_found);
-
-            /*
-             * make sure we got ifconfig settings from somewhere
-             */
-            if (!mi->context.c2.push_ifconfig_defined)
-            {
-                msg(D_MULTI_ERRORS, "MULTI: no dynamic or static remote --ifconfig address is available for %s",
-                    multi_instance_string(mi, false, &gc));
-            }
-
-            /*
-             * make sure that ifconfig settings comply with constraints
-             */
-            if (!ifconfig_push_constraint_satisfied(&mi->context))
-            {
-                /* JYFIXME -- this should cause the connection to fail */
-                msg(D_MULTI_ERRORS, "MULTI ERROR: primary virtual IP for %s (%s) violates tunnel network/netmask constraint (%s/%s)",
-                    multi_instance_string(mi, false, &gc),
-                    print_in_addr_t(mi->context.c2.push_ifconfig_local, 0, &gc),
-                    print_in_addr_t(mi->context.options.push_ifconfig_constraint_network, 0, &gc),
-                    print_in_addr_t(mi->context.options.push_ifconfig_constraint_netmask, 0, &gc));
-            }
-
-            /*
-             * For routed tunnels, set up internal route to endpoint
-             * plus add all iroute routes.
-             */
-            if (TUNNEL_TYPE(mi->context.c1.tuntap) == DEV_TYPE_TUN)
-            {
-                if (mi->context.c2.push_ifconfig_defined)
-                {
-                    multi_learn_in_addr_t(m, mi, mi->context.c2.push_ifconfig_local, -1, true);
-                    msg(D_MULTI_LOW, "MULTI: primary virtual IP for %s: %s",
-                        multi_instance_string(mi, false, &gc),
-                        print_in_addr_t(mi->context.c2.push_ifconfig_local, 0, &gc));
-                }
-
-                if (mi->context.c2.push_ifconfig_ipv6_defined)
-                {
-                    multi_learn_in6_addr(m, mi, mi->context.c2.push_ifconfig_ipv6_local, -1, true);
-                    /* TODO: find out where addresses are "unlearned"!! */
-                    msg(D_MULTI_LOW, "MULTI: primary virtual IPv6 for %s: %s",
-                        multi_instance_string(mi, false, &gc),
-                        print_in6_addr(mi->context.c2.push_ifconfig_ipv6_local, 0, &gc));
-                }
-
-                /* add routes locally, pointing to new client, if
-                 * --iroute options have been specified */
-                multi_add_iroutes(m, mi);
-
-                /*
-                 * iroutes represent subnets which are "owned" by a particular
-                 * client.  Therefore, do not actually push a route to a client
-                 * if it matches one of the client's iroutes.
-                 */
-                remove_iroutes_from_push_route_list(&mi->context.options);
-            }
-            else if (mi->context.options.iroutes)
-            {
-                msg(D_MULTI_ERRORS, "MULTI: --iroute options rejected for %s -- iroute only works with tun-style tunnels",
-                    multi_instance_string(mi, false, &gc));
-            }
-
-            /* set our client's VPN endpoint for status reporting purposes */
-            mi->reporting_addr = mi->context.c2.push_ifconfig_local;
-            mi->reporting_addr_ipv6 = mi->context.c2.push_ifconfig_ipv6_local;
-
-            /* set context-level authentication flag */
-            mi->context.c2.context_auth = CAS_SUCCEEDED;
-
-#ifdef ENABLE_ASYNC_PUSH
-            /* authentication complete, send push reply */
-            if (mi->context.c2.push_request_received)
-            {
-                process_incoming_push_request(&mi->context);
-            }
-#endif
+            multi_client_connect_late_setup(m, mi, option_types_found);
         }
         else
         {
             /* set context-level authentication flag */
-            mi->context.c2.context_auth = cc_succeeded_count ? CAS_PARTIAL : CAS_FAILED;
+            mi->context.c2.context_auth =
+                cc_succeeded_count ? CAS_PARTIAL : CAS_FAILED;
         }
 
         /* set flag so we don't get called again */
@@ -2097,11 +2186,11 @@ script_failed:
 #ifdef MANAGEMENT_DEF_AUTH
         if (management)
         {
-            management_connection_established(management, &mi->context.c2.mda_context, mi->context.c2.es);
+            management_connection_established
+                (management, &mi->context.c2.mda_context, mi->context.c2.es);
         }
 #endif
 
-        gc_free(&gc);
     }
 
     /*
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 3d3d6875..489c073a 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -628,7 +628,9 @@ multi_process_outgoing_tun(struct multi_context *m, const unsigned int mpp_flags
     return ret;
 }
 
-
+#define CLIENT_CONNECT_OPT_MASK (OPT_P_INSTANCE | OPT_P_INHERIT   \
+                                 |OPT_P_PUSH | OPT_P_TIMER | OPT_P_CONFIG   \
+                                 |OPT_P_ECHO | OPT_P_COMP | OPT_P_SOCKFLAGS)
 
 static inline bool
 multi_process_outgoing_link_dowork(struct multi_context *m, struct multi_instance *mi, const unsigned int mpp_flags)
-- 
2.19.1

[Openvpn-devel] [PATCH v4 05/13] client-connect: Refactor client-connect handling to calling a bunch of hooks in a loop

[Arne S. <ar...@rf...>]

From: Fabian Knittel <fab...@le...>

This patch changes the calling of the client-connect functions into an array
of hooks and a block of code that calls them in a loop.

Signed-off-by: Fabian Knittel <fab...@le...>
Signed-off-by: Arne Schwabe <ar...@rf...>
---
 src/openvpn/multi.c | 44 ++++++++++++++++----------------------------
 1 file changed, 16 insertions(+), 28 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 4182b553..18a1313e 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2144,6 +2144,20 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
     if (tls_authentication_status(mi->context.c2.tls_multi, 0)
         == TLS_AUTHENTICATION_SUCCEEDED)
     {
+        typedef enum client_connect_return
+        (*multi_client_connect_handler)
+            (struct multi_context *m, struct multi_instance *mi,
+            unsigned int *option_types_found);
+
+        multi_client_connect_handler handlers[] = {
+            multi_client_connect_source_ccd,
+            multi_client_connect_call_plugin_v1,
+            multi_client_connect_call_plugin_v2,
+            multi_client_connect_call_script,
+            multi_client_connect_mda,
+            NULL
+        };
+
         unsigned int option_types_found = 0;
 
         int cc_succeeded = true; /* client connect script status */
@@ -2152,35 +2166,9 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
 
         multi_client_connect_early_setup(m, mi);
 
-        ret = multi_client_connect_source_ccd(m, mi, &option_types_found);
-        cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
-
-        if (cc_succeeded)
-        {
-            ret = multi_client_connect_call_plugin_v1(m, mi,
-                                                      &option_types_found);
-            cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
-        }
-
-        if (cc_succeeded)
-        {
-            ret = multi_client_connect_call_plugin_v2(m, mi,
-                                                      &option_types_found);
-            cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
-        }
-
-        /*
-         * Check for client-connect script left by management interface client
-         */
-        if (cc_succeeded)
-        {
-            ret = multi_client_connect_call_script(m, mi, &option_types_found);
-            cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
-        }
-
-        if (cc_succeeded)
+        for (int i = 0; cc_succeeded && handlers[i]; i++)
         {
-            ret = multi_client_connect_mda(m, mi, &option_types_found);
+            ret = handlers[i](m, mi, &option_types_found);
             cc_succeeded = cc_check_return(&cc_succeeded_count, ret);
         }
 
-- 
2.19.1

[Openvpn-devel] [PATCH v4 09/13] client-connect: Move adding inotify watch into its own function

[Arne S. <ar...@rf...>]

This make the code a bit better readable and also prepares resuing
the function for client-connect return files

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 src/openvpn/multi.c | 46 +++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index df6e4610..63d9db05 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2710,6 +2710,32 @@ 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)
+static void
+add_inotify_file_watch(struct multi_context *m, struct multi_instance *mi,
+                       int inotify_fd, const char *file)
+{
+    /* watch acf file */
+    long watch_descriptor = inotify_add_watch(inotify_fd, file,
+                                              IN_CLOSE_WRITE | IN_ONESHOT);
+    if (watch_descriptor >= 0)
+    {
+        if (mi->inotify_watch != -1)
+        {
+            hash_remove(m->inotify_watchers,
+                        (void *) (unsigned long)mi->inotify_watch);
+        }
+        hash_add(m->inotify_watchers, (const uintptr_t *)watch_descriptor,
+                 mi, true);
+        mi->inotify_watch = watch_descriptor;
+    }
+    else
+    {
+        msg(M_NONFATAL | M_ERRNO, "MULTI: inotify_add_watch error");
+    }
+}
+#endif /* if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH) */
+
 /*
  * Figure instance-specific timers, convert
  * earliest to absolute time in mi->wakeup,
@@ -2739,23 +2765,11 @@ 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 (ks && ks->auth_control_file
+            && ks->auth_deferred && !was_authenticated)
         {
-            /* watch acf file */
-            long watch_descriptor = inotify_add_watch(m->top.c2.inotify_fd, ks->auth_control_file, IN_CLOSE_WRITE | IN_ONESHOT);
-            if (watch_descriptor >= 0)
-            {
-                if (mi->inotify_watch != -1)
-                {
-                    hash_remove(m->inotify_watchers, (void *) (unsigned long)mi->inotify_watch);
-                }
-                hash_add(m->inotify_watchers, (const uintptr_t *)watch_descriptor, mi, true);
-                mi->inotify_watch = watch_descriptor;
-            }
-            else
-            {
-                msg(M_NONFATAL | M_ERRNO, "MULTI: inotify_add_watch error");
-            }
+            add_inotify_file_watch(m, mi, m->top.c2.inotify_fd,
+                                   ks->auth_control_file);
         }
 #endif
 
-- 
2.19.1

[Openvpn-devel] [PATCH v4 11/13] client-connect: Add deferred support to the client-connect plugin v1 handler

[Arne S. <ar...@rf...>]

From: Fabian Knittel <fab...@le...>

Uses the infrastructure provided and used in the previous patch to provide
deferral support to the v1 client-connect plugin handler as well.

Signed-off-by: Fabian Knittel <fab...@le...>

PATCH V3: Modify the API to also (optionally) call the plugin on a deferred
call. This allows the plugin authors to be more flexible and make the V1 API
more similar to the V2 API.

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 include/openvpn-plugin.h.in | 30 ++++++------
 src/openvpn/multi.c         | 97 ++++++++++++++++++++++++++++---------
 src/openvpn/plugin.c        |  3 ++
 3 files changed, 94 insertions(+), 36 deletions(-)

diff --git a/include/openvpn-plugin.h.in b/include/openvpn-plugin.h.in
index 103844f7..38fbe097 100644
--- a/include/openvpn-plugin.h.in
+++ b/include/openvpn-plugin.h.in
@@ -116,20 +116,22 @@ extern "C" {
  * FUNC: openvpn_plugin_client_destructor_v1 (top-level "generic" client)
  * FUNC: openvpn_plugin_close_v1
  */
-#define OPENVPN_PLUGIN_UP                    0
-#define OPENVPN_PLUGIN_DOWN                  1
-#define OPENVPN_PLUGIN_ROUTE_UP              2
-#define OPENVPN_PLUGIN_IPCHANGE              3
-#define OPENVPN_PLUGIN_TLS_VERIFY            4
-#define OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY 5
-#define OPENVPN_PLUGIN_CLIENT_CONNECT        6
-#define OPENVPN_PLUGIN_CLIENT_DISCONNECT     7
-#define OPENVPN_PLUGIN_LEARN_ADDRESS         8
-#define OPENVPN_PLUGIN_CLIENT_CONNECT_V2     9
-#define OPENVPN_PLUGIN_TLS_FINAL             10
-#define OPENVPN_PLUGIN_ENABLE_PF             11
-#define OPENVPN_PLUGIN_ROUTE_PREDOWN         12
-#define OPENVPN_PLUGIN_N                     13
+#define OPENVPN_PLUGIN_UP                        0
+#define OPENVPN_PLUGIN_DOWN                      1
+#define OPENVPN_PLUGIN_ROUTE_UP                  2
+#define OPENVPN_PLUGIN_IPCHANGE                  3
+#define OPENVPN_PLUGIN_TLS_VERIFY                4
+#define OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY     5
+#define OPENVPN_PLUGIN_CLIENT_CONNECT            6
+#define OPENVPN_PLUGIN_CLIENT_DISCONNECT         7
+#define OPENVPN_PLUGIN_LEARN_ADDRESS             8
+#define OPENVPN_PLUGIN_CLIENT_CONNECT_V2         9
+#define OPENVPN_PLUGIN_TLS_FINAL                10
+#define OPENVPN_PLUGIN_ENABLE_PF                11
+#define OPENVPN_PLUGIN_ROUTE_PREDOWN            12
+#define OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER     13
+#define OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER_V2  14
+#define OPENVPN_PLUGIN_N                        15
 
 /*
  * Build a mask out of a set of plug-in types.
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 09974357..c14a21e4 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1938,55 +1938,108 @@ ccs_gen_config_file(struct multi_instance *mi)
 static enum client_connect_return
 multi_client_connect_call_plugin_v1(struct multi_context *m,
                                     struct multi_instance *mi,
-                                    unsigned int *option_types_found)
+                                    unsigned int *option_types_found,
+                                    bool deferred)
 {
     enum client_connect_return ret = CC_RET_SKIPPED;
 #ifdef ENABLE_PLUGIN
     ASSERT(m);
     ASSERT(mi);
     ASSERT(option_types_found);
+    struct client_connect_defer_state *ccs = &(mi->client_connect_defer_state);
 
     /* deprecated callback, use a file for passing back return info */
     if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
     {
         struct argv argv = argv_new();
-        struct gc_arena gc = gc_new();
-        const char *dc_file =
-            platform_create_temp_file(mi->context.options.tmp_dir, "cc", &gc);
-
-        if (!dc_file)
+        int call;
+        if (!deferred)
         {
-            ret = CC_RET_FAILED;
-            goto cleanup;
+            call = OPENVPN_PLUGIN_CLIENT_CONNECT;
+            if (!ccs_gen_config_file(mi)
+                || !ccs_gen_deferred_ret_file(mi))
+            {
+                ret = CC_RET_FAILED;
+                goto cleanup;
+            }
+        }
+        else
+        {
+            call = OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER;
+            /* the initial call should have create these files */
+            ASSERT(ccs->config_file);
+            ASSERT(ccs->deferred_ret_file);
         }
 
-        argv_printf(&argv, "%s", dc_file);
-        if (plugin_call(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT,
-                        &argv, NULL, mi->context.c2.es)
-            != OPENVPN_PLUGIN_FUNC_SUCCESS)
+        argv_printf(&argv, "%s", ccs->config_file);
+        int plug_ret = plugin_call(mi->context.plugins, call,
+                                   &argv, NULL, mi->context.c2.es);
+        if (plug_ret == OPENVPN_PLUGIN_FUNC_SUCCESS)
         {
-            msg(M_WARN, "WARNING: client-connect plugin call failed");
-            ret = CC_RET_FAILED;
+            multi_client_connect_post(m, mi, ccs->config_file,
+                                      option_types_found);
+            ret = CC_RET_SUCCEEDED;
+        }
+        else if (plug_ret == OPENVPN_PLUGIN_FUNC_DEFERRED)
+        {
+            ret = CC_RET_DEFERRED;
+            /**
+             * Contrary to the plugin v2 API, we do not dmeand a working
+             * deferred plugin as all return can be handled by the files
+             * and plugin_call return success if a plugin is not defined
+             */
         }
         else
         {
-            multi_client_connect_post(m, mi, dc_file, option_types_found);
-            ret = CC_RET_SUCCEEDED;
+            msg(M_WARN, "WARNING: client-connect plugin call failed");
+            ret = CC_RET_FAILED;
         }
 
+
+        /**
+         * plugin api v1 client connect async feature has both plugin and
+         * file return status, so in case that the file has a code that
+         * demands override, we override our return code
+         */
+        int file_ret = ccs_test_deferred_ret_file(mi);
+
+        if (file_ret == CC_RET_FAILED)
         {
-            msg(D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
-                dc_file);
+            ret = CC_RET_FAILED;
+        }
+        else if (ret == CC_RET_SUCCEEDED && file_ret == CC_RET_DEFERRED)
+        {
+            ret = CC_RET_DEFERRED;
         }
-
 cleanup:
         argv_reset(&argv);
-        gc_free(&gc);
+
+        if (ret != CC_RET_DEFERRED)
+        {
+            ccs_delete_config_file(mi);
+            ccs_delete_deferred_ret_file(mi);
+        }
     }
 #endif /* ifdef ENABLE_PLUGIN */
     return ret;
 }
 
+static enum client_connect_return
+multi_client_connect_call_plugin_v1_initial(struct multi_context *m,
+                                            struct multi_instance *mi,
+                                            unsigned int *option_types_found)
+{
+    return multi_client_connect_call_plugin_v1(m,mi, option_types_found, false);
+}
+
+static enum client_connect_return
+multi_client_connect_call_plugin_v1_deferred(struct multi_context *m,
+                                             struct multi_instance *mi,
+                                             unsigned int *option_types_found)
+{
+    return multi_client_connect_call_plugin_v1(m,mi, option_types_found, true);
+}
+
 static enum client_connect_return
 multi_client_connect_call_plugin_v2(struct multi_context *m,
                                     struct multi_instance *mi,
@@ -2333,8 +2386,8 @@ static const struct client_connect_handlers client_connect_handlers[] = {
         .deferred = multi_client_connect_fail
     },
     {
-        .main = multi_client_connect_call_plugin_v1,
-        .deferred = multi_client_connect_fail
+        .main = multi_client_connect_call_plugin_v1_initial,
+        .deferred = multi_client_connect_call_plugin_v1_deferred,
     },
     {
         .main = multi_client_connect_call_plugin_v2,
diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
index 4d17c821..51c130c1 100644
--- a/src/openvpn/plugin.c
+++ b/src/openvpn/plugin.c
@@ -104,6 +104,9 @@ plugin_type_name(const int type)
         case OPENVPN_PLUGIN_CLIENT_CONNECT_V2:
             return "PLUGIN_CLIENT_CONNECT";
 
+        case OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER:
+            return "PLUGIN_CLIENT_CONNECT";
+
         case OPENVPN_PLUGIN_CLIENT_DISCONNECT:
             return "PLUGIN_CLIENT_DISCONNECT";
 
-- 
2.19.1

[Openvpn-devel] [PATCH v4 12/13] client-connect: Implement deferred connect support for plugin API v2

[Arne S. <ar...@rf...>]

The V2 API is simpler than the V1 API since there is no passing of
data via files. This also means that with the current API the V2 API
cannot support async notify via files. Adding a file just for async
notify seems very hacky and when needed we should implement a better
option when async is needed for the plugin V2 API

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 src/openvpn/multi.c  | 58 +++++++++++++++++++++++++++++++++++---------
 src/openvpn/plugin.c |  3 +++
 2 files changed, 50 insertions(+), 11 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index c14a21e4..c94716ae 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2043,7 +2043,8 @@ multi_client_connect_call_plugin_v1_deferred(struct multi_context *m,
 static enum client_connect_return
 multi_client_connect_call_plugin_v2(struct multi_context *m,
                                     struct multi_instance *mi,
-                                    unsigned int *option_types_found)
+                                    unsigned int *option_types_found,
+                                    bool deferred)
 {
     enum client_connect_return ret = CC_RET_SKIPPED;
 #ifdef ENABLE_PLUGIN
@@ -2051,32 +2052,67 @@ multi_client_connect_call_plugin_v2(struct multi_context *m,
     ASSERT(mi);
     ASSERT(option_types_found);
 
+    int call = deferred ? OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER_V2 :
+               OPENVPN_PLUGIN_CLIENT_CONNECT_V2;
     /* V2 callback, use a plugin_return struct for passing back return info */
-    if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT_V2))
+    if (plugin_defined(mi->context.plugins, call))
     {
         struct plugin_return pr;
 
         plugin_return_init(&pr);
 
-        if (plugin_call(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT_V2,
-                        NULL, &pr, mi->context.c2.es)
-            != OPENVPN_PLUGIN_FUNC_SUCCESS)
+        int plug_ret = plugin_call(mi->context.plugins, call,
+                                   NULL, &pr, mi->context.c2.es);
+        if (plug_ret == OPENVPN_PLUGIN_FUNC_SUCCESS)
         {
-            msg(M_WARN, "WARNING: client-connect-v2 plugin call failed");
-            ret = CC_RET_FAILED;
+            multi_client_connect_post_plugin(m, mi, &pr, option_types_found);
+            ret = CC_RET_SUCCEEDED;
+        }
+        else if (plug_ret == OPENVPN_PLUGIN_FUNC_DEFERRED)
+        {
+            ret = CC_RET_DEFERRED;
+            if (!(plugin_defined(mi->context.plugins,
+                                 OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER_V2)))
+            {
+                msg(M_WARN, "A plugin that defers from the "
+                    "OPENVPN_PLUGIN_CLIENT_CONNECT_V2 call must also "
+                    "declare support for "
+                    "OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER_V2");
+                ret = CC_RET_FAILED;
+            }
         }
         else
         {
-            multi_client_connect_post_plugin(m, mi, &pr, option_types_found);
-            ret = CC_RET_SUCCEEDED;
+            msg(M_WARN, "WARNING: client-connect-v2 plugin call failed");
+            ret = CC_RET_FAILED;
         }
 
+
         plugin_return_free(&pr);
     }
 #endif /* ifdef ENABLE_PLUGIN */
     return ret;
 }
 
+
+static enum client_connect_return
+multi_client_connect_call_plugin_v2_initial(struct multi_context *m,
+                                            struct multi_instance *mi,
+                                            unsigned int *option_types_found)
+{
+    return multi_client_connect_call_plugin_v2(m, mi, option_types_found,
+                                               false);
+}
+
+static enum client_connect_return
+multi_client_connect_call_plugin_v2_deferred(struct multi_context *m,
+                                             struct multi_instance *mi,
+                                             unsigned int *option_types_found)
+{
+    return multi_client_connect_call_plugin_v2(m, mi, option_types_found,
+                                               true);
+}
+
 /**
  * Runs the --client-connect script if one is defined.
  */
@@ -2390,8 +2426,8 @@ static const struct client_connect_handlers client_connect_handlers[] = {
         .deferred = multi_client_connect_call_plugin_v1_deferred,
     },
     {
-        .main = multi_client_connect_call_plugin_v2,
-        .deferred = multi_client_connect_fail
+        .main = multi_client_connect_call_plugin_v2_initial,
+        .deferred = multi_client_connect_call_plugin_v2_deferred,
     },
     {
         .main = multi_client_connect_call_script,
diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
index 51c130c1..347acade 100644
--- a/src/openvpn/plugin.c
+++ b/src/openvpn/plugin.c
@@ -107,6 +107,9 @@ plugin_type_name(const int type)
         case OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER:
             return "PLUGIN_CLIENT_CONNECT";
 
+        case OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER_V2:
+            return "PLUGIN_CLIENT_CONNECT";
+
         case OPENVPN_PLUGIN_CLIENT_DISCONNECT:
             return "PLUGIN_CLIENT_DISCONNECT";
 
-- 
2.19.1

[Openvpn-devel] [PATCH v4 13/13] client-connect: Add documentation for the deferred client connect feature

[Arne S. <ar...@rf...>]

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 doc/openvpn.8               | 47 ++++++++++++++++++++++++++++++++++---
 include/openvpn-plugin.h.in | 21 ++++++++++++-----
 2 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index 94b5cc4f..9377bbf5 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -3346,6 +3346,13 @@ is significant.  If
 .B script
 returns a non\-zero error status, it will cause the client
 to be disconnected.
+
+If a
+.B \-\-client\-connect cmd
+wants to defer the generating the configuration the script should
+use the client_connect_deferred_file and client_connect_config_file
+environment variables and write status acorrdingly into these files
+(See the environment section below for more details).
 .\"*********************************************************
 .TP
 .B \-\-client\-disconnect cmd
@@ -3428,12 +3435,18 @@ This directory will be used by in the following cases:
 
 *
 .B \-\-client\-connect
-scripts to dynamically generate client\-specific
-configuration files.
+scripts and
+.B OPENVPN_PLUGIN_CLIENT_CONNECT
+plugin hook 
+to dynamically generate client\-specific configuration files
+and return success/failure via client_connect_deferred_file
+when using deferred client connect method
 
 *
 .B OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY
-plugin hook to return success/failure via auth_control_file
+and 
+
+plugin hook to return success/failure via auth_control_file/
 when using deferred auth method
 
 *
@@ -6431,6 +6444,34 @@ Set prior to execution of the
 script.
 .\"*********************************************************
 .TP
+.B client_connect_config_file
+The path to the configuration file that should be written by
+the
+.B \-\-client\-connect
+The content of this enviroment variable is identical to the
+file as a argument of the called
+.B \-\-client\-connect
+script.
+.\"*********************************************************
+.TP
+.B client_connect_defferred_file
+This file can be optionally written to communicate a status
+code of the
+.TP
+.B \-\-client\-connect
+script. If used for deferring, this file must be written
+before the
+.B \-\-client\-connect
+script exits. The first character in the file is interpreted
+and 1 is equal to normal script execution, 0 indicated and
+error (in the same way that a non zero exit status does) and
+2 indicates that the script deferred returning the config
+file. When the script defers the executing it must first write 2
+during the execution of the script. A background process or similar
+must then take of writing the client_connect_config_file and when
+finished, write the a 1 to this file.
+.\"*********************************************************
+.TP
 .B common_name
 The X509 common name of an authenticated client.
 Set prior to execution of
diff --git a/include/openvpn-plugin.h.in b/include/openvpn-plugin.h.in
index 38fbe097..fce59422 100644
--- a/include/openvpn-plugin.h.in
+++ b/include/openvpn-plugin.h.in
@@ -557,12 +557,21 @@ OPENVPN_PLUGIN_DEF openvpn_plugin_handle_t OPENVPN_PLUGIN_FUNC(openvpn_plugin_op
  * OPENVPN_PLUGIN_FUNC_SUCCESS on success, OPENVPN_PLUGIN_FUNC_ERROR on failure
  *
  * In addition, OPENVPN_PLUGIN_FUNC_DEFERRED may be returned by
- * OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY.  This enables asynchronous
- * authentication where the plugin (or one of its agents) may indicate
- * authentication success/failure some number of seconds after the return
- * of the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY handler by writing a single
- * char to the file named by auth_control_file in the environmental variable
- * list (envp).
+ * OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, OPENVPN_PLUGIN_CLIENT_CONNECT and
+ * OPENVPN_PLUGIN_CLIENT_CONNECT_V2. This enables asynchronous
+ * authentication or client connect  where the plugin (or one of its agents) 
+ * may indicate authentication success/failure or client configuration some
+ * number of seconds after the return of the function handler.
+ * For OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY and OPENVPN_PLUGIN_CLIENT_CONNECT
+ * this done by writing a single char to the file named by
+ * auth_control_file/client_connect_deferred_file
+ * in the environmental variable list (envp).
+ *
+ * In Addition the OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER and 
+ * OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER_V2 are called when OpenVPN tries to
+ * get the deferred result. For V2 call implementing this function is 
+ * required as information is not passed by files. For the normal version
+ * the call is optional.
  *
  * first char of auth_control_file:
  * '0' -- indicates auth failure
-- 
2.19.1

[Openvpn-devel] [PATCH applied] Re: Remove extra token after #endif

[Gert D. <ge...@gr...>]

Acked-by: Gert Doering <ge...@gr...>

Thanks.

Regarding Christian's comment about "#endif" vs. "#endif /* ENABLE_CRYPTO /*"
- well, we have both in our tree, so neither is "correct" or "wrong".

For such a short #ifdef/#endif span, I'd see it as optional because the
corresponding #ifdef is easily spotted.  For something spanning more
code, possibly nested #ifdef, a comment after the #endif would definitely
be better, though.

Your patch has been applied to the release/2.4 branch.

commit ca962f3837ab05b0e8a4382960391f289497eaa2
Author: Lev Stipakov
Date:   Fri Nov 9 11:59:33 2018 +0200

     Remove extra token after #endif

     Signed-off-by: Lev Stipakov <lst...@gm...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <154...@gm...>
     URL: https://www.mail-archive.com/ope...@li.../msg17883.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: tls-crypt-v2: fix client reconnect bug

[Gert D. <ge...@gr...>]

Your patch has been applied to the master branch.

(Gave it a test run, but didn't really look hard at the code)

commit 19d6d9c3533b83f934ea93359bca086a5d06011a
Author: Steffan Karger
Date:   Wed Oct 31 14:07:16 2018 +0100

     tls-crypt-v2: fix client reconnect bug

     Signed-off-by: Steffan Karger <ste...@fo...>
     Acked-by: Antonio Quartulli <an...@op...>
     Message-Id: <154...@fo...>
     URL: https://www.mail-archive.com/ope...@li.../msg17866.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: tls-crypt-v2: clarify --tls-crypt-v2-genkey man page section

[Gert D. <ge...@gr...>]

Documentation is always welcome :-)

Your patch has been applied to the master branch.

commit 01039891ece9f38f7a17c80e5afc261ab5bcbaf3
Author: Steffan Karger
Date:   Wed Oct 31 11:22:57 2018 +0100

     tls-crypt-v2: clarify --tls-crypt-v2-genkey man page section

     Signed-off-by: Steffan Karger <ste...@fo...>
     Acked-by: Antonio Quartulli <an...@op...>
     Message-Id: <154...@fo...>
     URL: https://www.mail-archive.com/ope...@li.../msg17865.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH v5 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key

[Arne S. <ar...@rf...>]

>  
> 
>     > (ii) tls version max is set 1.2 and openssl 1.1.1 is in use both on
>     > server and client.
>     > PSS signing will get negotiated but we will not error out early as TLS
>     > 1.3 is not in  use.
>     >
>     > That's why I say that this extension of management-external-key is
>     not worth it.
>     >
>     > Am I missing something?
>     >
> 
>     tls_version_max will still report TLS 1.3 as it not affected by the
>     version set in config but really the max the library is capable of
>     irrespectable tls min/max version.
> 
> 
> Aha, I missed that. Still I really do not understand the need for
> erroring here
> instead of when prompting for PK_SIGN based on client version.
> Much simpler.

It did this because my initial OpenSSL 1.1.1 client did not have any
problem, only after I upgraded the server to 1.1.1. I want to avoid the
situation that OpenSSL 1.1.1 is used and then just "breaks for no
reason" and erroring out early and tell the "This will not really work"
is a better approach.

Arne

Re: [Openvpn-devel] [PATCH v5 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key

[Selva N. <sel...@gm...>]

On Thu, Nov 15, 2018 at 2:22 AM Arne Schwabe <ar...@rf...> wrote:

>
> >> Unless I overlooked something, I don't see any situation in which we ask
> >> for an unsupported signature.
> >
> > Consider this:
> > (i) config has --management-external-key nopadding but client announces
> version
> > 2. We will not error out but send the signature request as
> > PK_SIGN <base64data>
> > without the ALG as client version is not 3 and fail
>
> We can error out on version on the management line. But this is also a
> kind of obscure misconfiguration since why would add nopadding to the
> config if you cannot support it?
>

True, but by same argument then we need not worry about backward
compatibility
of management  protocol at all, do we? We could say, why would any one use
openvpn.exe that speaks MI version 3 with a client that cannot support it.

Anyway, for backward compat its enough to check client version and then
either error out if ALG is not PKCS1 or ECDSA or add ALG to PK_SIGN.

I fail to appreciate the utility of early erroring out feature -- it was
meaningful
in v1 where you had tried to downgrade to TLS 1.2 to plough along without
causing an error. But that turned out to be not enough.


> > (ii) tls version max is set 1.2 and openssl 1.1.1 is in use both on
> > server and client.
> > PSS signing will get negotiated but we will not error out early as TLS
> > 1.3 is not in  use.
> >
> > That's why I say that this extension of management-external-key is not
> worth it.
> >
> > Am I missing something?
> >
>
> tls_version_max will still report TLS 1.3 as it not affected by the
> version set in config but really the max the library is capable of
> irrespectable tls min/max version.
>

Aha, I missed that. Still I really do not understand the need for erroring
here
instead of when prompting for PK_SIGN based on client version.
Much simpler.

Selva

Re: [Openvpn-devel] [PATCH v5 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key

[Arne S. <ar...@rf...>]

>> Unless I overlooked something, I don't see any situation in which we ask
>> for an unsupported signature.
> 
> Consider this:
> (i) config has --management-external-key nopadding but client announces version
> 2. We will not error out but send the signature request as
> PK_SIGN <base64data>
> without the ALG as client version is not 3 and fail

We can error out on version on the management line. But this is also a
kind of obscure misconfiguration since why would add nopadding to the
config if you cannot support it?

> (ii) tls version max is set 1.2 and openssl 1.1.1 is in use both on
> server and client.
> PSS signing will get negotiated but we will not error out early as TLS
> 1.3 is not in  use.
> 
> That's why I say that this extension of management-external-key is not worth it.
> 
> Am I missing something?
> 

tls_version_max will still report TLS 1.3 as it not affected by the
version set in config but really the max the library is capable of
irrespectable tls min/max version.

Arne

Re: [Openvpn-devel] [PATCH v5 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key

[Selva N. <sel...@gm...>]

Hi,

A thought: why not split this patch into two:

(i) extend PK_SIGN to optionally signal ALG (signalled only if
client_version > 2). Include all the changes to rsa_priv_enc()
etc to to handle PSS sign requests from OpenSSL 1.1.1.

If client version is <= 2 continue to use PK_SIGN as before
provided the signature required is PKCS1 for RSA or ECDSA.
Else error out manage.c.

This ensures backward compat to the extent possible.

(ii) Amend management-external-key to take an additional option
and do whatever one can do with it for an early error report.

Anyway, my suggestion is not even bother with (ii) but this way we
can quickly get (i) finalized.

Unless you already decided to drop (ii) :)

Only downside (or upside depending on your pov) to this is
once (i) is merged in we will start including ALG in PK_SIGN
for new clients (version 3+) so if merging (ii), that should happen
before a subsequent release.

Selva

[Openvpn-devel] Fwd: [PATCH v5 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key

[Selva N. <sel...@gm...>]

Somehow this didn't get copied to the list

---------- Forwarded message ---------
From: Selva Nair <sel...@gm...>
Date: Wed, Nov 14, 2018 at 11:06 AM
Subject: Re: [Openvpn-devel] [PATCH v5 2/2] Add support for OpenSSL TLS 1.3
when using management-external-key
To: Arne Schwabe <ar...@rf...>


Hi,

On Wed, Nov 14, 2018 at 10:56 AM Arne Schwabe <ar...@rf...> wrote:
>
> Am 14.11.18 um 16:18 schrieb Selva Nair:
> > Hi,
> >
> > snipping a lot of useful stuff as we tend agree on those..
> > ..
> >
> >> But in summary.
> >>
> >> My patch basically amounts to: Support old style
management-external-key
> >> with <= OpenSSL 1.1.0 and use and require a new API if we have OpenSSL
> >> 1.1.1 and to really move and have external key have a sane API, we
would
> >> drop rsa-sig and support for older clients, even with <= OpenSSL 1.1.0
> >> as you oppose my proposl for signalling that a new API should be used.
> >> (version in managemnet protocol is too late and not feasible in my
opinion)
> >> Supporting a half working RSA_SIG option that breaks left and right is
> >> not desirable in my opinion.
> >
> > My point is that even with "--management-external-key foo bar", we
> > will be sending
> > a non workable signature request to old clients so changing that
> > option is not really
> > useful. In other words the early erroring out works only half the time
> > and all that code
> > complication is not worth it.
>
> Okay, I think you misunderstood my code. What it does is basically if we
> detect having TLS 1.3 support (max_tls_version >= TLS 1.3) and not a
> nopadding argument we error out, saying that this combination cannot work.
>
> Yes that means that if you upgrade to OpenSSL 1.1.1 you need to support
> the new API but I think that is acceptable.
>
> Unless I overlooked something, I don't see any situation in which we ask
> for an unsupported signature.

Consider this:
(i) config has --management-external-key nopadding but client announces
version
2. We will not error out but send the signature request as
PK_SIGN <base64data>
without the ALG as client version is not 3 and fail

(ii) tls version max is set 1.2 and openssl 1.1.1 is in use both on
server and client.
PSS signing will get negotiated but we will not error out early as TLS
1.3 is not in  use.

That's why I say that this extension of management-external-key is not
worth it.

Am I missing something?

Selva

Re: [Openvpn-devel] [PATCH v5 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key

[Arne S. <ar...@rf...>]

>> For TLS 1.0 to 1.2 and OpenSSL 1.1.0 calls us and requires a PKCS1
>> padded response. As TLS 1.3 mandates RSA-PSS padding support and also
>> requires an TLS 1.3 implementation to support RSA-PSS for older TLS
>> version, OpenSSL will query us to sign an already RSA-PSS padded
>> string.
>>
>> This patch adds an 'unpadded' and 'pkcs1' parameter to the
>>
>> management-external-key option to signal that the client is
>> able to support pkcs1 as well as unpadded signature requests.
> 
> 
> "unpadded" in past tense is confusing -- I suggest "nopadding" or "raw" or
> "rsa-raw" to indicate that what it expects to do is a raw signature
> (or no padding will be added) which means the caller should do the padding.
> In  such cases the input will be invariably of the same size as the modulus
> and there is no room for padding anyway.
> 
> (NB: in the code proper, you do use "nopadding" and not "unpadded"
> which is better -- but also see below)

Note, will use nopadding consistently in the next patch.


> That said, I still fail to warm up to the idea of adding a parameter to
> management-external-key. First, even with this precaution, signing can
> fail in two cases:
> 
> (i) the config file has "--management-external-key nopadding" but the
> client version is <= 2. In such cases no warning will be printed but
> management will get prepadded data with no ALG indicator which the
> external signer will definitely fail to treat as PKCS1. Either signing will fail
> (not sure how an existing  client would handle that) or connection
> will restart with verification error.

I can add an extra warning to print if the client will not announce a
new version. But I think that config will not really happen (the tightly
coupled argument)


> 
> So, in short,  I suggest to just leave the management-external-key option alone
> and only add a new parameter to PK_SIGN (more on that below) and make sure
> the external sign dispatcher does not error out when padding is not PKCS1
> (ie. support at least RSA_PADDING_NONE and RSA_PADDING_PKCS1  in
> rsa_priv_enc() for now)
> 
> The early erroring out is only marginally useful as it does not catch
> all cases as argued above. Also a user may try to deal with the error by setting
> tls version max to 1.2 but that will also fail when openssl 1.1.1 is use -- this
> time without triggering the early error-out. So we gain little by
> mutilating this
> option and all the complications it brings.
> 
> The patch will be considerably simple once this change is taken out.

I get what you saying here but that breaks compability with management
clients that are on the old pkcs1/mangement interface.

> In any case, both here (if we retain this) and in PK_SIGN a more generic
> signalling is required. Else we wont be able to support external PSS
> padding later -- just padding = PSS is not enough to do that, the hash
> algorithm is
> also required. The same is true for PKCS1 too but currently we do not support
> external signer to add digest info for PKCS1 signatures. But going forward,
> some external signers would need that option.
> 
> I would suggest: PKS_SIGN <base64data>,ALG
> where
> ALG = RSA_NONE_PKCS1 (current default),
>           RSA_RAW (or RSA_NONE_NOPADDING)
>          RSA_<hash-alg>_PKCS1
>          ECDSA (current default if key is EC)
> with ALG missing to be interpreted as current defaults (PKCS1
> with digest info already prepended for RSA and ECDSA for EC).
> 

My patch adds exactly that but with slightly different names for the
algorithms.


>>
>> Using the management api client version instead might seem like the
>> more logical way but since we only now that version very late,
>> it would extra logic and complexity to deal with this asynchronous
>> behaviour. Instead just give an error early if OpenSSL 1.1.1 and
>> management-external-key without nopadding is detected.
>>
>> The interface is prepared for signalling PCKS1 and RSA-PSS support
>> instead of signalling unpadded support.
> 
> "instead of" --> "in addition to" ?
> unpadded support --> "signature algorithm to use"
> 
>>
>>
>> Patch v3: fix overlong lines and few other style patches. Note
>>       two overlong lines concerning mbedtls are not fixed as they
>>       are removed/shortend by the mbed tls patch to avoid conflicts
>>
>> Patch v4: Setting minimum TLS version proved to be not enough and
>>       instead of implementing a whole compability layer we require
>>       mangement-clients to implement the new feature when they want
>>       to use OpenSSL 1.1.1
>>
>>       Add a padding=ALGORITHM argument to pk-sig to indicate the
>>       algorithm. Drop adding PKCS1 ourselves.
> 
> The code below adds ",ALG" to PK_SIGN, not padding=ALG to pk-sig.
> Better make the above consistent with that.

Yeah, the commit message is wrong. Noted.


>>  .
>> @@ -833,6 +834,12 @@ END
>>  Base 64 encoded output of RSA_private_encrypt for RSA or ECDSA_sign()
>>  for EC using OpenSSL or mbedtls_pk_sign() using mbed TLS will provide a
>>  correct signature.
>> +The rsa-sig interfaces expects PKCS1 padded signatures for RSA keys
>> +(RSA_PKCS1_PADDING). EC signatures are always unpadded.
>>
>> +
>> +The padding field is only present when pk-sig is used and
>> +currently the following values can be requested PCKS1 and NOPADDING for RSA
>> +certificates and NOPADDING for EC certificates.
> 
> This is totally confusing. There is no reason to distinguish between
> rsa-sig and pk-sig as used by the client although we should encourage all new
> clients to use pk-sig only. These added lines may be replaced by:

That comes from the inflexibility of our management code. I only accepts
pk-sig when quering PKSIG and only accepts rsa-sig when querying RSSIG.


> "For RSA keys, if signature algorithm is "RSA_NONE_PKCS1" or
> unspecified, the data to sign contains the hash with digest info
> header added. If
> the algorithm is RSA_RAW, a raw RSA sign operation should be performed. Base 64
> encoded output of OpenSSL function RSA_private_encrypt() with
> padding=RSA_PADDING_PKCS1 for the first case and
> padding=RSA_PADDING_NONE in the second case creates the correct signature.
> In case of EC keys Base 64 encoded output of ECDSA_sign() provides the correct
> signature."
> 
> And remove those references to ECDSA_sign() and RSA_private_encrypt() earlier.

>>  This capability is intended to allow the use of arbitrary cryptographic
>>  service providers with OpenVPN via the management interface.
>> @@ -840,6 +847,10 @@ service providers with OpenVPN via the management interface.
>>  New and updated clients are expected to use the version command to announce
>>  a version > 1 and handle '>PK_SIGN' prompt and respond with 'pk-sig'.
>>
>> +The older rsa-sig and pk-sig interfaces hav no capability to indidicate the
>> +requested padding algorithm. When the 'nopadding' using version >= 2 is required.
>> +To support TLS 1.3 with OpenSSL 1.1.1 supporting unpadded signatures is required.
>> +
> 
> 
> Again, confusing. If you want to reiterate this what about:
> "The signature algorithm is indicated in the PK_SIGN request only if the
> or management client-version is > 2.  In particular, to support TLS1.3 and
> TLS1.2 using OpenSSL 1.1.1, non-pkcs1 padded signature support is required
> and this can be indicated in the signing request only if the client
> version is > 2."

Yes, your wording is better.

>>  char *
>>  /* returns allocated base64 signature */
>> -management_query_pk_sig(struct management *man,
>> -                         const char *b64_data)
>> +management_query_pk_sig(struct management *man, const char *b64_data,
>> +                        const char *padding)
> 
> 
> Suggest to change padding -> sign_alg

Okay.

>>
>>  {
>>      const char *prompt = "PK_SIGN";
>>      const char *desc = "pk-sign";
>> +    struct buffer buf_data = alloc_buf(strlen(b64_data) + strlen(padding) + 20);
>> +
>>      if (man->connection.client_version <= 1)
>>      {
>>          prompt = "RSA_SIGN";
>>          desc = "rsa-sign";
>>      }
>> -    return management_query_multiline_flatten(man, b64_data, prompt, desc,
>> +
>> +    buf_write(&buf_data, b64_data, (int) strlen(b64_data));
>> +    if (man->connection.client_version > 2)
>>
>> +    {
>> +        buf_write(&buf_data, ",", (int) strlen(","));
>> +        buf_write(&buf_data, padding, (int) strlen(padding));
>> +    }
>> +    char* ret = management_query_multiline_flatten(man,
>> +            (char *)buf_bptr(&buf_data), prompt, desc,
>>              &man->connection.ext_key_state, &man->connection.ext_key_input);
>> +    free_buf(&buf_data);
>> +    return ret;
>>  }
>>
>>  char *
>> diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h
>> index d24abe09..4bfcfdbe 100644
>> --- a/src/openvpn/manage.h
>> +++ b/src/openvpn/manage.h
>> @@ -31,7 +31,7 @@
>>  #include "socket.h"
>>  #include "mroute.h"
>>
>> -#define MANAGEMENT_VERSION                      2
>> +#define MANAGEMENT_VERSION                      3
>>  #define MANAGEMENT_N_PASSWORD_RETRIES           3
>>  #define MANAGEMENT_LOG_HISTORY_INITIAL_SIZE   100
>>  #define MANAGEMENT_ECHO_BUFFER_SIZE           100
>> @@ -341,12 +341,18 @@ struct management *management_init(void);
>>  #ifdef MANAGEMENT_PF
>>  #define MF_CLIENT_PF         (1<<7)
>>  #endif
>> -#define MF_UNIX_SOCK       (1<<8)
>> -#define MF_EXTERNAL_KEY    (1<<9)
>> -#define MF_UP_DOWN          (1<<10)
>> -#define MF_QUERY_REMOTE     (1<<11)
>> -#define MF_QUERY_PROXY      (1<<12)
>> -#define MF_EXTERNAL_CERT    (1<<13)
>> +#define MF_UNIX_SOCK                (1<<8)
>> +#define MF_EXTERNAL_KEY             (1<<9)
>> +#define MF_EXTERNAL_KEY_NOPADDING   (1<<10)
>> +#define MF_EXTERNAL_KEY_PKCS1PAD    (1<<11)
>> +#define MF_UP_DOWN                  (1<<12)
>> +#define MF_QUERY_REMOTE             (1<<13)
>> +#define MF_QUERY_PROXY              (1<<14)
>> +#define MF_EXTERNAL_CERT            (1<<15)
>> +
>> +#define MF_RSA_PKCS1_PADDING       1
>> +#define MF_RSA_NO_PADDING          2
>> +
> 
> 
> These extra defines and associated code  can go if --management-external-key
> is not changed.
> 
>>
>>
>>  bool management_open(struct management *man,
>>                       const char *addr,
>> @@ -430,7 +436,8 @@ void management_learn_addr(struct management *management,
>>
>>  #endif
>>
>> -char *management_query_pk_sig(struct management *man, const char *b64_data);
>> +char *management_query_pk_sig(struct management *man, const char *b64_data,
>> +                              const char* pading);
> 
> 
> pading -> padding for consistency..
> But padding -> sign_alg is my preference.
> 
>>
>>
>>  char *management_query_cert(struct management *man, const char *cert_name);
>>
>> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
>> index 406669a3..2291dd9d 100644
>> --- a/src/openvpn/options.c
>> +++ b/src/openvpn/options.c
>> @@ -2141,6 +2141,23 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
>>
>>  #endif
>>
>> +#if defined(ENABLE_CRYPTOAPI)
>> +    if (o->cryptoapi_cert && (tls_version_max() >= TLS_VER_1_3))
>> +    {
>> +        msg(M_ERR, "Cryptoapi support currently is incompatible "
>> +                   "with OpenSSL 1.1.1/TLS 1.3");
>> +    }
> 
> 
> TLS1.2 is also affected but for now ok like this.
> I have a fix to support PSS in cryptoapi, just have to find some time to test it
> thoroughly. Sigh..

I added these few lines as they quick to remove and point an error in
configuration at the moment. And what tls_version() does NOT report the
version that is currently enabled but the maximum TLS version that the
SSL library supports. So it will always error out on OpenSSL 1.1.1 and
crypto api use.

> 
>> +#endif
>> +#if  defined(ENABLE_MANAGEMENT)
>> +    if ((tls_version_max() >= TLS_VER_1_3) &&
>>
>> +        (options->management_flags & MF_EXTERNAL_KEY) &&
>> +        !(options->management_flags & (MF_EXTERNAL_KEY_NOPADDING))
>>
>> +        )
>> +    {
>> +        msg(M_ERR, "management-external-key with OpenSSL 1.1.1 requires "
>> +                   "the nopadding argument/support");
>> +    }
> 
> 
> Same as above but I think this check is not that useful..

The check can be translated as "If you use management-external-key with
OpenSSL 1.1.1 you need to support the new API"


I removed the rest of comment as I agree with them.

But in summary.

My patch basically amounts to: Support old style management-external-key
with <= OpenSSL 1.1.0 and use and require a new API if we have OpenSSL
1.1.1 and to really move and have external key have a sane API, we would
drop rsa-sig and support for older clients, even with <= OpenSSL 1.1.0
as you oppose my proposl for signalling that a new API should be used.
(version in managemnet protocol is too late and not feasible in my opinion)
Supporting a half working RSA_SIG option that breaks left and right is
not desirable in my opinion.

I would be really happy to drop the old support if no one objects.

Arne

[Openvpn-devel] Summary of the community meeting (Wed, 14th Nov 2018)

[Samuli S. <sa...@op...>]

Hi,

Here's the summary of the IRC meeting.

---

COMMUNITY MEETING

Place: #openvpn-meeting on irc.freenode.net
Date: Wednesday 14th November 2018
Time: 11:30 CET (10:30 UTC)

Planned meeting topics for this meeting were here:

<https://community.openvpn.net/openvpn/wiki/Topics-2018-11-14>

The next meeting has not been scheduled yet.

Your local meeting time is easy to check from services such as

<http://www.timeanddate.com/worldclock>

SUMMARY

cron2, dazo, mattock, ordex, plaisthos, rozmansi and syzzer participated
in this meeting.

--

Discussed tls-crypt-v2 patches. Noted that two of the are not merged yet:

<https://patchwork.openvpn.net/patch/582/>
<https://patchwork.openvpn.net/patch/583/>

--

Discussed networking API patches. The same fucntionality is implemented
in openvpn3-linux codebase and that has been working very well. One IPv6
related bug has been fixed and a patch is about to be sent for OpenVPN 2.

--

Discussed rozmansi's MSI patches. Noted that Jon has given ACKs with
different wording ("Looks good to me") to many of them so they can be
merged. There is also an openvpn-build PR pending which mattock is
having a look:

https://github.com/OpenVPN/openvpn-build/pull/141

--

Talked about tap-windows6 HLK testing. Sgstair has made good progress on
that front and has fixed a number of issues. So besides getting WHQL
certification we also get a better driver and OpenVPN in the process.

--

Discussed the HackerOne report about OpenVPN on Windows having (generic)
DLL hijacking vulnerabilities. Agreed to migrated the report from
HackerOne to Trac:

https://community.openvpn.net/openvpn/ticket/1141

Also agreed that fixing this in a meaningful way would be very tricky.

--

Briefly discussed the integration of openvpn-build and rozmansi's
WiX-based MSI packaging. Agreed that mounting the openvpn-build
directory (of the Linux builder) as a Samba share on the WiX builder
(Windows host) is adequate.

--

Full chatlog attached.

-- 
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

Re: [Openvpn-devel] [PATCH] tls-crypt-v2: fix client reconnect bug

[Antonio Q. <a...@un...>]

Hi,

On 31/10/2018 23:07, Steffan Karger wrote:
> As reported by tincantech on the openvpn-devel IRC channel, a tls-crypt-v2
> client could be caused to trigger an assert in tls_crypt_wrap() because the
> client key might not be correctly initialized after a reconnect attempt.
> 
> This was caused by code that was written before the connection-block
> tls-auth/tls-crypt logic was integrated (57d6f103), rebased on that change,
> but not sufficiently changed to be compatible with the new logic.
> 
> This commit fixes that bug.
> 
> Note that I also moved the violating hunk of code to the same function
> where the tls-auth and tls-crypt (v1) keys are initialized. Once moved
> there, it is immediately clear that v2 didn't follow the same (new) logic.
> 
> Signed-off-by: Steffan Karger <ste...@fo...>

Yeah, it probably was a "conflict" that went unnoticed.

We now need to rely on the data stored in the Connection Entry (ce
member of the options structure) as the tls-crypt* logic is "per
connection block" and not global anymore.

I performed some basic testing and all seems good.
Thanks for fixing this!


Acked-by: Antonio Quartulli <an...@op...>


-- 
Antonio Quartulli

Re: [Openvpn-devel] [PATCH] tls-crypt-v2: clarify --tls-crypt-v2-genkey man page section

[Antonio Q. <a...@un...>]

Hi,

On 31/10/2018 20:22, Steffan Karger wrote:
> As kitsune1 mentioned in IRC, this section should explain that
> "--tls-crypt-v2-genkey client" requires the user to supply the server
> key using "--tls-crypt-v2".
> 
> Signed-off-by: Steffan Karger <ste...@fo...>

Makes sense and, after listening to some people getting confused, it is
good to clarify the procedure.


Acked-by: Antonio Quartulli <an...@op...>

> ---
>  doc/openvpn.8 | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/doc/openvpn.8 b/doc/openvpn.8
> index 94b5cc4..f38fba9 100644
> --- a/doc/openvpn.8
> +++ b/doc/openvpn.8
> @@ -5314,6 +5314,11 @@ If no metadata is supplied, OpenVPN will use a 64\-bit unix timestamp
>  representing the current time in UTC, encoded in network order, as metadata for
>  the generated key.
>  
> +A tls\-crypt\-v2 client key is wrapped using a server key.  To generate a
> +client key, the user must therefore supply the server key using the
> +.B \-\-tls\-crypt\-v2
> +option.
> +
>  Servers can use
>  .B \-\-tls\-crypt\-v2\-verify
>  to specify a metadata verification command.
> 

-- 
Antonio Quartulli

Re: [Openvpn-devel] Community meeting tomorrow

[Samuli S. <sa...@op...>]

Il 13/11/18 22:36, sa...@op... ha scritto:
> 
> As planned last week we'll have a community meeting tomorrow at the usual time. I'll setup the topic page tomorrow morning unless somebody does it first. One of the main topics is undoubtedly the  openvpn 2.5 release.
>  
> Samuli
> _______________________________________________
> Openvpn-devel mailing list
> Ope...@li...
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
> 

Topic list is finally up:

https://community.openvpn.net/openvpn/wiki/Topics-2018-11-14

The meeting is in 1 hour 25 minutes (10:30 UTC, 11:30 CET) on
#openvpn-meeting channel on Freenode.

-- 
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

Re: [Openvpn-devel] [PATCH 4/5] Add MSI custom action for reliable Windows 10 detection

[Jon K. <jk...@mi...>]

> I gave the MsiBreak method a quick try, but I couldn't make it work. I have
> set MsiBreak as a system environment variable, restarted Windows Installer
> service, restarted the shell window from where I invoke msiexec calls to
> make sure the environment is updated. But it doesn't pop-up any prompt to
> attach debugger as advertised.

That's sad to hear. I'd toss out ideas to try, but...

> I haven't installed the Debugging Tools for Windows, as I run Visual Studio
> elevated to debug processes. I haven't restarted my computer: restart of a
> working machine with stage set to debug something is a royal PITA: the
> libopenvpnmsica.dll has more than one custom property to debug, restarting
> each time to switch MsiBreak to a different custom action is not viable.

I've debugged using MsiBreak set in the admin CMD prompt I used to launch the 
MSI, so restarts aren't required. The article goes into some very nearly useful 
details about when this is, but...

> Therefore, I'd prefer to keep own MessageBox() call in the beginning of each
> custom action. It works 100%.

...not only that, but the article even suggests this:

"To start debugging without MsiBreak, put a temporary message box at the 
beginning of the action's code. When the message box appears during the 
installation, attach the debugger to the process owning the message box."

If you ever need to debug say, the DLL loading, then MsiBreak is the way to go. 
As it stands, it's *REALLY* hard to beat 100%. :)

> Adding PID to the message is an option. I personally never needed it. When
> running Visual Studio elevated, you press Ctrl+Alt+P, followed by "msi"
> keystrokes to focus on "msiexec.exe" processes, then observe the
> MessageBox()'s title (which is deliberately set to function name) in the
> "Title" column of the available process list. Way faster than searching
> process by PID.

I've been wrong before about how easy different options are. :)

> Anyway, I have extended the debug pop-up dialogs to be more informative and
> include PID. Patch follows...

I took a look and it looks good to me, though I agree it's not strictly necessary. 

Thanks,
Jon

Re: [Openvpn-devel] [PATCH 4/5] Add MSI custom action for reliable Windows 10 detection

[Jon K. <jk...@mi...>]

> Note a side take-home message: msiexec.exe will never be Windows 10 aware.

Unfortunately, yes.

> > Let me know if that doesn't make sense or won't work for what you're
> > doing.

> I am familiar with EXE and manifests, thanks. Nevertheless, I really
> appreciate your time to extensively research and provide useful references
> in the debate.

It looks like I erred widely on the side of overinforming. I appreciate your 
patience with that. 

> In my professional career I make EXE (or WSH) "parent installers" only
> because end-users usually have no clue what platform is their Windows. You
> can't do one-MSI-fits-all-platforms for native apps, so the parent installer
> EXE takes platform choice away from users. All other decision logic is
> inside MSI packages making them fully autonomous - once you pick the right
> one for your platform of course.

Makes sense. Fortunately x86 EXEs run on ARM64, so this even works 
there. :)

> When the MSI package is installed by Group Policy Client, the MSI is
> launched directly by msiexec.exe. 

Ah, I see. That puts a bit of a damper in bolting intelligence on during the 
msiexec invocation, doesn't it?

I'm no expert with GP, and what little I know suggests that any methods for
deploying custom EXEs through it are either much more complex than is 
feasible for you to support or otherwise not worth using. You're the expert 
between the two of us.

Without this ability and with the compat shimming of the MSI infrastructure,
there's not much else to be done. 

> Hence such a complex workaround to detect the MSI has been launched on
> Windows 10. Microsoft made me do it. ;)

:)

> Please, note one last thing... I totally agree with everybody saying "you
> That's why this custom action *does not* return the Windows version - not to 
> tempt anybody else to use it for OS version detection. 

> The name "FindSystemInfo" was picked generic on
> purpose, to allow us to add other detections missing in MSI should we need
> any down the road.

Smart! I hadn't followed the function to its callers, so I missed this.

Samuli, LGTM.

Thanks,
Jon

Re: [Openvpn-devel] [PATCH v5 2/2] Add support for OpenSSL TLS 1.3 when using management-external-key

[Selva N. <sel...@gm...>]

Hi,

My comments below has grown too long so first a summary for those
who TLDR;

My suggestion:
- Leave management-external-key as is (there is not much gained by
adding a parameter to it)
- Append a fairly flexible signature algorithm specifier to PK_SIGN
request to management
 (nopadding or pkcs1 is not good enough in the long run)
 Do not take cue from OpenSSL's design of RSA_private_encrypt() and rsa_sign()
 both of which fall short for historic reasons.
- require version 3 clients to support at least certain algorithms

Some of the comments below were written assuming --management-external-key
will get a new optional parameter and some are based on my suggestion not to
do so. So the rest is sometimes a bit of flux and/or repetitive. But I
spent some
time on this so here goes:

On Wed, Oct 31, 2018 at 1:00 PM Arne Schwabe <ar...@rf...> wrote:
>
> For TLS 1.0 to 1.2 and OpenSSL 1.1.0 calls us and requires a PKCS1
> padded response. As TLS 1.3 mandates RSA-PSS padding support and also
> requires an TLS 1.3 implementation to support RSA-PSS for older TLS
> version, OpenSSL will query us to sign an already RSA-PSS padded
> string.
>
> This patch adds an 'unpadded' and 'pkcs1' parameter to the
>
> management-external-key option to signal that the client is
> able to support pkcs1 as well as unpadded signature requests.


"unpadded" in past tense is confusing -- I suggest "nopadding" or "raw" or
"rsa-raw" to indicate that what it expects to do is a raw signature
(or no padding will be added) which means the caller should do the padding.
In  such cases the input will be invariably of the same size as the modulus
and there is no room for padding anyway.

(NB: in the code proper, you do use "nopadding" and not "unpadded"
which is better -- but also see below)

That said, I still fail to warm up to the idea of adding a parameter to
management-external-key. First, even with this precaution, signing can
fail in two cases:

(i) the config file has "--management-external-key nopadding" but the
client version is <= 2. In such cases no warning will be printed but
management will get prepadded data with no ALG indicator which the
external signer will definitely fail to treat as PKCS1. Either signing will fail
(not sure how an existing  client would handle that) or connection
will restart with verification error.

(ii) Taking a long view, the real problem is not just "padding" but
"signature algorithm" in general. That is, PK_SIGN (or RSA_SIGN)
does not provide enough details for generating a signature. In good
old days when we supported only TLS 1.1 or older and RSA keys, there
was only one kind of signature. Today one needs to know the hash
algorithm and the padding scheme to generate a signature. We get
away without the hash algorithm as the digestinfo header is
prepended to the hash by the time rsa_priv_enc is called (not so in
mbedTLS so we add it in that handler). But that will not work for
PSS if we were to support external PSS padding -- which we should
do in the long term.

Part of the reason for this is we have been playing along with OpenSSL
which has been trying hard to preserve a dated API. Now with 1.1.1,
they have broken it for good and the rsa_sign() callback does not even
get called anymore as padding cannot be assumend to be PKCS1.

So we have to stop taking cue from OpenSSL especially since the purpose
of external key is to support arbitrary external signing agents/libraries.

So, in short,  I suggest to just leave the management-external-key option alone
and only add a new parameter to PK_SIGN (more on that below) and make sure
the external sign dispatcher does not error out when padding is not PKCS1
(ie. support at least RSA_PADDING_NONE and RSA_PADDING_PKCS1  in
rsa_priv_enc() for now)

The early erroring out is only marginally useful as it does not catch
all cases as argued above. Also a user may try to deal with the error by setting
tls version max to 1.2 but that will also fail when openssl 1.1.1 is use -- this
time without triggering the early error-out. So we gain little by
mutilating this
option and all the complications it brings.

The patch will be considerably simple once this change is taken out.

In any case, both here (if we retain this) and in PK_SIGN a more generic
signalling is required. Else we wont be able to support external PSS
padding later -- just padding = PSS is not enough to do that, the hash
algorithm is
also required. The same is true for PKCS1 too but currently we do not support
external signer to add digest info for PKCS1 signatures. But going forward,
some external signers would need that option.

I would suggest: PKS_SIGN <base64data>,ALG

where
ALG = RSA_NONE_PKCS1 (current default),
          RSA_RAW (or RSA_NONE_NOPADDING)
         RSA_<hash-alg>_PKCS1
         ECDSA (current default if key is EC)
with ALG missing to be interpreted as current defaults (PKCS1
with digest info already prepended for RSA and ECDSA for EC).

Here <hash_lag> could be SHA256, SHA1 etc. and indicates
that the data is just the hash and the signer should add digestinfo
header to the signature. We do not have to support this
as yet but will come handy to have a way to do a complete signature
algorithm specifier in ALG.

And require that version 3 clients are expected to support at least
RSA_NONE_PKCS1, RSA_RAW and ECDSA.

Anyway, I did review the code considering both options -- not all
comments below are relevant if we decide not to modify
--management-external-key. That would be my preference.

>
> Since clients that implement the management-external-key interface
> are usually rather tightly integrated solutions (OpenVPN Connect in the
> past, OpenVPN for Android), it is reasonable to expect that
> upgrading the OpenSSL library can be done together with
> management interface changes. Therefore we provide no backwards
> compatbility for mangement-interface clients not supporting
> OpenSSL 1.1.1.


Looks perfectly acceptable to me..

>
>
> Using the management api client version instead might seem like the
> more logical way but since we only now that version very late,
> it would extra logic and complexity to deal with this asynchronous
> behaviour. Instead just give an error early if OpenSSL 1.1.1 and
> management-external-key without nopadding is detected.
>
> The interface is prepared for signalling PCKS1 and RSA-PSS support
> instead of signalling unpadded support.

"instead of" --> "in addition to" ?
unpadded support --> "signature algorithm to use"

>
>
> Patch v3: fix overlong lines and few other style patches. Note
>       two overlong lines concerning mbedtls are not fixed as they
>       are removed/shortend by the mbed tls patch to avoid conflicts
>
> Patch v4: Setting minimum TLS version proved to be not enough and
>       instead of implementing a whole compability layer we require
>       mangement-clients to implement the new feature when they want
>       to use OpenSSL 1.1.1
>
>       Add a padding=ALGORITHM argument to pk-sig to indicate the
>       algorithm. Drop adding PKCS1 ourselves.

The code below adds ",ALG" to PK_SIGN, not padding=ALG to pk-sig.
Better make the above consistent with that.

>
>
> Patch v5: Send the right version of the patch
>
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
>  doc/management-notes.txt  | 13 ++++++++++-
>  doc/openvpn.8             |  7 ++++--
>  src/openvpn/manage.c      | 18 +++++++++++---
>  src/openvpn/manage.h      | 23 +++++++++++-------
>  src/openvpn/options.c     | 45 +++++++++++++++++++++++++++++++++--
>  src/openvpn/ssl_mbedtls.c |  7 ++++--
>  src/openvpn/ssl_openssl.c | 49 +++++++++++++++++++++++++++++++--------
>  7 files changed, 134 insertions(+), 28 deletions(-)
>
> diff --git a/doc/management-notes.txt b/doc/management-notes.txt
> index 17645c1d..f685af28 100644
> --- a/doc/management-notes.txt
> +++ b/doc/management-notes.txt
> @@ -816,6 +816,7 @@ actual private key. When the SSL protocol needs to perform a sign
>  operation, the data to be signed will be sent to the management interface
>  via a notification as follows:
>
> +>PK_SIGN:[BASE64_DATA],[ALG] (if client announces support for management version > 2)
>  >PK_SIGN:[BASE64_DATA] (if client announces support for management version > 1)
>  >RSA_SIGN:[BASE64_DATA] (only older clients will be prompted like this)

List values of ALG that a version 3 client should support. And add that if ALG
is missing it should be interpreted as ECDSA for EC keys and
RSA_NONE_PKCS1 for RSA keys. That nicely covers the two possible
forms of PK_SIGN directive.

>
>
> @@ -823,7 +824,7 @@ The management interface client should then create an appropriate signature of
>  the (decoded) BASE64_DATA using the private key and return the SSL signature as
>  follows:
>
> -pk-sig   (or rsa-sig)
> +pk-sig (or rsa-sig)
>  [BASE64_SIG_LINE]
>  .
>  .
> @@ -833,6 +834,12 @@ END
>  Base 64 encoded output of RSA_private_encrypt for RSA or ECDSA_sign()
>  for EC using OpenSSL or mbedtls_pk_sign() using mbed TLS will provide a
>  correct signature.
> +The rsa-sig interfaces expects PKCS1 padded signatures for RSA keys
> +(RSA_PKCS1_PADDING). EC signatures are always unpadded.
>
> +
> +The padding field is only present when pk-sig is used and
> +currently the following values can be requested PCKS1 and NOPADDING for RSA
> +certificates and NOPADDING for EC certificates.

This is totally confusing. There is no reason to distinguish between
rsa-sig and pk-sig as used by the client although we should encourage all new
clients to use pk-sig only. These added lines may be replaced by:

"For RSA keys, if signature algorithm is "RSA_NONE_PKCS1" or
unspecified, the data to sign contains the hash with digest info
header added. If
the algorithm is RSA_RAW, a raw RSA sign operation should be performed. Base 64
encoded output of OpenSSL function RSA_private_encrypt() with
padding=RSA_PADDING_PKCS1 for the first case and
padding=RSA_PADDING_NONE in the second case creates the correct signature.
In case of EC keys Base 64 encoded output of ECDSA_sign() provides the correct
signature."

And remove those references to ECDSA_sign() and RSA_private_encrypt() earlier.

>
>
>  This capability is intended to allow the use of arbitrary cryptographic
>  service providers with OpenVPN via the management interface.
> @@ -840,6 +847,10 @@ service providers with OpenVPN via the management interface.
>  New and updated clients are expected to use the version command to announce
>  a version > 1 and handle '>PK_SIGN' prompt and respond with 'pk-sig'.
>
> +The older rsa-sig and pk-sig interfaces hav no capability to indidicate the
> +requested padding algorithm. When the 'nopadding' using version >= 2 is required.
> +To support TLS 1.3 with OpenSSL 1.1.1 supporting unpadded signatures is required.
> +


Again, confusing. If you want to reiterate this what about:
"The signature algorithm is indicated in the PK_SIGN request only if the
or management client-version is > 2.  In particular, to support TLS1.3 and
TLS1.2 using OpenSSL 1.1.1, non-pkcs1 padded signature support is required
and this can be indicated in the signing request only if the client
version is > 2."

>  COMMAND -- certificate (OpenVPN 2.4 or higher)
>  ----------------------------------------------
>  Provides support for external storage of the certificate. Requires the
> diff --git a/doc/openvpn.8 b/doc/openvpn.8
> index e80d4696..3cd5d944 100644
> --- a/doc/openvpn.8
> +++ b/doc/openvpn.8
> @@ -2739,10 +2739,13 @@ Allow management interface to override
>  directives (client\-only).
>  .\"*********************************************************
>  .TP
> -.B \-\-management\-external\-key
> +.B \-\-management\-external\-key [nopadding] [pkcs1]
>  Allows usage for external private key file instead of
>  .B \-\-key
> -option (client\-only).
> +option (client\-only). The optional parameters nopadding and
> +pkcs1 signal support for different padding algorithms. See
> +doc/mangement-notes.txt for a complete description of this
> +feature.
>  .\"*********************************************************
>  .TP
>  .B \-\-management\-external\-cert certificate\-hint
> diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
> index 8b633f20..62d4bc7b 100644
> --- a/src/openvpn/manage.c
> +++ b/src/openvpn/manage.c
> @@ -3639,18 +3639,30 @@ management_query_multiline_flatten(struct management *man,
>
>  char *
>  /* returns allocated base64 signature */
> -management_query_pk_sig(struct management *man,
> -                         const char *b64_data)
> +management_query_pk_sig(struct management *man, const char *b64_data,
> +                        const char *padding)


Suggest to change padding -> sign_alg

>
>  {
>      const char *prompt = "PK_SIGN";
>      const char *desc = "pk-sign";
> +    struct buffer buf_data = alloc_buf(strlen(b64_data) + strlen(padding) + 20);
> +
>      if (man->connection.client_version <= 1)
>      {
>          prompt = "RSA_SIGN";
>          desc = "rsa-sign";
>      }
> -    return management_query_multiline_flatten(man, b64_data, prompt, desc,
> +
> +    buf_write(&buf_data, b64_data, (int) strlen(b64_data));
> +    if (man->connection.client_version > 2)
>
> +    {
> +        buf_write(&buf_data, ",", (int) strlen(","));
> +        buf_write(&buf_data, padding, (int) strlen(padding));
> +    }
> +    char* ret = management_query_multiline_flatten(man,
> +            (char *)buf_bptr(&buf_data), prompt, desc,
>              &man->connection.ext_key_state, &man->connection.ext_key_input);
> +    free_buf(&buf_data);
> +    return ret;
>  }
>
>  char *
> diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h
> index d24abe09..4bfcfdbe 100644
> --- a/src/openvpn/manage.h
> +++ b/src/openvpn/manage.h
> @@ -31,7 +31,7 @@
>  #include "socket.h"
>  #include "mroute.h"
>
> -#define MANAGEMENT_VERSION                      2
> +#define MANAGEMENT_VERSION                      3
>  #define MANAGEMENT_N_PASSWORD_RETRIES           3
>  #define MANAGEMENT_LOG_HISTORY_INITIAL_SIZE   100
>  #define MANAGEMENT_ECHO_BUFFER_SIZE           100
> @@ -341,12 +341,18 @@ struct management *management_init(void);
>  #ifdef MANAGEMENT_PF
>  #define MF_CLIENT_PF         (1<<7)
>  #endif
> -#define MF_UNIX_SOCK       (1<<8)
> -#define MF_EXTERNAL_KEY    (1<<9)
> -#define MF_UP_DOWN          (1<<10)
> -#define MF_QUERY_REMOTE     (1<<11)
> -#define MF_QUERY_PROXY      (1<<12)
> -#define MF_EXTERNAL_CERT    (1<<13)
> +#define MF_UNIX_SOCK                (1<<8)
> +#define MF_EXTERNAL_KEY             (1<<9)
> +#define MF_EXTERNAL_KEY_NOPADDING   (1<<10)
> +#define MF_EXTERNAL_KEY_PKCS1PAD    (1<<11)
> +#define MF_UP_DOWN                  (1<<12)
> +#define MF_QUERY_REMOTE             (1<<13)
> +#define MF_QUERY_PROXY              (1<<14)
> +#define MF_EXTERNAL_CERT            (1<<15)
> +
> +#define MF_RSA_PKCS1_PADDING       1
> +#define MF_RSA_NO_PADDING          2
> +


These extra defines and associated code  can go if --management-external-key
is not changed.

>
>
>  bool management_open(struct management *man,
>                       const char *addr,
> @@ -430,7 +436,8 @@ void management_learn_addr(struct management *management,
>
>  #endif
>
> -char *management_query_pk_sig(struct management *man, const char *b64_data);
> +char *management_query_pk_sig(struct management *man, const char *b64_data,
> +                              const char* pading);


pading -> padding for consistency..
But padding -> sign_alg is my preference.

>
>
>  char *management_query_cert(struct management *man, const char *cert_name);
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 406669a3..2291dd9d 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -2141,6 +2141,23 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
>
>  #endif
>
> +#if defined(ENABLE_CRYPTOAPI)
> +    if (o->cryptoapi_cert && (tls_version_max() >= TLS_VER_1_3))
> +    {
> +        msg(M_ERR, "Cryptoapi support currently is incompatible "
> +                   "with OpenSSL 1.1.1/TLS 1.3");
> +    }


TLS1.2 is also affected but for now ok like this.
I have a fix to support PSS in cryptoapi, just have to find some time to test it
thoroughly. Sigh..

> +#endif
> +#if  defined(ENABLE_MANAGEMENT)
> +    if ((tls_version_max() >= TLS_VER_1_3) &&
>
> +        (options->management_flags & MF_EXTERNAL_KEY) &&
> +        !(options->management_flags & (MF_EXTERNAL_KEY_NOPADDING))
>
> +        )
> +    {
> +        msg(M_ERR, "management-external-key with OpenSSL 1.1.1 requires "
> +                   "the nopadding argument/support");
> +    }


Same as above but I think this check is not that useful..

>
> +#endif
>      /*
>       * Windows-specific options.
>       */
> @@ -5151,9 +5168,33 @@ add_option(struct options *options,
>          options->management_write_peer_info_file = p[1];
>      }
>  #ifdef ENABLE_MANAGEMENT
> -    else if (streq(p[0], "management-external-key") && !p[1])
> +    else if (streq(p[0], "management-external-key"))
>      {
>          VERIFY_PERMISSION(OPT_P_GENERAL);
> +        for (int j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
> +        {
> +            if (streq(p[j], "nopadding"))
> +            {
> +                options->management_flags |= MF_EXTERNAL_KEY_NOPADDING;
> +            }
> +            else if (p[1] && streq(p[1], "pkcs1"))


That should be
               else if (streq(p[j], "pkcs1"))
Note the [j]

But the whole chunk could be removed..

>
> +            {
> +                options->management_flags |= MF_EXTERNAL_KEY_PKCS1PAD;
> +            }
> +            else
> +            {
> +                msg(msglevel, "Unknown management-external-key flag: %s" , p[j]);
> +            }
> +        }
> +        /*
> +         * When no option is present, assume that only PKCS1
> +         * padding is supported
> +         */
> +        if (! (options->management_flags &
> +            (MF_EXTERNAL_KEY_NOPADDING | MF_EXTERNAL_KEY_PKCS1PAD)))
> +        {
> +            options->management_flags |= MF_EXTERNAL_KEY_PKCS1PAD;
> +        }


As we will anyway prompt for PKCS1 signatures even if the client does
not announce it, we can always set that flag unconditionally and
assume/require that all clients support PKCS1. Then the above is a simple

options->management_flags |= MF_EXTERNAL_KEY_PKCS1PAD

Again the above is irrelevant if we go with my suggestion to not touch
this option.

>          options->management_flags |= MF_EXTERNAL_KEY;
>      }
>      else if (streq(p[0], "management-external-cert") && p[1] && !p[2])
> @@ -8456,4 +8497,4 @@ add_option(struct options *options,
>      }
>  err:
>      gc_free(&gc);
> -}
> \ No newline at end of file
> +}
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index f7e8c2d0..09b1a8fa 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -626,7 +626,6 @@ tls_ctx_use_external_signing_func(struct tls_root_ctx *ctx,
>  }
>
>  #ifdef ENABLE_MANAGEMENT
> -
>  /** Query the management interface for a signature, see external_sign_func. */
>  static bool
>  management_sign_func(void *sign_ctx, const void *src, size_t src_len,
> @@ -641,7 +640,11 @@ management_sign_func(void *sign_ctx, const void *src, size_t src_len,
>          goto cleanup;
>      }
>
> -    if (!(dst_b64 = management_query_pk_sig(management, src_b64)))
> +    /*
> +     * We only use PKCS1 signatures at the moment in mbed TLS,
> +     * there the signature parameter is hardcoded
> +     */
> +    if (!(dst_b64 = management_query_pk_sig(management, src_b64, "PKCS1")))

PKCS1 --> RSA_NONE_PKCS1  :)

>      {
>          goto cleanup;
>      }
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index c2c8fdc0..237c2b55 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -1079,24 +1079,46 @@ openvpn_extkey_rsa_finish(RSA *rsa)
>      return 1;
>  }
>
> -/* Pass the input hash in 'dgst' to management and get the signature back.
> +/*
> + * Convert OpenSSL's constant to the strings used in the management
> + * interface query
> + */
> +const char*
> +get_sig_padding_name(const int padding)
> +{
> +    switch(padding)
> +    {
> +        case RSA_PKCS1_PADDING:
> +            return "PKCS1";
> +        case RSA_NO_PADDING:
> +            return "NOPADDING";
> +        default:
> +            return "UNKNOWN";
> +    }
> +}

Change the above to sign_alg_name instead of padding_name

> +
> +/*
> + * Pass the input hash in 'dgst' to management and get the signature back.
>   * On input siglen contains the capacity of the buffer 'sig'.
>   * On return signature is in sig.
> + * pkcs1 controls if pkcs1 padding is required


That should be "The parameter padding controls the type of padding to use."

But I suggest sign_alg controls the signature algorithm to use.

The caller need only translate  padding (not hash algorithm) to
signature algorithm
as the hash_alg is currently not available in rsa_priv_enc() callback.
So for now
hash-alg will be always indicated as none. However,  we want the management
interface protocol be more flexible and hence we indicate the
signature algorithm
using more descriptive mnemonics like RSA_NONE_PKCS1 etc in PK_SIGN.

>   * Return value is signature length or -1 on error.
>   */
>  static int
>  get_sig_from_man(const unsigned char *dgst, unsigned int dgstlen,
> -                 unsigned char *sig, unsigned int siglen)
> +                 unsigned char *sig, unsigned int siglen,
> +                 int padding)


I suggest replace that "int padding" by "const char *padding"
and pass in the translated name --- see below for a rationale.

Actually "const char *sign_alg" is my preference.

>
>  {
>      char *in_b64 = NULL;
>      char *out_b64 = NULL;
>      int len = -1;
>
> -    /* convert 'dgst' to base64 */
> -    if (management
> -        && openvpn_base64_encode(dgst, dgstlen, &in_b64) > 0)
> +    int bencret = openvpn_base64_encode(dgst, dgstlen, &in_b64);
> +
> +    if (management && bencret > 0)
>      {
> -        out_b64 = management_query_pk_sig(management, in_b64);
> +        out_b64 = management_query_pk_sig(management, in_b64,
> +                                          get_sig_padding_name(padding));


Then that would be just padding or sign_alg

>
>      }
>      if (out_b64)
>      {
> @@ -1110,18 +1132,19 @@ get_sig_from_man(const unsigned char *dgst, unsigned int dgstlen,
>
>  /* sign arbitrary data */
>  static int
> -rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, int padding)
> +rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa,
> +             int padding)
>  {
>      unsigned int len = RSA_size(rsa);
>      int ret = -1;
>
> -    if (padding != RSA_PKCS1_PADDING)
> +    if (padding != RSA_PKCS1_PADDING && padding != RSA_NO_PADDING)
>      {
>          RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, RSA_R_UNKNOWN_PADDING_TYPE);
>          return -1;
>      }
>
> -    ret = get_sig_from_man(from, flen, to, len);
> +    ret = get_sig_from_man(from, flen, to, len, padding);


And use  "get_sig_padding_name(padding)" or preferably
"get_sign_alg_name(padding)" here.

>
>      return (ret == len)? ret : -1;
>  }
> @@ -1215,7 +1238,13 @@ ecdsa_sign(int type, const unsigned char *dgst, int dgstlen, unsigned char *sig,
>             unsigned int *siglen, const BIGNUM *kinv, const BIGNUM *r, EC_KEY *ec)
>  {
>      int capacity = ECDSA_size(ec);
> -    int len = get_sig_from_man(dgst, dgstlen, sig, capacity);
> +    /*
> +     * ECDSA does not seem to have proper constants for paddings since
> +     * there are only signatures without padding at the moment, reuse
> +     * RSA_NO_PADDING for now as it will trigger querying for "NOPADDING" in the
> +     * management interface
> +     */
>
> +    int len = get_sig_from_man(dgst, dgstlen, sig, capacity, RSA_NO_PADDING);


That comment reads like a hack. As suggested above, passing the padding or
sign_alg name avoids it. And one could instead say:

/* ECDSA signature involves no padding */
int len = get_sig_from_man(dgst, dgstlen, sig, capacity, "ECDSA");

Just as "PKCS1"  (or "RSA_NONE_PKCS1") is hard coded in the request
when using mbedTLS.

>
>
>      if (len > 0)
>      {
> --
> 2.19.1


Selva