[Openvpn-devel] [PATCH v2 4/9] client-connect: Move multi_client_connect_setenv into early_setup

[Fabian Knittel]

This patch moves multi_client_connect_setenv into
multi_client_connect_early_setup and makes sure that every client-connect
handling function updates the virtual address selection.

Background: This unifies how the client-connect handling functions work.

Signed-off-by: Fabian Knittel <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 8cc61d7..e9fbdc6 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1646,15 +1646,6 @@ multi_connection_established (struct multi_context *m, 
struct multi_instance *mi

   multi_client_connect_source_ccd (m, mi, _types_found);

-  /*
-   * 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, _types_found,
   _succeeded, _succeeded_count);

@@ -1711,6 +1702,10 @@ multi_client_connect_early_setup (struct multi_context 
*m,

   /* reset pool handle to null */
   mi->vaddr_handle = -1;
+
+  /* do --client-connect setenvs */
+  multi_select_virtual_addr (m, mi);
+  multi_client_connect_setenv (m, mi);
 }

 /*
@@ -1753,6 +1748,12 @@ multi_client_connect_source_ccd (struct multi_context *m,
 option_types_found,
 mi->context.c2.es);

+ /*
+  * Select a virtual address from either --ifconfig-push in
+  * --client-config-dir file or --ifconfig-pool.
+  */
+ multi_select_virtual_addr (m, mi);
+ multi_set_virtual_addr_env (m, mi);
}

   gc_free ();
-- 
2.1.4

[Openvpn-devel] [PATCH v2 7/9] client-connect: Add CC_RET_DEFERRED and cope with deferred client-connect

[Fabian Knittel]

This patch moves the state, that was previously tracked within the
multi_connection_established() function, into struct client_connect_state.  The
multi_connection_established() function can now be exited and re-entered as
many times as necessary - without losing the client-connect handling state.

The patch also adds the new return value CC_RET_DEFERRED which indicates that
the handler couldn't complete immediately, and needs to be called later.  At
that point multi_connection_established() will exit without indicating
completion.

Each client-connect handler now has an (optional) additional call-back:  The
call-back for handling the deferred case.  If the main call-back returns
CC_RET_DEFERRED, the next call to the handler will be through the deferred
call-back.

Signed-off-by: Fabian Knittel <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 255 ++--
 src/openvpn/multi.h |  18 
 2 files changed, 185 insertions(+), 88 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 648b026..7079946 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -48,6 +48,9 @@

 /*#define MULTI_DEBUG_EVENT_LOOP*/

+static void delete_client_connect_state (struct multi_instance *mi);
+
+
 #ifdef MULTI_DEBUG_EVENT_LOOP
 static const char *
 id (struct multi_instance *mi)
@@ -584,6 +587,8 @@ multi_close_instance (struct multi_context *m,
   set_cc_config (mi, NULL);
 #endif

+  delete_client_connect_state (mi);
+
   multi_client_disconnect_script (m, mi);

   if (mi->did_open_context)
@@ -1594,94 +1599,6 @@ multi_client_connect_setenv (struct multi_context *m,

 static void
 multi_client_connect_early_setup (struct multi_context *m,
- struct multi_instance *mi);
-static void
-multi_client_connect_source_ccd (struct multi_context *m,
-struct multi_instance *mi,
-unsigned int *option_types_found);
-static void
-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);
-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);
-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);
-static void
-multi_client_connect_late_setup (struct multi_context *m,
-struct multi_instance *mi,
-const unsigned int option_types_found,
-int cc_succeeded,
-const int cc_succeeded_count);
-
-/*
- * 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)
-{
-  typedef enum client_connect_return_t 
(*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
-  };
-
-  int cur_handler = 0;
-  unsigned int option_types_found = 0;
-  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);
-
-  while (succeeded && handlers[cur_handler])
-   {
- ret = handlers[cur_handler] (m, mi, _types_found);
- if (ret == CC_RET_SUCCEEDED)
-   ++cc_succeeded_count;
- else if (ret == CC_RET_FAILED)
-   succeeded = false;
- ++cur_handler;
-   }
-
-  multi_client_connect_late_setup (m, mi, option_types_found, cc_succeeded,
-  cc_succeeded_count);
-
-  /* set flag so we don't get called again */
-  mi->connection_established_flag = true;
-}

[Openvpn-devel] [PATCH v2 3/9] client-connect: Refactor multi_client_connect_source_ccd

[Fabian Knittel]

Refactor multi_client_connect_source_ccd(), so that options_server_import() (or
the success path in general) is only entered in one place within the function.

Signed-off-by: Fabian Knittel <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 29 +
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 80bd5a3..8cc61d7 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1732,7 +1732,19 @@ multi_client_connect_source_ccd (struct multi_context *m,
   );

   /* try common-name file */
-  if (test_file (ccd_file))
+  if (!test_file (ccd_file))
+   ccd_file = NULL;
+
+  if (!ccd_file)
+   {
+ ccd_file = gen_path (mi->context.options.client_config_dir,
+  CCD_DEFAULT, );
+ /* try default file */
+ if (!test_file (ccd_file))
+   ccd_file = NULL;
+   }
+
+  if (ccd_file)
{
  options_server_import (>context.options,
 ccd_file,
@@ -1740,22 +1752,7 @@ multi_client_connect_source_ccd (struct multi_context *m,
 CLIENT_CONNECT_OPT_MASK,
 option_types_found,
 mi->context.c2.es);
-   }
-  else /* try default file */
-   {
- ccd_file = gen_path (mi->context.options.client_config_dir,
-  CCD_DEFAULT,
-  );

- if (test_file (ccd_file))
-   {
- options_server_import (>context.options,
-ccd_file,
-D_IMPORT_ERRORS|M_OPTERR,
-CLIENT_CONNECT_OPT_MASK,
-option_types_found,
-mi->context.c2.es);
-   }
}

   gc_free ();
-- 
2.1.4

[Openvpn-devel] [PATCH v2 5/9] client-connect: Refactor to use return values instead of modifying a passed-in flag

[Fabian Knittel]

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 <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 122 +++-
 src/openvpn/multi.h |  11 -
 2 files changed, 82 insertions(+), 51 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index e9fbdc6..072a5f6 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1530,13 +1530,12 @@ multi_client_connect_post_plugin (struct multi_context 
*m,
 /*
  * Called to load management-derived client-connect config
  */
-static void
+static enum client_connect_return
 multi_client_connect_mda (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 MANAGEMENT_DEF_AUTH
   if (mi->cc_config)
 {
@@ -1562,9 +1561,10 @@ multi_client_connect_mda (struct multi_context *m,
   multi_select_virtual_addr (m, mi);
   multi_set_virtual_addr_env (m, mi);

-  ++*cc_succeeded_count;
+  ret = CC_RET_SUCCEEDED;
 }
 #endif
+  return ret;
 }

 static void
@@ -1641,38 +1641,59 @@ multi_connection_established (struct multi_context *m, 
struct multi_instance *mi
   unsigned int option_types_found = 0;
   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, _types_found);
+  if (cc_succeeded)
+   {
+ ret = multi_client_connect_source_ccd (m, mi, _types_found);
+ if (ret == CC_RET_SUCCEEDED)
+   ++cc_succeeded_count;
+ else if (ret == CC_RET_FAILED)
+   cc_succeeded = false;
+   }

-  multi_client_connect_call_plugin_v1 (m, mi, _types_found,
-  _succeeded, _succeeded_count);
+  if (cc_succeeded)
+   {
+ ret = multi_client_connect_call_plugin_v1 (m, mi,
+_types_found);
+ if (ret == CC_RET_SUCCEEDED)
+   ++cc_succeeded_count;
+ else if (ret == CC_RET_FAILED)
+   cc_succeeded = false;
+   }

-  multi_client_connect_call_plugin_v2 (m, mi, _types_found,
-  _succeeded, _succeeded_count);
+  if (cc_succeeded)
+   {
+ ret = multi_client_connect_call_plugin_v2 (m, mi,
+_types_found);
+ if (ret == CC_RET_SUCCEEDED)
+   ++cc_succeeded_count;
+ else if (ret == CC_RET_FAILED)
+   cc_succeeded = false;
+   }

-  /*
-   * Run --client-connect script.
-   */
   if (cc_succeeded)
{
- multi_client_connect_call_script (m, mi, _types_found,
-   _succeeded,
-   _succeeded_count);
+ ret = multi_client_connect_call_script (m, mi, _types_found);
+ if (ret == CC_RET_SUCCEEDED)
+   ++cc_succeeded_count;
+ else if (ret == CC_RET_FAILED)
+   cc_succeeded = false;
}

-  /*
-   * Check for client-connect script left by management interface client
-   */
   if (cc_succeeded)
{
- multi_client_connect_mda (m, mi, _types_found,
-   _succeeded, _succeeded_count);
+ ret = multi_client_connect_mda (m, mi, _types_found);
+ if (ret == CC_RET_SUCCEEDED)
+   ++cc_succeeded_count;
+ else if (ret == CC_RET_FAILED)
+   cc_succeeded = false;
}

-  multi_client_connect_late_setup (m, mi, option_types_found,
-  cc_succeeded, cc_succeeded_count);
+  multi_client_connect_late_setup (m, mi, option_types_found, cc_succeeded,
+  cc_succeeded_count);

   /* set flag so we don't get called again */
   mi->connection_established_flag = true;
@@ -1712,11 +1733,13 @@ multi_client_connect_early_setup (struct multi_context 
*m,
  * Try to source a dynamic config file from the
  * --client-config-dir directory.
  */
-static void
+static enum client_connect_return
 multi_client_connect_source_ccd (struct multi_context *m,
 struct multi_

[Openvpn-devel] [PATCH v2 8/9] client-connect: Add deferred support to the client-connect script handler

[Fabian Knittel]

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 <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 185 
 src/openvpn/multi.h |   3 +
 2 files changed, 177 insertions(+), 11 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 7079946..68a7248 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1621,6 +1621,129 @@ multi_client_connect_early_setup (struct multi_context 
*m,
   multi_client_connect_setenv (m, mi);
 }

+
+static void
+ccs_delete_deferred_ret_file (struct multi_instance *mi)
+{
+  struct client_connect_state *ccs = mi->client_connect_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;
+}
+}
+
+static bool
+ccs_gen_deferred_ret_file (struct multi_instance *mi)
+{
+  struct client_connect_state *ccs = mi->client_connect_state;
+  struct gc_arena gc = gc_new ();
+  const char *fn;
+
+  if (ccs->deferred_ret_file)
+ccs_delete_deferred_ret_file (mi);
+
+  fn = create_temp_file (mi->context.options.tmp_dir, "ccr", );
+  if (!fn)
+{
+  gc_free ();
+  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 ();
+  return true;
+}
+
+/*
+ * Tests whether the deferred return value file exists and returns the
+ * contained return value.
+ *
+ * Returns CC_RET_SKIPPED if the file doesn't exist or is empty.
+ * Returns 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_state *ccs = mi->client_connect_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:
+ ret = CC_RET_SKIPPED;
+ break;
+   default:
+ /* We received an unknown/unexpected value.  Assume failure. */
+ ret = CC_RET_FAILED;
+   }
+  fclose (fp);
+}
+  return ret;
+}
+
+
+static void
+ccs_delete_config_file (struct multi_instance *mi)
+{
+  struct client_connect_state *ccs = mi->client_connect_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;
+}
+}
+
+static bool
+ccs_gen_config_file (struct multi_instance *mi)
+{
+  struct client_connect_state *ccs = mi->client_connect_state;
+  struct gc_arena gc = gc_new ();
+  const char *fn;
+
+  if (ccs->config_file)
+ccs_delete_config_file (mi);
+
+  fn = create_temp_file (mi->context.options.tmp_dir, "cc", );
+  if (!fn)
+{
+  gc_free ();
+  return false;
+}
+  ccs->config_file = string_alloc (fn, NULL);
+
+  setenv_str (mi->context.c2.es, "client_connect_config_file",
+ ccs->config_file);
+
+  gc_free ();
+  return true;
+}
+
+
 /*
  * Try to source a dynamic config file from the
  * --client-config-dir directory.
@@ -1786,34 +1909,71 @@ multi_client_connect_call_script (struct multi_context 
*m,

   if (mi->context.options.client_connect_script)
 {
-  struct gc_arena gc = gc_new ();
+  struct client_con

[Openvpn-devel] [PATCH v2 9/9] client-connect: Add deferred support to the client-connect plugin v1 handler

[Fabian Knittel]

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 <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 32 
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 68a7248..ab73034 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1814,6 +1814,7 @@ multi_client_connect_call_plugin_v1 (struct multi_context 
*m,
 {
   enum client_connect_return ret = CC_RET_SKIPPED;
 #ifdef ENABLE_PLUGIN
+  struct client_connect_state *ccs = mi->client_connect_state;
   ASSERT (m);
   ASSERT (mi);
   ASSERT (option_types_found);
@@ -1821,34 +1822,41 @@ multi_client_connect_call_plugin_v1 (struct 
multi_context *m,
   if (plugin_defined (mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
 {
   int plug_ret;
-  struct gc_arena gc = gc_new ();
   struct argv argv = argv_new ();
-  const char *dc_file = create_temp_file (mi->context.options.tmp_dir, 
"cc", );

-  if (!dc_file)
+  if (!ccs_gen_config_file (mi) ||
+ !ccs_gen_deferred_ret_file (mi))
{
  ret = CC_RET_FAILED;
- goto script_depr_failed;
+ goto cleanup;
}

-  argv_printf (, "%s", dc_file);
+  argv_printf (, "%s", ccs->config_file);

   plug_ret = plugin_call (mi->context.plugins,
  OPENVPN_PLUGIN_CLIENT_CONNECT,
  , NULL, mi->context.c2.es);
   argv_reset ();
-  if (plug_ret != OPENVPN_PLUGIN_FUNC_SUCCESS)
+  if (plug_ret == OPENVPN_PLUGIN_FUNC_SUCCESS)
+   {
+ 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;
+  else
{
  msg (M_WARN, "WARNING: client-connect plugin call failed");
  ret = CC_RET_FAILED;
}
-  else
+
+cleanup:
+  if (ret != CC_RET_DEFERRED)
{
- multi_client_connect_post (m, mi, dc_file, option_types_found);
- ret = CC_RET_SUCCEEDED;
+ ccs_delete_config_file (mi);
+ ccs_delete_deferred_ret_file (mi);
}
-script_depr_failed:
-  gc_free ();
 }
 #endif
   return ret;
@@ -2141,7 +2149,7 @@ static const struct client_connect_handlers 
client_connect_handlers[] = {
   },
   {
 main: multi_client_connect_call_plugin_v1,
-deferred: multi_client_connect_fail
+deferred: multi_client_handle_deferred
   },
   {
 main: multi_client_connect_call_plugin_v2,
-- 
2.1.4

[Openvpn-devel] [PATCH v2 2/9] client-connect: Properly indent all functions

[Fabian Knittel]

This patch adds proper indentation to all new helper functions.  No functional
changes are performed.  Nothing is moved around.

Signed-off-by: Fabian Knittel <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 408 ++--
 1 file changed, 204 insertions(+), 204 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index c24b203..80bd5a3 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1697,20 +1697,20 @@ static void
 multi_client_connect_early_setup (struct multi_context *m,
  struct multi_instance *mi)
 {
-  /* 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);
+  /* 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);

-  /* generate a msg() prefix for this client instance */
-  generate_prefix (mi);
+  /* generate a msg() prefix for this client instance */
+  generate_prefix (mi);

-  /* delete instances of previous clients with same common-name */
-  if (!mi->context.options.duplicate_cn)
-   multi_delete_dup (m, mi);
+  /* delete instances of previous clients with same common-name */
+  if (!mi->context.options.duplicate_cn)
+multi_delete_dup (m, mi);

-  /* reset pool handle to null */
-  mi->vaddr_handle = -1;
+  /* reset pool handle to null */
+  mi->vaddr_handle = -1;
 }

 /*
@@ -1722,16 +1722,31 @@ 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)
+  if (mi->context.options.client_config_dir)
+{
+  struct gc_arena gc = gc_new ();
+  const char *ccd_file;
+  
+  ccd_file = gen_path (mi->context.options.client_config_dir,
+  tls_common_name (mi->context.c2.tls_multi, false),
+  );
+
+  /* try common-name file */
+  if (test_file (ccd_file))
+   {
+ options_server_import (>context.options,
+ccd_file,
+D_IMPORT_ERRORS|M_OPTERR,
+CLIENT_CONNECT_OPT_MASK,
+option_types_found,
+mi->context.c2.es);
+   }
+  else /* try default file */
{
- struct gc_arena gc = gc_new ();
- const char *ccd_file;
- 
  ccd_file = gen_path (mi->context.options.client_config_dir,
-  tls_common_name (mi->context.c2.tls_multi, 
false),
+  CCD_DEFAULT,
   );

- /* try common-name file */
  if (test_file (ccd_file))
{
  options_server_import (>context.options,
@@ -1741,25 +1756,10 @@ multi_client_connect_source_ccd (struct multi_context 
*m,
 option_types_found,
 mi->context.c2.es);
}
- else /* try default file */
-   {
- ccd_file = gen_path (mi->context.options.client_config_dir,
-  CCD_DEFAULT,
-  );
-
- if (test_file (ccd_file))
-   {
- options_server_import (>context.options,
-ccd_file,
-D_IMPORT_ERRORS|M_OPTERR,
-CLIENT_CONNECT_OPT_MASK,
-option_types_found,
-mi->context.c2.es);
-   }
-   }
-
- gc_free ();
}
+
+  gc_free ();
+}
 }

 /*
@@ -1775,44 +1775,44 @@ multi_client_connect_call_plugin_v1 (struct 
multi_context *m,
 int *cc_succeeded_count)
 {
 #ifdef ENABLE_PLUGIN
-  ASSERT (m);
-  ASSERT (mi);
-  ASSERT (option_types_found);
-  ASSERT (cc_succeeded);
-  ASSERT (cc_succeeded_count);
+  ASSERT (m);
+  ASSERT (mi);
+  ASSERT (option_types_found);
+  ASSERT (cc_succeeded);
+  ASSERT (cc_succeeded_count);

-  if (plugin_defined (mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
-   {
- int plug_ret;
- struct gc_arena gc = gc_new ();
- struct argv argv = argv_new ();
- const char *dc_file = create_temp_file (mi->context.options.tmp_dir, 
"cc", );
+  if (plugin_defined (mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
+ 

[Openvpn-devel] [PATCH v2 6/9] client-connect: Refactor client-connect handling to calling a bunch of hooks in a loop

[Fabian Knittel]

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 <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 57 +++--
 1 file changed, 16 insertions(+), 41 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 072a5f6..648b026 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1638,6 +1638,18 @@ 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_t 
(*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
+  };
+
+  int cur_handler = 0;
   unsigned int option_types_found = 0;
   int cc_succeeded = true; /* client connect script status */
   int cc_succeeded_count = 0;
@@ -1645,51 +1657,14 @@ multi_connection_established (struct multi_context *m, 
struct multi_instance *mi

   multi_client_connect_early_setup (m, mi);

-  if (cc_succeeded)
+  while (succeeded && handlers[cur_handler])
{
- ret = multi_client_connect_source_ccd (m, mi, _types_found);
+ ret = handlers[cur_handler] (m, mi, _types_found);
  if (ret == CC_RET_SUCCEEDED)
++cc_succeeded_count;
  else if (ret == CC_RET_FAILED)
-   cc_succeeded = false;
-   }
-
-  if (cc_succeeded)
-   {
- ret = multi_client_connect_call_plugin_v1 (m, mi,
-_types_found);
- if (ret == CC_RET_SUCCEEDED)
-   ++cc_succeeded_count;
- else if (ret == CC_RET_FAILED)
-   cc_succeeded = false;
-   }
-
-  if (cc_succeeded)
-   {
- ret = multi_client_connect_call_plugin_v2 (m, mi,
-_types_found);
- if (ret == CC_RET_SUCCEEDED)
-   ++cc_succeeded_count;
- else if (ret == CC_RET_FAILED)
-   cc_succeeded = false;
-   }
-
-  if (cc_succeeded)
-   {
- ret = multi_client_connect_call_script (m, mi, _types_found);
- if (ret == CC_RET_SUCCEEDED)
-   ++cc_succeeded_count;
- else if (ret == CC_RET_FAILED)
-   cc_succeeded = false;
-   }
-
-  if (cc_succeeded)
-   {
- ret = multi_client_connect_mda (m, mi, _types_found);
- if (ret == CC_RET_SUCCEEDED)
-   ++cc_succeeded_count;
- else if (ret == CC_RET_FAILED)
-   cc_succeeded = false;
+   succeeded = false;
+ ++cur_handler;
}

   multi_client_connect_late_setup (m, mi, option_types_found, cc_succeeded,
-- 
2.1.4

[Openvpn-devel] [PATCH v2 1/9] client-connect: Split multi_connection_established into separate functions

[Fabian Knittel]

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 <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c   | 322 ++
 src/openvpn/options.h |   6 +
 2 files changed, 224 insertions(+), 104 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 6ddfbb5..c24b203 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1461,7 +1461,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? */
@@ -1470,7 +1469,7 @@ multi_client_connect_post (struct multi_context *m,
   options_server_import (>context.options,
 dc_file,
 D_IMPORT_ERRORS|M_OPTERR,
-option_permissions_mask,
+CLIENT_CONNECT_OPT_MASK,
 option_types_found,
 mi->context.c2.es);

@@ -1494,7 +1493,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;
@@ -1511,7 +1509,7 @@ multi_client_connect_post_plugin (struct multi_context *m,
options_string_import (>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);
}
@@ -1529,29 +1527,28 @@ multi_client_connect_post_plugin (struct multi_context 
*m,

 #endif

-#ifdef MANAGEMENT_DEF_AUTH
-
 /*
  * Called to load management-derived client-connect config
  */
 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)
+ unsigned int *option_types_found,
+ int *cc_succeeded,
+ int *cc_succeeded_count)
 {
-  if (config)
+#ifdef MANAGEMENT_DEF_AUTH
+  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(>buf);
  options_string_import (>context.options,
 opt,
 D_IMPORT_ERRORS|M_OPTERR,
-option_permissions_mask,
+CLIENT_CONNECT_OPT_MASK,
 option_types_found,
 mi->context.c2.es);
}
@@ -1564,10 +1561,11 @@ multi_client_connect_mda (struct multi_context *m,
*/
   multi_select_virtual_addr (m, mi);
   multi_set_virtual_addr_env (m, mi);
-}
-}

+  ++*cc_succeeded_count;
+}
 #endif
+}

 static void
 multi_client_connect_setenv (struct multi_context *m,
@@ -1594,6 +1592,38 @@ multi_client_connect_setenv (struct multi_context *m,
   gc_free ();
 }

+static void
+multi_client_connect_early_setup (struct multi_context *m,
+ struct multi_instance *mi);
+static void
+multi_client_connect_source_ccd (struct multi_context *m,
+struct multi_instance *mi,
+unsigned int *option_types_found);
+static void
+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);
+static void
+multi_client_connect_call_plugin_v2 (struct multi_context *m,
+struct multi_instance *mi,
+

[Openvpn-devel] [PATCH v2 0/9] Refactor client-connect / add support for deferred handling

[Fabian Knittel]

Hi,

changes from v1:
 - rebased to the current master,
 - reports errors during file delete

Introduction to the patch-set:

A few years ago I wrote a patch-set concerning OpenVPN's client-connect
code.  The first part of the patch-set (patches 1 to 6) refactors and unifies
the client-connect code.

The second part of the patch-set allows client-connect handling to proceed
asynchronously, similar to how OpenVPN supports asynchronous (or deferred)
authentication.  Basically, the scripts or v1-plugins handling the
client-connect event can now write back an additional status code that
indicates deferred handling of the client-connect.  This causes the OpenVPN
server to continue with other things and to regularly re-read the status file.
As soon as the status changes from "deferred" to "failed" or "succeeded", the
client-connect processing for the connection is continued.  (As mentioned
before, the auth code does something very similar.)

The motivation for the deferred handling approach were relatively long running
client-connect scripts (> 2s) intended for high-traffic servers.  As the
OpenVPN server completely blocks while synchronously waiting for client-connect
scripts to complete, the asynchronous / deferred approach was needed.

The deferred script code path has been in production use for a few years now,
although this freshly rebased patch-set has only seen light testing so far.

Feed-back would be very welcome.

The patches are also availabe on a Github branch:
https://github.com/fknittel/openvpn/tree/feat_deferred_client-connect

Cheers
Fabian

PS: See
https://github.com/fknittel/openvpn/wiki/Patch-set-%22deferred-client-connect%22
in case you're interested in ready-made Debian packages.


Fabian Knittel (9):
  client-connect: Split multi_connection_established into separate
functions
  client-connect: Properly indent all functions
  client-connect: Refactor multi_client_connect_source_ccd
  client-connect: Move multi_client_connect_setenv into early_setup
  client-connect: Refactor to use return values instead of modifying a
passed-in flag
  client-connect: Refactor client-connect handling to calling a bunch of
hooks in a loop
  client-connect: Add CC_RET_DEFERRED and cope with deferred
client-connect
  client-connect: Add deferred support to the client-connect script
handler
  client-connect: Add deferred support to the client-connect plugin v1
handler

 src/openvpn/multi.c   | 839 +++---
 src/openvpn/multi.h   |  32 +-
 src/openvpn/options.h |   6 +
 3 files changed, 636 insertions(+), 241 deletions(-)

-- 
2.1.4

[Openvpn-devel] [PATCH 4/9] client-connect: Move multi_client_connect_setenv into early_setup

[Fabian Knittel]

This patch moves multi_client_connect_setenv into
multi_client_connect_early_setup and makes sure that every client-connect
handling function updates the virtual address selection.

Background: This unifies how the client-connect handling functions work.

Signed-off-by: Fabian Knittel <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 757e687..a34f985 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1635,15 +1635,6 @@ multi_connection_established (struct multi_context *m, 
struct multi_instance *mi

   multi_client_connect_source_ccd (m, mi, _types_found);

-  /*
-   * 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, _types_found,
   _succeeded, _succeeded_count);

@@ -1700,6 +1691,10 @@ multi_client_connect_early_setup (struct multi_context 
*m,

   /* reset pool handle to null */
   mi->vaddr_handle = -1;
+
+  /* do --client-connect setenvs */
+  multi_select_virtual_addr (m, mi);
+  multi_client_connect_setenv (m, mi);
 }

 /*
@@ -1742,6 +1737,12 @@ multi_client_connect_source_ccd (struct multi_context *m,
 option_types_found,
 mi->context.c2.es);

+ /*
+  * Select a virtual address from either --ifconfig-push in
+  * --client-config-dir file or --ifconfig-pool.
+  */
+ multi_select_virtual_addr (m, mi);
+ multi_set_virtual_addr_env (m, mi);
}

   gc_free ();
-- 
2.1.1

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

[Fabian Knittel]

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 <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 122 +++-
 src/openvpn/multi.h |  11 -
 2 files changed, 82 insertions(+), 51 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a34f985..4ead41e 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1519,13 +1519,12 @@ multi_client_connect_post_plugin (struct multi_context 
*m,
 /*
  * Called to load management-derived client-connect config
  */
-static void
+static enum client_connect_return
 multi_client_connect_mda (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 MANAGEMENT_DEF_AUTH
   if (mi->cc_config)
 {
@@ -1551,9 +1550,10 @@ multi_client_connect_mda (struct multi_context *m,
   multi_select_virtual_addr (m, mi);
   multi_set_virtual_addr_env (m, mi);

-  ++*cc_succeeded_count;
+  ret = CC_RET_SUCCEEDED;
 }
 #endif
+  return ret;
 }

 static void
@@ -1630,38 +1630,59 @@ multi_connection_established (struct multi_context *m, 
struct multi_instance *mi
   unsigned int option_types_found = 0;
   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, _types_found);
+  if (cc_succeeded)
+   {
+ ret = multi_client_connect_source_ccd (m, mi, _types_found);
+ if (ret == CC_RET_SUCCEEDED)
+   ++cc_succeeded_count;
+ else if (ret == CC_RET_FAILED)
+   cc_succeeded = false;
+   }

-  multi_client_connect_call_plugin_v1 (m, mi, _types_found,
-  _succeeded, _succeeded_count);
+  if (cc_succeeded)
+   {
+ ret = multi_client_connect_call_plugin_v1 (m, mi,
+_types_found);
+ if (ret == CC_RET_SUCCEEDED)
+   ++cc_succeeded_count;
+ else if (ret == CC_RET_FAILED)
+   cc_succeeded = false;
+   }

-  multi_client_connect_call_plugin_v2 (m, mi, _types_found,
-  _succeeded, _succeeded_count);
+  if (cc_succeeded)
+   {
+ ret = multi_client_connect_call_plugin_v2 (m, mi,
+_types_found);
+ if (ret == CC_RET_SUCCEEDED)
+   ++cc_succeeded_count;
+ else if (ret == CC_RET_FAILED)
+   cc_succeeded = false;
+   }

-  /*
-   * Run --client-connect script.
-   */
   if (cc_succeeded)
{
- multi_client_connect_call_script (m, mi, _types_found,
-   _succeeded,
-   _succeeded_count);
+ ret = multi_client_connect_call_script (m, mi, _types_found);
+ if (ret == CC_RET_SUCCEEDED)
+   ++cc_succeeded_count;
+ else if (ret == CC_RET_FAILED)
+   cc_succeeded = false;
}

-  /*
-   * Check for client-connect script left by management interface client
-   */
   if (cc_succeeded)
{
- multi_client_connect_mda (m, mi, _types_found,
-   _succeeded, _succeeded_count);
+ ret = multi_client_connect_mda (m, mi, _types_found);
+ if (ret == CC_RET_SUCCEEDED)
+   ++cc_succeeded_count;
+ else if (ret == CC_RET_FAILED)
+   cc_succeeded = false;
}

-  multi_client_connect_late_setup (m, mi, option_types_found,
-  cc_succeeded, cc_succeeded_count);
+  multi_client_connect_late_setup (m, mi, option_types_found, cc_succeeded,
+  cc_succeeded_count);

   /* set flag so we don't get called again */
   mi->connection_established_flag = true;
@@ -1701,11 +1722,13 @@ multi_client_connect_early_setup (struct multi_context 
*m,
  * Try to source a dynamic config file from the
  * --client-config-dir directory.
  */
-static void
+static enum client_connect_return
 multi_client_connect_source_ccd (struct multi_context *m,
 struct multi_

[Openvpn-devel] [PATCH 2/9] client-connect: Properly indent all functions

[Fabian Knittel]

This patch adds proper indentation to all new helper functions.  No functional
changes are performed.  Nothing is moved around.

Signed-off-by: Fabian Knittel <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 408 ++--
 1 file changed, 204 insertions(+), 204 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index ed10e20..23b86a5 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1686,20 +1686,20 @@ static void
 multi_client_connect_early_setup (struct multi_context *m,
  struct multi_instance *mi)
 {
-  /* 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);
+  /* 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);

-  /* generate a msg() prefix for this client instance */
-  generate_prefix (mi);
+  /* generate a msg() prefix for this client instance */
+  generate_prefix (mi);

-  /* delete instances of previous clients with same common-name */
-  if (!mi->context.options.duplicate_cn)
-   multi_delete_dup (m, mi);
+  /* delete instances of previous clients with same common-name */
+  if (!mi->context.options.duplicate_cn)
+multi_delete_dup (m, mi);

-  /* reset pool handle to null */
-  mi->vaddr_handle = -1;
+  /* reset pool handle to null */
+  mi->vaddr_handle = -1;
 }

 /*
@@ -1711,16 +1711,31 @@ 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)
+  if (mi->context.options.client_config_dir)
+{
+  struct gc_arena gc = gc_new ();
+  const char *ccd_file;
+  
+  ccd_file = gen_path (mi->context.options.client_config_dir,
+  tls_common_name (mi->context.c2.tls_multi, false),
+  );
+
+  /* try common-name file */
+  if (test_file (ccd_file))
+   {
+ options_server_import (>context.options,
+ccd_file,
+D_IMPORT_ERRORS|M_OPTERR,
+CLIENT_CONNECT_OPT_MASK,
+option_types_found,
+mi->context.c2.es);
+   }
+  else /* try default file */
{
- struct gc_arena gc = gc_new ();
- const char *ccd_file;
- 
  ccd_file = gen_path (mi->context.options.client_config_dir,
-  tls_common_name (mi->context.c2.tls_multi, 
false),
+  CCD_DEFAULT,
   );

- /* try common-name file */
  if (test_file (ccd_file))
{
  options_server_import (>context.options,
@@ -1730,25 +1745,10 @@ multi_client_connect_source_ccd (struct multi_context 
*m,
 option_types_found,
 mi->context.c2.es);
}
- else /* try default file */
-   {
- ccd_file = gen_path (mi->context.options.client_config_dir,
-  CCD_DEFAULT,
-  );
-
- if (test_file (ccd_file))
-   {
- options_server_import (>context.options,
-ccd_file,
-D_IMPORT_ERRORS|M_OPTERR,
-CLIENT_CONNECT_OPT_MASK,
-option_types_found,
-mi->context.c2.es);
-   }
-   }
-
- gc_free ();
}
+
+  gc_free ();
+}
 }

 /*
@@ -1764,44 +1764,44 @@ multi_client_connect_call_plugin_v1 (struct 
multi_context *m,
 int *cc_succeeded_count)
 {
 #ifdef ENABLE_PLUGIN
-  ASSERT (m);
-  ASSERT (mi);
-  ASSERT (option_types_found);
-  ASSERT (cc_succeeded);
-  ASSERT (cc_succeeded_count);
+  ASSERT (m);
+  ASSERT (mi);
+  ASSERT (option_types_found);
+  ASSERT (cc_succeeded);
+  ASSERT (cc_succeeded_count);

-  if (plugin_defined (mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
-   {
- int plug_ret;
- struct gc_arena gc = gc_new ();
- struct argv argv = argv_new ();
- const char *dc_file = create_temp_file (mi->context.options.tmp_dir, 
"cc", );
+  if (plugin_defined (mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
+ 

[Openvpn-devel] [PATCH 8/9] client-connect: Add deferred support to the client-connect script handler

[Fabian Knittel]

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 <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 177 +---
 src/openvpn/multi.h |   3 +
 2 files changed, 171 insertions(+), 9 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index c044d67..a3d94fa 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1610,6 +1610,125 @@ multi_client_connect_early_setup (struct multi_context 
*m,
   multi_client_connect_setenv (m, mi);
 }

+
+static void
+ccs_delete_deferred_ret_file (struct multi_instance *mi)
+{
+  struct client_connect_state *ccs = mi->client_connect_state;
+  if (ccs->deferred_ret_file)
+{
+  setenv_del (mi->context.c2.es, "client_connect_deferred_file");
+  platform_unlink (ccs->deferred_ret_file);
+  free (ccs->deferred_ret_file);
+  ccs->deferred_ret_file = NULL;
+}
+}
+
+static bool
+ccs_gen_deferred_ret_file (struct multi_instance *mi)
+{
+  struct client_connect_state *ccs = mi->client_connect_state;
+  struct gc_arena gc = gc_new ();
+  const char *fn;
+
+  if (ccs->deferred_ret_file)
+ccs_delete_deferred_ret_file (mi);
+
+  fn = create_temp_file (mi->context.options.tmp_dir, "ccr", );
+  if (!fn)
+{
+  gc_free ();
+  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 ();
+  return true;
+}
+
+/*
+ * Tests whether the deferred return value file exists and returns the
+ * contained return value.
+ *
+ * Returns CC_RET_SKIPPED if the file doesn't exist or is empty.
+ * Returns 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_state *ccs = mi->client_connect_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:
+ ret = CC_RET_SKIPPED;
+ break;
+   default:
+ /* We received an unknown/unexpected value.  Assume failure. */
+ ret = CC_RET_FAILED;
+   }
+  fclose (fp);
+}
+  return ret;
+}
+
+
+static void
+ccs_delete_config_file (struct multi_instance *mi)
+{
+  struct client_connect_state *ccs = mi->client_connect_state;
+  if (ccs->config_file)
+{
+  setenv_del (mi->context.c2.es, "client_connect_config_file");
+  platform_unlink (ccs->config_file);
+  free (ccs->config_file);
+  ccs->config_file = NULL;
+}
+}
+
+static bool
+ccs_gen_config_file (struct multi_instance *mi)
+{
+  struct client_connect_state *ccs = mi->client_connect_state;
+  struct gc_arena gc = gc_new ();
+  const char *fn;
+
+  if (ccs->config_file)
+ccs_delete_config_file (mi);
+
+  fn = create_temp_file (mi->context.options.tmp_dir, "cc", );
+  if (!fn)
+{
+  gc_free ();
+  return false;
+}
+  ccs->config_file = string_alloc (fn, NULL);
+
+  setenv_str (mi->context.c2.es, "client_connect_config_file",
+ ccs->config_file);
+
+  gc_free ();
+  return true;
+}
+
+
 /*
  * Try to source a dynamic config file from the
  * --client-config-dir directory.
@@ -1775,14 +1894,14 @@ multi_client_connect_call_script (struct multi_context 
*m,

   if (mi->context.options.client_connect_script)
 {
-  struct gc_arena gc = gc_new ();
+  struct client_connect_state *ccs = mi->client_connect_state;
   struct argv argv = argv_new ();
-  const char *dc_file;
+  ASSERT (ccs);

   setenv_str (mi->context.c2.es, "script_type", "client-connect");

-  dc_file = create_temp_file

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

[Fabian Knittel]

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 <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 30 +++---
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index a3d94fa..c888e39 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1799,6 +1799,7 @@ multi_client_connect_call_plugin_v1 (struct multi_context 
*m,
 {
   enum client_connect_return ret = CC_RET_SKIPPED;
 #ifdef ENABLE_PLUGIN
+  struct client_connect_state *ccs = mi->client_connect_state;
   ASSERT (m);
   ASSERT (mi);
   ASSERT (option_types_found);
@@ -1806,34 +1807,41 @@ multi_client_connect_call_plugin_v1 (struct 
multi_context *m,
   if (plugin_defined (mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
 {
   int plug_ret;
-  struct gc_arena gc = gc_new ();
   struct argv argv = argv_new ();
-  const char *dc_file = create_temp_file (mi->context.options.tmp_dir, 
"cc", );

-  if (!dc_file)
+  if (!ccs_gen_config_file (mi) ||
+ !ccs_gen_deferred_ret_file (mi))
{
  ret = CC_RET_FAILED;
  goto script_depr_failed;
}

-  argv_printf (, "%s", dc_file);
+  argv_printf (, "%s", ccs->config_file);

   plug_ret = plugin_call (mi->context.plugins,
  OPENVPN_PLUGIN_CLIENT_CONNECT,
  , NULL, mi->context.c2.es);
   argv_reset ();
-  if (plug_ret != OPENVPN_PLUGIN_FUNC_SUCCESS)
+  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;
   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;
}
+
 script_depr_failed:
-  gc_free ();
+  if (ret != CC_RET_DEFERRED)
+   {
+ ccs_delete_config_file (mi);
+ ccs_delete_deferred_ret_file (mi);
+   }
 }
 #endif
   return ret;
@@ -2126,7 +2134,7 @@ static const struct client_connect_handlers 
client_connect_handlers[] = {
   },
   {
 main: multi_client_connect_call_plugin_v1,
-deferred: multi_client_connect_fail
+deferred: multi_client_handle_deferred
   },
   {
 main: multi_client_connect_call_plugin_v2,
-- 
2.1.1

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

[Fabian Knittel]

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 <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 57 +++--
 1 file changed, 16 insertions(+), 41 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 4ead41e..7815146 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1627,6 +1627,18 @@ 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_t 
(*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
+  };
+
+  int cur_handler = 0;
   unsigned int option_types_found = 0;
   int cc_succeeded = true; /* client connect script status */
   int cc_succeeded_count = 0;
