code review 11921043: go.crypto/ssh: add workaround for broken port forwarding in

go.crypto/ssh: add workaround for broken port forwarding in
OpenSSH 5.

Tested with OpenSSH_5.9

[hanwen-google, 2013-07-26 14:56:49]

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

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

[agl1, 2013-07-26 15:06:22]

https://codereview.appspot.com/11921043/diff/7001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/11921043/diff/7001/ssh/tcpip.go#newcode45
ssh/tcpip.go:45: var openSSHVersionRegexp =
regexp.MustCompile("OpenSSH_([0-9]+)")
regexps are great for human interfaces, but not code. This should be a quick few
calls in strings.

https://codereview.appspot.com/11921043/diff/7001/ssh/tcpip.go#newcode47
ssh/tcpip.go:47: func autoPortListenBroken(versionStr string) bool {
// isBrokenOpenSSHVersion returns true if the given version string specifies a
version of OpenSSH that is known to have a bug in port forwarding.

https://codereview.appspot.com/11921043/diff/7001/ssh/tcpip.go#newcode60
ssh/tcpip.go:60: tries := 10
const

https://codereview.appspot.com/11921043/diff/7001/ssh/tcpip.go#newcode70
ssh/tcpip.go:70: return nil, fmt.Errorf("ssh: failed after %d tries: %v", tries,
err)
"finding random port" or something of that nature before "failed".

[hanwen-google, 2013-07-26 17:27:17]

https://codereview.appspot.com/11921043/diff/7001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/11921043/diff/7001/ssh/tcpip.go#newcode45
ssh/tcpip.go:45: var openSSHVersionRegexp =
regexp.MustCompile("OpenSSH_([0-9]+)")
On 2013/07/26 15:06:22, agl1 wrote:
> regexps are great for human interfaces, but not code. This should be a quick
few
> calls in strings.

Done.

https://codereview.appspot.com/11921043/diff/7001/ssh/tcpip.go#newcode47
ssh/tcpip.go:47: func autoPortListenBroken(versionStr string) bool {
On 2013/07/26 15:06:22, agl1 wrote:
> // isBrokenOpenSSHVersion returns true if the given version string specifies a
> version of OpenSSH that is known to have a bug in port forwarding.

Done.

https://codereview.appspot.com/11921043/diff/7001/ssh/tcpip.go#newcode60
ssh/tcpip.go:60: tries := 10
On 2013/07/26 15:06:22, agl1 wrote:
> const

Done.

https://codereview.appspot.com/11921043/diff/7001/ssh/tcpip.go#newcode70
ssh/tcpip.go:70: return nil, fmt.Errorf("ssh: failed after %d tries: %v", tries,
err)
On 2013/07/26 15:06:22, agl1 wrote:
> "finding random port" or something of that nature before "failed".

Done.

[agl1, 2013-07-26 18:38:05]

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

go.crypto/ssh: add workaround for broken port forwarding in
OpenSSH 5.

Tested with OpenSSH_5.9

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

Committer: Adam Langley <agl@golang.org>

https://codereview.appspot.com/11921043/diff/16001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/11921043/diff/16001/ssh/tcpip.go#newcode62
ssh/tcpip.go:62: fmt.Printf("V %q", versionStr[i:j])
Looks like debugging - will remove before landing.

[jmp, 2013-07-27 16:13:49]

The random port picker in this CL is not really random.

addr.Port = 1024 + rand.Intn(60000)

It will always produce the same set of values each time.  For a loop of 10,
they would be (in order):

39105
8911
32871
25083
23105
22342
15449
3564
41480
4324

Is that acceptable for the purpose of this CL?  You could use crypto/rand,
but simpler might be to have the seed change using something like:

1024 + rand.Intn(int(time.Now().Unix()%60000))

This is still predictable, but it will generate different sets of values
since the seed will change every second and only see a repeat every 60000
seconds (16h40m).  I would think this is better than always trying the same
first 10 ports each time it gets run.  See
http://play.golang.org/p/wXlwVvwcOj for an example, but try running it on a
local machine instead of in the playground since time in the playground is
always the same.


On Fri, Jul 26, 2013 at 2:38 PM, <agl@golang.org> wrote:

> *** Submitted as
>
https://code.google.com/p/go/**source/detail?r=752ba0444fdd&**repo=crypto<htt...
> ***
>
>
> go.crypto/ssh: add workaround for broken port forwarding in
> OpenSSH 5.
>
> Tested with OpenSSH_5.9
>
> R=agl, dave
> CC=golang-dev
>
https://codereview.appspot.**com/11921043<https://codereview.appspot.com/1192...
>
> Committer: Adam Langley <agl@golang.org>
>
>
>
>
https://codereview.appspot.**com/11921043/diff/16001/ssh/**tcpip.go<https://c...
> File ssh/tcpip.go (right):
>
> https://codereview.appspot.**com/11921043/diff/16001/ssh/**
>
tcpip.go#newcode62<https://codereview.appspot.com/11921043/diff/16001/ssh/tcpip.go#newcode62>
> ssh/tcpip.go:62: fmt.Printf("V %q", versionStr[i:j])
> Looks like debugging - will remove before landing.
>
>
>
https://codereview.appspot.**com/11921043/<https://codereview.appspot.com/119...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[hanwen-google, 2013-07-28 22:33:11]

You're completely right. I had forgotten about the deterministic seed.
I think we can init a random generator with time.Now(). I'll prepare a
CL.

On Sat, Jul 27, 2013 at 1:13 PM, Jonathan Pittman
<jonathan.mark.pittman@gmail.com> wrote:
> The random port picker in this CL is not really random.
>
> addr.Port = 1024 + rand.Intn(60000)
>
> It will always produce the same set of values each time.  For a loop of 10,
> they would be (in order):
>
> 39105
> 8911
> 32871
> 25083
> 23105
> 22342
> 15449
> 3564
> 41480
> 4324
>
> Is that acceptable for the purpose of this CL?  You could use crypto/rand,
> but simpler might be to have the seed change using something like:
>
> 1024 + rand.Intn(int(time.Now().Unix()%60000))
>
> This is still predictable, but it will generate different sets of values
> since the seed will change every second and only see a repeat every 60000
> seconds (16h40m).  I would think this is better than always trying the same
> first 10 ports each time it gets run.  See
> http://play.golang.org/p/wXlwVvwcOj for an example, but try running it on a
> local machine instead of in the playground since time in the playground is
> always the same.
>
>
> On Fri, Jul 26, 2013 at 2:38 PM, <agl@golang.org> wrote:
>>
>> *** Submitted as
>> https://code.google.com/p/go/source/detail?r=752ba0444fdd&repo=crypto
>> ***
>>
>>
>> go.crypto/ssh: add workaround for broken port forwarding in
>> OpenSSH 5.
>>
>> Tested with OpenSSH_5.9
>>
>> R=agl, dave
>> CC=golang-dev
>> https://codereview.appspot.com/11921043
>>
>> Committer: Adam Langley <agl@golang.org>
>>
>>
>>
>> https://codereview.appspot.com/11921043/diff/16001/ssh/tcpip.go
>> File ssh/tcpip.go (right):
>>
>> https://codereview.appspot.com/11921043/diff/16001/ssh/tcpip.go#newcode62
>> ssh/tcpip.go:62: fmt.Printf("V %q", versionStr[i:j])
>> Looks like debugging - will remove before landing.
>>
>>
>> https://codereview.appspot.com/11921043/
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-dev+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
>>
>>
>



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

[hanwen-google, 2013-07-29 13:00:42]

*** Abandoned ***

[hanwen-google, 2013-07-29 13:14:17]

It produces the same sequence, but the random generator is shared with
everything else, and a second call will not start with the same port
number. That said,

this change

 https://codereview.appspot.com/12027043

should fix things.


On Sat, Jul 27, 2013 at 1:13 PM, Jonathan Pittman
<jonathan.mark.pittman@gmail.com> wrote:
> The random port picker in this CL is not really random.
>
> addr.Port = 1024 + rand.Intn(60000)
>
> It will always produce the same set of values each time.  For a loop of 10,
> they would be (in order):
>
> 39105
> 8911
> 32871
> 25083
> 23105
> 22342
> 15449
> 3564
> 41480
> 4324
>
> Is that acceptable for the purpose of this CL?  You could use crypto/rand,
> but simpler might be to have the seed change using something like:
>
> 1024 + rand.Intn(int(time.Now().Unix()%60000))
>
> This is still predictable, but it will generate different sets of values
> since the seed will change every second and only see a repeat every 60000
> seconds (16h40m).  I would think this is better than always trying the same
> first 10 ports each time it gets run.  See
> http://play.golang.org/p/wXlwVvwcOj for an example, but try running it on a
> local machine instead of in the playground since time in the playground is
> always the same.
>
>
> On Fri, Jul 26, 2013 at 2:38 PM, <agl@golang.org> wrote:
>>
>> *** Submitted as
>> https://code.google.com/p/go/source/detail?r=752ba0444fdd&repo=crypto
>> ***
>>
>>
>> go.crypto/ssh: add workaround for broken port forwarding in
>> OpenSSH 5.
>>
>> Tested with OpenSSH_5.9
>>
>> R=agl, dave
>> CC=golang-dev
>> https://codereview.appspot.com/11921043
>>
>> Committer: Adam Langley <agl@golang.org>
>>
>>
>>
>> https://codereview.appspot.com/11921043/diff/16001/ssh/tcpip.go
>> File ssh/tcpip.go (right):
>>
>> https://codereview.appspot.com/11921043/diff/16001/ssh/tcpip.go#newcode62
>> ssh/tcpip.go:62: fmt.Printf("V %q", versionStr[i:j])
>> Looks like debugging - will remove before landing.
>>
>>
>> https://codereview.appspot.com/11921043/
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-dev+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
>>
>>
>



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