Automatic SSL certificate provisioning for localhost #2610

ye-lin-aung · 2021-04-24T09:02:01Z

Description

Add automatic SSL certificate provisioning for localhost.

Your checklist for this pull request

I have reviewed the guidelines for contributing to this repository.
I have added (or updated) 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.

I don't have much experience with FOSS contribution. This is one of my few contributions. Please let me know if the code doesn't work. I have added test cases as much as I can. I will create another pull request for docs once the commit is merged. I still can't run Rubocop. Its showing

Error: The Layout/Tab cop has been renamed to Layout/IndentationStyle.

… defined.

MSP-Greg · 2021-04-24T16:46:43Z

@ye-lin-aung

Thanks for the contribution/PR. A couple of suggestions from an old guy.

You can enable 'GitHub Actions' in your fork by clicking the 'Settings' tab, then 'Actions', then 'Allow all actions'. We've got RuboCop locked to an older version for our CI because we're currently testing back to Ruby 2.2.
You've done your work in your fork's master branch. You may find it easier to create new branches for PR work, as rebasing, keeping master clean, etc...
Puma works with TCP, UNIX, & SSL sockets. The methods in Binder are separated for each of those. You're adding ssl code to the add_tcp_listener method, the code should be added to the add_ssl_listener method. Also, MRI Puma doesn't need Ruby's OpenSSL, but does need the OpenSSL library files (normally consider part of the OS). I think.

In general, I'm probably not the best person to comment about the need for this. I'm not a hardcore 'crypto' type, but I've worked with OpenSSL for quite a while, contributing here and elsewhere. I also realize that there was a time when I knew nothing about it.

We've got the certs needed for our CI in the repo, and people can use them for additional testing, local use, etc. I don't have any issue with adding a gem by @ioquatix.

ioquatix · 2021-04-24T20:21:08Z

Looks great to me well done! Feedback from @MSP-Greg seems on point.

ioquatix · 2021-04-24T20:23:42Z

lib/puma/binder.rb

+      if defined? Localhost::Authority
+        raise "Puma compiled without SSL support" unless HAS_SSL
+        if ENV['RACK_ENV'] == "development" || ENV['RACK_ENV'] == "test"
+          authority = Localhost::Authority.fetch


Try to do this in the top level process (I assume this is) because there are some race conditions which I need to fix - if multiple processes try to get the same certificate at the same time, they may clobber each other - in theory not a big problem (especially for development) but you can simply avoid it by ensuring it's only called once per session and at some point I'll make sure it's more robust. See socketry/localhost#1 for more details.

By the way in several years of using It, I've not even had a single bug report about it. So, it's kind of a non issue in practice.

Okay, I fixed it :) Still do it in the top level process to avoid generating a different certificate per worker if possible :)

MSP-Greg · 2021-04-24T20:57:54Z

@ioquatix

A long time ago, Puma was setup to not use Ruby's OpenSSL. An example of how we set up the context is:

if Puma.jruby?
  ctx.keystore =  "#{CERT_PATH}/keystore.jks"
  ctx.keystore_pass = 'jruby_puma'
else
  ctx.key  = "#{CERT_PATH}/server.key"
  ctx.cert = "#{CERT_PATH}/server.crt"
  ctx.ca   = "#{CERT_PATH}/ca.crt"
end

Can that be done with localhost?

JFYI, the current context (Puma::MiniSSL::Context) is a ruby placeholder for info about the context, then it's created in minissl.c to pass along to the server.

ioquatix · 2021-04-25T00:54:30Z

I don't have any opinion about it. What problem are we trying to solve with multiple SSL providers?

ye-lin-aung · 2021-04-25T07:25:59Z

@ioquatix currently, localhost gem doesn't provide to get file path of cert or key via file path. Puma::MiniSSL accepts key and certs as file paths. Is it possible for Localhost::Authority.fetch to able to respond attributes like cert_path or key_path? I can help with PR if you are ok.

ioquatix · 2021-04-25T07:43:59Z

I am okay to receive a PR to expose the underlying paths of the certificates. The assumption here is that they will always be on disk, which I don't know if that's always true or not. The ability to create ephemeral certificates (not stored to disk) is a feature too.

Use MiniSSLContext and MiniSSLServer for localhost authority's self-signed ceritifcates

ye-lin-aung · 2021-04-25T10:06:37Z

I have made the changes according to @MSP-Greg suggestions. I have moved the usage of localhost authority inside tcp_listener to ssl_listener. And I have also make changes to use Puma::MiniSSL::Context with localhost authority and removed previous OpenSSL Context from commit. I have also moved the call of Localhost::Authority.fetch to Binder initialization according to @ioquatix suggestions. Thank you everyone for the feedbacks.

…t_authority doesn't suppot JKS yet.

pascalbetz · 2021-04-27T05:55:27Z

Could this feature be explained/mentioned in the README?

README.md

lib/puma/binder.rb

lib/puma/request.rb

test/test_puma_localhost_authority.rb

ioquatix · 2021-07-27T00:31:44Z

When this is merged, can you please submit a PR to the localhost gem to add a link to puma under "See Also" in the README.md.

nateberkopec · 2021-08-16T19:42:15Z

README.md

+```ruby
+# config.ru
+require './app'
+require 'localhost/authority'


can you gem 'localhost', require: 'localhost/authority'? Does that work?

presumably in my app I should also put localhost in the development group in my Gemfile since the release doesn't depend on the gem... :)

nateberkopec · 2021-08-18T14:34:21Z

@ye-lin-aung Since this is probably going to be a headline feature in the next release, why don't you take the honor of deciding the codename for us?

ye-lin-aung · 2021-08-20T06:34:16Z

@nateberkopec sure. Thank you for giving me chance. I will let you know this weekend

ioquatix · 2021-09-20T21:54:56Z

I released an update to localhost gem so require: true should just work without any custom gem config.

See https://github.com/socketry/localhost/tree/v1.1.9/guides/browser-configuration Uses the new integration in Puma: puma/puma#2610

* Added puma to automatically use localhost gem to self signed https if defined. * Update files according to rubocop rules * Moved localhost authority from tcp_listener to ssl_listener Use MiniSSLContext and MiniSSLServer for localhost authority's self-signed ceritifcates * Reformatted codes according to rubocop rules * Fixed test case crashing in production env * Removed transform_keys to support ruby version < 2.5.0 * Changed wrong keystore_pass key given to context to keystore-pass * Remove accept_nonblock.rb since we are using MiniSSL Server * Removed localhost_authority test case running in JRUBY since localhost_authority doesn't suppot JKS yet. * Reload Localhost authority if not loaded runned from puma cli * Memorise localhost authority object on init * Added readme for self-signed certificates * Removed jruby version * Update readme.md * Added validations to check certificate * Remove ssl test running in no ssl implementations * Changed host in localhost authority * Update ssl events wrong arguments error * Update README.md * Update binder.rb * Update binder.rb * Update binder.rb * Update binder.rb * Update request.rb * Update binder * Removed running test in JRUBY * Removed testing localhost authority file while in JRUBY * Removed unused variables * Updated readme Co-authored-by: Nate Berkopec <nate.berkopec@gmail.com>

Added puma to automatically use localhost gem to self signed https if…

90353b0

… defined.

nateberkopec added feature waiting-for-review Waiting on review from anyone labels Apr 24, 2021

Update files according to rubocop rules

5c1b82f

ioquatix reviewed Apr 24, 2021

View reviewed changes

ye-lin-aung added 2 commits April 25, 2021 16:25

Moved localhost authority from tcp_listener to ssl_listener

0c5ff76

Use MiniSSLContext and MiniSSLServer for localhost authority's self-signed ceritifcates

Reformatted codes according to rubocop rules

4b448ce

ye-lin-aung added 5 commits April 25, 2021 18:41

Fixed test case crashing in production env

e3cace7

Removed transform_keys to support ruby version < 2.5.0

944cc0d

Changed wrong keystore_pass key given to context to keystore-pass

0f33961

Remove accept_nonblock.rb since we are using MiniSSL Server

cc359e6

Removed localhost_authority test case running in JRUBY since localhos…

014c703

…t_authority doesn't suppot JKS yet.

ye-lin-aung added 3 commits April 27, 2021 18:37

Reload Localhost authority if not loaded runned from puma cli

c72f8de

Memorise localhost authority object on init

c44c1df

Added readme for self-signed certificates

c459993

pascalbetz reviewed Apr 27, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved