[Openvpn-devel] Enablement of quantum-safe key establishment

Message ID 400a4652-39d6-ec8d-6f58-824f552e0440@baentsch.ch
State Changes Requested
Headers show
Series
  • [Openvpn-devel] Enablement of quantum-safe key establishment
Related show

Commit Message

Michael Baentsch March 24, 2022, 1:40 p.m.
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

Regards,

--Michael

Comments

Arne Schwabe March 24, 2022, 5:48 p.m. | #1
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
Michael Baentsch March 25, 2022, 7:21 a.m. | #2
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
Arne Schwabe March 25, 2022, 3:04 p.m. | #3
Am 25.03.22 um 08:21 schrieb Michael Baentsch:
> 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.

Thanks v2 looks much better code wise and gets ack from me. To commit 
the patch could you either send the patch in git format-message format 
or provide a commit subject and text so we can make full commit of your 
patch?

Arne
Antonio Quartulli March 25, 2022, 3:09 p.m. | #4
On 25/03/2022 16:04, Arne Schwabe wrote:
> Am 25.03.22 um 08:21 schrieb Michael Baentsch:
>> 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.
> 
> Thanks v2 looks much better code wise and gets ack from me. To commit 
> the patch could you either send the patch in git format-message format 
> or provide a commit subject and text so we can make full commit of your 
> patch?

We also need the patch to bear the sign-off-by line, so better create an 
actual git commit and export it via git-format-patch.

Cheers,
Michael Baentsch March 25, 2022, 5:10 p.m. | #5
I'm not quite sure I understand what you're asking me to do. Why not 
simply use standard github cooperation mechanisms?

Whatever, I put the stuff into 
https://github.com/OpenVPN/openvpn/pull/170 but couldn't run the command 
suggested there:

$ git send-email --to=openvpn-devel@lists.sourceforge.net HEAD~1
git: 'send-email' ist kein Git-Befehl. Siehe 'git --help'.

Pardon the German of my computer: It says "send-email" is no known git 
command.

Please let me know what to do now.

Regards,

--Michael


FWIW, I now have
Am 25.03.22 um 16:09 schrieb Antonio Quartulli:
> On 25/03/2022 16:04, Arne Schwabe wrote:
>> Am 25.03.22 um 08:21 schrieb Michael Baentsch:
>>> 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.
>>
>> Thanks v2 looks much better code wise and gets ack from me. To commit 
>> the patch could you either send the patch in git format-message 
>> format or provide a commit subject and text so we can make full 
>> commit of your patch?
>
> We also need the patch to bear the sign-off-by line, so better create 
> an actual git commit and export it via git-format-patch.
>
> Cheers,
>
>
Gert Doering March 25, 2022, 5:21 p.m. | #6
Hi,

On Fri, Mar 25, 2022 at 06:10:10PM +0100, Michael Baentsch wrote:
> I'm not quite sure I understand what you're asking me to do. Why not 
> simply use standard github cooperation mechanisms?

Because our master repo is not "on github", and we do patch review
via the mailing list, with archived public ACKs, and list archive
URLs included in the commit message.  So it is very clear how a 
certain code change came into our tree.

(Is this highly annoying and very much time consuming?  Yes... 
but also "is this appropriate for software handling people's most
sensitive data packets?" - and the answer for that is "yes!" as 
well... :-) )

> $ git send-email --to=openvpn-devel@lists.sourceforge.net HEAD~1
> git: 'send-email' ist kein Git-Befehl. Siehe 'git --help'.
>
> Pardon the German of my computer: It says "send-email" is no known git 
> command.

Seems your distribution split this part of git into a separate package
(propably to avoid the dependency on a mail transport agent).

Of course you can do "git format-patch" and then send the result with
a regular mail client - but we've found out (the hard way) that most
mail clients mistreat patches one way or the other - breaking long
lines, reformatting whitespace, ... - so the end result does not 
apply anymore. "git send-email" works around this.

If you do not succeed in this, one of us can take the patch from the
PR and send it to the list.

gert
Arne Schwabe March 28, 2022, 11:52 a.m. | #7
Am 27.03.22 um 17:52 schrieb Michael Baentsch:
> Thanks again for your explanations: I finally figured out to correct my 
> git send-email configuration `smtpencryption` to be set to "ssl" 
> (instead of "tls": The latter caused a hang that I debugged for way too 
> long :-(. Maybe worth while adding to some FAQ for newbies? The guidance 
> at 
> https://github.com/git/git/blob/master/Documentation/git-send-email.txt 
> was clearly wrong.
> 
> Please let me know if that submission now arrived and meets your 
> requirements.

The commit message is still not great. Something I would have used instead:

Allow non-standard EC groups with OpenSSL3

OpenSSL3 no longer uses the NID to identify TLS groups, instead it uses
names. This allows also to use groups from external provider. It also 
recognises secp256r1 as the same group as prime256v1.



> 
> One further question: Is there interest on your side to add more/better 
> support for quantum-safe crypto to OpenVPN? 

Depends on what changes you are proposing. There is certainly some 
interest but depends on what exactly we are talking about.

> easyrsa isn't geared for 
> that right now (let alone suitably named :-), but openssl3 (with our 
> oqsprovider) can generate quantum-safe PKI (CA and client certs) without 
> problems.

Easyrsa has become also separate project. Development and maintainance 
of easyrsa have become quite slow in the last years.

> Quantum-safe key exchange works in OpenVPN just fine when the PR lands.
>
Michael Baentsch March 28, 2022, 1:56 p.m. | #8
Am 28.03.22 um 13:52 schrieb Arne Schwabe:
> Am 27.03.22 um 17:52 schrieb Michael Baentsch:
>> Thanks again for your explanations: I finally figured out to correct 
>> my git send-email configuration `smtpencryption` to be set to "ssl" 
>> (instead of "tls": The latter caused a hang that I debugged for way 
>> too long :-(. Maybe worth while adding to some FAQ for newbies? The 
>> guidance at 
>> https://github.com/git/git/blob/master/Documentation/git-send-email.txt 
>> was clearly wrong.
>>
>> Please let me know if that submission now arrived and meets your 
>> requirements.
>
> The commit message is still not great. Something I would have used 
> instead:
Well....
>
> Allow non-standard EC groups with OpenSSL3
This statement just is not correct: This has not a lot to do with EC. 
What about "Enable setting any TLS1.3 group [provided by the underlying 
crypto libraries]. "?
>
> OpenSSL3 no longer uses the NID to identify TLS groups, instead it uses
> names. This allows also to use groups from external provider. It also 
> recognises secp256r1 as the same group as prime256v1.
This statement also is not quite right: OpenSSL3 still uses NIDs to 
identify some groups, notably those not implemented by a provider, i.e., 
legacy/"classic" crypto. I would agree with the statement that "OpenSSL3 
prefers the use of names over NIDs for the identification of TLS1.3 
groups, including EC groups." Lastly, the "it" in your statement above 
is unclear (at least to me as a non-native speaker): What about 
explicitly stating "OpenSSL3 also recognises secp256r1 as the same group 
as prime256v1"? This fact of course has nothing to do with this patch.
>
>
>
>>
>> One further question: Is there interest on your side to add 
>> more/better support for quantum-safe crypto to OpenVPN? 
>
> Depends on what changes you are proposing. There is certainly some 
> interest but depends on what exactly we are talking about.
I'll flesh that out. It mostly has to do with modifications to "easyrsa" 
- see below.
>
>> easyrsa isn't geared for that right now (let alone suitably named 
>> :-), but openssl3 (with our oqsprovider) can generate quantum-safe 
>> PKI (CA and client certs) without problems.
>
> Easyrsa has become also separate project. Development and maintainance 
> of easyrsa have become quite slow in the last years.
That statement is very helpful, thanks. It might then be sensible (for 
me) to start a script from scratch that does not have such strong 
dependency on legacy crypto. Will report back when I have something of 
interest.
>
>> Quantum-safe key exchange works in OpenVPN just fine when the PR lands.
>>
Thanks in advance for any guidance how to proceed with the PR now.
Kristof Provost via Openvpn-devel March 28, 2022, 2:07 p.m. | #9
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

EasyRSA development is back on-track.

Happy to help.

BR

------- Original Message -------

On Monday, March 28th, 2022 at 14:56, Michael Baentsch <info@baentsch.ch> wrote:

> Am 28.03.22 um 13:52 schrieb Arne Schwabe:

> >
> > Easyrsa has become also separate project. Development and maintainance
> >
> > of easyrsa have become quite slow in the last years.
>

-----BEGIN PGP SIGNATURE-----
Version: ProtonMail

wsBzBAEBCAAGBQJiQcETACEJEE+XnPZrkLidFiEECbw9RGejjXJ5xVVVT5ec
9muQuJ2sGgf+ItPKF6rxjuLxtLJ8IeV8powsxVVmRCJ07c5OgyT98r8zDYsv
NzaQ//cQou736YsA5lPhSfAFvqxLcAAsjyUsqJ24uSOyR49IND7pOe1p06Ea
Jp31EQSCqZU0RkvGkgxGL//j+dg2dq7PLKYpy9axwPBeWb+GWjWZlso6QSrZ
Br73r6qz+nDdQ6JHdt1ZiAti2gewvxkamR+4H51dMjqzrJ81xUbZIjRi6ALX
niJ4hQf0yKtgeZBf3GCLRNtoNB56x5liJIBwrpUA4NTbFaEEiK+kDy/0Rgh2
omuOKZnBgtZR/K/jTh/VTCh8Rn1owKULTTMxnvGdB7oPCMmGN5QgdA==
=N9Ct
-----END PGP SIGNATURE-----
Arne Schwabe March 28, 2022, 7:29 p.m. | #10
>> Allow non-standard EC groups with OpenSSL3
> This statement just is not correct: This has not a lot to do with EC. 
> What about "Enable setting any TLS1.3 group [provided by the underlying 
> crypto libraries]. "?

A bit long for a commit subject. Maybe just:

Enable usage of TLS groups not identified by a NID in OpenSSL 3


>> OpenSSL3 no longer uses the NID to identify TLS groups, instead it uses
>> names. This allows also to use groups from external provider. It also 
>> recognises secp256r1 as the same group as prime256v1.
> This statement also is not quite right: OpenSSL3 still uses NIDs to 
> identify some groups, notably those not implemented by a provider, i.e., 
> legacy/"classic" crypto. I would agree with the statement that "OpenSSL3 
> prefers the use of names over NIDs for the identification of TLS1.3 
> groups, including EC groups." Lastly, the "it" in your statement above 
> is unclear (at least to me as a non-native speaker): What about 
> explicitly stating "OpenSSL3 also recognises secp256r1 as the same group 
> as prime256v1"? This fact of course has nothing to do with this patch.

This fact has to do with the patch since you no longer doing that dance 
with that curve for the new code that affects OpenSSL 3, so pointing 
that out in the commit message helps to understand the code change.

Maybe like this:

OpenSSL3 prefers to specify groups (including EC groups) with names 
instead of NID to allow also groups provided by providers.
This commits also remove the mapping of secp256r1 to prime256v1 for the 
OpenSSL3 code path as OpenSSL 3.0 recognises secp256r1.

Arne

Patch

index b8595174..73ab4b6a 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -572,7 +572,9 @@  void
 tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
 {
     ASSERT(ctx);
-    struct gc_arena gc = gc_new();
+    char *f = strstr(groups, "secp256r1");
+    int rc;
+    
     /* 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
@@ -580,43 +582,26 @@  tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
      * To support the same name for OpenSSL and mbedTLS, we do
      * this dance.
      */
-
-    int groups_count = get_num_elements(groups, ':');
-
-    int *glist;
-    /* Allocate an array for them */
-    ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
-
-    /* Parse allowed ciphers, getting IDs */
-    int glistlen = 0;
-    char *tmp_groups = string_alloc(groups, &gc);
-
-    const char *token;
-    while ((token = strsep(&tmp_groups, ":")))
-    {
-        if (streq(token, "secp256r1"))
-        {
-            token = "prime256v1";
-        }
-        int nid = OBJ_sn2nid(token);
-
-        if (nid == 0)
-        {
-            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
-        }
-        else
-        {
-            glist[glistlen] = nid;
-            glistlen++;
-        }
+    if (f) {
+         char *new_groups_list = malloc(strlen(groups)+2);
+	 char * idx = new_groups_list;
+	 memcpy(idx, groups, (f-groups));
+	 idx += (f-groups);
+	 memcpy(idx, "prime256v1", strlen("prime256v1"));
+	 idx += strlen("prime256v1");
+	 memcpy(idx, f+strlen("secp256r1"), strlen(groups)-(f-groups)-strlen("secp256r1"));
+	 new_groups_list[strlen(groups)+1] = '\0';
+
+	 rc = SSL_CTX_set1_groups_list(ctx->ctx, new_groups_list);
+	 free(new_groups_list);
     }
+    else
+	 rc = SSL_CTX_set1_groups_list(ctx->ctx, groups);
 
-    if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
-    {
+    if (!rc) {
         crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
                    groups);
     }
-    gc_free(&gc);
 }
 
 void