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

Add integration script and CI for ruby 3.1 and 3.2 #1563

Merged
merged 15 commits into from
Dec 16, 2024

Conversation

samuel40791765
Copy link
Contributor

@samuel40791765 samuel40791765 commented Apr 29, 2024

Description of changes:

We've finalized our support for Ruby 3.1 and 3.2. This adds the patch and integration CI to test against it.

Ruby 3.1 patch contents are as following:

  1. Slight logic to properly detect AWS-LC in FIPS mode.
  2. Differences in emitted error messages
  3. During signature verification, an invalid ASN.1 input raises an error in Ruby/OpenSSL and an actual verification failure returns false. AWS-LC simply returns false for all cases.
  4. OpenSSL allows invalid DH parameters to be parsed successfully and invalidates it in a subsequent call to params_ok?. AWS-LC simply disallows invalid parameters to be parsed.
  5. AWS-LC does not support the serialization of custom/explicit curves. Explicit curves are highly impractical to validate from a security standpoint and have been the source of many CVEs. See the following issues for more details on why explicit curves are discouraged.
  6. AWS-LC does not support BN::CONSTTIME. See 0a211df for more details.
  7. AWS-LC does not support DHE ciphersuites in SSL connections. 5 tests have been adjusted accordingly.
  8. Changes in pkcs12 are pieces of ruby/ruby@63e9eaa in upstream Ruby. This test was testing with the wrong number of parameters. AWS-LC does not support the "old MSIE extension" mentioned in the commit.
  9. Changes in test/openssl/test_pkey_rsa.rb are pieces of ruby/ruby@2e5680d in upstream Ruby. It accounts for RSA operations disallowed in FIPS mode in Ruby's RSA tests. The commit diff is a bit too large to muddle with the changes in this CR. It's also fairly recent and not directly applicable to Ruby 3.1's version of the file, so I've only taken pieces that ensure we're not losing coverage.

Ruby 3.2 patch contents are nearly identical to the points mentioned above. Only additional thing to note is AWS-LC does key checks while parsing EC Keys and disallows invalid keys to be parsed. This is similar to the DH params discrepancy described above in point 4.

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.

@samuel40791765 samuel40791765 force-pushed the ruby-integration branch 2 times, most recently from aa00d22 to 03b8ff6 Compare April 30, 2024 19:07
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein left a comment

Choose a reason for hiding this comment

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

(sorry for lurking on a draft)

tests/ci/integration/run_ruby_integration.sh Outdated Show resolved Hide resolved
tests/ci/integration/run_ruby_integration.sh Outdated Show resolved Hide resolved
@samuel40791765 samuel40791765 force-pushed the ruby-integration branch 2 times, most recently from 771ff8c to 0d9098b Compare September 4, 2024 23:12
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.75%. Comparing base (850af98) to head (938d980).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1563      +/-   ##
==========================================
- Coverage   78.76%   78.75%   -0.01%     
==========================================
  Files         598      598              
  Lines      103683   103683              
  Branches    14742    14742              
==========================================
- Hits        81666    81657       -9     
- Misses      21364    21372       +8     
- Partials      653      654       +1     

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

@samuel40791765 samuel40791765 force-pushed the ruby-integration branch 3 times, most recently from 6cf2b3d to 365401c Compare September 12, 2024 19:55
@samuel40791765 samuel40791765 force-pushed the ruby-integration branch 2 times, most recently from f69869d to 26b722a Compare September 18, 2024 23:51
@samuel40791765 samuel40791765 force-pushed the ruby-integration branch 2 times, most recently from 85edc6b to 6bacec1 Compare October 4, 2024 17:37
@samuel40791765 samuel40791765 force-pushed the ruby-integration branch 6 times, most recently from 44b7edb to 95cd424 Compare November 1, 2024 22:10
@samuel40791765 samuel40791765 force-pushed the ruby-integration branch 6 times, most recently from 08773c6 to 66b4176 Compare November 12, 2024 01:32
@samuel40791765 samuel40791765 force-pushed the ruby-integration branch 4 times, most recently from 7f2effd to b45eca3 Compare December 7, 2024 00:57
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein left a comment

Choose a reason for hiding this comment

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

reviewed everything but the patch files this pass. will review those in the next pass.

tests/ci/common_posix_setup.sh Show resolved Hide resolved
tests/ci/integration/run_ruby_integration.sh Show resolved Hide resolved
tests/ci/integration/run_ruby_integration.sh Show resolved Hide resolved
./install/bin/ruby -e 'require "openssl"; puts OpenSSL::OPENSSL_VERSION' | grep -q "AWS-LC" && echo "AWS-LC found!" || exit 1

#TODO: add more relevant tests here
make test-all TESTS="test/openssl/*.rb"
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be misunderstanding this, but from this PR's most recent CI run, it looks like a different executable than ./install/bin/ruby is used to execute the tests, namely miniruby:


test-all
  Run options: 
    --seed=24196
    "--ruby=./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems"
    --excludes-dir=./test/excludes
    --name=!/memory_leak/

could we patch a test into the OpenSSL module unit tests that asserts OpenSSL::OPENSSL_VERSION.include? 'AWS-LC'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a test, but we do have a version detection mechanism in the patch:

+ def aws_lc?(major = nil, minor = nil, fix = nil)
+    version = OpenSSL::OPENSSL_VERSION.scan(/AWS-LC (\d+)\.(\d+)\.(\d+).*/)[0]
+    return false unless version
+    !major || (version.map(&:to_i) <=> [major, minor, fix]) >= 0
+  end

It's used throughout the tests to skip across various AWS-LC specific discrepancies. I'm hesitant to add a specific test that asserts AWS-LC. Ultimate end goal is to run this patch down to 0 if upstream is willing to take us. I also think the various bash checks we do and the mentioned version detection should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

miniruby appears to be minimal lighter version of ruby used in their build. The ruby binary we're asserting in this script is the actual ruby binary that the general consumer uses.

Copy link
Contributor

Choose a reason for hiding this comment

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

miniruby appears to be minimal lighter version of ruby used in their build

right. i think we need some way to assert that the miniruby used to run the tests is actually linked against AWS-LC. otherwise, we might be testing against an OpenSSL-backed miniruby, which is meaningless to us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an assertion for miniruby

.github/workflows/integrations.yml Outdated Show resolved Hide resolved
tests/ci/integration/run_ruby_integration.sh Outdated Show resolved Hide resolved
tests/ci/integration/run_ruby_integration.sh Outdated Show resolved Hide resolved
@samuel40791765 samuel40791765 force-pushed the ruby-integration branch 2 times, most recently from edaae75 to 4b9f6f4 Compare December 12, 2024 01:44
justsmth
justsmth previously approved these changes Dec 12, 2024
@samuel40791765 samuel40791765 merged commit 57133c0 into aws:main Dec 16, 2024
125 of 126 checks passed
@samuel40791765 samuel40791765 deleted the ruby-integration branch December 16, 2024 20:04
samuel40791765 added a commit that referenced this pull request Dec 31, 2024
Follow up from #1563  where we add more test coverage for the
Ruby/OpenSSL gem.

* ruby_release_backport is for any commits that are already on
   the main branch and are required for some tests on older releases
   to pass through.
* ruby_patch_common is for commits that all branches should
   need for additional tests to pass. These are outside of the
   ruby/openssl gem boundary, so I chose to consolidate the logic in a
   separate patch. Patches in this folder would be submitted to their
   respective repos instead of https://github.com/ruby/openssl.

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants