code review 9753044: go.crypto/ssh: fix and test port forwarding.

go.crypto/ssh: fix and test port forwarding.

Set maxPacket in forwarded connection, and use the requested port
number as key in forwardList.

[hanwen-google, 2013-06-03 14:59:51]

Hello golang-dev@googlegroups.com,

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

[hanwen-google, 2013-06-05 13:32:58]

Adam, thoughts?

[dave_cheney.net, 2013-06-06 11:54:49]

Thank you for this change. I'd like to see it soon, I just a have a few
questions about the change.

https://codereview.appspot.com/9753044/diff/4001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/9753044/diff/4001/ssh/tcpip.go#newcode52
ssh/tcpip.go:52: ch := c.forwardList.add(*laddr)
why did you move this, could it leak on line 60 ?

https://codereview.appspot.com/9753044/diff/4001/ssh/tcpip.go#newcode56
ssh/tcpip.go:56: origPort := uint32(laddr.Port)
if laddr.Port is not already a uint32, how can I specify a port about 2^15 ?

https://codereview.appspot.com/9753044/diff/4001/ssh/tcpip.go#newcode102
ssh/tcpip.go:102: func (l *forwardList) remove(addr net.TCPAddr) {
can you explain your rational for making addr a value not a ptr ?

[hanwen-google, 2013-06-06 12:55:11]

https://codereview.appspot.com/9753044/diff/4001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/9753044/diff/4001/ssh/tcpip.go#newcode52
ssh/tcpip.go:52: ch := c.forwardList.add(*laddr)
On 2013/06/06 11:54:49, dfc wrote:
> why did you move this, could it leak on line 60 ?

Added a comment.

The forward has to be registered with the orignal port number, ie. if you listen
to :0 , you may get port number X, but you still have to register under number
0, as the the openChannelMsg for a port-forward will have 0 rather than X.

You can see this if you run

 ssh -R 0:localhost:2020 -vvv localhost 

I was surprised that you cannot have two separate listeners with auto-assigned
port numbers, but that's apparently how it works.

https://codereview.appspot.com/9753044/diff/4001/ssh/tcpip.go#newcode56
ssh/tcpip.go:56: origPort := uint32(laddr.Port)
On 2013/06/06 11:54:49, dfc wrote:
> if laddr.Port is not already a uint32, how can I specify a port about 2^15 ?

it's an int currently, see http://golang.org/pkg/net/#TCPAddr - this cast is
just to quelch the conversion error.

https://codereview.appspot.com/9753044/diff/4001/ssh/tcpip.go#newcode102
ssh/tcpip.go:102: func (l *forwardList) remove(addr net.TCPAddr) {
On 2013/06/06 11:54:49, dfc wrote:
> can you explain your rational for making addr a value not a ptr ?

it makes the semantics self-documenting. The original code did

  // addr.Port == Y
  list.add(addr)
  addr.Port = X

and it's unclear from reading the call-site whether this registers the address
with port number X or with Y.

Since TCPAddr is a small datastructure, and this codepath is not heavily
exercised, there is no performance penalty in using a pass-by-value.

https://codereview.appspot.com/9753044/diff/10001/ssh/test/forward_test.go
File ssh/test/forward_test.go (right):

https://codereview.appspot.com/9753044/diff/10001/ssh/test/forward_test.go#ne...
ssh/test/forward_test.go:88: }
i've also added this bit.

[agl1, 2013-06-06 14:02:33]

dfc knows this code better than I, so I'll leave approval to him.

https://codereview.appspot.com/9753044/diff/10001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/9753044/diff/10001/ssh/tcpip.go#newcode85
ssh/tcpip.go:85: // forward represents an incoming forwarded tcpip connection. 
The
(nit: only a single space after the period.)

https://codereview.appspot.com/9753044/diff/10001/ssh/tcpip.go#newcode96
ssh/tcpip.go:96: 
(nit: remove this new, empty line or make the same change in remove and lookup.)

[hanwen-google, 2013-06-06 14:18:53]

https://codereview.appspot.com/9753044/diff/10001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/9753044/diff/10001/ssh/tcpip.go#newcode85
ssh/tcpip.go:85: // forward represents an incoming forwarded tcpip connection. 
The
On 2013/06/06 14:02:33, agl1 wrote:
> (nit: only a single space after the period.)

Done.

https://codereview.appspot.com/9753044/diff/10001/ssh/tcpip.go#newcode96
ssh/tcpip.go:96: 
On 2013/06/06 14:02:33, agl1 wrote:
> (nit: remove this new, empty line or make the same change in remove and
lookup.)

Done.

[hanwen-google, 2013-06-10 16:23:17]

Dave, friendly ping?

On Thu, Jun 6, 2013 at 4:18 PM,  <hanwen@google.com> wrote:
>
> https://codereview.appspot.com/9753044/diff/10001/ssh/tcpip.go
> File ssh/tcpip.go (right):
>
> https://codereview.appspot.com/9753044/diff/10001/ssh/tcpip.go#newcode85
> ssh/tcpip.go:85: // forward represents an incoming forwarded tcpip
> connection.  The
> On 2013/06/06 14:02:33, agl1 wrote:
>>
>> (nit: only a single space after the period.)
>
>
> Done.
>
> https://codereview.appspot.com/9753044/diff/10001/ssh/tcpip.go#newcode96
> ssh/tcpip.go:96:
>
> On 2013/06/06 14:02:33, agl1 wrote:
>>
>> (nit: remove this new, empty line or make the same change in remove
>
> and lookup.)
>
> Done.
>
> https://codereview.appspot.com/9753044/

--
Google Germany GmbH - ABC-Str. 19 - 20354 Hamburg
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg - Geschäftsführer: Graham Law, Katherine
Stephens

[dave_cheney.net, 2013-06-11 10:46:06]

Thank you. I think this is very close. My remaining question is around the
origPort member, it doesn't appear to be needed.

https://codereview.appspot.com/9753044/diff/19001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/9753044/diff/19001/ssh/client.go#newcode369
ssh/client.go:369: ch.maxPacket = msg.MaxPacketSize
wow, how did this ever work ?

https://codereview.appspot.com/9753044/diff/19001/ssh/client.go#newcode376
ssh/client.go:376: // As per RFC 4253 6.1, 32k is also the mininimum.
s/mininimum/minimum/

https://codereview.appspot.com/9753044/diff/19001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/9753044/diff/19001/ssh/tcpip.go#newcode130
ssh/tcpip.go:130: origPort uint32
this field does not appear to be needed. I removed it and all the tests pass.

https://codereview.appspot.com/9753044/diff/19001/ssh/tcpip.go#newcode161
ssh/tcpip.go:161: origAddr.Port = int(l.origPort)
If the goal is to make a copy of l.laddr, please consider this alternative.

        origAddr := net.TCPAddr{
                IP: l.laddr.IP,
                Port: int(l.origPort),
        }

[hanwen-google, 2013-06-11 13:10:50]

https://codereview.appspot.com/9753044/diff/19001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/9753044/diff/19001/ssh/client.go#newcode369
ssh/client.go:369: ch.maxPacket = msg.MaxPacketSize
On 2013/06/11 10:46:06, dfc wrote:
> wow, how did this ever work ?

I guess it didn't?

https://codereview.appspot.com/9753044/diff/19001/ssh/client.go#newcode376
ssh/client.go:376: // As per RFC 4253 6.1, 32k is also the mininimum.
On 2013/06/11 10:46:06, dfc wrote:
> s/mininimum/minimum/

Done.

https://codereview.appspot.com/9753044/diff/19001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/9753044/diff/19001/ssh/tcpip.go#newcode130
ssh/tcpip.go:130: origPort uint32
On 2013/06/11 10:46:06, dfc wrote:
> this field does not appear to be needed. I removed it and all the tests pass.

If you remove this, on closing, the forwardList.remove() call will not remove
the entry. This means that handleChanOpen() may accept a port-forward channel
open message for port 0, where it says

	// Section 7.2, implementations MUST reject suprious incoming
	// connections.
	c.sendConnectionFailed(msg.PeersId)

in client.go. 

To test it properly, we'd need a server implementation that issues a spurious
port-forward channel open, and check that it gets rejected. I didn't see an easy
way to do so.

Alternatively, we could have remove() panic if it doesn't find the requested
entry, but then tcpListener must be protected against double-close, which is
also not straighforward.

What do you think?

https://codereview.appspot.com/9753044/diff/19001/ssh/tcpip.go#newcode161
ssh/tcpip.go:161: origAddr.Port = int(l.origPort)
On 2013/06/11 10:46:06, dfc wrote:
> If the goal is to make a copy of l.laddr, please consider this alternative.
> 
>         origAddr := net.TCPAddr{
>                 IP: l.laddr.IP,
>                 Port: int(l.origPort),
>         }

Done.

[dave_cheney.net, 2013-06-11 13:21:54]

I see, thank you for your patience. I'll submit this now.

[dave_cheney.net, 2013-06-11 13:24:18]

Opps, spoke too soon.

--- FAIL: TestPortForward (0.08 seconds)
        forward_test.go:65: tcpConn.CloseWrite: shutdown tcp
127.0.0.1:56897: transport endpoint is not connected
        forward_test.go:71: got 0 bytes, want 741451
        test_unix_test.go:141: sshd: ""
FAIL

On Tue, Jun 11, 2013 at 11:21 PM, Dave Cheney <dave@cheney.net> wrote:
> I see, thank you for your patience. I'll submit this now.

[hanwen-google, 2013-06-11 13:50:22]

Is this reproducible or flakiness? I tried running a couple of times, but didn't
see this

I'm at 

 changeset:   124:d73471ad7bea

I'm on a Ubuntu Precise derived OS, with

OpenSSH_5.9gg10, OpenSSL 1.0.1 14 Mar 2012

[dave_cheney.net, 2013-06-12 02:13:35]

On 2013/06/11 13:50:22, hanwen wrote:
> Is this reproducible or flakiness? I tried running a couple of times, but
didn't
> see this
> 
> I'm at 
> 
>  changeset:   124:d73471ad7bea
> 
> I'm on a Ubuntu Precise derived OS, with
> 
> OpenSSH_5.9gg10, OpenSSL 1.0.1 14 Mar 2012

I wasn't able to reproduce this failure on another machine so I'm assuming it is
a local issue.

[dave_cheney.net, 2013-06-12 02:14:01]

*** Submitted as
https://code.google.com/p/go/source/detail?r=40246d2ae2eb&repo=crypto ***

go.crypto/ssh: fix and test port forwarding.

Set maxPacket in forwarded connection, and use the requested port
number as key in forwardList.

R=golang-dev, agl, dave
CC=golang-dev
https://codereview.appspot.com/9753044

Committer: Dave Cheney <dave@cheney.net>

[dave_cheney.net, 2013-06-12 02:29:33]

Hmm, looks like it wasn't just that one machine.

http://build.golang.org/log/619fe15659caeca07b6e900ddeefcf544b414574

Could you take a look please ?

On Wed, Jun 12, 2013 at 12:14 PM,  <dave@cheney.net> wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=40246d2ae2eb&repo=crypto
> ***
>
>
> go.crypto/ssh: fix and test port forwarding.
>
> Set maxPacket in forwarded connection, and use the requested port
> number as key in forwardList.
>
> R=golang-dev, agl, dave
> CC=golang-dev
> https://codereview.appspot.com/9753044
>
> Committer: Dave Cheney <dave@cheney.net>
>
>
> https://codereview.appspot.com/9753044/

[hanwen-google, 2013-06-12 09:29:37]

Looking.

My home laptop (OpenSSH 6.2) also sees this. Do you know what platform
these builders run?

On Wed, Jun 12, 2013 at 4:29 AM, Dave Cheney <dave@cheney.net> wrote:
> Hmm, looks like it wasn't just that one machine.
>
> http://build.golang.org/log/619fe15659caeca07b6e900ddeefcf544b414574
>
> Could you take a look please ?
>
> On Wed, Jun 12, 2013 at 12:14 PM,  <dave@cheney.net> wrote:
>> *** Submitted as
>> https://code.google.com/p/go/source/detail?r=40246d2ae2eb&repo=crypto
>> ***
>>
>>
>> go.crypto/ssh: fix and test port forwarding.
>>
>> Set maxPacket in forwarded connection, and use the requested port
>> number as key in forwardList.
>>
>> R=golang-dev, agl, dave
>> CC=golang-dev
>> https://codereview.appspot.com/9753044
>>
>> Committer: Dave Cheney <dave@cheney.net>
>>
>>
>> https://codereview.appspot.com/9753044/



-- 
Han-Wen Nienhuys
Google Munich
hanwen@google.com

[dave_cheney.net, 2013-06-12 09:40:29]

http://code.google.com/p/go-wiki/wiki/DashboardBuilders

[hanwen-google, 2013-06-12 11:02:24]

Ugh.

It appears that openssh 5.9 and 6.2 handle dynamically allocated ports
differently. That is, for 5.9 what I submitted works, and for 6.2 you
can leave out the orig port mess.

See also https://bugzilla.mindrot.org/show_bug.cgi?id=2017

Options:

* make something that works for both 5.9 and 6.0. This will be ugly.

* have it break for 5.9 like it used to.

* have dynamic port allocation disabled for openssh <= 5.9

I favor the latter, as it will be less surprising, but we would need
access to the remote version string to know the server version.

I will prepare a patch that removes the origport manipulation, and a
follow-up which does something more intelligent for openssh 5.9.
Sounds good?


On Wed, Jun 12, 2013 at 4:29 AM, Dave Cheney <dave@cheney.net> wrote:
> Hmm, looks like it wasn't just that one machine.
>
> http://build.golang.org/log/619fe15659caeca07b6e900ddeefcf544b414574
>
> Could you take a look please ?
>
> On Wed, Jun 12, 2013 at 12:14 PM,  <dave@cheney.net> wrote:
>> *** Submitted as
>> https://code.google.com/p/go/source/detail?r=40246d2ae2eb&repo=crypto
>> ***
>>
>>
>> go.crypto/ssh: fix and test port forwarding.
>>
>> Set maxPacket in forwarded connection, and use the requested port
>> number as key in forwardList.
>>
>> R=golang-dev, agl, dave
>> CC=golang-dev
>> https://codereview.appspot.com/9753044
>>
>> Committer: Dave Cheney <dave@cheney.net>
>>
>>
>> https://codereview.appspot.com/9753044/



-- 
Han-Wen Nienhuys
Google Munich
hanwen@google.com

[hanwen-google, 2013-06-12 14:01:29]

*** Abandoned ***