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.

Sign up for GitHub

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

Jump to bottom

x/net/proxy: how to assert the underlying type from the returned value of Dialer.Dial #25104

Closed
rhs opened this issue Apr 26, 2018 · 5 comments
Closed

x/net/proxy: how to assert the underlying type from the returned value of Dialer.Dial #25104

rhs opened this issue Apr 26, 2018 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@rhs
Copy link

rhs commented Apr 26, 2018 •
edited by agnivade

Something recent (it appears to be this commit: golang/net@61147c4#diff-431387eb7262e1cfc79b125eb8a67c60) has broken the cast to *net.TCPConn. I don't know if this is intentional or not. If it is then I'd appreciate any advice on how to accomplish what I'm trying to do without the cast.

What version of Go are you using (go version)?

go version go1.10.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/rhs/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/rhs/go"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build051890453=/tmp/go-build -gno-record-gcc-switches"

What did you do?

This is slightly complicated to setup, so bear with me here. The reproducer program is available here: https://gist.github.com/rhs/7ed17d49dff7196c3539d2d1b01f6049

# the ssh will eat a terminal
ssh -D localhost:1080 -C -N -oExitOnForwardFailure=yes <host-you-have-ssh-access-to>

curl -o repro.go https://gist.githubusercontent.com/rhs/7ed17d49dff7196c3539d2d1b01f6049/raw/bacb83282856af91dc496b2864d0559c86371284/repro.go
# the go run will eat a terminal
go run repro.go

curl localhost:1234

What did you expect to see?

Until recently this worked fine and connections were proxied:

curl localhost:1234
<!DOCTYPE html>
	<html>
	  <head>
		<meta name="viewport" content="width=device-width, initial-scale=1">
		<meta charset="utf-8">
		<title>No such app</title>
		<style media="screen">
		  html,body,iframe {
			margin: 0;
			padding: 0;
		  }
		  html,body {
			height: 100%;
			overflow: hidden;
		  }
		  iframe {
			width: 100%;
			height: 100%;
			border: 0;
		  }
		</style>
	  </head>
	  <body>
		<iframe src="//www.herokucdn.com/error-pages/no-such-app.html"></iframe>
	  </body>
	</html>

What did you see instead?

Now the cast fails with a panic:

go run repro.go 
2018/04/26 08:34:59 Listening...
2018/04/26 08:35:03 CONNECTING: [::1]:45440 to 0xc4200102f0
panic: interface conversion: net.Conn is *socks.Conn, not *net.TCPConn

goroutine 5 [running]:
main.handleConnection(0xc42000e040)
	/home/rhs/repro.go:54 +0x2fa
created by main.main
	/home/rhs/repro.go:30 +0x16b
exit status 2
@rhs rhs changed the title socks5 dialer results can no longer be cast to *net.TCPConn x/net/proxy socks5 dialer results can no longer be cast to *net.TCPConn Apr 26, 2018
@rhs rhs changed the title x/net/proxy socks5 dialer results can no longer be cast to *net.TCPConn x/net/proxy: socks5 dialer results can no longer be cast to *net.TCPConn Apr 26, 2018
@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 26, 2018
@bradfitz
Copy link
Contributor

It is true that we changed the concrete type of the returned net.Conn interface from sock5.Dialer. It now returns a *socks.Conn wrapping the underlying connection. It does this so it can add methods like BoundAddr() net.Addr.

The type does include the underlying conn,

// A Conn represents a forward proxy connection.                                                                                                                              
type Conn struct {
        net.Conn

        boundAddr net.Addr
}

... but the whole package is internal for now.

/cc @mikioh

@mikioh
Copy link
Contributor

mikioh commented Apr 27, 2018 •
edited

I don't know if this is intentional or not.

There's no guarantee that the returned interface always contains a well-known concrete type, as the package description says that "for a variety of protocols." Otherwise, we cannot implement a bit complicated stuff like SOCKSv6 over TLS 1.3 in near future.

how to accomplish what I'm trying to do without the cast.

A simple way would be using the 4th parameter of the SOCKS5 function like the following:

type sled struct { // or sledge
        tls *tls.Conn
        tcp *net.TCPConn
        udp *net.UDPConn
}

func (s *sled) Dial(network, address string) (net.Conn, error) { // satisfies proxy.Dialer interface
        switch network {
        case "tcp", "tcp4", "tcp6":
                c, err := net.Dial(network, address)
                if err != nil {
                        return nil, err
                }
                s.tcp = c.(*net.TCPConn)
                return c, nil
        }
        return nil, errors.New("not implemented yet")
}

var s sled
d, err := proxy.SOCKS5("tcp", "somewhere:1080", nil, &s)
if err != nil {
        // error handling
}
_, err = d.Dial(...)
if err != nil {
        // error handling
}
// Now you may use s.tcp safely.

PS: s/cast/type assertion/; the notation x.(T) is called a type assertion in the language specification. See https://golang.org/ref/spec#Type_assertions

@mikioh mikioh changed the title x/net/proxy: socks5 dialer results can no longer be cast to *net.TCPConn x/net/proxy: how to assert the underlying type from the returned value of SOCKS5 Apr 27, 2018
@mikioh mikioh changed the title x/net/proxy: how to assert the underlying type from the returned value of SOCKS5 x/net/proxy: how to assert the underlying type from the returned value of Dialer.Dial Apr 27, 2018
@mikioh mikioh removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 27, 2018
@mikioh
Copy link
Contributor

mikioh commented Apr 27, 2018

Also, it's fine to add both Close{Read,Write} methods to socks.Conn to allow to assert the behavior of close-read and -write, for example,

type closeReadWriter interface {
        CloseRead() error
        CloseWrite() error
}

c, err = d.Dial(...)
if err != nil {
        // error handling
}
if cc, ok := c.(closeReadWriter); ok {
        cc.CloseRead()
        cc.CloseWrite()
}

@bcmills bcmills added this to the Unreleased milestone Apr 27, 2018
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 27, 2018
@gopherbot
Copy link

Change https://golang.org/cl/110135 mentions this issue: internal/socks: add Handshake method to Dialer

@gopherbot
Copy link

Change https://golang.org/cl/110136 mentions this issue: net/http: use Handshake method of socks.Dialer

gopherbot pushed a commit that referenced this issue May 30, 2018
@cixtor
net/http: use DialWithConn method of socks.Dialer
999e230 
This change uses the DialWithConn method of socks.Dialer to ensure that
the bundled SOCKS client implementation is agnostic to the behavior and
capabilities of transport connections.

Also updates the bundled golang.org/x/net/internal/socks at git rev
7594486. (golang.org/cl/110135)

Updates #25104.

Change-Id: I87c2e99eeb857f182ea5d8ef569181d4f45f2e5d
Reviewed-on: https://go-review.googlesource.com/110136
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 May 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@bradfitz @rhs @mikioh @agnivade @bcmills @gopherbot