Support additional headers for TLS proxies

[jjiang-stripe]

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.

[geemus]

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

[geemus]

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?

[jjiang-stripe]

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

[geemus]

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?

[jjiang-stripe]

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

[jjiang-stripe]

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]

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

[jjiang-stripe]

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]

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]

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

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]

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]

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]

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

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

[geemus]

Released in v0.81.0.