Cross library PQ interop test with s2n-tls #2138

chockalingamc · 2025-01-24T15:57:31Z

Description of changes:

Adding cross library PQ interop test with s2n-tls

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

codecov-commenter · 2025-01-24T16:18:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.96%. Comparing base (39a4383) to head (cf1df0c).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2138   +/-   ##
=======================================
  Coverage   78.95%   78.96%           
=======================================
  Files         611      611           
  Lines      105522   105522           
  Branches    14946    14943    -3     
=======================================
+ Hits        83317    83323    +6     
+ Misses      21553    21547    -6     
  Partials      652      652

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

torben-hansen · 2025-01-24T18:46:59Z

a s2n-tls integration script exists: https://github.com/aws/aws-lc/blob/main/tests/ci/integration/run_s2n_integration.sh
Should add any new tests to that IMO.

geedo0 · 2025-01-28T18:43:53Z

a s2n-tls integration script exists: main/tests/ci/integration/run_s2n_integration.sh
Should add any new tests to that IMO.

The intent of this work was to add a test that can test PQ-TLS interop between AWS-LC's libssl and different PQ-TLS providers of which S2N is one of several. That said, the naming of the script in this PR should be renamed to reflect that generic intent.

geedo0

Add/augment a YAML file inside .github/workflows to actually run this test. Assert that the test actually succeeds in the checks.

geedo0 · 2025-01-28T18:51:40Z

tests/ci/run_s2n_interop_test.sh

+# clone aws-lc
+git clone --depth 1 --branch "${lc_branch}" "${lc_url}" "${scratch_folder}/s2n-tls/aws-lc"
+
+# build aws-lc
+echo "building aws-lc"
+cd "${scratch_folder}/s2n-tls/aws-lc"
+cmake -GNinja -B build
+ninja -C build
+cmake --install build --prefix install


Use the run_build function to build AWS-LC from the source corresponding to the request and put the relevant artifacts in the conventional locations.

geedo0 · 2025-01-28T18:52:40Z

tests/ci/run_s2n_interop_test.sh

+cd "${scratch_folder}/s2n-tls"
+cmake . -Bbuild-with-lc \
+  -DCMAKE_BUILD_TYPE=Release \
+  -DCMAKE_PREFIX_PATH=aws-lc/install


Update this to use the correct LC location after making the change to how it gets built.

geedo0 · 2025-01-28T18:54:38Z

tests/ci/run_s2n_interop_test.sh

Rename this to be more generic across PQ-TLS implementations. e.g. run_pq_tls_interop_test.sh.

geedo0 · 2025-01-28T18:56:50Z

tests/ci/run_s2n_interop_test.sh

+  -DCMAKE_BUILD_TYPE=Release \
+  -DCMAKE_PREFIX_PATH=aws-lc/install
+cmake --build build-with-lc
+


Please refactor this to loop across different TLS groups to cut down on the repetition.

for group in X25519MLKEM768 SecP256r1MLKEM768; do # lc to s2n # s2n to lc done

geedo0 · 2025-01-28T18:57:14Z

tests/ci/run_s2n_interop_test.sh

+
+# handshake test 1 - aws-lc bssl server with s2n-tls s2nc client for X25519MLKEM768:X25519
+cd "${scratch_folder}/s2n-tls"
+./aws-lc/build/tool/bssl s_server -curves X25519MLKEM768:X25519 -accept 45000 -debug &> s_server_out &


Don't test against the purely classical x25519 group.

geedo0 · 2025-01-28T19:02:07Z

tests/ci/run_s2n_interop_test.sh

+  -DCMAKE_PREFIX_PATH=aws-lc/install
+cmake --build build-with-lc
+
+# handshake test 1 - aws-lc bssl server with s2n-tls s2nc client for X25519MLKEM768:X25519


Add a comment explaining how all this works. It's not immediately apparent what all these workarounds do. e.g. the sleep is there to allow the server time to listen on the socket.

andrewhop · 2025-01-28T22:31:15Z

Add/augment a YAML file inside .github/workflows to actually run this test. Assert that the test actually succeeds in the checks.

Our preference is to put this in the codebuild integration target like https://github.com/aws/aws-lc/blob/main/tests/ci/cdk/cdk/codebuild/github_ci_integration_omnibus.yaml#L9-L17

andrewhop · 2025-01-28T22:31:56Z

tests/ci/run_s2n_interop_test.sh

+git clone --depth 1 --branch "${s2n_branch}" "${s2n_url}" "${scratch_folder}/s2n-tls"
+
+# clone aws-lc
+git clone --depth 1 --branch "${lc_branch}" "${lc_url}" "${scratch_folder}/s2n-tls/aws-lc"


This shouldn't need to clone AWS-LC since it will already be there when run locally or run in codebuild.

chockalingamc · 2025-01-28T22:39:02Z

Add/augment a YAML file inside .github/workflows to actually run this test. Assert that the test actually succeeds in the checks.

Our preference is to put this in the codebuild integration target like https://github.com/aws/aws-lc/blob/main/tests/ci/cdk/cdk/codebuild/github_ci_integration_omnibus.yaml#L9-L17

All targets in that yaml file are for tests/ci/integration/* test scripts. This test is interop testing for aws-lc with different libraries and resides in tests/ci/. The .github/workflows/misc-test.yaml seemed like an appropriate place for this.

andrewhop · 2025-01-28T22:51:07Z

Add/augment a YAML file inside .github/workflows to actually run this test. Assert that the test actually succeeds in the checks.

Our preference is to put this in the codebuild integration target like https://github.com/aws/aws-lc/blob/main/tests/ci/cdk/cdk/codebuild/github_ci_integration_omnibus.yaml#L9-L17

All targets in that yaml file are for tests/ci/integration/* test scripts. This test is interop testing for aws-lc with different libraries and resides in tests/ci/. The .github/workflows/misc-test.yaml seemed like an appropriate place for this.

I'd argue an interop test is an integration test and we could move the bash file. Either way this shouldn't run in the GitHub actions and should move to CodeBuild general or CodeBuild integration target.

geedo0 · 2025-01-30T19:12:58Z

tests/ci/cdk/cdk/codebuild/github_ci_integration_omnibus.yaml

+        variables:
+          AWS_LC_CI_TARGET: "tests/ci/integration/run_pq_tls_integration.sh"
+
+    - identifier: pq_tls_integration_aarch


I'll defer to the LC team, but I don't think we need to cover both ARM and Intel for this test. This is purely application layer stuff so the CPU arch really doesn't matter.

Agreed, we're a bit inconsistent about that for other projects but just one platform is fine.

geedo0 · 2025-01-30T19:13:49Z

tests/ci/integration/run_pq_tls_integration.sh

+S2N_TLS_SRC_FOLDER="${SCRATCH_FOLDER}/s2n-tls"
+S2N_TLS_BUILD_FOLDER="${SCRATCH_FOLDER}/s2n-tls-build"
+
+# init setup


unnecessary comment.

andrewhop

Checked the build and it looks good! It also runs in 10 minutes which is great and using BUILD_GENERAL1_SMALL makes sense.

andrewhop · 2025-01-30T22:55:47Z

tests/ci/integration/run_pq_tls_integration.sh

+  cd "$S2N_TLS_BUILD_FOLDER"
+  ./bin/s2nc -c default_pq -i localhost 45000 &> s2nc_out
+  wait $S_PID || true
+  grep "libcrypto" s2nc_out | grep "AWS-LC"


I trust the greps but it would be nice to see the output here and down below:

Suggested change

grep "libcrypto" s2nc_out | grep "AWS-LC"

echo $s_server_out

echo $s2nc_out

grep "libcrypto" s2nc_out | grep "AWS-LC"

andrewhop · 2025-01-30T22:57:29Z

tests/ci/cdk/cdk/codebuild/github_ci_integration_omnibus.yaml

+        variables:
+          AWS_LC_CI_TARGET: "tests/ci/integration/run_pq_tls_integration.sh"
+
+    - identifier: pq_tls_integration_aarch


Agreed, we're a bit inconsistent about that for other projects but just one platform is fine.

Chocka Chidambaram added 2 commits January 24, 2025 00:53

Add cross library PQ interop test with s2n-tls

a1f19ed

wait on background server process to finish

88b28c8

chockalingamc requested a review from a team as a code owner January 24, 2025 15:57

geedo0 suggested changes Jan 28, 2025

View reviewed changes

andrewhop reviewed Jan 28, 2025

View reviewed changes

chockalingamc and others added 4 commits January 29, 2025 13:10

Merge branch 'aws:main' into main

4911902

Merge remote-tracking branch 'upstream/main'

b5f9df5

refactoring based on PR discussions

d2b8124

improve s2n build performance with ninja and parallel threads

cf1df0c

geedo0 reviewed Jan 30, 2025

View reviewed changes

andrewhop reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross library PQ interop test with s2n-tls #2138

Cross library PQ interop test with s2n-tls #2138

chockalingamc commented Jan 24, 2025

codecov-commenter commented Jan 24, 2025 •

edited

Loading

torben-hansen commented Jan 24, 2025

geedo0 commented Jan 28, 2025

geedo0 left a comment

geedo0 Jan 28, 2025

geedo0 Jan 28, 2025

geedo0 Jan 28, 2025

geedo0 Jan 28, 2025

geedo0 Jan 28, 2025

geedo0 Jan 28, 2025

andrewhop commented Jan 28, 2025

andrewhop Jan 28, 2025

chockalingamc commented Jan 28, 2025

andrewhop commented Jan 28, 2025

geedo0 Jan 30, 2025

andrewhop Jan 30, 2025

geedo0 Jan 30, 2025

andrewhop left a comment

andrewhop Jan 30, 2025

andrewhop Jan 30, 2025

Cross library PQ interop test with s2n-tls #2138

Are you sure you want to change the base?

Cross library PQ interop test with s2n-tls #2138

Conversation

chockalingamc commented Jan 24, 2025

Description of changes:

codecov-commenter commented Jan 24, 2025 • edited Loading

Codecov Report

torben-hansen commented Jan 24, 2025

geedo0 commented Jan 28, 2025

geedo0 left a 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

andrewhop commented Jan 28, 2025

Choose a reason for hiding this comment

chockalingamc commented Jan 28, 2025

andrewhop commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewhop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 24, 2025 •

edited

Loading