code review 14494058: go.crypto/ssh: support rekeying in both directions.

go.crypto/ssh: support rekeying in both directions.

Adds a largely symmetrical handshakeTransport, which can send
and process kexInit messages. Automatically rekey on every on
1G of data transmitted.

[hanwen-google, 2013-10-14 21:48:26]

Hello agl1, dfc, jpsugar@google.com (cc: golang-dev@googlegroups.com),

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

[hanwen-google, 2013-10-14 21:59:18]

this goes on top of CL 14641044.

We must submit this one before the mux change, since we'd break rekeying
otherwise. 

Beyond mechanism, this also implements a policy (rekey based on amount of data
passed through the connection.) suggested by the RFC

questions:
* should we implement the other policy (rekey after every hour)
* should we expose the rekeying functionality (OpenSSH apparently has a ~R
shortcut that issues a rekey.) ?

[jpsugar, 2013-10-14 22:27:31]

So for "rekey after N bytes", I think it's important to do that at a low level
to avoid races where significantly more than N bytes might go through before the
channel is blocked for a rekey.

For other policies, I prefer the idea of exporting a Rekey() method and allowing
the user to implement them, possibly with helpers.

[hanwen-google, 2013-10-17 05:56:12]

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

Please take another look.

[jpsugar, 2013-10-17 17:56:54]

Can we just eat the newKeys message instead of passing it up and having it be
ignored?

https://codereview.appspot.com/14494058/diff/45001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/14494058/diff/45001/ssh/client.go#newcode84
ssh/client.go:84: // discard newkeys packet.
Do we want to ensure that it's a newkeys packet?

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go
File ssh/handshake.go (right):

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode16
ssh/handshake.go:16: //const debug = true
Replace this with an explanation of what this does?

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode47
ssh/handshake.go:47: hostKeys []Signer // If hostKeys are given, we are the
server
Missing period on comment.

I think this is a really important thing to note in the protocol. Accidentally
missing hostKeys causes us to completely flip our role in the exchange, which
would probably produce weird errors. It may be simpler and safer to have a bool
isServer or similar.

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode80
ssh/handshake.go:80: func newClientTransport(conn keyingTransport,
clientVersion, serverVersion []byte,
Wrap at 80, 120, or don't wrap. Wrapping at 82 is weird. :-) I prefer no wrap.

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode91
ssh/handshake.go:91: func newServerTransport(
Why are the args on a new line?

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode154
ssh/handshake.go:154: // This is not completely accurate, as the debug
Add a TODO?

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode228
ssh/handshake.go:228: scramble := make([]byte, len(packet))
I'm not a fan of "scramble". "packetCopy"?

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode256
ssh/handshake.go:256: t.mu.Unlock()
defer this?

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode278
ssh/handshake.go:278: clientInit := otherInit
If I'm reading this correctly, these four vars only really affect the magics.
Can we fold this directly into the init of the magics?

https://codereview.appspot.com/14494058/diff/45001/ssh/test/test_unix_test.go
File ssh/test/test_unix_test.go (right):

https://codereview.appspot.com/14494058/diff/45001/ssh/test/test_unix_test.go...
ssh/test/test_unix_test.go:110: checkCount int
What is this checkCount for?

[hanwen-google, 2013-10-20 20:55:25]

Let me think for a bit about eating newkeys altogether, to make sure it doesnt
open holes.

https://codereview.appspot.com/14494058/diff/45001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/14494058/diff/45001/ssh/client.go#newcode84
ssh/client.go:84: // discard newkeys packet.
On 2013/10/17 17:56:55, jpsugar wrote:
> Do we want to ensure that it's a newkeys packet?

Done.

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go
File ssh/handshake.go (right):

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode16
ssh/handshake.go:16: //const debug = true
On 2013/10/17 17:56:55, jpsugar wrote:
> Replace this with an explanation of what this does?

it's for debugging, obviously. It's probably better to delete before submitting.

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode47
ssh/handshake.go:47: hostKeys []Signer // If hostKeys are given, we are the
server
On 2013/10/17 17:56:55, jpsugar wrote:
> Missing period on comment.
> 
> I think this is a really important thing to note in the protocol. Accidentally
> missing hostKeys causes us to completely flip our role in the exchange, which
> would probably produce weird errors. It may be simpler and safer to have a
bool
> isServer or similar.

The initialization takes care of it, though: either you run newClientTransport,
or newServerTransport.

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode80
ssh/handshake.go:80: func newClientTransport(conn keyingTransport,
clientVersion, serverVersion []byte,
On 2013/10/17 17:56:55, jpsugar wrote:
> Wrap at 80, 120, or don't wrap. Wrapping at 82 is weird. :-) I prefer no wrap.

Done.

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode91
ssh/handshake.go:91: func newServerTransport(
On 2013/10/17 17:56:55, jpsugar wrote:
> Why are the args on a new line?

Done.

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode154
ssh/handshake.go:154: // This is not completely accurate, as the debug
On 2013/10/17 17:56:55, jpsugar wrote:
> Add a TODO?

Done.

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode154
ssh/handshake.go:154: // This is not completely accurate, as the debug
On 2013/10/17 17:56:55, jpsugar wrote:
> Add a TODO?

I've moved the filtering into this layer up, so we get this correct now.

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode228
ssh/handshake.go:228: scramble := make([]byte, len(packet))
On 2013/10/17 17:56:55, jpsugar wrote:
> I'm not a fan of "scramble". "packetCopy"?

Done.

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode256
ssh/handshake.go:256: t.mu.Unlock()
On 2013/10/17 17:56:55, jpsugar wrote:
> defer this?

I recall that defer are a little expensive, since it does a heap allocation,
hence the attempt to avoid it. There are no early returns, so defer doesn't make
the code cleaner.

https://codereview.appspot.com/14494058/diff/45001/ssh/handshake.go#newcode278
ssh/handshake.go:278: clientInit := otherInit
On 2013/10/17 17:56:55, jpsugar wrote:
> If I'm reading this correctly, these four vars only really affect the magics.
> Can we fold this directly into the init of the magics?

the message is used for determining algorithms, see line 289.

[hanwen-google, 2013-10-21 00:46:50]

I tried eating newkeys (and managed to make it work), but decided against it. It
also serves to synchronize things in the initial handshake, in particular to
ensure that sessionID is set before we start authentication.

[hanwen-google, 2013-10-21 00:47:21]

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

Please take another look.

[jpsugar, 2013-10-21 17:03:50]

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go
File ssh/handshake.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode221
ssh/handshake.go:221: // TODO(hanwen): add random bits to kexInit.Cookie.
Um, this is pretty serious. No random bits is a security issue.

[hanwen-google, 2013-10-21 20:00:20]

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go
File ssh/handshake.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode221
ssh/handshake.go:221: // TODO(hanwen): add random bits to kexInit.Cookie.
On 2013/10/21 17:03:51, jpsugar wrote:
> Um, this is pretty serious. No random bits is a security issue.

go.crypto only support DH and ECDH key exchanges, both of which have randomness
of both sides. Maybe this is for GSS/Kerberos?

I wanted to address this in another CL (currently, the Cookie is not init'd
either.) to simplify review.

[jpsugar, 2013-10-21 20:00:55]

LGTM

provided agl ok's the no-random-cookie thing.

[hanwen-google, 2013-10-22 21:15:52]

Adam,  can you have a look?

On Mon, Oct 21, 2013 at 1:00 PM,  <jpsugar@google.com> wrote:
> LGTM
>
> provided agl ok's the no-random-cookie thing.
>
> https://codereview.appspot.com/14494058/



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

[dave_cheney.net, 2013-10-22 22:07:01]

I would really like to see this CL broken into smaller chunks. 1000 lines
changed is too many for me to review with confidence. 

I suggest breaking out all the sections I have noted into a separate CL.

Then breaking out adding handshake{_test}.go into a separate CL. 

Then a final one to hook them all up.

Thank you, I know I keep asking for smaller diffs, thank you for accomodating
me.

https://codereview.appspot.com/14494058/diff/45001/ssh/session_test.go
File ssh/session_test.go (right):

https://codereview.appspot.com/14494058/diff/45001/ssh/session_test.go#newcod...
ssh/session_test.go:486: if err := conn.writePacket(make([]byte, maxPacket*2));
err == nil {
This looks like an unrelated cleanup, is it possible to move this to another CL.

https://codereview.appspot.com/14494058/diff/45001/ssh/transport.go
File ssh/transport.go (right):

https://codereview.appspot.com/14494058/diff/45001/ssh/transport.go#newcode54
ssh/transport.go:54: 
excluding this addition, can the rest of this change be moved to another CL

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode19
ssh/client.go:19: *handshakeTransport
This exposes ClientConn.SessionID(), is this intended ?

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode21
ssh/client.go:21: sshConn
This is going to expose methods like ClientConn.SetDeadline() is this intended ?

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode59
ssh/client.go:59: func (c *ClientConn) Close() error {
Why is this needed ? how can ClientConn.sshConn.Conn ever be nil ?

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode82
ssh/client.go:82: _, _, err = c.sendKexInit()
if _, _, err := c.sendKexInit(); err != nil {
       return err
}

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode86
ssh/client.go:86: 
if packet, err := ... { 

} else if { ... }

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth.go#newcod...
ssh/client_auth.go:224: sign, err := p.Sign(i, rand, data)
Can you break this change out please ?

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth_test.go
File ssh/client_auth_test.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth_test.go#n...
ssh/client_auth_test.go:366: c.Close()
this can be broken out

https://codereview.appspot.com/14494058/diff/180001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/common.go#newcode361
ssh/common.go:361: // and writes.
I think this can be implemented as

type sshConn interface {
    Close() error
    LocalAddr() Addr
    RemoteAddr() Addr
    SetDeadline(t time.Time) error
    SetReadDeadline(t time.Time) error
    SetWriteDeadline(t time.Time) error
}

maybe you want to filter out the Deadline methods.

https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go
File ssh/transport.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go#newcode55
ssh/transport.go:55: func (t *transport) SessionID() []byte {
does this need to return a copy of the sessionID ?

https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go#newcode268
ssh/transport.go:268: func newTransport(conn io.ReadWriteCloser, rand io.Reader,
isClient bool) *transport {
s/conn/rwc/

[agl1, 2013-10-22 22:12:56]

https://codereview.appspot.com/14494058/diff/180001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/common.go#newcode360
ssh/common.go:360: // sshConn provides net.Conn metadata, but but disallows
direct reads
"but but".

https://codereview.appspot.com/14494058/diff/180001/ssh/common.go#newcode367
ssh/common.go:367: return 0, errors.New("open a channel to write to an SSH
connection")
"ssh: " prefix for error messages.

https://codereview.appspot.com/14494058/diff/180001/ssh/common.go#newcode371
ssh/common.go:371: return 0, errors.New("open a channel to read from an SSH
connection")
and here.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go
File ssh/handshake.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode1
ssh/handshake.go:1: // Copyright 2011 The Go Authors. All rights reserved.
2013

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode17
ssh/handshake.go:17: //const debug = false
delete this line.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode20
ssh/handshake.go:20: // keyingTransport is packet based transport that supports
key
"a" after "is".

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode42
ssh/handshake.go:42: // (why is Rand not part of CryptoConfig?)
// TODO: move into CryptoConfig

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode196
ssh/handshake.go:196: // transport.  This function is safe for concurrent use by
multiple
This file is inconsistent between one or two spaces after a period. (One is
better.)

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode204
ssh/handshake.go:204: func (t *handshakeTransport) unlockedSendKexInit()
(*kexInitMsg, []byte, error) {
Isn't this lockedSendKexInit()?

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode206
ssh/handshake.go:206: // Since send and receive of kex inits are not
This comment needs rewriting.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode221
ssh/handshake.go:221: // TODO(hanwen): add random bits to kexInit.Cookie.
On 2013/10/21 20:00:20, hanwen-google wrote:
> On 2013/10/21 17:03:51, jpsugar wrote:
> > Um, this is pretty serious. No random bits is a security issue.
> 
> go.crypto only support DH and ECDH key exchanges, both of which have
randomness
> of both sides. Maybe this is for GSS/Kerberos?
> 
> I wanted to address this in another CL (currently, the Cookie is not init'd
> either.) to simplify review.

Sadly the repo is Mercurial, so god knows how one checks back in time. But it's
Not Good that we're not filling in Cookie. I'm not sure when this disappeared or
whether I screwed it up from the start.

This change doesn't have to include it, but the next one should.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode313
ssh/handshake.go:313: // We don't send FirstKexFollows, but we handle receiving
it
period at the end.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake_test.go
File ssh/handshake_test.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake_test.go#new...
ssh/handshake_test.go:1: // Copyright 2011 The Go Authors. All rights reserved.
2013

[hanwen-google, 2013-10-23 00:15:17]

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode19
ssh/client.go:19: *handshakeTransport
On 2013/10/22 22:07:02, dfc wrote:
> This exposes ClientConn.SessionID(), is this intended ?

no; fixed.

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode21
ssh/client.go:21: sshConn
On 2013/10/22 22:07:02, dfc wrote:
> This is going to expose methods like ClientConn.SetDeadline() is this intended
?

this is intended. transport embeds net.Conn. sshConn is just to hide Write/Read.

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode59
ssh/client.go:59: func (c *ClientConn) Close() error {
On 2013/10/22 22:07:02, dfc wrote:
> Why is this needed ? how can ClientConn.sshConn.Conn ever be nil ?

Reverted (I managed to hit this debugging tests).

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode82
ssh/client.go:82: _, _, err = c.sendKexInit()
On 2013/10/22 22:07:02, dfc wrote:
> if _, _, err := c.sendKexInit(); err != nil {
>        return err
> }

Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode86
ssh/client.go:86: 
On 2013/10/22 22:07:02, dfc wrote:
> if packet, err := ... { 
> 
> } else if { ... }

Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth.go#newcod...
ssh/client_auth.go:224: sign, err := p.Sign(i, rand, data)
On 2013/10/22 22:07:02, dfc wrote:
> Can you break this change out please ?

will do.

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth_test.go
File ssh/client_auth_test.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth_test.go#n...
ssh/client_auth_test.go:366: c.Close()
On 2013/10/22 22:07:02, dfc wrote:
> this can be broken out

will do.

https://codereview.appspot.com/14494058/diff/180001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/common.go#newcode361
ssh/common.go:361: // and writes.
On 2013/10/22 22:07:02, dfc wrote:
> I think this can be implemented as
> 
> type sshConn interface {
>     Close() error
>     LocalAddr() Addr
>     RemoteAddr() Addr
>     SetDeadline(t time.Time) error
>     SetReadDeadline(t time.Time) error
>     SetWriteDeadline(t time.Time) error
> }
> 
> maybe you want to filter out the Deadline methods.

sure. This means that ClientConn no longer is a net.Conn, though. Is that a
problem?

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go
File ssh/handshake.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode17
ssh/handshake.go:17: //const debug = false
On 2013/10/22 22:12:57, agl1 wrote:
> delete this line.

Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode20
ssh/handshake.go:20: // keyingTransport is packet based transport that supports
key
On 2013/10/22 22:12:57, agl1 wrote:
> "a" after "is".

Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode42
ssh/handshake.go:42: // (why is Rand not part of CryptoConfig?)
On 2013/10/22 22:12:57, agl1 wrote:
> // TODO: move into CryptoConfig

Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode196
ssh/handshake.go:196: // transport.  This function is safe for concurrent use by
multiple
On 2013/10/22 22:12:57, agl1 wrote:
> This file is inconsistent between one or two spaces after a period. (One is
> better.)

Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode204
ssh/handshake.go:204: func (t *handshakeTransport) unlockedSendKexInit()
(*kexInitMsg, []byte, error) {
On 2013/10/22 22:12:57, agl1 wrote:
> Isn't this lockedSendKexInit()?

Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode206
ssh/handshake.go:206: // Since send and receive of kex inits are not
On 2013/10/22 22:12:57, agl1 wrote:
> This comment needs rewriting.

Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode221
ssh/handshake.go:221: // TODO(hanwen): add random bits to kexInit.Cookie.
On 2013/10/22 22:12:57, agl1 wrote:
> On 2013/10/21 20:00:20, hanwen-google wrote:
> > On 2013/10/21 17:03:51, jpsugar wrote:
> > > Um, this is pretty serious. No random bits is a security issue.
> > 
> > go.crypto only support DH and ECDH key exchanges, both of which have
> randomness
> > of both sides. Maybe this is for GSS/Kerberos?
> > 
> > I wanted to address this in another CL (currently, the Cookie is not init'd
> > either.) to simplify review.
> 
> Sadly the repo is Mercurial, so god knows how one checks back in time. But
it's
> Not Good that we're not filling in Cookie. I'm not sure when this disappeared
or
> whether I screwed it up from the start.

try 

  hg log -p | less 

I believe it was never set.

> This change doesn't have to include it, but the next one should.

understood.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode313
ssh/handshake.go:313: // We don't send FirstKexFollows, but we handle receiving
it
On 2013/10/22 22:12:57, agl1 wrote:
> period at the end.

Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake_test.go
File ssh/handshake_test.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake_test.go#new...
ssh/handshake_test.go:1: // Copyright 2011 The Go Authors. All rights reserved.
On 2013/10/22 22:12:57, agl1 wrote:
> 2013

Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go
File ssh/transport.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go#newcode55
ssh/transport.go:55: func (t *transport) SessionID() []byte {
On 2013/10/22 22:07:02, dfc wrote:
> does this need to return a copy of the sessionID ?

not necessarily, but it is safer and cost is neglible.

https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go#newcode268
ssh/transport.go:268: func newTransport(conn io.ReadWriteCloser, rand io.Reader,
isClient bool) *transport {
On 2013/10/22 22:07:02, dfc wrote:
> s/conn/rwc/ 

Done.

[dave_cheney.net, 2013-10-23 00:22:47]

> On 22 Oct 2013, at 17:15, hanwen@google.com wrote:
> 
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/client.go
> File ssh/client.go (right):
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode19
> ssh/client.go:19: *handshakeTransport
>> On 2013/10/22 22:07:02, dfc wrote:
>> This exposes ClientConn.SessionID(), is this intended ?
> 
> no; fixed.
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode21
> ssh/client.go:21: sshConn
>> On 2013/10/22 22:07:02, dfc wrote:
>> This is going to expose methods like ClientConn.SetDeadline() is this
> intended ?
> 
> this is intended. transport embeds net.Conn. sshConn is just to hide
> Write/Read.
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode59
> ssh/client.go:59: func (c *ClientConn) Close() error {
>> On 2013/10/22 22:07:02, dfc wrote:
>> Why is this needed ? how can ClientConn.sshConn.Conn ever be nil ?
> 
> Reverted (I managed to hit this debugging tests).
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode82
> ssh/client.go:82: _, _, err = c.sendKexInit()
>> On 2013/10/22 22:07:02, dfc wrote:
>> if _, _, err := c.sendKexInit(); err != nil {
>>        return err
>> }
> 
> Done.
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode86
> ssh/client.go:86:
>> On 2013/10/22 22:07:02, dfc wrote:
>> if packet, err := ... {
> 
>> } else if { ... }
> 
> Done.
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth.go
> File ssh/client_auth.go (right):
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth.go#newcod...
> ssh/client_auth.go:224: sign, err := p.Sign(i, rand, data)
>> On 2013/10/22 22:07:02, dfc wrote:
>> Can you break this change out please ?
> 
> will do.
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth_test.go
> File ssh/client_auth_test.go (right):
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth_test.go#n...
> ssh/client_auth_test.go:366: c.Close()
>> On 2013/10/22 22:07:02, dfc wrote:
>> this can be broken out
> 
> will do.
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/common.go
> File ssh/common.go (right):
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/common.go#newcode361
> ssh/common.go:361: // and writes.
>> On 2013/10/22 22:07:02, dfc wrote:
>> I think this can be implemented as
> 
>> type sshConn interface {
>>     Close() error
>>     LocalAddr() Addr
>>     RemoteAddr() Addr
>>     SetDeadline(t time.Time) error
>>     SetReadDeadline(t time.Time) error
>>     SetWriteDeadline(t time.Time) error
>> }
> 
>> maybe you want to filter out the Deadline methods.
> 
> sure. This means that ClientConn no longer is a net.Conn, though. Is
> that a problem?

You are absolutely correct. This was my mistake and I should not have forgotten
this. 



> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go
> File ssh/handshake.go (right):
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode17
> ssh/handshake.go:17: //const debug = false
>> On 2013/10/22 22:12:57, agl1 wrote:
>> delete this line.
> 
> Done.
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode20
> ssh/handshake.go:20: // keyingTransport is packet based transport that
> supports key
>> On 2013/10/22 22:12:57, agl1 wrote:
>> "a" after "is".
> 
> Done.
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode42
> ssh/handshake.go:42: // (why is Rand not part of CryptoConfig?)
>> On 2013/10/22 22:12:57, agl1 wrote:
>> // TODO: move into CryptoConfig
> 
> Done.
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode196
> ssh/handshake.go:196: // transport.  This function is safe for
> concurrent use by multiple
>> On 2013/10/22 22:12:57, agl1 wrote:
>> This file is inconsistent between one or two spaces after a period.
> (One is
>> better.)
> 
> Done.
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode204
> ssh/handshake.go:204: func (t *handshakeTransport) unlockedSendKexInit()
> (*kexInitMsg, []byte, error) {
>> On 2013/10/22 22:12:57, agl1 wrote:
>> Isn't this lockedSendKexInit()?
> 
> Done.
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode206
> ssh/handshake.go:206: // Since send and receive of kex inits are not
>> On 2013/10/22 22:12:57, agl1 wrote:
>> This comment needs rewriting.
> 
> Done.
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode221
> ssh/handshake.go:221: // TODO(hanwen): add random bits to
> kexInit.Cookie.
>> On 2013/10/22 22:12:57, agl1 wrote:
>> On 2013/10/21 20:00:20, hanwen-google wrote:
>> > On 2013/10/21 17:03:51, jpsugar wrote:
>> > > Um, this is pretty serious. No random bits is a security issue.
>> >
>> > go.crypto only support DH and ECDH key exchanges, both of which have
>> randomness
>> > of both sides. Maybe this is for GSS/Kerberos?
>> >
>> > I wanted to address this in another CL (currently, the Cookie is not
> init'd
>> > either.) to simplify review.
> 
>> Sadly the repo is Mercurial, so god knows how one checks back in time.
> But it's
>> Not Good that we're not filling in Cookie. I'm not sure when this
> disappeared or
>> whether I screwed it up from the start.
> 
> try
> 
>  hg log -p | less
> 
> I believe it was never set.
> 
>> This change doesn't have to include it, but the next one should.
> 
> understood.
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode313
> ssh/handshake.go:313: // We don't send FirstKexFollows, but we handle
> receiving it
>> On 2013/10/22 22:12:57, agl1 wrote:
>> period at the end.
> 
> Done.
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/handshake_test.go
> File ssh/handshake_test.go (right):
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/handshake_test.go#new...
> ssh/handshake_test.go:1: // Copyright 2011 The Go Authors. All rights
> reserved.
>> On 2013/10/22 22:12:57, agl1 wrote:
>> 2013
> 
> Done.
> 
> https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go
> File ssh/transport.go (right):
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go#newcode55
> ssh/transport.go:55: func (t *transport) SessionID() []byte {
>> On 2013/10/22 22:07:02, dfc wrote:
>> does this need to return a copy of the sessionID ?
> 
> not necessarily, but it is safer and cost is neglible.
> 
>
https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go#newcode268
> ssh/transport.go:268: func newTransport(conn io.ReadWriteCloser, rand
> io.Reader, isClient bool) *transport {
>> On 2013/10/22 22:07:02, dfc wrote:
>> s/conn/rwc/
> 
> Done.
> 
> https://codereview.appspot.com/14494058/

[hanwen-google, 2013-10-23 00:56:29]

On Tue, Oct 22, 2013 at 3:07 PM,  <dave@cheney.net> wrote:
> I would really like to see this CL broken into smaller chunks. 1000
> lines changed is too many for me to review with confidence.
>
> I suggest breaking out all the sections I have noted into a separate CL.
>
> Then breaking out adding handshake{_test}.go into a separate CL.
>
> Then a final one to hook them all up.

I can do that, although I don't think it the step makes the change less
complex.

I've sent the small bits separately, and I'll have another round to
check if I can separate out more things.

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

[hanwen-google, 2013-10-23 01:22:48]

I suspect I can factor out the sshConn{} change.

On Tue, Oct 22, 2013 at 5:56 PM, Han-Wen Nienhuys <hanwen@google.com> wrote:
> On Tue, Oct 22, 2013 at 3:07 PM,  <dave@cheney.net> wrote:
>> I would really like to see this CL broken into smaller chunks. 1000
>> lines changed is too many for me to review with confidence.
>>
>> I suggest breaking out all the sections I have noted into a separate CL.
>>
>> Then breaking out adding handshake{_test}.go into a separate CL.
>>
>> Then a final one to hook them all up.
>
> I can do that, although I don't think it the step makes the change less
complex.
>
> I've sent the small bits separately, and I'll have another round to
> check if I can separate out more things.
>
> --
> Han-Wen Nienhuys
> Google Munich
> hanwen@google.com



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

[hanwen-google, 2013-10-25 20:02:57]

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

Please take another look.

[dave_cheney.net, 2013-10-25 23:40:45]

On 2013/10/25 20:02:57, hanwen-google wrote:
> Hello mailto:agl@golang.org, mailto:dave@cheney.net, mailto:jpsugar@google.com
(cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Does this include the changes we landed to void Server/ClientConn implmenting
io.ReadWriter ? There is some sshConn stuff in this CL.

[hanwen-google, 2013-10-25 23:51:40]

On 2013/10/25 23:40:45, dfc wrote:
> On 2013/10/25 20:02:57, hanwen-google wrote:
> > Hello mailto:agl@golang.org, mailto:dave@cheney.net,
mailto:jpsugar@google.com
> (cc:
> > mailto:golang-dev@googlegroups.com),
> > 
> > Please take another look.
> 
> Does this include the changes we landed to void Server/ClientConn implmenting
> io.ReadWriter ? There is some sshConn stuff in this CL.

It includes them. Now that the transport moves down one level, we don't have
access to the RWC anymore, so I reintroduced to hold net.Conn. On top of that,
it gets rid of the RemoteAddr/LocalAddr/Close implementation duplicated between
server and client.

I'm not attached to how we do this, but this was the simplest I could think of.
Suggestions?

[agl1, 2013-10-29 14:33:47]

LGTM. (For landing in gosshnew.)

https://codereview.appspot.com/14494058/diff/460001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/14494058/diff/460001/ssh/common.go#newcode153
ssh/common.go:153: // The maximum amount of data sent or received after which a
s/amount of data/number of bytes/

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go
File ssh/handshake.go (right):

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode30
ssh/handshake.go:30: // Returns the session ID. prepareKeyChange must have been
getSessionID returns the session ID. ...

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode40
ssh/handshake.go:40: // requestKeyChange asks remote side to change keys. All
"asks the"

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode45
ssh/handshake.go:45: // Returns the session ID. This is only valid after the
first
getSessionID returns the session ID ...

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode175
ssh/handshake.go:175: if p[0] == msgKexInit {
(nit): by testing p[0] != msgKeyInit and returning immediately, the code to
handle the msgKexInit could have one less level of indentation.

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode223
ssh/handshake.go:223: // internalSendKexInit sends key change message. t.mu must
be locked
"sends a"

[hanwen-google, 2013-10-29 14:45:22]

If I remove the 1G rekeying and RekeyThreshold, can we land it in go.crypto? It
fixes bug with rekeying while writing, and this is necessary to land the
backward compatible version of the mux change (as the latter breaks the
server-side rekeying support)

https://codereview.appspot.com/14494058/diff/460001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/14494058/diff/460001/ssh/common.go#newcode153
ssh/common.go:153: // The maximum amount of data sent or received after which a
On 2013/10/29 14:33:47, agl1 wrote:
> s/amount of data/number of bytes/

Done.

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go
File ssh/handshake.go (right):

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode30
ssh/handshake.go:30: // Returns the session ID. prepareKeyChange must have been
On 2013/10/29 14:33:47, agl1 wrote:
> getSessionID returns the session ID. ...

Done.

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode40
ssh/handshake.go:40: // requestKeyChange asks remote side to change keys. All
On 2013/10/29 14:33:47, agl1 wrote:
> "asks the"

Done.

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode45
ssh/handshake.go:45: // Returns the session ID. This is only valid after the
first
On 2013/10/29 14:33:47, agl1 wrote:
> getSessionID returns the session ID ...

Done.

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode175
ssh/handshake.go:175: if p[0] == msgKexInit {
On 2013/10/29 14:33:47, agl1 wrote:
> (nit): by testing p[0] != msgKeyInit and returning immediately, the code to
> handle the msgKexInit could have one less level of indentation.

Done.

https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode223
ssh/handshake.go:223: // internalSendKexInit sends key change message. t.mu must
be locked
On 2013/10/29 14:33:47, agl1 wrote:
> "sends a"

Done.

[hanwen-google, 2013-10-31 12:53:07]

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

Please take another look.

[hanwen-google, 2013-10-31 12:54:57]

Also, RekeyThreshold is backward compatible, assuming people don't use
unnamed inits of structs. Leaving it in lets us test against sshd.

On Tue, Oct 29, 2013 at 3:45 PM,  <hanwen@google.com> wrote:
> If I remove the 1G rekeying and RekeyThreshold, can we land it in
> go.crypto? It fixes bug with rekeying while writing, and this is
> necessary to land the backward compatible version of the mux change (as
> the latter breaks the server-side rekeying support)
>
>
>
> https://codereview.appspot.com/14494058/diff/460001/ssh/common.go
> File ssh/common.go (right):
>
> https://codereview.appspot.com/14494058/diff/460001/ssh/common.go#newcode153
> ssh/common.go:153: // The maximum amount of data sent or received after
> which a
> On 2013/10/29 14:33:47, agl1 wrote:
>>
>> s/amount of data/number of bytes/
>
>
> Done.
>
>
> https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go
> File ssh/handshake.go (right):
>
>
https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode30
> ssh/handshake.go:30: // Returns the session ID. prepareKeyChange must
> have been
> On 2013/10/29 14:33:47, agl1 wrote:
>>
>> getSessionID returns the session ID. ...
>
>
> Done.
>
>
>
https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode40
> ssh/handshake.go:40: // requestKeyChange asks remote side to change
> keys. All
> On 2013/10/29 14:33:47, agl1 wrote:
>>
>> "asks the"
>
>
> Done.
>
>
>
https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode45
> ssh/handshake.go:45: // Returns the session ID. This is only valid after
> the first
> On 2013/10/29 14:33:47, agl1 wrote:
>>
>> getSessionID returns the session ID ...
>
>
> Done.
>
>
>
https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode175
> ssh/handshake.go:175: if p[0] == msgKexInit {
> On 2013/10/29 14:33:47, agl1 wrote:
>>
>> (nit): by testing p[0] != msgKeyInit and returning immediately, the
>
> code to
>>
>> handle the msgKexInit could have one less level of indentation.
>
>
> Done.
>
>
>
https://codereview.appspot.com/14494058/diff/460001/ssh/handshake.go#newcode223
> ssh/handshake.go:223: // internalSendKexInit sends key change message.
> t.mu must be locked
> On 2013/10/29 14:33:47, agl1 wrote:
>>
>> "sends a"
>
>
> Done.
>
> https://codereview.appspot.com/14494058/



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

[agl1, 2013-10-31 14:36:34]

LGTM for the diffs.

(Note: If I've said LGTM, but have included a few comments, you don't need to
wait for another review if you've just fixed the comments.)

[dave_cheney.net, 2013-11-03 08:23:03]

LGTM with minor comments.

https://codereview.appspot.com/14494058/diff/580001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/14494058/diff/580001/ssh/common.go#newcode154
ssh/common.go:154: // new key is negotiated.
Please add a statement about the default, something like

// If unspecified then the default value is every NNN bytes.

As a side note, setting this value to 128 bytes causes the client to never
connect to the remote side. Maybe also add that the value will be clamped at
4096 or higher.

https://codereview.appspot.com/14494058/diff/580001/ssh/test/session_test.go
File ssh/test/session_test.go (right):

https://codereview.appspot.com/14494058/diff/580001/ssh/test/session_test.go#...
ssh/test/session_test.go:156: t.Fatalf("Expected %d bytes but read only %d from
remote command", 2048, n)
the test doesn't match the failure message

[hanwen-google, 2013-11-04 18:04:02]

this has been submitted to gosshnew in
https://code.google.com/p/gosshnew/source/detail?r=2e99c4d7cb96a1285281daad7f...

[hanwen-google, 2013-11-04 18:04:17]

*** Abandoned ***