Remove s2n's internal Kyber512 implementation, and rely on AWS-LC for Kyber support #4283
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,6 @@ cp -r \ | |
| ../../api \ | ||
| ../../crypto \ | ||
| ../../error \ | ||
| ../../stuffer \ | ||
| ../../tls \ | ||
| ../../utils \ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,21 +16,20 @@ | |
| #include <openssl/evp.h> | ||
| #include <stddef.h> | ||
|
|
||
| #include "crypto/s2n_pq.h" | ||
| #include "error/s2n_errno.h" | ||
| #include "tls/s2n_kem.h" | ||
| #include "utils/s2n_safety.h" | ||
| #include "utils/s2n_safety_macros.h" | ||
|
|
||
| #if defined(S2N_LIBCRYPTO_SUPPORTS_KYBER) | ||
|
|
||
| DEFINE_POINTER_CLEANUP_FUNC(EVP_PKEY *, EVP_PKEY_free); | ||
| DEFINE_POINTER_CLEANUP_FUNC(EVP_PKEY_CTX *, EVP_PKEY_CTX_free); | ||
|
|
||
| int s2n_kyber_evp_generate_keypair(IN const struct s2n_kem *kem, OUT uint8_t *public_key, | ||
| OUT uint8_t *secret_key) | ||
| { | ||
| DEFER_CLEANUP(EVP_PKEY_CTX *kyber_pkey_ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_KEM, NULL), EVP_PKEY_CTX_free_pointer); | ||
| POSIX_GUARD_PTR(kyber_pkey_ctx); | ||
| POSIX_GUARD_OSSL(EVP_PKEY_CTX_kem_set_params(kyber_pkey_ctx, kem->kem_nid), S2N_ERR_PQ_CRYPTO); | ||
|
|
@@ -53,7 +52,6 @@ int s2n_kyber_evp_generate_keypair(IN const struct s2n_kem *kem, OUT uint8_t *pu | |
| int s2n_kyber_evp_encapsulate(IN const struct s2n_kem *kem, OUT uint8_t *ciphertext, OUT uint8_t *shared_secret, | ||
| IN const uint8_t *public_key) | ||
| { | ||
| DEFER_CLEANUP(EVP_PKEY *kyber_pkey = EVP_PKEY_kem_new_raw_public_key(kem->kem_nid, public_key, kem->public_key_length), EVP_PKEY_free_pointer); | ||
| POSIX_GUARD_PTR(kyber_pkey); | ||
|
|
||
|
|
@@ -74,7 +72,6 @@ int s2n_kyber_evp_encapsulate(IN const struct s2n_kem *kem, OUT uint8_t *ciphert | |
| int s2n_kyber_evp_decapsulate(IN const struct s2n_kem *kem, OUT uint8_t *shared_secret, IN const uint8_t *ciphertext, | ||
| IN const uint8_t *private_key) | ||
| { | ||
| DEFER_CLEANUP(EVP_PKEY *kyber_pkey = EVP_PKEY_kem_new_raw_secret_key(kem->kem_nid, private_key, kem->private_key_length), EVP_PKEY_free_pointer); | ||
| POSIX_GUARD_PTR(kyber_pkey); | ||
|
|
||
|
|
@@ -90,36 +87,24 @@ int s2n_kyber_evp_decapsulate(IN const struct s2n_kem *kem, OUT uint8_t *shared_ | |
| return S2N_SUCCESS; | ||
| } | ||
|
|
||
| #else /* If !S2N_LIBCRYPTO_SUPPORTS_KYBER, we won't have a Kyber impl so define relevant stubs here. */ | ||
|
|
||
|
Comment on lines
+90
to
91
There was a problem hiding this comment. Isn't this a breaking change? Wont' we break anyone using pq with a libcrypto other than awslc? There was a problem hiding this comment.
All AWS teams that are currently using Hybrid PQ TLS are able to use s2n-tls with AWS-LC. I'm not aware of any other external users, but if there are others using s2n's Kyber implementation with an Openssl libcrypto, they will gracefully fall back to regular classical algorithms. This PR would be a behavioral change in which SupportedGroup is negotiated at runtime by s2n, but likely not a "breaking" change. Customers won't start receiving errors from s2n after this change if using PQ TLS Policies, but if they have tests that confirm that a specific Kyber SupportedGroup was negotiated then those tests might fail. In the AWS SDK Java documentation, we've also mentioned that these PQ algorithms may stopped being supported at any time.
From reading the versioning policy doc, the line "Possible backwards-incompatible changes. These changes will be noted and explained in detail in the release notes" best fits this change. I think bumping s2n-tls from |
||
| int s2n_kyber_evp_generate_keypair(IN const struct s2n_kem *kem, OUT uint8_t *public_key, | ||
| OUT uint8_t *secret_key) | ||
| { | ||
| POSIX_BAIL(S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); | ||
| } | ||
|
|
||
| int s2n_kyber_evp_encapsulate(IN const struct s2n_kem *kem, OUT uint8_t *ciphertext, OUT uint8_t *shared_secret, | ||
| IN const uint8_t *public_key) | ||
| { | ||
| POSIX_BAIL(S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); | ||
|
Comment on lines
-112
to
+101
There was a problem hiding this comment. Maybe we should keep S2N_ERR_PQ_DISABLED and use that here? Since this is a breaking change, we might want a very specific error. You'll also need to update fewer tests. There was a problem hiding this comment.
In this case, something in s2n has called s2n's Kyber KEM API, and s2n doesn't have a Kyber libcrypto API that it can call (since the libcrypto s2n was compiled against doesn't support Kyber), so
I feel like There was a problem hiding this comment. I'm thinking about what you said above:
If we don't expect to ever actually hit this logic and to just fall back to classical, then S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API is fine. But if it's possible for us to break a customer and for them to see this error message related to that new failure, we should probably use a message that mentions PQ like S2N_ERR_NO_SUPPORTED_PQ_LIBCRYPTO_API. So it sounds like we're fine with S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API, because customers shouldn't receive any errors related to this change. |
||
| } | ||
|
|
||
| int s2n_kyber_evp_decapsulate(IN const struct s2n_kem *kem, OUT uint8_t *shared_secret, IN const uint8_t *ciphertext, | ||
| IN const uint8_t *secret_key) | ||
| { | ||
| POSIX_BAIL(S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API); | ||
| } | ||
|
|
||
| #endif | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep these options and just have them be no-ops? Does CMake warn or error out if you pass an option that doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my experience, cmake just warns about unrecognized options.
also, there may be cases when users link against AWS-LC but still don't want PQ enabled (admittedly, i can't think of a strong use-case for this off the top of my head). why remove these options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabling the
S2N_NO_PQflag causes the build to skip compiling the Kyber code in the pq-crypto directory of s2n. It's confusing to keep around a flag to skip compiling code that doesn't exist anymore.As far as I'm aware, the
S2N_NO_PQflag has only been enabled when it's been needed to keep s2n compiling on very old compiler versions that don't know about certain modern x86_64 instructions used in some of the Kyber assembly optimizations, and used to keep s2n compiling on some more obscure CPU architectures (MIPS, PowerPC, etc) that the NIST Kyber reference code doesn't compile for but that the AWS Common Runtime SDK targets. For both of these use cases (old compilers and obscure architectures), theS2N_NO_PQflag is no longer needed. Here's a link to when this flag was originally introduced to workaround build failures on MIPS platforms.We don't have equivalent flags to disable entire algorithms across the whole s2n-tls codebase (S2N_NO_RSA, S2N_NO_X25519, S2N_NO_CHACHAPOLY, etc). This PR is meant to bring Kyber more in line with how all other crypto algorithms are treated in s2n, and have the necessary logic that detects if the algorithm is supported by the libcrypto that s2n is compiled against.