Allow Puma to compile when built without SSL, load SSL files on demand #2305

MSP-Greg · 2020-07-06T03:44:00Z

Description

Currently, Puma will not function if built without SSL support. Also, SSL related files are all loaded regardless, so make them load-on-demand.

Changes

Allows compiling without OpenSSL & runs 'no ssl' CI on a few jobs
SSL test (now Puma::HAS_SSL or Puma.ssl?) relies on classes that only exist in the compiled extension if properly compiled with OpenSSL.
If compiled with 'no ssl', don't load ssl related files in Puma. Also, don't load Ruby MRI OpenSSL. JRuby currently requires it for some reason. (?)

Commits (need to be squashed for bisect)

'Adjust code for compiling without SSL (MRI & JRuby), add ssl detection' - Removes MiniSSL.check, replace with Puma::HAS_SSL or Puma.ssl?
'Adjust test files for 'no ssl' compile'
'Actions - add 'no ssl' workflow, puma-no-ssl.yml' - three jobs: Ubuntu-20.04 with 2.7 & JRuby, and Windows 2.7
'Update History.md'
'README.md - add 'SSL Connection Support' section'

Your checklist for this pull request

I have reviewed the guidelines for contributing to this repository.
I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
I have added appropriate tests if this PR fixes a bug or adds a feature.
My pull request is 100 lines added/removed or less so that it can be easily reviewed.
If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
I have updated the documentation accordingly.
All new and existing tests passed, including Rubocop.

lib/puma/binder.rb

sj26 · 2020-07-07T03:31:52Z

Thanks for the quick work on this! Sorry it took me a while to test it, complications. But I just gave it a go and it works great. 👍

lib/puma/binder.rb

eregon · 2020-07-07T17:36:05Z

lib/puma/minissl.rb

@@ -308,5 +307,5 @@ def close
        @socket.close unless @socket.closed?       # closed? call is for Windows
      end
    end
-  end
+  end if IS_JRUBY || (Puma::MiniSSL.check.nil? rescue nil)


This should be a better check, silently skipping tests if e.g. there is a typo, seems bad.

That check is to not load minissl.rb if MRI Puma wasn't compiled with SSL support. Similar checks for some of the tests also. The silent skips will only happen when Puma isn't compiled with SSL support. I'm working with it in my fork (adding a 'none SSL' job) and locally. JFYI, working with JRuby head locally.

I think we can leave a comment (or add a new method to Puma detect) to explain.

Ok. Let me think about that (MRI/JRuby/TruffleRuby). Puma detect is good, but it overlaps with Puma::MiniSSL.check. Maybe lose Puma::MiniSSL.check and have a method in detect that returns a boolean rather than the damn error in Puma::MiniSSL.check. Someone using it with UNIXSockets may chose to compile without OpenSSL... Sorry, rant off.

nateberkopec · 2020-09-06T00:35:24Z

Hmm, how do we want to do CI for this? Just trigger it with an environment variable?

MSP-Greg · 2020-09-06T00:46:52Z

Hmm, how do we want to do CI for this? Just trigger it with an environment variable?

I was looking at that this week, and didn't finish. extconf.rb works with MRI, but I'm not sure about other platforms?

nateberkopec · 2020-09-06T02:00:09Z

Well MRI is the only platform we're worried about supporting w/o SSL, though, right?

MSP-Greg · 2020-09-10T18:56:04Z

Well MRI is the only platform we're worried about supporting w/o SSL, though, right?

No, I need to check it with JRuby, I'll also check against TruffleRuby, but I think it should behave similar to MRI... (?)

MSP-Greg · 2020-09-12T15:36:38Z

I believe I've got JRuby compiling without OpenSSL (simple changes).

At present, runtime SSL detection is kind of odd (putting that nicely). I hate using rescue to do so.

I think a better way is checking what classes exist in puma_http11.

Question - Since puma_http11 is always used, maybe move the require to Puma, and add the runtime OpenSSL detection in puma/detect.rb so we have a constant/method to check?

