Reworking the interface for querying users

From: David Sommerseth <[email protected]...>

These patches enhances the mechanism in OpenVPN which queries the user for username, passwords, PIN codes, etc. When the management interface is not in use, the current behaviour is to ask for this information sequentially via the console OpenVPN is running on.

The first patch will introduce a more modular approach which also standardizes an API for bulk operations. This means that it is possible to implement more advanced mechanisms asking for both username and passwords at the same time through a single call in the core OpenVPN code. This patch also temporarily removes the systemd implementation.

The second patch will re-implements systemd support using this new API with the same functionality as before this patch set.

The third patch is a re-submission of an earlier patch, which is needed by the fourth patch. It is sent separately, as it provides extra version information on a general basis and is not strictly connected to systemd.

The fourth and last patch is a rebased patch of an earlier submission, removing the masking of usernames. This only happens when the proper systemd version which supports an --echo argument is found.

All patches has been tested locally with different configurations, requiring username, password and passphrases to PKCS#12 files. The challenge/response interface has not been tested, as well as PKCS#11. All runs via valgrind shows no memory leaks with this new implementation.

The v2 patch-set is basically rebased on top of master commit 23b6ba6378 and have just received slight modifications to comments and coding style. The core code itself has not been modified. The first submission was fairly well tested on Fedora 19 with a development version of systemd and this patch revision has been quickly tested on RHEL 7.1 with systemd version 208.

The previous patch revision can be found here:
http://thread.gmane.org/gmane.network.openvpn.devel/9232
Message-Id: [email protected]...

David Sommerseth (4):
Rework the user input interface to make it more modular Re-implement the systemd support using the new query user API
autotools: Add support for extracting version information from
pkg-config modules
systemd: Do not mask usernames when querying for it via
systemd-ask-password

 configure.ac                  |   3 +-
 m4/pkg.m4                     |   7 +-
 src/openvpn/Makefile.am       |  15 ++-
 src/openvpn/console.c         | 225 ++++++--------------------------------
 src/openvpn/console.h         |  94 +++++++++++++++-
 src/openvpn/console_builtin.c | 246 ++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/console_systemd.c | 118 ++++++++++++++++++++
 src/openvpn/misc.c            |  81 +++++++++++---
 src/openvpn/pkcs11.c          |  16 ++-
 9 files changed, 588 insertions(+), 217 deletions(-)
 create mode 100644 src/openvpn/console_builtin.c
 create mode 100644 src/openvpn/console_systemd.c

1.8.3.1

From: David Sommerseth <[email protected]...>

This is will provide an interface for other mechanisms to be used to query the user for information, such as usernames, passwords, etc.

It has also been a goal to make it possible to query for all the information in one call and not do it sequencially as before.

---
 src/openvpn/Makefile.am       |   2 +-
 src/openvpn/console.c         | 225 ++++++--------------------------------
 src/openvpn/console.h         |  94 +++++++++++++++-
 src/openvpn/console_builtin.c | 246 ++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/misc.c            |  81 +++++++++++---
 src/openvpn/pkcs11.c          |  16 ++-
 6 files changed, 450 insertions(+), 214 deletions(-)
 create mode 100644 src/openvpn/console_builtin.c

index d089f50..e07389f 100644
@@ -69,7 +69,7 @@ openvpn_SOURCES = \
 	memdbg.h \
 	misc.c misc.h \
 	platform.c platform.h \
-	console.c console.h \
+	console.c console.h console_builtin.c \
 	mroute.c mroute.h \
 	mss.c mss.h \
 	mstats.c mstats.h \

index 3a15ef6..8e57a85 100644
@@ -5,7 +5,8 @@
  *             packet encryption, packet authentication, and
  *             packet compression.
  *
- *  Copyright (C) 2002-2010 OpenVPN Technologies, Inc. <[email protected]...>
+ *  Copyright (C) 2002-2015 OpenVPN Technologies, Inc. <[email protected]...>
+ *  Copyright (C) 2014-2015 David Sommerseth <[email protected]...>
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2

@@ -740,6 +741,7 @@ _pkcs11_openvpn_show_pkcs11_ids_pin_prompt (
 	const size_t pin_max
 ) {
 	struct gc_arena gc = gc_new ();
+	struct query_user *qucfg = NULL;
 	struct buffer pass_prompt = alloc_buf_gc (128, &gc);
 
 	(void)global_data;

@@ -748,10 +750,18 @@ _pkcs11_openvpn_show_pkcs11_ids_pin_prompt (
 
 	ASSERT (token!=NULL);
 
+	if( !query_user_init (&gc, &qucfg) )
+	{
+	    msg (M_FATAL, "ERROR: Could not prepare for token PIN dialogue");
+	}
+
 	buf_printf (&pass_prompt, "Please enter '%s' token PIN or 'cancel': ", token->display);
+	query_user_add(qucfg, BSTR(&pass_prompt), BLEN(&pass_prompt),
+		       pin, pin_max, true, false);
 
-	if (!get_console_input (BSTR (&pass_prompt), false, pin, pin_max)) {
-		msg (M_FATAL, "Cannot read password from stdin");
+	if (!query_user_exec (qucfg))
+	{
+	    msg (M_FATAL, "Could not retrieve the PIN");
 	}
 
 	gc_free (&gc);

1.8.3.1

From: David Sommerseth <[email protected]...>

This provides exactly the same systemd functionality which existed before the query user infrastructure got implemented.

---
 configure.ac                  |   2 +-
 src/openvpn/Makefile.am       |  13 +++++
 src/openvpn/console_systemd.c | 112 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+), 1 deletion(-)
 create mode 100644 src/openvpn/console_systemd.c

index 9132468..4e9fbfc 100644
@@ -993,7 +993,7 @@ fi
 dnl
 dnl Check for systemd
 dnl
-
+AM_CONDITIONAL([ENABLE_SYSTEMD], [test "${enable_systemd}" = "yes"])
 if test "$enable_systemd" = "yes" ; then
     PKG_CHECK_MODULES([libsystemd], [systemd libsystemd],
                       [],

index e07389f..29cafd7 100644
@@ -117,6 +117,19 @@ openvpn_SOURCES = \
 	tun.c tun.h \
 	win32.h win32.c \
 	cryptoapi.h cryptoapi.c
+
+
+if ENABLE_SYSTEMD
+#
+# If --enable-systemd have been used, build the alternative systemd mechanism
+# The CFLAGS macro set here ensures the alternative mechanism is being used.
+# See console.h for more information.
+#
+AM_CFLAGS += -DQUERY_USER_EXEC_ALTERNATIVE
+openvpn_SOURCES += console_systemd.c
+endif
+
+
 openvpn_LDADD = \
 	$(top_builddir)/src/compat/libcompat.la \
 	$(SOCKETS_LIBS) \

1.8.3.1

From: David Sommerseth <[email protected]...>

---
 m4/pkg.m4 | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 9a71878..f82496b 100644
@@ -107,16 +107,18 @@ AC_DEFUN([PKG_CHECK_MODULES],
 [AC_REQUIRE([PKG_PROG_PKG_CONFIG])dnl
 AC_ARG_VAR([$1][_CFLAGS], [C compiler flags for $1, overriding pkg-config])dnl
 AC_ARG_VAR([$1][_LIBS], [linker flags for $1, overriding pkg-config])dnl
+AC_ARG_VAR([$1][_MODVERSION], [module version])dnl
 
 pkg_failed=no
 AC_MSG_CHECKING([for $1])
 
 _PKG_CONFIG([$1][_CFLAGS], [cflags], [$2])
 _PKG_CONFIG([$1][_LIBS], [libs], [$2])
+_PKG_CONFIG([$1][_MODVERSION], [modversion], [$2])
 
 m4_define([_PKG_TEXT], [Alternatively, you may set the environment variables $1[]_CFLAGS
-and $1[]_LIBS to avoid the need to call pkg-config.
-See the pkg-config man page for more details.])
+and $1[]_LIBS to avoid the need to call pkg-config.  The $1[]_MODVERSION provides version
+information.  See the pkg-config man page for more details.])
 
 if test $pkg_failed = yes; then
    	AC_MSG_RESULT([no])

@@ -153,6 +155,7 @@ To get pkg-config, see <http://pkg-config.freedesktop.org/>.])[]dnl
 else
 	$1[]_CFLAGS=$pkg_cv_[]$1[]_CFLAGS
 	$1[]_LIBS=$pkg_cv_[]$1[]_LIBS
+	$1[]_MODVERSION=$pkg_cv_[]$1[]_MODVERSION
         AC_MSG_RESULT([yes])
 	$3
 fi[]dnl

1.8.3.1

From: David Sommerseth <[email protected]...>

In systemd after version 216, systemd-ask-password will support --echo which will avoid masking the user input. As OpenVPN uses this mechanism collecting usernames when systemd is available, this will avoid the input of usernames to be masked.

This patch also adds the --icon argument, which is aimed at graphical inputs. For example when OpenVPN is started at system boot-time using a graphical boot interface such as Plymouth.

---
 configure.ac                  | 1 +
 src/openvpn/console_systemd.c | 6 ++++++
 2 files changed, 7 insertions(+)

index 4e9fbfc..0c7eed4 100644
@@ -1010,6 +1010,7 @@ if test "$enable_systemd" = "yes" ; then
     AC_CHECK_FUNCS([sd_booted], [], [AC_MSG_ERROR([systemd library is missing sd_booted()])])
     OPTIONAL_SYSTEMD_LIBS="${libsystemd_LIBS}"
     AC_DEFINE(ENABLE_SYSTEMD, 1, [Enable systemd integration])
+    AC_DEFINE_UNQUOTED(SYSTEMD_VERSION, [`echo ${libsystemd_MODVERSION}`], [systemd version installed])
     LIBS="${saved_LIBS}"
 fi
 

index 1fb0891..b8be691 100644
@@ -61,6 +61,12 @@ get_console_input_systemd (const char *prompt, const bool echo, char *input, con
 
     argv_init (&argv);
     argv_printf (&argv, SYSTEMD_ASK_PASSWORD_PATH);
+#if SYSTEMD_VERSION > 216
+    if( echo )
+    {
+	argv_printf_cat(&argv, "--echo");
+    }
+#endif
     argv_printf_cat (&argv, "%s", prompt);
 
     if ((std_out = openvpn_popen (&argv, NULL)) < 0) {

1.8.3.1

I've given some pointers in ticket 538 on how you can test PKCS#11
without actually having a hardware token — you can use GNOME keyring in many cases, or in this case where you really want to test password input,
you can use a NSS "softokn" database.

Even if I make these patches build by fixing the issue that leads to libsystemd_MODVERSION being set to '219\n219' when both systemd.pc and libsystemd.pc exist, they still don't solve the problem described in that ticket.

You are still calling fork() from a context (the PIN request callback from pkcs11-helper) in which it is forbidden, and in which it deadlocks:

#2  0x00007ffff7d9c096 in _pkcs1h_threading_mutexLockAll () at pkcs11h-threading.c:306
#3  0x00000038412c8362 in __libc_fork () at ../sysdeps/nptl/fork.c:89
#4  0x0000003841a0f4a5 in __fork () at pt-fork.c:25
#5  0x000000000042893b in openvpn_popen ([email protected]...=0x7fffffff9be0, [email protected]...=0x0) at misc.c:374
#6  0x000000000046f0e8 in get_console_input_systemd (capacity=4096, input=0x7fffffffb1f2 "", echo=false, 
    prompt=0x73cdc8 "Enter PIV_II (PIV Card Holder pin) token Password:") at console_systemd.c:72
#7  query_user_exec (setup=<optimized out>) at console_systemd.c:111
#8  0x0000000000427aed in get_user_pass_cr ([email protected]...=0x7fffffffa1f0, auth_file=<optimized out>, [email protected]...=0x0, 
    [email protected]...=0x7fffffff9df0 "PIV_II (PIV Card Holder pin) token", [email protected]...=21, [email protected]...=0x0) at misc.c:1144
#9  0x0000000000435711 in get_user_pass (flags=21, prefix=0x7fffffff9df0 "PIV_II (PIV Card Holder pin) token", auth_file=0x0, up=0x7fffffffa1f0) at misc.h:268
#10 _pkcs11_openvpn_pin_prompt (global_data=<optimized out>, user_data=<optimized out>, token=<optimized out>, retry=<optimized out>, pin=0x7fffffffc250 "K{`f=", 
    pin_max=1024) at pkcs11.c:236
#11 0x00007ffff7d9e0cb in _pkcs11h_session_login (session=0x6e9220, [email protected]...=0, [email protected]...=1, user_data=0x0, 
    mask_prompt=<optimized out>) at pkcs11h-session.c:1023

Perhaps the solution might be to *always* operate in 'management interface' mode, and to shift all the direct console access out to a separate process. *That* process would be permitted to run fork(), when the core OpenVPN process requests a password...

dwmw2

THanks a lot for a lot of value input, both here and in the trac ticket!
I'll go through that and digest it carefully.

In regards to fork()/vfork(). Personally, I'm not against changing it.
However, I don't know what the other reviewers may think. I'll dig
deeper into this matter and.

Having that said, looking isolated on systemd and pkcs#11. I've heard from a few of the systemd developers that they are going to retire the current approach for quering for user input for something more flexible.
And they point at some kind of DBus interface as a natural next step
(it has not yet materialized into code, so I'm currently sitting on the fence). That means that OpenVPN would not fork at all, it would send a request via a DBus interface describing what the user (or session) must provide. And the result is then returned via DBus. And this is what I've had in mind when changing these code paths. Which is also the reason that console_systemd.c isn't compiled unless configured with --enable-systemd ... to easier split out code which may make the non-Linux guys of the openvpn community scream and panic when seeing DBus library dependencies.

The current management interface works fairly well, but it has limitations which can cause some cause some hang-ups due to the single threaded nature of OpenVPN - there are also some who claims they notice performance drops when management commands are processed. I've not dug into these claims yet, but it doesn't surprise me if there is something to it. In addition the connection to the management interface is basically not authenticated (well, you can have a password but it's a clear-text connection).

Again, thanks a lot for all your input! I'm fairly loaded with work, so openvpn does unfortunately suffer from that from my part. But I do appreciate your involvement!

kind regards,

David Sommerseth