fix request.rb for intermittently raised error using TLS 1.3 #2486
Conversation
|
As of today 2018-11-21, the only 'standard' rubies available that are using OpenSSL 1.1.1 are windows Ruby builds 2.5.3 or greater. The current Travis rubies (both Ubuntu & OSX/MacOS) are using earlier versions. OpenSSL 1.1.1 is the first version with TLSv1_3, and it operates a little differently than TLSv1_2. Some of the differences are only an issue with CI where a server and a client are both being created for testing... |
|
@MSP-Greg can you add a test? thanks! |
|
See RG issue 2388.
If I could consistently repo the error, yes. Since I can't, no. As mentioned in the first msg, while using a new version of OpenSSL/TLS, this test has intermittently failed/errored due to a particular OpenSSL error being raised and not caught. But, an error should be raised. I could remove the code from |
|
Maybe we can mock the error with |
The issue occurs when TLS v1.3 exists. On Windows, I don't believe I've ever repo'd it locally. Appveyor fails occasionally on it (both testing rubygems/rubygems & ruby/ruby). Note that in ruby/openssl issue 227, it's stated that there are systems where the error being raised is not intermittent. Also note the statement "TLS 1.3, which is new in OpenSSL 1.1.1, handles a client certificate differently from TLS 1.2." This is messy, as until more repos test with more OS's and OpenSSL 1.1.1 configurations, I don't think it's well defined. Importantly, this issue here is about OpenSSL / net/http / RubyGems correctly sensing a client security error, and stopping it. The problem is an error is raised by OpenSSL that isn't allowed for or caught. I was an early adopter of 1.1.1, and was building & testing prereleases. I used it with ruby, puma, & eventmachine, and was involved in updating them & MSYS2 to use it. By no means am I an OpenSSL expert, but I've seen what adjustments have been needed with TLS 1.3. Long story short, I doubt this is the only change that will be needed with TLS 1.3, but it gets the CI to be consistent. As mentioned earlier, the test code could be changed, rather than |
|
Was looking to see about other trunk CI testing (make install before make test?), and came across the following: http://rubyci.s3.amazonaws.com/ubuntu/ruby-trunk/log/20181129T003002Z.log.html.gz#rubyspec Scroll up a bit, and you'll see this error on an Ubuntu 18.10 build... |
| rescue (defined?(OpenSSL::SSL) ? OpenSSL::SSL::SSLError : nil) => e | ||
| # As of 2018-11, error is intermittently raised when using TLS 1.3 with | ||
| # improper client authentication | ||
| if OpenSSL::SSL.const_defined? :TLS1_3_VERSION |
There was a problem hiding this comment.
Should this be checked earlier in
rubygems/lib/rubygems/request.rb
Line 134 in fccc313
OpenSSL::SSL is defined there?
There was a problem hiding this comment.
I believe all of the caller listings show hitting RG first at request.rb:222. The rescue in 240 catches errors in line 222.
| @@ -237,6 +237,14 @@ def perform_request(request) # :nodoc: | |||
| verbose "fatal error" | |||
|
|
|||
| raise Gem::RemoteFetcher::FetchError.new('fatal error', @uri) | |||
| rescue (defined?(OpenSSL::SSL) ? OpenSSL::SSL::SSLError : nil) => e | |||
There was a problem hiding this comment.
Why we rescuing for nil? if OpenSSL::SSL is not defined?
There was a problem hiding this comment.
I, possibly incorrectly, assumed that rescue was a form of is_a?. It's been a few days since I did the PR, but I think I locally tested to see if it was catching other errors.
For the test to pass, it has to receive a Gem::RemoteFetcher::FetchError error, and if the CI passed for Ruby versions without TLS v1.3, nil is not catching other errors.
|
Thanks for the explanation @MSP-Greg i think i understand better now, i left some comments |
|
@bronzdoc You're welcome, and thanks for the thanks. |
2507: Fixed test fails with the newer version of OpenSSL r=hsbt a=hsbt # Description: This backport fixes #2486 . I already fixed at ruby/ruby@edbac1b ______________ # 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](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: hsbt <hsbt@b2dd03c8-39d4-4d8f-98ff-823fe69b080e>
Description:
When running tests with a Ruby built with OpenSSL 1.1.1 or later, one test has intermittently failed both here and in Ruby trunk testing. With trunk, sometimes the failure occurs when testing parallel, but passes on retry. The test is:
An error should be raised in the test, but this one starts with OpenSSL, passes thru net/http, and should be caught in
request.rb. Since it seems to only occur with OpenSSL 1.1.1/TLSv1_3, code somewhat accounts for that...Two examples of test failure:
https://ci.appveyor.com/project/rubygems/rubygems/builds/20388929/job/lk27qkfajexyciuj#L30
https://ci.appveyor.com/project/MSP-Greg/ruby-loco/builds/20465475#L2933
Tasks:
I will abide by the code of conduct.