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

refactor: add alternative EVP signing method #5141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Feb 21, 2025

Release Summary:

Resolved issues:

related to #5105

Description of changes:

Final required change for #5105. This adds the new non-FIPS-140-3 EVP signing method and switches all libcryptos except awslc-fips to use it.

The state of the library after this change matches the desired state described in #5105, which is:

Libcrypto Hash method Signing Method
openssl-1.0.2 EVP EVP
awslc-fips EVP EVP-FIPS-140-3
openssl-3-fips EVP EVP
other EVP EVP

Testing:

I added s2n_evp_signing_test to the Openssl3fipsWIP build. It's nice to note that after this change, only 12/274 tests are failing for openssl-3.0-fips. A quick spot check doesn't suggest any of them are related to this change.

I also added known-value tests to s2n_evp_signing_test. That's important for a couple reasons:

  1. openssl-3.0-fips can't run the comparisons with the legacy signing methods because it doesn't support the legacy signing methods.
  2. I plan to remove the legacy signing methods, after which point no libcrypto will run the comparisons with the legacy signing methods
  3. There are subtle mistakes that aren't caught by self-talk tests. For example, I initially wasn't setting the EVP_MD on the pkey context, which is required to get the padding right when doing RSA PKCS1. Since neither the sign nor the verify methods were setting the EVP_MD, they were both wrong in the same way.

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

@lrstewart lrstewart mentioned this pull request Feb 21, 2025
9 tasks
@github-actions github-actions bot added the s2n-core team label Feb 21, 2025
Comment on lines +109 to +110
POSIX_GUARD(s2n_pkey_sign(priv, S2N_SIGNATURE_RSA_PSS_PSS, &sign_hash, &signature_data));
POSIX_GUARD(s2n_pkey_verify(pub, S2N_SIGNATURE_RSA_PSS_PSS, &verify_hash, &signature_data));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, s2n_evp_signing_test can't load the RSA-PSS certificate it needs for testing.

@lrstewart lrstewart marked this pull request as ready for review February 21, 2025 21:46
@lrstewart lrstewart requested a review from dougch as a code owner February 21, 2025 21:46
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.

1 participant