Support `localhost` integration in `ssl_bind` #2711

rodzyn · 2021-09-23T20:34:42Z

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?

lib/puma/dsl.rb

nateberkopec

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

lib/puma/dsl.rb

rodzyn · 2021-10-01T17:48:01Z

@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 · 2021-10-01T20:08:16Z

lib/puma/binder.rb

            @ios[ios_len..-1].each do |i|
              addr = loc_addr_str i
-              logger.log "* #{log_msg} on ssl://#{addr}?#{uri.query}"
+              logger.log "* #{log_msg} on ssl://#{addr}?#{uri_query}"


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

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

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

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 · 2021-10-01T20:10:13Z

LGTM, tests is the only thing it really needs 👍

test/test_integration_ssl.rb

Close puma#2708 Close puma#2711 Co-authored-by: Marcin Olichwirowicz <marcin.olichwirowicz@nedap.com>

dentarg · 2021-12-11T19:02:58Z

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

Close puma#2708 Close puma#2711 Co-authored-by: Marcin Olichwirowicz <marcin.olichwirowicz@nedap.com>

Close #2708 Close #2711 Co-authored-by: Marcin Olichwirowicz <marcin.olichwirowicz@nedap.com> Co-authored-by: Marcin Olichwirowicz <marcin.olichwirowicz@nedap.com>

Close puma#2708 Close puma#2711 Co-authored-by: Marcin Olichwirowicz <marcin.olichwirowicz@nedap.com> Co-authored-by: Marcin Olichwirowicz <marcin.olichwirowicz@nedap.com>

Support localhost integratin in ssl_bind

e3a05b0

dentarg reviewed Sep 25, 2021

View reviewed changes

lib/puma/dsl.rb Outdated Show resolved Hide resolved

nateberkopec requested changes Sep 25, 2021

View reviewed changes

lib/puma/dsl.rb Outdated Show resolved Hide resolved

lib/puma/dsl.rb Outdated Show resolved Hide resolved

lib/puma/dsl.rb Outdated Show resolved Hide resolved

lib/puma/dsl.rb Outdated Show resolved Hide resolved

Feedback changes

2f41a31

nateberkopec reviewed Oct 1, 2021

View reviewed changes

Add test

1d3f775

rodzyn force-pushed the fix_ssl_bind_compatibility branch from 1215cdf to 1d3f775 Compare October 4, 2021 21:19

rodzyn requested review from dentarg and nateberkopec October 4, 2021 21:22

dentarg reviewed Oct 4, 2021

View reviewed changes

test/test_integration_ssl.rb Outdated Show resolved Hide resolved

dentarg reviewed Oct 4, 2021

View reviewed changes

test/test_integration_ssl.rb Outdated Show resolved Hide resolved

Update tests

33cc92a

nateberkopec added bug waiting-for-review Waiting on review from anyone labels Oct 7, 2021

rodzyn requested a review from dentarg October 27, 2021 20:55

dentarg changed the title ~~Support localhost integratin in ssl_bind~~ Support localhost integration in ssl_bind Oct 28, 2021

nateberkopec approved these changes Nov 1, 2021

View reviewed changes

dentarg added a commit to dentarg/puma that referenced this pull request Dec 11, 2021

Support localhost integration in ssl_bind

6fba34a

Close puma#2708 Close puma#2711 Co-authored-by: Marcin Olichwirowicz <marcin.olichwirowicz@nedap.com>

dentarg mentioned this pull request Dec 11, 2021

Support localhost integration in ssl_bind #2764

Merged

7 tasks

dentarg added a commit to dentarg/puma that referenced this pull request Dec 12, 2021

Support localhost integration in ssl_bind

3f0ba58

Close puma#2708 Close puma#2711 Co-authored-by: Marcin Olichwirowicz <marcin.olichwirowicz@nedap.com>

nateberkopec closed this Dec 12, 2021

dentarg added a commit to dentarg/puma that referenced this pull request Dec 12, 2021

Support localhost integration in ssl_bind

f2b573c

Close puma#2708 Close puma#2711 Co-authored-by: Marcin Olichwirowicz <marcin.olichwirowicz@nedap.com>

dentarg added a commit to dentarg/puma that referenced this pull request Dec 13, 2021

Support localhost integration in ssl_bind

2b75c64

Close puma#2708 Close puma#2711 Co-authored-by: Marcin Olichwirowicz <marcin.olichwirowicz@nedap.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `localhost` integration in `ssl_bind` #2711

Support `localhost` integration in `ssl_bind` #2711

rodzyn commented Sep 23, 2021 •

edited

Loading

dentarg left a comment

nateberkopec left a comment

rodzyn commented Oct 1, 2021

nateberkopec Oct 1, 2021

rodzyn Oct 2, 2021 •

edited by dentarg

Loading

dentarg Oct 28, 2021

rodzyn Oct 29, 2021 •

edited

Loading

nateberkopec commented Oct 1, 2021

dentarg commented Dec 11, 2021

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

Support localhost integration in ssl_bind #2711

Support localhost integration in ssl_bind #2711

Conversation

rodzyn commented Sep 23, 2021 • edited Loading

Description

Your checklist for this pull request

dentarg left a comment

Choose a reason for hiding this comment

nateberkopec left a comment

Choose a reason for hiding this comment

rodzyn commented Oct 1, 2021

nateberkopec Oct 1, 2021

Choose a reason for hiding this comment

rodzyn Oct 2, 2021 • edited by dentarg Loading

Choose a reason for hiding this comment

dentarg Oct 28, 2021

Choose a reason for hiding this comment

rodzyn Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

nateberkopec commented Oct 1, 2021

dentarg commented Dec 11, 2021

Support `localhost` integration in `ssl_bind` #2711

Support `localhost` integration in `ssl_bind` #2711

rodzyn commented Sep 23, 2021 •

edited

Loading

rodzyn Oct 2, 2021 •

edited by dentarg

Loading

rodzyn Oct 29, 2021 •

edited

Loading