Re: [Openvpn-devel] [PATCH v2] Send push reply right after async auth complete

[David Sommerseth]

On 07/10/15 15:32, Lev Stipakov wrote:
> v2:
> More careful inotify_watchers handling
> * Ensure that same multi_instance is added only once
> * Ensure that multi_instance is always removed
> 
> v1:
> This feature speeds up connection establishment in cases when async
> authentication result is not ready when first push request arrives. At
> the moment server sends push reply only when it receives next push
> request, which comes 5 seconds later.
> 
> Implementation overview.
> 
> Add new configure option ENABLE_ASYNC_PUSH, which can be enabled if
> system supports inotify.
> 
> Add inotify descriptor to an event loop. Add inotify watch for a
> authentication control file. Store mapping between watch descriptor and
> multi_instance in a dictionary. When file is closed, inotify fires an
> event and we continue with connection establishment - call client-
> connect etc and send push reply.
> 
> Inotify watch descriptor got automatically deleted after file is closed
> or when file is removed. We catch that event and remove it from the
> dictionary.
> 
> Feature is easily tested with sample "defer" plugin and following settings:
> 
> auth-user-pass-optional
> setenv test_deferred_auth 3
> plugin simple.so
> 
> Signed-off-by: Lev Stipakov 
> ---
>  configure.ac  |  15 +
>  src/openvpn/forward.c |   8 +++
>  src/openvpn/mtcp.c|  28 ++
>  src/openvpn/mudp.c|  27 +
>  src/openvpn/multi.c   | 152 
> +-
>  src/openvpn/multi.h   |  14 +
>  src/openvpn/openvpn.h |  11 
>  src/openvpn/push.c|  69 +--
>  src/openvpn/push.h|   2 +
>  9 files changed, 295 insertions(+), 31 deletions(-)
> 

[...snip...]

> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 7a5d383..134905c 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -1371,6 +1371,9 @@ io_wait_dowork (struct context *c, const unsigned int 
> flags)
>  #ifdef ENABLE_MANAGEMENT
>static int management_shift = 6; /* depends on MANAGEMENT_READ and 
> MANAGEMENT_WRITE */
>  #endif
> +#ifdef ENABLE_ASYNC_PUSH
> +  static int file_shift = 8;
> +#endif

Can we please have a comment on what this 'file_shift' value means?  Just a
single line comment.  Perhaps 'file_change' would be a better name?

[...snip...]

> diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
> index 57118f8..30ec345 100644
> --- a/src/openvpn/mudp.c
> +++ b/src/openvpn/mudp.c
> @@ -38,6 +38,10 @@
>  
>  #include "memdbg.h"
>  
> +#ifdef ENABLE_ASYNC_PUSH
> +#include 
> +#endif
> +

Maybe HAVE_SYS_INOTIFY_H is better?

[...snip...]

> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 902c4dc..0da9ca7 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -28,6 +28,11 @@
>  #include "config-msvc.h"
>  #endif
>  
> +#ifdef ENABLE_ASYNC_PUSH
> +#include 
> +#define INOTIFY_EVENT_BUFFER_SIZE 16384
> +#endif
> +

Maybe HAVE_SYS_INOTIFY_H is better here too?

>  #include "syshead.h"
>  
>  #if P2MP_SERVER
> @@ -243,6 +248,20 @@ cid_compare_function (const void *key1, const void *key2)
>  
>  #endif
>  
> +#ifdef ENABLE_ASYNC_PUSH
> +static uint32_t
> +int_hash_function (const void *key, uint32_t iv)
> +{
> +  return (unsigned long)key;
> +}
> 
This looks very odd, but I understand it's required by hash_init().  Could you
add a little remark about why this function looks so "useless"?

[...snip...]

> @@ -1877,6 +1932,14 @@ multi_connection_established (struct multi_context *m, 
> struct multi_instance *mi
>  
> /* set context-level authentication flag */
> mi->context.c2.context_auth = CAS_SUCCEEDED;
> +
> +#ifdef ENABLE_ASYNC_PUSH
> +   /* authentication complete, send push reply */
> +   if (mi->context.c2.push_request_received)
> + {
> +   process_incoming_push_request(>context);
> + }
> +#endif
>   }
>else
>   {
> @@ -1906,6 +1969,54 @@ multi_connection_established (struct multi_context *m, 
> struct multi_instance *mi
>mi->context.c2.push_reply_deferred = false;
>  }
>  
> +#ifdef ENABLE_ASYNC_PUSH
> +void
> +multi_process_file_closed (struct multi_context *m, const unsigned int 
> mpp_flags)

It would be great to see a more verbose doxygen comment here, explaining what
this function do, why and who calls it in which situations.

> +{
> +  char buffer[INOTIFY_EVENT_BUFFER_SIZE];
> +  size_t buffer_i = 0;
> +  int r = read (m->top.c2.inotify_fd, buffer, INOTIFY_EVENT_BUFFER_SIZE);
> +
> +  while (buffer_i < r)
> +{
> +  /* parse inotify events */
> +  struct inotify_event *pevent = (struct inotify_event *) 
> [buffer_i];
> +  size_t event_size = sizeof (struct inotify_event) + pevent->len;
> +  buffer_i += event_size;
> +
> +  msg(D_MULTI_DEBUG, "MULTI: modified fd %d, mask %d", pevent->wd, 
> pevent->mask);
> +
> +  struct multi_instance* mi = hash_lookup(m->inotify_watchers, (void*) 
> (unsigned long) 

[Openvpn-devel] [PATCH v2] Send push reply right after async auth complete

[Lev Stipakov]

v2:
More careful inotify_watchers handling
* Ensure that same multi_instance is added only once
* Ensure that multi_instance is always removed

v1:
This feature speeds up connection establishment in cases when async
authentication result is not ready when first push request arrives. At
the moment server sends push reply only when it receives next push
request, which comes 5 seconds later.

Implementation overview.

Add new configure option ENABLE_ASYNC_PUSH, which can be enabled if
system supports inotify.

Add inotify descriptor to an event loop. Add inotify watch for a
authentication control file. Store mapping between watch descriptor and
multi_instance in a dictionary. When file is closed, inotify fires an
event and we continue with connection establishment - call client-
connect etc and send push reply.

Inotify watch descriptor got automatically deleted after file is closed
or when file is removed. We catch that event and remove it from the
dictionary.

Feature is easily tested with sample "defer" plugin and following settings:

auth-user-pass-optional
setenv test_deferred_auth 3
plugin simple.so

Signed-off-by: Lev Stipakov 
---
 configure.ac  |  15 +
 src/openvpn/forward.c |   8 +++
 src/openvpn/mtcp.c|  28 ++
 src/openvpn/mudp.c|  27 +
 src/openvpn/multi.c   | 152 +-
 src/openvpn/multi.h   |  14 +
 src/openvpn/openvpn.h |  11 
 src/openvpn/push.c|  69 +--
 src/openvpn/push.h|   2 +
 9 files changed, 295 insertions(+), 31 deletions(-)

diff --git a/configure.ac b/configure.ac
index 2e651d8..32620c6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -277,6 +277,13 @@ AC_ARG_ENABLE(
[enable_systemd="no"]
 )

+AC_ARG_ENABLE(
+   [async-push],
+   [AS_HELP_STRING([--enable-async-push], [enable async-push support 
@<:@default=no@:>@])],
+   [enable_async_push="yes"],
+   [enable_async_push="no"]
+)
+
 AC_ARG_WITH(
[special-build],
[AS_HELP_STRING([--with-special-build=STRING], [specify special build 
string])],
@@ -1201,6 +1208,14 @@ if test "${enable_plugin_auth_pam}" = "yes"; then
fi
 fi

+if test "${enable_async_push}" = "yes"; then
+   AC_CHECK_HEADERS(
+   [sys/inotify.h],
+   AC_DEFINE([ENABLE_ASYNC_PUSH], [1], [Enable async push]),
+   AC_MSG_ERROR([inotify.h not found.])
+   )
+fi
+
 CONFIGURE_DEFINES="`set | grep '^enable_.*=' ; set | grep '^with_.*='`"
 AC_DEFINE_UNQUOTED([CONFIGURE_DEFINES], ["`echo ${CONFIGURE_DEFINES}`"], 
[Configuration settings])

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 7a5d383..134905c 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1371,6 +1371,9 @@ io_wait_dowork (struct context *c, const unsigned int 
flags)
 #ifdef ENABLE_MANAGEMENT
   static int management_shift = 6; /* depends on MANAGEMENT_READ and 
MANAGEMENT_WRITE */
 #endif
+#ifdef ENABLE_ASYNC_PUSH
+  static int file_shift = 8;
+#endif

   /*
* Decide what kind of events we want to wait for.
@@ -1465,6 +1468,11 @@ io_wait_dowork (struct context *c, const unsigned int 
flags)
 management_socket_set (management, c->c2.event_set, 
(void*)_shift, NULL);
 #endif

+#ifdef ENABLE_ASYNC_PUSH
+  /* arm inotify watcher */
+  event_ctl (c->c2.event_set, c->c2.inotify_fd, EVENT_READ, 
(void*)_shift);
+#endif
+
   /*
* Possible scenarios:
*  (1) tcp/udp port has data available to read
diff --git a/src/openvpn/mtcp.c b/src/openvpn/mtcp.c
index dc15f09..b27c5eb 100644
--- a/src/openvpn/mtcp.c
+++ b/src/openvpn/mtcp.c
@@ -62,6 +62,10 @@
 # define MTCP_MANAGEMENT ((void*)4)
 #endif

+#ifdef ENABLE_ASYNC_PUSH
+#define MTCP_FILE_CLOSE_WRITE ((void*)5)
+#endif
+
 #define MTCP_N   ((void*)16) /* upper bound on MTCP_x */

 struct ta_iow_flags
@@ -245,6 +249,12 @@ multi_tcp_wait (const struct context *c,
   if (management)
 management_socket_set (management, mtcp->es, MTCP_MANAGEMENT, 
>management_persist_flags);
 #endif
+
+#ifdef ENABLE_ASYNC_PUSH
+  /* arm inotify watcher */
+  event_ctl (mtcp->es, c->c2.inotify_fd, EVENT_READ, MTCP_FILE_CLOSE_WRITE);
+#endif
+
   status = event_wait (mtcp->es, >c2.timeval, mtcp->esr, mtcp->maxevents);
   update_time ();
   mtcp->n_esr = 0;
@@ -636,6 +646,12 @@ multi_tcp_process_io (struct multi_context *m)
{
  get_signal (>top.sig->signal_received);
}
+#ifdef ENABLE_ASYNC_PUSH
+ else if (e->arg == MTCP_FILE_CLOSE_WRITE)
+   {
+ multi_process_file_closed (m, MPP_PRE_SELECT | MPP_RECORD_TOUCH);
+   }
+#endif
}
   if (IS_SIG (>top))
break;
@@ -684,6 +700,14 @@ tunnel_server_tcp (struct context *top)
   /* finished with initialization */
   initialization_sequence_completed (top, ISC_SERVER); /* --mode server 
--proto tcp-server */

+#ifdef ENABLE_ASYNC_PUSH
+  multi.top.c2.inotify_fd = inotify_init();
+  if