s2n bignum import method change #2324
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
torben-hansen
commented
Apr 10, 2025
torben-hansen
force-pushed
the
s2n_bignum_import
branch
from
00565ef to
6f2ba9a
Compare
torben-hansen
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2324 +/- ##
==========================================
- Coverage 78.75% 78.75% -0.01%
==========================================
Files 620 620
Lines 107958 107958
Branches 15329 15331 +2
==========================================
- Hits 85026 85022 -4
- Misses 22275 22279 +4
Partials 657 657 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dkostic
previously approved these changes
Apr 22, 2025
samuel40791765
previously approved these changes
Apr 25, 2025
|
|
||
| #define BN_EXPONENTIATION_S2N_BIGNUM_CAPABLE 1 | ||
|
|
||
| OPENSSL_INLINE int exponentiation_use_s2n_bignum(void) { return 1; } | ||
| OPENSSL_INLINE int exponentiation_use_s2n_bignum(void) { | ||
| return CRYPTO_is_NEON_capable(); |
There was a problem hiding this comment.
is this moving the capability checks in crypto/fipsmodule/bn/montgomery.c to here instead?
There was a problem hiding this comment.
Scratch that. They are guarding two different things :)
torben-hansen
added a commit
to aws/aws-lc-rs
that referenced
this pull request
Apr 28, 2025
…ream AWS-LC change #2324 (#792) aws/aws-lc#2324 implements a new import method for s2n-bignum upstream in AWS-LC. Among other things it introduces a new folder structure. This messes with assumptions for the aws-lc-rs prefix build, when not using cmake. aws-lc-rs has hard assumptions on the folder structure to add the correct include search paths. Now s2n-bignum source in upstream AWS-LC is located under third_party/s2n-bignum/s2n-bignum-imported. Note third_party/s2-bignum contains the AWS-LC-specific header files. Hence we keep the existing search path.
torben-hansen
dismissed stale reviews from samuel40791765 and dkostic
via
61db4e7
torben-hansen
force-pushed
the
s2n_bignum_import
branch
from
34766bd to
61db4e7
Compare
samuel40791765
approved these changes
Apr 28, 2025
skmcgrail
approved these changes
Apr 28, 2025
|
I think we can now remove the special processing that we do for these assembly files. I'm wondering whether that might also fix the |
This was referenced Apr 29, 2025
Merged
justsmth
added a commit
that referenced
this pull request
Apr 30, 2025
### Description of changes: Bump version to 1.50.1. (Needed so that `aws-lc-sys` can to pick up latest s2n-bignum changes.) ### Call-outs: #### What's Changed * Fix GCC 4.8 docker img; Also w/ GCC 7.5 by @justsmth in #2344 * Fix LibRdKafka CI by @smittals2 in #2372 * Expand .clang-tidy configuration by @justsmth in #2356 * nginx-1.28.0 aws-lc-nginx.patch by @robvanoostenrijk in #2373 * s2n bignum import method change by @torben-hansen in #2324 * Fix a theoretical overflow in BIO_printf by @justsmth in #2369 * Fix tpm2-tss integration test by @justsmth in #2370 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
This modifies how s2n-bignum is imported. Firstly, it axes the subtree. Secondly, it implements an import script
import.shthat will do a simple source import from upstream s2n-bignum repo. Finally, it modies the aws-lc-specific header file.This is motivated by subtree being a maintenance burden and the need for upstream testing of aws-lc+s2n-bignum code changes before import time.
Obviously, this will increase the size of AWS-LC source because we don't use everything from s2n-bignum currently, and the repo contains a bunch of code-duplication.
Only files we need from s2n-bignum are code from
include,arm, andx86_att. All other folders are not imported.Call-outs:
The aws-lc-specific header file should not be where it currently is. It's unrelated to the third-party source code and is more of an AWS-LC implementation detail.
This PR is not ready for merge yet, because the upstream must first fix all the non-
constparameters that's currently assumed by AWS-LCSome code-changes were needed in the s2n-bignum powered montgomery implementation. Because en upstream change modified file paths, file names, function names, and removed some functions.
Testing:
All relevant code should already be tested as part of CI.
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.