@@ -1634,51 +1646,14 @@ multi_connection_established (struct multi_context *m, 
struct multi_instance *mi

   multi_client_connect_early_setup (m, mi);

-  if (cc_succeeded)
+  while (succeeded && handlers[cur_handler])
{
- ret = multi_client_connect_source_ccd (m, mi, _types_found);
+ ret = handlers[cur_handler] (m, mi, _types_found);
  if (ret == CC_RET_SUCCEEDED)
++cc_succeeded_count;
  else if (ret == CC_RET_FAILED)
-   cc_succeeded = false;
-   }
-
-  if (cc_succeeded)
-   {
- ret = multi_client_connect_call_plugin_v1 (m, mi,
-_types_found);
- if (ret == CC_RET_SUCCEEDED)
-   ++cc_succeeded_count;
- else if (ret == CC_RET_FAILED)
-   cc_succeeded = false;
-   }
-
-  if (cc_succeeded)
-   {
- ret = multi_client_connect_call_plugin_v2 (m, mi,
-_types_found);
- if (ret == CC_RET_SUCCEEDED)
-   ++cc_succeeded_count;
- else if (ret == CC_RET_FAILED)
-   cc_succeeded = false;
-   }
-
-  if (cc_succeeded)
-   {
- ret = multi_client_connect_call_script (m, mi, _types_found);
- if (ret == CC_RET_SUCCEEDED)
-   ++cc_succeeded_count;
- else if (ret == CC_RET_FAILED)
-   cc_succeeded = false;
-   }
-
-  if (cc_succeeded)
-   {
- ret = multi_client_connect_mda (m, mi, _types_found);
- if (ret == CC_RET_SUCCEEDED)
-   ++cc_succeeded_count;
- else if (ret == CC_RET_FAILED)
-   cc_succeeded = false;
+   succeeded = false;
+ ++cur_handler;
}

   multi_client_connect_late_setup (m, mi, option_types_found, cc_succeeded,
-- 
2.1.1

[Openvpn-devel] [PATCH 0/9] Refactor client-connect / add support for deferred handling

[Fabian Knittel]

Hi,

a few years ago I wrote a patch-set concerning OpenVPN's client-connect
code.  The first part of the patch-set (patches 1 to 6) refactors and unifies
the client-connect code.  (This might be of interest in the current "Fix
temporary file leak"-thread.)

The second part of the patch-set allows client-connect handling to proceed
asynchronously, similar to how OpenVPN supports asynchronous (or deferred)
authentication.  Basically, the scripts or v1-plugins handling the
client-connect event can now write back an additional status code that
indicates deferred handling of the client-connect.  This causes the OpenVPN
server to continue with other things and to regularly re-read the status file.
As soon as the status changes from "deferred" to "failed" or "succeeded", the
client-connect processing for the connection is continued.  (As mentioned
before, the auth code does something very similar.)

The motivation for the deferred handling approach were relatively long running
client-connect scripts (> 2s) intended for high-traffic servers.  As the
OpenVPN server completely blocks while synchronously waiting for client-connect
scripts to complete, the asynchronous / deferred approach was needed.

The deferred script code path has been in production use for a few years now,
although this freshly rebased patch-set has only seen light testing so far.

Feed-back would be very welcome.

The patches are also availabe on a Github branch:
https://github.com/fknittel/openvpn/tree/feat_deferred_client-connect

Cheers
Fabian

PS: See
https://github.com/fknittel/openvpn/wiki/Patch-set-%22deferred-client-connect%22
in case you're interested in ready-made Debian packages.

Fabian Knittel (9):
  client-connect: Split multi_connection_established into separate
functions
  client-connect: Properly indent all functions
  client-connect: Refactor multi_client_connect_source_ccd
  client-connect: Move multi_client_connect_setenv into early_setup
  client-connect: Refactor to use return values instead of modifying a
passed-in flag
  client-connect: Refactor client-connect handling to calling a bunch of
hooks in a loop
  client-connect: Add CC_RET_DEFERRED and cope with deferred
client-connect
  client-connect: Add deferred support to the client-connect script
handler
  client-connect: Add deferred support to the client-connect plugin v1
handler

 src/openvpn/multi.c   | 831 --
 src/openvpn/multi.h   |  32 +-
 src/openvpn/options.h |   6 +
 3 files changed, 635 insertions(+), 234 deletions(-)

-- 
2.1.1

[Openvpn-devel] [PATCH 3/9] client-connect: Refactor multi_client_connect_source_ccd

[Fabian Knittel]

Refactor multi_client_connect_source_ccd(), so that options_server_import() (or
the success path in general) is only entered in one place within the function.

Signed-off-by: Fabian Knittel <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 29 +
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 23b86a5..757e687 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1721,7 +1721,19 @@ multi_client_connect_source_ccd (struct multi_context *m,
   );

   /* try common-name file */
-  if (test_file (ccd_file))
+  if (!test_file (ccd_file))
+   ccd_file = NULL;
+
+  if (!ccd_file)
+   {
+ ccd_file = gen_path (mi->context.options.client_config_dir,
+  CCD_DEFAULT, );
+ /* try default file */
+ if (!test_file (ccd_file))
+   ccd_file = NULL;
+   }
+
+  if (ccd_file)
{
  options_server_import (>context.options,
 ccd_file,
@@ -1729,22 +1741,7 @@ multi_client_connect_source_ccd (struct multi_context *m,
 CLIENT_CONNECT_OPT_MASK,
 option_types_found,
 mi->context.c2.es);
-   }
-  else /* try default file */
-   {
- ccd_file = gen_path (mi->context.options.client_config_dir,
-  CCD_DEFAULT,
-  );

- if (test_file (ccd_file))
-   {
- options_server_import (>context.options,
-ccd_file,
-D_IMPORT_ERRORS|M_OPTERR,
-CLIENT_CONNECT_OPT_MASK,
-option_types_found,
-mi->context.c2.es);
-   }
}

   gc_free ();
-- 
2.1.1

[Openvpn-devel] [PATCH 1/9] client-connect: Split multi_connection_established into separate functions

[Fabian Knittel]

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 <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c   | 312 +++---
 src/openvpn/options.h |   6 +
 2 files changed, 224 insertions(+), 94 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 5910154..ed10e20 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1446,7 +1446,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? */
@@ -1455,7 +1454,7 @@ multi_client_connect_post (struct multi_context *m,
   options_server_import (>context.options,
 dc_file,
 D_IMPORT_ERRORS|M_OPTERR,
-option_permissions_mask,
+CLIENT_CONNECT_OPT_MASK,
 option_types_found,
 mi->context.c2.es);

@@ -1483,7 +1482,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;
@@ -1500,7 +1498,7 @@ multi_client_connect_post_plugin (struct multi_context *m,
options_string_import (>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);
}
@@ -1518,29 +1516,28 @@ multi_client_connect_post_plugin (struct multi_context 
*m,

 #endif

-#ifdef MANAGEMENT_DEF_AUTH
-
 /*
  * Called to load management-derived client-connect config
  */
 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)
+ unsigned int *option_types_found,
+ int *cc_succeeded,
+ int *cc_succeeded_count)
 {
-  if (config)
+#ifdef MANAGEMENT_DEF_AUTH
+  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(>buf);
  options_string_import (>context.options,
 opt,
 D_IMPORT_ERRORS|M_OPTERR,
-option_permissions_mask,
+CLIENT_CONNECT_OPT_MASK,
 option_types_found,
 mi->context.c2.es);
}
@@ -1553,10 +1550,11 @@ multi_client_connect_mda (struct multi_context *m,
*/
   multi_select_virtual_addr (m, mi);
   multi_set_virtual_addr_env (m, mi);
-}
-}

+  ++*cc_succeeded_count;
+}
 #endif
+}

 static void
 multi_client_connect_setenv (struct multi_context *m,
@@ -1583,6 +1581,38 @@ multi_client_connect_setenv (struct multi_context *m,
   gc_free ();
 }

+static void
+multi_client_connect_early_setup (struct multi_context *m,
+ struct multi_instance *mi);
+static void
+multi_client_connect_source_ccd (struct multi_context *m,
+struct multi_instance *mi,
+unsigned int *option_types_found);
+static void
+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);
+static void
+multi_client_connect_call_plugin_v2 (struct multi_context *m,
+struct multi_instance *mi,
+

[Openvpn-devel] [PATCH 7/9] client-connect: Add CC_RET_DEFERRED and cope with deferred client-connect

[Fabian Knittel]

This patch moves the state, that was previously tracked within the
multi_connection_established() function, into struct client_connect_state.  The
multi_connection_established() function can now be exited and re-entered as
many times as necessary - without losing the client-connect handling state.

The patch also adds the new return value CC_RET_DEFERRED which indicates that
the handler couldn't complete immediately, and needs to be called later.  At
that point multi_connection_established() will exit without indicating
completion.

Each client-connect handler now has an (optional) additional call-back:  The
call-back for handling the deferred case.  If the main call-back returns
CC_RET_DEFERRED, the next call to the handler will be through the deferred
call-back.

Signed-off-by: Fabian Knittel <fabian.knit...@lettink.de>
---
 src/openvpn/multi.c | 255 ++--
 src/openvpn/multi.h |  18 
 2 files changed, 185 insertions(+), 88 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 7815146..c044d67 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -47,6 +47,9 @@

 /*#define MULTI_DEBUG_EVENT_LOOP*/

+static void delete_client_connect_state (struct multi_instance *mi);
+
+
 #ifdef MULTI_DEBUG_EVENT_LOOP
 static const char *
 id (struct multi_instance *mi)
@@ -573,6 +576,8 @@ multi_close_instance (struct multi_context *m,
   set_cc_config (mi, NULL);
 #endif

+  delete_client_connect_state (mi);
+
   multi_client_disconnect_script (m, mi);

   if (mi->did_open_context)
@@ -1583,94 +1588,6 @@ multi_client_connect_setenv (struct multi_context *m,

 static void
 multi_client_connect_early_setup (struct multi_context *m,
- struct multi_instance *mi);
-static void
-multi_client_connect_source_ccd (struct multi_context *m,
-struct multi_instance *mi,
-unsigned int *option_types_found);
-static void
-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);
-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);
-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);
-static void
-multi_client_connect_late_setup (struct multi_context *m,
-struct multi_instance *mi,
-const unsigned int option_types_found,
-int cc_succeeded,
-const int cc_succeeded_count);
-
-/*
- * 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)
-{
-  typedef enum client_connect_return_t 
(*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
-  };
-
-  int cur_handler = 0;
-  unsigned int option_types_found = 0;
-  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);
-
-  while (succeeded && handlers[cur_handler])
-   {
- ret = handlers[cur_handler] (m, mi, _types_found);
- if (ret == CC_RET_SUCCEEDED)
-   ++cc_succeeded_count;
- else if (ret == CC_RET_FAILED)
-   succeeded = false;
- ++cur_handler;
-   }
-
-  multi_client_connect_late_setup (m, mi, option_types_found, cc_succeeded,
-  cc_succeeded_count);
-
-  /* set flag so we don't get called again */
-  mi->connection_established_flag = true;
-}

Re: [Openvpn-devel] Async OPENVPN_PLUGIN_CLIENT_CONNECT plugin support

[Fabian Knittel]

Hi Lev,

2014-10-07 20:50 GMT+02:00 Lev Stipakov :

> Patch works great, thanks! I have rebased it a bit and added support
> for client-connect plugin call.
>

Great to hear that my patch works for you!

I've finally rebased the patches too, due to my current users complaining
about the old OpenVPN version and missing security updates. (See
https://github.com/fknittel/openvpn/tree/feat_deferred_client-connect -
there's a branch for master and a branch for the OpenVPN 2.3 stable branch.)

You say that you've added support for the client-connect plugin call. May I
ask what was missing? I'd definitely be interested in any changes you
needed to make it work. I would have expected the patch titled
"client-connect: Add deferred support to the client-connect plugin v1
handler" to provide everything, but as I only ever used the script handler,
it's not unlikely that I missed something.

I would like to offer a related feature (and implementation) I call
> async-push.
>
> Use case: authentication / authorization takes time. I have auth/az
> code in auth-user-pass-verify and client-connect calls, and sometimes
> it takes more that second to execute those. The problem is that after
> auth-user-pass-verify is done, OpenVPN server won’t proceed with
> client-connect unless some timeout/io event happens for that client.
> Also, server will not notify client that client-connect returned
> success unless client sends PULL_REQUEST. Client, in turn, sends
> PULL_REQUEST one second after connection initiation and after that
> once per 5 seconds. So, for example, if at the moment when first pull
> request has arrived, client-connect has not finished yet, we will have
> to wait another 5 seconds for the next PULL_REQUEST.
>

So this is basically about replacing a 5s poll-interval with something that
should proceed near instantaneously, correct?


> Solution: Inotify. Since OpenVPN creates itself files (auth-contro and
> client-connect-deferred) which names it passes to the plugin, we
> create one inotify descriptor for event loop and right after creating
> those files, we add inotify watch on those. Before calling epoll (or
> whatever we use) we add inotify descriptor to the list of watched
> descriptors. We also keep watch descriptor and multi_instance in a
> hashtable.
>
> When epoll informs us that an event has happened on inotify
> descriptor, we get multi_instance by watch descriptor (fetched from
> poll event) from our new hashtable and call multi_process_post for
> given multi_instance. This will check result from the file and
> eventually call multi_connection_established, from where we call
> send_push_reply.
>
> Since implementation uses Inotify, it will work on Linux only. Code is
> under #define, which is set at compile-time (--enable-async-push=yes).
>
> I have attached an implementation. So far has been working nicely in
> my test environment. I would love to hear a feedback from the
> community. Is the whole thing done more or less right? Any bugs got
> introduced that someone could spot?
>

Thanks for sharing! I'll have a look at it, when time permits. Though my
focus is currently on getting some or all of the patches from the basic
patch-set upstream.

Cheers
Fabian

Re: [Openvpn-devel] Async OPENVPN_PLUGIN_CLIENT_CONNECT plugin support

[Fabian Knittel]

Hi Lev,

2014-07-29 12:56 GMT+02:00 Lev Stipakov :

> I am pondering about asynchronous OPENVPN_PLUGIN_CLIENT_CONNECT
> callback. Basically, I want _not_ to establish connection until
> response is received and ofcI  don't want to block rest of traffic.
>

[ Details of approach snipped. ]

What do you think about that? Does that approach sound reasonable?
>

Some time ago I implemented something quite similar, but never quite
managed to officially submit it. You can find my old git branch here [0].
Unfortunately, to be of any use it would need to be ported to a current
OpenVPN release / master first.

The code has been in use for several years now [1], so the approach and the
code basically work quite well. (I think my use case involved calling a
Python script, but I might have implemented the plugin part too.)

If the OpenVPN commiters see a certain chance, that such a change could be
included upstream, I might even try to rebase the branch to master myself...

Cheers
Fabian

0:
http://opensource.fsmi.uni-karlsruhe.de/gitweb/?p=openvpn.git;a=shortlog;h=refs/heads/feat_deferred_client-connect
1: ... in a production environment with several hundred users (together
with the equally unofficial VLAN-tagging feature [2]). The feature is
needed by a daemon that does asynchronous IP-configuration via a central
DHCP server [3].
2:
http://opensource.fsmi.uni-karlsruhe.de/gitweb/?p=openvpn.git;a=shortlog;h=refs/heads/feat_vlan
3: https://gitorious.org/odr

Re: [Openvpn-devel] [PATCH] Repair "tap server" mode brokenness caused by fallout

[Fabian Knittel]

Hi everyone,

2012/6/30 Gert Doering :
> diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c
> index aecb702..850e336 100644
> --- a/src/openvpn/mroute.c
> +++ b/src/openvpn/mroute.c
> @@ -52,7 +52,7 @@ mroute_addr_init (struct mroute_addr *addr)
>  static inline bool
>  is_mac_mcast_addr (const uint8_t *mac)
>  {
> -  return (bool) mac[0] & 1;
> +  return (bool) (mac[0] & 1);
>  }
>
>  static inline bool

ACK. In the context of is_mac_mcast_addr, the check now looks obviously correct.

Cheers
Fabian

Re: [Openvpn-devel] [RFC] Split plugins into their own repositories

[Fabian Knittel]

Hi Alon,

2012/5/13 Alon Bar-Lev :
> On Sun, May 13, 2012 at 9:10 PM, Gert Doering  wrote:
>> Huh?  You're the master of Autoconf, and I'm sure you will be able to
>> produce a working PAM detection for those platforms that have it.
>
> Yes, and as such I tell you that automatic detection is something that
> leads to package breakage.

How does PAM detection relate to the question whether to split out the
plugin? Wouldn't the plugin's configure also need to detect PAM? As
already discussed previously, I much prefer configure to notice a
missing development package than to run into a failing "make" run.

And why would automatic pam detection lead to package breakage? Have
an option for enabling / disabling plugins. If a plugin is enabled and
needs PAM, check for the existence of PAM. If PAM isn't found, exit
configure and tell the user to pass the proper paths for pam headers
and libraries.

>>> >> These plugins should be optional I don't see any value in enforcing
>>> >> them and their dependencies.
>>> >
>>> > If these plugins are useful for a large number of users, there is no
>>> > point in not installing them by default.
>>>
>>> They are not.
>>
>> Unfounded claim.
>
> I know one or two installations, and I am tracking this project more
> than 6 years.
> Come on! most of installations are plain public key without any of
> these plugins.
> There is no need for these if you configure your server.
> I simply don't understand your attitude... sorry, I simply don't.

As already mentioned elsewhere in this thread, the currently included
plugins aren't a huge burden to maintain. So even if we were to agree
that most people don't need them by default, I would suggest to change
the build default, not throw them out of the repository.

I can see where the purity of the split-out would appeal to you, but
the increased number of separate repositories, releases, packages,
etc. to maintain forms a strong counter-point in my opinion. So I'm
against splitting out the plugins for now. (Just saw Eric's
openvpn-contrib proposal. Might serve as a compromise, but then we'd
still lose the purity advantage ...)

Cheers
Fabian

Re: [Openvpn-devel] [PATCH] Openvpn for Android 4.0 Changeset

[Fabian Knittel]

Hi Alon,

2012/5/11 Alon Bar-Lev <alon.bar...@gmail.com>:
> On Fri, May 11, 2012 at 12:11 AM, Fabian Knittel
> <fabian.knit...@lettink.de> wrote:
>> The main undecided points regarding the interface side of things appear to 
>> be:
>> * How to pass the fds back and forth: Special-case the management
>>  interface for unix domain sockets or use a dedicated unix domain
>>  socket for special operations. Could this potentially be the same
>>  interface that was discussed for some of the windows privilege
>>  separation approaches?
>
> Again, this is not the case of privileged separation as the process
> *IS* privileged to do the networking setup.
> People, don't get confused between running under root or not with privileges!
> Regular user can have privileges like creating tun and modify network
> setting without being root, as in this case, this is not privilege
> separation!

To be honest, I'm not sure what you're talking about. My point was,
that the ideas and approaches gathered in the wiki[0] might influence
the interface we're discussing right now. Specifically, Heiko's
interactive service would probably need a similar interface. (In your
COM+-approach this would be the interface between OpenVPNUI.Network
and OpenVPNUI.Tunnel, but as you're suggesting to use COM+, the shape
of an advanced management interface or unix-domain-socket-like
interface is ... of no real value.)

0: https://community.openvpn.net/openvpn/wiki/PrivilegeSeparation

> And again, these UI specific issues should should be modularize and
> not go into mainline as-is.

My point was, that we should agree on a stable interface first. In the
next step, we can decide whether a specific implementation of that
interface is worthy of inclusion. Unless of course, you're thinking
about COM+ as one of the UI interfaces. But in my opinion, if Windows
has a communication channel that is close to the semantics and
capabilities of a UNIX domain socket, we should probably use that as
_the_ interface and not needlessly start maintaining a separate
interface per UI/plattform.

[...]
> The question is do we want to make a library out of VPN with some
> interface such as openvpn_main() or not, the JNI question is totally
> unrelated to the mainline project.

I agree that your question is more to the point.

My opinion regarding the question is, that I don't yet see a need for
a library interface. Executable + unix domain sockets seem just fine.

>> * The precise names of the newly introduced management commands.
>
> If we provide plugin interface to management interface this can be a
> contract between the UI and the plugin, so that mainline should not be
> changed every time there is a special UI need.

If we create a contract that can be upheld by both a plugin and the
core, then we can agree on the contract first and agree or change the
implementation of the contract later. If we take several UI needs into
consideration, we might not need a zoo of management-UI-plugins.

> If we provide tun engine implementation that supports the management
> interface, all platforms can benefit from this change.

Sounds great. Why not make the management interface contract of that
tun engine the main topic of this discussion? That's what I'm aiming
at. That way, hopefully the short-term patch can go in and the
contract won't need to change as soon as 2.4 delivers the advanced
modularisation.

Cheers
Fabian

Re: [Openvpn-devel] [PATCH] Openvpn for Android 4.0 Changeset

[Fabian Knittel]

Hi,

2012/5/10 David Sommerseth :
> On 10/05/12 16:50, Alon Bar-Lev wrote:
>> Why? plugin is adding a custom logic, and you need custom logic. As
>> I wrote it does not imply that you implement your JNI there.
>
> Okay ... fine ... there are plenty of big visions for a future version
> of OpenVPN, and it will become more modular, yes.  I think we all
> agree there.
>
> What is far more important now is the review the *current* patch, this
> review process have so far derailed completely.  Lets put this current
> discussion aside.  Later on we will discuss this how to modularise
> OpenVPN anyway.

To allow OpenVPN to be properly refactored afterwards, the important
part is probably to get the external interfaces correct (which never
works in practice, but it would be nice to get close...).

The main undecided points regarding the interface side of things appear to be:
* How to pass the fds back and forth: Special-case the management
  interface for unix domain sockets or use a dedicated unix domain
  socket for special operations. Could this potentially be the same
  interface that was discussed for some of the windows privilege
  separation approaches?
* How to call OpenVPN from Java. Via thin JNI wrapper or as a regular
  executable.
* The precise names of the newly introduced management commands.

Anything else?

Cheers
Fabian

Re: [Openvpn-devel] [PATCH] build: add git revision to --version output if build from git repository

[Fabian Knittel]

Hi,

2012/5/3 David Sommerseth :
> What I would like to see is something more like what's found in TOR
> projects' obfsproxy, where the Makefile generates a micro-version.i,
> which is included.  As this file is forced to be (re-)created each
> time, it will always be accurate.
>
> I don't say that obfsproxy have done the implementation correct, but
> it can at least be a seed for inspiration:

I agree that the Makefile-based approach is more useful. Regarding the
implementation: Depending on compile vs link time it might be faster
to move the re-compiled part into a dedicated version.c which only
contains a

  const char *get_ovpn_version()
  {
return PACKAGE_VERSION_EXTRA_GIT_REVISION;
  }

Cheers
Fabian

Re: [Openvpn-devel] [PATCH] cleanup: add .gitattributes to control eol style explicitly

[Fabian Knittel]

Hi Alon,

2012/4/2 Alon Bar-Lev :
> Having the text auto detection is a risk, as the detection may detect
> text files that are not text and vise versa.
>
> Having global setting will create confusion and differentiate between
> users. So this patch also move this to local repository.

IMHO, this is a real benefit. Reduces the number of steps necessary to
contribute and avoids line-ending mistakes for new contributions.

> Having git to check out files differently in different OS is also
> a not correct, as checkouts may be used in shares or in *NIX emulation
> environments, so it have no effect.
>
> Another issue is packaging, if we change out the tree differently
> in several OSes, we may have different package content, which is
> something that should be avoided.
>
> Currently any editor of MS supports LF end of lines, so there is no
> need to convert source files while checking out.
>
> The visual studio files should be stored as CRLF as they are generated
> by visual studio every save, in a way that CRLF are added.
>
> I handled only the files that may be touch by MS users.

The rest I generally agree with, but don't have strong feelings about them.

> diff --git a/.gitattributes b/.gitattributes
> new file mode 100644
> index 000..34463f9
> --- /dev/null
> +++ b/.gitattributes
> @@ -0,0 +1,7 @@
> +*.c eol=lf
> +*.h eol=lf
> +*.rc eol=lf
> +*.txt eol=lf
> +*.bat eol=lf
> +*.vc*proj* eol=crlf
> +*.sln eol=crlf

Why not .rc and .bat as crlf too?

Apart from that, ACK.

Cheers
Fabian

Re: [Openvpn-devel] [PATCH] cleanup: plugin: support C++ plugin

[Fabian Knittel]

Hi Alon,

2012/4/7 Alon Bar-Lev :
> Signed-off-by: Alon Bar-Lev 
> ---
>  include/openvpn-plugin.h |    8 
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/include/openvpn-plugin.h b/include/openvpn-plugin.h
> index f82f61f..1c80eec 100644
> --- a/include/openvpn-plugin.h
> +++ b/include/openvpn-plugin.h
> @@ -43,6 +43,10 @@ typedef X509 openvpn_x509_cert_t;
>  #endif
>  #endif
>
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
>  /*
>  * Plug-in types.  These types correspond to the set of script callbacks
>  * supported by OpenVPN.
> @@ -724,4 +728,8 @@ OPENVPN_PLUGIN_DEF openvpn_plugin_handle_t 
> OPENVPN_PLUGIN_FUNC(openvpn_plugin_op
>  OPENVPN_PLUGIN_DEF int OPENVPN_PLUGIN_FUNC(openvpn_plugin_func_v1)
>      (openvpn_plugin_handle_t handle, const int type, const char *argv[], 
> const char *envp[]);
>
> +#ifdef __cplusplus
> +}
> +#endif
> +
>  #endif /* OPENVPN_PLUGIN_H_ */

ACK. (Although I haven't tried compiling a C++ plugin, the change looks fine.)

Cheers
Fabian

Re: [Openvpn-devel] [PATCH] cleanup: remove C++ comments

[Fabian Knittel]

Hi Alon,

2012/4/7 Alon Bar-Lev :
> Signed-off-by: Alon Bar-Lev 
> ---
>  src/openvpnserv/openvpnserv.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpnserv/openvpnserv.c b/src/openvpnserv/openvpnserv.c
> index a9a9441..56f5a02 100755
> --- a/src/openvpnserv/openvpnserv.c
> +++ b/src/openvpnserv/openvpnserv.c
> @@ -87,9 +87,9 @@ static HANDLE exit_event = NULL;
>  /*
>  * Message handling
>  */
> -#define M_INFO    (0)                                  // informational
> -#define M_SYSERR  (MSG_FLAGS_ERROR|MSG_FLAGS_SYS_CODE) // error + system code
> -#define M_ERR     (MSG_FLAGS_ERROR)                    // error
> +#define M_INFO    (0)                                  /* informational */
> +#define M_SYSERR  (MSG_FLAGS_ERROR|MSG_FLAGS_SYS_CODE) /* error + system 
> code */
> +#define M_ERR     (MSG_FLAGS_ERROR)                    /* error */
>
>  /* write error to event log */
>  #define MSG(flags, ...) \

ACK. Doesn't hurt to be consistent. Are those the only remaining
C++-style comments?

(Does someone know what C standard we aim at? "C++ comments" would be
just fine for C99.)

Cheers
Fabian

Re: [Openvpn-devel] [PATCH 1/6] Added support for new PolarSSL 1.1 RNG

[Fabian Knittel]

Hi Alon,

2012/4/2 Alon Bar-Lev :
> I also intend to work and cleanup the whole PolarSSL/OpenSSL mess...
>
> Design will be to introduce crypto engine callback structure,
> registering openssl and polarssl, in a way that code is using the
> callback structure while using runtime configuration one can select
> which engine to use (if both are available).

What would be the use-case for switching crypto libraries at runtime?
Would this be an important step towards the OpenVPN 3.0 concept?
(Otherwise this sounds like quite a bit of work and potentially more
processing overhead...)

Cheers
Fabian

Re: [Openvpn-devel] [DISCUSS] OpenVPN public repositories at github.com

[Fabian Knittel]

Hi,

2012/3/30 Samuli Seppänen :
>>> [*] As in "We will have the community services on our own servers. Period."

BTW: If you like self-hosting, we're experimenting with Gerrit for
code review (also provides Git repo hosting as a by-product) and it's
looking quite promising. Would formalise the ACK / NACK workflow and
allow build bots to verify a patch / patch-set before merging.

Cheers
Fabian

Re: [Openvpn-devel] [PATCH] Avoid using ~0

[Fabian Knittel]

Hi Alon,

I stumbled over a minor mistake:

2012/3/28 Alon Bar-Lev :
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index bf7af63..05ea57f 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -1367,7 +1367,7 @@ add_route (struct route *r,
>     else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_EXE)
>       {
>        netcmd_semaphore_lock ();
> -       status = openvpn_execve_check (, es, 0, "ERROR: Windows route 
> add command failed");
> +       status = openvpn_execve_check (, es, 0, "ERROR: Windows route 
> add command f(DWORD)ailed");
>        netcmd_semaphore_release ();
>       }
>     else if ((flags & ROUTE_METHOD_MASK) == ROUTE_METHOD_ADAPTIVE)

Cheers
Fabian

Re: [Openvpn-devel] 2.3alpha1 fails on OS X when the --up argument contains more than an execution path

[Fabian Knittel]

Hi Jonathan,

2012/3/28 Jonathan K. Bullard :
> There is one part of check_cmd_access() which I took from code in
> run_up_down() and I am not sure about -- the three lines:
>   gc_arena gc;
> and
>   gc = gc_new ();
> and
>   gc_free ();
>
> Are they needed? I put them in because I assume that they set up and tear
> down a global garbage-collection area where allocations are made, and that
> that area is used by argv_new() and whatever eventually allocates the argv
> structure's components when they are created by argv_printf() and/or
> parse_line().

gc_arena instances are used by explicitly passing a pointer to it. So,
unless one of the functions takes an instance of gc_arena as a
parameter, you don't need to prepare one. As many functions in OpenVPN
take one, there's some dead code scattered about that needlessly
creates and frees gc arenas (probably because uses have come and gone
over time). So you can drop gc because it's not used.

The argv family of functions apparently has its own memory management
and therefore argv_new() should be paired with an argv_reset().
Otherwise the memory allocated by argv_printf() is leaked.

Cheers
Fabian

Re: [Openvpn-devel] [PATCH 35/52] build: proper selinux detection and usage

[Fabian Knittel]

Hi Alon,

>> AC_CHECK_LIB([selinux], [setcon], [SELINUX_LIBS="-lselinux"])
>>
>> versus
>>
>> AC_CHECK_HEADER([selinux/selinux.h], [
>>    AC_CHECK_LIB([selinux], [setcon], [SELINUX_LIBS="-lselinux"],
>>        [AC_MSG_RESULT([SELinux library not found.])]
>>    )],  [AC_MSG_ERROR([SELinux headers not found.])]
>> )
>>
>> doesn't really qualify as "very complex" to me.
>
> This is untrue.
> As most features needs custom CFLAGs as well, it looks like:
>
> old_CFLAGS="${CFLAGS}"
> CFLAGS="${CFLAGS} ${SOME_CFLAGS}"
> AC_CHECK_HEADER([...])
> CFLAGS="${old_CFLAGS}"

I can see where this would be necessary if you wanted to "search
additional non-standard paths for the libraries and headers" (see my
last email), but it wouldn't be necessary for the simple case, where
you want to make sure that library and header are available and
compilable with the flags the user provided.

So it sounds as if we're actually somewhat in agreement! You don't
want complex, automatic detection of non-standard situations and I
agree wholeheartedly.
But I _do_ want basic checking to achieve clear error messages at
configuration time, not obscure build errors far later in the build
process. Rule of thumb: If "make" fails due to missing or wrong
dependencies, it's a bug in configure, because it didn't detect the
problem beforehand.

Cheers
Fabian

Re: [Openvpn-devel] New generic buildsystem: lzo enabled or disabled by default?

[Fabian Knittel]

Hi Alon,

2012/3/16 Alon Bar-Lev <alon.bar...@gmail.com>:
> On Fri, Mar 16, 2012 at 3:29 PM, Fabian Knittel
> <fabian.knit...@lettink.de> wrote:
>> In my opinion, the build defaults should reflect what the project
>> considers as the recommended defaults - the features we want to see in
>> every typical OpenVPN server and client.
>
> I strongly against that approach.
> This is a common mix up between distribution and software project.
>
> A software project provides the possibility of using the software in
> different configurations.
> A distribution is in charge of the "typical" configuration.

A typical free software project distributes the source as the primary
artefact, correct.  IMHO it also communicates a set of defaults,
especially for software that needs to interact across platforms and
architectures.  Part of communicating what those defaults are, is done
by providing a default build configuration.

A dedicated distribution project (like Gentoo or Debian) is in charge
of allowing a set of software to properly run (binary distribution) or
compile (source distribution) in combination with all the other
available software.
Whether the program is built on a 64 bit processor, where the
libraries are to be found, which SSL library is to be used, etc. are
questions a distribution may answer for you. Whether to enable
encryption or a specific compression mode would be something that the
distribution should only fiddle with in special cases (for example if
it's an embedded distribution).

I don't want to be able to connect to an OpenVPN server depending on
what distribution it was built on.


> Your comment is true for rhel packager, people expect that yum install
> openvpn will be a "typical" rhel configuration, what exactly this
> configuration is depends on distribution procedures.

OpenVPN provides a source distribution. People expect configure, make,
make install to provide a "typical" OpenVPN configuration.

> Why is lzo different than PKCS#11 or selinux? Which are "typical"?
> Mind "typical" is probably different between server and client.

It's not. I'm just saying that what we set as a default in the build
system is meaningful and should match what we document as the default
in the rest of the documentation.

> The software project should not make any assumption of its usage, nor
> enable/disable features for the sake of best practices instead of
> proper documentation.

Setting build defaults is a communication device which should be used
_in addition_ to other documentation.

Cheers
Fabian

Re: [Openvpn-devel] New generic buildsystem: lzo enabled or disabled by default?

[Fabian Knittel]

Hi,

2012/3/16 Alon Bar-Lev :
> If we do not want to do auto detection, enabling optional components
> may lead to configure failure, and force the user to explicitly
> disable this feature.
>
> It is possible, and I don't mind so much, however, I prefer configure
> to use the minimum absolutely needed components by default, and in
> majority of valid cases succeed.
>
> Disabling by default does not effect users nor building via build
> script nor building via package management (rpmbuild, deb). It only
> effects people who build manually from sources.

In my opinion, the build defaults should reflect what the project
considers as the recommended defaults - the features we want to see in
every typical OpenVPN server and client. Apart from the
Windows-binaries, the _source_ is the relevant artefact that gets
released as official "OpenVPN" and therefore the defaults in the build
system are important.

If we want to avoid confusing compile-time errors, we should present
useful and informative warnings to the user at configure-time: "Could
not find library X, if you want to compile without support for X, pass
--disable-X" or something similar. No need to hide this from the user.
(I haven't looked at the build system, so this type of error message
might already be present.)

While building a piece of software I often notice additional
dependencies based on what configure tells me. This then causes me to
install additional dev-packages / libraries before continuing
compilation. If we simply cut down the dependencies I'd probably
notice a missing feature much later, e.g. while debugging connection
problems...

The usual

  $ ./configure
  $ make
  $ make install

... should hold as few surprises as possible, IMHO.

Cheers
Fabian

Re: [Openvpn-devel] [Openvpn-users] OpenVPN 2.3-alpha1 released

[Fabian Knittel]

Hi Heiko,

Am 9. März 2012 14:42 schrieb Heiko Hund :
> Instead I plan to secure the process (and the probably the pipe handle as
> well) against malicious operations by not granting the user any sophisticated
> access to it, i.e. you can only inject code if you can write the process'
> memory. This will be enforced by the security descriptor assigned to the
> process by the service at creation time. The service account will own the
> process object, so that the user cannot sneak his way in by modifying the
> DACL.

As I'm not very familiar with the Windows nomenclature, I'm not sure
whether I've correctly understood what you're saying. Does your
approach prevent the user from injecting code into the OpenVPN
process? Or does it only prevent the user from directly accessing the
pipe? (IIUC you would need the integrity level approach to prevent the
former so I assume you're describing how the pipe handle will be
protected instead.)

Cheers
Fabian

Re: [Openvpn-devel] [Openvpn-users] OpenVPN 2.3-alpha1 released

[Fabian Knittel]

Hi Gert,

2012/2/29 Gert Doering :
> The model we follow is "openvpn.exe has the same permissions that you
> already have, so there is no benefit in manipulating anything".

That was my initial assumption, which would imply that there's no
reason to restrict access to the named pipe (apart from making sure
that whoever connects is running as a user with the needed
permissions).

If users can manipulate their openvpn session to do whatever they want
they can also manipulate what gets sent over the named pipe. (I'm not
necessarily talking about malformed messages; I'm talking about
manipulating the routing tables, etc. to contain arbitrary settings.)

> For those bits that need additional privileges, there's the named pipe
> to the openvpn service - with some very well-defined messages to
> add/delete routes, setup interfaces, and such.
>
> Part of the assumption here is "the user controls the openvpn config",
> and as such, he can make openvpn.exe run arbitrary scripts anyway - and
> to stop this from being a problem, just run openvpn.exe with your uid.

Either I'm misunderstanding Heiko's plans or you two aren't in sync
regarding this point. AFAIU, Heiko intends to safe-guard access to the
named pipe as much as possible, with the underlying assumption that
only a trusted OpenVPN process should be allowed to send somewhat
trusted commands over the pipe. In my opinion, this implies that the
openvpn config would need to be restricted to safe settings in some
way. I'm not (yet?) convinced that this approach can be secure without
crippling the type of tunnels that you can create.

Cheers
Fabian

Re: [Openvpn-devel] [Openvpn-users] OpenVPN 2.3-alpha1 released

[Fabian Knittel]

Hi Heiko,

2012/2/29 Heiko Hund <heiko.h...@sophos.com>:
> On Wednesday 29 February 2012 14:07:01 Fabian Knittel wrote:
[...]
>> (There must be something missing, otherwise
>> I don't get why you call it "interactive service" ...?)
>
> It's interactive in contrast to the other already existing service, that just
> starts all openvpn connections that it finds at the time the service itself is
> started. I internally called that service automatic. The GUI and openvpn
> interact with the interactive service, hence the name. And partially because I
> couldn't come up with something that made more sense.

Ah, I see. My confusion stemmed from the fact that I don't know
much/anything about how OpenVPN is currently used on Windows.

>> Why does the "interactive service" need to start OpenVPN? Why not let
>> the GUI start OpenVPN and let OpenVPN connect to the "interactive
>> service"?
>
> The key point here is the inheritance of the client end of the named pipe
> that's being used to request privileged operations. If there would just be a
> named pipe anyone could connect to, anyone could modify i.e. the routing
> table. Something MS tries to prevent obviously.

Ah, I see. So when you say that the "working directory, command line
options and stdin input for openpvn" are passed in, the idea is that
this MUST not allow the GUI-user to manipulate the OpenVPN process to
send arbitrary commands down the named pipe. (I initially you were
saying "pass in the path to the OpenVPN exe, but now I understand that
this is not what you meant.)

To ensure this in classic Linux this would mean that the OpenVPN
process needs to run as a _different_ user than the GUI user or else
the GUI user could freely manipulate the program using, e.g. ptrace. I
know that similar manipulations are possible in Windows, so can you
protect the service-started OpenVPN-executable from such
manipulations? (And I also assume that _named_ pipes still allow you
to hide the name from some processes of the same user?) (I'm not an
experienced Windows programmer, so please excuse my ignorance...)

>> OTOH, if you're going to start OpenVPN as a service anyway,
>> it probably doesn't really make much of a difference. Although this
>> could mean that you can keep the GUI-facing side of OpenVPN identical
>> to what it is now... the "interactive service" would just be an
>> implementation detail of how openvpn performs its privileged
>> operations.
>
> I got lost at "going to start OpenVPN as a service anyway". Openvpn isn't
> started as a service, the service starts openvpn. Openvpn is not running with
> same token the service runs, but the token of the GUI that invoked the
> service.

I was mistakenly using "run in background" and "service" as synonyms.
Anyway, you explained why who-starts-who makes all of the difference,
so this point is moot.

>> Does creating a tun/tap device belong to the operations that need
>> special privileges under windows? If so, this sounds a lot like an
>> interface that might also allow splitting off most of the system
>> specific code ... as in, this could also work on Android, no?
>
> No, that example was a spin off to my lengthy and highly fictional
> NetworkManager story. =) Essentially you're right, though. It could be used as
> such. Usually I#d say that stuff that can be setup before privileges are
> dropped should be done at that time. Setting of routes can only be done after
> privdrop and that's the main use for the new interface.

Yes, that makes sense. Hypothetically, the tun/tap-opening part would
be something the "interactive service" would do before launching the
OpenVPN executable, based on the parameters given by the GUI.

Thanks for clarifying.

Cheers
Fabian

Re: [Openvpn-devel] [Openvpn-users] OpenVPN 2.3-alpha1 released

[Fabian Knittel]

Hi Heiko,

Am 29. Februar 2012 13:18 schrieb Heiko Hund :
> [...] There will be a new service, I called it
> interactive service. The GUI/client connects to a named pipe of that service.
> It passes the working directory, command line options and stdin input for
> openpvn to the service. The service impersonates the client, creates another
> named pipe and starts openvpn. Besides the stuff from the GUI it also passes
> to client end of the created pipe to openvpn. The GUI may now connect the the
> management interface. If openvpn needs to perform a privileged operation it
> request it through the named pipe that was passed by the interactive service.
> There's only a limited and well defined set of privileged operations that are
> serviced through the pipe. Currently only setting of IPv4 and IPv6 routes, but
> that will be extended to whatever makes sense e.g. ARP table flush is the next
> thing that will come.

Let's see whether I understood the design. After initial setup, the
GUI has a connection via the mgmt interface to OpenVPN and OpenVPN has
a connection via the "privilege interface" to the "interactive
service". OpenVPN basically runs in the same context as the GUI, i.e.
without permission to change the network configuration (change routes,
etc.). The "interactive service" runs in a context with permissions to
change the network configuration. Any privileged operations are
requested by OpenVPN via the "privilege interface" and performed by
the "interactive service". (There must be something missing, otherwise
I don't get why you call it "interactive service" ...?)

Why does the "interactive service" need to start OpenVPN? Why not let
the GUI start OpenVPN and let OpenVPN connect to the "interactive
service"? OTOH, if you're going to start OpenVPN as a service anyway,
it probably doesn't really make much of a difference. Although this
could mean that you can keep the GUI-facing side of OpenVPN identical
to what it is now... the "interactive service" would just be an
implementation detail of how openvpn performs its privileged
operations.

Does creating a tun/tap device belong to the operations that need
special privileges under windows? If so, this sounds a lot like an
interface that might also allow splitting off most of the system
specific code ... as in, this could also work on Android, no?

Cheers
Fabian

Re: [Openvpn-devel] [PATCH 1/2] Added support for new PolarSSL 1.1 RNG

[Fabian Knittel]

Hi Adriaan,

I only found a minor nit:

2012/2/28 Adriaan de Jong :
> --- a/ssl.c
> +++ b/ssl.c
> @@ -385,6 +385,11 @@ init_ssl (const struct options *options, struct 
> tls_root_ctx *new_ctx)
>       tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
>     }
>
> +#ifdef USE_POLARSSL
> +  /* Fox-IT hardening: Personalise the random by mixing in the certificate */
> +  tls_ctx_personalise_random (new_ctx);
> +#endif

Unless it's intentional, the "Fox-IT hardening" string is probably
from an earlier, internal iteration?

The rest looks fine AFAICT. :)

Cheers
Fabian

Re: [Openvpn-devel] [PATCH 01/02] Add support for PolarSSL 1.1.x branch

[Fabian Knittel]

Hi Igor,

2012/2/28 Igor Novgorodov <i...@novg.net>:
> On 28.02.2012 1:37, Fabian Knittel wrote:
>> Your patch removes the code that causes havege_init() to only be
>> called once. You never want to initialise your PRNG more than once,
>> otherwise you increase the risk that your randomness is predictable.
>> So please revert that part of your patch.
>
> Yes, my fault. I didn't notice that the variable was static, so i though
> that it was local-scope only and removed the check... The fixed patch is 
> attached

Thanks!

>> ([...], although I haven't tested it and don't have any
>> experience with PolarSSL.)

Maybe Adriaan or someone else can take a quick peek and give a full-hearted ACK?

Cheers
Fabian

Re: [Openvpn-devel] [PATCH 01/02] Add support for PolarSSL 1.1.x branch

[Fabian Knittel]

Hi Igor,

2012/2/27 Igor Novgorodov :
> The attached patch adds checking for PolarSSL version on crypto_polarssl.c
> and depending on which version we are using (1.0.x or 1.1.x) chooses a new
> shiny havege_random() function, or an old ugly while{} loop hack to generate
> randomness.

Your patch removes the code that causes havege_init() to only be
called once. You never want to initialise your PRNG more than once,
otherwise you increase the risk that your randomness is predictable.
So please revert that part of your patch.

(The rest looks fine, although I haven't tested it and don't have any
experience with PolarSSL.)

Cheers
Fabian

Re: [Openvpn-devel] OpenVPN and Android 4.0 VPN API

[Fabian Knittel]

Hi Gert,

2012/2/8 Gert Doering :
> On Wed, Feb 08, 2012 at 11:27:10AM -0800, James Ring wrote:
>> Does other code within openvpn care whether the fd is a UNIX socket or
>> a tun/tap device? I'm guessing there may be some ioctls it wants to
>> perform on the device.
>
> There aren't any ioctl()s (I'm aware of) for tun/tap devices, but
> blocking/non-blocking behaviour might be an interesting problem,

"The file descriptor is put into non-blocking mode by default to avoid
blocking Java threads." [0]

>> Other than that, openvpn would be reading and
>> writing IP packets with an encrypted payload and the Java wrapper
>> would be responsible for forwarding the bytes between the UNIX domain
>> socket and the actual tun device.
>
> Don't show idea that to Jan-Just, he's already complaining that OpenVPN
> wastes too many CPU cycles...

So let's hope the fd passing I mentioned in my other mail works on Android. :)

Cheers
Fabian

0: 
http://developer.android.com/reference/android/net/VpnService.Builder.html#establish()

Re: [Openvpn-devel] OpenVPN and Android 4.0 VPN API

[Fabian Knittel]

Hi James,

2012/2/8 James Ring :
> On Wed, Feb 8, 2012 at 10:24 AM, Gert Doering  wrote:
>> Exactly.  The first three things are sort of "nearly done", the
>> "receive file descriptor to use for tun/tap" would need to be
>> implemented (tun.c, open_tun(), #ifdef ANDROID_MAGIC_VPN :-) )
>
> I was thinking about this a little more. Presumably openvpn will be
> forked and exec'd before the file descriptor is available. Presumably
> openvpn could connect to a UNIX domain socket inside open_tun() if
> ANDROID_MAGIC_VPN is specified.
>
> Does other code within openvpn care whether the fd is a UNIX socket or
> a tun/tap device? I'm guessing there may be some ioctls it wants to
> perform on the device. Other than that, openvpn would be reading and
> writing IP packets with an encrypted payload and the Java wrapper
> would be responsible for forwarding the bytes between the UNIX domain
> socket and the actual tun device.

Unless Android's Linux is stripped down in this respect, you can pass
file descriptors over UNIX domain sockets. (The first google hit is
[0]. The interface isn't beautiful, but it works nicely.)

This would allow you to take the java wrapper out of the loop as far
as the raw data shuffling is concerned.

Cheers
Fabian

0: http://www.lst.de/~okir/blackhats/node121.html

Re: [Openvpn-devel] Problem with alloc_buf_gc function

[Fabian Knittel]

Hi Tiran,

Am 11.12.2011 21:57, schrieb Tiran Kaskas:
> Looking into the function alloc_buf_gc() in file buffer.c, it returns a
> struct buffer, which seems to me is allocated on the stack, which is
> causing an issue, I believe, since the function calling alloc_buf_gc()
> will work on a buffer which becomes garbage.

'alloc_buf_gc()' is returning 'buf' (of type 'struct buffer') by value,
not by pointer, so there is no problem here.  If it were returning 'buf'
by pointer your observation would be correct.  (In C++ a typical mistake
would be to return 'buf' by reference.)

The function calling 'alloc_buf_gc()' will copy the contents of 'buf' to
its local instance of 'struct buffer' before continuing.  (An optimised
build might even eliminate the copy.)

Are you observing a specific problem with buffers or did you just happen
to stumble over this piece of code?

Cheers
Fabian



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH] Add option to disable priority tagged packets (VID=0)

[Fabian Knittel]

Hi Alon,

Am 08.12.2011 21:17, schrieb Alon Bar-Lev:
> I fail to understand why this is relevant as far as usage (openvpn --help) and
> manual (man openvpn) to document this.

Ah.  Sorry for the misunderstanding.

Are you referring to the specific patch by Michael?  I agree that it
would have needed more documentation before being acceptable for inclusion.
Or are you referring to the VLAN patch-set in general?  (BTW, the latest
version adds a few additional clarifications to the openvpn man page [0].)

Cheers
Fabian

0:
http://opensource.fsmi.uni-karlsruhe.de/gitweb/?p=openvpn.git;a=commit;h=5c5cce29f2230a8fdaf7b135d2e3255f54af1395



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] Summary of the IRC meeting (20th May 2010)

[Fabian Knittel]

Am 25.05.2010 10:42, schrieb David Sommerseth:
> Personally, I would also not enforce BSD as the only license for
> bounties.  We need to provide at least a choice, at least between GPL
> and BSD.
> 
> I would not consider to license my contributions to OpenVPN as BSD,
> because a) I want other people to be able to review my code at any
> point, no matter the circumstances the code is used, and b) If someone
> modifies/improves the code, I want these changes to be shared with the
> community.  GPL gives that possibility.

