Support for cert_pem and key_pem with ssl_bind DSL

[dalibor]

Description

We need a way to specify cert and key objects or PEM strings in Puma configuration without relying on file paths. The use-case is when deploying to cloud provider and fetching certificates from Secrets Manager on application boot-up to avoid persisting the certificates on disk for security reasons.

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.
  • This PR is more than 100 lines, but it all comes together to support this feature
• 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.

[MSP-Greg]

@dalibor

Thanks for working on this. I've got a few changes to the UNIXSocket handling that fix the issues with it, problem being that URI.parse doesn't handle them as expected.

For a long time, Puma's 'MiniSSL' code has not required that Ruby's OpenSSL is loaded. We actually have a test that checks for that, one of the tests that start Puma as a sub-process.

Question - At present the code is passing objects, created via Ruby's OpenSSL code. Haven't worked much with 'Secrets Manager', but could the code pass in strings, rather than objects?

Lastly, the current code is making some API changes, which might mean waiting until Puma 6.0. Not sure.

[dalibor]

@MSP-Greg Thanks for your help, I've cherry-picked your fix for UNIXSocket handling.

I changed the code to pass the cert_pem and key_pem so we don't require Ruby's OpenSSL in MiniSSL extension code. From AWS Secrets Manager, we get Ruby strings, so passing strings works too.

Let me know what I can do to make this change more backward compatible if we can get it out sooner than 6.0. Thanks!

[MSP-Greg]

@dalibor

Thanks for the update. This looks good, and it also seems to restart fine. A long time I thought it would be helpful, but was busy with other things. The update also fixed the Windows CI, which was due to Windows Ruby being built differently.

As to the API, it's a problem with Puma, as a lot of things have never been marked as private (either by actually doing so or comments). There are also some things that have been done just to make CI easier, but not noted as such.

A while ago I cleaned up a pattern that I don't like:

t = SomeClass.new opts
t.x = opts.x
t.y = opts.y
t.run

Well, that broke code in capybara. This code will not break that, but it does change the Binder#listeners signature.

Anyone have any thoughts re the API change?

[dalibor]

@MSP-Greg Thanks for your help! I've reverted the signature change to Binder#listeners since that is not required to support this feature.

I did some testing and I'm finding that the puma process title is not as expected as well as the bind_to_activated_sockets option will not work as expected (they are both in lib/puma/launcher.rb and depend on @options[:binds] internals).

I'm trying to avoid putting the cert/key pem strings in URI and I have an idea how to do it. I'll add an internal store config option that the ssl scheme binder will read the cert/key values from while preserving the cert_pem and key_pem DSL for the end-user. That way we'll also preserve all of the existing APIs.

[dentarg]

Excellent work here @dalibor! 💫 Thank you!

[dalibor]

@dentarg @MSP-Greg - What do you think about this approach? I think we're preserving all existing APIs now?

Initially, I was hoping we could pass the Ruby's OpenSSL objects and nullify the original cert/key strings from memory, but knowing that Ruby's OpenSSL will still keep them in memory this is still an improvement over writing the cert on disk.

Let me know what you think and I'll create a new PR with clean history and reference this one for the discussions. Thanks!

[nateberkopec]

Is this necessary to change?

[MSP-Greg]

I recall that assert_equal throws a warning ('use assert_nil') if one of the items are null/nil. Not sure about this case.

[dalibor]

Yep - this was the warning:

DEPRECATED: Use assert_nil if expecting nil from test/test_binder.rb:265. This will fail in Minitest 6.

[MSP-Greg]

What do you think about this approach?

I was fine with the old one. I'm AFK for a while today, and I do have to work on another OS repo. I'll look over the changes soon, but if others think it's ok, then I am too. Thanks. This is a good feature.

[dalibor]

Cool - I like that with the new approach we're preserving all existing APIs which should help get this out sooner than later. Thanks!

[dalibor]

I have a new branch cert_pem extracting only the final changes from this PR to avoid overriding the history unnecessarily. Let me know if you want me to force push that over this branch or close this PR and open a new one? Also, if there's anything else I can do here. I think all the comments so far has been addressed?

Thanks!

[dentarg]

We can squash merge this, it will keep master history clean while preserving the PR history

[dnasevic-godaddy]

Hello - any idea when we could have this merged and released? Thanks!

[dentarg]

We can squash merge this, it will keep master history clean while preserving the PR history

Actually, I'll go back on this. I like the clear history of your branch with 4 commits instead of the 19 commits we have here. Maybe a new PR with that gives the most clarity? The new PR could reference this one.

[MSP-Greg]

@dnasevic-godaddy I looked at this this morning, thought I'd look again tonite. In the middle of misc things, including OpenSSL 3.0.0 fun...

[dalibor]

@dentarg I created a new PR with clean history, I'm closing this one.

@MSP-Greg Thank you, whenever you get the time!

I noticed there a failing test in master for ubuntu-20.04 truffleruby-head that I noted in the other PR description.

New PR is here: #2728

Thank you all!