h2o / picotls

TLS 1.3 implementation in C (master supports RFC8446 as well as draft-26, -27, -28)
536 stars 140 forks source link

Add optional support for the AEGIS cipher suites #478

Closed jedisct1 closed 1 year ago

jedisct1 commented 1 year ago

This adds the following TLS 1.3 suites:

Requires libaegis, and currently OpenSSL/BoringSSL for the key derivation.

jedisct1 commented 1 year ago

There's probably a way for cmake to automatically find the headers, and maybe automatically build libaegis given the path to its source code.

Unfortunately, I'm not familiar at all with cmake, so help would be definitely welcome.

/cc @victorstewart

victorstewart commented 1 year ago

i will gladly write the automatic build code today 🤩. i can't wait to play with this in QUIC.

jedisct1 commented 1 year ago

Awesome, thank you!

victorstewart commented 1 year ago

that's done i sent you a PR. what do you think about adding experimental support for x2 so we can play with that too?

huitema commented 1 year ago

Is it possible to add a hardware-agnostic implementation of AEGIS to minicrypto? That would help experimenting with AEGIS on platforms like Arduino, for which the current options are not very appealing.

jedisct1 commented 1 year ago

libaegis also works on CPUs without hardware AES support.

Do you mean adding an AEGIS implementation directly to the minicrypto code?

jedisct1 commented 1 year ago

that's done i sent you a PR. what do you think about adding experimental support for x2 so we can play with that too?

AEGIS-128X/256X are not in the spec and don't have IANA identifiers yet. One thing at a time.

kazuho commented 1 year ago

PS. For CI, am I correct to understand that I should be adding https://github.com/jedisct1/libaegis to the CI image so that we can test the aegis integration? @jedisct1

jedisct1 commented 1 year ago

@kazuho What would be the best way to add libaegis as a dependency? Shall a copy of it be added to deps/ like cifra and micro-ecc?

kazuho commented 1 year ago

@jedisct1

What would be the best way to add libaegis as a dependency? Shall a copy of it be added to deps/ like cifra and micro-ecc?

I do not think we need to add libaegis to deps/.

Our CI (see .github/workflows/ci.yml and misc/docker-ci.mk) uses a Docker image built using https://github.com/h2o/h2o/blob/master/misc/docker-ci/Dockerfile.ubuntu2004.

Assuming that we want to test aegis integration, I am wondering how I should change this Dockerfile.

I would assume what I'm expected to do is fetch and build the HEAD of https://github.com/jedisct1/libaegis in the Dockerfile and set WITH_AEGIS=1 when running CMake of picotls. Am I correct?

jedisct1 commented 1 year ago

In that case, yes, you can build and install it in the Docker image from the libaegis repository.

kazuho commented 1 year ago

Thanks!

I've added libaegis to the CI image (https://github.com/h2o/h2o/pull/3268), and pushed a commit that turns on aegis in CI (https://github.com/h2o/picotls/commit/2897b64e2ddad9c0d348344ed730fa0e6c579340).

Please feel free to pick the commit to this PR.

However, the CI is failing.

Could you please take a look at it?

You can click Screenshot 2023-08-03 at 15 59 20 on the CI page and see the raw logs, I think that'd help.

jedisct1 commented 1 year ago

Support for minicrypto has been added :)

jedisct1 commented 1 year ago

Hi @kazuho ! Could you update the docker image with the current libaegis code? That should fix it.

victorstewart commented 1 year ago

@kazuho

@jedisct1

What would be the best way to add libaegis as a dependency? Shall a copy of it be added to deps/ like cifra and micro-ecc?

I do not think we need to add libaegis to deps/.

i think what makes most sense is what i wrote in my build PR that if aegis is enabled, but no libaegis source code directory (as requested by Frank) is specified and find_package is unable to find libaegis installed on any paths, that it automatically downloads and builds libaegis from GitHub.

kazuho commented 1 year ago

@jedisct1 Thank you for the changes!

I've updated the CI image, but we are seeing issues.

It seems to me that the aegis backends of openssl and minicrypto contradict to each other, as the "picotls" subtest is passing but "minicrypto vs." and "vs. minicrypto" are failing.

Could you look into it?

jedisct1 commented 1 year ago

Thank you! Yes, I'm looking into it.

jedisct1 commented 1 year ago

Alright. Rebased and squashed. CI's finally happy. 🎉

The fact that openssl_ctx_sha256only assumes that there is only one cipher suite using SHA-384 and that it's always the first element of the list got me confused ;)

kazuho commented 1 year ago

@jedisct1 Thank you for the fix!

I think all the code is ready, I only have one question.

Do you think support for aegis should be enabled by default? The thing is that ptls_openssl_cipher_suites and ptls_minicrypto_cipher_suites are our default, it makes me a bit uneasy to have aegis included there, as support for aegis will be enabled instantly on most programs that uses picotls.

I am considering of introducing ptls_openssl_cipher_suites_all and ptls_minicrypto_cipher_suites_all, and start with aegis added only to these. As time goes, we can add aegis to our default set.

The idea is that applications can continue using ptls_openssl_cipher_suites as the default, at the same time search for user-chosen cipher-suites from ptls_openssl_cipher_suites_all so that aegis can be used if the user manually choses the cipher-suites.

Does that make sense to you?

jedisct1 commented 1 year ago

@kazuho Yes, introducing a distinct cipher suite sounds good.

jedisct1 commented 1 year ago

ptls_{minicrypto,openssl}_cipher_suites_all has been added.

Is there any other changes you would like, @kazuho ?

kazuho commented 1 year ago

Thank you for the changes! Let's merge.