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

test: add minimal openssl-3.0-fips test #5081

Merged
merged 4 commits into from
Feb 7, 2025

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Feb 4, 2025

Release Summary:

Resolved issues:

early version of #5036

Description of changes:

Before I start pushing out fixes to get s2n-tls to build with openssl-3.0-fips, we need to be able to test with openssl-3.0-fips. I would rather not push out one huge PR with all the openssl-3.0-fips changes, but that will mean that some of the tests will still be failing in initial PRs.

I think the solution here is to add a minimal openssl-3.0-fips test. Initially, I'm only enabling two unit tests, s2n_build_test and s2n_fips_test. As more functionality is updated, I will either add more tests or (more likely) switch from the "include" regex (-R) to the "exclude" regex (-E).

So this PR adds the buildspec for that test and minimally enables s2n-tls to detect openssl-3.0-fips as fips.

Testing:

I created a new Codebuild job with this buildspec. Here is a passing run: https://us-west-2.console.aws.amazon.com/codesuite/codebuild/024603541914/projects/Openssl3fipsWIP/build/Openssl3fipsWIP%3Acb296aa7-a6e7-41ef-ba17-b60d843f76c7?region=us-west-2
After this PR is merged, I will update that job to trigger on github events.

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

@github-actions github-actions bot added the s2n-core team label Feb 4, 2025
@lrstewart lrstewart force-pushed the openssl3fips_test branch 4 times, most recently from b145415 to 87462ad Compare February 5, 2025 02:51
Comment on lines -153 to +152
* after its fork from BoringSSL. */
* after its fork from BoringSSL.
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm removing a sneaky tab character here :/ I'm surprised clang-format doesn't flag tabs.

Comment on lines +54 to +56
bool s2n_libcrypto_is_openssl_fips(void);
bool s2n_libcrypto_is_awslc();
bool s2n_libcrypto_is_awslc_fips(void);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These methods are just for convenience, but since we generally talk about the fips version of libcryptos like they're separate libcryptos, I think they're helpful and simplify common conditionals.

@lrstewart lrstewart marked this pull request as ready for review February 5, 2025 06:41
@lrstewart lrstewart requested a review from dougch as a code owner February 5, 2025 06:41
@lrstewart lrstewart requested a review from goatgoose February 5, 2025 06:42
Comment on lines +33 to +34
# openssl3fips is still a work-in-progress. Not all tests pass.
- make -C build test -- ARGS="-R 's2n_build_test|s2n_fips_test'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea on the strategy to review this! I was worried we would need a feature branch or something.

tests/unit/s2n_fips_test.c Show resolved Hide resolved
Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

small question, lgtm otherwise

@@ -82,24 +83,24 @@ S2N_RESULT s2n_check_supported_libcrypto(const char *s2n_libcrypto)
{ .libcrypto = "openssl-1.0.2", .is_openssl = true },
{ .libcrypto = "openssl-1.1.1", .is_openssl = true },
{ .libcrypto = "openssl-3.0", .is_openssl = true },
{ .libcrypto = "openssl-3.0-fips", .is_openssl = true, .is_opensslfips = true },
Copy link
Contributor

Choose a reason for hiding this comment

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

Is is_opensslfips set to false anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When initializing a struct this way in C, any field not specified is "0", which is false. So everywhere that doesn't set is_openssl_fips gets "false".

-DASAN=ON \
-DUBSAN=ON
- cmake --build ./build -- -j $(nproc)
post_build:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for separating build from test... we'll need to be more pedantic about this going forward...

@lrstewart lrstewart enabled auto-merge February 6, 2025 01:03
@lrstewart
Copy link
Contributor Author

lrstewart commented Feb 6, 2025

My local branch was super behind main and didn't have f146f06 😅 I fixed the build test to also work for openssl-1.0.2-fips.

@lrstewart lrstewart requested a review from goatgoose February 6, 2025 05:50
@lrstewart lrstewart added this pull request to the merge queue Feb 6, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 6, 2025
@lrstewart lrstewart added this pull request to the merge queue Feb 6, 2025
Merged via the queue into aws:main with commit f129bf7 Feb 7, 2025
45 checks passed
@lrstewart lrstewart deleted the openssl3fips_test branch February 7, 2025 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants