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

proposal: x/net/internal/socks, socks_bundle.go in net/http: new package #19688

Closed
mikioh opened this issue Mar 24, 2017 · 13 comments
Closed

proposal: x/net/internal/socks, socks_bundle.go in net/http: new package #19688

mikioh opened this issue Mar 24, 2017 · 13 comments

Comments

@mikioh
Copy link
Contributor

mikioh commented Mar 24, 2017

The commit f6698cf introduced the golang.org/x/net/proxy package into the standard library. At the same time, it also carried #19354. As far as I can see, the net/http package only requires a SOCKS client implementation, not a generic forward proxy client implementation. Moreover, the existing/introduced proxy package has several issues that will cause future troubleshooting difficulties like #11682.

I'd like to propose to do the following during Go 1.9 development cycle:

  1. factor out SOCKS client implementation code from x/net/proxy and make it a new package
  2. fix issues like x/net/proxy: does not support dialing with a given Context #19354 and x/net/proxy: SOCKS5 errors are all *errors.errorString #11682 in the new package
  3. replace vendored x/net/proxy with the new package
@mikioh mikioh added this to the Go1.9 milestone Mar 24, 2017
@gopherbot
Copy link

CL https://golang.org/cl/38278 mentions this issue.

@bradfitz
Copy link
Contributor

Do you propose breaking people currently using https://godoc.org/golang.org/x/net/proxy#SOCKS5 ?

@rsc
Copy link
Contributor

rsc commented Mar 27, 2017

Two issues:

  1. Make it easier to pull socks bug fixes into stdlib. That would be possible with no API changes by creating x/net/proxy/internal/socks and using it from x/net/proxy.

  2. Proxy doesn't have context support, and adding it could break existing users.

I think we should do (1) but not expose new socks details to users unless and until they need them.

@rsc
Copy link
Contributor

rsc commented Mar 27, 2017

And (2) can be a different issues.

@mikioh
Copy link
Contributor Author

mikioh commented Mar 27, 2017

Do you propose breaking people currently using ...

I don't understand your question. I didn't say anything about the package x/net/proxy API, also CL 38278 doesn't break the existing applications. What's the purpose of this question?

creating x/net/proxy/internal/socks

I'm fine with this.

@mikioh
Copy link
Contributor Author

mikioh commented Mar 30, 2017

Are there other opinions on preferring x/net/internal/socks than x/net/socks pacakge? I'm really fine either way because what I want to see is a) no troublesome for the use of http over socks, b) a concrete socks client implementation package for my use case.

@bradfitz
Copy link
Contributor

I'm okay with socks (not internal) if it can start minimal, and if it has package docs saying that it has no compatibility promises.

@rsc
Copy link
Contributor

rsc commented Mar 30, 2017

Let's just do internal please, until someone actually needs socks exposed. The new package in CL 38278 has an enormous amount of API for a trivial thing. I'd really like to keep the trivial thing trivial.

@mikioh
Copy link
Contributor Author

mikioh commented Mar 30, 2017

Sorry, what you are saying "trivial stuff" is important for my use case; especially accepting bind command and external authentication mechanisms, and notifying a server bound address on a proxy connection because the value of SOCKS to me is that it glues transport protocols together. I'd want to use x/net/internal/socks by vendoring, but if you dislike that approach, I'm happy to drop this proposal and CL 38278.

@bradfitz
Copy link
Contributor

So you're saying it can't be minimal.

Internal it is.

@bradfitz
Copy link
Contributor

(But I will still say no to unnecessary stuff like "bind" for now.)

@rsc
Copy link
Contributor

rsc commented Apr 3, 2017

It sounds like there are details to work out in the CL but we agree that x/net/socks should exist.

@mikioh mikioh changed the title proposal: x/net/socks, vendor/golang_org/x/net/socks: new package proposal: x/net/internal/socks, vendor/golang_org/x/net/socks: new package Apr 8, 2017
@gopherbot
Copy link

CL https://golang.org/cl/41031 mentions this issue.

@mikioh mikioh modified the milestones: Go1.10, Go1.9 Jun 14, 2017
@rsc rsc modified the milestones: Go1.10, Unreleased Nov 29, 2017
@mikioh mikioh changed the title proposal: x/net/internal/socks, vendor/golang_org/x/net/socks: new package proposal: x/net/internal/socks, socks_bundle.go in net/http: new package Mar 30, 2018
gopherbot pushed a commit to golang/net that referenced this issue Apr 6, 2018
This change factors out the code related to SOCKS protocol version 5
from the golang/x/net/proxy package and provides new SOCKS-specific
API to fix the following:
- inflexbility of forward proxy connection setup; e.g., no support for
  context-based deadline or canceling, no support for dial deadline,
  no support for working with external authentication mechanisms,
- useless error values for troubleshooting.

The new package socks is supposed to be used by the net/http package
of standard library and proxy package of golang.org/x/net repository.

Fixes golang/go#11682.
Updates golang/go#17759.
Updates golang/go#19354.
Updates golang/go#19688.
Fixes golang/go#21333.

Change-Id: I24098ac8522dcbdceb03d534147c5101ec9e7350
Reviewed-on: https://go-review.googlesource.com/38278
Run-TryBot: Mikio Hara <mikioh.mikioh@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Apr 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants