Migrate Kyber 512 to EVP KEM API #3853

WillChilds-Klein · 2023-02-24T20:27:56Z

Resolved issues:

This commit resolves CryptoAlg-1495.

Description of changes:

This commit does not implement any behavioral changes. It simply migrates from usage of AWS-LC's old kyber API to its newer, more generic KEM API.

New usage is based on the KEM API design document and header.
Now that we're on a stable KEM API, we remove the
S2N_AWSLC_KYBER_UNSTABLE build flag and always use the linked
libcrypto's Kyber implementation if available. This flag wasn't
previously specified in any of our CI scripts, meaning that
AWS-LC-backed kyber was previously uncovered in s2n's CI. This commit
ensures that coverage, and adds a unit test which asserts that if AWS-LC
is used as the backing libcrypto, it has the new Kyber 512 KEM API
available.

Call-outs:

n/a

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

I tested this locally (AL2) against AWS-LC at commit 161e747 (v1.5.0). See CI checks for full automated testing.

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

Yes, we rely on existing tests to assert behavioral consistency.

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

pq-crypto/s2n_kyber_512_evp.c

WillChilds-Klein · 2023-02-28T23:05:50Z

I checked whether we needed to update the CI's AWS-LC version under test and confirmed that the changes adding Kyber 512 Round 3 as well as those adding new KEM API are in the current version (1.4.0).

pq-crypto/s2n_kyber_512_evp.c

WillChilds-Klein · 2023-03-02T00:44:29Z

do we currently have test coverage of AWS-LC-backed Kyber? the S2N_AWSLC_KYBER_UNSTABLE cmake property is required to enable AWS-LC-backed kyber at build time, but it defaults to OFF and I don't see it used anywhere.

pq-crypto/s2n_kyber_512_evp.c

codebuild/bin/s2n_codebuild.sh

WillChilds-Klein · 2023-03-07T23:58:06Z

The s2nValgrindAwslc and s2nValgrindAwslcFips CI jobs are both failing the new s2n_pq_kem_kyber_available_awslc_test due to the outdated version of AWS-LC currently baked into the CI container lacking the new EVP KEM API. Here's a local s2n test run of these changes against AWS-LC v1.4.0:

$ cmake \
    -DCMAKE_PREFIX_PATH=${INSATLL_DIR} \
    -DCMAKE_INSTALL_PREFIX=${INSATLL_DIR} \
    -DCMAKE_VERBOSE_MAKEFILE=1 \
    ..
...
$ make -j $NPROC 
...
$ ctest -j $NPROC 
...
100% tests passed, 0 tests failed out of 236

Label Time Summary:
unit    = 286.58 sec*proc (236 tests)

Total Test time (real) =  49.42 sec

I don't think I have permissions to rebuild the CI docker image and update AWS-LC, so until that's done this PR is blocked.

tests/unit/s2n_pq_kem_kyber_available_awslc_test.c

pq-crypto/s2n_kyber_512_evp.c

lrstewart · 2023-03-10T00:39:12Z

The s2nValgrindAwslc and s2nValgrindAwslcFips CI jobs are both failing the new s2n_pq_kem_kyber_available_awslc_test due to the outdated version of AWS-LC currently baked into the CI container lacking the new EVP KEM API.

If some versions of awslc fail that test, then the test is wrong. The unit test shouldn't fail if someone tries to compile s2n with an old version of awslc. You may need to specify a version for awslc in that test.

pq-crypto/s2n_kyber_512_evp.c

tests/unit/s2n_pq_kem_test.c

lrstewart · 2023-03-10T19:28:54Z

tests/unit/s2n_pq_kem_test.c

+
+    /* If using non-FIPS AWS-LC >= v1.4.0 (API vers. 20), expect Kyber512 KEM from AWS-LC*/


You're very confident that this version is correct? Since this is just a sanity check and the feature probe actually turns on the feature, definitely err on the side of caution here: we never want this to fail, but it's fine / expected that it doesn't catch all environments where this is true.

yeah, i'm pretty sure this statement is correct. here's the 1.4.0 release at commit cfbc38b. you can see the KEM stuff here and the API version here.

CMakeLists.txt

goatgoose · 2023-03-15T22:50:25Z

tests/unit/s2n_pq_kem_test.c

