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

fix(http): use the right protocol for proxies #386

Merged

Conversation

Max1Truc
Copy link
Contributor

Fixes #385.

Needs some testing before merging.

@Max1Truc Max1Truc force-pushed the Max1Truc/https-over-http-proxy-fix branch from f483020 to b976764 Compare August 23, 2022 13:54
@Max1Truc
Copy link
Contributor Author

Max1Truc commented Aug 23, 2022

After testing, this was too simple to be true.

At least one proxy program (Privoxy) requires the user to use the CONNECT verb.

I'm now going to use CONNECT on HTTPS requests.

@Max1Truc Max1Truc force-pushed the Max1Truc/https-over-http-proxy-fix branch from b976764 to bd0f2ea Compare August 23, 2022 14:59
@Max1Truc
Copy link
Contributor Author

I could not figure a clean way of handling https over an https proxy, but it seems to be a luasec problem.

This PR thus only changes the behavior of http over an https proxy or https over an http proxy.
For http over http the behavior was already correct.
For https over https one would need to ssl.wrap over an existing tls (not tcp) socket.

@Max1Truc Max1Truc marked this pull request as ready for review August 27, 2022 18:38
@Max1Truc
Copy link
Contributor Author

Max1Truc commented Aug 27, 2022

By the way, this is a bit weird.
I realize this would be cleaner if I submitted a PR to luasec enabling https.tcp to reuse a socket.

What do you think ? Would it be worth it ?

Edit: This would also fix the https over https problem, but this PR is probably enough for now as it fixes http over https and https over http. I also have no idea if the PR for luasec is possible.

@alerque
Copy link
Member

alerque commented Aug 27, 2022

I realize this would be cleaner if I submitted a PR to luasec enabling https.tcp to reuse a socket.
What do you think ? Would it be worth it ?

If that's the hold up to making this cleaner and it's correct behavior then yes I think it is worth a shot.

Would that by any chance be already covered in this open PR?

@Max1Truc Max1Truc force-pushed the Max1Truc/https-over-http-proxy-fix branch from 30df3f2 to 4c06772 Compare August 27, 2022 18:52
@Max1Truc
Copy link
Contributor Author

If that's the hold up to making this cleaner and it's correct behavior then yes I think it is worth a shot.

Ok, I'll have a look.

Would that by any chance be already covered in this open PR?

Sadly, it seems session reuse doesn't solve our problem.
Looking at a sample file from the PR, it looks like reusing the same TLS session across multiple TCP sockets.

@alerque
Copy link
Member

alerque commented Sep 16, 2022

Where are we at with this?

@Max1Truc
Copy link
Contributor Author

I don't have the time for now, but it needs a Pull Request to luasec to allow wrapping tcp over tcp, and after my initial research such a PR would need to use C and OpenSSL.

This Pull Request is thus in pause, waiting for an update from luasec I guess.

@alerque alerque marked this pull request as draft September 16, 2022 10:24
@alerque
Copy link
Member

alerque commented Sep 16, 2022

Okay. Let me know if there is something actionable. I don't have time to dig into luasec and contribute anything upstream at the moment, but will help facilitate this getting through when possible. Also even if it is a little messy given a "proper" fix won't be timely if there is a way to shim things on this end that isolates the temporary code in a non-breaking way but that can get people by while they wait pending proper upstream support in the future we could consider that too.

@alerque
Copy link
Member

alerque commented Nov 10, 2023

I don't have the time for now, but it needs a Pull Request to luasec to allow wrapping tcp over tcp,

The LuaSec project has recently been migrated to this org and we have a few more maintainers around to poke it and facilitate contributions if there is still something that needs to be fixed on that end of things.

Just FYI.

@Max1Truc
Copy link
Contributor Author

Thanks, I'll have a look.

@Max1Truc Max1Truc force-pushed the Max1Truc/https-over-http-proxy-fix branch from d78986f to c19b111 Compare November 13, 2023 23:52
@Max1Truc
Copy link
Contributor Author

This PR should be focused on using the right protocol while connecting to proxies, and the last force-push reflects this.

I'll handle the CONNECT verb in another PR.

I'll let you know when I'll have thoroughly tested this implementation.

@Max1Truc Max1Truc force-pushed the Max1Truc/https-over-http-proxy-fix branch from c19b111 to 8051b11 Compare November 21, 2023 21:51
@alerque alerque marked this pull request as ready for review November 22, 2023 07:12
Previously, if doing a request to http://website.example.org
 through https://proxy.example.org, luasocket wouldn't use TLS
 and vice-versa
@Max1Truc Max1Truc force-pushed the Max1Truc/https-over-http-proxy-fix branch from 8051b11 to ebe3eeb Compare November 22, 2023 21:23
@Max1Truc
Copy link
Contributor Author

Max1Truc commented Nov 22, 2023

Ok great, this PR looks good to me. Feel free to review it.

Previously, luasocket wouldn't work with anything involving tls and proxies, with this PR luasocket supports http over an https proxy. https over http* works with some proxies (e.g. tailscale in user mode) but not many, as it does not follow the standard (GET https://... instead of the expected CONNECT).

I tested using tinyproxy as a proxy, and using socat to forward tls-encrypted packets back-and-forth between luasocket and tinyproxy.

I'm not sure how to (and whether I should) create automated tests for it though. That should probably be addressed in another PR anyway.

@alerque
Copy link
Member

alerque commented Nov 22, 2023

This project has serious issues with testing and we need to work up a way to make Busted setup and tear down some server and client processes. A proxy would need to be part of that of course.

@Max1Truc
Copy link
Contributor Author

Is there an issue for adding proper tests to luasocket ?

@alerque
Copy link
Member

alerque commented Nov 23, 2023

I thought there was but I couldn't find to link in my last comment. Feel free to open one!

@alerque alerque merged commit fa69770 into lunarmodules:master Nov 23, 2023
19 checks passed
@Max1Truc Max1Truc deleted the Max1Truc/https-over-http-proxy-fix branch November 23, 2023 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Requesting HTTPS over an HTTP proxy doesn't work
2 participants