code review 6038047: go.crypto/ssh: add support for remote tcpip forwarding

go.crypto/ssh: add support for remote tcpip forwarding

Add support for server (remote) forwarded tcpip channels.
See RFC4254 Section 7.1

[gpaul, 2012-04-16 20:46:01]

Works for me. I haven't driven anything fast through it but I tested the basic
functionality and it works.

http://codereview.appspot.com/6038047/diff/5003/ssh/tcpip.go
File ssh/tcpip.go (right):

http://codereview.appspot.com/6038047/diff/5003/ssh/tcpip.go#newcode50
ssh/tcpip.go:50: false, // no reply, assume it works
is that a safe assumption?

http://codereview.appspot.com/6038047/diff/5003/ssh/tcpip.go#newcode92
ssh/tcpip.go:92: l.forwards[i] = l.forwards[len(l.forwards)]
shouldn't this be l.forwards[len(l.forwards)-1] ?

[dfc, 2012-04-17 11:59:48]

PTAL

http://codereview.appspot.com/6038047/diff/5003/ssh/tcpip.go
File ssh/tcpip.go (right):

http://codereview.appspot.com/6038047/diff/5003/ssh/tcpip.go#newcode50
ssh/tcpip.go:50: false, // no reply, assume it works
Mostly harmless.

This comment relates to the TODO at the top of the file, if the port is 0, then
a reply message will contain the actual port chosen by the remote side. To
support that, I need to support the reply message first.

On 2012/04/16 20:46:01, gpaul wrote:
> is that a safe assumption?

http://codereview.appspot.com/6038047/diff/5003/ssh/tcpip.go#newcode92
ssh/tcpip.go:92: l.forwards[i] = l.forwards[len(l.forwards)]
Yup - barf worthy. I'll rewrite it.

On 2012/04/16 20:46:01, gpaul wrote:
> shouldn't this be l.forwards[len(l.forwards)-1] ?

[dfc, 2012-04-21 22:37:27]

Hello gustav.paul@gmail.com, jeff@somethingsimilar.com (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.crypto

[agl1, 2012-04-23 15:12:04]

http://codereview.appspot.com/6038047/diff/22001/ssh/client.go
File ssh/client.go (right):

http://codereview.appspot.com/6038047/diff/22001/ssh/client.go#newcode278
ssh/client.go:278: return
send channelOpenFailureMsg back?

http://codereview.appspot.com/6038047/diff/22001/ssh/client.go#newcode313
ssh/client.go:313: func decodeAddr(b []byte) (*net.TCPAddr, error) {
Needs a comment, probably referencing an RFC section.

Also, since the caller ignores the contents of the error, maybe just return a
bool for that?

http://codereview.appspot.com/6038047/diff/22001/ssh/client.go#newcode322
ssh/client.go:322: ip, err := net.ResolveIPAddr("tcp", string(addr))
addr must be an IP address? If so, net.ParseIP may be better. ResolveIPAddr also
disallows "tcp" as a value of the net argument.

http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go
File ssh/tcpip.go (right):

http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go#newcode67
ssh/tcpip.go:67: // forwardlist stores a mappin between remote
typo: mapping.

http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go#newcode76
ssh/tcpip.go:76: c chan struct {
this struct has been written a few times - it probably deserves a name.

http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go#newcode88
ssh/tcpip.go:88: f := forward{
Check for duplicates before appending? Otherwise Remove goes wrong.

[dfc, 2012-04-23 22:01:36]

Thanks for your comments agl, I will address them this evening. 

On 24/04/2012, at 1:12, agl@golang.org wrote:

> 
> http://codereview.appspot.com/6038047/diff/22001/ssh/client.go
> File ssh/client.go (right):
> 
> http://codereview.appspot.com/6038047/diff/22001/ssh/client.go#newcode278
> ssh/client.go:278: return
> send channelOpenFailureMsg back?
> 
> http://codereview.appspot.com/6038047/diff/22001/ssh/client.go#newcode313
> ssh/client.go:313: func decodeAddr(b []byte) (*net.TCPAddr, error) {
> Needs a comment, probably referencing an RFC section.
> 
> Also, since the caller ignores the contents of the error, maybe just
> return a bool for that?
> 
> http://codereview.appspot.com/6038047/diff/22001/ssh/client.go#newcode322
> ssh/client.go:322: ip, err := net.ResolveIPAddr("tcp", string(addr))
> addr must be an IP address? If so, net.ParseIP may be better.
> ResolveIPAddr also disallows "tcp" as a value of the net argument.
> 
> http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go
> File ssh/tcpip.go (right):
> 
> http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go#newcode67
> ssh/tcpip.go:67: // forwardlist stores a mappin between remote
> typo: mapping.
> 
> http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go#newcode76
> ssh/tcpip.go:76: c chan struct {
> this struct has been written a few times - it probably deserves a name.
> 
> http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go#newcode88
> ssh/tcpip.go:88: f := forward{
> Check for duplicates before appending? Otherwise Remove goes wrong.
> 
> http://codereview.appspot.com/6038047/

[lieqie, 2012-04-24 04:48:16]

It doesn't open the specified port in fact.
I think in this case should need a 
net.Dial("tcp", "127.0.0.1:8080") to test it.

http://codereview.appspot.com/6038047/diff/22001/ssh/example_test.go
File ssh/example_test.go (right):

http://codereview.appspot.com/6038047/diff/22001/ssh/example_test.go#newcode141
ssh/example_test.go:141: l, err := conn.Listen("tcp", "0.0.0.0:8080")
It doesn't open the specified port in fact.
I think in this case should need a 
net.Dial("tcp", "127.0.0.1:8080") to test it.

os:win7 msys go:go1

[dfc, 2012-04-24 12:40:55]

Thank you for your comments.

http://codereview.appspot.com/6038047/diff/22001/ssh/client.go
File ssh/client.go (right):

http://codereview.appspot.com/6038047/diff/22001/ssh/client.go#newcode278
ssh/client.go:278: return
Done. I have no idea why I did not do that, I do later on in the method.

On 2012/04/23 15:12:04, agl1 wrote:
> send channelOpenFailureMsg back?

http://codereview.appspot.com/6038047/diff/22001/ssh/client.go#newcode313
ssh/client.go:313: func decodeAddr(b []byte) (*net.TCPAddr, error) {
Added, and I use the error in the failure message.

On 2012/04/23 15:12:04, agl1 wrote:
> Needs a comment, probably referencing an RFC section.
> 
> Also, since the caller ignores the contents of the error, maybe just return a
> bool for that?

http://codereview.appspot.com/6038047/diff/22001/ssh/client.go#newcode322
ssh/client.go:322: ip, err := net.ResolveIPAddr("tcp", string(addr))
On 2012/04/23 15:12:04, agl1 wrote:
> addr must be an IP address? If so, net.ParseIP may be better. ResolveIPAddr
also
> disallows "tcp" as a value of the net argument.

Although net.Conn#RemoteAddr and friends return net.Adds', they dont' define
equality so are hard to use internally. Also, this type of tunneled connection
is only useful for stream connections, so *net.TCPAddr makes sense here. 

Having said that, I don't want to try to resolve it, as it's already an IP, in
string form, not a name, so you are correct that ParseIP is a better choice.

http://codereview.appspot.com/6038047/diff/22001/ssh/example_test.go
File ssh/example_test.go (right):

http://codereview.appspot.com/6038047/diff/22001/ssh/example_test.go#newcode141
ssh/example_test.go:141: l, err := conn.Listen("tcp", "0.0.0.0:8080")
On 2012/04/24 04:48:16, lieqie wrote:
> It doesn't open the specified port in fact.
> I think in this case should need a 
> net.Dial("tcp", "127.0.0.1:8080") to test it.

This is just an example, all the test harness guarantees is that it compiles. Is
that what you meant ?

http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go
File ssh/tcpip.go (right):

http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go#newcode67
ssh/tcpip.go:67: // forwardlist stores a mappin between remote
On 2012/04/23 15:12:04, agl1 wrote:
> typo: mapping.

Done.

http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go#newcode76
ssh/tcpip.go:76: c chan struct {
On 2012/04/23 15:12:04, agl1 wrote:
> this struct has been written a few times - it probably deserves a name.

Yup - it was an interesting experiment with anon structs, but clearly neither
the time nor the place.

http://codereview.appspot.com/6038047/diff/22001/ssh/tcpip.go#newcode88
ssh/tcpip.go:88: f := forward{
On 2012/04/23 15:12:04, agl1 wrote:
> Check for duplicates before appending? Otherwise Remove goes wrong.

Is it possible to have a duplicate ? That would imply that the remote end had
accepted two connections with the same IP:Port raddr.

[dfc, 2012-04-24 13:04:43]

Hello gustav.paul@gmail.com, jeff@somethingsimilar.com, agl@golang.org,
lieqiewang@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[agl1, 2012-04-24 15:03:30]

LGTM

http://codereview.appspot.com/6038047/diff/29003/ssh/client.go
File ssh/client.go (right):

http://codereview.appspot.com/6038047/diff/29003/ssh/client.go#newcode304
ssh/client.go:304: l <- struct {
This has a name now, right?

http://codereview.appspot.com/6038047/diff/29003/ssh/tcpip.go
File ssh/tcpip.go (right):

http://codereview.appspot.com/6038047/diff/29003/ssh/tcpip.go#newcode76
ssh/tcpip.go:76: *net.TCPAddr
I worry that this is excessive embedding. By not naming it, it's not clear what
this is the address of. (The server's local address?)

http://codereview.appspot.com/6038047/diff/29003/ssh/tcpip.go#newcode106
ssh/tcpip.go:106: func (l *forwardlist) Lookup(addr *net.TCPAddr) (chan
forwardEntry, bool) {
forwardList?

http://codereview.appspot.com/6038047/diff/29003/ssh/tcpip.go#newcode129
ssh/tcpip.go:129: return &tcpchanconn{
tcpChanConn?

http://codereview.appspot.com/6038047/diff/29003/ssh/tcpip.go#newcode130
ssh/tcpip.go:130: tcpchan: &tcpchan{
tcpChan?

[dfc, 2012-04-25 03:14:34]

Hello gustav.paul@gmail.com, jeff@somethingsimilar.com, agl@golang.org,
lieqiewang@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dfc, 2012-04-25 03:16:06]

Thank you for your comments. I've rejigged the forward/forwardList/ForwardEntry
to make a little more sense, and remove the excessive embedding.

http://codereview.appspot.com/6038047/diff/29003/ssh/client.go
File ssh/client.go (right):

http://codereview.appspot.com/6038047/diff/29003/ssh/client.go#newcode304
ssh/client.go:304: l <- struct {
On 2012/04/24 15:03:30, agl1 wrote:
> This has a name now, right?

Done.

http://codereview.appspot.com/6038047/diff/29003/ssh/tcpip.go
File ssh/tcpip.go (right):

http://codereview.appspot.com/6038047/diff/29003/ssh/tcpip.go#newcode76
ssh/tcpip.go:76: *net.TCPAddr
On 2012/04/24 15:03:30, agl1 wrote:
> I worry that this is excessive embedding. By not naming it, it's not clear
what
> this is the address of. (The server's local address?)

That is a fair comment, they don't need to be embedded, as forwardEntry
shouldn't acquire any of their methods.

http://codereview.appspot.com/6038047/diff/29003/ssh/tcpip.go#newcode106
ssh/tcpip.go:106: func (l *forwardlist) Lookup(addr *net.TCPAddr) (chan
forwardEntry, bool) {
On 2012/04/24 15:03:30, agl1 wrote:
> forwardList?

Done.

http://codereview.appspot.com/6038047/diff/29003/ssh/tcpip.go#newcode129
ssh/tcpip.go:129: return &tcpchanconn{
On 2012/04/24 15:03:30, agl1 wrote:
> tcpChanConn?

I'm fine with doing camel case, but I'd like to save it for a follow up CL. I
have some other cleanup things I'd like to submit

http://codereview.appspot.com/6038047/diff/29003/ssh/tcpip.go#newcode130
ssh/tcpip.go:130: tcpchan: &tcpchan{
On 2012/04/24 15:03:30, agl1 wrote:
> tcpChan?

See above

[lieqie, 2012-04-25 04:06:38]

// Request the remote side to open a port.
	l, err := client.Listen("tcp", ":1080")
	if err != nil {
		log.Fatalf("unable to register tcp forward: %v", err)
	}
	log.Println("ssh start successful on :1080")
	defer l.Close()
	http.Serve(l, http.HandlerFunc(func(resp http.ResponseWriter, req
*http.Request) {
 			fmt.Fprintf(resp, "Hello world!\n")
 	}))
-----------------------------------
this code compile ok, run ok, but don't open port 1080 to listen for
connections.
-----------------------------------
if I change 
l, err := client.Listen("tcp", ":1080")
to
l, err = net.Listen("tcp", ":1080") 
it works ok.

[dfc, 2012-04-25 05:57:22]

What you may be seeing is your ssh server filtering the hosts that can connect
to it. For example on my system, no matter what I pass to client.Listen, the ssh
daemon will only listen on the loopback. 

> if I change 
> l, err := client.Listen("tcp", ":1080")
> to
> l, err = net.Listen("tcp", ":1080") 

This is because (assuming your connecting to an ssh daemon on your local
machine), the first bind bound 127.0.0.1:1080 to your ssh daemon, the second
net.Listen will bind :1080 on any other interface.

Lets take this discussion to the golang-nuts mailing list.

[lieqie, 2012-04-25 07:05:44]

so let's assume 
ssh server: ssh.example.com:22

after
l, err := client.Listen("tcp", ":1080")
defer l.Close()
	http.Serve(l, http.HandlerFunc(func(resp http.ResponseWriter, req
*http.Request) {
 			fmt.Fprintf(resp, "Hello world!\n")
 	}))

how can we get to the "Hello world!" web page.

http://localhost:1080/
http://ssh.example.com:1080/
all refused.
the ssh server hasn't filter client ip.
ssh into ssh.example.com:22
in the shell do
wget http://localhost:1080/ 
refused too.

[dfc, 2012-04-26 10:32:18]

Hello gustav.paul@gmail.com, jeff@somethingsimilar.com, agl@golang.org,
lieqiewang@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dfc, 2012-04-26 10:37:16]

*** Submitted as
http://code.google.com/p/go/source/detail?r=306e41c8d097&repo=crypto ***

go.crypto/ssh: add support for remote tcpip forwarding

Add support for server (remote) forwarded tcpip channels.
See RFC4254 Section 7.1

R=gustav.paul, jeff, agl, lieqiewang
CC=golang-dev
http://codereview.appspot.com/6038047

[dfc, 2012-04-26 10:38:07]

Thank you for the feedback. agl: I make the change to tcpChan{,Conn} in this CL.