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

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

Merged
merged 5 commits into from Sep 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
51 changes: 51 additions & 0 deletions .github/workflows/puma-no-ssl.yml
@@ -0,0 +1,51 @@
name: No SSL

on: [push, pull_request]

jobs:
build:
name: >-
${{ matrix.os }} ${{ matrix.ruby }}
env:
CI: true
TESTOPTS: -v
DISABLE_SSL: no_ssl

runs-on: ${{ matrix.os }}
if: |
!( contains(github.event.pull_request.title, '[ci skip]')
|| contains(github.event.pull_request.title, '[skip ci]')
|| contains(github.event.head_commit.message, '[ci skip]')
|| contains(github.event.head_commit.message, '[skip ci]'))
strategy:
fail-fast: false
matrix:
include:
- { os: ubuntu-20.04, ruby: 2.7 }
- { os: ubuntu-20.04, ruby: jruby }
- { os: windows-2019, ruby: 2.7 }

steps:
- name: repo checkout
uses: actions/checkout@v2

- name: load ruby, ragel
uses: MSP-Greg/setup-ruby-pkgs@v1
with:
ruby-version: ${{ matrix.ruby }}
apt-get: ragel
brew: ragel
mingw: _upgrade_ openssl ragel

# won't run on Ruby 2.2, see puma.yml
- name: bundle install
shell: pwsh
run: bundle install --jobs 4 --retry 3

- name: compile
run: bundle exec rake compile

- name: test
id: test
timeout-minutes: 10
run: bundle exec rake test:all
1 change: 1 addition & 0 deletions 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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@MSP-Greg MSP-Greg Sep 13, 2020

Choose a reason for hiding this comment

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

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.

* EXPERIMENTAL: Add `fork_worker` option and `refork` command for reduced memory usage by forking from a worker process instead of the master process. (#2099)
* EXPERIMENTAL: Added `wait_for_less_busy_worker` config. This may reduce latency on MRI through inserting a small delay before re-listening on the socket if worker is busy (#2079).
* EXPERIMENTAL: Added `nakayoshi_fork` option. Reduce memory usage in preloaded cluster-mode apps by GCing before fork and compacting, where available. (#2093, #2256)
Expand Down
12 changes: 12 additions & 0 deletions README.md
Expand Up @@ -29,6 +29,18 @@ $ puma
Without arguments, puma will look for a rackup (.ru) file in
working directory called `config.ru`.

## SSL Connection Support

Puma will install/compile with support for ssl sockets, assuming OpenSSL
development files are installed on the system.

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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

to use ssl connections, set ENV['DISABLE_SSL'] to any value before installing
Puma.

## Frameworks

### Rails
Expand Down
23 changes: 23 additions & 0 deletions Rakefile
Expand Up @@ -47,6 +47,29 @@ if !Puma.jruby?
end
else
# Java (JRuby)
# ::Rake::JavaExtensionTask.source_files supplies the list of files to
# compile. At present, it only works with a glob prefixed with @ext_dir.
# override it so we can select the files
class ::Rake::JavaExtensionTask
def source_files
if ENV["DISABLE_SSL"]
# uses no_ssl/PumaHttp11Service.java, removes MiniSSL.java
FileList[
File.join(@ext_dir, "no_ssl/PumaHttp11Service.java"),
File.join(@ext_dir, "org/jruby/puma/Http11.java"),
File.join(@ext_dir, "org/jruby/puma/Http11Parser.java")
]
else
FileList[
File.join(@ext_dir, "PumaHttp11Service.java"),
File.join(@ext_dir, "org/jruby/puma/Http11.java"),
File.join(@ext_dir, "org/jruby/puma/Http11Parser.java"),
File.join(@ext_dir, "org/jruby/puma/MiniSSL.java")
]
end
end
end

Rake::JavaExtensionTask.new("puma_http11", gemspec) do |ext|
ext.lib_dir = "lib/puma"
end
Expand Down
15 changes: 15 additions & 0 deletions ext/puma_http11/no_ssl/PumaHttp11Service.java
@@ -0,0 +1,15 @@
package puma;

import java.io.IOException;

import org.jruby.Ruby;
import org.jruby.runtime.load.BasicLibraryService;

import org.jruby.puma.Http11;

public class PumaHttp11Service implements BasicLibraryService {
public boolean basicLoad(final Ruby runtime) throws IOException {
Http11.createHttp11(runtime);
return true;
}
}
4 changes: 4 additions & 0 deletions ext/puma_http11/puma_http11.c
Expand Up @@ -434,7 +434,9 @@ VALUE HttpParser_body(VALUE self) {
return http->body;
}

#ifdef HAVE_OPENSSL_BIO_H
void Init_mini_ssl(VALUE mod);
#endif

void Init_puma_http11()
{
Expand Down Expand Up @@ -463,5 +465,7 @@ void Init_puma_http11()
rb_define_method(cHttpParser, "body", HttpParser_body, 0);
init_common_fields();

#ifdef HAVE_OPENSSL_BIO_H
Init_mini_ssl(mPuma);
#endif
}
11 changes: 11 additions & 0 deletions lib/puma.rb
Expand Up @@ -10,6 +10,9 @@

require 'thread'

require_relative 'puma/puma_http11'
require_relative 'puma/detect'

module Puma
autoload :Const, 'puma/const'
autoload :Server, 'puma/server'
Expand All @@ -33,4 +36,12 @@ def self.set_thread_name(name)
return unless Thread.current.respond_to?(:name=)
Thread.current.name = "puma #{name}"
end

unless HAS_SSL
module MiniSSL
# this class is defined so that it exists when Puma is compiled
# without ssl support, as Server and Reactor use it in rescue statements.
class SSLError < StandardError ; end
end
end
end
18 changes: 12 additions & 6 deletions lib/puma/binder.rb
Expand Up @@ -5,10 +5,16 @@

require 'puma/const'
require 'puma/util'
require 'puma/minissl/context_builder'
require 'puma/configuration'

module Puma

if HAS_SSL
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

require 'puma/minissl'
require 'puma/minissl/context_builder'
require 'puma/accept_nonblock'
end

class Binder
include Puma::Const

Expand Down Expand Up @@ -155,6 +161,9 @@ def parse(binds, logger, log_msg = 'Listening')

@listeners << [str, io]
when "ssl"

raise "Puma compiled without SSL support" unless HAS_SSL

params = Util.parse_query uri.query
ctx = MiniSSL::ContextBuilder.new(params, @events).context

Expand Down Expand Up @@ -245,9 +254,8 @@ def inherit_tcp_listener(host, port, fd)

def add_ssl_listener(host, port, ctx,
optimize_for_latency=true, backlog=1024)
require 'puma/minissl'

MiniSSL.check
raise "Puma compiled without SSL support" unless HAS_SSL

if host == "localhost"
loopback_addresses.each do |addr|
Expand All @@ -264,7 +272,6 @@ def add_ssl_listener(host, port, ctx,
s.setsockopt(Socket::SOL_SOCKET,Socket::SO_REUSEADDR, true)
s.listen backlog


ssl = MiniSSL::Server.new s, ctx
env = @proto_env.dup
env[HTTPS_KEY] = HTTPS
Expand All @@ -275,8 +282,7 @@ def add_ssl_listener(host, port, ctx,
end

def inherit_ssl_listener(fd, ctx)
require 'puma/minissl'
MSP-Greg marked this conversation as resolved.
Show resolved Hide resolved
MiniSSL.check
raise "Puma compiled without SSL support" unless HAS_SSL

if fd.kind_of? TCPServer
s = fd
Expand Down
7 changes: 7 additions & 0 deletions lib/puma/detect.rb
@@ -1,6 +1,13 @@
# frozen_string_literal: true

module Puma
# 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)

def self.ssl?
HAS_SSL
end

IS_JRUBY = defined?(JRUBY_VERSION)

def self.jruby?
Expand Down
3 changes: 0 additions & 3 deletions lib/puma/minissl.rb
Expand Up @@ -10,7 +10,6 @@

module Puma
module MiniSSL

# define constant at runtime, as it's easy to determine at built time,
# but Puma could (it shouldn't) be loaded with an older OpenSSL version
HAS_TLS1_3 = !IS_JRUBY &&
Expand Down Expand Up @@ -203,8 +202,6 @@ def peercert
class SSLError < StandardError
# Define this for jruby even though it isn't used.
end

def self.check; end
end

class Context
Expand Down
3 changes: 0 additions & 3 deletions lib/puma/minissl/context_builder.rb
Expand Up @@ -2,9 +2,6 @@ module Puma
module MiniSSL
class ContextBuilder
def initialize(params, events)
require 'puma/minissl'
MiniSSL.check

@params = params
@events = events
end
Expand Down
2 changes: 1 addition & 1 deletion lib/puma/reactor.rb
@@ -1,7 +1,7 @@
# frozen_string_literal: true

require 'puma/util'
require 'puma/minissl'
require 'puma/minissl' if ::Puma::HAS_SSL

require 'nio'

Expand Down
1 change: 0 additions & 1 deletion lib/puma/runner.rb
Expand Up @@ -2,7 +2,6 @@

require 'puma/server'
require 'puma/const'
require 'puma/minissl/context_builder'

module Puma
# Generic class that is used by `Puma::Cluster` and `Puma::Single` to
Expand Down
3 changes: 0 additions & 3 deletions lib/puma/server.rb
Expand Up @@ -9,12 +9,9 @@
require 'puma/reactor'
require 'puma/client'
require 'puma/binder'
require 'puma/accept_nonblock'
require 'puma/util'
require 'puma/io_buffer'

require 'puma/puma_http11'

require 'socket'
require 'forwardable'

Expand Down