| Message ID | 400a4652-39d6-ec8d-6f58-824f552e0440@baentsch.ch |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] Enablement of quantum-safe key establishment | expand |
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
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
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
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,
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, > >
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
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. >
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.
-----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-----
>> 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
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