openvpn-devel Mailing List for OpenVPN

As discussed in the community meeting of 13-12-2017, we should warn our users that LibreSSL is not officially supported. We expect that it currently works, but it might suddenly break or we might decide to no longer build against LibreSSL in the future. There seem to be ongoing efforts to make LibreSSL compatible with the OpenSSL 1.1 API. If they truly do that, it might also keep working. For now, make sure people understand we do not really support LibreSSL. Signed-off-by: Steffan Karger <st...@ka...> --- src/openvpn/options.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 41a42cf2..36d67b0f 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -2949,6 +2949,11 @@ options_postprocess_mutate_invariant(struct options *options) static void options_postprocess_verify(const struct options *o) { +#ifdef LIBRESSL_VERSION_NUMBER + msg(M_WARN, "WARNING: This OpenVPN was built against LibreSSL. " + "This might work, but is *not* supported and can break at any time.") +#endif + if (o->connection_list) { int i; -- 2.14.1 

Hi, Steffan asked me to pull this into 2.4, which makes sense - it's somewhere between "code cleanup" and "bugfix", basically avoiding an ASSERT() when we could more gracefully handle a failure on an incoming client (... in a somewhat unlikely chain of events, but still, ASSERT() on the server side for recoverable things is bad). commit 77a0bdb77d4c5573fcb78f1e36c45d882a9923ba (HEAD -> release/2.4) Author: Steffan Karger <ste...@fo...> Date: Wed Nov 1 23:03:41 2017 +0100 Don't throw fatal errors from create_temp_file() Signed-off-by: Steffan Karger <ste...@fo...> Acked-by: Antonio Quartulli <an...@op...> Message-Id: <201...@ka...> URL: https://www.mail-archive.com/ope...@li.../msg15701.html Signed-off-by: Gert Doering <ge...@gr...> (cherry picked from commit 3e0fd2b0471cf4e53959902ca10d88db7a1ef916) gert On Fri, Nov 24, 2017 at 01:42:40PM +0100, Gert Doering wrote: > Your patch has been applied to the master branch. > > commit 3e0fd2b0471cf4e53959902ca10d88db7a1ef916 > Author: Steffan Karger > Date: Wed Nov 1 23:03:41 2017 +0100 > > Don't throw fatal errors from create_temp_file() > > Signed-off-by: Steffan Karger <ste...@fo...> > Acked-by: Antonio Quartulli <an...@op...> > Message-Id: <201...@ka...> > URL: https://www.mail-archive.com/ope...@li.../msg15701.html > Signed-off-by: Gert Doering <ge...@gr...> > > > -- > kind regards, > > Gert Doering > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Openvpn-devel mailing list > Ope...@li... > https://lists.sourceforge.net/lists/listinfo/openvpn-devel > -- now what should I write here... Gert Doering - Munich, Germany ge...@gr... 

Compile-tested on OpenBSD 6.0 (no EC/LibreSSL hickup, good :-) ) and mingw / ubuntu 16.04. All well. Your patch has been applied to the master branch. (Is there a strong case for including it in 2.4?) commit a6f38bafbbbd291d57ecb3610c2844e7f7e01412 Author: Selva Nair Date: Sun Feb 25 00:02:15 2018 -0500 Support EC certificates with cryptoapicert Signed-off-by: Selva Nair <sel...@gm...> Acked-by: Steffan Karger <ste...@fo...> Message-Id: <151...@gm...> URL: https://www.mail-archive.com/ope...@li.../msg16553.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

Acked-by: Gert Doering <ge...@gr...> as discussed on IRC this morning. Your patch has been applied to the master and release/2.4 branch. commit ec100d7e4ce7aaeb731c22b0d86826bf295df6cd (master) commit e5ee5121cbbeca6dcbee38dea5b40779e3f6da83 (release/2.4) Author: David Sommerseth Date: Wed Feb 28 14:19:17 2018 +0100 man: Reword --management to prefer unix sockets over TCP Signed-off-by: David Sommerseth <da...@op...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <201...@op...> URL: https://www.mail-archive.com/ope...@li.../msg16573.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

Acked-by: Gert Doering <ge...@gr...> (Tested on FreeBSD 9, where .TQ is not available out of the box) Your patch has been applied to the master and release/2.4 branch. commit 5ed5ac5cf869c0284ffeedda358da23e201357cc (master) commit c5a63d5a6ea78bee069cc503e0a396decfbbaf0e (release/2.4) Author: David Sommerseth Date: Wed Feb 28 14:19:16 2018 +0100 man: Add .TQ groff support macro Signed-off-by: David Sommerseth <da...@op...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <201...@op...> URL: https://www.mail-archive.com/ope...@li.../msg16575.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

