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
Automatic SSL certificate provisioning for localhost #2610
Conversation
Thanks for the contribution/PR. A couple of suggestions from an old guy.
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. |
Looks great to me well done! Feedback from @MSP-Greg seems on point. |
lib/puma/binder.rb
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I fixed it :) Still do it in the top level process to avoid generating a different certificate per worker if possible :)
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 ( |
I don't have any opinion about it. What problem are we trying to solve with multiple SSL providers? |
@ioquatix currently, |
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
I have made the changes according to @MSP-Greg suggestions. I have moved the usage of localhost authority inside |
…t_authority doesn't suppot JKS yet.
Could this feature be explained/mentioned in the README? |
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 |
```ruby | ||
# config.ru | ||
require './app' | ||
require 'localhost/authority' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you gem 'localhost', require: 'localhost/authority'
? Does that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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... :)
@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? |
@nateberkopec sure. Thank you for giving me chance. I will let you know this weekend |
I released an update to localhost gem so |
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>
Description
Add automatic SSL certificate provisioning for localhost.
Closes #2257
Your checklist for this pull request
[ci skip]
to the title of the PR.#issue
" to the PR description or my commit messages.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 toLayout/IndentationStyle
.