Thanks very much for the quick and thorough feedback. Indeed your last
question is pivotal making the patch _much_ simpler (attached): The
problem manifests itself only in the presence of providers introduced in
OpenSSL3.0. At the same time, the curve name causing the "dance code" is
permitted as of OpenSSL3.0...
So all other observations below are moot/should be resolved with the
much simpler new patch attached. Feel free to delete/amend the comment
changes as you see fit.
--Michael
Am 24.03.22 um 18:48 schrieb Arne Schwabe:
Am 24.03.22 um 14:40 schrieb Michael Baentsch:
Hello,
as per https://community.openvpn.net/openvpn/ticket/1460 the
current openvpn master fails when activating a TLS1.3 group
implemented in an external provider.
The patch attached fixes this and enables successful OpenSSL key
establishment using any of the quantum-safe and hybrid (classic/QSC)
algorithms supported by
https://github.com/open-quantum-safe/oqs-provider
Thanks for the patch. Usually we would like to have patches in a git
format that contains a commit message (see git format-patch) and
https://github.com/OpenVPN/openvpn/blob/master/CONTRIBUTING.rst
The current patch has a few problems:
- Breaks OpenSSL 1.0.2 compatibity. Currently we still support RHEL7,
which only has OpenSSL 1.0.2
- uses C90 variable declaration (int rc)
- indentation problems
We normally use our own gc_alloc etc usage for strings as the previous
code did. That usage was replaced with malloc/free.
And the code does not have any comments but removes the existing ones
and with a line like this:
memcpy(idx, f+strlen("secp256r1"),
strlen(groups)-(f-groups)-strlen("secp256r1"));
some comments what the code is doing would improve readibility.
The fix itself is right of using the newer SSL_CTX_set1_groups_list
function that uses strings instead of nids.
But if we use a function that only newer OpenSSL version, have you
double checked that the prime256v1 vs secp256r1 is still necessary
Build failure with OpenSSL 1.0.2:
https://github.com/schwabe/openvpn/runs/5680551849?check_suite_focus=true
Arne
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index b8595174..af97dabc 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -572,13 +572,15 @@ void
tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
{
ASSERT(ctx);
+#if OPENSSL_VERSION_NUMBER < 0x30000000L
struct gc_arena gc = gc_new();
/* This method could be as easy as
* SSL_CTX_set1_groups_list(ctx->ctx, groups)
- * but OpenSSL does not like the name secp256r1 for prime256v1
+ * but OpenSSL (< 3.0) does not like the name secp256r1 for prime256v1
* This is one of the important curves.
* To support the same name for OpenSSL and mbedTLS, we do
* this dance.
+ * Also note that the code is wrong in the presence of OpenSSL3 providers.
*/
int groups_count = get_num_elements(groups, ':');
@@ -617,6 +619,13 @@ tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
groups);
}
gc_free(&gc);
+#else
+ if (!SSL_CTX_set1_groups_list(ctx->ctx, groups))
+ {
+ crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
+ groups);
+ }
+#endif
}
void
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