A goal is to not need to require Ruby's OpenSSL if Puma is compiled without OpenSSL...

nateberkopec · 2020-09-12T17:46:08Z

Question - Since puma_http11 is always used, maybe move the require to Puma, and add the runtime OpenSSL detection in puma/detect.rb so we have a constant/method to check?

A goal is to not need to require Ruby's OpenSSL if Puma is compiled without OpenSSL...

Makes sense!

MSP-Greg · 2020-09-12T18:19:59Z

Two items:

There are three classes (I think) that rescue MiniSSL or OpenSSL errors (Client, Reactor, & Server). If one compiles without SSL, these constants are not defined. I'm thinking of setting the rescue errors to LocalJumpError, which is defined, and will never be raised?
I'll separate lib and test changes, but for this PR to bisect, it needs to be squashed.

MSP-Greg · 2020-09-12T21:59:53Z

This is done, updated notes, etc.

JRuby & TruffleRuby still intermittently lock up in testing. The last pair of jobs in my fork both passed, using JRuby for the 'no ssl' workflow (not jruby-head).

MSP-Greg · 2020-09-13T15:49:46Z

I just pushed an update. Before, the code to determine whether Puma was built with ssl in detect.rb was:

HAS_SSL = const_defined?(:MiniSSL, false)

Updated code is:

# at present, MiniSSL::Engine is only defined in extension code, not in minissl.rb
HAS_SSL = const_defined?(:MiniSSL, false) && MiniSSL.const_defined?(:Engine, false)

Puma::MiniSSL is defined in minissl.rb (using module MiniSSL). So if minissl.rb was loaded first, the detection would fail. Currently, Puma::MiniSSL::Engine is only defined in 'extension' code, so it's not affected by loading minissl.rb...

lib/puma/binder.rb

nateberkopec · 2020-09-13T15:52:17Z

lib/puma/binder.rb

 require 'puma/configuration'

 module Puma
+
+  if HAS_SSL


What's the purpose of moving these from the top of the file?

We could move it above and use Puma::HAS_SSL. I put it here for the namespace.

This was an issue I was about to post a message about.

Currently, if Puma successfully compiles with OpenSSL, it loads all the files. Should we make so that it only loads the files if one binds to an ssl socket?

That is trickier...

lib/puma/client.rb

nateberkopec · 2020-09-13T15:56:26Z

README.md

@@ -13,11 +13,19 @@ Puma is a **simple, fast, multi-threaded, and highly concurrent HTTP 1.1 server

 ## Built For Speed &amp; Concurrency

-Puma processes requests using a C-optimized Ragel extension (inherited from Mongrel) that provides fast, accurate HTTP 1.1 protocol parsing in a portable way. Puma then serves the request using a thread pool. Each request is served in a separate thread, so truly concurrent Ruby implementations (JRuby, Rubinius) will use all available CPU cores.
+Puma processes requests using a C-optimized Ragel extension (inherited from


Did you change anything in here other than 80-char line breaks?

I added a sentence to Quick Start, which maybe should be expanded on?

"Puma expects to find OpenSSL development files when installed/compiled. If you want to compile it without ssl support, set ENV['DISABLE_SSL']."

I can pull this and do a separate PR.

I removed to format changes and just added an ssl section.

nateberkopec · 2020-09-13T15:57:48Z

History.md

@@ -1,6 +1,7 @@
 ## 5.0.0

 * Features
+  * Allow compiling without OpenSSL and dynamically load files needed for SSL, add 'no ssl' CI (#2305)


It looks like for JRuby, for this to work, you need to have DISABLE_SSL set, but for MRI, you don't. Is that true?

Re MRI, I know how to uninstall 'openssl dev', and it works locally.

I don't know how to uninstall Java's equivalent of 'openssl dev'. For MRI systems, it's a separate install, I don't think that's the case with Java?

EDIT:

Sorry I wasn't clear.

for JRuby, for this to work, you need to have DISABLE_SSL set

Correct.

for MRI, you don't

If you don't have 'openssl dev' installed, there is no need for ENV['DISABLE_SSL']. If it is installed, you need to set the ENV variable in shell to disable compiling with OpenSSL.

MSP-Greg · 2020-09-13T23:29:18Z

Interesting. The last push (which passed CI here) here also ran in my fork. The only failure was on
ubuntu-18.04 truffleruby-head, see:

https://github.com/MSP-Greg/puma/runs/1109525323?check_suite_focus=true#step:7:599

Note that the test suite passed, but it just barely timed out, so it shows as a failure. First time I've seen that, but maybe extend the test step's timeout to 12? Hate to do that...

MSP-Greg · 2020-09-14T01:29:52Z

I noticed that test_puma_server_ssl.rb would not run as a single test file:

rake test TESTOPTS=--verbose TEST=test/test_puma_server_ssl.rb

Fixed.

MSP-Greg · 2020-09-14T17:26:06Z

Removed Actions TruffleRuby RuboCop commit

nateberkopec · 2020-09-14T21:15:54Z

README.md

+If the system does not have OpenSSL development files installed, Puma will
+install/compile, but it will not allow ssl connections.
+
+If the system has OpenSSL development files installed, but you don't want Puma


I feel like this is a little unclear. If you don't bind Puma to SSL, why should you need to use DISABLE_SSL?

As written, this makes it sound like anyone using Puma w/o SSL must set DISABLE_SSL.

If you don't bind Puma to SSL, why should you need to use DISABLE_SSL?

You don't have to, but 'If the system has OpenSSL development files', it will compile the SSL functions into puma_http11.so/jar and will load the OpenSSL libraries/dlls when using MRI. It also loads Ruby OpenSSL on MRI.
I believe using DISABLE_SSL stops all of that.

this makes it sound like anyone using Puma w/o SSL must set DISABLE_SSL.

Didn't mean to imply that. Maybe a rephrasing is in order. I meant to make it clear that it's optional. But, I didn't say anything about benefits...

BTW, thanks for reviewing.

Yeah I just ended up removing it. I think it's too confusing. Good feature to have but not one that needs explaining right in README.md.

Thanks so much for your work on the test suite over the last 2 weeks Greg, it's so much better now.

MSP-Greg mentioned this pull request Jul 6, 2020

Improve MiniSSL for users without OpenSSL available #2303

Closed

8 tasks

sj26 reviewed Jul 6, 2020

View reviewed changes

lib/puma/binder.rb Outdated Show resolved Hide resolved

MSP-Greg force-pushed the no-ssl branch 2 times, most recently from f364079 to 68cc2df Compare July 7, 2020 01:19

eregon reviewed Jul 7, 2020

View reviewed changes

lib/puma/binder.rb Outdated Show resolved Hide resolved

eregon reviewed Jul 7, 2020

View reviewed changes

nateberkopec added bug maintenance waiting-for-changes Waiting on changes from the requestor labels Jul 16, 2020

MSP-Greg force-pushed the no-ssl branch from 68cc2df to 4d157ba Compare July 18, 2020 23:20

MSP-Greg mentioned this pull request Jul 21, 2020

Puma loading, History.md questions #2315

Closed

nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Sep 5, 2020

nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Sep 6, 2020

MSP-Greg mentioned this pull request Sep 10, 2020

Add option to bind to systemd activated sockets #2362

Merged

8 tasks

MSP-Greg mentioned this pull request Sep 12, 2020

v5.0.0.beta2 - libssl w/o openssl CLI raises exception on boot #2363

Closed

MSP-Greg force-pushed the no-ssl branch 2 times, most recently from a9c1c8c to c2624ae Compare September 12, 2020 21:55

MSP-Greg marked this pull request as ready for review September 12, 2020 21:59

MSP-Greg force-pushed the no-ssl branch from c2624ae to e42653f Compare September 12, 2020 23:42

MSP-Greg force-pushed the no-ssl branch 2 times, most recently from e04335f to e20b7e8 Compare September 13, 2020 15:31

nateberkopec requested changes Sep 13, 2020

View reviewed changes

MSP-Greg force-pushed the no-ssl branch 3 times, most recently from e074cc7 to 5781aab Compare September 13, 2020 23:10

MSP-Greg force-pushed the no-ssl branch from 5781aab to 608cff9 Compare September 14, 2020 01:28

MSP-Greg added 5 commits September 14, 2020 12:20

Adjust code for compiling without SSL (MRI & JRuby), add SSL detection

b1c760a

Adjust test files for 'no ssl' compile

f291c63

Actions - add 'no ssl' workflow, puma-no-ssl.yml

55885c9

Update History.md

55e78e4

README.md - add 'SSL Connection Support' section

fe2c25b

MSP-Greg force-pushed the no-ssl branch from 608cff9 to fe2c25b Compare September 14, 2020 17:24

nateberkopec reviewed Sep 14, 2020

View reviewed changes

nateberkopec merged commit 1ebdf11 into puma:master Sep 14, 2020

MSP-Greg deleted the no-ssl branch September 14, 2020 21:46

MSP-Greg mentioned this pull request Sep 17, 2020

Uninitialized constant OPENSSL_VERSION - server won't start #2296

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Puma to compile when built without SSL, load SSL files on demand #2305

Allow Puma to compile when built without SSL, load SSL files on demand #2305

MSP-Greg commented Jul 6, 2020 •

edited

sj26 commented Jul 7, 2020

eregon Jul 7, 2020

MSP-Greg Jul 7, 2020

nateberkopec Sep 6, 2020

MSP-Greg Sep 6, 2020 •

edited

nateberkopec commented Sep 6, 2020

MSP-Greg commented Sep 6, 2020

nateberkopec commented Sep 6, 2020

MSP-Greg commented Sep 10, 2020

MSP-Greg commented Sep 12, 2020 •

edited

nateberkopec commented Sep 12, 2020

MSP-Greg commented Sep 12, 2020 •

edited

MSP-Greg commented Sep 12, 2020 •

edited

MSP-Greg commented Sep 13, 2020

nateberkopec Sep 13, 2020

MSP-Greg Sep 13, 2020

nateberkopec Sep 13, 2020

MSP-Greg Sep 13, 2020

MSP-Greg Sep 13, 2020

nateberkopec Sep 13, 2020

MSP-Greg Sep 13, 2020 •

edited

MSP-Greg commented Sep 13, 2020

MSP-Greg commented Sep 14, 2020

MSP-Greg commented Sep 14, 2020

nateberkopec Sep 14, 2020

MSP-Greg Sep 14, 2020

nateberkopec Sep 15, 2020

Allow Puma to compile when built without SSL, load SSL files on demand #2305

Allow Puma to compile when built without SSL, load SSL files on demand #2305

Conversation

MSP-Greg commented Jul 6, 2020 • edited

Description

Changes

Your checklist for this pull request

sj26 commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSP-Greg Sep 6, 2020 • edited

Choose a reason for hiding this comment

nateberkopec commented Sep 6, 2020

MSP-Greg commented Sep 6, 2020

nateberkopec commented Sep 6, 2020

MSP-Greg commented Sep 10, 2020

MSP-Greg commented Sep 12, 2020 • edited

nateberkopec commented Sep 12, 2020

MSP-Greg commented Sep 12, 2020 • edited

MSP-Greg commented Sep 12, 2020 • edited

MSP-Greg commented Sep 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSP-Greg Sep 13, 2020 • edited

Choose a reason for hiding this comment

MSP-Greg commented Sep 13, 2020

MSP-Greg commented Sep 14, 2020

MSP-Greg commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSP-Greg commented Jul 6, 2020 •

edited

MSP-Greg Sep 6, 2020 •

edited

MSP-Greg commented Sep 12, 2020 •

edited

MSP-Greg commented Sep 12, 2020 •

edited

MSP-Greg commented Sep 12, 2020 •

edited

MSP-Greg Sep 13, 2020 •

edited