Openssl updates #1836

MSP-Greg · 2019-07-09T23:08:58Z

1st commit - OpenSSL 1.1.1 updates, add #no_tlsv1_1
1.1 Adds three constants to MiniSSL, as many OpenSSL builds cannot/will not connect with older protocols:

   OPENSSL_NO_SSL3
   OPENSSL_NO_TLS1
   OPENSSL_NO_TLS1_1

1.2 Uses SSL_CTX_set_min_proto_version when available
1.3 Add #no_tlsv1_1 method to Puma::MiniSSL::Context

2nd commit - travis.yml - add bionic 18.04 job
2.1 Add Ubuntu bionic 18.04 job with Ruby 2.6.3. This tests with OpenSSL 1.1.1

3rd commit - test_puma_server_ssl.rb - add tests for #no_tlsv1, #no_tlsv1_1
3.1 Adds tests for #no_tlsv1 and #no_tlsv1_1
3.2 Changes setup (via adding a start_server method), allowing the context to be passed to a block

4th commit - add no_tlsv1_1 to binder, config, etc
4.1 Adds use of #no_tlsv1_1 with binder.rb and dsl.rb
4.2 Adds tests to test_binder.rb and test_config.rb for above
4.3 Adds feature to MiniSSL.java

MSP-Greg · 2019-07-09T23:30:11Z

A few notes:

I mentioned the new #no_tlsv1_1 method in another thread, immediately after that, I googled TLSv1.1. There are several major sites (including GitHub?) that have disabled TLSv1.1. It's time, and I had already started on the PR.
Travis/rvm seems to have Ruby master/trunk/head messed up again. If it isn't fixed soon, I'll patch then. I think RUBYOPT=--jit needs to be defined after bundle install, which fixed the issue in another repo...
Ruby 2.6.3 is currently tested in Xenial (1.0.2), and on Appveyor, Bionic, & OSX, all with 1.1.1. Since OSX jobs take quite a while, I'd suggest removing the OSX 2.6.3 job. On OSX, both the 2.4 & 2.5 jobs test on 1.1.1.
Lastly, I think with the additions/changes in this PR, all OpenSSL issues are resolved, and nothing will need to be added/changed in the foreseeable future. After all, we're testing on 1.0.1 thru 1.1.1, all is passing, and protocols can be restricted if needed...

nateberkopec · 2019-07-10T00:06:53Z

🙏 Really solid work on all of this @MSP-Greg. The way a PR should be done!

Will review soon, looks like there was an unrelated build failure.

MSP-Greg · 2019-07-10T01:05:46Z

@nateberkopec

Thank you, really. Sorry for adding the method, but I needed a break from a Ruby/JS mess, and I've done a few other OpenSSL PR's this week, similar issues. So, I just wanted to get it done...

looks like there was an unrelated build failure.

That pissed me off.

Passed in my fork.
Been involved with testing enought that I generally don't write unstable tests. Checked other jobs, and some ran all the SSL tests before the tests that failed.
I think the failed tests involve fork, and being a windows guy, I can't test them locally.

Long story short, I guess we'll all keep an eye on it...

nateberkopec · 2019-07-16T22:58:56Z

test/test_puma_server_ssl.rb

+  end
+
+  def teardown
+    @http.finish if !@http.nil? && @http.started?


style nitpick but I hate unnecessary predicates. I think if @http is sufficient here, as is unless @server in the next line.

@nateberkopec

I just pushed this change, and it's breaking things.

Although I can't recall what/why, there are times when connections aren't 'made' due to testing for restricted protocol versions. Are you ok with returning these to the original code in the PR?

EDIT: Generally, with new things, I will always test in my fork. I will not always check 'old code', but I am not the type to make code more complex. I also forget things/reasons (better with theory than details...). I'll try to add more comments in the future...

@http.finish if !@http.nil? && @http.started? @server.stop(true) unless @server.nil?

changed to:

@http.finish if @http && @http.started? @server.stop(true) if @server

Yup! if the predicate is necessary, then it's necessary. just thought it wasn't.

nateberkopec · 2019-07-16T23:00:31Z

test/test_puma_server_ssl.rb

+    else
+      @http.ssl_version = :TLSv1
+    end
+    # Ruby 2.4.5 on Travis raises ArgumentError


nateberkopec · 2019-07-16T23:01:21Z

The Ruby here is good with me, needs a review from someone else on the C.

nateberkopec · 2019-07-16T23:03:57Z

I think RUBYOPT=--jit needs to be defined after bundle install
I'd suggest removing the OSX 2.6.3 job. On OSX, both the 2.4 & 2.5 jobs test on 1.1.1.

MSP-Greg · 2019-07-17T02:53:04Z

Should be ok if the 'review from someone else on the C' is good.

Ruby master will fail on Travis until a bundler issue specific to 'nested bundle commands used when bundler is a default gem' is fixed. PR submitted to bundler...

MSP-Greg · 2019-07-17T13:35:13Z

@nateberkopec