I agree with both of your points.


I assume that the monetary value of a bounty is far lower than what a
professional programmer would usually be paid.  Therefore the
bounty-work would still be (at least in part) a voluntary work.
So, defining that whoever provides the bounty also always more-or-less
owns the rights on the resulting work, looks somewhat wrong.

Allowing bounties to define the licensing details to a certain degree
would circumvent the problem.  I assume the typical bounty would be
rather low and should therefore allow the contributor to stick with the
project's license - the GNU GPL.


A few links that might of interest on the topic of bounties:

http://live.gnome.org/BountiesDiscussion lists some interesting thoughts.

http://nextsprocket.com/ looks like a relatively active site for
opensource bounties in general.

https://www.bountysource.com/ Interesting "How do bounties work" diagram
on the front page.

Cheers
Fabian



signature.asc
Description: OpenPGP digital signature

[Openvpn-devel] [PATCH] ssl.c: fix use of openvpn_run_script()'s return value

[Fabian Knittel]

This patch fixes two bugs introduced in

commit 339f2a4d4b487afa53fa99d72c35b16f31e417d3
Author: David Sommerseth <d...@users.sourceforge.net>
Date:   Thu Apr 29 23:35:45 2010 +0200

David's patch replaced openvpn_execve() with openvpn_run_script() in two places,
but didn't adjust the return value handling.  openvpn_run_script() returns true
or false, while openvpn_execve() returns the program's exit code.

Without the fix, the --tls-verify script and the --auth-user-pass-verify
script fail to run.  (I noticed the latter, but haven't actually tested the
former.)

The return value handling is fine for the other places where
openvpn_run_script() is used, because those places previously used
openvpn_execve_check() (notice the "_check" suffix).

Signed-off-by: Fabian Knittel <fabian.knit...@avona.com>
---
 ssl.c |   20 ++--
 1 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/ssl.c b/ssl.c
index a4af6a5..276322f 100644
--- a/ssl.c
+++ b/ssl.c
@@ -957,21 +957,19 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
gc_free();
 }

-  if (system_ok (ret))
+  if (ret)
{
  msg (D_HANDSHAKE, "VERIFY SCRIPT OK: depth=%d, %s",
   ctx->error_depth, subject);
}
   else
{
- if (!system_executed (ret))
-   argv_msg_prefix (M_ERR, , "Verify command failed to execute");
  msg (D_HANDSHAKE, "VERIFY SCRIPT ERROR: depth=%d, %s",
   ctx->error_depth, subject);
  goto err; /* Reject connection */
}
 }
-  
+
   /* check peer cert against CRL */
   if (opt->crl_file)
 {
@@ -3192,7 +3190,6 @@ verify_user_pass_script (struct tls_session *session, 
const struct user_pass *up
   struct gc_arena gc = gc_new ();
   struct argv argv = argv_new ();
   const char *tmp_file = "";
-  int retval;
   bool ret = false;

   /* Is username defined? */
@@ -3230,16 +3227,11 @@ verify_user_pass_script (struct tls_session *session, 
const struct user_pass *up

   /* format command line */
   argv_printf (, "%sc %s", 
session->opt->auth_user_pass_verify_script, tmp_file);
-  
+
   /* call command */
-  retval = openvpn_run_script (, session->opt->es, 0, 
"--auth-user-pass-verify");
+  ret = openvpn_run_script (, session->opt->es, 0,
+   "--auth-user-pass-verify");

-  /* test return status of command */
-  if (system_ok (retval))
-   ret = true;
-  else if (!system_executed (retval))
-   argv_msg_prefix (D_TLS_ERRORS, , "TLS Auth Error: user-pass-verify 
script failed to execute");
- 
   if (!session->opt->auth_user_pass_verify_script_via_file)
setenv_del (session->opt->es, "password");
 }
-- 
1.7.0

[Openvpn-devel] [PATCH 6/6] vlan: move htons / ntohs into vlanhdr_get/_set functions

[Fabian Knittel]

The vlanhdr_set_*() family of methods expected the passed values to be in
network byte order.  Therefore all invocations used htons() on the parameter.

The vlanhdr_get_*() family of methods returned the values in network byte
order.  Therefore all invocations used ntohs() on the return value.

The patch moves all htons() and ntohs() conversions into the vlan_hdr_*
functions to avoid the needless repetition.  The vlan_hdr_* functions now
expect and return the values in host byte order.  The callee doesn't need
to perform any kind of conversion.

Signed-off-by: Fabian Knittel <fabian.knit...@avona.com>
---
 multi.c |   14 +++---
 proto.h |   33 +++--
 2 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/multi.c b/multi.c
index 2f531c2..b77791a 100644
--- a/multi.c
+++ b/multi.c
@@ -1946,7 +1946,7 @@ buf_filter_incoming_vlan_tags (const struct buffer *buf)
   if (ntohs (vlanhdr->tpid) != OPENVPN_ETH_P_8021Q)
 return false; /* Frame is untagged.  */

-  vid = vlanhdr_get_vid(vlanhdr);
+  vid = vlanhdr_get_vid (vlanhdr);
   if (vid == 0)
 return false; /* Frame only priority-tagged.  */

@@ -2228,8 +2228,8 @@ remove_vlan_tag (const struct context *c, struct buffer 
*buf)

   /* Tagged packet. */

-  vid = ntohs (vlanhdr_get_vid ());
-  pcp = ntohs (vlanhdr_get_pcp ());
+  vid = vlanhdr_get_vid ();
+  pcp = vlanhdr_get_pcp ();

   if (c->options.vlan_accept == VAF_ONLY_UNTAGGED_OR_PRIORITY)
 {
@@ -2265,7 +2265,7 @@ remove_vlan_tag (const struct context *c, struct buffer 
*buf)
 of the header intact. */
   msg (D_VLAN_DEBUG, "removing vlan-tag from priority frame: vid: %u, 
wrapped proto/len: 0x%04x, prio: %u",
vid, ntohs (vlanhdr.proto), pcp);
-  vlanhdr_set_vid (, htons (0));
+  vlanhdr_set_vid (, 0);
   memcpy (BPTR (buf), , sizeof vlanhdr);
 }

@@ -2315,11 +2315,11 @@ multi_prepend_vlan_tag (const struct context *c, struct 
buffer *buf)
   memcpy (vlanhdr, , sizeof eth);
   vlanhdr->tpid = htons (OPENVPN_ETH_P_8021Q);
   vlanhdr->proto = eth.proto;
-  vlanhdr_set_pcp (vlanhdr, htons (0));
-  vlanhdr_set_cfi (vlanhdr, htons (0));
+  vlanhdr_set_pcp (vlanhdr, 0);
+  vlanhdr_set_cfi (vlanhdr, 0);
 }

-  vlanhdr_set_vid (vlanhdr, htons (c->options.vlan_pvid));
+  vlanhdr_set_vid (vlanhdr, c->options.vlan_pvid);

   msg (D_VLAN_DEBUG, "tagging frame: vid %u (wrapping proto/len: %04x)",
c->options.vlan_pvid, vlanhdr->proto);
diff --git a/proto.h b/proto.h
index d8e5ea7..c1bd606 100644
--- a/proto.h
+++ b/proto.h
@@ -226,52 +226,73 @@ void ipv4_packet_size_verify (const uint8_t *data,

 /*
  * Retrieve the Priority Code Point (PCP) from the IEEE 802.1Q header.
+ *
+ * @param hdr Pointer to the Ethernet header with IEEE 802.1Q tagging.
+ * @returnReturns the PCP in host byte order.
  */
 static inline uint16_t
 vlanhdr_get_pcp (const struct openvpn_8021qhdr *hdr)
 {
-  return hdr->pcp_cfi_vid & OPENVPN_8021Q_MASK_PCP;
+  return ntohs (hdr->pcp_cfi_vid & OPENVPN_8021Q_MASK_PCP);
 }
 /*
  * Retrieve the Canonical Format Indicator (CFI) from the IEEE 802.1Q header.
+ *
+ * @param hdr Pointer to the Ethernet header with IEEE 802.1Q tagging.
+ * @returnReturns the CFI in host byte order.
  */
 static inline uint16_t
 vlanhdr_get_cfi (const struct openvpn_8021qhdr *hdr)
 {
-  return hdr->pcp_cfi_vid & OPENVPN_8021Q_MASK_CFI;
+  return ntohs (hdr->pcp_cfi_vid & OPENVPN_8021Q_MASK_CFI);
 }
 /*
  * Retrieve the VLAN Identifier (VID) from the IEEE 802.1Q header.
+ *
+ * @param hdr Pointer to the Ethernet header with IEEE 802.1Q tagging.
+ * @returnReturns the VID in host byte order.
  */
 static inline uint16_t
 vlanhdr_get_vid (const struct openvpn_8021qhdr *hdr)
 {
-  return hdr->pcp_cfi_vid & OPENVPN_8021Q_MASK_VID;
+  return ntohs (hdr->pcp_cfi_vid & OPENVPN_8021Q_MASK_VID);
 }

 /*
  * Set the Priority Code Point (PCP) in an IEEE 802.1Q header.
+ *
+ * @param hdr Pointer to the Ethernet header with IEEE 802.1Q tagging.
+ * @param pcp The PCP to set (in host byte order).
  */
 static inline void
 vlanhdr_set_pcp (struct openvpn_8021qhdr *hdr, const uint16_t pcp)
 {
-  hdr->pcp_cfi_vid = (hdr->pcp_cfi_vid & ~OPENVPN_8021Q_MASK_PCP) | (pcp & 
OPENVPN_8021Q_MASK_PCP);
+  hdr->pcp_cfi_vid = (hdr->pcp_cfi_vid & ~OPENVPN_8021Q_MASK_PCP) |
+(htons (pcp) & OPENVPN_8021Q_MASK_PCP);
 }
 /*
  * Set the Canonical Format Indicator (CFI) in an IEEE 802.1Q header.
+ *
+ * @param hdr Pointer to the Ethernet header with IEEE 802.1Q tagging.
+ * @param cfi The CFI to set (in host byte order).
  */
 static inline void
 vlanhdr_set_cfi (struct openvpn_8021qhdr *hdr, const uint16_t cfi)
 {
-  hdr->pcp_cfi_vid = (hdr->pcp_cfi_vid & ~OPENVPN_8021Q_MASK_CFI) | (cfi & 
OPENVPN_8021Q_MASK_CFI);
+  hdr->pcp_cfi_vid = (hdr->pcp_cfi_vid &

[Openvpn-devel] [PATCH 2/6] vlan: use uint16_t for storage of the VID where possible

[Fabian Knittel]

This patch switches from signed to unsigned short integers for storage of
the VID.  The change attempts to clarify, that the VID can't ever be negative.

The only place where the patch doesn't use unsigned int is for the return value
of remove_vlan_tag(): The function indicates errors by returning -1.

The patch should not cause any changes in behaviour.

The improvement was suggested by Peter Stuge.

Signed-off-by: Fabian Knittel <fabian.knit...@avona.com>
---
 mroute.c  |6 +++---
 mroute.h  |6 +++---
 multi.c   |   24 
 options.c |2 +-
 options.h |2 +-
 proto.h   |   12 ++--
 6 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/mroute.c b/mroute.c
index 72ef91a..1263342 100644
--- a/mroute.c
+++ b/mroute.c
@@ -166,7 +166,7 @@ mroute_extract_addr_ipv4 (struct mroute_addr *src,

 static void mroute_copy_ether_to_addr(struct mroute_addr *maddr,
  const uint8_t *eth_addr,
- int16_t vid)
+ uint16_t vid)
 {
   maddr->type = MR_ADDR_ETHER;
   maddr->netbits = 0;
@@ -185,7 +185,7 @@ mroute_extract_addr_ether (struct mroute_addr *src,
   struct mroute_addr *esrc,
   struct mroute_addr *edest,
   const struct buffer *buf,
-  int16_t vid)
+  uint16_t vid)
 {
   unsigned int ret = 0;
   if (BLEN (buf) >= (int) sizeof (struct openvpn_ethhdr))
@@ -323,7 +323,7 @@ mroute_addr_print_ex (const struct mroute_addr *ma,
case MR_ADDR_ETHER:
  buf_printf (, "%s", format_hex_ex (ma->addr, 6, 0, 1, ":", gc)); 
 #ifdef ENABLE_VLAN_TAGGING
- buf_printf (, "@%d", *(int16_t*)(ma->addr + 6));
+ buf_printf (, "@%u", *(uint16_t*)(ma->addr + 6));
 #endif
  break;
case MR_ADDR_IPV4:
diff --git a/mroute.h b/mroute.h
index ccf79ec..95ea443 100644
--- a/mroute.h
+++ b/mroute.h
@@ -140,7 +140,7 @@ mroute_extract_addr_from_packet (struct mroute_addr *src,
 struct mroute_addr *edest,
 const struct buffer *buf,
 int tunnel_type,
-int16_t bcast_domain)
+uint16_t vid)
 {
   unsigned int mroute_extract_addr_ipv4 (struct mroute_addr *src,
 struct mroute_addr *dest,
@@ -151,13 +151,13 @@ mroute_extract_addr_from_packet (struct mroute_addr *src,
  struct mroute_addr *esrc,
  struct mroute_addr *edest,
  const struct buffer *buf,
- int16_t vid);
+ uint16_t vid);
   unsigned int ret = 0;
   verify_align_4 (buf);
   if (tunnel_type == DEV_TYPE_TUN)
 ret = mroute_extract_addr_ipv4 (src, dest, buf);
   else if (tunnel_type == DEV_TYPE_TAP)
-ret = mroute_extract_addr_ether (src, dest, esrc, edest, buf, 
bcast_domain);
+ret = mroute_extract_addr_ether (src, dest, esrc, edest, buf, vid);
   return ret;
 }

diff --git a/multi.c b/multi.c
index dfd23a3..1a8bbbf 100644
--- a/multi.c
+++ b/multi.c
@@ -1761,7 +1761,7 @@ multi_bcast (struct multi_context *m,
 const struct buffer *buf,
 const struct multi_instance *sender_instance,
 const struct mroute_addr *sender_addr,
-int16_t vid)
+uint16_t vid)
 {
   struct hash_iterator hi;
   struct hash_element *he;
@@ -1934,10 +1934,10 @@ buf_filter_incoming_vlan_tags (const struct buffer *buf)

   if (ntohs (vlanhdr->tpid) == OPENVPN_ETH_P_8021Q)
 {
- const int16_t vid = vlanhdr_get_vid(vlanhdr);
+ const uint16_t vid = vlanhdr_get_vid(vlanhdr);
  if (vid != 0)
{
- msg (D_VLAN_DEBUG, "dropping tagged incoming frame, vid: %d", 
vid);
+ msg (D_VLAN_DEBUG, "dropping tagged incoming frame, vid: %u", 
vid);
  return true;
}
}
@@ -2063,9 +2063,9 @@ multi_process_incoming_link (struct multi_context *m, 
struct multi_instance *ins
  else if (TUNNEL_TYPE (m->top.c1.tuntap) == DEV_TYPE_TAP)
{
 #ifdef ENABLE_VLAN_TAGGING
- int16_t vid = 0;
+ uint16_t vid = 0;
 #else
- const int16_t vid = 0;
+ const uint16_t vid = 0;
 #endif
 #ifdef ENABLE_PF
  struct mroute_addr edest;
@@ -2185,8 +2185,8 @@ remove_vlan_tag (const struct context *c, struct buffer 
*buf)
 {
   struct openvpn_ethhdr eth;
   struct openvpn_8021qhdr vlanhdr;
-  int16_t vid;
-  int16_t pcp;
+  uint16_t vid;
+  uint16_t pcp;

   if (BLEN (buf) < (sizeof (struct openvpn_8021qhdr)))
 goto drop;
@@ -2206,7 

[Openvpn-devel] [PATCH 4/6] vlan: Improve documentation of remove_vlan_tag()

[Fabian Knittel]

This patch improves the comments that document the remove_vlan_tag() function.
It clarifies what is meant by "priority-tagged frames" and documents the
parameters and return value.

Signed-off-by: Fabian Knittel <fabian.knit...@avona.com>
---
 multi.c |9 +++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/multi.c b/multi.c
index 83c5aa3..2f531c2 100644
--- a/multi.c
+++ b/multi.c
@@ -2175,8 +2175,9 @@ multi_process_incoming_link (struct multi_context *m, 
struct multi_instance *ins
 #ifdef ENABLE_VLAN_TAGGING
 /*
  * For vlan_accept == VAF_ONLY_UNTAGGED_OR_PRIORITY:
- *   If a frame is VLAN-tagged, it is dropped.  Otherwise, the global
- *   vlan_pvid is returned as VID.
+ *   Only untagged frames and frames that are priority-tagged (VID == 0) are
+ *   accepted.  (This means that VLAN-tagged frames are dropped.)  For frames
+ *   that aren't dropped, the global vlan_pvid is returned as VID.
  *
  * For vlan_accept == VAF_ONLY_VLAN_TAGGED:
  *   If a frame is VLAN-tagged and contains no priority information, the
@@ -2188,6 +2189,10 @@ multi_process_incoming_link (struct multi_context *m, 
struct multi_instance *ins
  * For vlan_accept == VAF_ALL:
  *   Accepts both VLAN-tagged and untagged (or priority-tagged) frames and
  *   and handles them as described above.
+ *
+ * @param c   The global context.
+ * @param buf The ethernet frame.
+ * @returnReturns -1 if the frame is dropped or the VID if it is accepted.
  */
 static int16_t
 remove_vlan_tag (const struct context *c, struct buffer *buf)
-- 
1.7.0

[Openvpn-devel] [PATCH 5/6] vlan: is_ipv4: fix position of variable definition

[Fabian Knittel]

To avoid difficulties on older / stricter C compilers, this patch moves the
variable definition to the beginning of the block in is_ipv4().

Found by Peter Stuge.

Signed-off-by: Fabian Knittel <fabian.knit...@avona.com>
---
 proto.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/proto.c b/proto.c
index 64486de..5a9d3a6 100644
--- a/proto.c
+++ b/proto.c
@@ -54,10 +54,10 @@ is_ipv4 (int tunnel_type, struct buffer *buf)
return false;
   eh = (const struct openvpn_ethhdr *) BPTR (buf);
   if (ntohs (eh->proto) == OPENVPN_ETH_P_8021Q) {
+const struct openvpn_8021qhdr *evh;
 if (BLEN (buf) < (int)(sizeof (struct openvpn_8021qhdr)
+ sizeof (struct openvpn_iphdr)))
  return false;
-const struct openvpn_8021qhdr *evh;
 evh = (const struct openvpn_8021qhdr *) BPTR (buf);
 if (ntohs (evh->proto) != OPENVPN_ETH_P_IPV4)
   return false;
-- 
1.7.0

[Openvpn-devel] [PATCH 1/6] vlan: Mention 802.1Q in configuration descriptions

[Fabian Knittel]

To clarify that the tagging/untagging is based on the 802.1Q standard, this
patch adds "802.1Q-based" to a few descriptive strings.

The improvement was suggested by Peter Stuge.

Signed-off-by: Fabian Knittel <fabian.knit...@avona.com>
---
 configure.ac |4 ++--
 options.c|2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index b99bc11..89e3d30 100644
--- a/configure.ac
+++ b/configure.ac
@@ -213,7 +213,7 @@ AC_ARG_ENABLE(selinux,
 )

 AC_ARG_ENABLE(vlan-tagging,
-   [  --disable-vlan-tagging  Disable support for VLAN tagging/untagging],
+   [  --disable-vlan-tagging  Disable support for 802.1Q-based VLAN tagging],
[VLAN_TAGGING="$enableval"],
[VLAN_TAGGING="yes"]
 )
@@ -894,7 +894,7 @@ fi

 dnl enable --vlan-tagging
 if test "$VLAN_TAGGING" = "yes"; then
-   AC_DEFINE(ENABLE_VLAN_TAGGING, 1, [Enable VLAN tagging/untagging])
+   AC_DEFINE(ENABLE_VLAN_TAGGING, 1, [Enable 802.1Q-based VLAN 
tagging/untagging])
 fi


diff --git a/options.c b/options.c
index bf8b3a1..2d47f20 100644
--- a/options.c
+++ b/options.c
@@ -422,7 +422,7 @@ static const char usage_message[] =
   "  to a web server at host:port.\n"
 #endif
 #ifdef ENABLE_VLAN_TAGGING
-  "--vlan-tagging  : Enable VLAN tagging.\n"
+  "--vlan-tagging  : Enable 802.1Q-based VLAN tagging.\n"
   "--vlan-accept tagged|untagged|all : Set VLAN tagging mode. Default is 
'all'.\n"
   "--vlan-pvid v   : Sets the Port VLAN Identifier. Defaults to 1.\n"
 #endif
-- 
1.7.0

[Openvpn-devel] [PATCH 3/6] vlan: slightly clean-up buf_filter_incoming_vlan_tags()

[Fabian Knittel]

This patch changes buf_filter_incoming_vlan_tags() to use a less nested
code-style.  It also improves documentation of the function.

In addition, the function is made static, as it is only used locally.

Code based on a snippet by Peter Stuge.

Signed-off-by: Fabian Knittel <fabian.knit...@avona.com>
---
 multi.c |   39 ---
 1 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/multi.c b/multi.c
index 1a8bbbf..83c5aa3 100644
--- a/multi.c
+++ b/multi.c
@@ -1925,24 +1925,33 @@ multi_process_post (struct multi_context *m, struct 
multi_instance *mi, const un
 }

 #ifdef ENABLE_VLAN_TAGGING
-bool
+/*
+ * Decides whether or not to drop an ethernet frame.  VLAN-tagged frames are
+ * dropped.  All other frames are accepted.
+ *
+ * @param buf The ethernet frame.
+ * @returnReturns true if the frame should be dropped, false otherwise.
+ */
+static bool
 buf_filter_incoming_vlan_tags (const struct buffer *buf)
 {
-  if (BLEN (buf) >= (int) sizeof (struct openvpn_8021qhdr))
-{
-  const struct openvpn_8021qhdr *vlanhdr = (const struct openvpn_8021qhdr 
*) BPTR (buf);
+  const struct openvpn_8021qhdr *vlanhdr;
+  uint16_t vid;

-  if (ntohs (vlanhdr->tpid) == OPENVPN_ETH_P_8021Q)
-{
- const uint16_t vid = vlanhdr_get_vid(vlanhdr);
- if (vid != 0)
-   {
- msg (D_VLAN_DEBUG, "dropping tagged incoming frame, vid: %u", 
vid);
- return true;
-   }
-   }
-}
-  return false;
+  if (BLEN (buf) < (int) sizeof (struct openvpn_8021qhdr))
+return false; /* Frame too small.  */
+
+  vlanhdr = (const struct openvpn_8021qhdr *) BPTR (buf);
+
+  if (ntohs (vlanhdr->tpid) != OPENVPN_ETH_P_8021Q)
+return false; /* Frame is untagged.  */
+
+  vid = vlanhdr_get_vid(vlanhdr);
+  if (vid == 0)
+return false; /* Frame only priority-tagged.  */
+
+  msg (D_VLAN_DEBUG, "dropping VLAN-tagged incoming frame, vid: %u", vid);
+  return true;
 }
 #endif

-- 
1.7.0

Re: [Openvpn-devel] [PULL-REQUEST v3] VLAN-Tagging

[Fabian Knittel]

Hi David,

David Sommerseth wrote:
> I've finally found some time to dig into this again.  After some
> consideration, I decided to rebase your work on your feat_vlan_tagging
> branch against the openvpn-testing.git feat_vlan_tagging branch.
> 
> This means that your earlier patches without signed-off-by tags are not
   ^^^--- with?
> merged in.  I am fine with that, as I've become stricter on those tags
> later on.
> 
> The alternative is to scratch the feat_vlan_tagging branch now in
> openvpn-testing.git and re-establish it on your feat_vlan branch, which
> has those tags all from the beginning.
> 
> So I will leave it up to you now how you want it.  But in the moment I
> this branch gets merged into allmerged, its too late to change your
> opinion.  I will wait for your reply on which approach you would like.

If I may choose freely, I'd prefer the fresh patch-set that has the
proper signed-of-by lines.  But I'm fine with both approaches.

I'll do all future changes incrementally so that this decision doesn't
come up again. :)

> When this is settled, the only missing thing is to get someone who can
> understand the code path being changed in your feature branch a bit
> better than me to give an official ACK.  When I get that ACK, it goes
> into allmerged.

Sounds great!

Cheers
Fabian



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCHv2 1/3] Harden create_temp_filename() (version 2)

[Fabian Knittel]

Hi David,

David Sommerseth wrote:
> +}
> +  while (attempts < 6);
>  
> -  return gen_path (directory, BSTR (), gc);
> +  msg (M_FATAL, "Failed to create temporary file after %i attempts", 
> attempts);
> +  return NULL;
>  }

I noticed something else ... if - hypothetically - someone manages to
guess our file names 5 times in a row, we abort the OpenVPN process.
Maybe that's a bit drastic?

If I understand M_FATAL correctly, msg() doesn't even return.  So
effectively, the create_temp_file() function never returns NULL, because
all error cases are currently fatal.

As you've apparently prepared the code for the NULL case in your 2/3
patch, I would suggest a non-fatal error code for at least the last case.

Cheers
Fabian

Re: [Openvpn-devel] [PATCHv2 2/3] Renamed all calls to create_temp_filename()

[Fabian Knittel]

Hi David,

David Sommerseth wrote:
> diff --git a/multi.c b/multi.c
> index 2b04428..826a113 100644
> --- a/multi.c
> +++ b/multi.c
> @@ -1530,7 +1530,13 @@ multi_connection_established (struct multi_context *m, 
> struct multi_instance *mi
>if (plugin_defined (mi->context.plugins, 
> OPENVPN_PLUGIN_CLIENT_CONNECT))
>   {
> struct argv argv = argv_new ();
> -   const char *dc_file = create_temp_filename 
> (mi->context.options.tmp_dir, "cc", );
> +   const char *dc_file = create_temp_file (mi->context.options.tmp_dir, 
> "cc", );
> +
> +  if( !dc_file ) {
> +cc_succeeded = false;
> +goto script_depr_failed;
> +  }
> +
> argv_printf (, "%s", dc_file);
> delete_file (dc_file);
> if (plugin_call (mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT, 
> , NULL, mi->context.c2.es) != OPENVPN_PLUGIN_FUNC_SUCCESS)

You'll probably want to remove the remaining delete_file, right? :)

[...]
> diff --git a/ssl.c b/ssl.c
> index ddd5ee7..552bcbe 100644
> --- a/ssl.c
> +++ b/ssl.c
> @@ -1094,10 +1094,11 @@ key_state_gen_auth_control_file (struct key_state 
> *ks, const struct tls_options
>const char *acf;
>  
>key_state_rm_auth_control_file (ks);
> -  acf = create_temp_filename (opt->tmp_dir, "acf", );
> -  ks->auth_control_file = string_alloc (acf, NULL);
> -  setenv_str (opt->es, "auth_control_file", ks->auth_control_file);
> -
> +  acf = create_temp_file (opt->tmp_dir, "acf", );
> +  if( acf ) {
> +ks->auth_control_file = string_alloc (acf, NULL);
> +setenv_str (opt->es, "auth_control_file", ks->auth_control_file);
> +  } /* FIXME: Should have better error handling? */
>gc_free ();   
>  }
>  

I just noticed your FIXME. With the above change, the client auth plugin
would have the chance to notice that there's no "auth_control_file"
environment variable and error out. It was already logged that the temp
file creation failed, so it should be clear to the admin why it happened.

An improvement might be to ensure that verify_user_pass_plugin()
disallows the plugin from doing OPENVPN_PLUGIN_FUNC_DEFERRED if there's
no auth_control_file. (The function could simply check the return value
and force FAILURE.)

Cheers
Fabian

Re: [Openvpn-devel] [PATCH] Harden create_temp_filename()

[Fabian Knittel]

Hi David,

David Sommerseth wrote:
> On 16/04/10 11:35, Gert Doering wrote:
>> Hi,
> 
>> On Fri, Apr 16, 2010 at 11:16:32AM +0200, David Sommerseth wrote:
>>> I'll look more into this, as the only advantage is that if open() with
>>> O_EXCL|O_CREAT fails if the file exists, it should be used instead.
>> Unfortunately, this won't help against symlink attacks directed to
>> non-existant files (like "-> /etc/nologin").  
> 
> That's right, this could create a local DoS.  I'm going to have a more
> careful look at test_file() afterwards.  Considering to make it use
> stat() instead of just trying to open the file for reading.

As Davide already pointed out, the problem is taken care of by open().
So ... there should be no need for additional checking after a
successful open() with O_CREAT|O_EXCL.

(BTW, I thought creat() took a flags parameter, but it only takes a mode
param. My mistake. So you're correct in wanting to use open() instead of
creat().)

To let the code speak, here's my suggestion (not compile-tested):

> do
>   {
> uint8_t rndbytes[16];
> const char *rndstr;
> 
> ++attempts;
> mutex_lock_static (L_CREATE_TEMP);
> ++counter;
> mutex_unlock_static (L_CREATE_TEMP);
> 
> prng_bytes (rndbytes, sizeof rndbytes);
> rndstr = format_hex_ex (rndbytes, sizeof rndbytes, 40, 0, NULL, gc);
> buf_printf (, PACKAGE "_%s_%s.tmp", prefix, rndstr);
> 
> retfname = gen_path (directory, BSTR (), gc);
> if (!retfname)
>   {
> msg (M_FATAL, "Failed to create temporary filename and path");
> return NULL;
>   }
> 
> /* Atomically create the file.  Errors out if the file already
>exists.  */
> fd = open (retfname, O_CREAT | O_EXCL | O_WRONLY, S_IRUSR | S_IWUSR);
> if (fd != -1)
>   {
> close (fd);
> return retfname;
>   }
> else if (fd == -1 && errno != EEXIST)
>   {
> /* Something else went wrong, no need to retry.  */
> struct gc_arena gcerr = gc_new ();
> msg (M_FATAL, "Could not create temporary file '%s': %s",
>  retfname, strerror_ts (errno, ));
> gc_free ();
> return NULL;
>   }
>   }
> while (attempts < 6);
> 
> msg (M_FATAL, "Failed to create temporary file after %i attempts", attempts);
> return NULL;

(Regarding the code style: The above uses what I had thought would be
the current style, but as I've only read parts of the OpenVPN code, I'm
likely to be wrong. Are there any code-parts / newer code-parts which
should be considered reference material?)

> I've dived into the kernel code to see what it *really* does (when the
> man page are so unclear), and it should behave as those other Unices
> does as well.  So, O_EXCL do make sense to avoid overwriting existing
> files if it is a symlink to an existing file.

IMHO the Linux man-page ist quite clear (I'm using version 3.24):

>O_EXCL Ensure that this call creates the file: if this flag  is  speci‐
>   fied  in  conjunction with O_CREAT, and pathname already exists,
>   then open() will fail.  The behavior of O_EXCL is  undefined  if
>   O_CREAT is not specified.
> 
>   When  these two flags are specified, symbolic links are not fol‐
>   lowed: if pathname is a symbolic link, then open() fails regard‐
>   less of where the symbolic link points to.
> 
>   O_EXCL  is  only  supported  on NFS when using NFSv3 or later on
>   kernel 2.6 or later.  In environments where NFS  O_EXCL  support
>   is not provided, programs that rely on it for performing locking
>   tasks will contain a race  condition.   Portable  programs  that
>   want  to  perform atomic file locking using a lockfile, and need
>   to avoid reliance on NFS support for O_EXCL, can create a unique
>   file  on  the same file system (e.g., incorporating hostname and
>   PID), and use link(2) to  make  a  link  to  the  lockfile.   If
>   link(2)  returns  0,  the  lock  is  successful.  Otherwise, use
>   stat(2) on the unique file  to  check  if  its  link  count  has
>   increased to 2, in which case the lock is also successful.

Using open with O_CREAT|O_EXCL:
  - makes the atomic guarantee, that the file was created by us (or
the function call fails)
  - does not follow symlinks (if a symlink exists, it fails)
  - works over NFS for NFS implementations that support O_EXCL. (This
should apply to all current Linux NFS implementations.)

So, anything remaining that you'd like cleared up?


> Btw ... When diving into the kernel code, I stumbled upon this comment
> in fs/namei.c:1872:
> 
>   /* Does someone understand code flow here? Or it is only
>* me so stupid? Anathema to whoever designed this non-sense
>* with "intent.open".
>*/
> 
> Thought that one was worth sharing ;-)

:-D

Re: [Openvpn-devel] [PATCH] Harden create_temp_filename()

[Fabian Knittel]

Hi David,

David Sommerseth schrieb:
> As promised in the meeting today, a patch for hardening
> create_temp_filename().

Great! :)

> I've added more checks to what create_temp_filename() returns where it
> is called in addition, to make it even safer.

> +  do {
>  uint8_t rndbytes[16];
>  const char *rndstr;
>  
> +attempts++;
> +mutex_lock_static (L_CREATE_TEMP);
> +++counter;
> +mutex_unlock_static (L_CREATE_TEMP);
> +
>  prng_bytes (rndbytes, sizeof (rndbytes));
>  rndstr = format_hex_ex (rndbytes, sizeof (rndbytes), 40, 0, NULL, gc);
>  buf_printf (, PACKAGE "_%s_%s.tmp", prefix, rndstr);
> +
> +retfname = gen_path (directory, BSTR (), gc);
> +if( !retfname ) {
> +  msg (M_FATAL, "Failed to create temporary filename and path");
> +  return NULL;
> +}
> +  } while ( test_file (retfname) && (attempts < 6) );
> +
> +  if( attempts > 5 ) {
> +msg (M_FATAL, "Failed to create temporary file after %i attempts", 
> attempts);
> +return NULL;
> +  }
> +
> +  /* Create the tempfile to avoid a race condition */
> +  fd = creat (retfname, S_IRUSR | S_IWUSR);
> +  if( fd == -1 ) {

You'll probably want to add O_EXCL to creat()'s mode flags:

|O_EXCL Ensure that this call creates the file: if this flag  is
 speci‐
|   fied  in  conjunction with O_CREAT, and pathname already
exists,
|   then open() will fail.  The behavior of O_EXCL is
undefined  if
|   O_CREAT is not specified.

Otherwise, there would be a race-condition between test_file() and
creat() and someone could create a symlink under you feet.

And as the important "does it exist?" test thereby moves to creat(), I
would like to suggest that you move the creat() call into the loop. The
 exit condition would then be "successful creat()" or "max attempts".

Cheers
Fabian

PS: Is there some kind of code style guide for OpenVPN? I notice that
you're using a different style than I expected to see...



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH 4/9] vlan: Prepend and remove VLAN identifiers on outgoing and incoming frames

[Fabian Knittel]

[ I just noticed that I accidentally sent this only to David and not to
  the list. It was written and sent on Thu, 01 Apr 2010 15:46:21 +0200 ]

David Sommerseth wrote:
> But what kind traffic does hit the OpenVPN clients?  Does the OpenVPN
> server send only traffic to the corresponding VLAN the OpenVPN client is
> "assigned" to?  Or does it receive packages for all the VLAN's and does
> the "filtering" on the client side?

The client is unmodified and everything is handled on the server-side.
VLAN handling is transparent for the clients.  They only see packets
belonging to their VLAN and packets sent by them are implicitly assigned
to the VLAN they belong to.

> From a security and not the least from a performance perspective, the
> OpenVPN clients should only receive traffic which hits it's own VLAN
> (ie. the server does the "filtering" before sending data to the client).

I agree.  It also has the nice side-effect for us, that clients don't
need to be upgraded. :)

>  I'm not sure if I saw this in code or not ... but if it is in place and
> somebody could point me to the patch which does it, I would be happy.

I attempt to achieve this in patches 5 and 6. Patch 5 takes care of
unicast traffic and patch 6 takes care of broadcast traffic.


As far I understand it, packets are generally routed based on struct
mroute_addr.  As soon as a client sends a packet to the openvpn server,
the packet's sending address is learned and stored as an mroute_addr.
If a bit-wise comparison of a destination mroute_addr and learned
client's mroute_addr match, the packet is routed to that client.

In tap mode, the addresses of interest are the MAC addresses.  In patch
5 I extend what is stored in the mroute_addr: Instead of only storing
the 6 bytes of the MAC address, I also append the 2 bytes of the VID.
So now a destination MAC address of one VLAN will never match the
destination MAC address of another VLAN, because the mroute_addr fields
never match.

For broadcasting, no mroute_addr comparisons are done, so the change
from patch 5 doesn't help.  For a broadcast, the packet is simply
forwarded to each of the connected clients (unless PF prevents it).
Patch 6 adds a check to the broadcast loop to check whether the sender's
vid and the intended broadcast recipient have a matching vid.  So we now
selectively broadcast based on matching vid.


As I'm not fully familiar with OpenVPN's code base, so I can't be
absolutely sure that patch 5 and 6 catch all cases, but it's what I've
found to be relevant during my tests and code reviews.  I agree with
Peter, that this part is critical for security and should be thoroughly
reviewed.

Cheers
Fabian





signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [RFC][PATCH 0/9] VLAN tagging on TAP devices in OpenVPN server mode

[Fabian Knittel]

David Sommerseth schrieb:
> If you have a public git tree available, I could pull that as well.  (I
> tried the git URL the webUI gave me yesterday, without luck).

Ah, thanks for noticing ... the non-ssh path apparently doesn't get used
often. I forgot to fix the URLs since switching to gitosis. I've now
verified that

  git://fsmi-dev.fsmi.uni-karlsruhe.de/openvpn.git

definitely works. Anyway, I'll mail again when there's something new to
pull.

Cheers
Fabian



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [RFC][PATCH 0/9] VLAN tagging on TAP devices in OpenVPN server mode

[Fabian Knittel]

David Sommerseth schrieb:
> On 01/04/10 10:32, Fabian Knittel wrote:
>> We'll definitely be doing that over here.  My main concern was whether
>> we would have to patch OpenVPN indefinitely with local enhancements or
>> whether there was a chance to include it upstream.  And now that things
>> look quite positive, we can go forward with the chosen approach. :)
> 
> Thanks a lot!  This really will help us a lot.  If you even dare to try
> a local merge against the 'allmerged' branch when you do some bigger
> tests, that would also be very much appreciated.

OK. I assume feat_passtos isn't merged into allmerged yet, right?  I'll
probably pluck Davide Guerri's patch from feat_passtos and do the
testing together with my vlan patches on allmerged. (Assuming things
don't explode too badly on allmerged :).)

> Btw!  How is it with IPv6 and your patches?  We have quite some patches
> for that in the tree already.  And as I see you patch in the [PATCH 1/9]
> the is_ipv4() function, you might want to be sure this will also work in
> the IPv6 world as well.

AFAICS, IPv6 support for tap should only come into the picture when we
attempt to inspect the packets for specific purposes, like packet
filtering or retrieving the packet's priority (TOS in IPv4).  So
although the patches don't yet explicitly handle IPv6, I don't expect
them to conflict with it either...

I'll have a look when I merge to allmerged.  And we'll be testing
autoconfigured IPv6 addresses over tap in our setup, so we'll hopefully
catch obvious problems.


>> Sounds great!  I'll definitely continue polishing the patch-set and
>> continue hitting the vlan code with more tests.
> 
> Cool!  Please let me know if you patches will go on top of the current
> patch-set you sent, or if you come with a new patch-set based upon the
> feat_passtos branch.

I'd be fine with whatever you would prefer ... If you have no
preferences, I'd probably continue rebasing my internal branch on
feat_passtos until all comments have been worked in and we've completed
our basic tests.  But I can also use a more incremental approach, if
that simplifies your reviewing work or you'd like the existing code to
be visible on openvpn-testing.git.

Cheers
Fabian



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH 4/9] vlan: Prepend and remove VLAN identifiers on outgoing and incoming frames

[Fabian Knittel]

Peter Stuge schrieb:
> Jan Just Keijser wrote:
>> FYI: 802.1Q defines VLAN 1 as the 'native' LAN: all packets on VLAN 1 
>> are *by definition* not encapsulated (according to my CCNA guide ;-))
[...]
>> Perhaps we need to make sure that VID 1 means untagged ...
> 
> Any VID can be untagged. While 1 is the default it can change and
> OpenVPN shouldn't really care.
> 
> One alternative approach to using tag 0 would be to introduce a
> vlan-pvid (or vlan-default-tag) option to set the PVID.

So packets coming in on the tap device that aren't tagged would be
assumed to have a vid == PVID.  And packets going out on the tap device
with a vid == PVID would go out untagged.  (A vid of 0 would continue to
be rejected as configuration option.)
Not specifying --vlan-pvid would mean that only tagged packets are
accepted (and sent).

I'm still unsure what to do with incoming frames from clients who's vid
matches the pvid and where the frames contain a full 802.1Q header with
a non-zero vid.  I'll probably just drop those packets.  Maybe we should
drop such packets regardless of the PVID value while in --vlan-tagging
mode.   (Tags in tags are apparently specified by 802.1ad and we don't
support that anyway.)

> But explicitly allowing tag 0 can also be useful.

What would be the use-case?  Packets with tag 0 are priority packets and
we currently completely ignore / drop the priority values.  We could of
course add an option to specify whether the priority part of 802.1Q
packets should be preserved.

Instead of removing the 802.1Q packets when untagging, we would instead
change the vid to 0 and leave the priority fields untouched.  We'd
probably want an option separate from the --vlan-tag option to specify
that though ...

And would this be a global setting or per-client? Per-client could make
the broadcasting code a tad bit more difficult, as the packets would
need to be modified for each client. (Stripping or not stripping the
802.1Q headers...)

Cheers
Fabian



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH 4/9] vlan: Prepend and remove VLAN identifiers on outgoing and incoming frames

[Fabian Knittel]

Fabian Knittel schrieb:
> Peter Stuge schrieb:
>> It would be nice to be able to use VID 0 to mean untagged packets.
> 
> Hm, nice idea.  I'll implement it in my next round of patches.

I've just noticed a detail that might warrant discussion.  To make sure
we're talking about the same thing, this is how I intended --vlan-tag 0
to be handled:

Packets coming in from the tap device:
 If the packet has no 802.1Q header or the 802.1Q header's VID field is
 0, the vid is assumed to be 0 and the packet will be forwarded to all
 clients who's --vlan-tag is set to 0.

Packets outgoing via the tap device:
 Packets originating from clients who's --vlan-tag is set to 0 are
 passed on to the tap device without change, i.e. no 802.1Q header is
 attached or removed.  A potential 802.1Q header already present within
 the packet is left untouched.  This would allow the Priority Code Point
 (PCP) field to be used.

Now, to the slight difficulty: What happens with outgoing frames from
clients who's --vlan-tag is set to 0, where the frame contains a 802.1Q
header with VID field != 0?  Pushed onto a network which uses vlan tags,
this would allow a client to inject packets into a different network.
(For --vlan-tag != 0 this is no problem, because we unconditionally wrap
the packets in 802.1Q headers with a new VID.)

Possible solutions:

a) Inspect outgoing packets for --vlan-tag == 0 clients.  Drop packets
   that have a VID != 0.
b) Always wrap outgoing packets with 802.1Q headers and set the VID to
   0. (This would make the code-patch uniform for all --vlan-tag
   values.)

I think I slightly prefer solution a), as it allows the PCP field to be
used and doesn't add needless overhead to outgoing packets.

Cheers
Fabian



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [RFC][PATCH 0/9] VLAN tagging on TAP devices in OpenVPN server mode

[Fabian Knittel]

Hi David,

David Sommerseth schrieb:
> Thank you very much for your patches!  I'll look into them soon.

Thanks!

> The
> patches seems to apply nicely against the feat_passtos branch.  I was
> worried about a conflict here, until I noticed where you had your roots :)

Actually, I was lucky to notice the openvpn-unstable.git repo before
submission.  My original patch-set was based on the subversion repo
branch beta21 and would have conflicted with feat_passtos.  I didn't
notice the git repo and the Wiki on secure-computing.net until I
happened to look at some openvpn-devel mails in the archive.  Does
openvpn.net link to them from somewhere?


> One thing, it would be good if you would do your commit with -s
> (--signoff).  We've not been strict about this so far, but I would like
> to see those sign-off messages.  (I'll make sure the developers
> documentation is 'up-to-date' in this matter, as I don't think that's
> mentioned now).

Will do.

>> Another question would be whether I should turn the feature into a 
>> compile-time
>> selectable option.
> 
> It's been several discussions about if such features should be #ifdef'ed
> or not.  The general consensus of the discussions is that it will most
> probably be accepted into a stable tree in the future quicker if it is a
> compile-time enablement.
> 
> The main argument which I find acceptable, is that if a stability and/or
> security issue is found, it would be possible to easily disable all
> features and disable them one-by-one to find the offending feature.

I'll add the necessary #ifdefs and such.


> If you could do a bit more testing, also some stress/performance testing
> with several VLAN's being tested in parallel, that would be beneficial.

We'll definitely be doing that over here.  My main concern was whether
we would have to patch OpenVPN indefinitely with local enhancements or
whether there was a chance to include it upstream.  And now that things
look quite positive, we can go forward with the chosen approach. :)

> Having all this said, the feature itself seems reasonable for me to
> include into OpenVPN, so the missing step is just to mature the code to
> be sure we don't cause any regression.  And here some stress/performance
> testing will be helpful.  You scare at least me when stating that this
> code "was originally only intended as a proof of concept", which is why
> I'm not signing off these patches immediately and giving you a feature
> branch.  But I'm open for full inclusion!

Sounds great!  I'll definitely continue polishing the patch-set and
continue hitting the vlan code with more tests.


> And keep us updated on the progress with your patches!

Will do. :)

Cheers
Fabian





signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH 4/9] vlan: Prepend and remove VLAN identifiers on outgoing and incoming frames

[Fabian Knittel]

Peter Stuge schrieb:
> Fabian Knittel wrote:
>> +  if (ntohs (vlanhdr.tpid) != OPENVPN_ETH_P_8021Q)
>> +{
>> +  /* Drop untagged frames */
>> +  goto err;
>> +}
> 
> It would be nice to be able to use VID 0 to mean untagged packets.

Hm, nice idea.  I'll implement it in my next round of patches.

Cheers
Fabian

Re: [Openvpn-devel] [PATCH 3/9] vlan: Add per-client --vlan-tag option

[Fabian Knittel]

Hi Peter,

Peter Stuge schrieb:
> Fabian Knittel wrote:
>> +#define OPENVPN_8021Q_MAX_VID 0xFFFE
> 
> The max VID in 802.1q is 4095 = 0xfff.

You are absolutely correct.  Thanks for catching that.  I intended it to
say 0xFFE, because the standard talks about VID values being valid
within the range 0 through 4094 [1].  But OPENVPN_8021Q_MASK_VID should
definitely be defined as 0xFFF, as the bit field is 12 bits.

VID == 0xFFF is "Reserved for implementation use." and "shall not be
configured as PVID [...] or transmitted in a tag header." [2]  But I'm
not sure whether it's used in reality and if you'd like that value to be
accepted, I have no objections.

Thanks again for reviewing!

Cheers
Fabian

1: IEEE Std 8021.Q-2005, "9.6 VLAN Tag Control Information", p.76
2: IEEE Std 8021.Q-2005, Table 9-2, p.76

[Openvpn-devel] [PATCH 2/9] vlan: Add global --vlan-tagging option

[Fabian Knittel]

This patch adds the new "--vlan-tagging" boolean option.  The option is valid
in server mode.  It is off by default.

The flag indicates whether openvpn should assume the tap device
to be in tagged mode, i.e. packets coming in on the device are tagged via
IEEE 802.1Q and packets going out through the device should be tagged likewise.

The option has no immediate effect yet, but will be used by later patches.
---
 options.c |   13 +
 options.h |2 ++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/options.c b/options.c
index 36b9913..506fb49 100644
--- a/options.c
+++ b/options.c
@@ -651,6 +651,8 @@ static const char usage_message[] =
   "--show-pkcs11-ids provider [cert_private] : Show PKCS#11 available ids.\n" 
   "--verb option can be added 
*BEFORE* this.\n"
 #endif /* ENABLE_PKCS11 */
+  "\n"
+  "--vlan-tagging  : Enable VLAN tagging/untagging to/from TAP device.\n"
  ;

 #endif /* !ENABLE_SMALL */
@@ -1175,6 +1177,8 @@ show_settings (const struct options *o)
   SHOW_BOOL (ifconfig_noexec);
   SHOW_BOOL (ifconfig_nowarn);

+  SHOW_BOOL (vlan_tagging);
+
 #ifdef HAVE_GETTIMEOFDAY
   SHOW_INT (shaper);
 #endif
