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

Add support for HTTP(S) proxies to connect() #422

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Add support for HTTP(S) proxies to connect() #422

wants to merge 4 commits into from

Conversation

aaugustin
Copy link
Member

Fix #364.

@codecov
Copy link

codecov bot commented Jun 2, 2018

Codecov Report

Merging #422 into master will decrease coverage by 1.35%.
The diff coverage is 56.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
- Coverage     100%   98.64%   -1.36%     
==========================================
  Files          29       29              
  Lines        3170     3252      +82     
  Branches      333      352      +19     
==========================================
+ Hits         3170     3208      +38     
- Misses          0       37      +37     
- Partials        0        7       +7
Impacted Files Coverage Δ
websockets/test_http.py 100% <ø> (ø) ⬆️
websockets/http.py 100% <100%> (ø) ⬆️
websockets/test_uri.py 100% <100%> (ø) ⬆️
websockets/uri.py 100% <100%> (ø) ⬆️
websockets/client.py 77.43% <33.33%> (-22.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68371dd...c9883d4. Read the comment docs.

@aaugustin
Copy link
Member Author

Most likely this doesn't close one of the two transports when the connection terminates.

@lambdalisue
Copy link

So any updates?

@aaugustin
Copy link
Member Author

Nothing more than the public comments here.

@aaugustin aaugustin changed the title Add support for proxies to connect() Add support for HTTP(S) proxies to connect() Sep 23, 2018
@aaugustin aaugustin force-pushed the master branch 2 times, most recently from afc924a to 8d51ce2 Compare May 23, 2019 20:14
@lambdalisue
Copy link

I'm really looking forward to this 👍

@kb-1000
Copy link

kb-1000 commented May 31, 2019

@aaugustin could you please update this, so that I can still use it with the latest version?

@RemiCardona
Copy link
Collaborator

Feel free to do the rebase yourself and post a link to it here, if you're really in a hurry that's probably the best course of action. Others may pitch in by testing said new branch and giving feedback on it as well. Cheers

@aaugustin
Copy link
Member Author

aaugustin commented May 31, 2019

Well, going from this PoC to a properly tested implementation that supports all cases (HTTP/HTTPS => HTTP/HTTPS) is rather boring. I haven't been motivated to do it.

Besides writing tests:

  • I'm not confident that connection termination works well.
  • I'd like to revisit the part after # Wrap socket with TLS. This ugly hack ... Perhaps there's a better solution now that websockets requires Python > 3.6.

Proxies are most common in corporate networks. If you work at a company that relies on websockets and needs this feature, please consider paying for its development.

Get in touch at aymeric.augustin at fractalideas.com if you're interested.

Before downvotes pour in, I'd like to say that paying people is a time-proven way to get them to do things they wouldn't do by themselves otherwise ;-)

@lambdalisue
Copy link

So what should we do for this feature (not PR) to be merged? Shall we continue this PR (I'm sorry but I haven't tested this yet) or start from a scratch? I'm not really familiar so I'm not sure if I can help but at least I can try.

Or buy enough ☕️ for you and wait? If it works, I think it's a fair request 👍 (but then how much? 😁)

@aaugustin
Copy link
Member Author

Python 3.7 added start_tls, which removes the need for the ugly hack.

If we use it, proxying to a WSS endpoint will require Python ≥ 3.7. I think that's fine.

@lgrahl
Copy link
Collaborator

lgrahl commented Jun 5, 2019

Agree. Less code to maintain. 🙂

@mdymike
Copy link

mdymike commented Nov 12, 2019

Hi guys... I had to make proxies work with websockets, where the proxy server required NTLM authentication (just to make things interesting)... My fork is here with explanatory notes added to README: https://github.com/mdymike/websockets

Hopefully someone finds that useful. I don't feel it's polished enough yet to do a pull request so just putting it out there 'as-is' for now :)

Best, Mike

@andrewshadura
Copy link

@aaugustin, hi, I don’t want to bug you with this too much, but now that start_tls is available and Python 3.8 is already around, maybe you can find some time to have a look at this again?

Thanks in advance, and I hope you stay safe in this pandemic.

@Loffe
Copy link

Loffe commented Apr 21, 2020

Hi guys... I had to make proxies work with websockets, where the proxy server required NTLM authentication (just to make things interesting)... My fork is here with explanatory notes added to README: https://github.com/mdymike/websockets

Hopefully someone finds that useful. I don't feel it's polished enough yet to do a pull request so just putting it out there 'as-is' for now :)

Best, Mike

I tested the fork by Mike and can confirm that it works behind a corporate proxy 😄 👍
(I did not test the NTLM authentication though)

It would be nice to see proxy support in the official websockets library.

@andrewshadura
Copy link

I have forward-ported this pull request to the current websockets code using start_tls; I will submit a pull request superseding this one soon.

@andrewshadura andrewshadura mentioned this pull request Apr 24, 2020
5 tasks
@aaugustin aaugustin marked this pull request as draft July 26, 2020 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for HTTP(S) proxies to connect()
8 participants