rebased
added the code to use no_tlsv1_1 to binder, config, tests, etc. Previously, the PR just added it to MiniSSL::Context, which isn't very useful...

MSP-Greg · 2019-08-03T15:42:06Z

2019-08-03 15:30 UTC, I selfishly rebased to trigger a CI run to test ruby-head.

The fix for 'nested bundle commands when bundler is a default gem' has been pushed from Bundler to ruby-head/trunk, and ruby-head is now passing.

A small patch in this PR additionally fixes ruby-head with --jit, and it is also passing.

See:
https://travis-ci.org/puma/puma/builds/567321441

Note that this wasn't an issue on Appveyor, as all the tests that have bundle commands also use fork, which is unavailable on Windows & JRuby...

This was referenced Jul 9, 2019

Add server test for no_tlsv1, updates to mini_ssl.c #1799

Closed

Add constants for availability of SSLv3 & TLSv1, adjust tests, add no_tlsv1 test #1788

Closed

nateberkopec added feature ssl labels Jul 10, 2019

nateberkopec reviewed Jul 16, 2019

View reviewed changes

test/test_puma_server_ssl.rb

else

@http.ssl_version = :TLSv1

end

# Ruby 2.4.5 on Travis raises ArgumentError

Copy link

Member

nateberkopec Jul 16, 2019

Choose a reason for hiding this comment

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

lmao

nateberkopec added the waiting-for-changes Waiting on changes from the requestor label Jul 16, 2019

MSP-Greg force-pushed the openssl-updates branch 2 times, most recently from 0cca7c0 to b397cc5 Compare July 17, 2019 02:35

MSP-Greg force-pushed the openssl-updates branch from b397cc5 to 44b00a5 Compare July 17, 2019 13:32

nateberkopec removed the waiting-for-changes Waiting on changes from the requestor label Jul 17, 2019

MSP-Greg mentioned this pull request Jul 21, 2019

Do not override RUBYOPT on travis #1858

Closed

nateberkopec added this to the 4.1.0 milestone Jul 28, 2019

MSP-Greg force-pushed the openssl-updates branch from 44b00a5 to 0237611 Compare August 3, 2019 15:22

MSP-Greg added 4 commits August 3, 2019 14:36

OpenSSL 1.1.1 updates, add #no_tlsv1_1

599dc33

travis.yml - add bionic 18.04 job

01a3bba

test_puma_server_ssl.rb - add tests for #no_tlsv1, #no_tlsv1_1

a119661

add no_tlsv1_1 to binder, config, etc

35dbec0

MSP-Greg force-pushed the openssl-updates branch from 0237611 to 35dbec0 Compare August 3, 2019 19:57

nateberkopec assigned evanphx Aug 4, 2019

nateberkopec removed this from the 4.1.0 milestone Aug 4, 2019

MSP-Greg mentioned this pull request Aug 6, 2019

Track and wait on dead workers to prevent zombies #1884

Closed

evanphx merged commit 3ea2471 into puma:master Aug 6, 2019

MSP-Greg deleted the openssl-updates branch September 18, 2019 22:54

dentarg mentioned this pull request Mar 16, 2020

Spike an implementation of replacing minissl w/openssl #2188

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Openssl updates #1836

Openssl updates #1836

MSP-Greg commented Jul 9, 2019 •

edited

Loading

MSP-Greg commented Jul 9, 2019

nateberkopec commented Jul 10, 2019

MSP-Greg commented Jul 10, 2019 •

edited

Loading

nateberkopec Jul 16, 2019

MSP-Greg Jul 17, 2019 •

edited

Loading

nateberkopec Jul 17, 2019

nateberkopec Jul 16, 2019

nateberkopec commented Jul 16, 2019

nateberkopec commented Jul 16, 2019

MSP-Greg commented Jul 17, 2019

MSP-Greg commented Jul 17, 2019 •

edited

Loading

MSP-Greg commented Aug 3, 2019 •

edited

Loading

Openssl updates #1836

Openssl updates #1836

Conversation

MSP-Greg commented Jul 9, 2019 • edited Loading

MSP-Greg commented Jul 9, 2019

nateberkopec commented Jul 10, 2019

MSP-Greg commented Jul 10, 2019 • edited Loading

nateberkopec Jul 16, 2019

Choose a reason for hiding this comment

MSP-Greg Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

nateberkopec Jul 17, 2019

Choose a reason for hiding this comment

nateberkopec Jul 16, 2019

Choose a reason for hiding this comment

nateberkopec commented Jul 16, 2019

nateberkopec commented Jul 16, 2019

MSP-Greg commented Jul 17, 2019

MSP-Greg commented Jul 17, 2019 • edited Loading

MSP-Greg commented Aug 3, 2019 • edited Loading

MSP-Greg commented Jul 9, 2019 •

edited

Loading

MSP-Greg commented Jul 10, 2019 •

edited

Loading

MSP-Greg Jul 17, 2019 •

edited

Loading

MSP-Greg commented Jul 17, 2019 •

edited

Loading

MSP-Greg commented Aug 3, 2019 •

edited

Loading