Hi, On Wed, Feb 28, 2018 at 8:34 AM, Arne Schwabe <ar...@rf...> wrote: > Am 28.02.18 um 14:19 schrieb David Sommerseth: >> It is not recommended to use --management on a TCP port without also >> adding a password authentication, as this can easily be abused by other >> users or processes being able to connect to the managmement interface. >> >> Thus issue a warning that this configuration is strongly discouraged. >> >> Signed-off-by: David Sommerseth <da...@op...> >> --- >> src/openvpn/options.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/src/openvpn/options.c b/src/openvpn/options.c >> index 41a42cf2..e0c0894b 100644 >> --- a/src/openvpn/options.c >> +++ b/src/openvpn/options.c >> @@ -2170,6 +2170,14 @@ options_postprocess_verify_ce(const struct options *options, const struct connec >> { >> msg(M_USAGE, "--management-client-(user|group) can only be used on unix domain sockets"); >> } >> + >> + if (!(options->management_flags & MF_UNIX_SOCK) >> + && (!options->management_user_pass)) >> + { >> + msg(M_WARN, "WARNING: Using --management on a TCP port WITHOUT " >> + "passwords is STRONGLY discouraged and considered insecure"); >> + } >> + >> #endif >> >> /* >> > > Does not break existing configs and warns about a real problem. Some > users of management might scream that, users now get a warning none was > before but honestely I don't care. > > @All does our own Windows UI use management and if yes does it set a > random user/pw to connect to it? Yes and yes. Selva 

In tls_ctx_use_external_private_key, the return codes were inverted compared to what is documented in ssl_backend.h (and what can reasonably be expected). Internally the return code is never checked, so this did not directly result in any change of behavior. --- src/openvpn/ssl_mbedtls.c | 6 +++--- src/openvpn/ssl_openssl.c | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c index 3906cd55..8e31980a 100644 --- a/src/openvpn/ssl_mbedtls.c +++ b/src/openvpn/ssl_mbedtls.c @@ -630,7 +630,7 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx, if (ctx->crt_chain == NULL) { - return 0; + return 1; } ALLOC_OBJ_CLEAR(ctx->external_key, struct external_context); @@ -640,10 +640,10 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx, if (!mbed_ok(mbedtls_pk_setup_rsa_alt(ctx->priv_key, ctx->external_key, NULL, external_pkcs1_sign, external_key_len))) { - return 0; + return 1; } - return 1; + return 0; } #endif /* ifdef MANAGMENT_EXTERNAL_KEY */ diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index d91458b0..8ef68ebd 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -1327,11 +1327,11 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx, goto err; } #endif /* OPENSSL_VERSION_NUMBER > 1.1.0 dev */ - return 1; + return 0; err: crypto_msg(M_FATAL, "Cannot enable SSL external private key capability"); - return 0; + return 1; } #endif /* ifdef MANAGMENT_EXTERNAL_KEY */ -- 2.16.2 

Am 28.02.18 um 14:19 schrieb David Sommerseth: > It is not recommended to use --management on a TCP port without also > adding a password authentication, as this can easily be abused by other > users or processes being able to connect to the managmement interface. > > Thus issue a warning that this configuration is strongly discouraged. > > Signed-off-by: David Sommerseth <da...@op...> > --- > src/openvpn/options.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index 41a42cf2..e0c0894b 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -2170,6 +2170,14 @@ options_postprocess_verify_ce(const struct options *options, const struct connec > { > msg(M_USAGE, "--management-client-(user|group) can only be used on unix domain sockets"); > } > + > + if (!(options->management_flags & MF_UNIX_SOCK) > + && (!options->management_user_pass)) > + { > + msg(M_WARN, "WARNING: Using --management on a TCP port WITHOUT " > + "passwords is STRONGLY discouraged and considered insecure"); > + } > + > #endif > > /* > Does not break existing configs and warns about a real problem. Some users of management might scream that, users now get a warning none was before but honestely I don't care. @All does our own Windows UI use management and if yes does it set a random user/pw to connect to it? Acked-By: Arne Schwabe <ar...@rf...> Arne 

It is not recommended to use --management on a TCP port without also adding a password authentication, as this can easily be abused by other users or processes being able to connect to the managmement interface. Thus issue a warning that this configuration is strongly discouraged. Signed-off-by: David Sommerseth <da...@op...> --- src/openvpn/options.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 41a42cf2..e0c0894b 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -2170,6 +2170,14 @@ options_postprocess_verify_ce(const struct options *options, const struct connec { msg(M_USAGE, "--management-client-(user|group) can only be used on unix domain sockets"); } + + if (!(options->management_flags & MF_UNIX_SOCK) + && (!options->management_user_pass)) + { + msg(M_WARN, "WARNING: Using --management on a TCP port WITHOUT " + "passwords is STRONGLY discouraged and considered insecure"); + } + #endif /* -- 2.13.5 

It is more secure to use unix sockets instead of TCP ports for the management interface, so reword it and provide some details why TCP is not recommended. Also re-arranged this section to be somewhat easier to read and clearer on a few related details. Signed-off-by: David Sommerseth <da...@op...> --- This patch depends on the .TQ macro. If the support macro patch has not been applied, it will not render nicely on platforms not containing .TQ support. --- doc/openvpn.8 | 76 +++++++++++++++++++++++++++++------------------------------ 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/doc/openvpn.8 b/doc/openvpn.8 index bd9f2606..a923da02 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -2555,54 +2555,52 @@ the compression efficiency will be very low, triggering openvpn to disable compression for a period of time until the next re\-sample test. .\"********************************************************* .TP +.B \-\-management socket\-name unix [pw\-file] \ \ \ \ \ (recommended) +.TQ .B \-\-management IP port [pw\-file] -Enable a TCP server on -.B IP:port -to handle daemon management functions. -.B pw\-file, -if specified, -is a password file (password on first line) -or "stdin" to prompt from standard input. The password -provided will set the password which TCP clients will need -to provide in order to access management functions. +Enable a management server on a +.B socket\-name +Unix socket on those platforms supporting it, or on +a designated TCP port. -The management interface can also listen on a unix domain socket, -for those platforms that support it. To use a unix domain socket, specify -the unix socket pathname in place of -.B IP -and set -.B port -to 'unix'. While the default behavior is to create a unix domain socket -that may be connected to by any process, the +.B pw\-file +, if specified, is a password file where the password must be on first line. +Instead of a filename it can use the keyword stdin which will prompt the user +for a password to use when OpenVPN is starting. + +For unix sockets, the default behaviour is to create a unix domain socket +that may be connected to by any process. Use the .B \-\-management\-client\-user and .B \-\-management\-client\-group -directives can be used to restrict access. +directives to restrict access. -The management interface provides a special mode where the TCP -management link can operate over the tunnel itself. To enable this mode, -set -.B IP -= "tunnel". Tunnel mode will cause the management interface -to listen for a TCP connection on the local VPN address of the -TUN/TAP interface. +The management interface provides a special mode where the TCP management link +can operate over the tunnel itself. To enable this mode, set IP to +.B tunnel. +Tunnel mode will cause the management interface to listen for a +TCP connection on the local VPN address of the TUN/TAP interface. -While the management port is designed for programmatic control -of OpenVPN by other applications, it is possible to telnet -to the port, using a telnet client in "raw" mode. Once connected, -type "help" for a list of commands. +.B BEWARE +of enabling the management interface over TCP. In these cases you should +.I ALWAYS +make use of +.B pw\-file +to password protect the management interface. Any user who can connect to this +TCP +.B IP:port +will be able to manage and control (and interfere with) the OpenVPN process. +It is also strongly recommended to set IP to 127.0.0.1 (localhost) to restrict +accessibility of the management server to local clients. -For detailed documentation on the management interface, see -the management\-notes.txt file in the -.B management -folder of -the OpenVPN source distribution. +While the management port is designed for programmatic control of OpenVPN by +other applications, it is possible to telnet to the port, using a telnet client +in "raw" mode. Once connected, type "help" for a list of commands. + +For detailed documentation on the management interface, see the +.I management\-notes.txt +file in the management folder of the OpenVPN source distribution. -It is strongly recommended that -.B IP -be set to 127.0.0.1 -(localhost) to restrict accessibility of the management -server to local clients. .TP .B \-\-management\-client Management interface will connect as a TCP/unix domain client to -- 2.13.5 

This introduces the .TQ groff macro. Even though this can be found in newer groff versions, not all platforms we support carries this one. This macro makes it possible to have mulitple lines of options as headers before describing all of these options in the same segment. Signed-off-by: David Sommerseth <da...@op...> --- doc/openvpn.8 | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/doc/openvpn.8 b/doc/openvpn.8 index 364aee8a..bd9f2606 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -33,7 +33,15 @@ .\" .ft -- normal face .\" .in +|-{n} -- indent .\" -.TH openvpn 8 "31 January 2018" +.\" Support macros - this is not present on all platforms +.\" Continuation line for .TP header. +.de TQ +. br +. ns +. TP \\$1\" no doublequotes around argument! +.. +.\" End of TQ macro +.TH openvpn 8 "28 February 2018" .\"********************************************************* .SH NAME openvpn \- secure IP tunnel daemon. -- 2.13.5 

Hi, On Wed, Feb 21, 2018 at 2:42 AM, Gert Doering <ge...@gr...> wrote: > Hi, > > On Tue, Feb 20, 2018 at 06:33:35PM -0500, Selva Nair wrote: >> > (It does happen for my own binary and for the installers Samuli builds, >> > so it's not "my build environment" - theoretically it could be part 1/2 >> > of that patch set, which I haven't run yet) >> >> Yeah, I could reproduce it on Windows 7. And, at last, after several >> painful Windows builds, it turns out: >> >> RegGetValue with RRF_RT_REG_EXPAND_SZ|RRF_RT_REG_SZ is broken on >> Windows 7 (and most likely Vista too). > > Thanks for investigating and finding the root cause. "This is not what > the documentation says...!" -> I need to actually *run* these things, not > just test compile. When I say painful builds, I mean edit build[*], copy to Windows, run in a few configurations, look through the event viewer, rinse rather repeat. And finally when it seems to start working connect to a my work VPN, remove the registry keys and try to break it etc.. FYI, I don't just "test compile", and leave it at that. Selva [*] Yes, that fetch-all and rebuild from scratch part of openvpn-build is very time consuming. Making it work the usual "remake only what changed" way would save a lot of time and frustration. 

Hi, On Tue, Feb 20, 2018 at 06:33:35PM -0500, Selva Nair wrote: > > (It does happen for my own binary and for the installers Samuli builds, > > so it's not "my build environment" - theoretically it could be part 1/2 > > of that patch set, which I haven't run yet) > > Yeah, I could reproduce it on Windows 7. And, at last, after several > painful Windows builds, it turns out: > > RegGetValue with RRF_RT_REG_EXPAND_SZ|RRF_RT_REG_SZ is broken on > Windows 7 (and most likely Vista too). Thanks for investigating and finding the root cause. "This is not what the documentation says...!" -> I need to actually *run* these things, not just test compile. > But once you know what to look for, just google: > > https://stackoverflow.com/questions/47096940/reggetvalue-for-value-that-may-be-either-reg-sz-or-reg-expand-sz > > Will send a patch later. Thanks! PS: when you say "several painful Windows builds" - I found that things are actually working quite nicely using "build-snapshot --use-depcache", pointing this to a private git repo which has the code I want to test. Still a bit annoying because the machinery insists on "fetch all, clean all, rebuild all" instead of "there's the source directory, just run make!"... shall we try to spend a bit time on this, to make interactively working on source changes nicer? gert -- now what should I write here... Gert Doering - Munich, Germany ge...@gr... 

Hi, On Wed, Feb 21, 2018 at 8:20 AM, Selva Nair <sel...@gm...> wrote: > Hi, > > On Wed, Feb 21, 2018 at 2:42 AM, Gert Doering <ge...@gr...> wrote: >> Hi, >> >> On Tue, Feb 20, 2018 at 06:33:35PM -0500, Selva Nair wrote: >>> > (It does happen for my own binary and for the installers Samuli builds, >>> > so it's not "my build environment" - theoretically it could be part 1/2 >>> > of that patch set, which I haven't run yet) >>> >>> Yeah, I could reproduce it on Windows 7. And, at last, after several >>> painful Windows builds, it turns out: >>> >>> RegGetValue with RRF_RT_REG_EXPAND_SZ|RRF_RT_REG_SZ is broken on >>> Windows 7 (and most likely Vista too). >> >> Thanks for investigating and finding the root cause. "This is not what >> the documentation says...!" -> I need to actually *run* these things, not >> just test compile. > > When I say painful builds, I mean edit build[*], copy to Windows, run > in a few configurations, look through the event viewer, rinse rather > repeat. And finally when it seems to start working connect to a my > work VPN, remove the registry keys and try to break it etc.. > > FYI, I don't just "test compile", and leave it at that. On re-reading, it sounds different from what I wanted to say. I was just cursing stupid Windows that's no fun to debug on... In this particular case it was my fault that I never tested the patch on Windows 7 in the first place. I used to run a Win7 VM, but now all my desktop memory is eaten by those 50 chrome tabs.. Anyway, sorry to all (and to myself) for causing a "must-fix" bug close to a release. Selva 

Hi, On Tue, Feb 20, 2018 at 08:40:31PM +0100, Steffan Karger wrote: > We could revert to an older version of the macro we ship, or to a > simpler (though less clear) AC_CHECK_COMPILE-based check. > > I'm tempted to ditch the macro and go for the AC_CHECK_COMPILE. The macro seems to be more painful than expected - so I tend to agree with you :-) - patches welcome. Though with the state of the sf.net list, patch handling is quite annoying :-/ (read: some mails get through, some mails take ages, patchwork isn't seeing stuff, the list archive isn't seeing stuff that I could reference on merging, etc. - gah) gert -- now what should I write here... Gert Doering - Munich, Germany ge...@gr... 

Hi, On Wed, Feb 21, 2018 at 02:07:03AM -0500, Selva Nair wrote: > >> *This* one breaks LibreSSL compilation (namely, the OpenBSD buildbot), > >> because [..] > Tested using the freebsd-11 box in openvpn-vagrant and did pkg install > libressl-2.6.4 (that replaces openssl 1.1.0). > > This particular error gets fixed by adding > !defined(LIBRESSL_VERSION_NUMBER) in two places. > > I can send a patch for this. This would be appreciated (please --cc me directly as the list is still not working properly). > But there are other errors: > > In file included from crypto_openssl.c:44: > ./openssl_compat.h:699:1: error: static declaration of > 'SSL_CTX_set_min_proto_version' follows [..] > ./openssl_compat.h:728:1: error: static declaration of > 'SSL_CTX_set_max_proto_version' follows > non-static declaration [..] > In openssl those are macros but not in libressl so the current > openssl_compat.h defs don't work. This is interesting. Seems my OpenBSD version is too old, as I've not seen these errors in my buildbots. Let's fix one thing at a time, and start with the easy ones, which will make the buildbots happy for the time being :-) - and then look at "more recent LibreSSL fallouts". gert -- now what should I write here... Gert Doering - Munich, Germany ge...@gr... 

Acked-by: Gert Doering <ge...@gr...> Your patch has been applied to the master branch. (I took the ACK from Steffan for v1, and verified that v2 is really idential except for the extra empty line - so line numbers differ, but code is the same. Adding an extra ACK from me for staring-at-the-code - trivial enough in this case) commit 29a475c6ce0fa51b173121033cbd0f280948e06c Author: Selva Nair Date: Fri Jan 26 11:05:32 2018 -0500 Move code to free cd to a function CAPI_DATA_free() Signed-off-by: Selva Nair <sel...@gm...> Acked-by: Steffan Karger <ste...@fo...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <151...@gm...> URL: https://www.mail-archive.com/ope...@li.../msg16383.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

Hi, On Wed, Feb 21, 2018 at 08:20:34AM -0500, Selva Nair wrote: > > Thanks for investigating and finding the root cause. "This is not what > > the documentation says...!" -> I need to actually *run* these things, not > > just test compile. > > When I say painful builds, I mean edit build[*], copy to Windows, run > in a few configurations, look through the event viewer, rinse rather > repeat. And finally when it seems to start working connect to a my > work VPN, remove the registry keys and try to break it etc.. Indeed, that takes quite a bit of time. > FYI, I don't just "test compile", and leave it at that. I did not want to imply that you did - sorry if it came across that way. *I* only test compiled, and was then happy merging your patch ("I'm sure Selva tested this, it is according to documentation and the code looks good, and compiles fine") - but as I learned, there are more caveats on Windows. So, when we do more basic changes (new API, etc), I need to do actual test installations on my windows zoo, like, win7/32, win7/64, win10/64... I usually do this already today if I suspect breakage, but in this case, it came as a surprise. gert -- now what should I write here... Gert Doering - Munich, Germany ge...@gr... 

Hi, On Wed, Feb 21, 2018 at 2:36 AM, Gert Doering <ge...@gr...> wrote: > Hi, > > On Wed, Feb 21, 2018 at 02:07:03AM -0500, Selva Nair wrote: >> >> *This* one breaks LibreSSL compilation (namely, the OpenBSD buildbot), >> >> because > [..] >> Tested using the freebsd-11 box in openvpn-vagrant and did pkg install >> libressl-2.6.4 (that replaces openssl 1.1.0). >> >> This particular error gets fixed by adding >> !defined(LIBRESSL_VERSION_NUMBER) in two places. >> >> I can send a patch for this. > > This would be appreciated (please --cc me directly as the list is still > not working properly). > >> But there are other errors: >> >> In file included from crypto_openssl.c:44: >> ./openssl_compat.h:699:1: error: static declaration of >> 'SSL_CTX_set_min_proto_version' follows > [..] >> ./openssl_compat.h:728:1: error: static declaration of >> 'SSL_CTX_set_max_proto_version' follows >> non-static declaration > [..] >> In openssl those are macros but not in libressl so the current >> openssl_compat.h defs don't work. > > This is interesting. Seems my OpenBSD version is too old, as I've not > seen these errors in my buildbots. This could be also because of using the vagrant box with freebsd-11. It came up with openssl 1.0 (not libressl) installed -- is that the default? And there were no build errors as external-ec-key gets disabled with openssl 1.0. On installing openssl 1.1.0g and rebuilding, again, no errors as expected. The only libressl package that showed up with "pkg search" was 2.6.4 and installing that removed openssl 1.1.0g (which it shouldn't but that's a different topic). Then the missing EC_KEY_METHOD and conflicting signatures for SSL_CTX_set_max_proto_version etc showed up. Anyway, I am sending a patch for disabling external-ec-key code when libressl is in use. Selva 

Hi, On 20-02-18 11:00, Gert Doering wrote: > On Tue, Feb 20, 2018 at 10:41:08AM +0100, Gert Doering wrote: >> Your patch has been applied to the master and release/2.4 branch. >> >> Tested on FreeBSD 10, Linux and, indeed, AIX 7 :-) - Linux/gcc is nicely >> clean, FreeBSD/clang finds a few that are silly and want fixing... > > Yeah. Right. And it blows up on centos-6 and opensolaris-10. > > Now I can't claim that I understand this... David double-checked this > macro and m4/ stuff, so "everything should have been good", but it > isn't. > > CentOS 6: > > checking lz4.h usability... no > checking lz4.h presence... no > checking for lz4.h... no > usable LZ4 library or header not found, using version in src/com > +pat/compat-lz4.* > checking git checkout... yes > ./configure: line 24067: syntax error near unexpected token `newline' > ./configure: line 24067: `AX_CHECK_COMPILE_FLAG(' > make: *** [config.status] Error 2 They both already fail in the step before it seems: > configure.ac:1295: error: Autoconf version 2.64 or higher is required ContOS 6 ships with autoconf 2.63, I don't know what opensolaris-10 has. We could revert to an older version of the macro we ship, or to a simpler (though less clear) AC_CHECK_COMPILE-based check. I'm tempted to ditch the macro and go for the AC_CHECK_COMPILE. -Steffan 

Hi, On Tue, Feb 20, 2018 at 11:19:23AM -0500, Selva Nair wrote: > Hmm.. I thought I had tested the patch.. Looking at it right now -- hold on :) Missed you on IRC... 18:21 < selvanair> cron2: iservives in 2.4 local build and snapshot build works on Windows 10. Have to go now, but will test Win7 later today when I can get hold of a one. 18:22 < selvanair> s/iservives/iservice ... seems this is different on Win7...? To be sure it's not something messed up on my system, I have checked the HKLM\Software\OpenVPN\(null) path that it's trying to access - and all is there (standard) REG_SZ c:\Program Files\OpenVPN config_dir REG_SZ c:\Program Files\OpenVPN\config ... and according to the documentation in https://msdn.microsoft.com/en-us/library/windows/desktop/ms724868(v=vs.85).aspx passing "lpValue = NULL" really should work... the error code 0x57 OTOH is documented as	ERROR_INVALID_PARAMETER	87 (0x57)	The parameter is incorrect. ... which isn't exactly helpful to see *which* argument it does not like - everything *should* be according to specification... (It does happen for my own binary and for the installers Samuli builds, so it's not "my build environment" - theoretically it could be part 1/2 of that patch set, which I haven't run yet) gert -- now what should I write here... Gert Doering - Munich, Germany ge...@gr... 

Hi, On 25-02-18 06:02, sel...@gm... wrote: > From: Selva Nair <sel...@gm...> > > Requires openssl 1.1.0 or higher > > Signed-off-by: Selva Nair <sel...@gm...> > --- > v3 changes: > - check return value of ECDSA_SIG_set0 > - ensure buffer size needed by i2d_ECDSA_SIG does not exceed the expected > capacity of the sig buffer > - Fix a typo and add contextual info to a debug message > - Remove an superflous cast leftover from an older version > v4 changes: > - Add ECDSA_SIG_free() cleanup calls missed in v3 (2 places). > > src/openvpn/cryptoapi.c | 210 +++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 209 insertions(+), 1 deletion(-) > > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c > index 1097286..7e211f0 100644 > --- a/src/openvpn/cryptoapi.c > +++ b/src/openvpn/cryptoapi.c > @@ -1,5 +1,6 @@ > /* > * Copyright (c) 2004 Peter 'Luna' Runestig <pe...@ru...> > + * Copyright (c) 2018 Selva Nair <sel...@gm...> > * All rights reserved. > * > * Redistribution and use in source and binary forms, with or without modifi- > @@ -101,6 +102,9 @@ static ERR_STRING_DATA CRYPTOAPI_str_functs[] = { > { 0, NULL } > }; > > +/* index for storing external data in EC_KEY: < 0 means uninitialized */ > +static int ec_data_idx = -1; > + > typedef struct _CAPI_DATA { > const CERT_CONTEXT *cert_context; > HCRYPTPROV_OR_NCRYPT_KEY_HANDLE crypt_prov; > @@ -394,6 +398,201 @@ finish(RSA *rsa) > return 1; > } > > +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L) && !defined(OPENSSL_NO_EC) > + > +static EC_KEY_METHOD *ec_method = NULL; > + > +/** EC_KEY_METHOD callback: called when the key is freed */ > +static void > +ec_finish(EC_KEY *ec) > +{ > + EC_KEY_METHOD_free(ec_method); > + ec_method = NULL; > + CAPI_DATA *cd = EC_KEY_get_ex_data(ec, ec_data_idx); > + CAPI_DATA_free(cd); > + EC_KEY_set_ex_data(ec, ec_data_idx, NULL); > +} > + > +/** EC_KEY_METHOD callback sign_setup(): we do nothing here */ > +static int > +ecdsa_sign_setup(EC_KEY *eckey, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp) > +{ > + return 1; > +} > + > +/** > + * Helper to convert ECDSA signature returned by NCryptSignHash > + * to an ECDSA_SIG structure. > + * On entry 'buf[]' of length len contains r and s concatenated. > + * Returns a newly allocated ECDSA_SIG or NULL (on error). > + */ > +static ECDSA_SIG * > +ecdsa_bin2sig(unsigned char *buf, int len) > +{ > + ECDSA_SIG *ecsig = NULL; > + DWORD rlen = len/2; > + BIGNUM *r = BN_bin2bn(buf, rlen, NULL); > + BIGNUM *s = BN_bin2bn(buf+rlen, rlen, NULL); > + if (!r || !s) > + { > + goto err; > + } > + ecsig = ECDSA_SIG_new(); /* in openssl 1.1 this does not allocate r, s */ > + if (!ecsig) > + { > + goto err; > + } > + if (!ECDSA_SIG_set0(ecsig, r, s)) /* ecsig takes ownership of r and s */ > + { > + ECDSA_SIG_free(ecsig); > + goto err; > + } > + return ecsig; > +err: > + BN_free(r); /* it is ok to free NULL BN */ > + BN_free(s); > + return NULL; > +} > + > +/** EC_KEY_METHOD callback sign_sig(): sign and return an ECDSA_SIG pointer. */ > +static ECDSA_SIG * > +ecdsa_sign_sig(const unsigned char *dgst, int dgstlen, > + const BIGNUM *in_kinv, const BIGNUM *in_r, EC_KEY *ec) > +{ > + ECDSA_SIG *ecsig = NULL; > + CAPI_DATA *cd = (CAPI_DATA *)EC_KEY_get_ex_data(ec, ec_data_idx); > + > + ASSERT(cd->key_spec == CERT_NCRYPT_KEY_SPEC); > + > + NCRYPT_KEY_HANDLE hkey = cd->crypt_prov; > + BYTE buf[512]; /* large enough buffer for signature to avoid malloc */ > + DWORD len = _countof(buf); > + > + msg(D_LOW, "Cryptoapi: signing hash using EC key: data size = %d", dgstlen); > + > + DWORD status = NCryptSignHash(hkey, NULL, (BYTE *)dgst, dgstlen, (BYTE *)buf, len, &len, 0); > + if (status != ERROR_SUCCESS) > + { > + SetLastError(status); > + CRYPTOAPIerr(CRYPTOAPI_F_NCRYPT_SIGN_HASH); > + } > + else > + { > + /* NCryptSignHash returns r, s concatenated in buf[] */ > + ecsig = ecdsa_bin2sig(buf, len); > + } > + return ecsig; > +} > + > +/** EC_KEY_METHOD callback sign(): sign and return a DER encoded signature */ > +static int > +ecdsa_sign(int type, const unsigned char *dgst, int dgstlen, unsigned char *sig, > + unsigned int *siglen, const BIGNUM *kinv, const BIGNUM *r, EC_KEY *ec) > +{ > + ECDSA_SIG *s; > + > + *siglen = 0; > + s = ecdsa_sign_sig(dgst, dgstlen, NULL, NULL, ec); > + if (s == NULL) > + { > + return 0; > + } > + > + /* convert internal signature structure 's' to DER encoded byte array in sig */ > + int len = i2d_ECDSA_SIG(s, NULL); > + if (len > ECDSA_size(ec)) > + { > + ECDSA_SIG_free(s); > + msg(M_NONFATAL,"Error: DER encoded ECDSA signature is too long (%d bytes)", len); > + return 0; > + } > + *siglen = i2d_ECDSA_SIG(s, &sig); > + ECDSA_SIG_free(s); > + > + return 1; > +} > + > +static int > +ssl_ctx_set_eckey(SSL_CTX *ssl_ctx, CAPI_DATA *cd, EVP_PKEY *pkey) > +{ > + EC_KEY *ec = NULL; > + EVP_PKEY *privkey = NULL; > + > + if (cd->key_spec != CERT_NCRYPT_KEY_SPEC) > + { > + msg(M_NONFATAL, "ERROR: cryptoapicert with only legacy private key handle available." > + " EC certificate not supported."); > + goto err; > + } > + /* create a method struct with default callbacks filled in */ > + ec_method = EC_KEY_METHOD_new(EC_KEY_OpenSSL()); > + if (!ec_method) > + { > + goto err; > + } > + > + /* We only need to set finish among init methods, and sign methods */ > + EC_KEY_METHOD_set_init(ec_method, NULL, ec_finish, NULL, NULL, NULL, NULL); > + EC_KEY_METHOD_set_sign(ec_method, ecdsa_sign, ecdsa_sign_setup, ecdsa_sign_sig); > + > + ec = EC_KEY_dup(EVP_PKEY_get0_EC_KEY(pkey)); > + if (!ec) > + { > + goto err; > + } > + if (!EC_KEY_set_method(ec, ec_method)) > + { > + goto err; > + } > + > + /* get an index to store cd as external data */ > + if (ec_data_idx < 0) > + { > + ec_data_idx = EC_KEY_get_ex_new_index(0, "cryptapicert ec key", NULL, NULL, NULL); > + if (ec_data_idx < 0) > + { > + goto err; > + } > + } > + EC_KEY_set_ex_data(ec, ec_data_idx, cd); > + > + /* cd assigned to ec as ex_data, increase its refcount */ > + cd->ref_count++; > + > + privkey = EVP_PKEY_new(); > + if (!EVP_PKEY_assign_EC_KEY(privkey, ec)) > + { > + EC_KEY_free(ec); > + goto err; > + } > + /* from here on ec will get freed with privkey */ > + > + if (!SSL_CTX_use_PrivateKey(ssl_ctx, privkey)) > + { > + goto err; > + } > + EVP_PKEY_free(privkey); /* this will dn_ref or free ec as well */ > + return 1; > + > +err: > + if (privkey) > + { > + EVP_PKEY_free(privkey); > + } > + else if (ec) > + { > + EC_KEY_free(ec); > + } > + if (ec_method) /* do always set ec_method = NULL after freeing it */ > + { > + EC_KEY_METHOD_free(ec_method); > + ec_method = NULL; > + } > + return 0; > +} > + > +#endif /* OPENSSL_VERSION_NUMBER >= 1.1.0 */ > + > static const CERT_CONTEXT * > find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) > { > @@ -642,9 +841,18 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) > goto err; > } > } > +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L) && !defined(OPENSSL_NO_EC) > + else if (EVP_PKEY_id(pkey) == EVP_PKEY_EC) > + { > + if (!ssl_ctx_set_eckey(ssl_ctx, cd, pkey)) > + { > + goto err; > + } > + } > +#endif /* OPENSSL_VERSION_NUMBER >= 1.1.0 */ > else > { > - msg(M_WARN, "cryptoapicert requires an RSA certificate"); > + msg(M_WARN, "WARNING: cryptoapicert: certificate type not supported"); > goto err; > } > CAPI_DATA_free(cd); /* this will do a ref_count-- */ > Thanks, changes wrt v2 look good, so: Acked-by: Steffan Karger <st...@ka...> -Steffan 

