Remove s2n's internal Kyber512 implementation, and rely on AWS-LC for Kyber support #4283

alexw91 · 2023-11-10T01:09:54Z

Resolved issues:

Now that AWS-LC has a Kyber implementation, we no longer need s2n to have it's own separate Kyber512 implementation.

Description of changes:

Removes s2n's internal Kyber512 implementation, and updates s2n to rely on AWS-LC's Kyber implementation.

Call-outs:

Does not update Rust bindings to account for missing pq-crypto directory. This change will be made in a follow up PR.

Testing:

s2n unit tests pass with both AWS-LC and Openssl 1.1.1 on my Ubuntu host.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

tests/integrationv2/common.py

camshaft · 2023-11-15T21:05:12Z

CMakeLists.txt

-option(S2N_NO_PQ "Disables all Post Quantum Crypto code. You likely want this
-for older compilers or uncommon platforms." OFF)
-option(S2N_NO_PQ_ASM "Turns off the ASM for PQ Crypto even if it's available for the toolchain.
-You likely want this on older compilers." OFF)


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?

in my experience, cmake just warns about unrecognized options.

CMake Warning: Manually-specified variables were not used by the project: S2N_FIPS S2N_INTERN_LIBCRYPTO S2N_NO_PQ -- Build files have been written to: /home/ubuntu/aws-lc/build

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?

Why remove these options?

Enabling the S2N_NO_PQ flag 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_PQ flag 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), the S2N_NO_PQ flag 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.

camshaft · 2023-11-15T21:06:28Z

bindings/rust/s2n-tls-sys/build.rs

+    // TODO: update rust bindings to handle no pq-crypto dir
+


I can do a separate PR to fix the rust build once this is merged.

Does not updating the bindings break anything?

No it shouldn't. If pq was enabled at all, we would fall back to the cmake build. The rust build change will basically just be deleting the special casing for that.

WillChilds-Klein · 2023-11-20T19:46:04Z

crypto/s2n_pq.c

+bool s2n_libcrypto_supports_kyber()
+{
+    /* S2N_LIBCRYPTO_SUPPORTS_KYBER will auto-detected and #defined if
+   * ./tests/features/S2N_LIBCRYPTO_SUPPORTS_KYBER.c returns 1 */


minor nit: hm, i think these checks are compile-time and don't execute the feature test or examine its output.

Yes that's correct. I added this comment since I was confused how the current CMake build would ever be able to enable this #define since the S2N_LIBCRYPTO_SUPPORTS_KYBER flag was removed from s2n's CMakeLists.txt file. But it turns out the logic was just moved into a feature detection tests in ./tests/features/*.

The issue is that this comment is misleading in a way that's tripped developers up before. "if returns 1" is incorrect. We don't run the feature tests, we just compile them. So if ./tests/features/S2N_LIBCRYPTO_SUPPORTS_KYBER.c compiles but would always return "-1", S2N_LIBCRYPTO_SUPPORTS_KYBER will still be defined.

WillChilds-Klein · 2023-11-20T20:05:44Z

tests/integrationv2/common.py

@@ -33,7 +33,7 @@ def pq_enabled():
    """
    Returns true or false to indicate whether PQ crypto is enabled in s2n
    """
-    return not (get_flag(S2N_NO_PQ, False) or get_flag(S2N_FIPS_MODE, False))
+    return "awslc" in get_flag(S2N_PROVIDER_VERSION)


this is true for newer versions of AWS-LC, but not versions < 1.4.0. should we include that version check here?

I don't think we test with any <1.4.0 versions in the integration tests. Not sure we need to include that.

ah, right. that makes sense.

CMakeLists.txt

lrstewart · 2023-11-20T20:34:24Z

crypto/s2n_kyber_evp.c

+#else /* If !S2N_LIBCRYPTO_SUPPORTS_KYBER, we won't have a Kyber impl so define relevant stubs here. */



Isn't this a breaking change? Wont' we break anyone using pq with a libcrypto other than awslc?
What kind of effect should this have on our version?

Isn't this a breaking change? Wont' we break anyone using pq with a libcrypto other than awslc?

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.

What kind of effect should this have on our version?

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 1.3.56 to 1.4.0 would be preferred.

tests/fuzz/allowed_coverage_failures.cfg

lrstewart · 2023-11-20T20:42:26Z

crypto/s2n_kyber_evp.c

-    POSIX_BAIL(S2N_ERR_UNIMPLEMENTED);
+    POSIX_BAIL(S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API);


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.

Maybe we should keep S2N_ERR_PQ_DISABLED and use that here?

S2N_ERR_PQ_DISABLED, to me, has always meant "the user who was compiling s2n-tls decided to disable the PQ code in the ./pq-crypto directory". Since the pq-crypto directory doesn't exist anymore, keeping around that error message seems confusing to me.

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 S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API seems like the best fit.

we might want a very specific error.

I feel like S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API is that very specific error. Or would you prefer that I create a S2N_ERR_NO_SUPPORTED_PQ_LIBCRYPTO_API?

I'm thinking about what you said above:

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.

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.

tests/unit/s2n_pq_kem_test.c

crypto/s2n_pq.c

Co-authored-by: Lindsay Stewart <[email protected]>

github-advanced-security bot found potential problems Nov 14, 2023

View reviewed changes

tests/integrationv2/common.py Fixed Show fixed Hide fixed

alexw91 force-pushed the remove-pq-crypto branch 5 times, most recently from b9befe2 to d3fd5c7 Compare November 15, 2023 20:38

camshaft reviewed Nov 15, 2023

View reviewed changes

alexw91 changed the title ~~[DRAFT] Remove Kyber512~~ Remove s2n's internal Kyber512 implementation, and rely on AWS-LC for Kyber support Nov 15, 2023

alexw91 marked this pull request as ready for review November 15, 2023 22:16

alexw91 requested a review from dougch as a code owner November 15, 2023 22:16

alexw91 marked this pull request as draft November 15, 2023 22:31

alexw91 changed the title ~~Remove s2n's internal Kyber512 implementation, and rely on AWS-LC for Kyber support~~ [DO NOT MERGE] Remove s2n's internal Kyber512 implementation, and rely on AWS-LC for Kyber support Nov 15, 2023

alexw91 changed the title ~~[DO NOT MERGE] Remove s2n's internal Kyber512 implementation, and rely on AWS-LC for Kyber support~~ Remove s2n's internal Kyber512 implementation, and rely on AWS-LC for Kyber support Nov 20, 2023

alexw91 marked this pull request as ready for review November 20, 2023 18:34

alexw91 force-pushed the remove-pq-crypto branch from d3fd5c7 to d08ad98 Compare November 20, 2023 18:38

camshaft approved these changes Nov 20, 2023

View reviewed changes

camshaft requested a review from lrstewart November 20, 2023 19:25

WillChilds-Klein reviewed Nov 20, 2023

View reviewed changes

WillChilds-Klein approved these changes Nov 20, 2023

View reviewed changes

lrstewart reviewed Nov 20, 2023

View reviewed changes

alexw91 added 4 commits November 20, 2023 15:27

Remove pq-crypto dir

b7bcd97

WIP

7a224a1

Remove S2N_ERR_PQ_DISABLED

60e2922

Address feedback

03e505b

alexw91 force-pushed the remove-pq-crypto branch from d08ad98 to 03e505b Compare November 20, 2023 23:28

alexw91 requested a review from lrstewart November 21, 2023 23:28

lrstewart reviewed Nov 22, 2023

View reviewed changes

crypto/s2n_pq.c Outdated Show resolved Hide resolved

Update crypto/s2n_pq.c

af4915b

Co-authored-by: Lindsay Stewart <[email protected]>

lrstewart approved these changes Nov 22, 2023

View reviewed changes

toidiu merged commit 8b10ecf into aws:main Nov 24, 2023
27 checks passed

BrewTestBot mentioned this pull request Dec 6, 2023

s2n 1.4.0 Homebrew/homebrew-core#156623

Merged

alexw91 mentioned this pull request Dec 21, 2023

Add PQ integration tests between s2n and AWS-LC's libssl #4267

Merged

This was referenced Jan 9, 2024

fix(bindings): remove optional cmake dependency #4347

Merged

Remove S2N_NO_PQ usage #4349

Closed

jmayclin mentioned this pull request Jun 12, 2024

Is sike_r1 being used inside s2n-tls or its removed #4593

Closed

lrstewart mentioned this pull request Aug 27, 2024

fix: Cleanup libcrypto errors #4733

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove s2n's internal Kyber512 implementation, and rely on AWS-LC for Kyber support #4283

Remove s2n's internal Kyber512 implementation, and rely on AWS-LC for Kyber support #4283

alexw91 commented Nov 10, 2023 •

edited

Loading

camshaft Nov 15, 2023

WillChilds-Klein Nov 15, 2023

alexw91 Nov 15, 2023 •

edited

Loading

camshaft Nov 15, 2023

lrstewart Nov 20, 2023

camshaft Nov 21, 2023

WillChilds-Klein Nov 20, 2023

alexw91 Nov 20, 2023

lrstewart Nov 22, 2023 •

edited

Loading

WillChilds-Klein Nov 20, 2023

camshaft Nov 20, 2023

WillChilds-Klein Nov 20, 2023

lrstewart Nov 20, 2023

alexw91 Nov 20, 2023

lrstewart Nov 20, 2023

alexw91 Nov 20, 2023

lrstewart Nov 22, 2023

		#else /* If !S2N_LIBCRYPTO_SUPPORTS_KYBER, we won't have a Kyber impl so define relevant stubs here. */

		POSIX_BAIL(S2N_ERR_UNIMPLEMENTED);
		POSIX_BAIL(S2N_ERR_NO_SUPPORTED_LIBCRYPTO_API);

Remove s2n's internal Kyber512 implementation, and rely on AWS-LC for Kyber support #4283

Remove s2n's internal Kyber512 implementation, and rely on AWS-LC for Kyber support #4283

Conversation

alexw91 commented Nov 10, 2023 • edited Loading

Resolved issues:

Description of changes:

Call-outs:

Testing:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexw91 Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrstewart Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexw91 commented Nov 10, 2023 •

edited

Loading

alexw91 Nov 15, 2023 •

edited

Loading

lrstewart Nov 22, 2023 •

edited

Loading