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 optional support for the AEGIS cipher suites #478

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Conversation

jedisct1
Copy link
Contributor

@jedisct1 jedisct1 commented Aug 1, 2023

This adds the following TLS 1.3 suites:

  • TLS_AEGIS_256_SHA384 (0x13, 0x07)
  • TLS_AEGIS_128L_SHA256 (0x13, 0x06)

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

@jedisct1
Copy link
Contributor Author

jedisct1 commented Aug 1, 2023

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
Copy link
Contributor

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

@jedisct1
Copy link
Contributor Author

jedisct1 commented Aug 2, 2023

Awesome, thank you!

@victorstewart
Copy link
Contributor

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
Copy link
Collaborator

huitema commented Aug 2, 2023

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
Copy link
Contributor Author

jedisct1 commented Aug 2, 2023

libaegis also works on CPUs without hardware AES support.

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

@jedisct1
Copy link
Contributor Author

jedisct1 commented Aug 2, 2023

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.

Copy link
Member

@kazuho kazuho left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request. This is exciting!

Changes look good, I only have nitpicks.

At first glance, it did seem odd to me to have the definitions in libaegis.h and including them in openssl.c. But that makes sense, a TLS cipher suite is a combination of an AEAD cipher and a hash function. We have to borrow a hash function from somewhere.

As to @huitema's point of having aegis support on the side of minicrypto, I think it makes sense. We can add a source file that just includes libaegis.h and combines the AEAD ciphers with the ptls-minicrypto sha objects. But I would not mind that being deferred to a separate pull request.

CMakeLists.txt Outdated Show resolved Hide resolved
lib/libaegis.h Show resolved Hide resolved
@kazuho
Copy link
Member

kazuho commented Aug 3, 2023

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
Copy link
Contributor Author

jedisct1 commented Aug 3, 2023

@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
Copy link
Member

kazuho commented Aug 3, 2023

@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
Copy link
Contributor Author

jedisct1 commented Aug 3, 2023

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

@kazuho
Copy link
Member

kazuho commented Aug 3, 2023

Thanks!

I've added libaegis to the CI image (h2o/h2o#3268), and pushed a commit that turns on aegis in CI (2897b64).

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
Copy link
Contributor Author

jedisct1 commented Aug 3, 2023

Support for minicrypto has been added :)

@jedisct1
Copy link
Contributor Author

jedisct1 commented Aug 3, 2023

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

@victorstewart
Copy link
Contributor

@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
Copy link
Member

kazuho commented Aug 4, 2023

@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
Copy link
Contributor Author

jedisct1 commented Aug 4, 2023

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

This adds the following TLS 1.3 suites:

- `TLS_AEGIS_256_SHA384 (0x13, 0x07)`
- `TLS_AEGIS_128L_SHA256 (0x13, 0x06)`

Requires `libaegis`.
@jedisct1
Copy link
Contributor Author

jedisct1 commented Aug 4, 2023

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
Copy link
Member

kazuho commented Aug 7, 2023

@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
Copy link
Contributor Author

jedisct1 commented Aug 7, 2023

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

@jedisct1
Copy link
Contributor Author

jedisct1 commented Aug 8, 2023

ptls_{minicrypto,openssl}_cipher_suites_all has been added.

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

@kazuho kazuho merged commit fe2cb6b into h2o:master Aug 9, 2023
9 checks passed
@kazuho
Copy link
Member

kazuho commented Aug 9, 2023

Thank you for the changes! Let's merge.

kazuho added a commit to h2o/h2o that referenced this pull request Aug 9, 2023
@kazuho kazuho mentioned this pull request Sep 19, 2023
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.

4 participants