@@ -1742,6 +1746,8 @@ options_postprocess_verify_ce (const struct options 
*options, const struct conne

if ((options->ssl_flags & SSLF_NO_NAME_REMAPPING) && script_method == 
SM_SYSTEM)
  msg (M_USAGE, "--script-security method='system' cannot be combined 
with --no-name-remapping");
+  if (options->vlan_tagging && dev != DEV_TYPE_TAP)
+   msg (M_USAGE, "--vlan-tagging only works with --dev tap");
 }
   else
 {
@@ -1788,6 +1794,8 @@ options_postprocess_verify_ce (const struct options 
*options, const struct conne
   if (options->port_share_host || options->port_share_port)
msg (M_USAGE, "--port-share requires TCP server mode (--mode server 
--proto tcp-server)");
 #endif
+  if (options->vlan_tagging)
+   msg (M_USAGE, "--vlan-tagging requires --mode server");

 }
 #endif /* P2MP_SERVER */
@@ -5730,6 +5738,11 @@ add_option (struct options *options,
   options->persist_mode = 1;
 }
 #endif
+  else if (streq (p[0], "vlan-tagging"))
+{
+  VERIFY_PERMISSION (OPT_P_GENERAL);
+  options->vlan_tagging = true;
+}
   else
 {
   if (file)
diff --git a/options.h b/options.h
index 740e18e..49fa596 100644
--- a/options.h
+++ b/options.h
@@ -509,6 +509,8 @@ struct options
   bool show_net_up;
   int route_method;
 #endif
+
+  bool vlan_tagging;
 };

 #define streq(x, y) (!strcmp((x), (y)))
-- 
1.7.0

[Openvpn-devel] [PATCH 3/9] vlan: Add per-client --vlan-tag option

[Fabian Knittel]

This patch adds the new "--vlan-tag" integer option.  The option is valid
in server mode and can be set in a client context (e.g. from the client-connect
hook).  It defaults to 0.

The value indicates which VID (VLAN identifier) to associate with a client.
The client will only receive packets which belong to the same VLAN.  Packets
going out via the tap devie will be marked as belonging to the indicated VID.

The option has no immediate effect yet, but will be used by later patches.
---
 options.c |   25 +++--
 options.h |1 +
 proto.h   |4 
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/options.c b/options.c
index 506fb49..aaf92f0 100644
--- a/options.c
+++ b/options.c
@@ -1178,6 +1178,7 @@ show_settings (const struct options *o)
   SHOW_BOOL (ifconfig_nowarn);

   SHOW_BOOL (vlan_tagging);
+  SHOW_INT (vlan_tag);

 #ifdef HAVE_GETTIMEOFDAY
   SHOW_INT (shaper);
@@ -1748,6 +1749,8 @@ options_postprocess_verify_ce (const struct options 
*options, const struct conne
  msg (M_USAGE, "--script-security method='system' cannot be combined 
with --no-name-remapping");
   if (options->vlan_tagging && dev != DEV_TYPE_TAP)
msg (M_USAGE, "--vlan-tagging only works with --dev tap");
+  if (!options->vlan_tagging && options->vlan_tag)
+   msg (M_USAGE, "--vlan-tag must be used with activated --vlan-tagging");
 }
   else
 {
@@ -1794,8 +1797,8 @@ options_postprocess_verify_ce (const struct options 
*options, const struct conne
   if (options->port_share_host || options->port_share_port)
msg (M_USAGE, "--port-share requires TCP server mode (--mode server 
--proto tcp-server)");
 #endif
-  if (options->vlan_tagging)
-   msg (M_USAGE, "--vlan-tagging requires --mode server");
+  if (options->vlan_tagging || options->vlan_tag)
+   msg (M_USAGE, "--vlan-tagging/--vlan-tag requires --mode server");

 }
 #endif /* P2MP_SERVER */
@@ -5743,6 +5746,24 @@ add_option (struct options *options,
   VERIFY_PERMISSION (OPT_P_GENERAL);
   options->vlan_tagging = true;
 }
+  else if (streq (p[0], "vlan-tag"))
+{
+  VERIFY_PERMISSION (OPT_P_INSTANCE);
+  if (p[1])
+   {
+ options->vlan_tag = positive_atoi (p[1]);
+ if (options->vlan_tag < OPENVPN_8021Q_MIN_VID || options->vlan_tag > 
OPENVPN_8021Q_MAX_VID)
+   {
+ msg (msglevel, "the parameter of --vlan-tag parameters must be >= 
%d and <= %d", OPENVPN_8021Q_MIN_VID, OPENVPN_8021Q_MAX_VID);
+ goto err;
+   }
+   }
+  else
+   {
+ msg (msglevel, "error parsing --vlan-tag parameters");
+ goto err;
+   }
+}
   else
 {
   if (file)
diff --git a/options.h b/options.h
index 49fa596..f4ca502 100644
--- a/options.h
+++ b/options.h
@@ -511,6 +511,7 @@ struct options
 #endif

   bool vlan_tagging;
+  int vlan_tag;
 };

 #define streq(x, y) (!strcmp((x), (y)))
diff --git a/proto.h b/proto.h
index 628e991..f26cbc0 100644
--- a/proto.h
+++ b/proto.h
@@ -211,4 +211,8 @@ void ipv4_packet_size_verify (const uint8_t *data,
  counter_type *errors);
 #endif

+
+#define OPENVPN_8021Q_MIN_VID 1
+#define OPENVPN_8021Q_MAX_VID 0xFFFE
+
 #endif
-- 
1.7.0

[Openvpn-devel] [PATCH 4/9] vlan: Prepend and remove VLAN identifiers on outgoing and incoming frames

[Fabian Knittel]

This patch adds parsing of the IEEE 802.1Q headers for incoming and outgoing
ethernet frames.

For frames coming in from the tap interface, the 802.1Q header is parsed and
translated into a regular Ethernet II header.  Note that the Priority Code
Point (PCP) and Canonical Format Indicator (CFI) fields contained within the
802.1Q header are ignored.  Only the VLAN Identifier (VID) is used.

For frames going out over the tap interface, the Ethernet II header is
replaced with a 802.1Q-based header.  PCP and CFI are permanently set to 0.
The VID is set according to the instance's vlan_tag value (also see patch
adding the "--vlan-tag" option).
---
 multi.c |   73 +++
 multi.h |5 
 proto.h |   34 +
 3 files changed, 107 insertions(+), 5 deletions(-)

diff --git a/multi.c b/multi.c
index 342871a..c2563a0 100644
--- a/multi.c
+++ b/multi.c
@@ -2117,6 +2117,73 @@ multi_process_incoming_link (struct multi_context *m, 
struct multi_instance *ins
 }

 /*
+ * Removes the VLAN tagging of a frame and returns the frame's VID.  Frames
+ * without tagging are dropped.
+ */
+static int16_t
+remove_vlan_identifier (struct buffer *buf)
+{
+  struct openvpn_ethhdr eth;
+  struct openvpn_8021qhdr vlanhdr;
+  int16_t vid;
+
+  if (BLEN (buf) < (sizeof (struct openvpn_8021qhdr)))
+goto err;
+
+  vlanhdr = *(const struct openvpn_8021qhdr *) BPTR (buf);
+
+  if (ntohs (vlanhdr.tpid) != OPENVPN_ETH_P_8021Q)
+{
+  /* Drop untagged frames */
+  goto err;
+}
+
+  /* Ethernet II frame with 802.1Q */
+  vid = ntohs (vlan_get_vid ());
+  memcpy (, , sizeof (eth));
+  eth.proto = vlanhdr.proto;
+
+  buf_advance (buf, SIZE_ETH_TO_8021Q_HDR);
+  memcpy (BPTR (buf), , sizeof eth);
+
+  return vid;
+err:
+  /* Drop the frame. */
+  buf->len = 0;
+  return -1;
+}
+
+/*
+ * Adds VLAN tagging to a frame.  Short frames that can't be tagged are
+ * dropped.
+ */
+void
+multi_prepend_vlan_identifier (struct multi_instance *mi, struct buffer *buf)
+{
+  struct openvpn_ethhdr eth;
+  struct openvpn_8021qhdr vlanhdr;
+  uint8_t *p;
+
+  /* Frame too small or not enough head room for VLAN tag? */
+  if ((BLEN (buf) < (int) sizeof (struct openvpn_ethhdr)) ||
+  (buf_reverse_capacity (buf) < SIZE_ETH_TO_8021Q_HDR))
+{
+  /* Drop the frame. */
+  buf->len = 0;
+  return;
+}
+
+  eth = *(const struct openvpn_ethhdr *) BPTR (buf);
+  memcpy (, , sizeof eth);
+  vlanhdr.tpid = htons (OPENVPN_ETH_P_8021Q);
+  vlanhdr.proto = eth.proto;
+  vlan_set_vid (, htons (mi->context.options.vlan_tag));
+
+  ASSERT (p = buf_prepend (buf, SIZE_ETH_TO_8021Q_HDR));
+  memcpy (p, , sizeof vlanhdr);
+}
+
+/*
  * Process packets in the TUN/TAP interface -> TCP/UDP socket direction,
  * i.e. server -> client direction.
  */
@@ -2158,6 +2225,12 @@ multi_process_incoming_tun (struct multi_context *m, 
const unsigned int mpp_flag
* the appropriate multi_instance object.
*/

+  if (dev_type == DEV_TYPE_TAP && m->top.options.vlan_tagging)
+{
+ if (remove_vlan_identifier (>top.c2.buf) == -1)
+   return false;
+}
+
   mroute_flags = mroute_extract_addr_from_packet (,
  ,
 #ifdef ENABLE_PF
diff --git a/multi.h b/multi.h
index ad26c12..26b3e1a 100644
--- a/multi.h
+++ b/multi.h
@@ -405,6 +405,7 @@ multi_get_timeout (struct multi_context *m, struct timeval 
*dest)
 static inline bool
 multi_process_outgoing_tun (struct multi_context *m, const unsigned int 
mpp_flags)
 {
+  void multi_prepend_vlan_identifier (struct multi_instance *mi, struct buffer 
*buf);
   struct multi_instance *mi = m->pending;
   bool ret = true;

@@ -415,6 +416,10 @@ multi_process_outgoing_tun (struct multi_context *m, const 
unsigned int mpp_flag
  mi->context.c2.to_tun.len);
 #endif
   set_prefix (mi);
+  if (mi->context.options.vlan_tag)
+{
+  multi_prepend_vlan_identifier (mi, >context.c2.to_tun);
+}
   process_outgoing_tun (>context);
   ret = multi_process_post (m, mi, mpp_flags);
   clear_prefix ();
diff --git a/proto.h b/proto.h
index f26cbc0..671331f 100644
--- a/proto.h
+++ b/proto.h
@@ -61,20 +61,26 @@ struct openvpn_ethhdr
 # define OPENVPN_ETH_P_IPV4   0x0800  /* IPv4 protocol */
 # define OPENVPN_ETH_P_IPV6   0x86DD  /* IPv6 protocol */
 # define OPENVPN_ETH_P_ARP0x0806  /* ARP protocol */
+# define OPENVPN_ETH_P_8021Q  0x8100  /* 802.1Q protocol */
   uint16_t proto; /* packet type ID field */
 };

-# define OPENVPN_ETH_P_8021Q  0x8100  /* 802.1Q protocol */
-
 struct openvpn_8021qhdr
 {
   uint8_t dest[OPENVPN_ETH_ALEN]; /* destination ethernet addr */
   uint8_t source[OPENVPN_ETH_ALEN];   /* source ethernet addr  */

-  uint32_t tag;   /* packet 802.1Q Vlan Tag */
-  uint16_t proto; /* packet type ID field */
+  uint16_t tpid;  /* 802.1Q Tag 

[Openvpn-devel] [PATCH 8/9] vlan: add debug logging to tagging / untagging

[Fabian Knittel]

---
 multi.c |6 ++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/multi.c b/multi.c
index 822ae29..661fd98 100644
--- a/multi.c
+++ b/multi.c
@@ -2143,11 +2143,15 @@ remove_vlan_identifier (struct buffer *buf)
   if (ntohs (vlanhdr.tpid) != OPENVPN_ETH_P_8021Q)
 {
   /* Drop untagged frames */
+  msg (M_INFO, "dropping untagged ethernet frame (proto/len 0x%04x)",
+  ntohs (vlanhdr.tpid));
   goto err;
 }

   /* Ethernet II frame with 802.1Q */
   vid = ntohs (vlan_get_vid ());
+  msg (M_INFO, "untagging ethernet frame: vid: %u, wrapped proto/len: 0x%04x",
+   vid, ntohs (vlanhdr.proto));
   memcpy (, , sizeof (eth));
   eth.proto = vlanhdr.proto;

@@ -2189,6 +2193,8 @@ multi_prepend_vlan_identifier (struct multi_instance *mi, 
struct buffer *buf)

   ASSERT (p = buf_prepend (buf, SIZE_ETH_TO_8021Q_HDR));
   memcpy (p, , sizeof vlanhdr);
+
+  msg (M_INFO, "tagging frame with vid %u, type is %04x", 
mi->context.options.vlan_tag, vlanhdr.proto);
 }

 /*
-- 
1.7.0

[Openvpn-devel] [PATCH 7/9] vlan: Slightly enhance PF's protocol inspection of 802.1Q packets

[Fabian Knittel]

To allow openvpn's PF code to inspect IP packets contained within 802.1Q
packets, this patch enhances mroute_extract_addr_ether() to properly
skip over the 802.1Q header.
---
 mroute.c |   10 +-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/mroute.c b/mroute.c
index 1040b8f..4aa7bb4 100644
--- a/mroute.c
+++ b/mroute.c
@@ -205,7 +205,15 @@ mroute_extract_addr_ether (struct mroute_addr *src,
  struct buffer b = *buf;
  if (buf_advance (, sizeof (struct openvpn_ethhdr)))
{
- switch (ntohs (eth->proto))
+ uint16_t proto = ntohs (eth->proto);
+ if (proto == OPENVPN_ETH_P_8021Q)
+   {
+ const struct openvpn_8021qhdr *tag = (const struct 
openvpn_8021qhdr *) BPTR (buf);
+ proto = ntohs (tag->proto);
+ buf_advance (, SIZE_ETH_TO_8021Q_HDR);
+   }
+
+ switch (proto)
{
case OPENVPN_ETH_P_IPV4:
  ret |= (mroute_extract_addr_ipv4 (esrc, edest, ) << 
MROUTE_SEC_SHIFT);
-- 
1.7.0

[Openvpn-devel] [PATCH 6/9] vlan: Restrict broadcasts to the sender's VLAN.

[Fabian Knittel]

This patch enhances openvpn's internal packet routing to restrict broadcast
packets to destinations with a matching VID.

I.e. broadcasts from client to client or from tap interface to clients are now
filtered based on whether the client belongs to the correct VLAN id.
---
 multi.c |   15 +--
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/multi.c b/multi.c
index 96644e5..822ae29 100644
--- a/multi.c
+++ b/multi.c
@@ -1759,7 +1759,8 @@ static void
 multi_bcast (struct multi_context *m,
 const struct buffer *buf,
 const struct multi_instance *sender_instance,
-const struct mroute_addr *sender_addr)
+const struct mroute_addr *sender_addr,
+int16_t vid)
 {
   struct hash_iterator hi;
   struct hash_element *he;
@@ -1804,6 +1805,8 @@ multi_bcast (struct multi_context *m,
}
}
 #endif
+ if (vid != 0 && vid != mi->context.options.vlan_tag)
+   continue;
  multi_add_mbuf (m, mi, mb);
}
}
@@ -1997,7 +2000,7 @@ multi_process_incoming_link (struct multi_context *m, 
struct multi_instance *ins
  if (mroute_flags & MROUTE_EXTRACT_MCAST)
{
  /* for now, treat multicast as broadcast */
- multi_bcast (m, >c2.to_tun, m->pending, NULL);
+ multi_bcast (m, >c2.to_tun, m->pending, NULL, 0);
}
  else /* possible client to client routing */
{
@@ -2063,7 +2066,7 @@ multi_process_incoming_link (struct multi_context *m, 
struct multi_instance *ins
{
  if (mroute_flags & 
(MROUTE_EXTRACT_BCAST|MROUTE_EXTRACT_MCAST))
{
- multi_bcast (m, >c2.to_tun, m->pending, NULL);
+ multi_bcast (m, >c2.to_tun, m->pending, NULL, 
vid);
}
  else /* try client-to-client routing */
{
@@ -2258,9 +2261,9 @@ multi_process_incoming_tun (struct multi_context *m, 
const unsigned int mpp_flag
{
  /* for now, treat multicast as broadcast */
 #ifdef ENABLE_PF
- multi_bcast (m, >top.c2.buf, NULL, e2);
+ multi_bcast (m, >top.c2.buf, NULL, e2, vid);
 #else
- multi_bcast (m, >top.c2.buf, NULL, NULL);
+ multi_bcast (m, >top.c2.buf, NULL, NULL, vid);
 #endif
}
  else
@@ -2429,7 +2432,7 @@ gremlin_flood_clients (struct multi_context *m)
ASSERT (buf_write_u8 (, get_random () & 0xFF));

   for (i = 0; i < parm.n_packets; ++i)
-   multi_bcast (m, , NULL, NULL);
+   multi_bcast (m, , NULL, NULL, 0);

   gc_free ();
 }
-- 
1.7.0

[Openvpn-devel] [PATCH 9/9] vlan: add debug logging to broadcast filter

[Fabian Knittel]

---
 multi.c |   16 +++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/multi.c b/multi.c
index 661fd98..641d464 100644
--- a/multi.c
+++ b/multi.c
@@ -1806,7 +1806,21 @@ multi_bcast (struct multi_context *m,
}
 #endif
  if (vid != 0 && vid != mi->context.options.vlan_tag)
-   continue;
+   {
+ struct gc_arena gc = gc_new ();
+ msg (M_INFO, "VLAN: addr[%s]@%d -> client[%s]@%d packet 
dropped by BCAST VLAN filter",
+ mroute_addr_print_ex (sender_addr, MAPF_SHOW_ARP, ),
+ vid, mi_prefix (mi), mi->context.options.vlan_tag);
+ gc_free ();
+ continue;
+   }
+   {
+ struct gc_arena gc = gc_new ();
+ msg (M_INFO, "BCAST: addr[%s]@%d -> client[%s]@%d",
+ mroute_addr_print_ex (sender_addr, MAPF_SHOW_ARP, ),
+ vid, mi_prefix (mi), mi->context.options.vlan_tag);
+ gc_free ();
+   }
  multi_add_mbuf (m, mi, mb);
}
}
-- 
1.7.0

[Openvpn-devel] [PATCH 5/9] vlan: Add VLAN identifier to mroute_addr for ethernet addresses

[Fabian Knittel]

This patch appends the VID to the ethernet address in mroute_addr.

By including the VID in mroute_addr, the routing space is divided by VLAN.
This means:
 - duplicate MAC addresses on different VLANs no longer conflict and
 - all unicast-traffic is constrained to whatever VLAN the traffic came
   from (i.e. cross-VLAN unicast routing is no longer possible).

(This patch doesn't take care of broadcast traffic.  Broadcast traffic across
VLANs is still be possible at this point.)
---
 mroute.c |   10 +++---
 mroute.h |8 +---
 multi.c  |   15 +++
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/mroute.c b/mroute.c
index 9d8fa66..1040b8f 100644
--- a/mroute.c
+++ b/mroute.c
@@ -169,7 +169,8 @@ mroute_extract_addr_ether (struct mroute_addr *src,
   struct mroute_addr *dest,
   struct mroute_addr *esrc,
   struct mroute_addr *edest,
-  const struct buffer *buf)
+  const struct buffer *buf,
+  int16_t vid)
 {
   unsigned int ret = 0;
   if (BLEN (buf) >= (int) sizeof (struct openvpn_ethhdr))
@@ -179,15 +180,17 @@ mroute_extract_addr_ether (struct mroute_addr *src,
{
  src->type = MR_ADDR_ETHER;
  src->netbits = 0;
- src->len = 6;
+ src->len = 8;
  memcpy (src->addr, eth->source, 6);
+ memcpy (src->addr + 6, , 2);
}
   if (dest)
{
  dest->type = MR_ADDR_ETHER;
  dest->netbits = 0;
- dest->len = 6;
+ dest->len = 8;
  memcpy (dest->addr, eth->dest, 6);
+ memcpy (dest->addr + 6, , 2);

  /* ethernet broadcast/multicast packet? */
  if (is_mac_mcast_addr (eth->dest))
@@ -303,6 +306,7 @@ mroute_addr_print_ex (const struct mroute_addr *ma,
{
case MR_ADDR_ETHER:
  buf_printf (, "%s", format_hex_ex (ma->addr, 6, 0, 1, ":", gc)); 
+ buf_printf (, "@%d", *(int16_t*)(ma->addr + 6));
  break;
case MR_ADDR_IPV4:
  {
diff --git a/mroute.h b/mroute.h
index 9b9087d..ccf79ec 100644
--- a/mroute.h
+++ b/mroute.h
@@ -139,7 +139,8 @@ mroute_extract_addr_from_packet (struct mroute_addr *src,
 struct mroute_addr *esrc,
 struct mroute_addr *edest,
 const struct buffer *buf,
-int tunnel_type)
+int tunnel_type,
+int16_t bcast_domain)
 {
   unsigned int mroute_extract_addr_ipv4 (struct mroute_addr *src,
 struct mroute_addr *dest,
@@ -149,13 +150,14 @@ mroute_extract_addr_from_packet (struct mroute_addr *src,
  struct mroute_addr *dest,
  struct mroute_addr *esrc,
  struct mroute_addr *edest,
- const struct buffer *buf);
+ const struct buffer *buf,
+ int16_t vid);
   unsigned int ret = 0;
   verify_align_4 (buf);
   if (tunnel_type == DEV_TYPE_TUN)
 ret = mroute_extract_addr_ipv4 (src, dest, buf);
   else if (tunnel_type == DEV_TYPE_TAP)
-ret = mroute_extract_addr_ether (src, dest, esrc, edest, buf);
+ret = mroute_extract_addr_ether (src, dest, esrc, edest, buf, 
bcast_domain);
   return ret;
 }

diff --git a/multi.c b/multi.c
index c2563a0..96644e5 100644
--- a/multi.c
+++ b/multi.c
@@ -1975,7 +1975,8 @@ multi_process_incoming_link (struct multi_context *m, 
struct multi_instance *ins
  NULL,
  NULL,
  >c2.to_tun,
- DEV_TYPE_TUN);
+ DEV_TYPE_TUN,
+ 0);

  /* drop packet if extract failed */
  if (!(mroute_flags & MROUTE_EXTRACT_SUCCEEDED))
@@ -2033,10 +2034,13 @@ multi_process_incoming_link (struct multi_context *m, 
struct multi_instance *ins
}
  else if (TUNNEL_TYPE (m->top.c1.tuntap) == DEV_TYPE_TAP)
{
+ int16_t vid = 0;
 #ifdef ENABLE_PF
  struct mroute_addr edest;
  mroute_addr_reset ();
 #endif
+ if (m->top.options.vlan_tagging)
+   vid = c->options.vlan_tag;
  /* extract packet source and dest addresses */
  mroute_flags = mroute_extract_addr_from_packet (,
  ,
@@ -2047,7 +2051,8 @@ multi_process_incoming_link (struct multi_context 

[Openvpn-devel] [PATCH 1/9] is_ipv4(): add packet length check for 802.1Q packets

[Fabian Knittel]

This patch adds an additional length check to is_ipv4().

Currently is_ipv4() only checks whether the frame is large enough for struct
openvpn_ethhdr. In case of an 802.1Q packet the function now also checks
whether the frame is large enough for struct openvpn_8021qhdr, which is 4 bytes
larger than struct openvpn_ethhdr.
---
 proto.c |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/proto.c b/proto.c
index 1f582ce..64486de 100644
--- a/proto.c
+++ b/proto.c
@@ -54,6 +54,9 @@ is_ipv4 (int tunnel_type, struct buffer *buf)
return false;
   eh = (const struct openvpn_ethhdr *) BPTR (buf);
   if (ntohs (eh->proto) == OPENVPN_ETH_P_8021Q) {
+if (BLEN (buf) < (int)(sizeof (struct openvpn_8021qhdr)
+   + sizeof (struct openvpn_iphdr)))
+ return false;
 const struct openvpn_8021qhdr *evh;
 evh = (const struct openvpn_8021qhdr *) BPTR (buf);
 if (ntohs (evh->proto) != OPENVPN_ETH_P_IPV4)
-- 
1.7.0