Hi, On Tue, Feb 20, 2018 at 2:59 PM, Gert Doering <ge...@gr...> wrote: > Hi, > > On Tue, Feb 20, 2018 at 11:19:23AM -0500, Selva Nair wrote: >> Hmm.. I thought I had tested the patch.. Looking at it right now -- hold on :) > > Missed you on IRC... > > 18:21 < selvanair> cron2: iservives in 2.4 local build and snapshot build works > on Windows 10. Have to go now, but will test Win7 later > today when I can get hold of a one. > 18:22 < selvanair> s/iservives/iservice > > ... seems this is different on Win7...? > > To be sure it's not something messed up on my system, I have checked > the HKLM\Software\OpenVPN\(null) path that it's trying to access - and > all is there > > (standard) REG_SZ c:\Program Files\OpenVPN > config_dir REG_SZ c:\Program Files\OpenVPN\config > ... > > > and according to the documentation in > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms724868(v=vs.85).aspx > > passing "lpValue = NULL" really should work... the error code 0x57 > OTOH is documented as > > ERROR_INVALID_PARAMETER > 87 (0x57) > The parameter is incorrect. > > ... which isn't exactly helpful to see *which* argument it does not like > - everything *should* be according to specification... > > (It does happen for my own binary and for the installers Samuli builds, > so it's not "my build environment" - theoretically it could be part 1/2 > of that patch set, which I haven't run yet) Yeah, I could reproduce it on Windows 7. And, at last, after several painful Windows builds, it turns out: RegGetValue with RRF_RT_REG_EXPAND_SZ|RRF_RT_REG_SZ is broken on Windows 7 (and most likely Vista too). But once you know what to look for, just google: https://stackoverflow.com/questions/47096940/reggetvalue-for-value-that-may-be-either-reg-sz-or-reg-expand-sz Will send a patch later. Selva 

