code review 10049045: go.crypto/ssh: fix test breakages introduced by 125:402...

go.crypto/ssh: fix test breakages introduced by 125:40246d2ae2eb

* Remove special handling for dynamically allocated
  ports. This was a bug in OpenSSH 5.x sshd.

* Run the test with a preselected port number.

* Run TestPortForward only on unix platforms.

[hanwen-google, 2013-06-12 14:00:58]

Hello dave@cheney.net, agl@golang.org (cc: golang-dev@googlegroups.com),

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

[hanwen-google, 2013-06-12 14:14:47]

Note: I tested this with openssh 6.2 and 5.9


On Wed, Jun 12, 2013 at 4:00 PM,  <hanwen@google.com> wrote:
> Reviewers: dfc, agl1,
>
> Message:
> Hello dave@cheney.net, agl@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go.crypto
>
>
> Description:
> go.crypto/ssh: fix test breakages introduced by 125:40246d2ae2eb
>
> * Remove special handling for dynamically allocated
>   ports. This was a bug in OpenSSH 5.x sshd.
>
> * Run the test with a preselected port number.
>
> * Run TestPortForward only on unix platforms.
>
> Please review this at https://codereview.appspot.com/10049045/
>
> Affected files:
>   M ssh/tcpip.go
>   M ssh/test/forward_unix_test.go
>
>
> Index: ssh/tcpip.go
> ===================================================================
> --- a/ssh/tcpip.go
> +++ b/ssh/tcpip.go
> @@ -48,15 +48,8 @@
>                 return nil, err
>         }
>
> -       // Register this forward, using the port number we requested.
> -       // If we requested port 0 (auto allocated port), we have to
> -       // register under 0, since the channelOpenMsg will list 0
> -       // rather than the allocated port number.
> -       ch := c.forwardList.add(*laddr)
> -
>         // If the original port was 0, then the remote side will
>         // supply a real port number in the response.
> -       origPort := uint32(laddr.Port)
>         if laddr.Port == 0 {
>                 port, _, ok := parseUint32(resp.Data)
>                 if !ok {
> @@ -65,7 +58,14 @@
>                 laddr.Port = int(port)
>         }
>
> -       return &tcpListener{laddr, origPort, c, ch}, nil
> +       // Register this forward, using the port number we obtained.
> +       //
> +       // This does not work on OpenSSH < 6.0, which will send a
> +       // channelOpenMsg with port number 0, rather than the actual
> +       // port number.
> +       ch := c.forwardList.add(*laddr)
> +
> +       return &tcpListener{laddr, c, ch}, nil
>  }
>
>  // forwardList stores a mapping between remote
> @@ -126,10 +126,8 @@
>  type tcpListener struct {
>         laddr *net.TCPAddr
>
> -       // The port with which we made the request, which can be 0.
> -       origPort uint32
> -       conn     *ClientConn
> -       in       <-chan forward
> +       conn *ClientConn
> +       in   <-chan forward
>  }
>
>  // Accept waits for and returns the next connection to the listener.
> @@ -155,13 +153,9 @@
>                 "cancel-tcpip-forward",
>                 true,
>                 l.laddr.IP.String(),
> -               l.origPort,
> +               uint32(l.laddr.Port),
>         }
> -       origAddr := net.TCPAddr{
> -               IP:   l.laddr.IP,
> -               Port: int(l.origPort),
> -       }
> -       l.conn.forwardList.remove(origAddr)
> +       l.conn.forwardList.remove(*l.laddr)
>         if _, err := l.conn.sendGlobalRequest(m); err != nil {
>                 return err
>         }
> Index: ssh/test/forward_unix_test.go
> ===================================================================
> rename from ssh/test/forward_test.go
> rename to ssh/test/forward_unix_test.go
> --- a/ssh/test/forward_test.go
> +++ b/ssh/test/forward_unix_test.go
> @@ -1,7 +1,14 @@
> +// Copyright 2012 The Go Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style
> +// license that can be found in the LICENSE file.
> +
> +// +build darwin freebsd linux netbsd openbsd
> +
>  package test
>
>  import (
>         "bytes"
> +       "fmt"
>         "io"
>         "io/ioutil"
>         "math/rand"
> @@ -9,17 +16,42 @@
>         "testing"
>  )
>
> +// pickUnusedPort finds an unused port. Starting sshd with this
> +// afterwards is a race condition, but hopefully it won't trigger too
> +// often. The alternative is to start sshd in a loop, but that would
> +// make it hard to distinguish port-in-use from other configuration
> +// errors.
> +func pickUnusedPort(t *testing.T) int {
> +       l, err := net.Listen("tcp", ":0")
> +       if err != nil {
> +               t.Fatalf("net.Listen: %v", err)
> +       }
> +       port := l.Addr().(*net.TCPAddr).Port
> +       if err := l.Close(); err != nil {
> +               t.Fatalf("listener.Close: %v", err)
> +       }
> +       return port
> +}
> +
>  func TestPortForward(t *testing.T) {
>         server := newServer(t)
>         defer server.Shutdown()
>         conn := server.Dial(clientConfig())
>         defer conn.Close()
>
> -       sshListener, err := conn.Listen("tcp", "127.0.0.1:0")
> +       // We can't reliably test dynamic port allocation, as it does
> +       // not work correctly with OpenSSH before 6.0. See also
> +       // https://bugzilla.mindrot.org/show_bug.cgi?id=2017
> +       port := pickUnusedPort(t)
> +       sshListener, err := conn.Listen("tcp", fmt.Sprintf("127.0.0.1:%d",
> port))
>         if err != nil {
>                 t.Fatalf("conn.Listen failed: %v", err)
>         }
>
> +       if sshListener.Addr().(*net.TCPAddr).Port != port {
> +               t.Fatalf("forwarding %v, want port %d", sshListener.Addr(),
> port)
> +       }
> +
>         go func() {
>                 sshConn, err := sshListener.Accept()
>                 if err != nil {
>
>



--

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

[hanwen-google, 2013-06-14 11:02:10]

Gentle ping?

On Wed, Jun 12, 2013 at 4:14 PM, Han-Wen Nienhuys <hanwen@google.com> wrote:
> Note: I tested this with openssh 6.2 and 5.9
>
>
> On Wed, Jun 12, 2013 at 4:00 PM,  <hanwen@google.com> wrote:
>> Reviewers: dfc, agl1,
>>
>> Message:
>> Hello dave@cheney.net, agl@golang.org (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go.crypto
>>
>>
>> Description:
>> go.crypto/ssh: fix test breakages introduced by 125:40246d2ae2eb
>>
>> * Remove special handling for dynamically allocated
>>   ports. This was a bug in OpenSSH 5.x sshd.
>>
>> * Run the test with a preselected port number.
>>
>> * Run TestPortForward only on unix platforms.
>>
>> Please review this at https://codereview.appspot.com/10049045/
>>
>> Affected files:
>>   M ssh/tcpip.go
>>   M ssh/test/forward_unix_test.go
>>
>>
>> Index: ssh/tcpip.go
>> ===================================================================
>> --- a/ssh/tcpip.go
>> +++ b/ssh/tcpip.go
>> @@ -48,15 +48,8 @@
>>                 return nil, err
>>         }
>>
>> -       // Register this forward, using the port number we requested.
>> -       // If we requested port 0 (auto allocated port), we have to
>> -       // register under 0, since the channelOpenMsg will list 0
>> -       // rather than the allocated port number.
>> -       ch := c.forwardList.add(*laddr)
>> -
>>         // If the original port was 0, then the remote side will
>>         // supply a real port number in the response.
>> -       origPort := uint32(laddr.Port)
>>         if laddr.Port == 0 {
>>                 port, _, ok := parseUint32(resp.Data)
>>                 if !ok {
>> @@ -65,7 +58,14 @@
>>                 laddr.Port = int(port)
>>         }
>>
>> -       return &tcpListener{laddr, origPort, c, ch}, nil
>> +       // Register this forward, using the port number we obtained.
>> +       //
>> +       // This does not work on OpenSSH < 6.0, which will send a
>> +       // channelOpenMsg with port number 0, rather than the actual
>> +       // port number.
>> +       ch := c.forwardList.add(*laddr)
>> +
>> +       return &tcpListener{laddr, c, ch}, nil
>>  }
>>
>>  // forwardList stores a mapping between remote
>> @@ -126,10 +126,8 @@
>>  type tcpListener struct {
>>         laddr *net.TCPAddr
>>
>> -       // The port with which we made the request, which can be 0.
>> -       origPort uint32
>> -       conn     *ClientConn
>> -       in       <-chan forward
>> +       conn *ClientConn
>> +       in   <-chan forward
>>  }
>>
>>  // Accept waits for and returns the next connection to the listener.
>> @@ -155,13 +153,9 @@
>>                 "cancel-tcpip-forward",
>>                 true,
>>                 l.laddr.IP.String(),
>> -               l.origPort,
>> +               uint32(l.laddr.Port),
>>         }
>> -       origAddr := net.TCPAddr{
>> -               IP:   l.laddr.IP,
>> -               Port: int(l.origPort),
>> -       }
>> -       l.conn.forwardList.remove(origAddr)
>> +       l.conn.forwardList.remove(*l.laddr)
>>         if _, err := l.conn.sendGlobalRequest(m); err != nil {
>>                 return err
>>         }
>> Index: ssh/test/forward_unix_test.go
>> ===================================================================
>> rename from ssh/test/forward_test.go
>> rename to ssh/test/forward_unix_test.go
>> --- a/ssh/test/forward_test.go
>> +++ b/ssh/test/forward_unix_test.go
>> @@ -1,7 +1,14 @@
>> +// Copyright 2012 The Go Authors. All rights reserved.
>> +// Use of this source code is governed by a BSD-style
>> +// license that can be found in the LICENSE file.
>> +
>> +// +build darwin freebsd linux netbsd openbsd
>> +
>>  package test
>>
>>  import (
>>         "bytes"
>> +       "fmt"
>>         "io"
>>         "io/ioutil"
>>         "math/rand"
>> @@ -9,17 +16,42 @@
>>         "testing"
>>  )
>>
>> +// pickUnusedPort finds an unused port. Starting sshd with this
>> +// afterwards is a race condition, but hopefully it won't trigger too
>> +// often. The alternative is to start sshd in a loop, but that would
>> +// make it hard to distinguish port-in-use from other configuration
>> +// errors.
>> +func pickUnusedPort(t *testing.T) int {
>> +       l, err := net.Listen("tcp", ":0")
>> +       if err != nil {
>> +               t.Fatalf("net.Listen: %v", err)
>> +       }
>> +       port := l.Addr().(*net.TCPAddr).Port
>> +       if err := l.Close(); err != nil {
>> +               t.Fatalf("listener.Close: %v", err)
>> +       }
>> +       return port
>> +}
>> +
>>  func TestPortForward(t *testing.T) {
>>         server := newServer(t)
>>         defer server.Shutdown()
>>         conn := server.Dial(clientConfig())
>>         defer conn.Close()
>>
>> -       sshListener, err := conn.Listen("tcp", "127.0.0.1:0")
>> +       // We can't reliably test dynamic port allocation, as it does
>> +       // not work correctly with OpenSSH before 6.0. See also
>> +       // https://bugzilla.mindrot.org/show_bug.cgi?id=2017
>> +       port := pickUnusedPort(t)
>> +       sshListener, err := conn.Listen("tcp", fmt.Sprintf("127.0.0.1:%d",
>> port))
>>         if err != nil {
>>                 t.Fatalf("conn.Listen failed: %v", err)
>>         }
>>
>> +       if sshListener.Addr().(*net.TCPAddr).Port != port {
>> +               t.Fatalf("forwarding %v, want port %d", sshListener.Addr(),
>> port)
>> +       }
>> +
>>         go func() {
>>                 sshConn, err := sshListener.Accept()
>>                 if err != nil {
>>
>>
>
>
>
> --
>
> 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



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

[agl1, 2013-06-14 19:29:51]

Supporting 5.9 would seem to be more important than 6 at the moment since it's
the basis of Ubuntu Precise.

(Of course, it would be nice to support both.)

[dave_cheney.net, 2013-06-16 12:10:42]

Some small comments. I'm having trouble running the tests on the machine I have
today. I'll try again tomorrow.

https://codereview.appspot.com/10049045/diff/7003/ssh/test/forward_unix_test.go
File ssh/test/forward_unix_test.go (right):

https://codereview.appspot.com/10049045/diff/7003/ssh/test/forward_unix_test....
ssh/test/forward_unix_test.go:1: // Copyright 2012 The Go Authors. All rights
reserved.
2013. Thanks for fixing the copyright header, I should have picked this up in
the previous review.

https://codereview.appspot.com/10049045/diff/7003/ssh/test/forward_unix_test....
ssh/test/forward_unix_test.go:41: t.Fatalf("net.Listen failed: %v (try no. %d)",
err, tries)
not conn.Listen ?

[hanwen-google, 2013-06-17 13:02:42]

On Fri, Jun 14, 2013 at 9:29 PM,  <agl@golang.org> wrote:
> Supporting 5.9 would seem to be more important than 6 at the moment
> since it's the basis of Ubuntu Precise.
>
> (Of course, it would be nice to support both.)
>
> https://codereview.appspot.com/10049045/

My primary objective is to get the build back to green.  I'll have a
look at making dynamic port allocation working after that.

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

[hanwen-google, 2013-06-18 16:21:12]

https://codereview.appspot.com/10049045/diff/7003/ssh/test/forward_unix_test.go
File ssh/test/forward_unix_test.go (right):

https://codereview.appspot.com/10049045/diff/7003/ssh/test/forward_unix_test....
ssh/test/forward_unix_test.go:1: // Copyright 2012 The Go Authors. All rights
reserved.
On 2013/06/16 12:10:42, dfc wrote:
> 2013. Thanks for fixing the copyright header, I should have picked this up in
> the previous review.

ack.

https://codereview.appspot.com/10049045/diff/7003/ssh/test/forward_unix_test....
ssh/test/forward_unix_test.go:41: t.Fatalf("net.Listen failed: %v (try no. %d)",
err, tries)
On 2013/06/16 12:10:42, dfc wrote:
> not conn.Listen ?

Done.

[agl1, 2013-06-18 16:38:07]

On Mon, Jun 17, 2013 at 9:02 AM, Han-Wen Nienhuys <hanwen@google.com> wrote:
> My primary objective is to get the build back to green.  I'll have a
> look at making dynamic port allocation working after that.

But it *is* green for most people, right? It passes for me, although
it dumps far too much to stdout.


Cheers

AGL

[hanwen-google, 2013-06-18 16:40:15]

On Tue, Jun 18, 2013 at 6:38 PM, Adam Langley <agl@golang.org> wrote:
> On Mon, Jun 17, 2013 at 9:02 AM, Han-Wen Nienhuys <hanwen@google.com> wrote:
>> My primary objective is to get the build back to green.  I'll have a
>> look at making dynamic port allocation working after that.
>
> But it *is* green for most people, right? It passes for me, although
> it dumps far too much to stdout.

It's red on http://build.golang.org/, so we're in a bad state for
further work on the package.


>
> Cheers
>
> AGL



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

[agl1, 2013-06-18 16:44:11]

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

go.crypto/ssh: fix test breakages introduced by 125:40246d2ae2eb

* Remove special handling for dynamically allocated
  ports. This was a bug in OpenSSH 5.x sshd.

* Run the test with a preselected port number.

* Run TestPortForward only on unix platforms.

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

Committer: Adam Langley <agl@golang.org>

[hanwen-google, 2013-06-18 16:54:48]

*** Abandoned ***