code review 13352055: go.crypto/ssh: separate kex algorithms into kexAlgorith...

go.crypto/ssh: separate kex algorithms into kexAlgorithm class.

Adds readPacket() to conn, and renames conn to packetConn.
Key exchanges operate on packetConn, so they can be
unittested.

[hanwen-google, 2013-09-23 13:32:00]

Hello agl@golang.org, 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-09-23 13:36:33]

I originally also wanted to unittest user auth separately, hence the changes to
client_auth.go, but that is non-trivial, as the callbacks get a ServerConn
argument. Is that so the callbacks can do remote-address specific checks?

I left packetConn as argument to the client functions, as it clarifies that the
functions only do packet i/o.

[jpsugar, 2013-09-23 21:19:05]

https://codereview.appspot.com/13352055/diff/3001/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/13352055/diff/3001/ssh/client_auth.go#newcode84
ssh/client_auth.go:84: auth(session []byte, user string, t packetConn, rand
io.Reader) (bool, []string, error)
I hate to ask, but can we rename "t" since it's no longer a "t"ransport?

https://codereview.appspot.com/13352055/diff/3001/ssh/kex.go
File ssh/kex.go (right):

https://codereview.appspot.com/13352055/diff/3001/ssh/kex.go#newcode72
ssh/kex.go:72: var kexes = map[string]kexAlgorithm{}
I'm not thrilled by this name. kexAlgoMap, perhaps?

[hanwen-google, 2013-09-24 12:55:24]

https://codereview.appspot.com/13352055/diff/3001/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/13352055/diff/3001/ssh/client_auth.go#newcode84
ssh/client_auth.go:84: auth(session []byte, user string, t packetConn, rand
io.Reader) (bool, []string, error)
On 2013/09/23 21:19:05, jpsugar wrote:
> I hate to ask, but can we rename "t" since it's no longer a "t"ransport?

Done.

https://codereview.appspot.com/13352055/diff/3001/ssh/kex.go
File ssh/kex.go (right):

https://codereview.appspot.com/13352055/diff/3001/ssh/kex.go#newcode72
ssh/kex.go:72: var kexes = map[string]kexAlgorithm{}
On 2013/09/23 21:19:05, jpsugar wrote:
> I'm not thrilled by this name. kexAlgoMap, perhaps?

Done.

[agl1, 2013-09-24 15:11:11]

LGTM with nits.

https://codereview.appspot.com/13352055/diff/10001/ssh/kex.go
File ssh/kex.go (right):

https://codereview.appspot.com/13352055/diff/10001/ssh/kex.go#newcode1
ssh/kex.go:1: package ssh
Copyright comment.

https://codereview.appspot.com/13352055/diff/10001/ssh/kex.go#newcode36
ssh/kex.go:36: // Data that always goes into the session hash.
// handshakeMagics contains data that is always incluced in the session hash.

https://codereview.appspot.com/13352055/diff/10001/ssh/kex.go#newcode53
ssh/kex.go:53: Hash() crypto.Hash
Hash needs a comment as it's not immediately obvious what it's for.

[hanwen-google, 2013-09-24 15:28:25]

https://codereview.appspot.com/13352055/diff/10001/ssh/kex.go
File ssh/kex.go (right):

https://codereview.appspot.com/13352055/diff/10001/ssh/kex.go#newcode1
ssh/kex.go:1: package ssh
On 2013/09/24 15:11:11, agl1 wrote:
> Copyright comment.

Done.

https://codereview.appspot.com/13352055/diff/10001/ssh/kex.go#newcode36
ssh/kex.go:36: // Data that always goes into the session hash.
On 2013/09/24 15:11:11, agl1 wrote:
> // handshakeMagics contains data that is always incluced in the session hash.

Done.

https://codereview.appspot.com/13352055/diff/10001/ssh/kex.go#newcode53
ssh/kex.go:53: Hash() crypto.Hash
On 2013/09/24 15:11:11, agl1 wrote:
> Hash needs a comment as it's not immediately obvious what it's for.

Done.

[agl1, 2013-09-24 16:26:41]

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

go.crypto/ssh: separate kex algorithms into kexAlgorithm class.

Adds readPacket() to conn, and renames conn to packetConn.
Key exchanges operate on packetConn, so they can be
unittested.

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

Committer: Adam Langley <agl@golang.org>

[jpsugar, 2013-09-24 16:28:04]

LGTM

(for the record)

[hanwen-google, 2013-09-24 16:46:30]

*** Abandoned ***