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

SOCKS proxy support (attempt #2) #486

Closed
wants to merge 1 commit into from
Closed

Conversation

jschneier
Copy link
Contributor

initial shot at revitalizing #284

continuation of work from discussion at #68 -- going to see about using pycurl + Tornado for tests.

@jschneier
Copy link
Contributor Author

what do you guys think about creating new SOCKSHTTPConnection and SOCKSHTTPSConnection classes vs having something like an @property is_socks sort of deal? We don't have separate classes for proxies after all or is socks fundamentally different enough to warrant separate classes? It's just another kind of proxy...

@shazow
Copy link
Member

shazow commented Oct 14, 2014

I'm generally a fan of separate *Connection classes that implement different functionality, rather than branching. Ideally if they're composeable somehow and maybe you could have a SOCKSConnection being used by HTTP(S)Connection objects?

@Anorov
Copy link

Anorov commented Oct 14, 2014

SOCKS proxies are fairly different from HTTP proxies. They operate at a lower level.

Separate classes are not strictly necessary, but I think the design makes sense. httplib.HTTPConnection.connect will have to be overridden no matter what, and classes are a simple way of doing that.

The only bottleneck for me in finishing this was getting the tests to work. Finding decent SOCKS4 and SOCKS5 daemons that worked with Tornado was a lot more difficult than I imagined. I also had all sorts of indeterministic-seeming exceptions that seemed to stem from conflicts with Tornado's and Twisted's event loops. I eventually gave up, though I think some changes were later made to the Tornado test cases that could have resolved the issues I was seeing.

If you can help set up the test servers I can rewrite the original tests (and add new ones) and ensure the code is working as intended, on top of making the integration as clean and idiomatic as possible. I think my last commit was decent but it'll need serious modification to account for some changes in connection.py and a few other places.

Edit

Actually, I might have a better idea for integrating the support. I'd appreciate it if you left this commit as it is for now (excluding any work on the tests you may be doing) while I work on a new PR.

@shazow
Copy link
Member

shazow commented Oct 14, 2014

I'm not familiar with the SOCKS protocol but FWIW we've been trying to use socket-level tests wherever possible. It may be impractical to implement subsets of a SOCKS server like this, though. It may be more feasible to find a python socks lib which implements enough of the protocol to use ad-hoc like this.

Building another test suite alongside our Tornado-based one is brutally painful, especially using Twisted (we had sooo many problems with Twisted way back when).

Note to self: Break up socket-level tests into submodules.

@jschneier
Copy link
Contributor Author

@Anorov okay will leave everything else alone. Hmm on second reading Tornado with pycurl only currently comes with support for clients while we clearly need servers for the testing. More research is required.

@schlamar
Copy link
Contributor

I think some changes were later made to the Tornado test cases that could have resolved the issues I was seeing.

I put a lot of work to make the tornado tests more stable and self explaining. Mainly:

So it's very likely you have seen one of those issues and you should definitely give it a try again. Feel free to ping me if you run in another issue.

@bpowers
Copy link

bpowers commented Jan 15, 2015

@Anorov any word on this, or link to a new PR?

@Anorov
Copy link

Anorov commented Jan 16, 2015

@bpowers I have an awful habit of writing code while neglecting to finish the tests. I have a working version with what I feel is a better design, I'll submit a PR within a week or two.

@shazow
Copy link
Member

shazow commented Jan 20, 2015

@Anorov I invite you to open a PR early and often. :) If it's not ready, just add [WIP] to the title, and we're happy to watch it evolve (and maybe give some help as it goes along).

Worst case, somebody might find it useful to bootstrap their own PR later.

@zhao-ji
Copy link

zhao-ji commented Mar 27, 2015

We do need the SOCKS proxy support!

@Lukasa
Copy link
Sponsor Contributor

Lukasa commented Mar 27, 2015

@zhao-ji Lots of people need the SOCKS proxy support, but relatively few seem to need it enough to put the work in to build it. This is at least the second pull request for this function that has stalled. =(

If you're prepared to do the work, we'll be happy to help get it into a shape that we can merge it!

@ofek
Copy link
Contributor

ofek commented May 28, 2015

how is urllib3 SOCKS support coming?

@shazow shazow changed the title initial shot at revitalizing https://github.com/shazow/urllib3/pull/284 SOCKS proxy support (attempt #2) May 28, 2015
@joehoyle joehoyle mentioned this pull request Jun 13, 2015
@JeremyRand
Copy link

Hello,

I have $100 USD that I'm interested in putting toward SOCKS support bounties. In particular, the features I'm interested in are:

  1. Built-in urllib3 support of routing HTTP and HTTPS connections through a SOCKS proxy
  2. Having the SOCKS proxy do the DNS resolution
  3. Allowing username/password authentication for the SOCKS proxy
  4. Built-in support for the above in requests once urllib3 supports them.

I'm not sure if $100 is sufficient to achieve all of the above, or what the distribution would be between those features. If a urllib3 developer can advise on how to split up the $100 between those task bounties (and on whether a bounty would even be helpful for getting this done faster), I'll happily create the relevant GitHub issues and post the bounties on BountySource.

Cheers.

@shazow
Copy link
Member

shazow commented Aug 7, 2015

@JeremyRand That's a great idea, thank you. Now that you mention it, we really should open up an umbrella issue (or a couple) to track SOCKS support that isn't a PR. Would you like to put that together? Ideally reference all the related attempts we've had so far.

In terms of splitting up the bounty, I would advise to put it all towards getting somekind of baseline SOCKS proxy support to start. It's a fairly massive task that has been stalled for a years now, so I think we should put all of our energy/finances into incentivizing the baseline to be completed before we worry about adding specific features to it.

$100 is a great start, but I suspect we'll need to raise more funds to justify somebody spending a few of weeks working on this ~full time. Once we have an umbrella issue, it would be really helpful to try and rally more people towards kickstarting the bounty towards that issue (maybe by reaching out to all the people who pinged the issue over the years, or finding corporate sponsors). I can work on this with you too. :)

@shazow
Copy link
Member

shazow commented Aug 7, 2015

Actually I'll start the issue here: #690

@Anorov
Copy link

Anorov commented Aug 9, 2015

@JeremyRand I'm not interested in the bounty, but between the PySocks library I maintain and my PR, the requirements for 1-4 have been met. Once test code has been added, it should work with no changes needed in Requests. Usage example: requests.get(url, proxies={"http": "socks5://localhost:9050"})

Please split the bounty between the other contributors, particularly anyone who helps with tests.

@shazow
Copy link
Member

shazow commented Dec 29, 2015

Obsoleted by #762

@shazow shazow closed this Dec 29, 2015
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.

None yet

10 participants