s2n bignum import method change#2324
Merged
torben-hansen merged 15 commits intoaws:mainfrom Apr 29, 2025
Merged
Conversation
torben-hansen
commented
Apr 10, 2025
00565ef to
6f2ba9a
Compare
Codecov Report✅ All 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
Contributor
dkostic
left a comment
There was a problem hiding this comment.
lgtm.
Nit: do we need to import CODE_OF_CONDUCT.md, CONTRINUTING.md, non_ct_functions.txt?
samuel40791765
previously approved these changes
Apr 25, 2025
|
|
||
| OPENSSL_INLINE int exponentiation_use_s2n_bignum(void) { return 1; } | ||
| OPENSSL_INLINE int exponentiation_use_s2n_bignum(void) { | ||
| return CRYPTO_is_NEON_capable(); |
Contributor
There was a problem hiding this comment.
is this moving the capability checks in crypto/fipsmodule/bn/montgomery.c to here instead?
Contributor
Author
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.
61db4e7
34766bd to
61db4e7
Compare
samuel40791765
approved these changes
Apr 28, 2025
skmcgrail
approved these changes
Apr 28, 2025
Contributor
|
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.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.