+    if (s2n_libcrypto_is_awslc() && lc_vers >= 20 && !s2n_libcrypto_is_fips()) {
+        EXPECT_TRUE(s2n_libcrypto_supports_kyber_512());
+    } else {
+        EXPECT_FALSE(s2n_libcrypto_supports_kyber_512());


Is this EXPECT_FALSE check needed? If kyber is enabled when it shouldn't be, the build should fail, which means this check may not be needed in a test. I'm just worried that this test will start failing if kyber is added to AWSLC-FIPS. But maybe this isn't a concern!

it's not strictly needed, i suppose, but I like to use tests to tightly define expected behavior at a given point in time. AWS-LC runs a CI check building s2n and running its tests against AWS-LC PR changes, so we'll catch this on the AWS-LC side if/when Kyber is enabled in AWS-LC-FIPS. you shouldn't get any failing tests on the s2n-tls side of things.

New usage is based on [the KEM API design document][1] and [header][2]. Now that we're on a stable KEM API, we remove the S2N_AWSLC_KYBER_UNSTABLE build flag and always use the linked libcrypto's Kyber implementation if available. This flag wasn't previously specified in any of our CI scripts, meaning that AWS-LC-backed kyber was previously uncovered in s2n's CI. This commit ensures that coverage and updates the PQ KEM unit test to asserts that if (non-FIPS) AWS-LC is used as the backing libcrypto, it has the new Kyber 512 KEM API available. [1]: https://github.com/aws/aws-lc/blob/main/crypto/kem/README.md [2]: https://github.com/aws/aws-lc/blob/92c56fbc15f9bb43c4ff062c6c02f7991fd417f6/include/openssl/evp.h#L880 Check for other pre-processor symbol from AWS-LC

This reverts commit 10ec83a.

geedo0 suggested changes Feb 27, 2023

View reviewed changes

pq-crypto/s2n_kyber_512_evp.c Outdated Show resolved Hide resolved

WillChilds-Klein force-pushed the main branch 4 times, most recently from 705f275 to 4cce492 Compare February 28, 2023 22:21

WillChilds-Klein force-pushed the main branch from 4cce492 to 5b4583e Compare February 28, 2023 23:10

WillChilds-Klein requested review from lrstewart and geedo0 February 28, 2023 23:11

WillChilds-Klein marked this pull request as ready for review February 28, 2023 23:11

lrstewart reviewed Mar 1, 2023

View reviewed changes

pq-crypto/s2n_kyber_512_evp.c Outdated Show resolved Hide resolved

WillChilds-Klein force-pushed the main branch 5 times, most recently from 0a7ea42 to 5f11a79 Compare March 2, 2023 01:54

WillChilds-Klein requested a review from dougch as a code owner March 2, 2023 07:18

WillChilds-Klein force-pushed the main branch 4 times, most recently from 75906ee to 4be5511 Compare March 6, 2023 16:24

geedo0 suggested changes Mar 6, 2023

View reviewed changes

pq-crypto/s2n_kyber_512_evp.c Outdated Show resolved Hide resolved

codebuild/bin/s2n_codebuild.sh Outdated Show resolved Hide resolved

WillChilds-Klein force-pushed the main branch 2 times, most recently from e38ee8d to d3bdb16 Compare March 7, 2023 18:40

WillChilds-Klein requested review from geedo0 and lrstewart March 7, 2023 23:58

geedo0 approved these changes Mar 8, 2023

View reviewed changes

lrstewart reviewed Mar 8, 2023

View reviewed changes

tests/unit/s2n_pq_kem_kyber_available_awslc_test.c Outdated Show resolved Hide resolved

pq-crypto/s2n_kyber_512_evp.c Show resolved Hide resolved

WillChilds-Klein force-pushed the main branch 3 times, most recently from adaa2f9 to dbcfe56 Compare March 9, 2023 20:59

WillChilds-Klein force-pushed the main branch 2 times, most recently from 65fbc0c to 72a0d8f Compare March 10, 2023 17:35

lrstewart reviewed Mar 10, 2023

View reviewed changes

lrstewart approved these changes Mar 10, 2023

View reviewed changes

lrstewart mentioned this pull request Mar 10, 2023

Enforce that ENSURE and GUARD_OSSL use valid error codes #3873

Merged

WillChilds-Klein force-pushed the main branch 4 times, most recently from 2e576b6 to 0530044 Compare March 13, 2023 16:14

WillChilds-Klein requested review from lrstewart and removed request for dougch March 13, 2023 19:23

WillChilds-Klein force-pushed the main branch 3 times, most recently from c2ea814 to 037ed4a Compare March 15, 2023 15:49

lrstewart requested a review from goatgoose March 15, 2023 18:04

lrstewart reviewed Mar 15, 2023

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

goatgoose reviewed Mar 15, 2023

View reviewed changes

WillChilds-Klein requested a review from goatgoose March 16, 2023 00:39

WillChilds-Klein force-pushed the main branch from 037ed4a to 9e6f4a8 Compare March 16, 2023 02:47

goatgoose approved these changes Mar 16, 2023

View reviewed changes

lrstewart merged commit 10ec83a into aws:main Mar 16, 2023

toidiu added a commit to toidiu/s2n-tls that referenced this pull request Mar 22, 2023

Revert "Migrate Kyber 512 to EVP KEM API (aws#3853)"

73de4b5

This reverts commit 10ec83a.

This was referenced Mar 22, 2023

fix: remove broken check in test #3901

Merged

Fix Kyber512 KEM feature probe check #3902

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Kyber 512 to EVP KEM API #3853

Migrate Kyber 512 to EVP KEM API #3853

WillChilds-Klein commented Feb 24, 2023 •

edited

Loading

WillChilds-Klein commented Feb 28, 2023

WillChilds-Klein commented Mar 2, 2023

WillChilds-Klein commented Mar 7, 2023

lrstewart commented Mar 10, 2023

lrstewart Mar 10, 2023

WillChilds-Klein Mar 10, 2023

goatgoose Mar 15, 2023

WillChilds-Klein Mar 15, 2023 •

edited

Loading


		/* If using non-FIPS AWS-LC >= v1.4.0 (API vers. 20), expect Kyber512 KEM from AWS-LC*/

Migrate Kyber 512 to EVP KEM API #3853

Migrate Kyber 512 to EVP KEM API #3853

Conversation

WillChilds-Klein commented Feb 24, 2023 • edited Loading

Resolved issues:

Description of changes:

Call-outs:

Testing:

WillChilds-Klein commented Feb 28, 2023

WillChilds-Klein commented Mar 2, 2023

WillChilds-Klein commented Mar 7, 2023

lrstewart commented Mar 10, 2023

lrstewart Mar 10, 2023

Choose a reason for hiding this comment

WillChilds-Klein Mar 10, 2023

Choose a reason for hiding this comment

goatgoose Mar 15, 2023

Choose a reason for hiding this comment

WillChilds-Klein Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

WillChilds-Klein commented Feb 24, 2023 •

edited

Loading

WillChilds-Klein Mar 15, 2023 •

edited

Loading