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.

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

Support additional headers for TLS proxies #742

Merged
merged 8 commits into from Apr 24, 2021
Merged

Support additional headers for TLS proxies #742

merged 8 commits into from Apr 24, 2021

Conversation

@jjiang-stripe
Copy link
Contributor

@jjiang-stripe jjiang-stripe commented Apr 14, 2021

Fixes #710

I don't write a whole lot of Ruby, so feel free to ask many questions/correct me if things look wonky!

This adds a new ssl_proxy_headers option to the connection parameters that can be used to specify headers to send on the proxy CONNECT request. I added it as a separate parameter, since @data[:proxy] is typically used as a string value, and while users could build their own @data[:proxy] hash, it seems a little cumbersome to use properly.

Copy link
Contributor

@geemus geemus left a comment

@jjiang-stripe Thanks for your help! I like the separate key in the config, as I definitely think that seems cleaner/safer. I've added some areas for discussion and possible refinement, and don't think we are too far off from a great addition. Thanks!

@@ -104,6 +104,12 @@ def initialize(data = {})

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

if @data[:proxy][:headers]

This comment has been minimized.

@geemus

geemus Apr 15, 2021
Contributor

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?

This comment has been minimized.

@jjiang-stripe

jjiang-stripe Apr 15, 2021
Author Contributor

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

@@ -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

This comment has been minimized.

@geemus

geemus Apr 15, 2021
Contributor

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?

This comment has been minimized.

@jjiang-stripe

jjiang-stripe Apr 15, 2021
Author Contributor

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

@jjiang-stripe
Copy link
Contributor Author

@jjiang-stripe jjiang-stripe commented Apr 17, 2021

okay updated the @data parameters and also moved the header hash to string parsing into a helper function!

I wasn't able to get the test set up properly working with the proxy CONNECT request though. it seems like Sinatra does not like CONNECT requests :( I pushed my (failing) wip tests in case you have any ideas on how I can fix them.

@geemus
Copy link
Contributor

@geemus geemus commented Apr 17, 2021

@jjiang-stripe Looks great overall, and I appreciate you extracting the shared code to utils for the header writing.

I'll have to research proxy testing more, I recall us having some challenges there before, and don't know the answer off hand.

This reverts commit 3a9dba4.
@jjiang-stripe
Copy link
Contributor Author

@jjiang-stripe jjiang-stripe commented Apr 22, 2021

So I've found that adding "--env", "production" to this line makes the proxy CONNECT succeed in the test (it was erroring with env missing required key SERVER_NAME, so seems like we were missing some env variables for rack?)

Then the test ends up failing with an SSL error SSL_connect returned=1 errno=0 state=error: wrong version number (OpenSSL::SSL::SSLError) (Excon::Error::Socket) since we're not actually proxying anything with our mock proxy (nor do we really want to write a full http connect proxy here 😅 ). I'm not sure if there's really a way to get around this unfortunately. I've reverted my test commit for now, if you're open to just merging this without tests 😬

@jjiang-stripe
Copy link
Contributor Author

@jjiang-stripe jjiang-stripe commented Apr 22, 2021

oof just uncovered an unexpected consequence of using [:ssl_proxy_headers] vs [:proxy][:headers]. my actual use case is setting [:ssl_proxy_headers] in a middleware. with using [:ssl_proxy_headers] on the middleware, it's only set on the datum and not on @data by the time we initialize the socket here.

It was working when we were using [:proxy][:headers] since the [:proxy] hash was shared between datum and @data due to the @data.merge over here. since [:ssl_proxy_headers] is used in the ssl socket's initialize, the socket.data = datum line here doesn't end up writing [:ssl_proxy_headers] to the socket's @data until after it's already been initialized.

there's a couple options here, but i'm not sure what's the best:

  1. go back to using [:proxy][:headers]
  2. make the socket function take datum as a parameter instead of using @data (which feels like maybe the original intent of the socket.data = datum line?)
  3. explicitly overwrite @data[:ssl_proxy_headers] = datum[:ssl_proxy_headers] before intializing the socket

what do you think?

@geemus
Copy link
Contributor

@geemus geemus commented Apr 23, 2021

@jjiang-stripe thanks for the ongoing work here.

For the time being, I think having it with fewer tests is probably ok. I think I see some possibility for using the webrick proxy server class to actually test all of this, but I don't think we need to block on that, as I suspect it will be a little while before I get to it.

I hadn't considered the data vs datum issue, so thanks for finding that and considering the options. I'm not sure we've had a case previously where middlewares impacted socket settings, so it may have just not come up before.

From those options, I think adding an argument to socket which takes a datum sounds like a good approach to solve this issue and also make things a little more consistent in usage and expectations. Does that sound good to you?

@jjiang-stripe
Copy link
Contributor Author

@jjiang-stripe jjiang-stripe commented Apr 23, 2021

I was leaning towards the socket argument option too! I just updated the code and verified it's working with our middleware (setting [:ssl_proxy_headers] in the middleware properly sets it on the CONNECT request that I'm logging on the proxy server). I'm a little confused why it appears we're initializing a new socket for each socket.write call (maybe I'm reading it wrong? ruby confuses me 😅), but I kept it the same to be safe and passed the datum into each call.

@geemus
Copy link
Contributor

@geemus geemus commented Apr 24, 2021

I believe it is reusing sockets, but it's easy to miss. The socket method has an ||= in it which should either use the existing socket, or set one if it doesn't yet exist. Because of that, unless the socket_key changes it should reuse the socket (at least within that thread).

@geemus
Copy link
Contributor

@geemus geemus commented Apr 24, 2021

Also, fwiw, here is a quick issue I just created around improved proxy testing with some of the more promising candidates: #746

I hope to return to it at some point, but haven't had a lot of time for development lately, so we'll see.

@geemus geemus merged commit 7c154c1 into excon:master Apr 24, 2021
5 checks passed
5 checks passed
@github-actions
test (2.4)
Details
@github-actions
test (2.5)
Details
@github-actions
test (2.6)
Details
@github-actions
test (2.7)
Details
@github-actions
test (3.0)
Details
@geemus
Copy link
Contributor

@geemus geemus commented Apr 24, 2021

@jjiang-stripe Thanks! Just let me know if you have other questions or suggestions. I'll try to get a release out soon (maybe early in the week, as I prefer not to do it on the weekend when I'd be less likely to notice if something goes wrong with it).

@jjiang-stripe
Copy link
Contributor Author

@jjiang-stripe jjiang-stripe commented Apr 26, 2021

oh nice I see that bit for reusing sockets now. thanks for all your help here! 😁

@jjiang-stripe jjiang-stripe deleted the jjiang-stripe:jjiang/ssl-proxy-headers branch Apr 26, 2021
@geemus
Copy link
Contributor

@geemus geemus commented Apr 28, 2021

Released in v0.81.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants