Support localhost integration in ssl_bind

[rodzyn]

Description

Hello, here is my attempt to fix #2708

Closes #2708

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.

[dentarg]

Is it possible to add a test for this?

[nateberkopec]

Thank you for your contribution! Agreed with all @dentarg's points and added a few of my own.

[rodzyn]

@nateberkopec @dentarg
I made some more bold changes in this approach. It's more of a draft for now so the code is messy but I just wanted to double-check if that might be a good direction to follow.

It still needs some additional tests to be added (I only tested it on my local example-apps)

[nateberkopec]

Why change this? Looks to me like uri.query is still available?

[rodzyn]

So uri.query as uri comes from

puma/lib/puma/dsl.rb

Lines 66 to 67 in 20dc923

	"ssl://#{host}:#{port}?cert=#{opts[:cert]}&key=#{opts[:key]}" \
	"#{ssl_cipher_filter}&verify_mode=#{verify}#{tls_str}#{ca_additions}#{v_flags}"

(where there is no possibility to detect localhost gem, that was in my try in the first attempt) so it gets the URI with empty key and cert values (&key=&cert=).
These empty values are transformed to Hash and stored in params variable here

puma/lib/puma/binder.rb

Line 230 in 20dc923

params = Util.parse_query uri.query

, and then the current implementation is transforming them (default localhost gem paths). In the end only params is "enriched" by new values, thus I'm transforming it back from Hash to String

[dentarg]

In the end only params is "enriched" by new values

I think we should have a comment about this somewhere, to help future readers understand that directly (I didn't know either), maybe where uri_query is defined or where params is passed to MiniSSL::ContextBuilder.new. WDYT?

Just mentioning this, not sure what we can do right now about it: there's another PR (#2728) changing code related to this, see https://github.com/puma/puma/pull/2728/files#diff-b771cc8da488f9700644eadd48510a23b1c2d5a7776dbc983eae1d2338536fd7. If I understand things correctly, with both these changes as-is, we would be logging cert_pem and cert_key if you are using the functionality added in #2728. I guess it would be up to the PR that's merged last to deal with that :) cc @dalibor

[rodzyn]

Thank @dentarg for bringing up #2728, it looks like really nice work, so I'll probably wait for that to be merged, and rebase that PR on these changes. I'm not sure if that will fix this workaround here, I'll need to dive again into this ;)

[nateberkopec]

LGTM, tests is the only thing it really needs 👍

[dentarg]

@rodzyn After #2728 this only needed a small change so I made it in #2764 and added you as co-author