We're developing a new version of our client at ​Mullvad <https://mullvad.net/en/guides/beta-app/> and we're using a plugin to listen to events from OpenVPN. We have found however that we very much would like to receive an event when authentication fails. The patch at ​https://github.com/mullvad/openvpn/pull/1 basically copies the same event on the management interface side but sends it to plugins instead. Please note that I'm not a C developer and might be doing some very silly mistakes. I apologize in advance for any glaring faults and humbly accept any feedback you might have. Cheers! 

Hi, On Sun, Feb 25, 2018 at 12:13:10AM -0500, Selva Nair wrote: > Sourceforge keeps rejecting my attempts to send a patch through > gmail.com as smtp server. Something like "this message scored x.y. > Congratulations!" is the response from mx.sourceforge.net. Tripping > some spam filter? I had to send it via another server to get through. > > This started only a couple of days ago, but was only a temporary issue > in the beginning. Now it just fails repeatedly. Anyone else has seen > this? I haven't seen it, but it might be related to their datacenter move (which they did last week, and which caused the big mailing list outage because the list relays moved to new IP addresses which became grey- and blacklisted all over the place, due to the sudden high volume of mail sent...). Unfortunately, I do not think we have any influence on their spam scoring (except send mail to their support... sfn...@sl...) gert -- now what should I write here... Gert Doering - Munich, Germany ge...@gr...