Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support additional headers for TLS proxies #742

Merged
merged 8 commits into from Apr 24, 2021
Merged

Support additional headers for TLS proxies #742

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4a09e0f
Support additional headers for TLS proxies
jjiang-stripe Apr 14, 2021
7e660c7
Move test
jjiang-stripe Apr 14, 2021
654a6a3
use :ssl_proxy_headers directly
jjiang-stripe Apr 15, 2021
69d6167
use helper method to convert headers
jjiang-stripe Apr 17, 2021
3a9dba4
wip test updates
jjiang-stripe Apr 17, 2021
1dd844b
Revert "wip test updates"
jjiang-stripe Apr 22, 2021
1e5a5eb
remove test
jjiang-stripe Apr 22, 2021
b415556
pass datum to socket
jjiang-stripe Apr 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/excon/connection.rb
View file
Open in desktop
Expand Up @@ -567,6 +567,12 @@ def setup_proxy
:port => uri.port,
:scheme => uri.scheme,
}
if @data[:ssl_proxy_headers]
if !@data[:ssl_uri_schemes].include?(@data[:scheme])
raise ArgumentError, "The `:ssl_proxy_headers` parameter should only be used with HTTPS requests."
end
@data[:proxy][:headers] = @data.delete(:ssl_proxy_headers)
end
if uri.password
@data[:proxy][:password] = uri.password
end
Expand Down
1 change: 1 addition & 0 deletions lib/excon/constants.rb
View file
Open in desktop
Expand Up @@ -99,6 +99,7 @@ module Excon
:ssl_version,
:ssl_min_version,
:ssl_max_version,
:ssl_proxy_headers,
:ssl_uri_schemes,
:tcp_nodelay,
:thread_safe_sockets,
Expand Down
6 changes: 6 additions & 0 deletions lib/excon/ssl_socket.rb
View file
Open in desktop
Expand Up @@ -104,6 +104,12 @@ def initialize(data = {})

request += "Proxy-Connection: Keep-Alive#{Excon::CR_NL}"

if @data[:proxy][:headers]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the approach of using a separate key of ssl_proxy_headers makes a lot of sense, especially as proxy is generally expected to be a string (though the code is tolerant of a hash, like you suggest, I doubt it gets used that way most of the time).

That being said, if we are already using a separate key, I wonder if it might be simpler/cleaner if we just leave that key/value as it is (instead of translating it to be inside the proxy hash) and just read directly from it here when we are setting up the ssl socket?

Does that make sense? I certainly might be missing something, but on review it occurred to me that sort of "cutting out the middle person" here and going directly to the hash could keep things simpler coding wise and avoid any potential issues of the mutated proxy hash being interpreted incorrectly elsewhere.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes that makes total sense! I'll update in a bit

@data[:proxy][:headers].each do |key, value|
request << key.to_s << ': ' << value.to_s << Excon::CR_NL
end
end

request += Excon::CR_NL

# write out the proxy setup request
Expand Down
6 changes: 5 additions & 1 deletion tests/proxy_tests.rb
View file
Open in desktop
Expand Up @@ -88,7 +88,7 @@ def env_proxy_tests(env)
connection = nil

tests('connection.data[:proxy][:host]').returns('mysecureproxy') do
connection = Excon.new('https://secret.com')
connection = Excon.new('https://secret.com', :ssl_proxy_headers => {'x-proxy-id': 'abc123' })
connection.data[:proxy][:host]
end

Expand All @@ -100,6 +100,10 @@ def env_proxy_tests(env)
connection.data[:proxy][:scheme]
end

tests('connection.data[:proxy][:headers]').returns({ 'x-proxy-id': 'abc123' }) do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are good for ensuring the data mutation into proxy happens as expected (unless we decide not to do that).

I think it would be great to also add a test toward the end of this file, where it uses the proxy.ru rackup. These tests actually run a server, which receives proxy stuff and echos it back (so you can be more confident it was sent as expected). I think it would be great to also test the roundtrip on these proxy settings, though this would require updating proxy.ru as well. I think maybe we could update it so the response passes back the value of x-proxy-id and check the response headers to gain this confidence.

Does that make sense? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep! always happy to write more tests 😁 I'll dig into that

connection.data[:proxy][:headers]
end

tests('with disable_proxy set') do
connection = nil

Expand Down