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

Bring in testing changes from upstream commit 5ee4e95 #2048

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

WillChilds-Klein
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein commented Dec 9, 2024

Issues:

Resolves i/CryptoAlg-2805

Description of changes:

Upstream commit 5ee4e95's source changes are almost identical to our
addition of BIO_FP_TEXT in PR 1153. The only difference in behavior is
that we will call _setmode w/ _O_BINARY on windows when no flags are
specified, where BoringSSL will simply no-op and leave the underlying file's
mode alone.

Upstream's commit has some nice tests, though, so we crib those and change a
single line of expectation to account for our difference in behavior.

Open Question to Reviewers

In the interests of maximizing compatibility, I initially retained
our/OpenSSL's behavior of setting _O_BINARY mode on the file when
BIO_FP_TEXT is not set
. It's possible that downstream consumers have worked
around this behavior and have come to expect it. BoringSSL's behavior better
honors the (likely) intent of the caller.

Should we conform to BoringSSL's behavior and (marginally) risk downstream
compatibility issues or stick with our/OpenSSL's current behavior?

Testing:

  • 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.

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.77%. Comparing base (b649c44) to head (0dab716).
Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2048   +/-   ##
=======================================
  Coverage   78.77%   78.77%           
=======================================
  Files         598      598           
  Lines      103667   103716   +49     
  Branches    14738    14742    +4     
=======================================
+ Hits        81659    81699   +40     
- Misses      21355    21365   +10     
+ Partials      653      652    -1     

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

@WillChilds-Klein WillChilds-Klein marked this pull request as ready for review December 11, 2024 15:01
@WillChilds-Klein WillChilds-Klein requested a review from a team as a code owner December 11, 2024 15:01
crypto/bio/bio_test.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@nebeid nebeid left a comment

Choose a reason for hiding this comment

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

I'm leaning towards being compatible with OpenSSL as it's also not a change in our current behaviour, so it's less risky.

@nebeid nebeid requested a review from andrewhop December 11, 2024 16:51
Copy link
Contributor

@andrewhop andrewhop left a comment

Choose a reason for hiding this comment

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

Initial impression everything looks good in the test. I'm waiting to hear back from any Windows customers before picking a side for default behavior.

@WillChilds-Klein WillChilds-Klein merged commit 637b5d2 into aws:main Dec 13, 2024
121 of 124 checks passed
@WillChilds-Klein WillChilds-Klein deleted the bssl-5ee4e95 branch December 16, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants