Skip to content

Audit openssl requiring code#3816

Merged
deivid-rodriguez merged 17 commits intomasterfrom
openssl_require
Jul 19, 2020
Merged

Audit openssl requiring code#3816
deivid-rodriguez merged 17 commits intomasterfrom
openssl_require

Conversation

@deivid-rodriguez
Copy link
Contributor

Description:

This PR reviews all code requiring openssl in both repos, and centralizes it as much as possible.

As a result, this PR fixes #3173, and closes #3059.

It also fixes https://bugs.ruby-lang.org/issues/16475.

I added a small test (just run gem list bundler) to make sure that bare functionality of rubygems still works without openssl.

Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

@bundlerbot bundlerbot added Bundler CI CI related issues RubyGems labels Jul 11, 2020
@deivid-rodriguez deivid-rodriguez force-pushed the openssl_require branch 3 times, most recently from 98c70b1 to d55c909 Compare July 13, 2020 09:24
@hsbt
Copy link
Member

hsbt commented Jul 13, 2020

This pull-request is useful.

But I'm still not sure why we should support non-openssl environment. Is there real use-case it?

@deivid-rodriguez
Copy link
Contributor Author

Looking at the issues that came up, very few people, but still some people, have such environments.

I think we could consider fully dropping support for non-openssl environments on the next major version. But the way this got broken was completely unintentional and we still have code around (what I'm unifying here) to actually support non-openssl environments.

So, I think we can restore bare support for these environments for now (note that without this fix, as per the ruby-core issue, ruby won't even install without openssl).

Also, note that fixing this was only a side effect, my original intention was to unify the code that requires openssl to try to figure out the jruby failures we are currently experiencing (like https://github.com/rubygems/rubygems/runs/864749290?check_suite_focus=true). But since I was at it, I decided to fix this.

@deivid-rodriguez deivid-rodriguez force-pushed the openssl_require branch 2 times, most recently from 2703d3e to 4906036 Compare July 14, 2020 15:25
deivid-rodriguez and others added 17 commits July 19, 2020 13:13
The `rubygems/security` require already does this.
It will give more useful information.
No check is done for the other expectation and they are completely
symmetric as far as I can see.
The `openssl` require when openssl not present was having the
side-effect the our custom require fallbacks would end up loading `Gem::Specification.stubs`.

Co-authored-by: Alyssa Ross <hi@alyssa.is>
We patch `net-http-persistent` to not autoload `openssl`.
The whole test suite actually passes on my system, but it has weird
errors in CI. Since I don't want to spend time on it, instead of running
the whole test suite, I'm just adding a bare test to check that `gem
list` works.
@deivid-rodriguez
Copy link
Contributor Author

This PR makes the code dealing with loading openssl more unified and clear, and also fixes some (admittedly rare) usages as a side effect. So I'll merge this after CI passes. We can consider fully dropping all usages without openssl for rubygems 4.

@deivid-rodriguez deivid-rodriguez merged commit fa291a4 into master Jul 19, 2020
@deivid-rodriguez deivid-rodriguez deleted the openssl_require branch July 19, 2020 12:24
@hsbt hsbt added this to the 3.2.0 milestone Sep 23, 2020
hsbt pushed a commit that referenced this pull request Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature request: behaviour change for gem unpack] Less stringent requirement for openssl

3 participants