Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more PQC hybrid key exchange algorithms #7821

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Frauschi
Copy link
Contributor

@Frauschi Frauschi commented Aug 1, 2024

Hi,

This PR adds support for all remaining hybrid PQC + ECC hybrid key exchange groups to match OQS. Next to two new combinations with SECP curves, this mainly also adds support for combinations with X25519 and X448.

This also enables compatibility with the PQC key exchange support in Chromium browsers and Mozilla Firefox (hybrid Kyber768 and X25519; when WOLFSSL_ML_KEM is not defined).

In the process of extending support, some code and logic cleanup happened. Furthermore, two memory leaks within the hybrid code path have been fixed.

Looking forward to your feedback.

@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@dgarske
Copy link
Contributor

dgarske commented Aug 1, 2024

Okay to test. Contributor agreement on file. @anhu please review. Thanks

@dgarske
Copy link
Contributor

dgarske commented Aug 1, 2024

Seems to be consistently failing with:

   842: test_multiple_crls_same_issuer                      :error = -361, certificate revoked
error = -313, received alert fatal error

ERROR - tests/api.c line 7417 failed with:
    expected: test_ssl_memio_do_handshake(&test_ctx, 10, ((void *)0)) == (1)
    result:   0 != 1

error = -361, certificate revoked
error = -313, received alert fatal error

ERROR - tests/api.c line 7417 failed with:
    expected: test_ssl_memio_do_handshake(&test_ctx, 10, ((void *)0)) == (1)
    result:   0 != 1

...

Server Random : 136CBD0B1E6A36EBA50790532D5963534E14BF12E3SSL_accept error -352, Bad ECC Peer Key
wolfSSL error: SSL_accept failed
C35D79C957A2C04C2DF2C2
Client message: hello wolfssl!
I hear you fa shizzle!
trying server command line[1541]: SuiteTest -v 4 -l TLS13-AES128-GCM-SHA256 -c ./certs/server-ecc.pem -k ./certs/ecc-key.pem -Y -2 -p 0 
trying client command line[1542]: SuiteTest -v 4 -l TLS13-AES128-GCM-SHA256 -A ./certs/ca-ecc-cert.pem -y -2 -p 34205 
FAIL scripts/unit.test (exit status: 1)

Copy link
Member

@anhu anhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be sure to test against --enable-kyber

Comment on lines +299 to +301
{ WOLFSSL_X25519_KYBER_LEVEL1, "X25519_KYBER_LEVEL1" },
{ WOLFSSL_X448_KYBER_LEVEL3, "X448_KYBER_LEVEL3" },
{ WOLFSSL_X25519_KYBER_LEVEL3, "X25519_KYBER_LEVEL3" },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its interesting.

You are adding :

  • WOLFSSL_X25519_KYBER_LEVEL1
  • WOLFSSL_X448_KYBER_LEVEL3
  • WOLFSSL_X25519_KYBER_LEVEL3
  • WOLFSSL_P256_KYBER_LEVEL3
  • WOLFSSL_P384_KYBER_LEVEL5

Can you let us know why you are adding these? I suppose they are to interop with other places, but can you let us know specifically (ie which ones are mozilla and which ones are liboqs)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am currently investigating the real-world performance impact of various PQC deployments in different embedded context (for my PhD). As hybrid constellations using X25519 and X448 are pretty popular in other projects, I wanted to test them, too. That's why I started this work in the first place.

The compatibility with the web browsers is based on the use case to access a web-based management interface of an embedded device and to make that PQC secure. For that, WOLFSSL_X25519_KYBER_LEVEL3 is the relevant hybrid constellation (that is what browser vendors are currently settled on).

The other constellations are based on what OQS has defined. I added them (besides my internal performance tests) for general interoperability with OQS and because it wasn't much overhead anyway.

@Frauschi
Copy link
Contributor Author

Frauschi commented Aug 2, 2024

Updated the PR with a fix for the failing tests.

Please be sure to test against --enable-kyber

The tests now properly work with --enable-all --enable-asn=template, --enable-all --enable-asn=template --enable-experimental --enable-kyber and --enable-all --enable-asn=template --enable-experimental --with-liboqs

Furthermore, I updated the unit tests to also test the new changes. The only thing I had to test manually are the new hybrid curves with DTLS (when also enabled with --enable-dtls13 --enable-dtls-frag-ch), as the unit tests break before even reaching those tests (pretty surely not related to this PR, as it fails equally on current master). Using the example client and server with the same arguments as the unit tests, the new curves also work with DTLS. I opened an issue for the failing DTLS tests (see #7825).

@Frauschi Frauschi requested a review from anhu August 2, 2024 14:16
@dgarske
Copy link
Contributor

dgarske commented Aug 5, 2024

Hi @Frauschi we had some issues with our GitHub CI actions. If you rebase this to latest master it should resolve the errors you are seeing. Thank you

@Frauschi
Copy link
Contributor Author

Frauschi commented Aug 5, 2024

Rebased to current master. Thanks for the hint.

FYI, I'm on vacation for the next two weeks, so any upcoming work on this will probably be delayed until afterward.

@Frauschi
Copy link
Contributor Author

Rebased to current master (since #7807 has been merged). I also think that the one NGINX test that has been failing wasn't related to this PR's changes? Can someone retest this please?

@douzzer
Copy link
Contributor

douzzer commented Aug 23, 2024

retest this please.

@JacobBarthelmeh
Copy link
Contributor

This says the branch has conflicts that must be resolved. Could you rebase on top of wolfssl/master please?

Add support for all remaining hybrid PQC + ECC hybrid key exchange
groups to match OQS. Next to two new combinations with SECP curves, this
mainly also adds support for combinations with X25519 and X448.

This also enables compatability with the PQC key exchange support in
Chromium browsers and Mozilla Firefox (hybrid Kyber768 and X25519; when
`WOLFSSL_KYBER_ORIGINAL` is defined).

In the process of extending support, some code and logic cleanup
happened. Furthermore, two memory leaks within the hybrid code path have
been fixed.

Signed-off-by: Tobias Frauenschläger <tobias.frauenschlaeger@oth-regensburg.de>
@Frauschi
Copy link
Contributor Author

This says the branch has conflicts that must be resolved. Could you rebase on top of wolfssl/master please?

Rebased to current master.

I also updated the PR to reflect the latest Code Point changes in OQS and also incorporated the new Code Points currently set in draft-kwiatkowski-tls-ecdhe-mlkem.

The only thing that is not done yet is the proposed swap of classic and PQC key material within draft-kwiatkowski-tls-ecdhe-mlkem in the key share in case of X25519 hybrids. This is done to always have a FIPS approved algorithms first in case of hybrids (and X25519 is not FIPS approved). See here.

This swap still has to be implemented. However, that will require more thorough code changes. I think I can tackle that around next week. You can decide if this should also go into this PR, or if that should go in a separate one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants