[Openvpn-devel] [PATCH v2] Document that auth-user-pass may be inlined
From: Selva Nair Commits 7d48d31b, 39619b7f added support for inlining username and, optionally, password. Add a description of its usage in the man page. Github: resolves OpenVPN/openvpn#370 Change-Id: I7a1765661f7676eeba8016024080fd1026220ced Signed-off-by: Selva Nair --- v2: Add '--' prefix when referring to auth-user-pass and mention related github issue doc/man-sections/client-options.rst | 11 +++ doc/man-sections/inline-files.rst | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst index b92b1a46..b75fe5bd 100644 --- a/doc/man-sections/client-options.rst +++ b/doc/man-sections/client-options.rst @@ -73,6 +73,17 @@ configuration. If ``up`` is omitted, username/password will be prompted from the console. + This option can also be inlined + :: + + +username +[password] + + + where password is optional, and will be prompted from the console if + missing. + The server configuration must specify an ``--auth-user-pass-verify`` script to verify the username/password provided by the client. diff --git a/doc/man-sections/inline-files.rst b/doc/man-sections/inline-files.rst index f46301e8..4dba73c9 100644 --- a/doc/man-sections/inline-files.rst +++ b/doc/man-sections/inline-files.rst @@ -5,7 +5,7 @@ OpenVPN allows including files in the main configuration for the ``--ca``, ``--cert``, ``--dh``, ``--extra-certs``, ``--key``, ``--pkcs12``, ``--crl-verify``, ``--http-proxy-user-pass``, ``--tls-auth``, ``--auth-gen-token-secret``, ``--peer-fingerprint``, ``--tls-crypt``, -``--tls-crypt-v2`` and ``--verify-hash`` options. +``--tls-crypt-v2``, ``--verify-hash`` and ``--auth-user-pass`` options. Each inline file started by the line and ended by the line -- 2.34.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Document that auth-user-pass may be inlined
From: Selva Nair Commits 7d48d31b, 39619b7f added support for inlining username and, optionally, password. Add a description of its usage in the man page. Change-Id: I7a1765661f7676eeba8016024080fd1026220ced Signed-off-by: Selva Nair --- Does this have to go through gerrit? doc/man-sections/client-options.rst | 11 +++ doc/man-sections/inline-files.rst | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/doc/man-sections/client-options.rst b/doc/man-sections/client-options.rst index b92b1a46..b75fe5bd 100644 --- a/doc/man-sections/client-options.rst +++ b/doc/man-sections/client-options.rst @@ -73,6 +73,17 @@ configuration. If ``up`` is omitted, username/password will be prompted from the console. + This option can also be inlined + :: + + +username +[password] + + + where password is optional, and will be prompted from the console if + missing. + The server configuration must specify an ``--auth-user-pass-verify`` script to verify the username/password provided by the client. diff --git a/doc/man-sections/inline-files.rst b/doc/man-sections/inline-files.rst index f46301e8..ad02c855 100644 --- a/doc/man-sections/inline-files.rst +++ b/doc/man-sections/inline-files.rst @@ -5,7 +5,7 @@ OpenVPN allows including files in the main configuration for the ``--ca``, ``--cert``, ``--dh``, ``--extra-certs``, ``--key``, ``--pkcs12``, ``--crl-verify``, ``--http-proxy-user-pass``, ``--tls-auth``, ``--auth-gen-token-secret``, ``--peer-fingerprint``, ``--tls-crypt``, -``--tls-crypt-v2`` and ``--verify-hash`` options. +``--tls-crypt-v2``, ``--verify-hash`` and ``auth-user-pass`` options. Each inline file started by the line and ended by the line -- 2.34.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] man: extend description for "dhcp-option DNS" on Windows
Hi, On Tue, Sep 5, 2023 at 5:41 PM Antonio Quartulli wrote: > From: Antonio Quartulli > > Add an important detail about the DNS configured via this option > to be an "interface-specific" DNS. This detail is important when > troubleshooting DNS issues since this logic will bypass the > routing table. > > Signed-off-by: Antonio Quartulli > --- > doc/man-sections/vpn-network-options.rst | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/doc/man-sections/vpn-network-options.rst > b/doc/man-sections/vpn-network-options.rst > index b7f33acd..17c7d4a2 100644 > --- a/doc/man-sections/vpn-network-options.rst > +++ b/doc/man-sections/vpn-network-options.rst > @@ -158,6 +158,12 @@ routing. > Set primary domain name server IPv4 or IPv6 address. > Repeat this option to set secondary DNS server addresses. > > +On Windows this option sets an "interface-specific" DNS, meaning > +that the system will try to reach it via the VPN interface, even if the routing table says otherwise. > If the wanted DNS is not > If the desired DNS server is not +expected to be reached via VPN, please don't use this option, but > please don't --> do not > +rather set the DNS manually (or via DHCP). > delete "rather" > + > Note: DNS IPv6 servers are currently set using netsh (the existing > DHCP code can only do IPv4 DHCP, and that protocol only permits > IPv4 addresses anywhere). The option will be put into the > That said, I'm not convinced this is the right way to document this. IMO, using "--dhcp-option DNS n.n.n" to set a DNS server not reachable through the tunnel is a misuse of that option on any platform. Even on "non-windows" one shouldn't do that as it's very reasonable for a script that handles it to set it on the interface, instead of globally, if the platform permits it. For example, using systemd-resolved on Linux (though I'm not totally sure how exactly interface-specific DNS works in this case). So, if at all, why not state that the DNS server specified here should be reachable through the tunnel irrespective of the platform? Regards, Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Log OpenSSL errors on failure to set certificate
From: Selva Nair
Currently we log a bogus error message saying private key password verification
failed when SSL_CTX_use_cert_and_key() fails in pkcs11_openssl.c. Instead print
OpenSSL error queue and exit promptly.
Also log OpenSSL errors when SSL_CTX_use_certiifcate() fails in cryptoapi.c
and elsewhere. Such logging could be useful especially when the ceritficate is
rejected by OpenSSL due to stricter security restrictions in recent versions
of the library.
Change-Id: Ic7ec25ac0503a91d5869b8da966d0065f264af22
Signed-off-by: Selva Nair
---
src/openvpn/cryptoapi.c | 2 ++
src/openvpn/pkcs11_openssl.c | 6 --
src/openvpn/ssl_openssl.c | 2 ++
tests/unit_tests/openvpn/test_cryptoapi.c | 11 +++
tests/unit_tests/openvpn/test_pkcs11.c| 11 +++
5 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 3b92e481..f7e5b674 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -51,6 +51,7 @@
#include "openssl_compat.h"
#include "win32.h"
#include "xkey_common.h"
+#include "crypto_openssl.h"
#ifndef HAVE_XKEY_PROVIDER
@@ -505,6 +506,7 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const
char *cert_prop)
if (SSL_CTX_use_certificate(ssl_ctx, cert)
&& SSL_CTX_use_PrivateKey(ssl_ctx, privkey))
{
+crypto_print_openssl_errors(M_WARN);
ret = 1;
}
diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index 40080efa..aa0819f9 100644
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -302,7 +302,8 @@ xkey_load_from_pkcs11h(pkcs11h_certificate_t certificate,
if (!SSL_CTX_use_cert_and_key(ctx->ctx, x509, pkey, NULL, 0))
{
-msg(M_WARN, "PKCS#11: Failed to set cert and private key for OpenSSL");
+crypto_print_openssl_errors(M_WARN);
+msg(M_FATAL, "PKCS#11: Failed to set cert and private key for
OpenSSL");
goto cleanup;
}
ret = 1;
@@ -369,7 +370,8 @@ pkcs11_init_tls_session(pkcs11h_certificate_t certificate,
if (!SSL_CTX_use_certificate(ssl_ctx->ctx, x509))
{
-msg(M_WARN, "PKCS#11: Cannot set certificate for openssl");
+crypto_print_openssl_errors(M_WARN);
+msg(M_FATAL, "PKCS#11: Cannot set certificate for openssl");
goto cleanup;
}
ret = 0;
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 0b310de3..b5cc9a7f 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -857,6 +857,7 @@ tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char
*pkcs12_file,
/* Load Certificate */
if (!SSL_CTX_use_certificate(ctx->ctx, cert))
{
+crypto_print_openssl_errors(M_WARN);
crypto_msg(M_FATAL, "Cannot use certificate");
}
@@ -1007,6 +1008,7 @@ tls_ctx_load_cert_file(struct tls_root_ctx *ctx, const
char *cert_file,
end:
if (!ret)
{
+crypto_print_openssl_errors(M_WARN);
if (cert_file_inline)
{
crypto_msg(M_FATAL, "Cannot load inline certificate file");
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c
b/tests/unit_tests/openvpn/test_cryptoapi.c
index 008f41c0..d90bfc35 100644
--- a/tests/unit_tests/openvpn/test_cryptoapi.c
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -58,6 +58,17 @@ management_query_pk_sig(struct management *man, const char
*b64_data,
return NULL;
}
+/* replacement for crypto_print_openssl_errors() */
+void
+crypto_print_openssl_errors(const unsigned int flags)
+{
+unsigned long e;
+while ((e = ERR_get_error()))
+{
+msg(flags, "OpenSSL error %lu: %s\n", e, ERR_error_string(e, NULL));
+}
+}
+
/* tls_libctx is defined in ssl_openssl.c which we do not want to compile in */
OSSL_LIB_CTX *tls_libctx;
diff --git a/tests/unit_tests/openvpn/test_pkcs11.c
b/tests/unit_tests/openvpn/test_pkcs11.c
index 235cc43f..b6c130ec 100644
--- a/tests/unit_tests/openvpn/test_pkcs11.c
+++ b/tests/unit_tests/openvpn/test_pkcs11.c
@@ -44,6 +44,17 @@
struct management *management; /* global */
+/* replacement for crypto_print_openssl_errors() */
+void
+crypto_print_openssl_errors(const unsigned int flags)
+{
+unsigned long e;
+while ((e = ERR_get_error()))
+{
+msg(flags, "OpenSSL error %lu: %s\n", e, ERR_error_string(e, NULL));
+}
+}
+
/* stubs for some unused functions instead of pulling in too many dependencies
*/
int
parse_line(const char *line, char **p, const int n, const char *file,
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] pkcs11 config changes from 2.5.4 to 2.6.6 ?
> > > > > Good point. But, unless the config has "tls-cert-profile foo", we still > > default to legacy and call SSL_CTX_set_security_level(ctx, 1), isn't it? > > Wouldn't that allow SHA1 with 3.1.x ? > > For SHA1 you need security 0 aka tls-cert-profile insecure. > > But we might update OpenVPN to longer fiddle with the default security > level in OpenSSL 3+ to avoid downgrading security from 2 to 1 on OpenSSL > 3.1 > Even OpenSSL 3.0 is built with -DOPENSSL_TLS_SECURITY_LEVEL=2 in Debian and Ubuntu packages (and likely other distributions). Looks like a good idea to stop overriding it by default. We could also improve logging when SSL_CTX_set_certificate and similar fail by printing OpenSSL error queue. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] pkcs11 config changes from 2.5.4 to 2.6.6 ?
Hi Mike I misunderstood Arne's comment. We default to security level 1 but that forbids SHA1 signatures in OpenSSL 3.0+. Could you test with "tls-cert-profile Insecure" in the config file? It's not recommended but useful to check. Thanks, Selva On Thu, Sep 28, 2023 at 7:08 PM mike tancsa wrote: > Hi Selva, > > Thank you for looking! > > My guess is that something in the certificate or private key is not to > OpenSSL 3.1's liking and it rejects it. Is there any way for you to check > the > contents of the token independently using a tool linked against OpenSSL > 3.1 ? > > What am I looking for in that case ? Taking a look at the cert just with > openssl 3.0 on FreeBSD releng14 it seems ok with it. Same with the Windows > version 3.1.x that comes with OpenVPN. Is it possible it doesnt like the > sha1RSA sig ? > > # openssl version > OpenSSL 3.0.10 1 Aug 2023 (Library: OpenSSL 3.0.10 1 Aug 2023) > # > > Certificate: > Data: > Version: 3 (0x2) > Serial Number: 7109 (0x1bc5) > Signature Algorithm: sha1WithRSAEncryption > Issuer: C = CA, ST = ON, L = Cambridge, O = Sentex CA, CN = Sentex > private1test CA CA, emailAddress = m...@sentex.ca > Validity > Not Before: Sep 27 19:43:01 2023 GMT > Not After : Nov 13 19:43:01 2033 GMT > Subject: C = CA, ST = ON, L = Cambridge, O = Sentex CA, OU = > win10, CN = test123456mdt, emailAddress = m...@sentex.ca > Subject Public Key Info: > Public Key Algorithm: rsaEncryption > Public-Key: (2048 bit) > Modulus: > 00:f5:e0:27:b5:28:0a:f8:a9:ce:13:33:a2:ca:27: > > ... > > ac:a8:b6:55:bb:a3:a4:43:e5:74:05:aa:c8:69:3d: > ed:ef > Exponent: 65537 (0x10001) > X509v3 extensions: > X509v3 Basic Constraints: > CA:FALSE > Netscape Comment: > Easy-RSA Generated Certificate > X509v3 Subject Key Identifier: > 74:72:3A:87:0D:34:7B:1E:11:C6:18:D2:41:99:C6:5E:D1:8A:81:95 > X509v3 Authority Key Identifier: > > keyid:4F:A0:B0:94:92:6F:24:A7:D4:C6:93:A6:AA:25:63:6C:ED:1E:E3:8C > DirName:/C=CA/ST=ON/L=Cambridge/O=Sentex Parklands > CA/CN=Sentex Parklands CA CA/emailAddress=ppsupp...@sentex.ca > serial:F5:3E:37:76:69:AC:EF:EC > X509v3 Extended Key Usage: > TLS Web Client Authentication > X509v3 Key Usage: > Digital Signature > Signature Algorithm: sha1WithRSAEncryption > Signature Value: > 10:72:36:db:5c:f3:f5:fb:52:82:c7:4c:72:8f:31:ae: > ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] pkcs11 config changes from 2.5.4 to 2.6.6 ?
On Thu, Sep 28, 2023 at 8:55 PM Arne Schwabe wrote: > > Am 29.09.2023 um 01:08 schrieb mike tancsa: > > Hi Selva, > > Thank you for looking! > > My guess is that something in the certificate or private key is not to > OpenSSL 3.1's liking and it rejects it. Is there any way for you to check > the > contents of the token independently using a tool linked against OpenSSL > 3.1 ? > > What am I looking for in that case ? Taking a look at the cert just with > openssl 3.0 on FreeBSD releng14 it seems ok with it. Same with the Windows > version 3.1.x that comes with OpenVPN. Is it possible it doesnt like the > sha1RSA sig ? > > OpenSSL 3.0 has security 1 by default (OpenSSL 3.1 has 2 by default) and > that does not allow SHA1 signatures anymore. See > https://www.openssl.org/docs/man3.1/man3/SSL_CTX_set_security_level.html > Good point. But, unless the config has "tls-cert-profile foo", we still default to legacy and call SSL_CTX_set_security_level(ctx, 1), isn't it? Wouldn't that allow SHA1 with 3.1.x ? Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] pkcs11 config changes from 2.5.4 to 2.6.6 ?
Hi Mike, On Thu, Sep 28, 2023 at 5:28 PM mike tancsa wrote: > I am starting to test out 2.6.6 with a config that worked in 2.5.4 but > am getting a failure con connect. I did have a look through the > Changes.rst file but didnt see anything different ? The only pkcs11 bits > I have in the config are > > pkcs11-providers eTpkcs11.dll > pkcs11-id 'pkcs11:model=eToken;token=' > > and the same config works with the older version. Are there new > directives I need to add ? This is an Gemalto/Thales etoken. Again, it > works fine in this environment with the only change being the version of > OpenVPN. > The main change is upgrade to OpenSSL 3.x which seems not to like the certificate or key. Normally it should just work with no changes to the config. > > 2023-09-28 17:05:12 us=578000 PKCS#11: Failed to set cert and private > key for OpenSSL > This implies the call to OpenSSL's "SSL_CTX_set_cert_and_key()" failed. The certificate and private key handle from the token are acquired before this and set in xkey-provider --- both of those tasks have completed without errors. Very unusual and rare to error out at this point. Unfortunately we do not log the reason for this failure. Instead we clear OpenSSL's error queue and print a generic error saying private key password verification failed. A retry is triggered if "auth-retry" is set to "interact", else we exit as happened in your case. > 2023-09-28 17:05:12 us=578000 Error: private key password verification > failed > Not a very useful error message. My guess is that something in the certificate or private key is not to OpenSSL 3.1's liking and it rejects it. Is there any way for you to check the contents of the token independently using a tool linked against OpenSSL 3.1 ? Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] show extra info for OpenSSL errors
On Fri, Aug 11, 2023 at 8:16 AM Arne Schwabe wrote:
> This also shows the extra data from the OpenSSL error function that
> can contain extra information. For example, the command
>
> openvpn --providers vollbit
>
> will print out (on macOS):
>
> OpenSSL: error:12800067:DSO support routines::could not load the
> shared
> library:filename(/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib):
> dlopen(/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib,
> 0x0002): tried:
> '/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file),
> '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file),
> '/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file)
>
> Patch v2: Format message more like current messages
>
> Change-Id: Ic2ee89937dcd85721bcacd1b700a20c640364f80
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/crypto_openssl.c | 21 +++--
> src/openvpn/openssl_compat.h | 12
> 2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index b043bb95e..22c6d6840 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -238,9 +238,16 @@ void
> crypto_print_openssl_errors(const unsigned int flags)
> {
> unsigned long err = 0;
> +int line, errflags;
> +const char *file, *data, *func;
>
> -while ((err = ERR_get_error()))
> +while ((err = ERR_get_error_all(, , , ,
> )) != 0)
> {
> +if (!(errflags & ERR_TXT_STRING))
> +{
> +data = "";
> +}
> +
> /* Be more clear about frequently occurring "no shared cipher"
> error */
> if (ERR_GET_REASON(err) == SSL_R_NO_SHARED_CIPHER)
> {
> @@ -258,7 +265,17 @@ crypto_print_openssl_errors(const unsigned int flags)
> "tls-version-min 1.0 to the client configuration to use
> TLS 1.0+ "
> "instead of TLS 1.0 only");
> }
> -msg(flags, "OpenSSL: %s", ERR_error_string(err, NULL));
> +
> +/* print file and line if verb >=8 */
> +if (!check_debug_level(D_TLS_DEBUG_MED))
> +{
> +msg(flags, "OpenSSL: %s:%s", ERR_error_string(err, NULL),
> data);
> +}
> +else
> +{
> +msg(flags, "OpenSSL: %s:%s:%s:%d:%s", ERR_error_string(err,
> NULL),
> +data, file, line, func);
> +}
> }
> }
>
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index ffb64adf6..736ce1bd5 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -43,6 +43,7 @@
> #include
> #include
> #include
> +#include
>
> /* Functionality missing in 1.1.0 */
> #if OPENSSL_VERSION_NUMBER < 0x10101000L &&
> !defined(ENABLE_CRYPTO_WOLFSSL)
> @@ -799,6 +800,17 @@ EVP_MD_free(const EVP_MD *md)
> /* OpenSSL 1.1.1 and lower use only const EVP_MD, nothing to free */
> }
>
> +static inline unsigned long
> +ERR_get_error_all(const char **file, int *line,
> + const char **func,
> + const char **data, int *flags)
> +{
> +static const char *empty = "";
> +*func = empty;
> +long err = ERR_get_error_line_data(file, line, data, flags);
>
I think you missed to change that to "unsigned long err = "
+return err;
> +}
> +
> #endif /* OPENSSL_VERSION_NUMBER < 0x3000L */
>
> #endif /* OPENSSL_COMPAT_H_ */
> --
> 2.39.2 (Apple Git-143)
>
The above could be handled at merge time, so:
Acked-by: Selva Nair
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] signal_reset(): combine check and reset operations
On Tue, Jul 25, 2023 at 6:18 AM Frank Lichtenheld
wrote:
> On Sat, Jan 28, 2023 at 04:59:01PM -0500, selva.n...@gmail.com wrote:
> > From: Selva Nair
> >
> > - "if (sig == X) signal_reset(sig)" now becomes
> > "signal_reset(sig, X)" so that the check and assignment
> > can be done in one place where signals are masked.
> > This is required to avoid change of signal state between
> > check and reset operations.
> >
> > - Avoid resetting the signal except when absolutely necessary
> > (resetting has the potential of losing signals)
> >
> > - In 'pre_init_signal_catch()', when certain low priority signals
> > are set to SIG_IGN, clear any pending signals of the same
> > type. Also, reset signal at the end of the SIGUSR1 and
> > SIGHUP loops where their values are checked instead of later. This
> > avoids the need for 'signal_reset()' after SIGHUP or in
> 'init_instance()'
> > which could cause a signal like SIGTERM to be lost.
> >
> [...]
> > diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c
> > index cba58276..ad0aa8a2 100644
> > --- a/src/openvpn/openvpn.c
> > +++ b/src/openvpn/openvpn.c
> > @@ -194,7 +194,6 @@ openvpn_main(int argc, char *argv[])
> > context_clear_all_except_first_time();
> >
> > /* static signal info object */
> > -CLEAR(siginfo_static);
> > c.sig = _static;
>
> Is that actually save? Doesn't that mean that siginfo_static might be
> used uninitialized?
>
siginfo_static is a global variable with no explicit initialization, so it
is guaranteed to
get initialized to {0} at start up.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH applied] Re: Improve signal handling using POSIX sigaction
On Sun, Jul 23, 2023 at 5:28 AM Gert Doering wrote: > > The actual "change to POSIX ways" part of this is fairly trivial > and easy to understand :-) - though I do wonder why you're using > an extra variable for block_mask -> sa.sa_mask, and not using > sigfillset(_mask) - at least on BSD on Linux, both are sigset_t, > so it should do the same thing in a slightly more direct way. > I can't recall any particular reason for having that intermediate variable. It does look redundant. Now I wish someone could review 2/2 of this patch-set. That's where further potential loss of signal during signal-reset is avoided. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] show extra info for OpenSSL errors
Hi,
This looks good except that the format of the log could be kept closer to
the current one:
On Fri, Jul 7, 2023 at 2:59 PM Arne Schwabe wrote:
> This also shows the extra data from the OpenSSL error function that
> can contain extra information. For example, the command
>
> openvpn --providers vollbit
>
> will print out (on macOS):
>
> OpenSSLerror:12800067:DSO support routines::could not load the shared
> library:filename(/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib):
> dlopen(/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib,
> 0x0002): tried:
> '/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file),
> '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file),
> '/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file)
>
> Change-Id: Ic2ee89937dcd85721bcacd1b700a20c640364f80
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/crypto_openssl.c | 20 ++--
> src/openvpn/openssl_compat.h | 12
> 2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index b043bb95e..d6916fc9b 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -238,9 +238,16 @@ void
> crypto_print_openssl_errors(const unsigned int flags)
> {
> unsigned long err = 0;
> +int line, errflags;
> +const char *file, *data, *func;
>
> -while ((err = ERR_get_error()))
> +while ((err = ERR_get_error_all(, , , ,
> )) != 0)
> {
> +if (!(errflags & ERR_TXT_STRING))
> +{
> +data = "";
> +}
> +
> /* Be more clear about frequently occurring "no shared cipher"
> error */
> if (ERR_GET_REASON(err) == SSL_R_NO_SHARED_CIPHER)
> {
> @@ -258,7 +265,16 @@ crypto_print_openssl_errors(const unsigned int flags)
> "tls-version-min 1.0 to the client configuration to use
> TLS 1.0+ "
> "instead of TLS 1.0 only");
> }
> -msg(flags, "OpenSSL: %s", ERR_error_string(err, NULL));
> +
> +/* print file and line if verb >=8 */
> +if (!check_debug_level(D_TLS_DEBUG_MED))
> +{
> +msg(flags, "OpenSSL%s:%s", ERR_error_string(err, NULL), data);
>
Better to retain the ':" after "OpenSSL" in case anyone is parsing the logs
msg(flags, "OpenSSL: %s:%s, ERR_error_string(err, NULL), data);
> +}
> +else
> +{
> +msg(flags, "OpenSSL (%s:%d %s): %s:%s", file, line, func,
> ERR_error_string(err, NULL), data);
>
Again, I think we could keep the same order as above with additional info
(file:line:func) moved to the end with a consistent use of the delimiter ':'
msg(flags, "OpenSSL: %s:%s:%s:%s:%s, ERR_error_string(err, NULL), data,
file, line, func);
> +}
> }
> }
>
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index ffb64adf6..736ce1bd5 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -43,6 +43,7 @@
> #include
> #include
> #include
> +#include
>
> /* Functionality missing in 1.1.0 */
> #if OPENSSL_VERSION_NUMBER < 0x10101000L &&
> !defined(ENABLE_CRYPTO_WOLFSSL)
> @@ -799,6 +800,17 @@ EVP_MD_free(const EVP_MD *md)
> /* OpenSSL 1.1.1 and lower use only const EVP_MD, nothing to free */
> }
>
> +static inline unsigned long
> +ERR_get_error_all(const char **file, int *line,
> + const char **func,
> + const char **data, int *flags)
> +{
> +static const char *empty = "";
> +*func = empty;
> +long err = ERR_get_error_line_data(file, line, data, flags);
>
unsigned long err =
> +return err;
> +}
> +
> #endif /* OPENSSL_VERSION_NUMBER < 0x3000L */
>
> #endif /* OPENSSL_COMPAT_H_ */
> --
>
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] Fwd: [PATCH] tun.c: enclose DNS domain in single quotes in WMIC call
Hi,
On Mon, Jul 10, 2023 at 7:22 AM Lev Stipakov wrote:
> From: Lev Stipakov
>
> This is needed to support domains with hyphens.
>
> Not using double quotes here, since our code replaces
> them with underbars (see
> https://github.com/OpenVPN/openvpn/blob/master/src/openvpn/win32.c#L980).
>
> Fixes https://github.com/OpenVPN/openvpn/issues/363
>
> Change-Id: Iab536922d0731635cef529b5caf542f637b8d491
> Signed-off-by: Lev Stipakov
> ---
> src/openvpn/tun.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index d1fd6def..60974208 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -333,7 +333,7 @@ do_dns_domain_wmic(bool add, const struct tuntap *tt)
> }
>
> struct argv argv = argv_new();
> -argv_printf(, "%s%s nicconfig where (InterfaceIndex=%ld) call
> SetDNSDomain %s",
> +argv_printf(, "%s%s nicconfig where (InterfaceIndex=%ld) call
> SetDNSDomain '%s'",
> get_win_sys_path(), WMIC_PATH_SUFFIX, tt->adapter_index,
> add ? tt->options.domain : "");
> exec_command("WMIC", , 1, M_WARN);
>
Quoting is required as wmic interprets characters such as hyphen and /.
Double quotes would have been better (as in interactive.c) as there are
some cases where characters within single quotes get interpreted special
(like 'foo>bar' vs "foo>bar").
That said, for valid domain names, the only expected characters are
alpha-numeric, hyphen and period, and single quotes should work. I have
only tested this using wmic command line, not the resulting openvpn.exe.
Acked-by: Selva Nair
P.S.
We probably need to sanitize the user-supplied domain name before passing
it to wmic.
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 2/2] Fix CR_RESPONSE mangaement message using wrong key_id
It seems I missed to send this ACK to the list.. Here it is.
On Mon, May 22, 2023 at 6:12 AM Arne Schwabe wrote:
> the management interface expects the management key id instead
> of the openvpn key id. In the past they often were the same for low ids
> which hid the bug quite well.
>
> Also do not pick uninitialised keystates (management key_id is not valid
> in these).
>
> Patch v2: do not add logging
>
> Change-Id: If9fa1165a0e886b570b3738546ed810a32367cbe
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/push.c | 4 ++--
> src/openvpn/ssl_common.h | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 8e9627199..8f0a534ac 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -267,9 +267,9 @@ receive_cr_response(struct context *c, const struct
> buffer *buffer)
> struct tls_session *session = >c2.tls_multi->session[TM_ACTIVE];
> struct man_def_auth_context *mda = session->opt->mda_context;
> struct env_set *es = session->opt->es;
> -int key_id = get_primary_key(c->c2.tls_multi)->key_id;
> +unsigned int mda_key_id =
> get_primary_key(c->c2.tls_multi)->mda_key_id;
>
> -management_notify_client_cr_response(key_id, mda, es, m);
> +management_notify_client_cr_response(mda_key_id, mda, es, m);
> #endif
> #if ENABLE_PLUGIN
> verify_crresponse_plugin(c->c2.tls_multi, m);
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index ebfd25432..be0f18746 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -733,7 +733,7 @@ get_key_by_management_key_id(struct tls_multi *multi,
> unsigned int mda_key_id)
> for (int i = 0; i < KEY_SCAN_SIZE; ++i)
> {
> struct key_state *ks = get_key_scan(multi, i);
> -if (ks->mda_key_id == mda_key_id)
> +if (ks->mda_key_id == mda_key_id && ks->state > S_UNDEF)
> {
> return ks;
> }
> --
> 2.39.2 (Apple Git-143)
>
>
Acked-by Selva Nair
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/2] Improve signal handling using POSIX sigaction
On Mon, May 29, 2023 at 3:07 PM Gert Doering wrote: > Hi, > > On Thu, May 25, 2023 at 02:41:10PM -0400, Selva Nair wrote: > > Now that 2.6 appears to have reached a fairly stable state, may I request > > you to look into this patch for 2.7 -- this one has an ACK (thanks to > > Frank), 2/2 may need a closer look but that one is small. > > > > I dread the prospect of this developing serious merge conflicts and > having > > to drill down into the details to resolve them. Right now it looks like > no > > one has yet touched related chunks. > > Will not really have much available time next week, but I have heard > the message - will look into this the week after. If not, please feel > free to send me another reminder. > Here it is :) Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/2] Improve signal handling using POSIX sigaction
Hi Gert, Now that 2.6 appears to have reached a fairly stable state, may I request you to look into this patch for 2.7 -- this one has an ACK (thanks to Frank), 2/2 may need a closer look but that one is small. I dread the prospect of this developing serious merge conflicts and having to drill down into the details to resolve them. Right now it looks like no one has yet touched related chunks. Thanks, Selva On Tue, Jan 31, 2023 at 5:48 AM Frank Lichtenheld wrote: > On Sat, Jan 28, 2023 at 04:59:00PM -0500, selva.n...@gmail.com wrote: > > From: Selva Nair > > > > Currently we use the old signal API which follows system-V or > > BSD semantics depending on the platform and/or feature-set macros. > > Further, signal has many weaknesses which makes proper masking > > (deferring) of signals during update not possible. > > > > Improve this: > > > > - Use sigaction to properly mask signals when modifying. > > > > Acked-By: Frank Lichtenheld > > Stared at code intensively. AFAICT this should not change the > general behavior except to be more generally safe. > > Only compile+t_client tested. > > Regards, > -- > Frank Lichtenheld > ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] tapctl: generate driver-specific adapter names
Acked-by: Selva Nair
On Fri, May 19, 2023 at 4:27 AM Lev Stipakov wrote:
> From: Lev Stipakov
>
> At the moment if --name is not specified, adapter names
> are generated by Windows and they look a bit confusing
> like "Local Area Connection 2".
>
> This is also behavior of "Add a new virtual network
> adapter" shortcuts.
>
> This makes tapctl generate driver-specific names for adapters
> if --name is missing, inclusing resolving duplicates. For instance
> following commands:
>
> tapctl.exe create --hwid ovpn-dco
>
> will create an adapter named
>
> OpenVPN Data Channel Offload
>
> If the name is taken, the next one will be
>
> OpenVPN Data Channel Offload #1
>
> and so on up to 100.
>
> Fixes https://github.com/OpenVPN/openvpn/issues/337
>
> Change-Id: Ic5afb470d14ac7b231d91f0f5de0a0046043a7e0
> Signed-off-by: Lev Stipakov
> ---
> v3:
> - use _stprintf_s instead of _tcscat_s
> - make functions static
> - ensure iResult is always assigned
>
> v2: fix MinGW compilation
>
> src/tapctl/main.c | 132 --
> 1 file changed, 105 insertions(+), 27 deletions(-)
>
> diff --git a/src/tapctl/main.c b/src/tapctl/main.c
> index 1194036f..d76d553c 100644
> --- a/src/tapctl/main.c
> +++ b/src/tapctl/main.c
> @@ -126,6 +126,85 @@ usage(void)
>title_string);
> }
>
> +/**
> + * Checks if adapter with given name doesn't already exist
> + */
> +static BOOL
> +is_adapter_name_available(LPCTSTR name, struct tap_adapter_node
> *adapter_list, BOOL log)
> +{
> +for (struct tap_adapter_node *a = adapter_list; a; a = a->pNext)
> +{
> +if (_tcsicmp(name, a->szName) == 0)
> +{
> +if (log)
> +{
> +LPOLESTR adapter_id = NULL;
> +StringFromIID((REFIID)>guid, _id);
> +_ftprintf(stderr, TEXT("Adapter \"%") TEXT(PRIsLPTSTR)
> TEXT("\" already exists (GUID %")
> + TEXT(PRIsLPOLESTR) TEXT(").\n"), a->szName,
> adapter_id);
> +CoTaskMemFree(adapter_id);
> +}
> +
> +return FALSE;
> +}
> +}
> +
> +return TRUE;
> +}
> +
> +/**
> + * Returns unique adapter name based on hwid or NULL if name cannot be
> generated.
> + * Caller is responsible for freeing it.
> + */
> +static LPTSTR
> +get_unique_adapter_name(LPCTSTR hwid, struct tap_adapter_node
> *adapter_list)
> +{
> +if (hwid == NULL)
> +{
> +return NULL;
> +}
> +
> +LPCTSTR base_name;
> +if (_tcsicmp(hwid, TEXT("ovpn-dco")) == 0)
> +{
> +base_name = TEXT("OpenVPN Data Channel Offload");
> +}
> +else if (_tcsicmp(hwid, TEXT("wintun")) == 0)
> +{
> +base_name = TEXT("OpenVPN Wintun");
> +}
> +else if (_tcsicmp(hwid, TEXT("root\\") TEXT(TAP_WIN_COMPONENT_ID)) ==
> 0)
> +{
> +base_name = TEXT("OpenVPN TAP-Windows6");
> +}
> +else
> +{
> +return NULL;
> +}
> +
> +if (is_adapter_name_available(base_name, adapter_list, FALSE))
> +{
> +return _tcsdup(base_name);
> +}
> +
> +size_t name_len = _tcslen(base_name) + 10;
> +LPTSTR name = malloc(name_len * sizeof(TCHAR));
> +if (name == NULL)
> +{
> +return NULL;
> +}
> +for (int i = 1; i < 100; ++i)
> +{
> +_stprintf_s(name, name_len, TEXT("%ls #%d"), base_name, i);
> +
> +if (is_adapter_name_available(name, adapter_list, FALSE))
> +{
> +return name;
> +}
> +}
> +
> +return NULL;
> +}
>
> /**
> * Program entry point
> @@ -210,50 +289,49 @@ _tmain(int argc, LPCTSTR argv[])
> iResult = 1; goto quit;
> }
>
> -if (szName)
> +/* Get existing network adapters. */
> +struct tap_adapter_node *pAdapterList = NULL;
> +dwResult = tap_list_adapters(NULL, NULL, );
> +if (dwResult != ERROR_SUCCESS)
> {
> -/* Get existing network adapters. */
> -struct tap_adapter_node *pAdapterList = NULL;
> -dwResult = tap_list_adapters(NULL, NULL, );
> -if (dwResult != ERROR_SUCCESS)
> -{
> -_ftprintf(stderr, TEXT("Enumerating adapters failed
> (error 0x%x).\n"), dwResult);
> -iResult = 1; goto create_delete_adapter;
> -
Re: [Openvpn-devel] [PATCH 2/2] Fix CR_RESPONSE mangaement message using wrong key_id
Hi,
While this bugfix should be merged, I'm a conflicted about the way these
two patches are split up. It just makes reviewing harder than it should be.
They actually form two independent changes but with one half intersecting
with the other for no reason.
On Wed, May 17, 2023 at 7:03 AM Arne Schwabe wrote:
> the management interface expects the management key id instead
> of the openvpn key id. In the past they often were the same for low ids
> which hid the bug quite well.
>
> Also do not pick uninitialised keystates (management key_id is not valid
> in these) and provide better logging when a key state is not found.
>
> Change-Id: If9fa1165a0e886b570b3738546ed810a32367cbe
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/push.c | 4 ++--
> src/openvpn/ssl_common.h | 2 +-
> src/openvpn/ssl_verify.c | 5 +
> 3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 8e9627199..8f0a534ac 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -267,9 +267,9 @@ receive_cr_response(struct context *c, const struct
> buffer *buffer)
> struct tls_session *session = >c2.tls_multi->session[TM_ACTIVE];
> struct man_def_auth_context *mda = session->opt->mda_context;
> struct env_set *es = session->opt->es;
> -int key_id = get_primary_key(c->c2.tls_multi)->key_id;
> +unsigned int mda_key_id =
> get_primary_key(c->c2.tls_multi)->mda_key_id;
>
> -management_notify_client_cr_response(key_id, mda, es, m);
> +management_notify_client_cr_response(mda_key_id, mda, es, m);
> #endif
> #if ENABLE_PLUGIN
verify_crresponse_plugin(c->c2.tls_multi, m);
>
I think, this patch could end here the bugfix is complete without the need
for 1/2.
The rest of the patch depends on 1/2, but is not required for the above. It
could be combined with 1/2 and submitted as a follow up refactoring patch.
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index ebfd25432..be0f18746 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -733,7 +733,7 @@ get_key_by_management_key_id(struct tls_multi *multi,
> unsigned int mda_key_id)
> for (int i = 0; i < KEY_SCAN_SIZE; ++i)
> {
> struct key_state *ks = get_key_scan(multi, i);
> -if (ks->mda_key_id == mda_key_id)
> +if (ks->mda_key_id == mda_key_id && ks->state > S_UNDEF)
> {
> return ks;
> }
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index 4c78c2b2c..567f55ea3 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -1279,6 +1279,11 @@ tls_authenticate_key(struct tls_multi *multi, const
> unsigned int mda_key_id, con
> {
> ks->mda_status = auth ? ACF_SUCCEEDED : ACF_FAILED;
> }
> +else
> +{
> +msg(D_TLS_DEBUG_LOW, "%s: no key state found for management
> key id "
> +"%d", __func__, mda_key_id);
> +}
> }
> return (bool) ks;
> }
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] tapctl: generate driver-specific adapter names
> +dwResult = tap_list_adapters(NULL, NULL, );
> +if (dwResult != ERROR_SUCCESS)
> {
> -/* Get existing network adapters. */
> -struct tap_adapter_node *pAdapterList = NULL;
> -dwResult = tap_list_adapters(NULL, NULL, );
> -if (dwResult != ERROR_SUCCESS)
> -{
> -_ftprintf(stderr, TEXT("Enumerating adapters failed
> (error 0x%x).\n"), dwResult);
> -iResult = 1; goto create_delete_adapter;
> -}
> +_ftprintf(stderr, TEXT("Enumerating adapters failed (error
> 0x%x).\n"), dwResult);
> +iResult = 1; goto create_delete_adapter;
>
Add a line-break to avoid multiple statements per line? I know it's as in
the original, but we can "fix" at least the contexts that are touched..
> +}
>
> -/* Check for duplicates. */
> -for (struct tap_adapter_node *pAdapter = pAdapterList;
> pAdapter; pAdapter = pAdapter->pNext)
> +LPTSTR adapter_name = szName ? _tcsdup(szName) :
> get_unique_adapter_name(szHwId, pAdapterList);
> +if (adapter_name)
> +{
> +/* Check for duplicates when name was specified,
> + * otherwise get_adapter_default_name() takes care of it */
> +if (szName && !is_adapter_name_available(adapter_name,
> pAdapterList, TRUE))
> {
> -if (_tcsicmp(szName, pAdapter->szName) == 0)
> -{
> -StringFromIID((REFIID)>guid, );
> -_ftprintf(stderr, TEXT("Adapter \"%")
> TEXT(PRIsLPTSTR) TEXT("\" already exists (GUID %")
> - TEXT(PRIsLPOLESTR) TEXT(").\n"),
> pAdapter->szName, szAdapterId);
> -CoTaskMemFree(szAdapterId);
> -iResult = 1; goto create_cleanup_pAdapterList;
> -}
> +iResult = 1; goto create_cleanup_pAdapterList;
>
same here..
> }
>
> /* Rename the adapter. */
> -dwResult = tap_set_adapter_name(, szName, FALSE);
> +dwResult = tap_set_adapter_name(, adapter_name,
> FALSE);
> if (dwResult != ERROR_SUCCESS)
> {
> StringFromIID((REFIID), );
> _ftprintf(stderr, TEXT("Renaming TUN/TAP adapter %")
> TEXT(PRIsLPOLESTR)
>TEXT(" to \"%") TEXT(PRIsLPTSTR) TEXT("\"
> failed (error 0x%x).\n"),
> - szAdapterId, szName, dwResult);
> + szAdapterId, adapter_name, dwResult);
> CoTaskMemFree(szAdapterId);
> iResult = 1; goto quit;
>
..
> }
>
> iResult = 0;
> +}
>
If (adapter_name) is false, we reach here with iResult not set, but it gets
referenced below. Add an else { iResult = 1; } or initialize iResult to 1
at top?
> create_cleanup_pAdapterList:
> -tap_free_adapter_list(pAdapterList);
> -if (iResult)
> -{
> -goto create_delete_adapter;
> -}
> +free(adapter_name);
> +
> +tap_free_adapter_list(pAdapterList);
> +if (iResult)
> +{
> +goto create_delete_adapter;
> }
>
> /* Output adapter GUID. */
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Interactive service: do not force a target desktop for openvpn.exe
From: Selva Nair Setting the desktop as "winsta0\default" does not always work when run from a non-interactive session which may not have access to the the window station "Winsta0". Leave this as NULL to let the system automatically assign a window station and desktop. Test runs on Win10 confirm that "Winsta0\Default" still gets selected when run interactively (e.g., using the GUI or from task scheduler as an interactive job). This is the same behaviour as now. The change allows "interactive service" to be used for launching OpenVPN from non-interactive sessions. For example, when service client is a non-interactive task from the task scheduler, the default desktop in a custom window station gets assigned to openvpn.exe. Note that we already run openvpn.exe in a non-interactive window station when directly launched by "automatic service". Github: Fixes OpenVPN/openvpn-gui#626 Signed-off-by: Selva Nair --- src/openvpnserv/interactive.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index ec196274..d73cef04 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -1868,7 +1868,6 @@ RunOpenvpn(LPVOID p) } startup_info.cb = sizeof(startup_info); -startup_info.lpDesktop = L"winsta0\\default"; startup_info.dwFlags = STARTF_USESTDHANDLES; startup_info.hStdInput = stdin_read; startup_info.hStdOutput = stdout_write; -- 2.34.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix CR_RESPONSE mangaement message using wrong key_id
Hi, Is this dependent on some patch not yet merged? See missing context below. On Tue, May 16, 2023 at 12:36 PM Arne Schwabe wrote: > the management interface expects the management key id instead > of the openvpn key id. In the past they often were the same for low ids > which hid the bug quite well. > > Also do not pick uninitialised keystates (management key_id is not valid > in these) and provide better logging when a key state is not found. > > Change-Id: If9fa1165a0e886b570b3738546ed810a32367cbe > Signed-off-by: Arne Schwabe > --- > src/openvpn/push.c | 4 ++-- > src/openvpn/ssl_common.h | 2 +- > src/openvpn/ssl_verify.c | 5 + > 3 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/src/openvpn/push.c b/src/openvpn/push.c > index 54e53f6aa..99abe5440 100644 > --- a/src/openvpn/push.c > +++ b/src/openvpn/push.c > @@ -267,9 +267,9 @@ receive_cr_response(struct context *c, const struct > buffer *buffer) > struct tls_session *session = >c2.tls_multi->session[TM_ACTIVE]; > struct man_def_auth_context *mda = session->opt->mda_context; > struct env_set *es = session->opt->es; > -int key_id = get_primary_key(c->c2.tls_multi)->key_id; > +unsigned int mda_key_id = > get_primary_key(c->c2.tls_multi)->mda_key_id; > > -management_notify_client_cr_response(key_id, mda, es, m); > +management_notify_client_cr_response(mda_key_id, mda, es, m); > #endif > #if ENABLE_PLUGIN > verify_crresponse_plugin(c->c2.tls_multi, m); > diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h > index ebfd25432..be0f18746 100644 > --- a/src/openvpn/ssl_common.h > +++ b/src/openvpn/ssl_common.h > @@ -733,7 +733,7 @@ get_key_by_management_key_id(struct tls_multi *multi, > unsigned int mda_key_id) > ssl_common.h in master (and 2.6) has only 725 lines and no function by that name. Am I missing something? Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Correctly handle Unicode names for exit event
From: Selva Nair
Currently we use the ANSI version of CreateEvent causing name of the
exit event to be interpreted differently depending on the code page
in effect. Internally all strings parsed from command line and config
file are stored as UTF8-encoded Uniode. When passed to Windows API calls,
these should be converted to UTF16 and wide character version of the API
should be used.
CreateEvent calls for unnamed events are left unchanged as there is no
text-encoding dependence in those cases.
Signed-off-by: Selva Nair
---
src/openvpn/win32.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 1ae3723f..25da54ab 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -509,19 +509,19 @@ win32_signal_open(struct win32_signal *ws,
&& !HANDLE_DEFINED(ws->in.read) && exit_event_name)
{
struct security_attributes sa;
+struct gc_arena gc = gc_new();
+const wchar_t *exit_event_nameW = wide_string(exit_event_name, );
if (!init_security_attributes_allow_all())
{
msg(M_ERR, "Error: win32_signal_open: init SA failed");
}
-ws->in.read = CreateEvent(,
- TRUE,
- exit_event_initial_state ? TRUE : FALSE,
- exit_event_name);
+ws->in.read = CreateEventW(, TRUE, exit_event_initial_state ?
TRUE : FALSE,
+ exit_event_nameW);
if (ws->in.read == NULL)
{
-msg(M_WARN|M_ERRNO, "NOTE: CreateEvent '%s' failed",
exit_event_name);
+msg(M_WARN|M_ERRNO, "NOTE: CreateEventW '%s' failed",
exit_event_name);
}
else
{
@@ -534,6 +534,7 @@ win32_signal_open(struct win32_signal *ws,
ws->mode = WSO_MODE_SERVICE;
}
}
+gc_free();
}
/* set the ctrl handler in both console and service modes */
if (!SetConsoleCtrlHandler((PHANDLER_ROUTINE) win_ctrl_handler, true))
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Bugfix: dangling pointer passed to pkcs11-helper
From: Selva Nair
Github: Fixes OpenVPN/openvpn#323
Signed-off-by: Selva Nair
---
This will fix #323 is my best guess, untested as yet..
This is a bug that needs fixing, regardless.
src/openvpn/pkcs11_openssl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index eee86e17..9b0ab39f 100644
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -165,6 +165,7 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
{
pkcs11h_certificate_t cert = handle;
CK_MECHANISM mech = {CKM_RSA_PKCS, NULL, 0}; /* default value */
+CK_RSA_PKCS_PSS_PARAMS pss_params = {0};
unsigned char buf[EVP_MAX_MD_SIZE];
size_t buflen;
@@ -203,7 +204,6 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
}
else if (!strcmp(sigalg.padmode, "pss"))
{
-CK_RSA_PKCS_PSS_PARAMS pss_params = {0};
mech.mechanism = CKM_RSA_PKCS_PSS;
if (!set_pss_params(_params, sigalg, cert))
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Add Apache2 linking with for new commits
Hi, On Tue, Apr 25, 2023 at 6:22 AM Arne Schwabe wrote: > After first round of mailing people with more than 10 commits we have > almost all committers have agreed. This put this license in the realm > of having a realistic change to work. Had any of these contributers > disagreed, rewriting all their code might have been not feasible. > > The rationale of adding this exception now is to avoid having to > have a second round of agreement for new contributers and ensure > that all new code will include the exemption. > > patch v2: add explaination and use exception rather than excemption > looks like a wrong version of patch below ? > Change-Id: Ide83f914f383b53ef37ddf628e4da5a78e241bf0 > Signed-off-by: Arne Schwabe > --- > COPYING | 23 +++ > 1 file changed, 23 insertions(+) > > diff --git a/COPYING b/COPYING > index e12c51414..b4a148494 100644 > --- a/COPYING > +++ b/COPYING > @@ -31,6 +31,29 @@ OpenVPN license: >file, but you are not obligated to do so. If you do not wish to >do so, delete this exception statement from your version. > > +Apache2 linking exemption: > --> exception > +--- > +OpenVPN is currently undergoing a license change to add an exemption for > --> exception > +Apache 2 linking. The following exemption is only valid for new > contributions > --> exception > +after COMMITDATE and past contribution where the authors have already > agreed > +to the exemption. > --> exception > + > + In addition, as a special exception, OpenVPN Inc and the > + contributors give permission to link the code of this program to > + libraries (the "Libraries") licensed under the Apache License > + version 2.0 (this work and any linked library the "Combined Work") > + and copy and distribute the Combined Work without an obligation to > + license the Libraries under the GNU General Public License v2 > + (GPL-2.0) as required by Section 2 of the GPL-2.0, and without an > + obligation to refrain from imposing any additional restrictions in > + the Apache License version 2 that are not in the GPL-2.0, as > + required by Section 6 of the GPL-2.0. You must comply with the > + GPL-2.0 in all other respects for the Combined Work, including > + the obligation to provide source code. If you modify this file, you > + may extend this exception to your version of the file, but you are > + not obligated to do so. If you do not wish to do so, delete this > + exception statement from your version. > + > LZO license: > > > -- > 2.39.2 (Apple Git-143) > > > > ___ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel > ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Add Apache2 linking with for new commits
exemption -> exception in a number of places below Though similar in meaning, better to use the standard wording here. On Fri, Apr 21, 2023 at 11:02 AM Arne Schwabe wrote: > After first round of mailing people with more than 10 commits we have > almost all committers have agreed. This put this license in the realm > of having a realistic change to work. Had any of these contributers > disagreed, rewriting all their code might have been not feasible. > > The rationale of adding this exception now is to avoid having to > have a second round of agreement for new contributers and ensure > that all new code will include the exemption. > Change-Id: Ide83f914f383b53ef37ddf628e4da5a78e241bf0 > Signed-off-by: Arne Schwabe > --- > COPYING | 23 +++ > 1 file changed, 23 insertions(+) > > diff --git a/COPYING b/COPYING > index e12c51414..b4a148494 100644 > --- a/COPYING > +++ b/COPYING > @@ -31,6 +31,29 @@ OpenVPN license: >file, but you are not obligated to do so. If you do not wish to >do so, delete this exception statement from your version. > > +Apache2 linking exemption: > +--- > +OpenVPN is currently undergoing a license change to add an exemption for > +Apache 2 linking. The following exemption is only valid for new > contributions > +after COMMITDATE and past contribution where the authors have already > agreed > +to the exemption. > + > + In addition, as a special exception, OpenVPN Inc and the > + contributors give permission to link the code of this program to > + libraries (the "Libraries") licensed under the Apache License > + version 2.0 (this work and any linked library the "Combined Work") > + and copy and distribute the Combined Work without an obligation to > + license the Libraries under the GNU General Public License v2 > + (GPL-2.0) as required by Section 2 of the GPL-2.0, and without an > + obligation to refrain from imposing any additional restrictions in > + the Apache License version 2 that are not in the GPL-2.0, as > + required by Section 6 of the GPL-2.0. You must comply with the > + GPL-2.0 in all other respects for the Combined Work, including > + the obligation to provide source code. If you modify this file, you > + may extend this exception to your version of the file, but you are > + not obligated to do so. If you do not wish to do so, delete this > + exception statement from your version. > + > LZO license: > > > -- > 2.39.2 (Apple Git-143) > > > > ___ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel > ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2] Format Windows error message in Unicode
From: Selva Nair
- We assume that all text passed to the management interface
and written to log file are in Unicode (UTF-8). This is broken by
the use of the ANSI version of FormatMessage() for Windows error
messages. Fix by using FormatMessageW() and converting the UTF-16
result to UTF-8.
v2: assign return value of FormatMessageW() to DWORD, not int
Github: fixes OpenVPN/openvpn#319
Signed-off-by: Selva Nair
---
src/openvpn/error.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/openvpn/error.c b/src/openvpn/error.c
index a2c9aa4c..9a234e67 100644
--- a/src/openvpn/error.c
+++ b/src/openvpn/error.c
@@ -970,19 +970,24 @@ strerror_win32(DWORD errnum, struct gc_arena *gc)
/* format a windows error message */
{
-char message[256];
+wchar_t wmessage[256];
+char *message = NULL;
struct buffer out = alloc_buf_gc(256, gc);
-const int status = FormatMessage(
+const DWORD status = FormatMessageW(
FORMAT_MESSAGE_IGNORE_INSERTS
| FORMAT_MESSAGE_FROM_SYSTEM
| FORMAT_MESSAGE_ARGUMENT_ARRAY,
NULL,
errnum,
0,
-message,
-sizeof(message),
+wmessage,
+SIZE(wmessage),
NULL);
-if (!status)
+if (status)
+{
+message = utf16to8(wmessage, gc);
+}
+if (!status || !message)
{
buf_printf(, "[Unknown Win32 Error]");
}
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Format Windows error message in Unicode
From: Selva Nair
- We assume that all text passed to the management interface
and written to log file are in Unicode (UTF-8). This is broken by
the use of the ANSI version of FormatMessage() for Windows error
messages. Fix by using FormatMessageW() and converting the UTF-16
result to UTF-8.
Github: fixes OpenVPN/openvpn#319
Signed-off-by: Selva Nair
---
src/openvpn/error.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/openvpn/error.c b/src/openvpn/error.c
index a2c9aa4c..ef821fcd 100644
--- a/src/openvpn/error.c
+++ b/src/openvpn/error.c
@@ -970,19 +970,24 @@ strerror_win32(DWORD errnum, struct gc_arena *gc)
/* format a windows error message */
{
-char message[256];
+wchar_t wmessage[256];
+char *message = NULL;
struct buffer out = alloc_buf_gc(256, gc);
-const int status = FormatMessage(
+const int status = FormatMessageW(
FORMAT_MESSAGE_IGNORE_INSERTS
| FORMAT_MESSAGE_FROM_SYSTEM
| FORMAT_MESSAGE_ARGUMENT_ARRAY,
NULL,
errnum,
0,
-message,
-sizeof(message),
+wmessage,
+SIZE(wmessage),
NULL);
-if (!status)
+if (status)
+{
+message = utf16to8(wmessage, gc);
+}
+if (!status || !message)
{
buf_printf(, "[Unknown Win32 Error]");
}
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Support of DNS domain for DHCP-less drivers
tatus = openvpn_execve_check(a, NULL, 0, "ERROR: command
> failed");
> netcmd_semaphore_release();
> if (status)
> {
> @@ -5260,7 +5286,13 @@ netsh_command(const struct argv *a, int n, int
> msglevel)
> }
> management_sleep(4);
> }
> -msg(msglevel, "NETSH: command failed");
> +msg(msglevel, "%s: command failed", prefix);
> +}
> +
> +static void
> +netsh_command(const struct argv *a, int n, int msglevel)
> +{
> +exec_command("NETSH", a, n, msglevel);
> }
>
> void
> @@ -6927,6 +6959,11 @@ close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
> }
> else
> {
> +if (!tt->did_ifconfig_setup)
> +{
> +do_dns_domain_wmic(false, tt);
> +}
> +
> netsh_delete_address_dns(tt, true, );
> }
> }
> @@ -6947,9 +6984,14 @@ close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
> do_dns_service(false, AF_INET, tt);
> do_address_service(false, AF_INET, tt);
> }
> -else if (tt->options.ip_win32_type == IPW32_SET_NETSH)
> +else
> {
> -netsh_delete_address_dns(tt, false, );
> +do_dns_domain_wmic(false, tt);
> +
> +if (tt->options.ip_win32_type == IPW32_SET_NETSH)
> +{
> +netsh_delete_address_dns(tt, false, );
> +}
> }
> }
>
> diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
> index 72ffb012..36059662 100644
> --- a/src/openvpn/win32.h
> +++ b/src/openvpn/win32.h
> @@ -38,6 +38,7 @@
> #define WIN_ROUTE_PATH_SUFFIX "\\system32\\route.exe"
> #define WIN_IPCONFIG_PATH_SUFFIX "\\system32\\ipconfig.exe"
> #define WIN_NET_PATH_SUFFIX "\\system32\\net.exe"
> +#define WMIC_PATH_SUFFIX "\\system32\\wbem\\wmic.exe"
>
> /*
> * Win32-specific OpenVPN code, targeted at the mingw
>
Acked-by: Selva Nair
Useful to have in 2.6 as well, as dco is the default now. The changes are
minimal and should not have any side effects.
Did some quick test runs on Win 10 using tap-windows6 and dco drivers with
and without "dhcp-option DOMAIN".
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Support of DNS domain for DHCP-less drivers
_dns(tt, false, );
> +}
> }
> }
>
> diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
> index e19e1a2e..0d8e2307 100644
> --- a/src/openvpn/tun.h
> +++ b/src/openvpn/tun.h
> @@ -668,6 +668,12 @@ tuntap_is_dco_win_timeout(struct tuntap *tt, int
> status)
> const char *
> print_windows_driver(enum windows_driver_type windows_driver);
>
> +static inline bool
> +tuntap_maybe_dhcp(struct tuntap_options *o)
> +{
> +return o->ip_win32_type == IPW32_SET_DHCP_MASQ || o->ip_win32_type ==
> IPW32_SET_ADAPTIVE;
> +}
> +
> #else /* ifdef _WIN32 */
>
> static inline bool
> diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
> index 72ffb012..36059662 100644
> --- a/src/openvpn/win32.h
> +++ b/src/openvpn/win32.h
> @@ -38,6 +38,7 @@
> #define WIN_ROUTE_PATH_SUFFIX "\\system32\\route.exe"
> #define WIN_IPCONFIG_PATH_SUFFIX "\\system32\\ipconfig.exe"
> #define WIN_NET_PATH_SUFFIX "\\system32\\net.exe"
> +#define WMIC_PATH_SUFFIX "\\system32\\wbem\\wmic.exe"
>
> /*
> * Win32-specific OpenVPN code, targeted at the mingw
> --
> 2.23.0.windows.1
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH applied] Re: Bug-fix: segfault in dco_get_peer_stats()
On Tue, Mar 28, 2023 at 3:25 AM Gert Doering wrote: > Acked-by: Antonio Quartulli > > Thanks for the good find. Since I could reproduce the crash yesterday > (and I do need management for it) I can verify that it does no longer > crash with the patch. > > (For whatever reason, --tls-version-max 1.0 did not suffice to trigger > the error this morning, but --tls-version-min 1.3 + management + bytecount > still did) > Since then I've found this can be triggered quite simply by adding "--status /dev/stdout" or some file to the client options. Which means this should also fix #301 Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Bug-fix: segfault in dco_get_peer_stats()
Hi,
I should have added that as written the fault is with get_dco_peer stats().
It takes a pointer to "struct context" and dereferences a couple of layers
below it. The caller has no idea what it wants to see set in c. So, until
that is changed on all platforms (right now windows and linux), too much to
ask the caller to check that a valid dco handle is available.
Selva
On Mon, Mar 27, 2023 at 4:42 PM Selva Nair wrote:
>
>
> On Mon, Mar 27, 2023 at 4:30 PM Antonio Quartulli wrote:
>
>> Hi,
>>
>> On 27/03/2023 19:12, selva.n...@gmail.com wrote:
>> > From: Selva Nair
>> >
>> >We persist peer-stats when restarting, but an early restart
>> >before open_tun results in a segfault in dco_get_peer_stats().
>> >To reproduce, trigger a TLS handshake error due to lack of common
>> >protocols, for example.
>> >
>> >Fix by checking that tuntap is defined before dereferencing it.
>> >
>> > Signed-off-by: Selva Nair
>>
>> Nice catch! Thanks a lot!
>>
>> > ---
>> > I'm not entirely sure this is the right place to fix this.
>> > Or is it the caller at fault exercising dco_get_peer_stats()
>> > when tuntap is not set?
>>
>> Indeed that was my assumption.
>>
>> Does somebody have the stacktrace?
>> I wanted to check where this call is coming from exactly.
>>
>
> I didn't save the stacktrace.. The call comes through
>
> persist_client_stats(c); (openvpn.c ~ line 100)
>
> which gets called on exiting the main eventloop for restart. Without it we
> used to lose
> the statistics on restart.
>
> Imho it'd be reasonable if the caller could check if we have a device
>> before invoking any DCO API.
>>
>
> Sure, I would think that the caller of dco_get_peer_stats() should just
> pass the dco handle to it
> and get the stats back. It's debatable who should check whether
> the dco handle is valid or not.
>
> But this may not be the time and place for a refactor. As dco is young, I
> think it can mature in 2.7..
>
> Selva
>
>
>
>>
>> Cheers,
>>
>> >
>> > src/openvpn/dco_linux.c | 5 +
>> > 1 file changed, 5 insertions(+)
>> >
>> > diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
>> > index 317f9dc0..41540c0f 100644
>> > --- a/src/openvpn/dco_linux.c
>> > +++ b/src/openvpn/dco_linux.c
>> > @@ -975,6 +975,11 @@ dco_get_peer_stats(struct context *c)
>> > uint32_t peer_id = c->c2.tls_multi->dco_peer_id;
>> > msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id);
>> >
>> > +if (!c->c1.tuntap)
>> > +{
>> > +return 0;
>> > +}
>> > +
>> > dco_context_t *dco = >c1.tuntap->dco;
>> > struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco,
>> OVPN_CMD_GET_PEER);
>> > struct nlattr *attr = nla_nest_start(nl_msg, OVPN_ATTR_GET_PEER);
>>
>> --
>> Antonio Quartulli
>>
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Bug-fix: segfault in dco_get_peer_stats()
On Mon, Mar 27, 2023 at 4:30 PM Antonio Quartulli wrote:
> Hi,
>
> On 27/03/2023 19:12, selva.n...@gmail.com wrote:
> > From: Selva Nair
> >
> >We persist peer-stats when restarting, but an early restart
> >before open_tun results in a segfault in dco_get_peer_stats().
> >To reproduce, trigger a TLS handshake error due to lack of common
> >protocols, for example.
> >
> >Fix by checking that tuntap is defined before dereferencing it.
> >
> > Signed-off-by: Selva Nair
>
> Nice catch! Thanks a lot!
>
> > ---
> > I'm not entirely sure this is the right place to fix this.
> > Or is it the caller at fault exercising dco_get_peer_stats()
> > when tuntap is not set?
>
> Indeed that was my assumption.
>
> Does somebody have the stacktrace?
> I wanted to check where this call is coming from exactly.
>
I didn't save the stacktrace.. The call comes through
persist_client_stats(c); (openvpn.c ~ line 100)
which gets called on exiting the main eventloop for restart. Without it we
used to lose
the statistics on restart.
Imho it'd be reasonable if the caller could check if we have a device
> before invoking any DCO API.
>
Sure, I would think that the caller of dco_get_peer_stats() should just
pass the dco handle to it
and get the stats back. It's debatable who should check whether
the dco handle is valid or not.
But this may not be the time and place for a refactor. As dco is young, I
think it can mature in 2.7..
Selva
>
> Cheers,
>
> >
> > src/openvpn/dco_linux.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
> > index 317f9dc0..41540c0f 100644
> > --- a/src/openvpn/dco_linux.c
> > +++ b/src/openvpn/dco_linux.c
> > @@ -975,6 +975,11 @@ dco_get_peer_stats(struct context *c)
> > uint32_t peer_id = c->c2.tls_multi->dco_peer_id;
> > msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id);
> >
> > +if (!c->c1.tuntap)
> > +{
> > +return 0;
> > +}
> > +
> > dco_context_t *dco = >c1.tuntap->dco;
> > struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco,
> OVPN_CMD_GET_PEER);
> > struct nlattr *attr = nla_nest_start(nl_msg, OVPN_ATTR_GET_PEER);
>
> --
> Antonio Quartulli
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Bug-fix: segfault in dco_get_peer_stats()
From: Selva Nair
We persist peer-stats when restarting, but an early restart
before open_tun results in a segfault in dco_get_peer_stats().
To reproduce, trigger a TLS handshake error due to lack of common
protocols, for example.
Fix by checking that tuntap is defined before dereferencing it.
Signed-off-by: Selva Nair
---
I'm not entirely sure this is the right place to fix this.
Or is it the caller at fault exercising dco_get_peer_stats()
when tuntap is not set?
src/openvpn/dco_linux.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
index 317f9dc0..41540c0f 100644
--- a/src/openvpn/dco_linux.c
+++ b/src/openvpn/dco_linux.c
@@ -975,6 +975,11 @@ dco_get_peer_stats(struct context *c)
uint32_t peer_id = c->c2.tls_multi->dco_peer_id;
msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id);
+if (!c->c1.tuntap)
+{
+return 0;
+}
+
dco_context_t *dco = >c1.tuntap->dco;
struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_GET_PEER);
struct nlattr *attr = nla_nest_start(nl_msg, OVPN_ATTR_GET_PEER);
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Make cert_data.h and test_cryptoapi/pkcs11.c MSVC compliant
Hi,
On Mon, Mar 27, 2023 at 9:59 AM Matthias Andree
wrote:
> Am 27.03.23 um 13:49 schrieb selva.n...@gmail.com:
> > From: Selva Nair
> >
> > - Do not use non-literal initializers for static objects
> > - Replace empty initializer {} by {0}
>
> Should we go to a revision, I would suggest to not make something
> compliant to a compiler because that is assigning it way too much power
> and control. A terms such as "ready to be compiled with MSVC" or so
> would be in order, or making this code compliant with some wisely chosen
> version of the ISO 9899 standard, aka. standard C.
>
In this case "MSVC compliant" was arguably a poor choice of words as what
the change does is to avoid non-C99 constructs. However it made sense to
refer to MSVC in the context of what prompted this patch.
That said, in a cross-platform code base one often has to make changes to
please compilers just to get things to build.
> Signed-off-by: Selva Nair
> > ---
> > To be applied after the test-pkcs11 patch set
> >
> > tests/unit_tests/openvpn/cert_data.h | 6 ++---
> > tests/unit_tests/openvpn/test_cryptoapi.c | 24 ++--
> > tests/unit_tests/openvpn/test_pkcs11.c| 27 ---
> > 3 files changed, 39 insertions(+), 18 deletions(-)
> >
> > diff --git a/tests/unit_tests/openvpn/cert_data.h
> b/tests/unit_tests/openvpn/cert_data.h
> > index 33de35ec..0886b071 100644
> > --- a/tests/unit_tests/openvpn/cert_data.h
> > +++ b/tests/unit_tests/openvpn/cert_data.h
> > @@ -79,7 +79,7 @@ static const char *const cert2 =
> >
> "HeTsAlHjfFEReVDiNCI9vMQLKFKKWnAorT2+iyRueA3bt2gchf863BBhZvJddL7Q\n"
> > "KBa0osXw+eGBRAwsm7m1qCho3b3fN2nFAa+k07ptRkOeablmFdXE81nVlA==\n"
> > "-END CERTIFICATE-\n";
> > -static const char *const key2 = key1;
> > +#define key2 key1
>
> So we are not getting rid of #defines yet? Can't we make this data
> somehow local? Personally, I would probably - at least outside unit
tests - write (key1) instead of bare key, so that it's clear this is one
name and can't be somehow extended.
I was not getting rid of any defines -- just trying to avoid defines if it
can be done without jumping through too many hoops. Right, (key1) would be
better.
Instead of making key2 local, we can write it out fully without referring
to the fact that key1 = key2. The latter was anyway a lazy man's choice,
not relevant for the working of the tests.
If anyone feels strongly I can submit a v2 with key2, key4, and cname4
fully written out like:
static const char *const key2 = "-BEGIN PRIVATE KEY-\n" ...
etc..
static const char *cname4 = "ovpn-test-rsa1";
That may also make maintaining this data easier -- ie., any cert/key pair
could be replaced without affecting others.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Make cert_data.h and test_cryptoapi/pkcs11.c MSVC compliant
On Mon, Mar 27, 2023 at 8:09 AM Frank Lichtenheld
wrote:
> On Mon, Mar 27, 2023 at 07:49:37AM -0400, selva.n...@gmail.com wrote:
> > From: Selva Nair
> >
> > - Do not use non-literal initializers for static objects
> > - Replace empty initializer {} by {0}
> >
> > Signed-off-by: Selva Nair
>
I should have referenced Frank's original patch with:
Co-authored-by: Frank Lichtenheld
> ---
> > To be applied after the test-pkcs11 patch set
>
> Tested MSVC build (in my cmake branch) and verified that this is a
> suitable replacement for my earlier attempt to fix same issue.
>
> Acked-By: Frank Lichtenheld
>
> Regards,
> --
> Frank Lichtenheld
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Make cert_data.h and test_cryptoapi/pkcs11.c MSVC compliant
From: Selva Nair
- Do not use non-literal initializers for static objects
- Replace empty initializer {} by {0}
Signed-off-by: Selva Nair
---
To be applied after the test-pkcs11 patch set
tests/unit_tests/openvpn/cert_data.h | 6 ++---
tests/unit_tests/openvpn/test_cryptoapi.c | 24 ++--
tests/unit_tests/openvpn/test_pkcs11.c| 27 ---
3 files changed, 39 insertions(+), 18 deletions(-)
diff --git a/tests/unit_tests/openvpn/cert_data.h
b/tests/unit_tests/openvpn/cert_data.h
index 33de35ec..0886b071 100644
--- a/tests/unit_tests/openvpn/cert_data.h
+++ b/tests/unit_tests/openvpn/cert_data.h
@@ -79,7 +79,7 @@ static const char *const cert2 =
"HeTsAlHjfFEReVDiNCI9vMQLKFKKWnAorT2+iyRueA3bt2gchf863BBhZvJddL7Q\n"
"KBa0osXw+eGBRAwsm7m1qCho3b3fN2nFAa+k07ptRkOeablmFdXE81nVlA==\n"
"-END CERTIFICATE-\n";
-static const char *const key2 = key1;
+#define key2 key1
static const char *const hash2 = "FA18FD34BAABE47D6E2910E080F421C109CA97F5";
static const char *const cname2 = "ovpn-test-ec2";
@@ -159,8 +159,8 @@ static const char *const cert4 =
"353PpJJ9s2b/Fqoc4d7udqhQogA7jqbayTKhJxbT134l2NzqDROzuS0kXbX8bXCi\n"
"mXSa4c8=\n"
"-END CERTIFICATE-\n";
-static const char *const key4 = key3;
+#define key4 key3
static const char *const hash4 = "E1401D4497C944783E3D62CDBD2A1F69F5E5071E";
-static const char *const cname4 = cname3; /* same CN as that of cert3 */
+#define cname4 cname3 /* same CN as that of cert3 */
#endif /* CERT_DATA_H */
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c
b/tests/unit_tests/openvpn/test_cryptoapi.c
index c8468103..2150b77c 100644
--- a/tests/unit_tests/openvpn/test_cryptoapi.c
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -99,17 +99,26 @@ static struct test_cert
const char *const friendly_name;/* identifies certs loaded to the
store -- keep unique */
const char *hash; /* SHA1 fingerprint */
int valid; /* nonzero if certificate has not
expired */
-} certs[] = {
-{cert1, key1, cname1, "OVPN TEST CA1", "OVPN Test Cert 1", hash1, 1},
-{cert2, key2, cname2, "OVPN TEST CA2", "OVPN Test Cert 2", hash2, 1},
-{cert3, key3, cname3, "OVPN TEST CA1", "OVPN Test Cert 3", hash3, 1},
-{cert4, key4, cname4, "OVPN TEST CA2", "OVPN Test Cert 4", hash4, 0},
-{}
-};
+} certs[5];
static bool certs_loaded;
static HCERTSTORE user_store;
+/* Fill-in certs[] array */
+void
+init_cert_data()
+{
+struct test_cert certs_local[] = {
+{cert1, key1, cname1, "OVPN TEST CA1", "OVPN Test Cert 1", hash1,
1},
+{cert2, key2, cname2, "OVPN TEST CA2", "OVPN Test Cert 2", hash2,
1},
+{cert3, key3, cname3, "OVPN TEST CA1", "OVPN Test Cert 3", hash3,
1},
+{cert4, key4, cname4, "OVPN TEST CA2", "OVPN Test Cert 4", hash4,
0},
+{0}
+};
+assert(sizeof(certs_local) == sizeof(certs));
+memcpy(certs, certs_local, sizeof(certs_local));
+}
+
/* Lookup a certificate in our certificate/key db */
static struct test_cert *
lookup_cert(const char *friendly_name)
@@ -131,6 +140,7 @@ import_certs(void **state)
{
return;
}
+init_cert_data();
user_store = CertOpenStore(CERT_STORE_PROV_SYSTEM, 0, 0,
CERT_SYSTEM_STORE_CURRENT_USER
|CERT_STORE_OPEN_EXISTING_FLAG, L"MY");
assert_non_null(user_store);
diff --git a/tests/unit_tests/openvpn/test_pkcs11.c
b/tests/unit_tests/openvpn/test_pkcs11.c
index ea394bea..df5f8c7e 100644
--- a/tests/unit_tests/openvpn/test_pkcs11.c
+++ b/tests/unit_tests/openvpn/test_pkcs11.c
@@ -112,13 +112,7 @@ static struct test_cert
const char *const friendly_name;/* identifies certs loaded to the
store -- keep unique */
uint8_t hash[HASHSIZE]; /* SHA1 fingerprint: computed and
filled in later */
char *p11_id; /* PKCS#11 id -- filled in later */
-} certs[] = {
-{cert1, key1, cname1, "OVPN TEST CA1", "OVPN Test Cert 1", {}, NULL},
-{cert2, key2, cname2, "OVPN TEST CA2", "OVPN Test Cert 2", {}, NULL},
-{cert3, key3, cname3, "OVPN TEST CA1", "OVPN Test Cert 3", {}, NULL},
-{cert4, key4, cname4, "OVPN TEST CA2", "OVPN Test Cert 4", {}, NULL},
-{}
-};
+} certs[5];
static bool pkcs11_id_management;
static char softhsm2_tokens_path[] = "softhsm2_tokens_XX";
@@ -127,6 +121,21 @@ int num_certs;
static const char *pkcs11_id_current;
struct env_set *es;
+/* Fill-in certs[] array */
+void
+init_cert_data()
+{
+str
Re: [Openvpn-devel] [PATCH] unit_tests: make cert_data.h compile with MSVC
On Mon, Mar 27, 2023 at 4:49 AM Frank Lichtenheld wrote: > On Fri, Mar 24, 2023 at 01:13:22PM -0400, Selva Nair wrote: > > Would the attached small patch be acceptable instead? It covers only > > test_cryptoapi --- if this will do, I can incorporate similar changes for > > test_pkcs11 into a v2 of the relevant patch as those have not been > > ACK-ed yet. > > Actually, I acked that whole series. However there was a problem with > my mail configuration on that day and so Gmail rejected them. They > are on the list however. > But feel free to integrate this and I can re-ack the patch. > In that case I'll send this as a single patch covering both cryptoapi and pkcs11 tests. Thanks, Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] unit_tests: make cert_data.h compile with MSVC
Hi, On Fri, Mar 24, 2023 at 4:18 PM Matthias Andree wrote: > Am 23.03.23 um 15:31 schrieb Frank Lichtenheld: > > Currently this is not obvious since we never build the > > UTs with MSVC, but it doesn't like the initializers with > > "const" variables. They cause > > error C2099: initializer is not a constant > > What MSVC version are you using? What options? I've tried with a minimal > C program with static const char *const foo = "something"; and it > compiled with the latest 2017 or 2022 MSVC. > That will work but the following wont: const char *const foo = "something"; const char *const bar = foo; which is essentially the issue at hand. And more of the same with struct initialization. As bar has static storage duration, it has to be intialized by a "constant expression" which in C does not include "const variables". Though it works with gcc. In C99, automatic variables can be intialized so, and the alternative I suggested uses that approach. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] unit_tests: make cert_data.h compile with MSVC
Hi,
On Thu, Mar 23, 2023 at 10:31 AM Frank Lichtenheld
wrote:
> Currently this is not obvious since we never build the
> UTs with MSVC, but it doesn't like the initializers with
> "const" variables. They cause
> error C2099: initializer is not a constant
> when used in an initializer.
> So change all of them to preprocessor defines instead.
>
> It also doesn't like the empty initializer.
> error C2059: syntax error: '}'
>
> CC: Selva Nair
> Signed-off-by: Frank Lichtenheld
> ---
> tests/unit_tests/openvpn/cert_data.h | 240 +++---
> tests/unit_tests/openvpn/test_cryptoapi.c | 10 +-
> tests/unit_tests/openvpn/test_pkcs11.c| 10 +-
> 3 files changed, 127 insertions(+), 133 deletions(-)
>
> Notes:
> * this is an prereq for my CMake patch but is otherwise independent,
> so submitting separately
> * this is on top of Selva's "Unit tests: Test for PKCS#11 using a
> softhsm2 token"
>
> diff --git a/tests/unit_tests/openvpn/cert_data.h
> b/tests/unit_tests/openvpn/cert_data.h
> index 33de35ec..359718bb 100644
> --- a/tests/unit_tests/openvpn/cert_data.h
> +++ b/tests/unit_tests/openvpn/cert_data.h
> @@ -36,131 +36,125 @@
> */
>
...
> /* sample-ec.crt */
> +#define cd_cert1 "-BEGIN CERTIFICATE-\n" \
> +"MIIClzCCAX+gAwIBAgIRAIJr3cy95V63CPEtaAA8JN4wDQYJKoZIhvcNAQELBQAw\n" \
> +"GDEWMBQGA1UEAwwNT1ZQTiBURVNUIENBMTAgFw0yMzAzMTMxNjExMjhaGA8yMTIz\n" \
> +"MDIxNzE2MTEyOFowGDEWMBQGA1UEAwwNb3Zwbi10ZXN0LWVjMTBZMBMGByqGSM49\n" \
> +"AgEGCCqGSM49AwEHA0IABHhJG+dK4Z0mY+K0pupwVtyDLOwwGWHjBY6u3LgjRmUh\n" \
> +"fFjaoSfJvdgrPg50wbOkrsUt9Bl6EeDosZuVwuzgRbujgaQwgaEwCQYDVR0TBAIw\n" \
> +"ADAdBgNVHQ4EFgQUPWeU5BEmD8VEOSKeNf9kAvhcVuowUwYDVR0jBEwwSoAU3MLD\n" \
> +"NDOK13DqflQ8ra7FeGBXK06hHKQaMBgxFjAUBgNVBAMMDU9WUE4gVEVTVCBDQTGC\n" \
> +"FD55ErHXpK2JXS3WkfBm0NB1r3vKMBMGA1UdJQQMMAoGCCsGAQUFBwMCMAsGA1Ud\n" \
> +"DwQEAwIHgDANBgkqhkiG9w0BAQsFAAOCAQEAhH/wOFqP4R+FK5QvU+oW/XacFMku\n" \
> +"+qT8lL9J7BG28WhZ0ZcAy/AmtnyynkDyuZSwnlzGgJ5m4L/RfwTzJKhEHiSU3BvB\n" \
> +"5C1Z1Q8k67MHSfb565iCn8GzPUQLK4zsILCoTkJPvimv2bJ/RZmNaD+D4LWiySD4\n" \
> +"tuOEdHKrxIrbJ5eAaN0WxRrvDdwGlyPvbMFvfhXzd/tbkP4R2xvlm7S2DPeSTJ8s\n" \
> +"srXMaPe0lAea4etMSZsjIRPwGRMXBrwbRmb6iN2Cq40867HdaJoAryYig7IiDwSX\n" \
> +"htCbOA6sX+60+FEOYDEx5cmkogl633Pw7LJ3ICkyzIrUSEt6BOT1Gsc1eQ==\n" \
> +"-END CERTIFICATE-\n"
>
I would like to avoid such long preprocessor #defines with continuation
lines.
Would the attached small patch be acceptable instead? It covers only
test_cryptoapi --- if this will do, I can incorporate similar changes for
test_pkcs11 into a v2 of the relevant patch as those have not been
ACK-ed yet.
> --- a/tests/unit_tests/openvpn/test_pkcs11.c
> +++ b/tests/unit_tests/openvpn/test_pkcs11.c
> @@ -113,11 +113,11 @@ static struct test_cert
> uint8_t hash[HASHSIZE]; /* SHA1 fingerprint: computed and
> filled in later */
> char *p11_id; /* PKCS#11 id -- filled in later
> */
> } certs[] = {
> -{cert1, key1, cname1, "OVPN TEST CA1", "OVPN Test Cert 1", {},
> NULL},
> -{cert2, key2, cname2, "OVPN TEST CA2", "OVPN Test Cert 2", {},
> NULL},
> -{cert3, key3, cname3, "OVPN TEST CA1", "OVPN Test Cert 3", {},
> NULL},
> -{cert4, key4, cname4, "OVPN TEST CA2", "OVPN Test Cert 4", {},
> NULL},
> -{}
> +{cd_cert1, cd_key1, cd_cname1, "OVPN TEST CA1", "OVPN Test Cert
> 1", {}, NULL},
>
Isn't it necessary to change these {} to {0} as well? The build may not
report it now as we haven't yet enabled test_pkcs11 on Windows.
> +{cd_cert2, cd_key2, cd_cname2, "OVPN TEST CA2", "OVPN Test Cert
> 2", {}, NULL},
> +{cd_cert3, cd_key3, cd_cname3, "OVPN TEST CA1", "OVPN Test Cert
> 3", {}, NULL},
> +{cd_cert4, cd_key4, cd_cname4, "OVPN TEST CA2", "OVPN Test Cert
> 4", {}, NULL},
> +{NULL, NULL, NULL, NULL, NULL,
>{}, NULL}
>
A single {0} should be enough here as well.
Selva
From 08d4813fdc3c90969c49dc07be529111a7e84ac4 Mon Sep 17 00:00:00 2001
From: Selva Nair
Date: Thu, 23 Mar 2023 23:24:38 -0400
Subject: [PATCH 1/2] Make cert_data.h and test_cryptoapi.c MSVC compliant
- Do not use non-literal initializers for static objects
- Replace empty initializer {} by {0}
Signed-off-by: Selva Nair
---
tests/unit_tests/openvpn/cert_data.h | 6 +++---
tes
[Openvpn-devel] Fwd: [PATCH] Print DCO client stats on SIGUSR2
I didn't realize it until Lev pointed out that this reply yesterday didn't go to the list. FTR, copying to the list. -- Forwarded message - From: Selva Nair Date: Wed, Mar 22, 2023 at 9:42 AM Subject: Re: [Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2 To: Lev Stipakov > > > >> +status_printf(so, "TCP/UDP read bytes," counter_format, > c->c2.link_read_bytes + c->c2.dco_read_bytes); > >> +status_printf(so, "TCP/UDP write bytes," counter_format, > c->c2.link_write_bytes + c->c2.dco_write_bytes); > > > > Same here -- have a place to read the total count from independent of > dco. > > But what is wrong in fetching dco stats when we need them? What is the > advantage of timer? > What happens when we have three other drivers like DCO? Not saying that will ever happen, but using that as an example to clarify why I think this is "wrong" style. That said, I agree why a timer may not be a good idea either. If an approach that can keep internal details "private" is going to be too complex, we can live with this. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 3/3] Enable pkcs11 an dtest_pkcs11 in github actions
From: Selva Nair
- Enabled for the Ubuntu 22.04 build (OpenSSL 3) and one of the
Ubuntu 20.04 builds (OpenSSL 1.1.1).
Signed-off-by: Selva Nair
---
.github/workflows/build.yaml | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
index a3ca7a2e..2538c818 100644
--- a/.github/workflows/build.yaml
+++ b/.github/workflows/build.yaml
@@ -297,11 +297,14 @@ jobs:
sslpkg: "libssl-dev"
libname: OpenSSL 3.0.2
ssllib: openssl
+pkcs11pkg: "libpkcs11-helper1-dev softhsm2 gnutls-bin"
+extraconf: --enable-pkcs11
- os: ubuntu-20.04
sslpkg: "libssl-dev"
libname: OpenSSL 1.1.1
ssllib: openssl
-extraconf: "--enable-iproute2"
+pkcs11pkg: "libpkcs11-helper1-dev softhsm2 gnutls-bin"
+extraconf: "--enable-iproute2 --enable-pkcs11"
- os: ubuntu-20.04
sslpkg: "libssl-dev"
libname: OpenSSL 1.1.1
@@ -326,11 +329,12 @@ jobs:
name: "gcc - ${{matrix.os}} - ${{matrix.libname}} ${{matrix.extraconf}}"
env:
SSLPKG: "${{matrix.sslpkg}}"
+ PKCS11PKG: "${{matrix.pkcs11pkg}}"
runs-on: ${{matrix.os}}
steps:
- name: Install dependencies
-run: sudo apt update && sudo apt install -y liblzo2-dev libpam0g-dev
liblz4-dev libcap-ng-dev libnl-genl-3-dev linux-libc-dev man2html libcmocka-dev
python3-docutils libtool automake autoconf ${SSLPKG}
+run: sudo apt update && sudo apt install -y liblzo2-dev libpam0g-dev
liblz4-dev libcap-ng-dev libnl-genl-3-dev linux-libc-dev man2html libcmocka-dev
python3-docutils libtool automake autoconf ${SSLPKG} ${PKCS11PKG}
- name: Checkout OpenVPN
uses: actions/checkout@v3
- name: autoconf
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 1/3] Move digest_sign_verify out of test_cryptoapi.c
From: Selva Nair
- This function will be reused for testing pkcs11
Signed-off-by: Selva Nair
---
tests/unit_tests/openvpn/Makefile.am | 1 +
tests/unit_tests/openvpn/pkey_test_utils.c | 141 +
tests/unit_tests/openvpn/test_cryptoapi.c | 98 +-
3 files changed, 143 insertions(+), 97 deletions(-)
create mode 100644 tests/unit_tests/openvpn/pkey_test_utils.c
diff --git a/tests/unit_tests/openvpn/Makefile.am
b/tests/unit_tests/openvpn/Makefile.am
index 4391a54e..30659561 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -156,6 +156,7 @@ cryptoapi_testdriver_CFLAGS = @TEST_CFLAGS@ \
cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
$(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
+ pkey_test_utils.c \
$(top_srcdir)/src/openvpn/xkey_helper.c \
$(top_srcdir)/src/openvpn/xkey_provider.c \
$(top_srcdir)/src/openvpn/buffer.c \
diff --git a/tests/unit_tests/openvpn/pkey_test_utils.c
b/tests/unit_tests/openvpn/pkey_test_utils.c
new file mode 100644
index ..7adaf33f
--- /dev/null
+++ b/tests/unit_tests/openvpn/pkey_test_utils.c
@@ -0,0 +1,141 @@
+/*
+ * OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ * Copyright (C) 2023 Selva Nair
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by the
+ * Free Software Foundation, either version 2 of the License,
+ * or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+
+#include "syshead.h"
+#include "xkey_common.h"
+#include
+#include
+
+#ifdef HAVE_XKEY_PROVIDER
+
+#include
+#include
+
+extern OSSL_LIB_CTX *tls_libctx;
+
+/* A message for signing */
+static const char *test_msg = "Lorem ipsum dolor sit amet, consectetur "
+ "adipisici elit, sed eiusmod tempor incidunt "
+ "ut labore et dolore magna aliqua.";
+
+/**
+ * Sign "test_msg" using a private key. The key may be a "provided" key
+ * in which case its signed by the provider's backend -- cryptoapi in our
+ * case. Then verify the signature using OpenSSL.
+ * Returns 1 on success, 0 on error.
+ */
+int
+digest_sign_verify(EVP_PKEY *privkey, EVP_PKEY *pubkey)
+{
+uint8_t *sig = NULL;
+size_t siglen = 0;
+int ret = 0;
+
+OSSL_PARAM params[2] = {OSSL_PARAM_END};
+const char *mdname = "SHA256";
+
+if (EVP_PKEY_get_id(privkey) == EVP_PKEY_RSA)
+{
+const char *padmode = "pss"; /* RSA_PSS: for all other params, use
defaults */
+params[0] =
OSSL_PARAM_construct_utf8_string(OSSL_SIGNATURE_PARAM_PAD_MODE,
+ (char *)padmode, 0);
+params[1] = OSSL_PARAM_construct_end();
+}
+else if (EVP_PKEY_get_id(privkey) == EVP_PKEY_EC)
+{
+params[0] = OSSL_PARAM_construct_end();
+}
+else
+{
+print_error("Unknown key type in digest_sign_verify()");
+return ret;
+}
+
+EVP_PKEY_CTX *pctx = NULL;
+EVP_MD_CTX *mctx = EVP_MD_CTX_new();
+
+if (!mctx
+|| EVP_DigestSignInit_ex(mctx, , mdname, tls_libctx, NULL,
privkey, params) <= 0)
+{
+/* cmocka assert output for these kinds of failures is hardly
explanatory,
+ * print a message and assert in caller. */
+print_error("Failed to initialize EVP_DigestSignInit_ex()\n");
+goto done;
+}
+
+/* sign with sig = NULL to get required siglen */
+if (EVP_DigestSign(mctx, sig, , (uint8_t *)test_msg,
strlen(test_msg)) != 1)
+{
+print_error("EVP_DigestSign: failed to get required signature size");
+goto done;
+}
+assert_true(siglen > 0);
+
+if ((sig = test_calloc(1, siglen)) == NULL)
+{
+print_error("Out of memory");
+goto done;
+}
+if (EVP_DigestSign(mctx, sig, , (uint8_t *)test_msg,
strlen(test_msg)) != 1)
+{
+
[Openvpn-devel] [PATCH 2/3] Unit tests: Test for PKCS#11 using a softhsm2 token
From: Selva Nair
- Load some test certificate/key pairs into a temporary softhsm2 token
and enumerate available objects through pkcs11-helper interface
- For each object, load it into SSL_CTX and test sign (if using OpenSSL 3)
or check the certificate and public-key match (if using OpenSSl 1.1.1.).
The pkcs11-id for each object is specified directly or
through a mocked management callback to test pkcs11-id-management
Limitations:
Depends on libsofthsm2.so and p11tool (install softhsm2 and gnutls-bin
packages). Mbed-TLS/pkcs11-helper combination is not tested.
If locations of these binaries are not auto-detected or need to be
overridden, use -DSOFTHSM2_UTIL= -DP11TOOL= to configure.
Location of SOFTHSM2_MODULE is not auto-detected and defaults to
/usr/lib/softhsm/libsofthsm2.so. It may be changed by passing
-DSOFTHSM2_MODULE=/some-path/libsofthsm2.so to configure.
Also see "configure --help".
The test is enabled only if --enable-pkcs11 is in use, and SOFTHSM2_UTIL
& P11TOOL are found in path or manually defined during configuring.
Changes relative to github PR
- Explicitly disable building the test on Windows: need to port mkstemp,
mkdtemp, setenv etc., before enabling this on Windows.
Signed-off-by: Selva Nair
---
configure.ac | 16 +
tests/unit_tests/openvpn/Makefile.am | 29 ++
tests/unit_tests/openvpn/mock_msg.c| 6 +
tests/unit_tests/openvpn/test_pkcs11.c | 453 +
4 files changed, 504 insertions(+)
create mode 100644 tests/unit_tests/openvpn/test_pkcs11.c
diff --git a/configure.ac b/configure.ac
index 67f680b2..ca85e5ed 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1345,6 +1345,7 @@ if test "${enable_comp_stub}" = "yes"; then
AC_DEFINE([ENABLE_COMP_STUB], [1], [Enable compression stub capability])
fi
+AM_CONDITIONAL([HAVE_SOFTHSM2], [false])
if test "${enable_pkcs11}" = "yes"; then
test "${have_pkcs11_helper}" != "yes" && AC_MSG_ERROR([PKCS11 enabled
but libpkcs11-helper is missing])
OPTIONAL_PKCS11_HELPER_CFLAGS="${PKCS11_HELPER_CFLAGS}"
@@ -1357,6 +1358,21 @@ if test "${enable_pkcs11}" = "yes"; then
AC_DEFINE_UNQUOTED([DEFAULT_PKCS11_MODULE], "${proxy_module}",
[p11-kit proxy])],
[]
)
+ #
+ # softhsm2 for pkcs11 tests
+ #
+ AC_ARG_VAR([P11TOOL], [full path to p11tool])
+ AC_PATH_PROGS([P11TOOL], [p11tool],,
[$PATH:/usr/local/bin:/usr/bin:/bin])
+ AC_DEFINE_UNQUOTED([P11TOOL_PATH], ["$P11TOOL"], [Path to p11tool])
+ AC_ARG_VAR([SOFTHSM2_UTIL], [full path to softhsm2-util])
+ AC_ARG_VAR([SOFTHSM2_MODULE], [full path to softhsm2 module
@<:@default=/usr/lib/softhsm/libsofthsm2.so@:>@])
+ AC_PATH_PROGS([SOFTHSM2_UTIL], [softhsm2-util],,
[$PATH:/usr/local/bin:/usr/bin:/bin])
+ test -z "$SOFTHSM2_MODULE" &&
SOFTHSM2_MODULE=/usr/lib/softhsm/libsofthsm2.so
+ AC_DEFINE_UNQUOTED([SOFTHSM2_UTIL_PATH], ["$SOFTHSM2_UTIL"], [Path to
softhsm2-util])
+ AC_DEFINE_UNQUOTED([SOFTHSM2_MODULE_PATH], ["$SOFTHSM2_MODULE"], [Path
to softhsm2 module])
+ if test "${with_crypto_library}" = "openssl"; then
+ AM_CONDITIONAL([HAVE_SOFTHSM2], [test "${P11TOOL}" -a
"${SOFTHSM2_UTIL}" -a "${SOFTHSM2_MODULE}"])
+ fi
fi
# When testing a compiler option, we add -Werror to force
diff --git a/tests/unit_tests/openvpn/Makefile.am
b/tests/unit_tests/openvpn/Makefile.am
index 30659561..ccd0e37b 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -21,6 +21,12 @@ test_binaries += cryptoapi_testdriver
LDADD = -lws2_32
endif
+if HAVE_SOFTHSM2
+if !WIN32
+test_binaries += pkcs11_testdriver
+endif
+endif
+
if !CROSS_COMPILING
TESTS = $(test_binaries)
endif
@@ -166,6 +172,29 @@ cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c
\
$(top_srcdir)/src/openvpn/win32-util.c
endif
+if HAVE_SOFTHSM2
+if !WIN32
+pkcs11_testdriver_CFLAGS = @TEST_CFLAGS@ \
+ -I$(top_srcdir)/include -I$(top_srcdir)/src/compat
-I$(top_srcdir)/src/openvpn \
+ $(OPTIONAL_CRYPTO_CFLAGS)
+pkcs11_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+ $(OPTIONAL_CRYPTO_LIBS)
+pkcs11_testdriver_SOURCES = test_pkcs11.c mock_msg.c \
+ pkey_test_utils.c mock_get_random.c \
+ $(top_srcdir)/src/openvpn/xkey_helper.c \
+ $(top_srcdir)/src/openvpn/xkey_provider.c \
+ $(top_srcdir)/src/openvpn/argv.c \
+ $(top_srcdir)/src/openvpn/env_set.c \
+ $(top_srcdir)/src/openvpn/buffer.c \
+ $(top_srcdir)/src/openvpn/otime.c \
+ $(top_srcdir)/src/openvpn/run_command.c \
+ $(top_srcdir)/src/openvpn/base64.c \
+ $(top_srcdir)/src/openvpn/platform.c \
+ $
[Openvpn-devel] [PATCH] Make error in setting metric for IPv6 interface non-fatal
From: Selva Nair
- Unfortunately there are still users out there who disable IPv6
on tun/tap/dco interfaces or even system-wide.
Fixes: Github issue #294
Signed-off-by: Selva Nair
---
src/openvpnserv/interactive.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index a3d43752..98a42b1f 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -847,8 +847,8 @@ AddBlockDNS(const block_dns_message_t *msg, undo_lists_t
*lists)
BLOCK_DNS_IFACE_METRIC);
if (!err)
{
-err = set_interface_metric(msg->iface.index, AF_INET6,
- BLOCK_DNS_IFACE_METRIC);
+set_interface_metric(msg->iface.index, AF_INET6,
+ BLOCK_DNS_IFACE_METRIC);
}
if (err)
{
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2
Hi,
On Wed, Mar 22, 2023 at 7:34 AM Lev Stipakov wrote:
> From: Lev Stipakov
>
> Change-Id: I465febdf7ee5fe573e88255844f718efb60f8e8a
> Signed-off-by: Lev Stipakov
> ---
> src/openvpn/sig.c | 13 +
> src/openvpn/sig.h | 2 +-
> 2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/src/openvpn/sig.c b/src/openvpn/sig.c
> index 5b89bb42..05c0054b 100644
> --- a/src/openvpn/sig.c
> +++ b/src/openvpn/sig.c
> @@ -300,18 +300,23 @@ restore_signal_state(void)
> * Triggered by SIGUSR2 or F2 on Windows.
> */
> void
> -print_status(const struct context *c, struct status_output *so)
> +print_status(struct context *c, struct status_output *so)
{
> struct gc_arena gc = gc_new();
>
> status_reset(so);
>
> +if (dco_enabled(>options))
> +{
> +dco_get_peer_stats(c);
> +}
> +
>
While this is expedient, adding dco dependence to parts of code which
should be independent of such inner details look wrong style to me. Finding
a way to keep struct context a const here will lead to a better logic, I
think.
Can't we have a coarse timer that keeps the stats updated? It doesn't have
to be frequent as this is just for information.
> status_printf(so, "OpenVPN STATISTICS");
> status_printf(so, "Updated,%s", time_string(0, 0, false, ));
> status_printf(so, "TUN/TAP read bytes," counter_format,
> c->c2.tun_read_bytes);
> status_printf(so, "TUN/TAP write bytes," counter_format,
> c->c2.tun_write_bytes);
> -status_printf(so, "TCP/UDP read bytes," counter_format,
> c->c2.link_read_bytes);
> -status_printf(so, "TCP/UDP write bytes," counter_format,
> c->c2.link_write_bytes);
> +status_printf(so, "TCP/UDP read bytes," counter_format,
> c->c2.link_read_bytes + c->c2.dco_read_bytes);
> +status_printf(so, "TCP/UDP write bytes," counter_format,
> c->c2.link_write_bytes + c->c2.dco_write_bytes);
>
Same here -- have a place to read the total count from independent of dco.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH applied] Re: Improve error message on short read from socks proxy
> > > > Thanks, this change makes sense. I have not actively tried to provoke > it (like, by connecting to a "fake SOCKS server" that will trigger it), > but the change is obviously an improvement to "if it's not ==1, it > must be a TCP error!"). > An easy way to "provoke" this is to use openssh proxy (say, -D 1080) and use it to proxy to a udp server. SSH will close the connection as it does not support udp association. Probably it should return one of the socks5 error code instead, but doesn't. Even if it did, our recv_socks_reply() is not capable of handling such errors. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Unit tests: add test for SSL_CTX_use_Cryptoapi_certificate()
From: Selva Nair
- This is the only remaining function in cryptoapi.c that has no
direct or indirect test.
This test confirms that an SSL_CTX context gets a certificate and
private key loaded into it and the public key in the certificate
matches the private key. As signing with certificate/key pairs
fetched from the store is independently tested by the 'cryptoapi_sign'
test, signing is not re-tested here.
The functions "setup_/teardown_cryptoapi_sign()" are renamed to
"setup_/teardown_xkey_provider()" to better reflect their purpose.
These are also reused for the new test.
While touching this context, also fix a memory leak in
test_cryptoapi_sign: X509_get_pubkey() -> X509_get0_pubkey()
Signed-off-by: Selva Nair
---
tests/unit_tests/openvpn/test_cryptoapi.c | 51 ---
1 file changed, 46 insertions(+), 5 deletions(-)
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c
b/tests/unit_tests/openvpn/test_cryptoapi.c
index b07e8935..e64a1de3 100644
--- a/tests/unit_tests/openvpn/test_cryptoapi.c
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -343,7 +343,7 @@ test_find_cert_byissuer(void **state)
}
static int
-setup_cryptoapi_sign(void **state)
+setup_xkey_provider(void **state)
{
(void) state;
/* Initialize providers in a way matching what OpenVPN core does */
@@ -358,7 +358,7 @@ setup_cryptoapi_sign(void **state)
}
static int
-teardown_cryptoapi_sign(void **state)
+teardown_xkey_provider(void **state)
{
(void) state;
for (size_t i = 0; i < _countof(prov); i++)
@@ -493,13 +493,52 @@ test_cryptoapi_sign(void **state)
fail_msg("Load_CryptoAPI_certificate failed: <%s>",
c->friendly_name);
return;
}
-EVP_PKEY *pubkey = X509_get_pubkey(x509);
+EVP_PKEY *pubkey = X509_get0_pubkey(x509);
+assert_non_null(pubkey);
assert_int_equal(digest_sign_verify(privkey, pubkey), 1);
X509_free(x509);
EVP_PKEY_free(privkey);
}
}
+/* Test that SSL_CTX_use_Cryptoapi_certificate() sets a matching certificate
+ * and key in ssl_ctx.
+ */
+void
+test_ssl_ctx_use_cryptoapicert(void **state)
+{
+(void) state;
+char select_string[64];
+
+import_certs(state); /* a no-op if already imported */
+assert_true(certs_loaded);
+
+for (struct test_cert *c = certs; c->cert; c++)
+{
+if (c->valid == 0)
+{
+continue;
+}
+SSL_CTX *ssl_ctx = SSL_CTX_new_ex(tls_libctx, NULL,
SSLv23_client_method());
+assert_non_null(ssl_ctx);
+
+openvpn_snprintf(select_string, sizeof(select_string), "THUMB:%s",
c->hash);
+if (!SSL_CTX_use_CryptoAPI_certificate(ssl_ctx, select_string))
+{
+fail_msg("SSL_CTX_use_CryptoAPI_certificate failed: <%s>",
c->friendly_name);
+return;
+}
+/* Use OpenSSL to check that the cert and private key in ssl_ctx
"match" */
+if (!SSL_CTX_check_private_key(ssl_ctx))
+{
+fail_msg("Certificate and private key in ssl_ctx do not match for
<%s>", c->friendly_name);
+return;
+}
+
+SSL_CTX_free(ssl_ctx);
+}
+}
+
static void
test_parse_hexstring(void **state)
{
@@ -530,8 +569,10 @@ main(void)
cmocka_unit_test(test_find_cert_bythumb),
cmocka_unit_test(test_find_cert_byname),
cmocka_unit_test(test_find_cert_byissuer),
-cmocka_unit_test_setup_teardown(test_cryptoapi_sign,
setup_cryptoapi_sign,
-teardown_cryptoapi_sign),
+cmocka_unit_test_setup_teardown(test_cryptoapi_sign,
setup_xkey_provider,
+teardown_xkey_provider),
+cmocka_unit_test_setup_teardown(test_ssl_ctx_use_cryptoapicert,
setup_xkey_provider,
+teardown_xkey_provider),
};
int ret = cmocka_run_group_tests_name("cryptoapi tests", tests, NULL,
cleanup);
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Improve error message on short read from socks proxy
From: Selva Nair
Change-Id: Id6bf8ea705d02eff2cbfba7d841e1cdb6ae1
Signed-off-by: Selva Nair
---
src/openvpn/socks.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c
index 6a672c25..2cf0cc9f 100644
--- a/src/openvpn/socks.c
+++ b/src/openvpn/socks.c
@@ -357,11 +357,16 @@ recv_socks_reply(socket_descriptor_t sd,
size = recv(sd, , 1, MSG_NOSIGNAL);
/* error? */
-if (size != 1)
+if (size < 0)
{
msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read
failed on recv()");
return false;
}
+else if (size == 0)
+{
+msg(D_LINK_ERRORS, "ERROR: recv_socks_reply: empty response from
socks server");
+return false;
+}
if (len == 3)
{
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH applied] Re: Import some sample certificates into Windows store for testing
> > > I have tried testing this on a "real" Win10 VM, but I am missing some > intricacies with the cert store - I wanted to see if certs are properly > cleaned up, but I can't even see my own .p12 I have imported, so I am > doing something wrong... This works for me: Run "certmgr" from a user command prompt. The UI that opens up will show the "Certificates - Current User" snapin. Under it select "Personal->Certificates", and a list of all imported certificates will open up. This will not include any Root certificates that may have been imported. Those go to "Trusted Root Certificates" listed below "Personal". I'm not entirely sure whether it's possible to select a wrong destination during import, causing client certificates to go into root certificates. During import I only select the store (user or machine) and let it decide the appropriate location within the store. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 0/4] Add some tests for cryptoapi.c functions
On Wed, Mar 15, 2023 at 4:30 AM Gert Doering wrote: > Hi, > > On Tue, Mar 14, 2023 at 09:35:12PM -0400, selva.n...@gmail.com wrote: > > From: Selva Nair > > > > Import some sample certificates into Windows store for testing > > - 4 test certificates imported to user store > > and removed at the end. > [..] > > This is extremely valuable. Thanks. > > If I apply these 4 patches on top of "master", will the test then succeed, > or will it show failure due to the not-yet-merged padding patch? > As cryptoapi works correctly without the padding patch (only PKCS11 was affected), and this tests only exercises those bits should work without the padding patch. Though I tested it only with both included. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 1/4] Import some sample certificates into Windows store for testing
From: Selva Nair - A few sample certificates are defined and imported into Windows certificate store (user store). This only tests the import process. Use of these certs to test the core functionality of 'cryptoapicert' are in following commits. Change-Id: Ida5fc12c5bad5fde202da0bf0e8cdc71efe548c2 Signed-off-by: Selva Nair --- tests/unit_tests/openvpn/cert_data.h | 166 ++ tests/unit_tests/openvpn/test_cryptoapi.c | 160 - 2 files changed, 324 insertions(+), 2 deletions(-) create mode 100644 tests/unit_tests/openvpn/cert_data.h diff --git a/tests/unit_tests/openvpn/cert_data.h b/tests/unit_tests/openvpn/cert_data.h new file mode 100644 index ..33de35ec --- /dev/null +++ b/tests/unit_tests/openvpn/cert_data.h @@ -0,0 +1,166 @@ +/* + * OpenVPN -- An application to securely tunnel IP networks + * over a single UDP port, with support for SSL/TLS-based + * session authentication and key exchange, + * packet encryption, packet authentication, and + * packet compression. + * + * Copyright (C) 2023 Selva Nair + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by the + * Free Software Foundation, either version 2 of the License, + * or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#ifndef CERT_DATA_H +#define CERT_DATA_H + +/* Some certificates and their private keys for testing cryptoapi.c. + * Two certificates, cert1 (EC) and cert3 (RSA) are signed by one CA + * and the other two, cert2 (EC) and cert4 (RSA), by another to have a + * different issuer name. The common name of cert4 is the same as + * that of cert3 but the former has expired. It is used to test + * retrieval of valid certificate by name when an expired one with same + * common name exists. + * To reduce data volume, certs of same keytype use the same private key. + */ + +/* sample-ec.crt */ +static const char *const cert1 = +"-BEGIN CERTIFICATE-\n" +"MIIClzCCAX+gAwIBAgIRAIJr3cy95V63CPEtaAA8JN4wDQYJKoZIhvcNAQELBQAw\n" +"GDEWMBQGA1UEAwwNT1ZQTiBURVNUIENBMTAgFw0yMzAzMTMxNjExMjhaGA8yMTIz\n" +"MDIxNzE2MTEyOFowGDEWMBQGA1UEAwwNb3Zwbi10ZXN0LWVjMTBZMBMGByqGSM49\n" +"AgEGCCqGSM49AwEHA0IABHhJG+dK4Z0mY+K0pupwVtyDLOwwGWHjBY6u3LgjRmUh\n" +"fFjaoSfJvdgrPg50wbOkrsUt9Bl6EeDosZuVwuzgRbujgaQwgaEwCQYDVR0TBAIw\n" +"ADAdBgNVHQ4EFgQUPWeU5BEmD8VEOSKeNf9kAvhcVuowUwYDVR0jBEwwSoAU3MLD\n" +"NDOK13DqflQ8ra7FeGBXK06hHKQaMBgxFjAUBgNVBAMMDU9WUE4gVEVTVCBDQTGC\n" +"FD55ErHXpK2JXS3WkfBm0NB1r3vKMBMGA1UdJQQMMAoGCCsGAQUFBwMCMAsGA1Ud\n" +"DwQEAwIHgDANBgkqhkiG9w0BAQsFAAOCAQEAhH/wOFqP4R+FK5QvU+oW/XacFMku\n" +"+qT8lL9J7BG28WhZ0ZcAy/AmtnyynkDyuZSwnlzGgJ5m4L/RfwTzJKhEHiSU3BvB\n" +"5C1Z1Q8k67MHSfb565iCn8GzPUQLK4zsILCoTkJPvimv2bJ/RZmNaD+D4LWiySD4\n" +"tuOEdHKrxIrbJ5eAaN0WxRrvDdwGlyPvbMFvfhXzd/tbkP4R2xvlm7S2DPeSTJ8s\n" +"srXMaPe0lAea4etMSZsjIRPwGRMXBrwbRmb6iN2Cq40867HdaJoAryYig7IiDwSX\n" +"htCbOA6sX+60+FEOYDEx5cmkogl633Pw7LJ3ICkyzIrUSEt6BOT1Gsc1eQ==\n" +"-END CERTIFICATE-\n"; +static const char *const key1 = +"-BEGIN PRIVATE KEY-\n" +"MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg5Xpw/lLvBrWjAWDq\n" +"L6dm/4a1or6AQ6O3yXYgw78B23ihRANCAAR4SRvnSuGdJmPitKbqcFbcgyzsMBlh\n" +"4wWOrty4I0ZlIXxY2qEnyb3YKz4OdMGzpK7FLfQZehHg6LGblcLs4EW7\n" +"-END PRIVATE KEY-\n"; +static const char *const hash1 = "A4B74F1D68AF50691F62CBD675E24C8655369567"; +static const char *const cname1 = "ovpn-test-ec1"; + +static const char *const cert2 = +"-BEGIN CERTIFICATE-\n" +"MIIClzCCAX+gAwIBAgIRAN9fIkTDOjX0Bd9adHVcLx8wDQYJKoZIhvcNAQELBQAw\n" +"GDEWMBQGA1UEAwwNT1ZQTiBURVNUIENBMjAgFw0yMzAzMTMxODAzMzFaGA8yMTIz\n" +"MDIxNzE4MDMzMVowGDEWMBQGA1UEAwwNb3Zwbi10ZXN0LWVjMjBZMBMGByqGSM49\n" +"AgEGCCqGSM49AwEHA0IABHhJG+dK4Z0mY+K0pupwVtyDLOwwGWHjBY6u3LgjRmUh\n" +"fFjaoSfJvdgrPg50wbOkrsUt9Bl6EeDosZuVwuzgRbujgaQwgaEwCQYDVR0TBAIw\n" +"ADAdBgNVHQ4EFgQUPWeU5BEmD8VEOSKeNf9kAvhcVuowUwYDVR0jBEwwSoAUyX3c\n" +"tpRP5cKlESsG80rOGhEphsGhHKQaMBgxFjAUBgNVBAMMDU9WUE4gVEVTVCBDQTKC\n" +
[Openvpn-devel] [PATCH 4/4] Add a test for signing with certificates in Windows store
From: Selva Nair
- For each sample certificate/key pair imported into the store,
load the key into xkey-provider and sign a test message.
As the key is "provided", signing will use appropriate
backend (Windows CNG in this case).
The signature is then verified using OpenSSL.
Change-Id: I520b34ba51e8c6d0247a82edc52bde181ab5a717
Signed-off-by: Selva Nair
---
tests/unit_tests/openvpn/Makefile.am | 1 +
tests/unit_tests/openvpn/test_cryptoapi.c | 166 ++
2 files changed, 167 insertions(+)
diff --git a/tests/unit_tests/openvpn/Makefile.am
b/tests/unit_tests/openvpn/Makefile.am
index 339c7ef3..4391a54e 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -157,6 +157,7 @@ cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
$(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
$(top_srcdir)/src/openvpn/xkey_helper.c \
+ $(top_srcdir)/src/openvpn/xkey_provider.c \
$(top_srcdir)/src/openvpn/buffer.c \
$(top_srcdir)/src/openvpn/base64.c \
$(top_srcdir)/src/openvpn/platform.c \
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c
b/tests/unit_tests/openvpn/test_cryptoapi.c
index ccb3207c..b07e8935 100644
--- a/tests/unit_tests/openvpn/test_cryptoapi.c
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -47,6 +47,7 @@
#include /* pull-in the whole file to test static functions */
struct management *management; /* global */
+static OSSL_PROVIDER *prov[2];
/* mock a management function that xkey_provider needs */
char *
@@ -66,6 +67,11 @@ OSSL_LIB_CTX *tls_libctx;
#define _countof(x) sizeof((x))/sizeof(*(x))
#endif
+/* A message for signing */
+static const char *test_msg = "Lorem ipsum dolor sit amet, consectetur "
+ "adipisici elit, sed eiusmod tempor incidunt "
+ "ut labore et dolore magna aliqua.";
+
/* test data */
static const uint8_t test_hash[] = {
0x77, 0x38, 0x65, 0x00, 0x1e, 0x96, 0x48, 0xc6, 0x57, 0x0b, 0xae,
@@ -336,6 +342,164 @@ test_find_cert_byissuer(void **state)
gc_free();
}
+static int
+setup_cryptoapi_sign(void **state)
+{
+(void) state;
+/* Initialize providers in a way matching what OpenVPN core does */
+tls_libctx = OSSL_LIB_CTX_new();
+prov[0] = OSSL_PROVIDER_load(tls_libctx, "default");
+OSSL_PROVIDER_add_builtin(tls_libctx, "ovpn.xkey", xkey_provider_init);
+prov[1] = OSSL_PROVIDER_load(tls_libctx, "ovpn.xkey");
+
+/* set default propq as we do in ssl_openssl.c */
+EVP_set_default_properties(tls_libctx, "?provider!=ovpn.xkey");
+return 0;
+}
+
+static int
+teardown_cryptoapi_sign(void **state)
+{
+(void) state;
+for (size_t i = 0; i < _countof(prov); i++)
+{
+if (prov[i])
+{
+OSSL_PROVIDER_unload(prov[i]);
+prov[i] = NULL;
+}
+}
+OSSL_LIB_CTX_free(tls_libctx);
+tls_libctx = NULL;
+return 0;
+}
+
+/**
+ * Sign "test_msg" using a private key. The key may be a "provided" key
+ * in which case its signed by the provider's backend -- cryptoapi in our
+ * case. Then verify the signature using OpenSSL.
+ * Returns 1 on success, 0 on error.
+ */
+static int
+digest_sign_verify(EVP_PKEY *privkey, EVP_PKEY *pubkey)
+{
+uint8_t *sig = NULL;
+size_t siglen = 0;
+int ret = 0;
+
+OSSL_PARAM params[2] = {OSSL_PARAM_END};
+const char *mdname = "SHA256";
+
+if (EVP_PKEY_get_id(privkey) == EVP_PKEY_RSA)
+{
+const char *padmode = "pss"; /* RSA_PSS: for all other params, use
defaults */
+params[0] =
OSSL_PARAM_construct_utf8_string(OSSL_SIGNATURE_PARAM_PAD_MODE,
+ (char *)padmode, 0);
+params[1] = OSSL_PARAM_construct_end();
+}
+else if (EVP_PKEY_get_id(privkey) == EVP_PKEY_EC)
+{
+params[0] = OSSL_PARAM_construct_end();
+}
+else
+{
+print_error("Unknown key type in digest_sign_verify()");
+return ret;
+}
+
+EVP_PKEY_CTX *pctx = NULL;
+EVP_MD_CTX *mctx = EVP_MD_CTX_new();
+
+if (!mctx
+|| EVP_DigestSignInit_ex(mctx, , mdname, tls_libctx, NULL,
privkey, params) <= 0)
+{
+/* cmocka assert output for these kinds of failures is hardly
explanatory,
+ * print a message and assert in caller. */
+print_error("Failed to initialize EVP_DigestSignInit_ex()\n");
+goto done;
+}
+
+/* sign with sig = NULL to get required siglen */
+if (EVP_DigestSign(mctx, sig, , (uint8_t *)test_msg,
strlen(test_msg)) != 1)
+{
+print_error("EVP_DigestSign: failed to get required signature size");
+goto done;
+}
+assert_true(siglen > 0);
+
+if ((sig = tes
[Openvpn-devel] [PATCH 2/4] Add tests for finding certificates in Windows cert store
From: Selva Nair
- find_certificate_in_store tested using 'SUBJ:', 'THUMB:'
and 'ISSUER:' select strings. Uses test certificates
imported into the store during the import test.
Change-Id: Ib5138465e6228538af592ca98b3d877277355f59
Signed-off-by: Selva Nair
---
tests/unit_tests/openvpn/test_cryptoapi.c | 102 ++
1 file changed, 102 insertions(+)
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c
b/tests/unit_tests/openvpn/test_cryptoapi.c
index 54dbd094..ccb3207c 100644
--- a/tests/unit_tests/openvpn/test_cryptoapi.c
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -237,6 +237,105 @@ cleanup(void **state)
return 0;
}
+static void
+test_find_cert_bythumb(void **state)
+{
+(void) state;
+char select_string[64];
+struct gc_arena gc = gc_new();
+const CERT_CONTEXT *ctx;
+
+import_certs(state); /* a no-op if already imported */
+assert_non_null(user_store);
+
+for (struct test_cert *c = certs; c->cert; c++)
+{
+openvpn_snprintf(select_string, sizeof(select_string), "THUMB:%s",
c->hash);
+ctx = find_certificate_in_store(select_string, user_store);
+if (ctx)
+{
+/* check we got the right certificate and is valid */
+assert_int_equal(c->valid, 1);
+char *friendly_name = get_cert_name(ctx, );
+assert_string_equal(c->friendly_name, friendly_name);
+CertFreeCertificateContext(ctx);
+}
+else
+{
+/* find should fail only if the certificate has expired */
+assert_int_equal(c->valid, 0);
+}
+}
+
+gc_free();
+}
+
+static void
+test_find_cert_byname(void **state)
+{
+(void) state;
+char select_string[64];
+struct gc_arena gc = gc_new();
+const CERT_CONTEXT *ctx;
+
+import_certs(state); /* a no-op if already imported */
+assert_non_null(user_store);
+
+for (struct test_cert *c = certs; c->cert; c++)
+{
+openvpn_snprintf(select_string, sizeof(select_string), "SUBJ:%s",
c->cname);
+ctx = find_certificate_in_store(select_string, user_store);
+/* In this case we expect a successful return as there is at least one
valid
+ * cert that matches the common name. But the returned cert may not
exactly match
+ * c->cert as multiple certs with same common names exist in the db.
We check that
+ * the return cert is one from our db, has a matching common name and
is valid.
+ */
+assert_non_null(ctx);
+
+char *friendly_name = get_cert_name(ctx, );
+struct test_cert *found = lookup_cert(friendly_name);
+assert_non_null(found);
+assert_string_equal(found->cname, c->cname);
+assert_int_equal(found->valid, 1);
+CertFreeCertificateContext(ctx);
+}
+
+gc_free();
+}
+
+static void
+test_find_cert_byissuer(void **state)
+{
+(void) state;
+char select_string[64];
+struct gc_arena gc = gc_new();
+const CERT_CONTEXT *ctx;
+
+import_certs(state); /* a no-op if already imported */
+assert_non_null(user_store);
+
+for (struct test_cert *c = certs; c->cert; c++)
+{
+openvpn_snprintf(select_string, sizeof(select_string), "ISSUER:%s",
c->issuer);
+ctx = find_certificate_in_store(select_string, user_store);
+/* In this case we expect a successful return as there is at least one
valid
+ * cert that matches the issuer. But the returned cert may not exactly
match
+ * c->cert as multiple certs with same issuer exist in the db. We
check that
+ * the returned cert is one from our db, has a matching issuer name
and is valid.
+ */
+assert_non_null(ctx);
+
+char *friendly_name = get_cert_name(ctx, );
+struct test_cert *found = lookup_cert(friendly_name);
+assert_non_null(found);
+assert_string_equal(found->issuer, c->issuer);
+assert_int_equal(found->valid, 1);
+CertFreeCertificateContext(ctx);
+}
+
+gc_free();
+}
+
static void
test_parse_hexstring(void **state)
{
@@ -264,6 +363,9 @@ main(void)
const struct CMUnitTest tests[] = {
cmocka_unit_test(test_parse_hexstring),
cmocka_unit_test(import_certs),
+cmocka_unit_test(test_find_cert_bythumb),
+cmocka_unit_test(test_find_cert_byname),
+cmocka_unit_test(test_find_cert_byissuer),
};
int ret = cmocka_run_group_tests_name("cryptoapi tests", tests, NULL,
cleanup);
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 3/4] Refactor SSL_CTX_use_CryptoAPI_certificate()
From: Selva Nair
- Loading the certificate and key into the provider is split out of
setting up the SSL context. This allows testing of signing by
cryptoapi-provider interface without dependence on SSL context
or link-time wrapping.
Change-Id: I269b94589636425e1ba9bf953047d238fa830376
Signed-off-by: Selva Nair
---
src/openvpn/cryptoapi.c | 63 +++--
1 file changed, 41 insertions(+), 22 deletions(-)
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 022f53d4..20b7d985 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -401,11 +401,17 @@ get_cert_name(const CERT_CONTEXT *cc, struct gc_arena *gc)
return name;
}
-int
-SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
+/**
+ * Load certificate matching 'cert_prop' from Windows cert store
+ * into xkey provider and return pointers to X509 cert and private key.
+ * Returns 1 on success, 0 on error.
+ * Caller must free 'cert' and 'privkey' after use.
+ */
+static int
+Load_CryptoAPI_certificate(const char *cert_prop, X509 **cert, EVP_PKEY
**privkey)
{
+
HCERTSTORE cs;
-X509 *cert = NULL;
CAPI_DATA *cd = calloc(1, sizeof(*cd));
struct gc_arena gc = gc_new();
@@ -450,9 +456,9 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const
char *cert_prop)
}
/* cert_context->pbCertEncoded is the cert X509 DER encoded. */
-cert = d2i_X509(NULL, (const unsigned char **)
>cert_context->pbCertEncoded,
-cd->cert_context->cbCertEncoded);
-if (cert == NULL)
+*cert = d2i_X509(NULL, (const unsigned char **)
>cert_context->pbCertEncoded,
+ cd->cert_context->cbCertEncoded);
+if (*cert == NULL)
{
msg(M_NONFATAL, "Error in cryptoapicert: X509 certificate decode
failed");
goto err;
@@ -468,28 +474,16 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const
char *cert_prop)
/* private key may be in a token not available, or incompatible with
CNG */
msg(M_NONFATAL|M_ERRNO, "Error in cryptoapicert: failed to acquire
key. Key not present or "
"is in a legacy token not supported by Windows CNG API");
-goto err;
-}
-
-/* Public key in cert is NULL until we call SSL_CTX_use_certificate(),
- * so we do it here then... */
-if (!SSL_CTX_use_certificate(ssl_ctx, cert))
-{
+X509_free(*cert);
goto err;
}
/* the public key */
-EVP_PKEY *pkey = X509_get_pubkey(cert);
+EVP_PKEY *pkey = X509_get_pubkey(*cert);
cd->pubkey = pkey; /* will be freed with cd */
-/* SSL_CTX_use_certificate() increased the reference count in 'cert', so
- * we decrease it here with X509_free(), or it will never be cleaned up. */
-X509_free(cert);
-cert = NULL;
-
-EVP_PKEY *privkey = xkey_load_generic_key(tls_libctx, cd, pkey,
- xkey_cng_sign,
(XKEY_PRIVKEY_FREE_fn *) CAPI_DATA_free);
-SSL_CTX_use_PrivateKey(ssl_ctx, privkey);
+*privkey = xkey_load_generic_key(tls_libctx, cd, pkey,
+ xkey_cng_sign, (XKEY_PRIVKEY_FREE_fn *)
CAPI_DATA_free);
gc_free();
return 1; /* do not free cd -- its kept by xkey provider */
@@ -498,5 +492,30 @@ err:
gc_free();
return 0;
}
+
+int
+SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
+{
+X509 *cert = NULL;
+EVP_PKEY *privkey = NULL;
+int ret = 0;
+
+if (!Load_CryptoAPI_certificate(cert_prop, , ))
+{
+return ret;
+}
+if (SSL_CTX_use_certificate(ssl_ctx, cert)
+&& SSL_CTX_use_PrivateKey(ssl_ctx, privkey))
+{
+ret = 1;
+}
+
+/* Always free cert and privkey even if retained by ssl_ctx as
+ * they are reference counted */
+X509_free(cert);
+EVP_PKEY_free(privkey);
+return ret;
+}
+
#endif /* HAVE_XKEY_PROVIDER */
#endif /* _WIN32 */
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 0/4] Add some tests for cryptoapi.c functions
From: Selva Nair Import some sample certificates into Windows store for testing - 4 test certificates imported to user store and removed at the end. Add tests for finding certificates in Windows certficate store - test using SUBJ:, THUMB: and ISSUER: select-strings Refactor SSL_CTX_use_CryptoAPI_certificate() - A minor reorganization avoids wrapping for the next test Add a test for signing with certificates in Windows store - Test loading keys into xkey-provider and sign a test message. The signature is then verified using OpenSSL. Sample output the test runs: https://github.com/selvanair/openvpn/actions/runs/4418774866/jobs/7746404938#step:8:1 src/openvpn/cryptoapi.c | 63 ++-- tests/unit_tests/openvpn/Makefile.am | 1 + tests/unit_tests/openvpn/cert_data.h | 166 + tests/unit_tests/openvpn/test_cryptoapi.c | 429 +- 4 files changed, 635 insertions(+), 24 deletions(-) create mode 100644 tests/unit_tests/openvpn/cert_data.h -- 2.34.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Bugfix: Convert ECDSA signature form pkcs11-helper to DER encoded form
Hi, On Tue, Mar 14, 2023 at 5:54 AM David Sommerseth wrote: > > > Just got feedback from the reporter in the Fedora bugzilla; this patch > works well on Fedora 38. > > I suggest adding this tag to the commit log. Feel free to add the URL > tag to the bugzilla ticket too. > > Tested-by: flor...@apolloner.eu Thanks. Updated patch is on the list. I "heard" whispers of "2.6.2 coming soon" in another thread. Would be great if this fix can make it. As this also touches cryptoapicert (via a refactor), an independent test on Windows would be useful. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2] Bugfix: Convert ECDSA signature form pkcs11-helper to DER encoded form
From: Selva Nair
- With OpenSSL 3.0 and xkey-provider, we use pkcs11h_certificate_signAny_ex()
which returns EC signature as raw r|s concatenated. But OpenSSL expects
a DER encoded ASN.1 structure.
Do this conversion as done in cryptoapi.c. For code re-use, ecdsa_bin2sig()
is consolidated with sig to DER conversion as ecdsa_bin2der() and
moved to xkey_helper.c
In the past when we used OpenSSL hooks installed by pkcs11-helper,
such a conversion was not required as it was internally handled by
the library.
Reported by: Tom
Also see: https://bugzilla.redhat.com/show_bug.cgi?id=2177834
Tested-by: Florian Apolloner
Change-Id: Ie20cf81edd643ab8ef3c41321353d11fd66c188c
Signed-off-by: Selva Nair
---
Same as v1 with Tested by: and link to redhat bugzilla ticket added
src/openvpn/cryptoapi.c | 61 +---
src/openvpn/pkcs11_openssl.c | 23 --
src/openvpn/xkey_common.h| 15 +
src/openvpn/xkey_helper.c| 45 ++
4 files changed, 88 insertions(+), 56 deletions(-)
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 136c6ffc..022f53d4 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -146,40 +146,6 @@ CAPI_DATA_free(CAPI_DATA *cd)
free(cd);
}
-/**
- * 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;
-}
-
/**
* Parse a hex string with optional embedded spaces into
* a byte array.
@@ -287,12 +253,11 @@ static int
xkey_cng_ec_sign(CAPI_DATA *cd, unsigned char *sig, size_t *siglen, const
unsigned char *tbs,
size_t tbslen)
{
-BYTE buf[1024]; /* large enough for EC keys upto 1024 bits */
-DWORD len = _countof(buf);
+DWORD len = *siglen;
msg(D_LOW, "Signing using NCryptSignHash with EC key");
-DWORD status = NCryptSignHash(cd->crypt_prov, NULL, (BYTE *)tbs, tbslen,
buf, len, , 0);
+DWORD status = NCryptSignHash(cd->crypt_prov, NULL, (BYTE *)tbs, tbslen,
sig, len, , 0);
if (status != ERROR_SUCCESS)
{
@@ -301,26 +266,14 @@ xkey_cng_ec_sign(CAPI_DATA *cd, unsigned char *sig,
size_t *siglen, const unsign
return 0;
}
-/* NCryptSignHash returns r|s -- convert to OpenSSL's ECDSA_SIG */
-ECDSA_SIG *ecsig = ecdsa_bin2sig(buf, len);
-if (!ecsig)
+/* NCryptSignHash returns r|s -- convert to DER encoded buffer expected by
OpenSSL */
+int derlen = ecdsa_bin2der(sig, (int) len, *siglen);
+if (derlen <= 0)
{
-msg(M_NONFATAL, "Error in cryptopicert: Failed to convert ECDSA
signature");
return 0;
}
-
-/* convert internal signature structure 's' to DER encoded byte array in
sig */
-if (i2d_ECDSA_SIG(ecsig, NULL) > EVP_PKEY_size(cd->pubkey))
-{
-ECDSA_SIG_free(ecsig);
-msg(M_NONFATAL, "Error in cryptoapicert: DER encoded ECDSA signature
is too long");
-return 0;
-}
-
-*siglen = i2d_ECDSA_SIG(ecsig, );
-ECDSA_SIG_free(ecsig);
-
-return (*siglen > 0);
+*siglen = derlen;
+return 1;
}
/** Sign hash in tbs using RSA key in cd and NCryptSignHash */
diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index 5c1de44e..eee86e17 100644
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -168,6 +168,7 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
unsigned char buf[EVP_MAX_MD_SIZE];
size_t buflen;
+size_t siglen_max = *siglen;
unsigned char enc[EVP_MAX_MD_SIZE + 32]; /* 32 bytes enough for DigestInfo
header */
size_t enc_len = sizeof(enc);
@@ -234,8 +235,26 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
ASSERT(0); /* coding error -- we couldnt have created any such key */
}
-return CKR_OK == pkcs11h_certificate_signAny_ex(cert, ,
-tbs, tbslen, sig, siglen);
+if (CKR_OK != pkcs11h_certificate_signAny_ex(cert, ,
+ tbs, tbslen, sig, siglen))
+{
+return 0;
+}
+if (strcmp(sigalg.keytype, "EC"))
+{
+
[Openvpn-devel] [PATCH] Bugfix: Convert ECDSA signature form pkcs11-helper to DER encoded form
From: Selva Nair
- With OpenSSL 3.0 and xkey-provider, we use pkcs11h_certificate_signAny_ex()
which returns EC signature as raw r|s concatenated. But OpenSSL expects
a DER encoded ASN.1 structure.
Do this conversion as done in cryptoapi.c. For code re-use, ecdsa_bin2sig()
is consolidated with sig to DER conversion as ecdsa_bin2der() and
moved to xkey_helper.c
In the past when we used OpenSSL hooks installed by pkcs11-helper,
such a conversion was not required as it was internally handled by
the library.
Reported by: Tom
Signed-off-by: Selva Nair
---
src/openvpn/cryptoapi.c | 61 +---
src/openvpn/pkcs11_openssl.c | 23 --
src/openvpn/xkey_common.h| 15 +
src/openvpn/xkey_helper.c| 45 ++
4 files changed, 88 insertions(+), 56 deletions(-)
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 136c6ffc..022f53d4 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -146,40 +146,6 @@ CAPI_DATA_free(CAPI_DATA *cd)
free(cd);
}
-/**
- * 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;
-}
-
/**
* Parse a hex string with optional embedded spaces into
* a byte array.
@@ -287,12 +253,11 @@ static int
xkey_cng_ec_sign(CAPI_DATA *cd, unsigned char *sig, size_t *siglen, const
unsigned char *tbs,
size_t tbslen)
{
-BYTE buf[1024]; /* large enough for EC keys upto 1024 bits */
-DWORD len = _countof(buf);
+DWORD len = *siglen;
msg(D_LOW, "Signing using NCryptSignHash with EC key");
-DWORD status = NCryptSignHash(cd->crypt_prov, NULL, (BYTE *)tbs, tbslen,
buf, len, , 0);
+DWORD status = NCryptSignHash(cd->crypt_prov, NULL, (BYTE *)tbs, tbslen,
sig, len, , 0);
if (status != ERROR_SUCCESS)
{
@@ -301,26 +266,14 @@ xkey_cng_ec_sign(CAPI_DATA *cd, unsigned char *sig,
size_t *siglen, const unsign
return 0;
}
-/* NCryptSignHash returns r|s -- convert to OpenSSL's ECDSA_SIG */
-ECDSA_SIG *ecsig = ecdsa_bin2sig(buf, len);
-if (!ecsig)
+/* NCryptSignHash returns r|s -- convert to DER encoded buffer expected by
OpenSSL */
+int derlen = ecdsa_bin2der(sig, (int) len, *siglen);
+if (derlen <= 0)
{
-msg(M_NONFATAL, "Error in cryptopicert: Failed to convert ECDSA
signature");
return 0;
}
-
-/* convert internal signature structure 's' to DER encoded byte array in
sig */
-if (i2d_ECDSA_SIG(ecsig, NULL) > EVP_PKEY_size(cd->pubkey))
-{
-ECDSA_SIG_free(ecsig);
-msg(M_NONFATAL, "Error in cryptoapicert: DER encoded ECDSA signature
is too long");
-return 0;
-}
-
-*siglen = i2d_ECDSA_SIG(ecsig, );
-ECDSA_SIG_free(ecsig);
-
-return (*siglen > 0);
+*siglen = derlen;
+return 1;
}
/** Sign hash in tbs using RSA key in cd and NCryptSignHash */
diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index 5c1de44e..eee86e17 100644
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -168,6 +168,7 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
unsigned char buf[EVP_MAX_MD_SIZE];
size_t buflen;
+size_t siglen_max = *siglen;
unsigned char enc[EVP_MAX_MD_SIZE + 32]; /* 32 bytes enough for DigestInfo
header */
size_t enc_len = sizeof(enc);
@@ -234,8 +235,26 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
ASSERT(0); /* coding error -- we couldnt have created any such key */
}
-return CKR_OK == pkcs11h_certificate_signAny_ex(cert, ,
-tbs, tbslen, sig, siglen);
+if (CKR_OK != pkcs11h_certificate_signAny_ex(cert, ,
+ tbs, tbslen, sig, siglen))
+{
+return 0;
+}
+if (strcmp(sigalg.keytype, "EC"))
+{
+return 1;
+}
+
+/* For EC keys, pkcs11 returns signature as r|s: convert to der encoded */
+int derlen = ecdsa_bin2der(sig, (int) *siglen, siglen_max);
+
+if (derlen <= 0)
+{
+return 0;
+
Re: [Openvpn-devel] [PATCH] tests/unit_tests: Fix 'make distcheck' with subdir-objects enabled
Hi, On Wed, Mar 8, 2023 at 10:08 AM Frank Lichtenheld wrote: > Commit 7f72abcf8a56bb35a510a3409e03a4e2aaba50da enabled subdir-objects > when using automake 1.16+. > > There is an issue with the handling of .deps directories with this option. > While automake 1.16 fixed subdir-objects to work at all when _SOURCES > contains "unexpanded references" and it did fix subdir-objects to work > with out-of-tree build for "source files specified with an explicit > '$(srcdir)'" those fixes are not transitive. "unexpanded references" > still break out-of-tree builds when enforcing a read-only source dir > like 'make distcheck' does. When using *explicit* references to > srcdir and top_srcdir it works correctly. > Thanks for quickly finding a fix for this. Even after re-reading the related changelog in automake a dozen times, I cannot figure arbitrary variable expansion won't work as expected for out-of-tree builds with read-only sources. But testing does show only $(srcdir) and $(top_srcdir) get correctly handled, and the fix below appears to be a reasonable way out. Tested "make distcheck" and also compared with a locally generated search and replace version. LGTM. Acked-by: Selva Nair We waited long enough to enable subdir-objects though automake has been warning about this for almost a decade. Now they have softened their stand on it from "this behaviour will change ..." and "unconditionally cause ..." etc.. to "may change" in future. And, the implementation is still lacking. In retrospect, we (I) could have waited longer. Selva > Signed-off-by: Frank Lichtenheld > --- > tests/unit_tests/openvpn/Makefile.am | 220 +- > tests/unit_tests/plugins/auth-pam/Makefile.am | 6 +- > 2 files changed, 110 insertions(+), 116 deletions(-) > > diff --git a/tests/unit_tests/openvpn/Makefile.am > b/tests/unit_tests/openvpn/Makefile.am > index ee0a3d8a..339c7ef3 100644 > --- a/tests/unit_tests/openvpn/Makefile.am > +++ b/tests/unit_tests/openvpn/Makefile.am > @@ -30,189 +30,185 @@ if HAVE_SITNL > check_PROGRAMS += networking_testdriver > endif > > -openvpn_includedir = $(top_srcdir)/include > -openvpn_srcdir = $(top_srcdir)/src/openvpn > -compat_srcdir = $(top_srcdir)/src/compat > - > -argv_testdriver_CFLAGS = @TEST_CFLAGS@ -I$(openvpn_srcdir) > -I$(compat_srcdir) > -argv_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) > -Wl,--wrap=parse_line > +argv_testdriver_CFLAGS = @TEST_CFLAGS@ -I$(top_srcdir)/src/openvpn > -I$(top_srcdir)/src/compat > +argv_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(top_srcdir)/src/openvpn > -Wl,--wrap=parse_line > argv_testdriver_SOURCES = test_argv.c mock_msg.c mock_msg.h \ > mock_get_random.c \ > - $(openvpn_srcdir)/platform.c \ > - $(openvpn_srcdir)/buffer.c \ > - $(openvpn_srcdir)/win32-util.c \ > - $(openvpn_srcdir)/argv.c > + $(top_srcdir)/src/openvpn/platform.c \ > + $(top_srcdir)/src/openvpn/buffer.c \ > + $(top_srcdir)/src/openvpn/win32-util.c \ > + $(top_srcdir)/src/openvpn/argv.c > > -buffer_testdriver_CFLAGS = @TEST_CFLAGS@ -I$(openvpn_srcdir) > -I$(compat_srcdir) > -buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) > -Wl,--wrap=parse_line > +buffer_testdriver_CFLAGS = @TEST_CFLAGS@ -I$(top_srcdir)/src/openvpn > -I$(top_srcdir)/src/compat > +buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(top_srcdir)/src/openvpn > -Wl,--wrap=parse_line > buffer_testdriver_SOURCES = test_buffer.c mock_msg.c mock_msg.h \ > mock_get_random.c \ > - $(openvpn_srcdir)/win32-util.c \ > - $(openvpn_srcdir)/platform.c > + $(top_srcdir)/src/openvpn/win32-util.c \ > + $(top_srcdir)/src/openvpn/platform.c > > crypto_testdriver_CFLAGS = @TEST_CFLAGS@ \ > - -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) > + -I$(top_srcdir)/include -I$(top_srcdir)/src/compat > -I$(top_srcdir)/src/openvpn > crypto_testdriver_LDFLAGS = @TEST_LDFLAGS@ > crypto_testdriver_SOURCES = test_crypto.c mock_msg.c mock_msg.h \ > - $(openvpn_srcdir)/buffer.c \ > - $(openvpn_srcdir)/crypto.c \ > - $(openvpn_srcdir)/crypto_mbedtls.c \ > - $(openvpn_srcdir)/crypto_openssl.c \ > - $(openvpn_srcdir)/otime.c \ > - $(openvpn_srcdir)/packet_id.c \ > - $(openvpn_srcdir)/platform.c \ > - $(openvpn_srcdir)/mtu.c \ > - $(openvpn_srcdir)/win32-util.c \ > - $(openvpn_srcdir)/mss.c > + $(top_srcdir)/src/openvpn/buffer.c \ > + $(top_srcdir)/src/openvpn/crypto.c \ > + $(top_srcdir)/src/openvpn/crypto_mbedtls.c \ > + $(top_srcdir)/src/openvpn/crypto_openssl.c \ > + $(top_sr
Re: [Openvpn-devel] [PATCH 2/2] Include supplementary groups when checking management-client-group
Hi
FTR, I just noticed that the patch is missing an endgrent() call:
On Mon, Mar 6, 2023 at 12:33 AM wrote:
>
> +struct group *gr = getgrent();
> +char **members = NULL;
> +while (gr)
> +{
> +if (gr->gr_gid == gid)
> +{
> +/* found the group -- check user is a member */
> +members = gr->gr_mem;
> +for (char *s = *members; s && !ret; s++)
> +{
> +ret = !strcmp(s, pw->pw_name);
> +}
> +break; /* out of the while loop */
> +}
> +gr = getgrent();
> +}
>
endgrent();
> +#endif /* if defined(HAVE_GETPWUID) && defined(HAVE_GETGRENT) */
> +return ret;
> +}
> +
>
Will delay v2 depending on the fate of this patch.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] Include supplementary groups when checking management-client-group
Hi, On Mon, Mar 6, 2023 at 3:24 AM Gert Doering wrote: > Hi, > > On Mon, Mar 06, 2023 at 12:33:46AM -0500, selva.n...@gmail.com wrote: > > From: Selva Nair > > > > - When management-client-group is in use, allow access if any of > > the supplementary groups of the user matches the specified group. > > > > Currently only the effective gid of the peer socket is checked > > which is normally the primary group of user. As unprivileged users > > have no easy way of changing the effective gid of a process, > > group based access control is of very limited use without this change. > > I'm not really convinced this is a good way forward - it's yet another > extra function for a very niche use case, as in "the code has been like > this for 100 years, and nobody has ever used it". > > What you do is also not exactly "the user connecting has this gid at the > time of connection" but "the uid reported by getpeerid() has this group > listed in /etc/groups", which will be the same in many cases, but is > still checking something different. > I was of the same opinion when I first saw issue #264, but I think the original intent of "--management-client-group has to be what I've implemented or something similar. Docs refer to it as group (not effective gid of the socket), and restricting based on gid reported by getpeerid is kind of useless. Why use "group" if it can't be used to restrict access to a set of users using a supplementary group... I would propose to extend the documentation with "this only checks the > primary gid at time of connection, if this does not work for your use > case, put the socket into a directory with the right gid and mode 750 > and do not use that openvpn option" - as you wrote in the github ticket. > It's impossible to save that option by documenting. In that case, let's just make it a no-op and document that this option has no effect. And suggest the above for how to achieve group-based access control.. > > - Also accept if uid = 0 irrespective of the group. > > This part is fine for me, as it follows the traditional unix way of > "root may pass". > I'm not keen on touching some code and ignore that a closely related code stays crippled (IMO). Unless we can make --management-client-group a no-op as mentioned above. As you say this is an obscure option -- we can leave it "as is". Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 2/2] Include supplementary groups when checking management-client-group
From: Selva Nair
- When management-client-group is in use, allow access if any of
the supplementary groups of the user matches the specified group.
Currently only the effective gid of the peer socket is checked
which is normally the primary group of user. As unprivileged users
have no easy way of changing the effective gid of a process,
group based access control is of very limited use without this change.
- Also accept if uid = 0 irrespective of the group.
Github: OpenVPN/openvpn#264
Signed-off-by: Selva Nair
---
configure.ac | 2 +-
src/openvpn/manage.c | 43 ++-
2 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac
index 67f680b2..979903a8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -659,7 +659,7 @@ AC_CHECK_FUNCS([ \
daemon chroot getpwnam setuid nice system dup dup2 \
syslog openlog mlockall getrlimit getgrnam setgid \
setgroups flock readv writev time gettimeofday \
- setsid chdir \
+ setsid chdir getpwuid getgrent \
chsize ftruncate execve getpeereid basename dirname access \
epoll_create strsep \
])
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index db88e347..73dc7d57 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -1771,6 +1771,42 @@ man_new_connection_post(struct management *man, const
char *description)
}
#if UNIX_SOCK_SUPPORT
+/**
+ * Return true if user uid is a member of group gid.
+ * On error or lack of platform support, we return false.
+ */
+
+static bool
+is_user_in_group(uid_t uid, gid_t gid)
+{
+bool ret = false;
+#if defined(HAVE_GETPWUID) && defined(HAVE_GETGRENT)
+struct passwd *pw = getpwuid(uid);
+if (!pw)
+{
+return ret;
+}
+
+struct group *gr = getgrent();
+char **members = NULL;
+while (gr)
+{
+if (gr->gr_gid == gid)
+{
+/* found the group -- check user is a member */
+members = gr->gr_mem;
+for (char *s = *members; s && !ret; s++)
+{
+ret = !strcmp(s, pw->pw_name);
+}
+break; /* out of the while loop */
+}
+gr = getgrent();
+}
+#endif /* if defined(HAVE_GETPWUID) && defined(HAVE_GETGRENT) */
+return ret;
+}
+
static bool
man_verify_unix_peer_uid_gid(struct management *man, const socket_descriptor_t
sd)
{
@@ -1780,13 +1816,18 @@ man_verify_unix_peer_uid_gid(struct management *man,
const socket_descriptor_t s
int uid, gid;
if (unix_socket_get_peer_uid_gid(man->connection.sd_cli, , ))
{
+if (uid == 0) /* accept if root */
+{
+return true;
+}
if (man->settings.client_uid != -1 && man->settings.client_uid !=
uid)
{
msg(D_MANAGEMENT, "%s UID of socket peer (%d) doesn't match
required value (%d) as given by --management-client-user",
err_prefix, uid, man->settings.client_uid);
return false;
}
-if (man->settings.client_gid != -1 && man->settings.client_gid !=
gid)
+if (man->settings.client_gid != -1 && man->settings.client_gid !=
gid
+&& !is_user_in_group(uid, man->settings.client_gid))
{
msg(D_MANAGEMENT, "%s GID of socket peer (%d) doesn't match
required value (%d) as given by --management-client-group",
err_prefix, gid, man->settings.client_gid);
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 1/2] Do not save pointer to 'struct passwd' returned by getpwnam etc.
From: Selva Nair
- This pointer is to a static area which can change on further
calls to getpwnam, getpwuid etc.
Same with struct group returned by getgrnam.
As the only field later referred to is uid or gid, fix
by saving them instead.
Signed-off-by: Selva Nair
---
Though we call getpwnam()/getgrnam() more than once with potentially
different user/group names, this has not yet led to a bug because
only in one place the state is persisted, and that call happens later.
But looks like a bug waiting to happen.
src/openvpn/platform.c | 36 +++-
src/openvpn/platform.h | 14 --
src/openvpn/tun.c | 4 ++--
3 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
index 580c4cb8..f6b856f3 100644
--- a/src/openvpn/platform.c
+++ b/src/openvpn/platform.c
@@ -85,11 +85,16 @@ platform_user_get(const char *username, struct
platform_state_user *state)
if (username)
{
#if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID)
-state->pw = getpwnam(username);
-if (!state->pw)
+state->uid = -1;
+const struct passwd *pw = getpwnam(username);
+if (!pw)
{
msg(M_ERR, "failed to find UID for user %s", username);
}
+else
+{
+state->uid = pw->pw_uid;
+}
state->username = username;
ret = true;
#else /* if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID) */
@@ -103,9 +108,9 @@ static void
platform_user_set(const struct platform_state_user *state)
{
#if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID)
-if (state->username && state->pw)
+if (state->username && state->uid >= 0)
{
-if (setuid(state->pw->pw_uid))
+if (setuid(state->uid))
{
msg(M_ERR, "setuid('%s') failed", state->username);
}
@@ -124,11 +129,16 @@ platform_group_get(const char *groupname, struct
platform_state_group *state)
if (groupname)
{
#if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID)
-state->gr = getgrnam(groupname);
-if (!state->gr)
+state->gid = -1;
+const struct group *gr = getgrnam(groupname);
+if (!gr)
{
msg(M_ERR, "failed to find GID for group %s", groupname);
}
+else
+{
+state->gid = gr->gr_gid;
+}
state->groupname = groupname;
ret = true;
#else /* if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID) */
@@ -142,9 +152,9 @@ static void
platform_group_set(const struct platform_state_group *state)
{
#if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID)
-if (state->groupname && state->gr)
+if (state->groupname && state->gid >= 0)
{
-if (setgid(state->gr->gr_gid))
+if (setgid(state->gid))
{
msg(M_ERR, "setgid('%s') failed", state->groupname);
}
@@ -152,7 +162,7 @@ platform_group_set(const struct platform_state_group *state)
#ifdef HAVE_SETGROUPS
{
gid_t gr_list[1];
-gr_list[0] = state->gr->gr_gid;
+gr_list[0] = state->gid;
if (setgroups(1, gr_list))
{
msg(M_ERR, "setgroups('%s') failed", state->groupname);
@@ -225,13 +235,13 @@ platform_user_group_set(const struct platform_state_user
*user_state,
* new_uid/new_gid defaults to -1, which will not make
* libcap-ng change the UID/GID unless configured
*/
-if (group_state->groupname && group_state->gr)
+if (group_state->groupname && group_state->gid >= 0)
{
-new_gid = group_state->gr->gr_gid;
+new_gid = group_state->gid;
}
-if (user_state->username && user_state->pw)
+if (user_state->username && user_state->uid >= 0)
{
-new_uid = user_state->pw->pw_uid;
+new_uid = user_state->uid;
}
/* Prepare capabilities before dropping UID/GID */
diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h
index a35a571c..d8dad74b 100644
--- a/src/openvpn/platform.h
+++ b/src/openvpn/platform.h
@@ -63,7 +63,7 @@ struct context;
struct platform_state_user {
#if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID)
const char *username;
-struct passwd *pw;
+uid_t uid;
#else
int dummy;
#endif
@@ -74,7 +74,7 @@ struct platform_state_user {
struct platform_state_group {
#if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID)
const char *groupname;
-struct group *gr;
+gid_t gid;
#else
int dummy;
#endif
@@ -97,10 +97,7 @@ static inline int
platform_state_user_uid(const struct platform_state_user *s)
{
Re: [Openvpn-devel] [PATCH applied] Re: Add a unit test for functions in cryptoapi.c
Hi
On Sat, Feb 25, 2023 at 11:29 AM Gert Doering wrote:
> Acked-by: Gert Doering
>
> Thanks for the v4. This enabled me to just push to GH to have
> to build and run the tests, without having to bother myself with
> copying binaries around :-)
>
> OTOH, there might be a bit of polishing needed - the other tests
> print out what they are doing ("Running 7 test(s)"), while the
> cryptoapi unit test "just succeeds". Is there something missing
> wrt cmocka initalization?
> https://github.com/cron2/openvpn/actions/runs/4270790193/jobs/7434873211
That one is for OpenSSL 1.1.1 while this test is built only for OpenSSL3,
so nothing to run. Do we need to continue testing Windows builds of 2.6 for
OpenSSL 1.1?
Logs for OpenSSL 3 build look okay:
>
https://github.com/OpenVPN/openvpn/actions/runs/4270860888/jobs/7434989645#logs
>
Run ./unittests/cryptoapi_testdriver.exe
> 4[==] Running 1 test(s).
> 5[ RUN ] test_parse_hexstring
> 6[ OK ] test_parse_hexstring
> 7[ PASSED ] 1 test(s).
> 8[==] 1 test(s) run.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v4 3/3] Add a unit test for functions in cryptoapi.c
From: Selva Nair
- Though named cryptoapi_testdriver, right now this only tests
parsing of thumbprint specified as a selector for --cryptioapicert
option. More tests coming..
v2: a line that belongs here was mistakenly included in the previous
commit. Corrected.
v3: add to list of tests run in github actions
v4: - correct comment above invalid strings (copy paste error)
- make invalid strings differ from correct value only in the
explicitly introduced invalid characters/separators (one had
two distinct errors which is not a robust test).
Signed-off-by: Selva Nair
---
Sorry for not noticing these earlier...
.github/workflows/build.yaml | 3 +
tests/unit_tests/openvpn/Makefile.am | 16 +++
tests/unit_tests/openvpn/test_cryptoapi.c | 126 ++
3 files changed, 145 insertions(+)
create mode 100644 tests/unit_tests/openvpn/test_cryptoapi.c
diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
index 699964fd..5bd6da89 100644
--- a/.github/workflows/build.yaml
+++ b/.github/workflows/build.yaml
@@ -243,6 +243,9 @@ jobs:
- name: Run bufferunit test
run: ./unittests/buffer_testdriver.exe
+ - name: Run cryptoapi unit test
+run: ./unittests/cryptoapi_testdriver.exe
+
- name: Run cryptounit test
run: ./unittests/crypto_testdriver.exe
diff --git a/tests/unit_tests/openvpn/Makefile.am
b/tests/unit_tests/openvpn/Makefile.am
index 8d2386e0..ee0a3d8a 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -17,6 +17,7 @@ endif
test_binaries += provider_testdriver
if WIN32
+test_binaries += cryptoapi_testdriver
LDADD = -lws2_32
endif
@@ -152,6 +153,21 @@ provider_testdriver_SOURCES = test_provider.c mock_msg.c \
$(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
+if WIN32
+cryptoapi_testdriver_CFLAGS = @TEST_CFLAGS@ \
+ -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+ $(OPTIONAL_CRYPTO_CFLAGS)
+cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+ $(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
+cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
+ $(openvpn_srcdir)/xkey_helper.c \
+ $(openvpn_srcdir)/buffer.c \
+ $(openvpn_srcdir)/base64.c \
+ $(openvpn_srcdir)/platform.c \
+ mock_get_random.c \
+ $(openvpn_srcdir)/win32-util.c
+endif
+
auth_token_testdriver_CFLAGS = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
$(OPTIONAL_CRYPTO_CFLAGS)
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c
b/tests/unit_tests/openvpn/test_cryptoapi.c
new file mode 100644
index ..73ef34e9
--- /dev/null
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -0,0 +1,126 @@
+/*
+ * OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ * Copyright (C) 2023 Selva Nair
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by the
+ * Free Software Foundation, either version 2 of the License,
+ * or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+#include "manage.h"
+#include "integer.h"
+#include "xkey_common.h"
+
+#if defined(HAVE_XKEY_PROVIDER) && defined (ENABLE_CRYPTOAPI)
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include /* pull-in the whole file to test static functions */
+
+struct management *management; /* global */
+
+/* mock a management function that xkey_provider needs */
+char *
+management_query_pk_sig(struct management *man, const char *b64_data,
+const char *algorithm)
+{
+(void) man;
+(void) b64_data;
+(void) algorithm;
+return NULL;
+}
+
+/* tls_libctx is defined in ssl_openssl.c which we do not want to compile in */
+OSSL_LIB_CTX *tls_libctx;
+
+#ifndef _countof
+#define _countof(x) sizeof((x))/sizeof(*(x))
+#endif
+
+/* test data */
+static const uint8_t test_hash[] = {
+0x77, 0x38, 0x65, 0x00, 0x1e, 0x96, 0x48, 0xc6, 0
[Openvpn-devel] [PATCH v3 3/3] Add a unit test for functions in cryptoapi.c
From: Selva Nair
- Though named cryptoapi_testdriver, right now this only tests
parsing of thumbprint specified as a selector for --cryptioapicert
option. More tests coming..
v2: a line that belongs here was mistakenly included in the previous
commit. Corrected.
v3: add to list of tests run in github actions
Signed-off-by: Selva Nair
fixup
---
.github/workflows/build.yaml | 3 +
tests/unit_tests/openvpn/Makefile.am | 16 +++
tests/unit_tests/openvpn/test_cryptoapi.c | 126 ++
3 files changed, 145 insertions(+)
create mode 100644 tests/unit_tests/openvpn/test_cryptoapi.c
diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
index 699964fd..5bd6da89 100644
--- a/.github/workflows/build.yaml
+++ b/.github/workflows/build.yaml
@@ -243,6 +243,9 @@ jobs:
- name: Run bufferunit test
run: ./unittests/buffer_testdriver.exe
+ - name: Run cryptoapi unit test
+run: ./unittests/cryptoapi_testdriver.exe
+
- name: Run cryptounit test
run: ./unittests/crypto_testdriver.exe
diff --git a/tests/unit_tests/openvpn/Makefile.am
b/tests/unit_tests/openvpn/Makefile.am
index 8d2386e0..ee0a3d8a 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -17,6 +17,7 @@ endif
test_binaries += provider_testdriver
if WIN32
+test_binaries += cryptoapi_testdriver
LDADD = -lws2_32
endif
@@ -152,6 +153,21 @@ provider_testdriver_SOURCES = test_provider.c mock_msg.c \
$(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
+if WIN32
+cryptoapi_testdriver_CFLAGS = @TEST_CFLAGS@ \
+ -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+ $(OPTIONAL_CRYPTO_CFLAGS)
+cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+ $(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
+cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
+ $(openvpn_srcdir)/xkey_helper.c \
+ $(openvpn_srcdir)/buffer.c \
+ $(openvpn_srcdir)/base64.c \
+ $(openvpn_srcdir)/platform.c \
+ mock_get_random.c \
+ $(openvpn_srcdir)/win32-util.c
+endif
+
auth_token_testdriver_CFLAGS = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
$(OPTIONAL_CRYPTO_CFLAGS)
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c
b/tests/unit_tests/openvpn/test_cryptoapi.c
new file mode 100644
index ..2bea3f42
--- /dev/null
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -0,0 +1,126 @@
+/*
+ * OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ * Copyright (C) 2023 Selva Nair
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by the
+ * Free Software Foundation, either version 2 of the License,
+ * or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+#include "manage.h"
+#include "integer.h"
+#include "xkey_common.h"
+
+#if defined(HAVE_XKEY_PROVIDER) && defined (ENABLE_CRYPTOAPI)
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include /* pull-in the whole file to test static functions */
+
+struct management *management; /* global */
+
+/* mock a management function that xkey_provider needs */
+char *
+management_query_pk_sig(struct management *man, const char *b64_data,
+const char *algorithm)
+{
+(void) man;
+(void) b64_data;
+(void) algorithm;
+return NULL;
+}
+
+/* tls_libctx is defined in ssl_openssl.c which we do not want to compile in */
+OSSL_LIB_CTX *tls_libctx;
+
+#ifndef _countof
+#define _countof(x) sizeof((x))/sizeof(*(x))
+#endif
+
+/* test data */
+static const uint8_t test_hash[] = {
+0x77, 0x38, 0x65, 0x00, 0x1e, 0x96, 0x48, 0xc6, 0x57, 0x0b, 0xae,
+0xc0, 0xb7, 0x96, 0xf9, 0x66, 0x4d, 0x5f, 0xd0, 0xb7
+};
+
+/* valid test strings to test with and without embedded and trailing spaces */
+static const char *valid_str[] = {
+"773865001e9648c6570baec0b796f
Re: [Openvpn-devel] [PATCH v3 2/3] Build unit tests in mingw Windows build
Hi, On Sat, Feb 11, 2023 at 1:32 PM Gert Doering wrote: > > > /bin/bash ../../../libtool --tag=CC --mode=link i686-w64-mingw32-gcc This is a 32 bit build which I never tried. Maybe cmocka is not built for 32 bit? > -I/home/gert/mingw/opt/include -I/home/gert/mingw/opt/include > -I../../../include -I/home/gert/mingw/opt/include -Wall > -Wno-stringop-truncation -I/home/gert/mingw/opt/include -std=c99 > -L/home/gert/mingw/lib -L/home/gert/mingw/opt/lib64 -lssl -lcrypto > -L/home/gert/mingw/opt/lib -llzo2 -L/home/gert/mingw/opt/lib -lcmocka > -L/home/gert/mingw/opt/lib -o example_testdriver.exe > example_testdriver-test.o > libtool: link: i686-w64-mingw32-gcc -I/home/gert/mingw/opt/include > -I/home/gert/mingw/opt/include -I../../../include > -I/home/gert/mingw/opt/include -Wall -Wno-stringop-truncation > -I/home/gert/mingw/opt/include -std=c99 -o .libs/example_testdriver.exe > example_testdriver-test.o -L/home/gert/mingw/lib > -L/home/gert/mingw/opt/lib64 -lssl -lcrypto -L/home/gert/mingw/opt/lib > /home/gert/mingw/opt/lib/liblzo2.a -lwinmm -lcmocka > /usr/bin/i686-w64-mingw32-ld: > example_testdriver-test.o:test.c:(.text+0x89): undefined reference to > `_assert_int_equal' > /usr/bin/i686-w64-mingw32-ld: > example_testdriver-test.o:test.c:(.text+0xc6): undefined reference to > `_assert_int_equal' > /usr/bin/i686-w64-mingw32-ld: > example_testdriver-test.o:test.c:(.text+0x153): undefined reference to > `_cmocka_run_group_tests' > I had seen something similar with the master branch -- the def had some functions missing. But 1.1.5 worked out of the box. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3 2/3] Build unit tests in mingw Windows build
Hi
On Fri, Feb 10, 2023 at 4:13 PM Gert Doering wrote:
> Hi,
>
> On Tue, Feb 07, 2023 at 07:59:25PM -0500, selva.n...@gmail.com wrote:
> > From: Selva Nair
> >
> > - Minor changes to the build system to include some
> > dependencies for Windows build
> >
> > - test_tls_crypt not built as it will pull in win32.c and
> > its dependencies
> >
> > - If cross-compiling, "make check" will only build the tests but not
> > run any. Copy to Windows and run manually. Executables are in
> > /tests/unit_tests/openvpn/.libs/ and these depend on
> > cmocka.dll in addition to openssl libs that some tests link to.
>
> I am something wrong, or I'm missing some bits.
>
> I took this patch, on top of commit e80720ef939 ("top of master as of
> now"). MinGW on Ubuntu. "autoreconf -iv", make clean, then
> make && make check.
>
> make check claims
>
> make[4]: Entering directory '/home/gert/mingw/openvpn.git/tests'
> ==
> All 0 tests passed
> ==
>
> ... but it seems to have done... nothing?
>
> gert@ubuntu2204:~/mingw/openvpn.git$ ls -l tests/unit_test/openvpn/.libs
> ls: cannot access 'tests/unit_test/openvpn/.libs': No such file or
> directory
>
> ... given argv_testdriver exists, I should find "something" with argv*.o*
> here...
>
> gert@ubuntu2204:~/mingw/openvpn.git$ find . -name "*argv*o"
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-mock_get_random.Po
> ./tests/unit_tests/openvpn/.deps/tls_crypt_testdriver-argv.Po
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-test_argv.Po
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-mock_msg.Po
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-platform.Po
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-buffer.Po
> ./tests/unit_tests/openvpn/.deps/pkt_testdriver-argv.Po
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-argv.Po
> ./src/openvpn/.deps/tls_crypt_testdriver-argv.Po
> ./src/openvpn/.deps/argv.Po
> ./src/openvpn/.deps/argv_testdriver-win32-util.Po
> ./src/openvpn/.deps/argv_testdriver-platform.Po
> ./src/openvpn/.deps/argv_testdriver-buffer.Po
> ./src/openvpn/.deps/pkt_testdriver-argv.Po
> ./src/openvpn/.deps/argv_testdriver-argv.Po
> ./src/openvpn/argv.o
>
> ... so it seems as if it did the "make depend" dance for the unit_tests,
> but then didn't compile anything for real.
>
>
> Did I apply stuff in the wrong order? Do I need to more forcefully clean
> my build tree?
>
Ignore the previous email -- (I behaved as if I can't read more than a
pageful).
Looks like unit-tests are not enabled during configure. For that, cmocka
should be available in the search path or set using CMOCKA_LIBS and
CMOCKA_INCLUDES.
It took a while for me to figure out how to cross-compile cmocka using
cmake and mingw on Linux. Finally what worked was pretty simple:
git clone https://github.com/clibs/cmocka.git
cd cmocka
checkout cmocka-1.1.5
mkdir build
cd build
cmake -DCMAKE_SYSTEM_NAME=Windows -DCMAKE_C_COMPILER=x86_64-w64-mingw32-gcc
\
-DCMAKE_SHARED_LINKER_FLAGS=-static-libgcc \
-DCMAKE_INSTALL_PREFIX=$HOME/mingw-cmocka/ -S .. -B .
make install
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3 2/3] Build unit tests in mingw Windows build
On Fri, Feb 10, 2023 at 4:13 PM Gert Doering wrote:
> Hi,
>
> On Tue, Feb 07, 2023 at 07:59:25PM -0500, selva.n...@gmail.com wrote:
> > From: Selva Nair
> >
> > - Minor changes to the build system to include some
> > dependencies for Windows build
> >
> > - test_tls_crypt not built as it will pull in win32.c and
> > its dependencies
> >
> > - If cross-compiling, "make check" will only build the tests but not
> > run any. Copy to Windows and run manually. Executables are in
> > /tests/unit_tests/openvpn/.libs/ and these depend on
> > cmocka.dll in addition to openssl libs that some tests link to.
>
> I am something wrong, or I'm missing some bits.
>
> I took this patch, on top of commit e80720ef939 ("top of master as of
> now"). MinGW on Ubuntu. "autoreconf -iv", make clean, then
> make && make check.
>
> make check claims
>
> make[4]: Entering directory '/home/gert/mingw/openvpn.git/tests'
> ==
> All 0 tests passed
> ==
>
> ... but it seems to have done... nothing?
>
My patch only enables to build the tests while cross-compiling but disables
running them. As the executables are not native, you have to manually copy
them to Windows and run there.
With the next patch from Arne, GHA actions will copy it automagically to
Windows and run there. But that works only for github actions.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Improve format specifier for socket handle in Windows
Hi On Fri, Feb 10, 2023 at 2:21 PM Antonio Quartulli wrote: > Hi, > > On 10/02/2023 14:31, Lev Stipakov wrote: > > From: Lev Stipakov > > > > Socket is a handle on Windows, which is usually logged in hex. > > Also an interesting value is INVALID_SOCKET, which is ~0. > > > > PRIuPTR prints decimals, and for INVALID_SOCKET it prints something like > > > >2023-02-10 14:45:21 us=906000 write to TUN/TAP : Jrjestelmkutsulle > > annettu data-alue on liian pieni. (fd=18446744073709551615,code=122) > > > > PRIxPTR prints hex, and INVALID_SOCKET looks a bit nicer: > > > >2023-02-10 15:17:11 us=828000 write to TUN/TAP : Jrjestelmkutsulle > > annettu data-alue on liian pieni. (fd=,code=122) > > > > Reported-by: Selva Nair > > Signed-off-by: Lev Stipakov > > I also discussed this with Lev and, despite this being different from > what we do in the *nix world (where decimal representations make sense > for file descriptors), it seems to be the right hting to do on Windows > when using HANDLEs (shrug). > Windows kernel handles are very much "like" fds though the handle table also covers processes, threads, semaphores etc. They are not pointers, are generally smallish numbers or ~0 (== -1). But not as small as fds in the Unix/Linux world --- mainly because several objects share the name space and they have to be multiples of 4 for some arcane reason. The problem here was not printing as "decimal" but as "unsigned decimal". Use of the latter would have caused the same issue on Linux too. Using hex is a compromise here as handles are defined as "unsigned ints" of the same size as pointers. So PRIdPTR somehow looks improper though I would have chosen that. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/5] Do not run check_engine test when crosscompiling
Hi, On Tue, Feb 7, 2023 at 7:19 PM Arne Schwabe wrote: > Signed-off-by: Arne Schwabe > --- > tests/unit_tests/engine-key/Makefile.am | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tests/unit_tests/engine-key/Makefile.am > b/tests/unit_tests/engine-key/Makefile.am > index 246222514..0c2888576 100644 > --- a/tests/unit_tests/engine-key/Makefile.am > +++ b/tests/unit_tests/engine-key/Makefile.am > @@ -12,7 +12,9 @@ TESTS_ENVIRONMENT = srcdir="$(abs_srcdir)"; \ > top_srcdir="$(top_srcdir)"; \ > export srcdir builddir top_builddir top_srcdir; > > +if !CROSS_COMPILING > TESTS = check_engine_keys.sh > +endif > check_engine_keys.sh: $(conffiles) > > CLEANFILES = \ > FTR, this patch is no longer needed (included in #2 below). So, the order of interdependent patches are: (All acked). 1. [Openvpn-devel,1/3] Conditionally add subdir-objects option to automake <https://patchwork.openvpn.net/project/openvpn2/patch/20230204004512.250271-1-selva.n...@gmail.com/> 2. [Openvpn-devel,v3,2/3] Build unit tests in mingw Windows build <https://patchwork.openvpn.net/project/openvpn2/patch/20230208005925.393200-1-selva.n...@gmail.com/> 3. [Openvpn-devel,4/5] Add missing stdint.h includes in unit tests files <https://patchwork.openvpn.net/project/openvpn2/patch/20230208001819.244694-5-a...@rfc2549.org/> 4. [Openvpn-devel,v2,5/5] Add building unit tests with mingw to github actions <https://patchwork.openvpn.net/project/openvpn2/patch/20230209163705.466173-1-a...@rfc2549.org/> Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] Fwd: [PATCH v2 3/5] Windows: fix wrong printf format in x_check_status
CC: list -- Forwarded message - From: Selva Nair Date: Tue, Feb 7, 2023 at 11:57 AM Subject: Re: [Openvpn-devel] [PATCH v2 3/5] Windows: fix wrong printf format in x_check_status To: Frank Lichtenheld Nitpicking: > - use PRIuPTR as discussed on IRC (added relevant defines to >commit message) > The most interesting value to log for SOCKET on Windows is INVALID_SOCKET = ~0 (i.e, -1). It will print with PRIuPTR as 18446744073709551615. PRIdPTR which will give -1 or PRIxPTR () may be better in this case? Though socket is a handle on Windows, I think it never gets too large to be -ve except for this special value. I'm fine with this too as unsigned decimals for -1 in both 32 bit and 64 bit are not hard to spot. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] Fwd: [PATCH 4/5] Add missing stdint.h includes in unit tests files
CC: list -- Forwarded message - From: Selva Nair Date: Wed, Feb 8, 2023 at 11:34 PM Subject: Re: [Openvpn-devel] [PATCH 4/5] Add missing stdint.h includes in unit tests files To: Arne Schwabe Hi, On Tue, Feb 7, 2023 at 7:19 PM Arne Schwabe wrote: > My mingw compiler/headers (mingw-w64 10.0.0 on macOS) seem to be more > pendantic than the one that comes with Ubuntu 22.04 (github actions) or > any of the other platforms including msvc/normal windows header. > > Signed-off-by: Arne Schwabe > --- > tests/unit_tests/example_test/test.c | 1 + > tests/unit_tests/example_test/test2.c | 1 + > tests/unit_tests/openvpn/mock_msg.c | 1 + > 3 files changed, 3 insertions(+) > > diff --git a/tests/unit_tests/example_test/test.c > b/tests/unit_tests/example_test/test.c > index ea31b884d..c174025cc 100644 > --- a/tests/unit_tests/example_test/test.c > +++ b/tests/unit_tests/example_test/test.c > @@ -4,6 +4,7 @@ > #include > #include > #include > +#include > #include > > static int > diff --git a/tests/unit_tests/example_test/test2.c > b/tests/unit_tests/example_test/test2.c > index 5a186d5d7..bb54633c8 100644 > --- a/tests/unit_tests/example_test/test2.c > +++ b/tests/unit_tests/example_test/test2.c > @@ -4,6 +4,7 @@ > #include > #include > #include > +#include > #include > > > diff --git a/tests/unit_tests/openvpn/mock_msg.c > b/tests/unit_tests/openvpn/mock_msg.c > index 3ede98c00..3fa9a166f 100644 > --- a/tests/unit_tests/openvpn/mock_msg.c > +++ b/tests/unit_tests/openvpn/mock_msg.c > @@ -32,6 +32,7 @@ > #include > #include > #include > +#include > #include Actually it may not be your setup: mingw on Linux also requires this if using cmocka-master branch. cmocka-1.1.5 works without this include for me. That would explain why we haven't seen this error yet in native builds. Anyway, cmocka docs also tell us to always include . Acked-by: Selva Nair ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] Fwd: [PATCH v2 5/5] Add building unit tests with mingw to github actions
CC: list was missed..
-- Forwarded message -
From: Selva Nair
Date: Thu, Feb 9, 2023 at 2:54 PM
Subject: Re: [Openvpn-devel] [PATCH v2 5/5] Add building unit tests with
mingw to github actions
To: Arne Schwabe
Hi,
Thanks, this is much better with tests grouped together.
Some nitpicks below that could be fixed while merging.
On Thu, Feb 9, 2023 at 11:38 AM Arne Schwabe wrote:
> This runs each test in its own action since order of stderr and stdout
> is seemingly random in github action Windows output and this way at least
> tests outputs are groups gy test
>
"test outputs are grouped by"
>
> Patch v2: use -static-libgcc to avoid comping gcc runtime libraries.
>
> Signed-off-by: Arne Schwabe
> ---
> .github/workflows/build.yaml | 111 ++-
> 1 file changed, 109 insertions(+), 2 deletions(-)
>
> diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
> index 5888e91e5..a1026fddb 100644
> --- a/.github/workflows/build.yaml
> +++ b/.github/workflows/build.yaml
> @@ -55,9 +55,10 @@ jobs:
>PKCS11_HELPER_VERSION: "1.29.0"
>OPENSSL_VERSION: "${{ matrix.osslver }}"
>TAP_WINDOWS_VERSION: "9.23.3"
> + CMOCKA_VERSION: "1.1.5"
> steps:
>- name: Install dependencies
> -run: sudo apt update && sudo apt install -y mingw-w64 libtool
> automake autoconf man2html unzip
> +run: sudo apt update && sudo apt install -y mingw-w64 libtool
> automake autoconf man2html unzip cmake ninja-build build-essential wget
>- name: Checkout OpenVPN
> uses: actions/checkout@v3
> with:
> @@ -72,7 +73,7 @@ jobs:
> uses: actions/cache@v3
> with:
>path: '~/mingw/'
> - key: ${{ matrix.target }}-mingw-${{ matrix.osslver }}-${{
> env.LZO_VERSION }}-${{ env.PKCS11_HELPER_VERSION }}-${{
> env.TAP_WINDOWS_VERSION }}
> + key: ${{ matrix.target }}-mingw-${{ matrix.osslver }}-${{
> env.LZO_VERSION }}-${{ env.PKCS11_HELPER_VERSION }}-${{
> env.TAP_WINDOWS_VERSION }}--${{ env.CMOCKA_VERSION }}
>
># Repeating if: steps.cache.outputs.cache-hit != 'true'
># on every step for building dependencies is ugly but
> @@ -84,12 +85,33 @@ jobs:
>wget -c -P download-cache/ "
> https://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip
> "
>wget -c -P download-cache/ "
> https://www.oberhumer.com/opensource/lzo/download/lzo-${LZO_VERSION}.tar.gz
> "
>wget -c -P download-cache/ "
> https://github.com/OpenSC/pkcs11-helper/releases/download/pkcs11-helper-${PKCS11_HELPER_VERSION}/pkcs11-helper-${PKCS11_HELPER_VERSION}.tar.bz2
> "
> + wget -c -P download-cache/ "
> https://github.com/coreboot/cmocka/archive/refs/tags/cmocka-${CMOCKA_VERSION}.tar.gz
> "
>tar jxf
> "download-cache/pkcs11-helper-${PKCS11_HELPER_VERSION}.tar.bz2"
>wget -c -P download-cache/ "
> https://www.openssl.org/source/old/1.1.1/openssl-${OPENSSL_VERSION}.tar.gz;
> || wget -c -P download-cache/ "
> https://www.openssl.org/source/openssl-${OPENSSL_VERSION}.tar.gz;
>tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
>tar zxf "download-cache/lzo-${LZO_VERSION}.tar.gz"
> + tar zxf "download-cache/cmocka-${CMOCKA_VERSION}.tar.gz"
>unzip download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip
>
> + - name: create cmocka build directory
> +if: steps.cache.outputs.cache-hit != 'true'
> +run: mkdir cmocka-build
> +
> + - name: configure cmocka
> +if: steps.cache.outputs.cache-hit != 'true'
> +working-directory: "./cmocka-build"
> +run: cmake -GNinja -DCMAKE_C_COMPILER=${{ matrix.chost }}-gcc
> -DCMAKE_CXX_COMPILER=${{ matrix.chost }}-g++ -DCMAKE_SYSTEM_NAME=Windows
> -DCMAKE_SHARED_LINKER_FLAGS=-static-libgcc
> -DCMAKE_PREFIX_PATH=${HOME}/mingw/opt/lib/pkgconfig/
> -DCMAKE_INCLUDE_PATH=${HOME}/mingw/opt/lib/include
> -DCMAKE_LIBRARY_PATH=${HOME}/mingw/opt/lib
> -DCMAKE_INSTALL_PREFIX=${HOME}/mingw/opt/ ../cmocka-cmocka-${{
> env.CMOCKA_VERSION }}
> +
> + - name: build cmocka
> +if: steps.cache.outputs.cache-hit != 'true'
> +working-directory: "./cmocka-build"
> +run: ninja
> +
> + - name: install cmocka
> +if: steps.cache.outputs.cache-hit != 'true'
> +working-directory: "./cmocka-build"
> +run: ninja install
> +
>- name: Configure OpenSSL
> if: steps.cache.out
Re: [Openvpn-devel] [PATCH 5/5] Add building and running mingw unittests to github actions
run: make -j3 check
> +working-directory: openvpn
> +
> + # We use multiple upload-artifact here, so it becomes a flat folder
> + # structure since we need the ddls on the same level as the binaries
>
ddls -> dlls
+ - name: Archive cmocka/openssl/lzo ddls
> +uses: actions/upload-artifact@v3
> +with:
> + retention-days: 1
> + name: mingw-unittest-${{matrix.target}}-ossl${{ matrix.osslver
> }}-dlls
> + path: '~/mingw/opt/bin/*.dll'
> +
> + - name: Archive gcc dlls
> +uses: actions/upload-artifact@v3
> +with:
> + retention-days: 1
> + name: mingw-unittest-${{matrix.target}}-ossl${{ matrix.osslver
> }}-dlls
> + path: /usr/lib/gcc/${{ matrix.chost }}/10-win32/*.dll
>
This section ("Archive gcc dlls") is not required when mocka.dll is built
with -static-libgcc as above. None of our exe's or dlls will have any
dependency on these.
> +
> + - name: Archive mingw dlls
> +uses: actions/upload-artifact@v3
> +with:
> + retention-days: 1
> + name: mingw-unittest-${{matrix.target}}-ossl${{ matrix.osslver
> }}-dlls
> + path: /usr/${{ matrix.chost }}/lib/*.dll
>
Same with this "Archive mingw dlls" section
I tested these changes here:
https://github.com/selvanair/openvpn/tree/automake-cryptoapi
+
> + # libtool puts some wrapper binaries in
> openvpn/tests/unit_tests/openvpn/
> + # and the real binaries in openvpn/tests/unit_tests/openvpn/.libs/
> + - name: Archive unittest artifacts
> +uses: actions/upload-artifact@v3
> +with:
> + retention-days: 1
> + name: mingw-unittest-${{matrix.target}}-ossl${{ matrix.osslver
> }}-tests
> + path: openvpn/tests/unit_tests/openvpn/.libs/*.exe
> +
> + # Currently not used by the unit test but might in the future and
> also
> + # helpful if manually downloading and running openvpn.exe from a
> mingw
> + # build
> + - name: Archive openvpn binary
> +uses: actions/upload-artifact@v3
> +with:
> + retention-days: 1
> + name: mingw-unittest-${{matrix.target}}-ossl${{ matrix.osslver
> }}-tests
> + path: openvpn/src/openvpn/.libs/*.exe
> +
> + mingw-unittest:
> +needs: [ mingw ]
> +strategy:
> + fail-fast: false
> + matrix:
> +osslver: [ 1.1.1q, 3.0.5 ]
> +target: [ mingw64, mingw ]
> +
> +runs-on: windows-latest
> +name: "mingw unittests - ${{matrix.target}} - OSSL ${{ matrix.osslver
> }}"
> +steps:
> + - name: Retrieve mingw unittest dlls
> +uses: actions/download-artifact@v3
> +with:
> + name: mingw-unittest-${{matrix.target}}-ossl${{ matrix.osslver
> }}-dlls
> + path: unittests
> +
> + - name: Retrieve mingw unittest
> +uses: actions/download-artifact@v3
> +with:
> + name: mingw-unittest-${{matrix.target}}-ossl${{ matrix.osslver
> }}-tests
> + path: unittests
> +
> + - name: List unittests directory
> +run: "dir unittests"
> +
> + - name: Run unit tests
> +run: |
> + ./unittests/argv_testdriver.exe
> + ./unittests/auth_token_testdriver.exe
> + ./unittests/buffer_testdriver.exe
> + ./unittests/crypto_testdriver.exe
> + ./unittests/misc_testdriver.exe
> + ./unittests/ncp_testdriver.exe
> + ./unittests/packet_id_testdriver.exe
> + ./unittests/pkt_testdriver.exe
> + ./unittests/provider_testdriver.exe
>
>ubuntu:
> strategy:
> @@ -442,3 +547,4 @@ jobs:
> run: make -j3
>- name: make check
> run: make check
> +
> --
A couple of minor issues with the way test results are reported:
(1) The main name of the test being run is not printed
Normally when run from make check, we will get "PASSED pkt_testdriver" etc
at the end which would be hard to recreate here. But we could add a line
saying "Running pkt_testdriver" at the top. The individual tests within
each exe are printed by cmocka, but unfortunately not all tests are
carefully named to quickly see which group it belongs to.
(2) Messages like "Running 4 tests", "PASSED 4 tests" etc., appear out of
order
Often "PASSED" appears before running tests or in the middle.
Eg.,
[==] 9 test(s) run.
[ PASSED ] 11 test(s).
[ PASSED ] 7 test(s).
[==] Running 11 test(s)
Then comes the description of those 11 tests.
Some tasks are running in parallel?
That said, reporting improvements could be done as a follow-up as this is
already quite useful.
Regards,
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/3] Add a unit test for functions in cryptoapi.c
On Wed, Feb 8, 2023 at 6:16 AM Arne Schwabe wrote: > Am 08.02.23 um 02:05 schrieb Selva Nair: > > Hi, > > > > On Tue, Feb 7, 2023 at 7:18 PM Arne Schwabe > <mailto:a...@rfc2549.org>> wrote: > > > > Am 04.02.23 um 07:40 schrieb selva.n...@gmail.com > > <mailto:selva.n...@gmail.com>: > > > From: Selva Nair selva.n...@gmail.com>> > > > > > > - Though named cryptoapi_testdriver, right now this only tests > > >parsing of thumbprint specified as a selector for > --cryptioapicert > > >option. More tests coming.. > > > > > > v2: a line that belongs here was mistakenly included in the > previous > > > commit. Corrected. > > > > > > Signed-off-by: Selva Nair > <mailto:selva.n...@gmail.com>> > > > > While a good step, this still has problems. With OpenSSL 1.1.1 it > still > > tries to run the check-engine.sh script and unsurprisingly fails: > > > > begin log.txt > > ./check_engine_keys.sh: 25: ../../../src/openvpn/openvpn: not found > > --- end log.txt --- > > Key mismatch not detected > > FAIL: check_engine_keys.sh > > > > This can be fixed by: > > > > --- a/tests/unit_tests/engine-key/Makefile.am > > +++ b/tests/unit_tests/engine-key/Makefile.am > > @@ -12,7 +12,9 @@ TESTS_ENVIRONMENT = srcdir="$(abs_srcdir)"; \ > > top_srcdir="$(top_srcdir)"; \ > > export srcdir builddir top_builddir top_srcdir; > > > > +if !CROSS_COMPILING > >TESTS = check_engine_keys.sh > > +endif > >check_engine_keys.sh: $(conffiles) > > > >CLEANFILES = \ > > > > > > Thanks for this. I added this to 2/3 where it belongs and submitted it > > as v3 2/3. With that, this patch should be okay as is. > > > > For those not following closely, this patch should be applied after the > > cryptoapi patch set where the function being tested here is introduced. > > > > Selva > > No, I think this that is independent. This is about the engine keys > support added by James Bottomley. I needed and tested this patch > without the cryptoapi patch. > Got that. I think gmail is messing up my thread view -- this email appear for me as linked to my original 3/3 which is on the test for cryptoapi. That's why I wrote "this patch should be okay as is." Anyway... Will look into your patches later today. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/3] Add a unit test for functions in cryptoapi.c
Hi, On Tue, Feb 7, 2023 at 7:18 PM Arne Schwabe wrote: > Am 04.02.23 um 07:40 schrieb selva.n...@gmail.com: > > From: Selva Nair > > > > - Though named cryptoapi_testdriver, right now this only tests > >parsing of thumbprint specified as a selector for --cryptioapicert > >option. More tests coming.. > > > > v2: a line that belongs here was mistakenly included in the previous > > commit. Corrected. > > > > Signed-off-by: Selva Nair > > While a good step, this still has problems. With OpenSSL 1.1.1 it still > tries to run the check-engine.sh script and unsurprisingly fails: > > begin log.txt > ./check_engine_keys.sh: 25: ../../../src/openvpn/openvpn: not found > --- end log.txt --- > Key mismatch not detected > FAIL: check_engine_keys.sh > > This can be fixed by: > > --- a/tests/unit_tests/engine-key/Makefile.am > +++ b/tests/unit_tests/engine-key/Makefile.am > @@ -12,7 +12,9 @@ TESTS_ENVIRONMENT = srcdir="$(abs_srcdir)"; \ > top_srcdir="$(top_srcdir)"; \ > export srcdir builddir top_builddir top_srcdir; > > +if !CROSS_COMPILING > TESTS = check_engine_keys.sh > +endif > check_engine_keys.sh: $(conffiles) > > CLEANFILES = \ > Thanks for this. I added this to 2/3 where it belongs and submitted it as v3 2/3. With that, this patch should be okay as is. For those not following closely, this patch should be applied after the cryptoapi patch set where the function being tested here is introduced. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v3 2/3] Build unit tests in mingw Windows build
From: Selva Nair
- Minor changes to the build system to include some
dependencies for Windows build
- test_tls_crypt not built as it will pull in win32.c and
its dependencies
- If cross-compiling, "make check" will only build the tests but not
run any. Copy to Windows and run manually. Executables are in
/tests/unit_tests/openvpn/.libs/ and these depend on
cmocka.dll in addition to openssl libs that some tests link to.
Building with mingw on Windows should run the tests (untested).
v2: networking_testdriver was mistakenly enabled to run, while
originally it was only set to build. Corrected.
v3: exclude check_engine_keys.sh when cross-compiling
As suggested by Arne Schwabe
Signed-off-by: Selva Nair
---
configure.ac | 2 ++
tests/Makefile.am | 2 ++
tests/unit_tests/engine-key/Makefile.am | 2 ++
tests/unit_tests/example_test/Makefile.am | 2 ++
tests/unit_tests/openvpn/Makefile.am | 28 +++
5 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac
index 95d795c3..66ba6f38 100644
--- a/configure.ac
+++ b/configure.ac
@@ -364,6 +364,8 @@ case "$host" in
;;
esac
+AM_CONDITIONAL([CROSS_COMPILING], test "${cross_compiling}" = "yes")
+
PKG_PROG_PKG_CONFIG
AC_PROG_CPP
AC_PROG_INSTALL
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 87dd7e17..a46f2573 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -14,10 +14,12 @@ MAINTAINERCLEANFILES = \
SUBDIRS = unit_tests
+if !WIN32
test_scripts = t_client.sh t_lpback.sh t_cltsrv.sh
if HAVE_SITNL
test_scripts += t_net.sh
endif
+endif
TESTS_ENVIRONMENT = top_srcdir="$(top_srcdir)"
TESTS = $(test_scripts)
diff --git a/tests/unit_tests/engine-key/Makefile.am
b/tests/unit_tests/engine-key/Makefile.am
index 24622251..0c288857 100644
--- a/tests/unit_tests/engine-key/Makefile.am
+++ b/tests/unit_tests/engine-key/Makefile.am
@@ -12,7 +12,9 @@ TESTS_ENVIRONMENT = srcdir="$(abs_srcdir)"; \
top_srcdir="$(top_srcdir)"; \
export srcdir builddir top_builddir top_srcdir;
+if !CROSS_COMPILING
TESTS = check_engine_keys.sh
+endif
check_engine_keys.sh: $(conffiles)
CLEANFILES = \
diff --git a/tests/unit_tests/example_test/Makefile.am
b/tests/unit_tests/example_test/Makefile.am
index 04a5ad35..24eb0ba1 100644
--- a/tests/unit_tests/example_test/Makefile.am
+++ b/tests/unit_tests/example_test/Makefile.am
@@ -2,7 +2,9 @@ AUTOMAKE_OPTIONS = foreign
check_PROGRAMS = example_testdriver example2_testdriver
+if !CROSS_COMPILING
TESTS = $(check_PROGRAMS)
+endif
example_testdriver_CFLAGS = @TEST_CFLAGS@
example_testdriver_LDFLAGS = @TEST_LDFLAGS@
diff --git a/tests/unit_tests/openvpn/Makefile.am
b/tests/unit_tests/openvpn/Makefile.am
index 7720a85d..8d2386e0 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -7,14 +7,22 @@ test_binaries += argv_testdriver buffer_testdriver
endif
test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver
ncp_testdriver misc_testdriver \
-pkt_testdriver
+ pkt_testdriver
if HAVE_LD_WRAP_SUPPORT
+if !WIN32
test_binaries += tls_crypt_testdriver
endif
+endif
test_binaries += provider_testdriver
+if WIN32
+LDADD = -lws2_32
+endif
+
+if !CROSS_COMPILING
TESTS = $(test_binaries)
+endif
check_PROGRAMS = $(test_binaries)
if HAVE_SITNL
@@ -31,12 +39,14 @@ argv_testdriver_SOURCES = test_argv.c mock_msg.c mock_msg.h
\
mock_get_random.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/buffer.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/argv.c
buffer_testdriver_CFLAGS = @TEST_CFLAGS@ -I$(openvpn_srcdir)
-I$(compat_srcdir)
buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir)
-Wl,--wrap=parse_line
buffer_testdriver_SOURCES = test_buffer.c mock_msg.c mock_msg.h \
mock_get_random.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
crypto_testdriver_CFLAGS = @TEST_CFLAGS@ \
@@ -51,6 +61,7 @@ crypto_testdriver_SOURCES = test_crypto.c mock_msg.c
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/mtu.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/mss.c
packet_id_testdriver_CFLAGS = @TEST_CFLAGS@ \
@@ -63,14 +74,14 @@ packet_id_testdriver_SOURCES = test_packet_id.c mock_msg.c
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/reliable.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/session_id.c
-
pkt_testdriver_CFLAGS = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir)
pkt_testdriver_LDFLAGS = @TEST_LDFLAGS@
pkt_testdriver_SOURCES = test_pkt.c mock_msg.c moc
Re: [Openvpn-devel] [PATCH 1/5] Conditionally add subdir-objects option to automake
On Tue, Feb 7, 2023 at 7:18 PM Arne Schwabe wrote: > From: Selva Nair > > - Eliminates repeated warnings such as > warning: source file '$(openvpn_srcdir)/env_set.c' is in a subdirectory, > but option 'subdir-objects' is disabled > - Enabled only for automake >= 1.16 as older versions have a buggy > implementation > of this option > > Main side effect of this option is that object files like > openvpnserv-blockdns.o > are now created in src/openvpn where block-dns.c resides instead of in > src/openvpnserv. Same for object files for sources from $(openvpn_srcdir) > compiled > into test executables. > > See also past discussion on this topic: > > https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg00013.html > > Signed-off-by: Selva Nair > Signed-off-by: Arne Schwabe > I'm a bit confused here -- it seems you have made some edits to my patch (signed-off), but not clear what has changed. As for my original 3/3, I will add your fix for engine-test and submit as v3 -- thanks for working on this. I've no idea why libtool adds that wrong line break in its wrapper script on OSX. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 2/3] Build unit tests in mingw Windows build
Hi, On Tue, Feb 7, 2023 at 6:59 AM Arne Schwabe wrote: > Am 04.02.23 um 07:40 schrieb selva.n...@gmail.com: > > From: Selva Nair > > > > - Minor changes to the build system to include some > >dependencies for Windows build > > > > - test_tls_crypt not built as it will pull in win32.c and > >its dependencies > > > > - If cross-compiling, "make check" will only build the tests but not > >run any. Copy to Windows and run manually. Executables are in > >/tests/unit_tests/openvpn/.libs/ and these depend on > >cmocka.dll in addition to openssl libs that some tests link to. > > > >Building with mingw on Windows should run the tests (untested). > > > > v2: networking_testdriver was mistakenly enabled to run, while > > originally it was only set to build. Corrected. > > > > I am currently testing this and also trying to add this to the Github > actions. This patch seems fine but I get other weird errors and cmocka > builds fine crosscompiling on my own machine but breaks for some reason > in Github Actions. I am looking into these problems and will give a full > review after I solved those issues. > That would be nice. I was not sure how to copy the artifacts into a Windows instance and add a job for testing them in GHA. Regarding "weird errors" an obvious thing easy to overlook is that the exe's are in .libs/ -- the wrapper that libtool generates is only useful for running native builds in-place with uninstalled shared libraries. Also, I just noticed that I had locally disabled the example test because of some missing header warning -- it needs an include to build. Regards, Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] configure: enable DCO by default on FreeBSD/Linux
On Tue, Feb 7, 2023 at 8:22 AM Frank Lichtenheld
wrote:
> Automatically disabled when
> - iproute2 is enabled
> (Don't want to force people specifying --disable-dco explicitely)
> - libnv is missing on FreeBSD
> (FreeBSD version too old anyway)
>
> Will still error out if libnl-genl is missing on Linux to
> make people aware of the new dependency.
>
> Signed-off-by: Frank Lichtenheld
> ---
> configure.ac | 82
> 1 file changed, 63 insertions(+), 19 deletions(-)
>
> v2: error out when libnl-genl is missing as discussed with ordex on
> IRC.
>
> v3:
> - improvements to the messages, suggested by Selva
> - further improvements to the default specification, trying to make it
> clear
> - if enabling iproute2, do not test for libnl-genl
>
>
> diff --git a/configure.ac b/configure.ac
> index 78c99740..ba33ccd8 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -146,14 +146,27 @@ AC_ARG_ENABLE(
>
> AC_ARG_ENABLE(
> [dco],
> - [AS_HELP_STRING([--enable-dco], [enable data channel offload
> support using the ovpn-dco kernel module (always enabled on Windows)
> @<:@default=no@:>@])],
> + [AS_HELP_STRING([--disable-dco], [disable data channel offload
> support using the ovpn-dco kernel module @<:@default=yes@:>@ on
> Linux/FreeBSD, can't disable on Windows])],
> ,
> - [enable_dco="no"]
> + [
> + case "$host" in
> + *-*-linux*)
> + enable_dco="auto"
> + ;;
> + *-*-freebsd*)
> + enable_dco="auto"
> + ;;
> + *)
> + # note that this does not disable it for
> Windows
> + enable_dco="no"
> + ;;
> + esac
> + ]
> )
>
> AC_ARG_ENABLE(
> [iproute2],
> - [AS_HELP_STRING([--enable-iproute2], [enable support for iproute2
> @<:@default=no@:>@])],
> + [AS_HELP_STRING([--enable-iproute2], [enable support for iproute2
> (disables DCO) @<:@default=no@:>@])],
> ,
> [enable_iproute2="no"]
> )
> @@ -536,7 +549,7 @@ AC_CHECK_DECLS(
> ,
> [[${SOCKET_INCLUDES}]]
> )
> -AC_CHECKING([anonymous union support])
> +AC_MSG_CHECKING([anonymous union support])
> AC_COMPILE_IFELSE(
> [AC_LANG_PROGRAM(
> [[
> @@ -771,28 +784,59 @@ PKG_CHECK_MODULES(
> )
>
>
> -if test "$enable_dco" = "yes"; then
> -dnl
> -dnl Include generic netlink library used to talk to ovpn-dco
> -dnl
> +if test "$enable_dco" != "no"; then
> + enable_dco_arg="$enable_dco"
> + if test "${enable_iproute2}" = "yes"; then
> + AC_MSG_WARN([DCO cannot be enabled when using iproute2])
> + enable_dco="no"
> + fi
> case "$host" in
> *-*-linux*)
> - PKG_CHECK_MODULES([LIBNL_GENL],
> + if test "$enable_dco" = "no"; then
> + if test "$enable_dco_arg" = "auto"; then
> + AC_MSG_WARN([DCO support
> disabled])
+ else
> + AC_MSG_ERROR([DCO support can't be
> enabled])
> + fi
> + else
> + dnl
> + dnl Include generic netlink library used
> to talk to ovpn-dco
> + dnl
> + PKG_CHECK_MODULES([LIBNL_GENL],
> [libnl-genl-3.0 >= 3.4.0],
> [have_libnl="yes"],
> - [AC_MSG_ERROR([libnl-genl-3.0
> package not found or too old. Is the development package and pkg-config
> installed? Must be version 3.4.0 or newer])]
> - )
> -
> - CFLAGS="${CFLAGS} ${LIBNL_GENL_CFLAGS}"
> - LIBS="${LIBS} ${LIBNL_GENL_LIBS}"
> + [
> + AC_MSG_ERROR([libnl-genl-3.0
> package not found or too old. Is the development package and pkg-config
> installed? Must be version 3.4
Re: [Openvpn-devel] [PATCH v2] configure: enable DCO by default on FreeBSD/Linux
Hi,
On Mon, Feb 6, 2023 at 6:24 AM Frank Lichtenheld
wrote:
> Automatically disabled when
> - iproute2 is enabled
> (Don't want to force people specifying --disable-dco explicitely)
> - libnv is missing on FreeBSD
> (FreeBSD version too old anyway)
>
> Will still error out if libnl-genl is missing on Linux to
> make people aware of new dependency.
>
> Signed-off-by: Frank Lichtenheld
> ---
> configure.ac | 78
> 1 file changed, 61 insertions(+), 17 deletions(-)
>
> v2: error out when libnl-genl is missing as discussed with ordex on
> IRC.
>
>
> diff --git a/configure.ac b/configure.ac
> index 91500087..acfa4bc1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -144,14 +144,27 @@ AC_ARG_ENABLE(
>
> AC_ARG_ENABLE(
> [dco],
> - [AS_HELP_STRING([--enable-dco], [enable data channel offload
> support using the ovpn-dco kernel module (always enabled on Windows)
> @<:@default=no@:>@])],
> + [AS_HELP_STRING([--disable-dco], [enable data channel offload
> support using the ovpn-dco kernel module (always enabled on Windows)
> @<:@default=yes@:>@])],
>
The help string should be
"disable data channel offload support ..." instead of "enable data channel
offload ..." as it's now prefixed with "--disable-dco" .
The "default=yes" in this case may be better written as "default=auto" (or
"platform specific") but it's not critical. Here, default always refers to
the "--enable-FEATURE" form of the option. Confusing but consistent with
how other options are described.
We could be more helpful by describing both enable and disable forms of all
options, as well as custom args that the enable form takes (if any), but
that's beyond the scope of this patch.
-if test "$enable_dco" = "yes"; then
> +if test "$enable_dco" != "no"; then
> + enable_dco_arg="$enable_dco"
> + if test "${enable_iproute2}" = "yes"; then
> + AC_MSG_WARN([iproute2 support cannot be enabled when using
> DCO])
>
This is mildly misleading as here iproute2 is going to win over DCO but the
message appears to indicate otherwise. It would result in outputs like:
WARNING: iproute2 support cannot be enabled when using DCO
WARNING: DCO support disabled
(or in an error)
Phrasing the first as "DCO cannot be enabled when using iproute2" would be
better.
> + enable_dco="no"
> + fi
> + case "$host" in
> + *-*-linux*)
I'll defer to those who understand DCO better to ACK the technical part.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2 2/3] Build unit tests in mingw Windows build
From: Selva Nair
- Minor changes to the build system to include some
dependencies for Windows build
- test_tls_crypt not built as it will pull in win32.c and
its dependencies
- If cross-compiling, "make check" will only build the tests but not
run any. Copy to Windows and run manually. Executables are in
/tests/unit_tests/openvpn/.libs/ and these depend on
cmocka.dll in addition to openssl libs that some tests link to.
Building with mingw on Windows should run the tests (untested).
v2: networking_testdriver was mistakenly enabled to run, while
originally it was only set to build. Corrected.
Signed-off-by: Selva Nair
---
configure.ac | 2 ++
tests/Makefile.am | 2 ++
tests/unit_tests/example_test/Makefile.am | 2 ++
tests/unit_tests/openvpn/Makefile.am | 28 +++
4 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac
index 95d795c3..66ba6f38 100644
--- a/configure.ac
+++ b/configure.ac
@@ -364,6 +364,8 @@ case "$host" in
;;
esac
+AM_CONDITIONAL([CROSS_COMPILING], test "${cross_compiling}" = "yes")
+
PKG_PROG_PKG_CONFIG
AC_PROG_CPP
AC_PROG_INSTALL
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 87dd7e17..a46f2573 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -14,10 +14,12 @@ MAINTAINERCLEANFILES = \
SUBDIRS = unit_tests
+if !WIN32
test_scripts = t_client.sh t_lpback.sh t_cltsrv.sh
if HAVE_SITNL
test_scripts += t_net.sh
endif
+endif
TESTS_ENVIRONMENT = top_srcdir="$(top_srcdir)"
TESTS = $(test_scripts)
diff --git a/tests/unit_tests/example_test/Makefile.am
b/tests/unit_tests/example_test/Makefile.am
index 04a5ad35..24eb0ba1 100644
--- a/tests/unit_tests/example_test/Makefile.am
+++ b/tests/unit_tests/example_test/Makefile.am
@@ -2,7 +2,9 @@ AUTOMAKE_OPTIONS = foreign
check_PROGRAMS = example_testdriver example2_testdriver
+if !CROSS_COMPILING
TESTS = $(check_PROGRAMS)
+endif
example_testdriver_CFLAGS = @TEST_CFLAGS@
example_testdriver_LDFLAGS = @TEST_LDFLAGS@
diff --git a/tests/unit_tests/openvpn/Makefile.am
b/tests/unit_tests/openvpn/Makefile.am
index 7720a85d..8d2386e0 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -7,14 +7,22 @@ test_binaries += argv_testdriver buffer_testdriver
endif
test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver
ncp_testdriver misc_testdriver \
-pkt_testdriver
+ pkt_testdriver
if HAVE_LD_WRAP_SUPPORT
+if !WIN32
test_binaries += tls_crypt_testdriver
endif
+endif
test_binaries += provider_testdriver
+if WIN32
+LDADD = -lws2_32
+endif
+
+if !CROSS_COMPILING
TESTS = $(test_binaries)
+endif
check_PROGRAMS = $(test_binaries)
if HAVE_SITNL
@@ -31,12 +39,14 @@ argv_testdriver_SOURCES = test_argv.c mock_msg.c mock_msg.h
\
mock_get_random.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/buffer.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/argv.c
buffer_testdriver_CFLAGS = @TEST_CFLAGS@ -I$(openvpn_srcdir)
-I$(compat_srcdir)
buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir)
-Wl,--wrap=parse_line
buffer_testdriver_SOURCES = test_buffer.c mock_msg.c mock_msg.h \
mock_get_random.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
crypto_testdriver_CFLAGS = @TEST_CFLAGS@ \
@@ -51,6 +61,7 @@ crypto_testdriver_SOURCES = test_crypto.c mock_msg.c
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/mtu.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/mss.c
packet_id_testdriver_CFLAGS = @TEST_CFLAGS@ \
@@ -63,14 +74,14 @@ packet_id_testdriver_SOURCES = test_packet_id.c mock_msg.c
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/reliable.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/session_id.c
-
pkt_testdriver_CFLAGS = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir)
pkt_testdriver_LDFLAGS = @TEST_LDFLAGS@
pkt_testdriver_SOURCES = test_pkt.c mock_msg.c mock_msg.h \
-$(openvpn_srcdir)/argv.c \
+ $(openvpn_srcdir)/argv.c \
$(openvpn_srcdir)/base64.c \
$(openvpn_srcdir)/buffer.c \
$(openvpn_srcdir)/crypto.c \
@@ -84,9 +95,10 @@ $(openvpn_srcdir)/argv.c \
$(openvpn_srcdir)/run_command.c \
$(openvpn_srcdir)/session_id.c \
$(openvpn_srcdir)/ssl_pkt.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/tls_crypt.c
-
+if !WIN32
tls_crypt_testdriver_CFLAGS = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir)
tls_crypt_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
@@ -106,6 +118,7 @@ tls_crypt_testdriver
[Openvpn-devel] [PATCH v2 3/3] Add a unit test for functions in cryptoapi.c
From: Selva Nair
- Though named cryptoapi_testdriver, right now this only tests
parsing of thumbprint specified as a selector for --cryptioapicert
option. More tests coming..
v2: a line that belongs here was mistakenly included in the previous
commit. Corrected.
Signed-off-by: Selva Nair
---
tests/unit_tests/openvpn/Makefile.am | 16 +++
tests/unit_tests/openvpn/test_cryptoapi.c | 126 ++
2 files changed, 142 insertions(+)
create mode 100644 tests/unit_tests/openvpn/test_cryptoapi.c
diff --git a/tests/unit_tests/openvpn/Makefile.am
b/tests/unit_tests/openvpn/Makefile.am
index 8d2386e0..ee0a3d8a 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -17,6 +17,7 @@ endif
test_binaries += provider_testdriver
if WIN32
+test_binaries += cryptoapi_testdriver
LDADD = -lws2_32
endif
@@ -152,6 +153,21 @@ provider_testdriver_SOURCES = test_provider.c mock_msg.c \
$(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
+if WIN32
+cryptoapi_testdriver_CFLAGS = @TEST_CFLAGS@ \
+ -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+ $(OPTIONAL_CRYPTO_CFLAGS)
+cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+ $(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
+cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
+ $(openvpn_srcdir)/xkey_helper.c \
+ $(openvpn_srcdir)/buffer.c \
+ $(openvpn_srcdir)/base64.c \
+ $(openvpn_srcdir)/platform.c \
+ mock_get_random.c \
+ $(openvpn_srcdir)/win32-util.c
+endif
+
auth_token_testdriver_CFLAGS = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
$(OPTIONAL_CRYPTO_CFLAGS)
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c
b/tests/unit_tests/openvpn/test_cryptoapi.c
new file mode 100644
index ..2bea3f42
--- /dev/null
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -0,0 +1,126 @@
+/*
+ * OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ * Copyright (C) 2023 Selva Nair
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by the
+ * Free Software Foundation, either version 2 of the License,
+ * or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+#include "manage.h"
+#include "integer.h"
+#include "xkey_common.h"
+
+#if defined(HAVE_XKEY_PROVIDER) && defined (ENABLE_CRYPTOAPI)
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include /* pull-in the whole file to test static functions */
+
+struct management *management; /* global */
+
+/* mock a management function that xkey_provider needs */
+char *
+management_query_pk_sig(struct management *man, const char *b64_data,
+const char *algorithm)
+{
+(void) man;
+(void) b64_data;
+(void) algorithm;
+return NULL;
+}
+
+/* tls_libctx is defined in ssl_openssl.c which we do not want to compile in */
+OSSL_LIB_CTX *tls_libctx;
+
+#ifndef _countof
+#define _countof(x) sizeof((x))/sizeof(*(x))
+#endif
+
+/* test data */
+static const uint8_t test_hash[] = {
+0x77, 0x38, 0x65, 0x00, 0x1e, 0x96, 0x48, 0xc6, 0x57, 0x0b, 0xae,
+0xc0, 0xb7, 0x96, 0xf9, 0x66, 0x4d, 0x5f, 0xd0, 0xb7
+};
+
+/* valid test strings to test with and without embedded and trailing spaces */
+static const char *valid_str[] = {
+"773865001e9648c6570baec0b796f9664d5fd0b7",
+" 77 386500 1e 96 48 c6570b aec0b7 96f9664d5f d0 b7",
+" 773865001e9648c6570baec0b796f9664d5fd0b7 ",
+};
+
+/* some invalid strings to test with and without embedded and trailing spaces
*/
+static const char *invalid_str[] = {
+"773 865001e9648c6570baec0b796f9664d5fd012", /* space within byte */
+"77:38:65001e9648c6570baec0b796f9664d5fd0b7", /* invalid separator */
+"7738x5001e9648c6570baec0b796f9664d5fd0b7", /* non hex character */
+};
+
+static void
+test_parse_hexstring(void **state)
+{
+unsigned c
[Openvpn-devel] [PATCH 3/3] Add a unit test for functions in cryptoapi.c
From: Selva Nair
- Though named cryptoapi_testdriver, right now this only tests
parsing of thumbprint specified as a selector for --cryptioapicert
option. More cryptoapi tests coming..
Signed-off-by: Selva Nair
---
As requested during review of the 4/4 patch of cryptoapi series.
Note that this doesn't currently build with MSVC -- either cross
compile on Linux and run the binary on Windows or use mingw to
compile on Windows.
tests/unit_tests/openvpn/Makefile.am | 15 +++
tests/unit_tests/openvpn/test_cryptoapi.c | 126 ++
2 files changed, 141 insertions(+)
create mode 100644 tests/unit_tests/openvpn/test_cryptoapi.c
diff --git a/tests/unit_tests/openvpn/Makefile.am
b/tests/unit_tests/openvpn/Makefile.am
index 909ac4e2..0a1ad439 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -153,6 +153,21 @@ provider_testdriver_SOURCES = test_provider.c mock_msg.c \
$(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
+if WIN32
+cryptoapi_testdriver_CFLAGS = @TEST_CFLAGS@ \
+ -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+ $(OPTIONAL_CRYPTO_CFLAGS)
+cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+ $(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
+cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
+ $(openvpn_srcdir)/xkey_helper.c \
+ $(openvpn_srcdir)/buffer.c \
+ $(openvpn_srcdir)/base64.c \
+ $(openvpn_srcdir)/platform.c \
+ mock_get_random.c \
+ $(openvpn_srcdir)/win32-util.c
+endif
+
auth_token_testdriver_CFLAGS = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
$(OPTIONAL_CRYPTO_CFLAGS)
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c
b/tests/unit_tests/openvpn/test_cryptoapi.c
new file mode 100644
index ..2bea3f42
--- /dev/null
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -0,0 +1,126 @@
+/*
+ * OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ * Copyright (C) 2023 Selva Nair
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by the
+ * Free Software Foundation, either version 2 of the License,
+ * or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+#include "manage.h"
+#include "integer.h"
+#include "xkey_common.h"
+
+#if defined(HAVE_XKEY_PROVIDER) && defined (ENABLE_CRYPTOAPI)
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include /* pull-in the whole file to test static functions */
+
+struct management *management; /* global */
+
+/* mock a management function that xkey_provider needs */
+char *
+management_query_pk_sig(struct management *man, const char *b64_data,
+const char *algorithm)
+{
+(void) man;
+(void) b64_data;
+(void) algorithm;
+return NULL;
+}
+
+/* tls_libctx is defined in ssl_openssl.c which we do not want to compile in */
+OSSL_LIB_CTX *tls_libctx;
+
+#ifndef _countof
+#define _countof(x) sizeof((x))/sizeof(*(x))
+#endif
+
+/* test data */
+static const uint8_t test_hash[] = {
+0x77, 0x38, 0x65, 0x00, 0x1e, 0x96, 0x48, 0xc6, 0x57, 0x0b, 0xae,
+0xc0, 0xb7, 0x96, 0xf9, 0x66, 0x4d, 0x5f, 0xd0, 0xb7
+};
+
+/* valid test strings to test with and without embedded and trailing spaces */
+static const char *valid_str[] = {
+"773865001e9648c6570baec0b796f9664d5fd0b7",
+" 77 386500 1e 96 48 c6570b aec0b7 96f9664d5f d0 b7",
+" 773865001e9648c6570baec0b796f9664d5fd0b7 ",
+};
+
+/* some invalid strings to test with and without embedded and trailing spaces
*/
+static const char *invalid_str[] = {
+"773 865001e9648c6570baec0b796f9664d5fd012", /* space within byte */
+"77:38:65001e9648c6570baec0b796f9664d5fd0b7", /* invalid separator */
+"7738x5001e9648c6570baec0b796f9664d5fd0b7", /* non hex character */
+};
+
+static void
+test_parse_hexstring(void **state)
+{
+unsigned c
[Openvpn-devel] [PATCH 2/3] Build unit tests in mingw Windows build
From: Selva Nair
- Minor changes to the build system to include some
dependencies for Windows build
- test_tls_crypt not built as it will pull in win32.c and
its dependencies
- If cross-compiling, "make check" will only build the tests but not
run any. Copy to Windows and run manually. Executables are in
/tests/unit_tests/openvpn/.libs/ and these depend on
cmocka.dll in addition to openssl libs that some tests link to.
Building with mingw on Windows should run the tests (untested).
Signed-off-by: Selva Nair
---
configure.ac | 2 ++
tests/Makefile.am | 2 ++
tests/unit_tests/example_test/Makefile.am | 2 ++
tests/unit_tests/openvpn/Makefile.am | 35 +--
4 files changed, 33 insertions(+), 8 deletions(-)
diff --git a/configure.ac b/configure.ac
index 95d795c3..66ba6f38 100644
--- a/configure.ac
+++ b/configure.ac
@@ -364,6 +364,8 @@ case "$host" in
;;
esac
+AM_CONDITIONAL([CROSS_COMPILING], test "${cross_compiling}" = "yes")
+
PKG_PROG_PKG_CONFIG
AC_PROG_CPP
AC_PROG_INSTALL
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 87dd7e17..a46f2573 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -14,10 +14,12 @@ MAINTAINERCLEANFILES = \
SUBDIRS = unit_tests
+if !WIN32
test_scripts = t_client.sh t_lpback.sh t_cltsrv.sh
if HAVE_SITNL
test_scripts += t_net.sh
endif
+endif
TESTS_ENVIRONMENT = top_srcdir="$(top_srcdir)"
TESTS = $(test_scripts)
diff --git a/tests/unit_tests/example_test/Makefile.am
b/tests/unit_tests/example_test/Makefile.am
index 04a5ad35..24eb0ba1 100644
--- a/tests/unit_tests/example_test/Makefile.am
+++ b/tests/unit_tests/example_test/Makefile.am
@@ -2,7 +2,9 @@ AUTOMAKE_OPTIONS = foreign
check_PROGRAMS = example_testdriver example2_testdriver
+if !CROSS_COMPILING
TESTS = $(check_PROGRAMS)
+endif
example_testdriver_CFLAGS = @TEST_CFLAGS@
example_testdriver_LDFLAGS = @TEST_LDFLAGS@
diff --git a/tests/unit_tests/openvpn/Makefile.am
b/tests/unit_tests/openvpn/Makefile.am
index 7720a85d..909ac4e2 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -7,20 +7,29 @@ test_binaries += argv_testdriver buffer_testdriver
endif
test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver
ncp_testdriver misc_testdriver \
-pkt_testdriver
+pkt_testdriver
if HAVE_LD_WRAP_SUPPORT
+if !WIN32
test_binaries += tls_crypt_testdriver
endif
+endif
test_binaries += provider_testdriver
-TESTS = $(test_binaries)
-check_PROGRAMS = $(test_binaries)
+if WIN32
+test_binaries += cryptoapi_testdriver
+LDADD = -lws2_32
+endif
if HAVE_SITNL
-check_PROGRAMS += networking_testdriver
+test_binaries += networking_testdriver
endif
+if !CROSS_COMPILING
+TESTS = $(test_binaries)
+endif
+check_PROGRAMS = $(test_binaries)
+
openvpn_includedir = $(top_srcdir)/include
openvpn_srcdir = $(top_srcdir)/src/openvpn
compat_srcdir = $(top_srcdir)/src/compat
@@ -31,12 +40,14 @@ argv_testdriver_SOURCES = test_argv.c mock_msg.c mock_msg.h
\
mock_get_random.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/buffer.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/argv.c
buffer_testdriver_CFLAGS = @TEST_CFLAGS@ -I$(openvpn_srcdir)
-I$(compat_srcdir)
buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir)
-Wl,--wrap=parse_line
buffer_testdriver_SOURCES = test_buffer.c mock_msg.c mock_msg.h \
mock_get_random.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
crypto_testdriver_CFLAGS = @TEST_CFLAGS@ \
@@ -51,6 +62,7 @@ crypto_testdriver_SOURCES = test_crypto.c mock_msg.c
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/mtu.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/mss.c
packet_id_testdriver_CFLAGS = @TEST_CFLAGS@ \
@@ -63,14 +75,14 @@ packet_id_testdriver_SOURCES = test_packet_id.c mock_msg.c
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/reliable.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/session_id.c
-
pkt_testdriver_CFLAGS = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir)
pkt_testdriver_LDFLAGS = @TEST_LDFLAGS@
pkt_testdriver_SOURCES = test_pkt.c mock_msg.c mock_msg.h \
-$(openvpn_srcdir)/argv.c \
+ $(openvpn_srcdir)/argv.c \
$(openvpn_srcdir)/base64.c \
$(openvpn_srcdir)/buffer.c \
$(openvpn_srcdir)/crypto.c \
@@ -84,9 +96,10 @@ $(openvpn_srcdir)/argv.c \
$(openvpn_srcdir)/run_command.c \
$(openvpn_srcdir)/session_id.c \
$(openvpn_srcdir)/ssl_pkt.c \
+ $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/tls_crypt.c
-
+if !WIN32
tls_cry
[Openvpn-devel] [PATCH 1/3] Conditionally add subdir-objects option to automake
From: Selva Nair
- Eliminates repeated warnings such as
warning: source file '$(openvpn_srcdir)/env_set.c' is in a subdirectory,
but option 'subdir-objects' is disabled
- Enabled only for automake >= 1.16 as older versions have a buggy
implementation
of this option
Main side effect of this option is that object files like openvpnserv-blockdns.o
are now created in src/openvpn where block-dns.c resides instead of in
src/openvpnserv. Same for object files for sources from $(openvpn_srcdir)
compiled
into test executables.
See also past discussion on this topic:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg00013.html
Signed-off-by: Selva Nair
---
configure.ac | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index 91500087..95d795c3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -54,9 +54,22 @@ m4_define([serial_tests], [
awk '{split ($NF,a,"."); if (a[1] == 1 && a[2] >= 12) { print
"serial-tests" }}'
])
])
+
+dnl Automake 1.14+ warns if sources are in sub-directories but subdir-objects
+dnl options is not enabled. However, automake before 1.15a has a bug that
causes
+dnl variable expansion to fail in foo_SOURCES when this option is used.
+dnl As most of our build systems are now likely to use automake 1.16+ add a
+dnl work around to conditionally add subdir-objects option.
+m4_define([subdir_objects], [
+m4_esyscmd([automake --version |
+head -1 |
+awk '{split ($NF,a,"."); if (a[1] == 1 && a[2] >= 16) { print
"subdir-objects" }}'
+])
+])
+
# This foreign option prevents autoreconf from overriding our COPYING and
# INSTALL targets:
-AM_INIT_AUTOMAKE(foreign serial_tests 1.9) dnl NB: Do not [quote] this
parameter.
+AM_INIT_AUTOMAKE(foreign serial_tests subdir_objects 1.9) dnl NB: Do not
[quote] this parameter.
AC_CANONICAL_HOST
AC_USE_SYSTEM_EXTENSIONS
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2 4/4] cryptoapi.c: simplify parsing of thumbprint hex string
From: Selva Nair
v2: Moved the "parse_hexstring" chunk to a function for clarity
and to permit unit-testing.
A test is submitted as a follow up patch.
Signed-off-by: Selva Nair
---
src/openvpn/cryptoapi.c | 77 -
1 file changed, 37 insertions(+), 40 deletions(-)
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index eafef1b1..136c6ffc 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -180,6 +180,39 @@ err:
return NULL;
}
+/**
+ * Parse a hex string with optional embedded spaces into
+ * a byte array.
+ * @param p pointer to the input string
+ * @param arr on output contains the parsed bytes
+ * @param capacity capacity of the byte array arr
+ * @returns the number of bytes parsed or 0 on error
+ */
+int
+parse_hexstring(const char *p, unsigned char *arr, size_t capacity)
+{
+int i = 0;
+for ( ; *p && i < capacity; p += 2)
+{
+/* skip spaces */
+while (*p == ' ')
+{
+p++;
+}
+if (!*p) /* ending with spaces is not an error */
+{
+break;
+}
+
+if (!isxdigit(p[0]) || !isxdigit(p[1])
+|| sscanf(p, "%2hhx", [i++]) != 1)
+{
+return 0;
+}
+}
+return i;
+}
+
static const CERT_CONTEXT *
find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
{
@@ -210,51 +243,15 @@ find_certificate_in_store(const char *cert_prop,
HCERTSTORE cert_store)
}
else if (!strncmp(cert_prop, "THUMB:", 6))
{
-const char *p;
-int i, x = 0;
find_type = CERT_FIND_HASH;
find_param =
-/* skip the tag */
-cert_prop += 6;
-for (p = cert_prop, i = 0; *p && i < sizeof(hash); i++)
+blob.cbData = parse_hexstring(cert_prop + 6, hash, sizeof(hash));
+if (blob.cbData == 0)
{
-if (*p >= '0' && *p <= '9')
-{
-x = (*p - '0') << 4;
-}
-else if (*p >= 'A' && *p <= 'F')
-{
-x = (*p - 'A' + 10) << 4;
-}
-else if (*p >= 'a' && *p <= 'f')
-{
-x = (*p - 'a' + 10) << 4;
-}
-if (!*++p) /* unexpected end of string */
-{
-msg(M_WARN|M_INFO, "WARNING: cryptoapicert: error parsing
.", cert_prop);
-goto out;
-}
-if (*p >= '0' && *p <= '9')
-{
-x += *p - '0';
-}
-else if (*p >= 'A' && *p <= 'F')
-{
-x += *p - 'A' + 10;
-}
-else if (*p >= 'a' && *p <= 'f')
-{
-x += *p - 'a' + 10;
-}
-hash[i] = x;
-/* skip any space(s) between hex numbers */
-for (p++; *p && *p == ' '; p++)
-{
-}
+msg(M_WARN|M_INFO, "WARNING: cryptoapicert: error parsing <%s>.",
cert_prop);
+goto out;
}
-blob.cbData = i;
}
else
{
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 4/4] cryptoapi.c: simplify parsing of thumbprint hex string
On Fri, Feb 3, 2023 at 5:25 AM Arne Schwabe wrote:
>
> > Well, replying to myself:
> >
> > I tried building the unit tests using cmocka for Windows
> > (cross-compiling using mingw-w64 with locally built cmocka).
> > Unfortunately, none of the tests could be built out of the box because
> > of missing dependencies, so I guess no one is running these tests on
> > Windows.
> >
> > Good news is that I could get provider_testdriver to build and run on
> > Windows with minor tweaks and a test for this patch could be easily
> > written too.
> >
> > I can follow up with a test patch, but wonder whether it's worth
> > spending time on if no one else is building these tests for Windows.
>
> I have a cmake build file for OpenVPN that actually builds part of the
> unit tests ("test_packet_id" "test_crypto" "test_ncp" "test_auth_token"
> "test_misc" "test_buffer" "test_provider" "test_pkt") and run them in
> the Github actions pipeline. But the cmake stuff is more "work in
> progress" and the general consensus was to stick with the autoconf
> approach.
>
> I looked into building the test binaries as part of the normal MSVC
> build but gave up on trying to figure out how to deal with MSVC and
> their solutions, especially since you can nowadays just point it at a
> CmakeLists.txt and it will pick that up just fine.
>
> If you want to take a look:
>
> Cmake build file:
> https://github.com/schwabe/openvpn/blob/bloom/CMakeLists.txt
>
> Running tests as part of GHA:
>
> https://github.com/schwabe/openvpn/blob/bloom/.github/workflows/build.yaml#L561
Thanks a lot for that.
By adding win32-util.c and -lws2_32 where required, I can now build almost
all tests using the autotools framework --- cmocka had to be cross-compiled
using cmake which is a pain.
I do not particularly like cmake though it's convenient for Windows MSVC
build, so sticking to autoconf/automake for now. Unless the "consensus" is
to move to cmake --- hope not :)
Anyway, this is motivating to write some tests.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 4/4] cryptoapi.c: simplify parsing of thumbprint hex string
Hi > > On Wed, Feb 1, 2023 at 6:56 AM Frank Lichtenheld > wrote: > >> On Sat, Jan 28, 2023 at 05:34:21PM -0500, selva.n...@gmail.com wrote: >> > From: Selva Nair >> > >> > Signed-off-by: Selva Nair >> > --- >> > src/openvpn/cryptoapi.c | 44 +++-- >> > 1 file changed, 12 insertions(+), 32 deletions(-) >> > >> > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c >> > index 6ff4fcb5..9fd5aea9 100644 >> > --- a/src/openvpn/cryptoapi.c >> > +++ b/src/openvpn/cryptoapi.c >> > @@ -210,49 +210,29 @@ find_certificate_in_store(const char *cert_prop, >> HCERTSTORE cert_store) >> >> This seems to ask for a unittest. > > > Would be nice indeed. > > But, I've never tried to enable cmocka in our Windows build -- does it > work? If it does, I would be inclined to add tests for this as well as some > other functions in cryptoapi.c > Well, replying to myself: I tried building the unit tests using cmocka for Windows (cross-compiling using mingw-w64 with locally built cmocka). Unfortunately, none of the tests could be built out of the box because of missing dependencies, so I guess no one is running these tests on Windows. Good news is that I could get provider_testdriver to build and run on Windows with minor tweaks and a test for this patch could be easily written too. I can follow up with a test patch, but wonder whether it's worth spending time on if no one else is building these tests for Windows. Regards, Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 4/4] cryptoapi.c: simplify parsing of thumbprint hex string
Hi, On Wed, Feb 1, 2023 at 6:56 AM Frank Lichtenheld wrote: > On Sat, Jan 28, 2023 at 05:34:21PM -0500, selva.n...@gmail.com wrote: > > From: Selva Nair > > > > Signed-off-by: Selva Nair > > --- > > src/openvpn/cryptoapi.c | 44 +++-- > > 1 file changed, 12 insertions(+), 32 deletions(-) > > > > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c > > index 6ff4fcb5..9fd5aea9 100644 > > --- a/src/openvpn/cryptoapi.c > > +++ b/src/openvpn/cryptoapi.c > > @@ -210,49 +210,29 @@ find_certificate_in_store(const char *cert_prop, > HCERTSTORE cert_store) > > This seems to ask for a unittest. Would be nice indeed. But, I've never tried to enable cmocka in our Windows build -- does it work? If it does, I would be inclined to add tests for this as well as some other functions in cryptoapi.c Regards, Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2 3/4] cryptoapi.c: remove pre OpenSSL-3.01 support
From: Selva Nair
- Require xkey-provider (thus OpenSSL 3.01+) for --cryptoapicert
Note:
Ideally we should also make ENABLE_CRYPTOAPI conditional
on HAVE_XKEY_PROVIDER but that looks hard unless we can agree
to move HAVE_XKEY_PROVIDER to configure/config.h.
v2: use "binary" instead of "version" in the error message
Signed-off-by: Selva Nair
---
src/openvpn/cryptoapi.c | 555 +---
src/openvpn/options.c | 2 +-
2 files changed, 11 insertions(+), 546 deletions(-)
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index e3c0bc99..eafef1b1 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -55,17 +55,17 @@
#include "xkey_common.h"
#ifndef HAVE_XKEY_PROVIDER
-/* index for storing external data in EC_KEY: < 0 means uninitialized */
-static int ec_data_idx = -1;
-/* Global EVP_PKEY_METHOD used to override the sign operation */
-static EVP_PKEY_METHOD *pmethod;
-static int (*default_pkey_sign_init) (EVP_PKEY_CTX *ctx);
-static int (*default_pkey_sign) (EVP_PKEY_CTX *ctx, unsigned char *sig,
- size_t *siglen, const unsigned char *tbs,
size_t tbslen);
-#else /* ifndef HAVE_XKEY_PROVIDER */
+int
+SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
+{
+msg(M_NONFATAL, "ERROR: this binary was built without cryptoapicert
support");
+return 0;
+}
+
+#else /* HAVE_XKEY_PROVIDER */
+
static XKEY_EXTERNAL_SIGN_fn xkey_cng_sign;
-#endif /* HAVE_XKEY_PROVIDER */
typedef struct _CAPI_DATA {
const CERT_CONTEXT *cert_context;
@@ -146,127 +146,6 @@ CAPI_DATA_free(CAPI_DATA *cd)
free(cd);
}
-#ifndef HAVE_XKEY_PROVIDER
-
-/* Translate OpenSSL padding type to CNG padding type
- * Returns 0 for unknown/unsupported padding.
- */
-static DWORD
-cng_padding_type(int padding)
-{
-DWORD pad = 0;
-
-switch (padding)
-{
-case RSA_NO_PADDING:
-break;
-
-case RSA_PKCS1_PADDING:
-pad = BCRYPT_PAD_PKCS1;
-break;
-
-case RSA_PKCS1_PSS_PADDING:
-pad = BCRYPT_PAD_PSS;
-break;
-
-default:
-msg(M_WARN|M_INFO, "cryptoapicert: unknown OpenSSL padding type
%d.",
-padding);
-}
-
-return pad;
-}
-
-/**
- * Sign the hash in 'from' using NCryptSignHash(). This requires an NCRYPT
- * key handle in cd->crypt_prov. On return the signature is in 'to'. Returns
- * the length of the signature or 0 on error.
- * This is used only for RSA and padding should be BCRYPT_PAD_PKCS1 or
- * BCRYPT_PAD_PSS.
- * If the hash_algo is not NULL, PKCS #1 DigestInfo header gets added
- * to |from|, else it is signed as is. Use NULL for MD5 + SHA1 hash used
- * in TLS 1.1 and earlier.
- * In case of PSS padding, |saltlen| should specify the size of salt to use.
- * If |to| is NULL returns the required buffer size.
- */
-static int
-priv_enc_CNG(const CAPI_DATA *cd, const wchar_t *hash_algo, const unsigned
char *from,
- int flen, unsigned char *to, int tlen, DWORD padding, DWORD
saltlen)
-{
-NCRYPT_KEY_HANDLE hkey = cd->crypt_prov;
-DWORD len = 0;
-ASSERT(cd->key_spec == CERT_NCRYPT_KEY_SPEC);
-
-DWORD status;
-
-msg(D_LOW, "Signing hash using CNG: data size = %d padding = %lu", flen,
padding);
-
-if (padding == BCRYPT_PAD_PKCS1)
-{
-BCRYPT_PKCS1_PADDING_INFO padinfo = {hash_algo};
-status = NCryptSignHash(hkey, , (BYTE *)from, flen,
-to, tlen, , padding);
-}
-else if (padding == BCRYPT_PAD_PSS)
-{
-BCRYPT_PSS_PADDING_INFO padinfo = {hash_algo, saltlen};
-status = NCryptSignHash(hkey, , (BYTE *)from, flen,
-to, tlen, , padding);
-}
-else
-{
-msg(M_NONFATAL, "Error in cryptoapicert: Unknown padding type");
-return 0;
-}
-
-if (status != ERROR_SUCCESS)
-{
-SetLastError(status);
-msg(M_NONFATAL|M_ERRNO, "Error in cryptoapicert: NCryptSignHash
failed");
-len = 0;
-}
-
-/* Unlike CAPI, CNG signature is in big endian order. No reversing needed.
*/
-return len;
-}
-
-/* called at RSA_free */
-static int
-rsa_finish(RSA *rsa)
-{
-const RSA_METHOD *rsa_meth = RSA_get_method(rsa);
-CAPI_DATA *cd = (CAPI_DATA *) RSA_meth_get0_app_data(rsa_meth);
-
-if (cd == NULL)
-{
-return 0;
-}
-CAPI_DATA_free(cd);
-RSA_meth_free((RSA_METHOD *) rsa_meth);
-return 1;
-}
-
-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(
[Openvpn-devel] [PATCH v2] block-dns using iservice: fix a potential double free
From: Selva Nair
- An item added to undo-list was not removed on error, causing
attempt to free again in Undo().
Also fix a memory leak possibility in the same context.
Github: fixes OpenVPN/openvpn#232
v2: Split add and delete functions and reuse the delete
function for cleanup.
Signed-off-by: Selva Nair
---
Same as PR #235 except for DeleteBlockDNS moved up and
the its forward declaration removed.
src/openvpnserv/interactive.c | 132 --
1 file changed, 78 insertions(+), 54 deletions(-)
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 03361d66..a3d43752 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -777,87 +777,111 @@ CmpAny(LPVOID item, LPVOID any)
}
static DWORD
-HandleBlockDNSMessage(const block_dns_message_t *msg, undo_lists_t *lists)
+DeleteBlockDNS(const block_dns_message_t *msg, undo_lists_t *lists)
{
DWORD err = 0;
-block_dns_data_t *interface_data;
+block_dns_data_t *interface_data = RemoveListItem(&(*lists)[block_dns],
CmpAny, NULL);
+
+if (interface_data)
+{
+err = delete_block_dns_filters(interface_data->engine);
+if (interface_data->metric_v4 >= 0)
+{
+set_interface_metric(msg->iface.index, AF_INET,
+ interface_data->metric_v4);
+}
+if (interface_data->metric_v6 >= 0)
+{
+set_interface_metric(msg->iface.index, AF_INET6,
+ interface_data->metric_v6);
+}
+free(interface_data);
+}
+else
+{
+MsgToEventLog(M_ERR, TEXT("No previous block DNS filters to delete"));
+}
+
+return err;
+}
+
+static DWORD
+AddBlockDNS(const block_dns_message_t *msg, undo_lists_t *lists)
+{
+DWORD err = 0;
+block_dns_data_t *interface_data = NULL;
HANDLE engine = NULL;
LPCWSTR exe_path;
exe_path = settings.exe_path;
-if (msg->header.type == msg_add_block_dns)
+err = add_block_dns_filters(, msg->iface.index, exe_path,
BlockDNSErrHandler);
+if (!err)
{
-err = add_block_dns_filters(, msg->iface.index, exe_path,
BlockDNSErrHandler);
+interface_data = malloc(sizeof(block_dns_data_t));
+if (!interface_data)
+{
+err = ERROR_OUTOFMEMORY;
+goto out;
+}
+interface_data->engine = engine;
+interface_data->index = msg->iface.index;
+int is_auto = 0;
+interface_data->metric_v4 = get_interface_metric(msg->iface.index,
+ AF_INET, _auto);
+if (is_auto)
+{
+interface_data->metric_v4 = 0;
+}
+interface_data->metric_v6 = get_interface_metric(msg->iface.index,
+ AF_INET6, _auto);
+if (is_auto)
+{
+interface_data->metric_v6 = 0;
+}
+
+err = AddListItem(&(*lists)[block_dns], interface_data);
if (!err)
{
-interface_data = malloc(sizeof(block_dns_data_t));
-if (!interface_data)
-{
-return ERROR_OUTOFMEMORY;
-}
-interface_data->engine = engine;
-interface_data->index = msg->iface.index;
-int is_auto = 0;
-interface_data->metric_v4 = get_interface_metric(msg->iface.index,
- AF_INET,
_auto);
-if (is_auto)
-{
-interface_data->metric_v4 = 0;
-}
-interface_data->metric_v6 = get_interface_metric(msg->iface.index,
- AF_INET6,
_auto);
-if (is_auto)
-{
-interface_data->metric_v6 = 0;
-}
-err = AddListItem(&(*lists)[block_dns], interface_data);
+err = set_interface_metric(msg->iface.index, AF_INET,
+ BLOCK_DNS_IFACE_METRIC);
if (!err)
{
-err = set_interface_metric(msg->iface.index, AF_INET,
+err = set_interface_metric(msg->iface.index, AF_INET6,
BLOCK_DNS_IFACE_METRIC);
-if (!err)
-{
-set_interface_metric(msg->iface.index, AF_INET6,
- BLOCK_DNS_IFACE_METRIC);
-}
-}
-}
-}
-else
-{
-interface_data = RemoveListItem(&(*lists)[block_dns], CmpAny, NULL);
-if (interface_data)
-{
-engine = interface_data->engine;
-err = delete_block_dns_filters(engine);
-engine = NU
Re: [Openvpn-devel] [PATCH] block-dns using iservice: fix a potential double free
> > > Also I replaced 0x%x with %u in win_block_dns_service() for > consistency. You may want to do it in your patch too :) > We have at least another place where it's %x, so will leave that for another day. btw, shouldn't it be %d? Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] block-dns using iservice: fix a potential double free
Hi, On Wed, Feb 1, 2023 at 4:37 AM Lev Stipakov wrote: > Hi, > > I made a slightly different fix but then noticed your mail. > > Indeed the problem is that get/set_interface_metric fails, > and we call FwpmEngineClose0 after updating the undo list. When > openvpn process exits, we execute commands in undo list, > and second call to FwpmEngineClose0 causes Access Violation > inside WFP. > > Note that since get_interface_metric() fails, it sets > interface_data->metric_v4 > to -1. In this case we should not restore metrics back (which will > fail in any case). > We have a very similar code (including metrics value check) just below > which > handles msg_del_block_dns. Can we factor it out into a separate function? > Good point. I have a version that splits "add" and "delete" actions into separate functions and does something like this. Please take a look here: https://github.com/selvanair/openvpn/tree/block-dns-fix The add and delete functions are in that order (with a forward declaration) only to make review easier (less changes with ignore-all-spaces), will move the latter up if it looks okay. That said, if you have a better fix, I'm ready to review it in lieu of this. Thanks, Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] block-dns using iservice: fix a potential double free
From: Selva Nair
- An item added to undo-list was not removed on error, causing
attempt to free again in Undo().
Also fix a memory leak possibility in the same context.
Github: fixes OpenVPN/openvpn#232
Signed-off-by: Selva Nair
---
src/openvpnserv/interactive.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 03361d66..636f32e9 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -780,7 +780,7 @@ static DWORD
HandleBlockDNSMessage(const block_dns_message_t *msg, undo_lists_t *lists)
{
DWORD err = 0;
-block_dns_data_t *interface_data;
+block_dns_data_t *interface_data = NULL;
HANDLE engine = NULL;
LPCWSTR exe_path;
@@ -794,6 +794,7 @@ HandleBlockDNSMessage(const block_dns_message_t *msg,
undo_lists_t *lists)
interface_data = malloc(sizeof(block_dns_data_t));
if (!interface_data)
{
+delete_block_dns_filters(engine);
return ERROR_OUTOFMEMORY;
}
interface_data->engine = engine;
@@ -818,8 +819,17 @@ HandleBlockDNSMessage(const block_dns_message_t *msg,
undo_lists_t *lists)
BLOCK_DNS_IFACE_METRIC);
if (!err)
{
-set_interface_metric(msg->iface.index, AF_INET6,
- BLOCK_DNS_IFACE_METRIC);
+err = set_interface_metric(msg->iface.index, AF_INET6,
+ BLOCK_DNS_IFACE_METRIC);
+}
+if (err)
+{
+/* try to restore the interface metric values in case
changed */
+set_interface_metric(msg->iface.index, AF_INET,
+ interface_data->metric_v4);
+set_interface_metric(msg->iface.index, AF_INET,
+ interface_data->metric_v4);
+RemoveListItem(&(*lists)[block_dns], CmpAny, NULL);
}
}
}
@@ -853,6 +863,7 @@ HandleBlockDNSMessage(const block_dns_message_t *msg,
undo_lists_t *lists)
if (err && engine)
{
delete_block_dns_filters(engine);
+free(interface_data);
}
return err;
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 2/4] cyryptapi.c: log the selected certificate's name
From: Selva Nair
- With various ways of specifying the selector-string to the
"--cryptoapicert" option, its not immediately obvious
which certificate gets selected from the store. Log it.
The "name" logged is a friendly name (if present), or a
representative element of the subject (usually the common-name).
Signed-off-by: Selva Nair
---
src/openvpn/cryptoapi.c | 29 +
src/openvpn/win32-util.c | 15 +++
src/openvpn/win32-util.h | 3 +++
3 files changed, 47 insertions(+)
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 39eeec1b..e3c0bc99 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -939,12 +939,31 @@ xkey_cng_sign(void *handle, unsigned char *sig, size_t
*siglen, const unsigned c
#endif /* HAVE_XKEY_PROVIDER */
+static char *
+get_cert_name(const CERT_CONTEXT *cc, struct gc_arena *gc)
+{
+DWORD len = CertGetNameStringW(cc, CERT_NAME_FRIENDLY_DISPLAY_TYPE, 0,
NULL, NULL, 0);
+char *name = NULL;
+if (len)
+{
+wchar_t *wname = gc_malloc(len*sizeof(wchar_t), false, gc);
+if (!wname
+|| CertGetNameStringW(cc, CERT_NAME_FRIENDLY_DISPLAY_TYPE, 0,
NULL, wname, len) == 0)
+{
+return NULL;
+}
+name = utf16to8(wname, gc);
+}
+return name;
+}
+
int
SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
{
HCERTSTORE cs;
X509 *cert = NULL;
CAPI_DATA *cd = calloc(1, sizeof(*cd));
+struct gc_arena gc = gc_new();
if (cd == NULL)
{
@@ -979,6 +998,13 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const
char *cert_prop)
}
}
+/* try to log the "name" of the selected certificate */
+char *cert_name = get_cert_name(cd->cert_context, );
+if (cert_name)
+{
+msg(D_LOW, "cryptapicert: using certificate with name <%s>",
cert_name);
+}
+
/* cert_context->pbCertEncoded is the cert X509 DER encoded. */
cert = d2i_X509(NULL, (const unsigned char **)
>cert_context->pbCertEncoded,
cd->cert_context->cbCertEncoded);
@@ -1022,6 +1048,7 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const
char *cert_prop)
EVP_PKEY *privkey = xkey_load_generic_key(tls_libctx, cd, pkey,
xkey_cng_sign,
(XKEY_PRIVKEY_FREE_fn *) CAPI_DATA_free);
SSL_CTX_use_PrivateKey(ssl_ctx, privkey);
+gc_free();
return 1; /* do not free cd -- its kept by xkey provider */
#else /* ifdef HAVE_XKEY_PROVIDER */
@@ -1047,12 +1074,14 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx,
const char *cert_prop)
goto err;
}
CAPI_DATA_free(cd); /* this will do a ref_count-- */
+gc_free(gc);
return 1;
#endif /* HAVE_XKEY_PROVIDER */
err:
CAPI_DATA_free(cd);
+gc_free();
return 0;
}
#endif /* _WIN32 */
diff --git a/src/openvpn/win32-util.c b/src/openvpn/win32-util.c
index 35f2a311..32f7a00b 100644
--- a/src/openvpn/win32-util.c
+++ b/src/openvpn/win32-util.c
@@ -48,6 +48,21 @@ wide_string(const char *utf8, struct gc_arena *gc)
return ucs16;
}
+char *
+utf16to8(const wchar_t *utf16, struct gc_arena *gc)
+{
+char *utf8 = NULL;
+int n = WideCharToMultiByte(CP_UTF8, 0, utf16, -1, NULL, 0, NULL, NULL);
+if (n > 0)
+{
+utf8 = gc_malloc(n, true, gc);
+if (utf8)
+{
+WideCharToMultiByte(CP_UTF8, 0, utf16, -1, utf8, n, NULL, NULL);
+}
+}
+return utf8;
+}
/*
* Return true if filename is safe to be used on Windows,
diff --git a/src/openvpn/win32-util.h b/src/openvpn/win32-util.h
index b24242c8..ac37979f 100644
--- a/src/openvpn/win32-util.h
+++ b/src/openvpn/win32-util.h
@@ -34,6 +34,9 @@
/* Convert a string from UTF-8 to UCS-2 */
WCHAR *wide_string(const char *utf8, struct gc_arena *gc);
+/* Convert a string from UTF-16 to UTF-8 */
+char *utf16to8(const wchar_t *utf16, struct gc_arena *gc);
+
/* return true if filename is safe to be used on Windows */
bool win_safe_filename(const char *fn);
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 1/4] Option --cryptoapicert: support issuer name as a selector
From: Selva Nair
- Certificate selection string can now specify a partial
issuer name string as "--cryptoapicert ISSUER:" where
is matched as a substring of the issuer (CA) name in
the certificate.
Partial case-insensitive matching against the "issuer name" is
used. Here "issuer name" is a text representation of the RDN's
separated by commas.
E.g., "CA, Ontario, Toronto, Acme Inc., IT, Acme Root CA".
See MSDN docs on CertFindCertificateInStore() with CERT_FIND_ISSUER_STR
as "FindType" for more details.
As the order of RDN's is not well-defined[*] and type names like "OU"
or "CN" are not included, its best to match against a single attribute
like the CN of the issuer:
E.g., --cryptoapicert "ISSUER:Acme Root"
[*] Windows appears to order RDN's in the reverse order to which
its written in the certificate but do not rely on this.
Signed-off-by: Selva Nair
---
doc/man-sections/windows-options.rst | 13 +++--
src/openvpn/cryptoapi.c | 5 +
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/doc/man-sections/windows-options.rst
b/doc/man-sections/windows-options.rst
index 368f7b19..e87291f4 100644
--- a/doc/man-sections/windows-options.rst
+++ b/doc/man-sections/windows-options.rst
@@ -41,13 +41,22 @@ Windows-Specific Options
cryptoapicert "SUBJ:Peter Runestig"
- To select a certificate, based on certificate's thumbprint:
+ To select a certificate, based on certificate's thumbprint (SHA1 hash):
::
cryptoapicert "THUMB:f6 49 24 41 01 b4 ..."
The thumbprint hex string can easily be copy-and-pasted from the Windows
- Certificate Store GUI.
+ Certificate Store GUI. The embedded spaces in the hex string are optional.
+
+ To select a certificate based on a substring in certificate's
+ issuer name:
+ ::
+
+ cryptoapicert "ISSUER:Sample CA"
+
+ The first non-expired certificate found in the user's store or the
+ machine store that matches the select-string is used.
--dhcp-release
Ask Windows to release the TAP adapter lease on shutdown. This option
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 661a9a6d..39eeec1b 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -459,6 +459,11 @@ find_certificate_in_store(const char *cert_prop,
HCERTSTORE cert_store)
find_param = wide_string(cert_prop + 5, );
find_type = CERT_FIND_SUBJECT_STR_W;
}
+else if (!strncmp(cert_prop, "ISSUER:", 7))
+{
+find_param = wide_string(cert_prop + 7, );
+find_type = CERT_FIND_ISSUER_STR_W;
+}
else if (!strncmp(cert_prop, "THUMB:", 6))
{
const char *p;
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 4/4] cryptoapi.c: simplify parsing of thumbprint hex string
From: Selva Nair
Signed-off-by: Selva Nair
---
src/openvpn/cryptoapi.c | 44 +++--
1 file changed, 12 insertions(+), 32 deletions(-)
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 6ff4fcb5..9fd5aea9 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -210,49 +210,29 @@ find_certificate_in_store(const char *cert_prop,
HCERTSTORE cert_store)
}
else if (!strncmp(cert_prop, "THUMB:", 6))
{
-const char *p;
-int i, x = 0;
find_type = CERT_FIND_HASH;
find_param =
-/* skip the tag */
-cert_prop += 6;
-for (p = cert_prop, i = 0; *p && i < sizeof(hash); i++)
+int i = 0;
+
+for (const char *p = cert_prop + 6; *p && i < sizeof(hash); p += 2)
{
-if (*p >= '0' && *p <= '9')
-{
-x = (*p - '0') << 4;
-}
-else if (*p >= 'A' && *p <= 'F')
+/* skip spaces */
+while (*p == ' ')
{
-x = (*p - 'A' + 10) << 4;
+p++;
}
-else if (*p >= 'a' && *p <= 'f')
+if (!*p) /* ending with spaces is not an error */
{
-x = (*p - 'a' + 10) << 4;
+break;
}
-if (!*++p) /* unexpected end of string */
+
+if (!isxdigit(p[0]) || !isxdigit(p[1])
+|| sscanf(p, "%2hhx", [i++]) != 1)
{
-msg(M_WARN|M_INFO, "WARNING: cryptoapicert: error parsing
.", cert_prop);
+msg(M_WARN|M_INFO, "WARNING: cryptoapicert: error parsing
<%s>.", cert_prop);
goto out;
}
-if (*p >= '0' && *p <= '9')
-{
-x += *p - '0';
-}
-else if (*p >= 'A' && *p <= 'F')
-{
-x += *p - 'A' + 10;
-}
-else if (*p >= 'a' && *p <= 'f')
-{
-x += *p - 'a' + 10;
-}
-hash[i] = x;
-/* skip any space(s) between hex numbers */
-for (p++; *p && *p == ' '; p++)
-{
-}
}
blob.cbData = i;
}
--
2.34.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 0/4] Improvements for cryptoapi.c
From: Selva Nair 1. Support selecting certificate using issuer name (goal: "planned obsolescence" of 2.6, already :) 2. Log the selected certificate's name 3. Remove Pre OpenSSL-3.01 support (goal: leaner and meaner) 4. Simplify parsing of thumbprint hex string doc/man-sections/windows-options.rst | 13 +- src/openvpn/cryptoapi.c | 629 +++ src/openvpn/options.c| 2 +- src/openvpn/win32-util.c | 15 + src/openvpn/win32-util.h | 3 + 5 files changed, 84 insertions(+), 578 deletions(-) -- 2.34.1 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
