code review 5447049: exp/ssh: cleanup client auth tests

exp/ssh: cleanup client auth tests

This CL cleans up the client auth tests, making the
individual test body more manageable.

Also, adds tests for rsa and dsa key negotiation.

Finally, remove the package level use of the variable
strings, which avoids conflicting with the strings pkg.

[dave_cheney.net, 2011-12-01 10:52:23]

Hello gustav.paul@gmail.com, agl@golang.org, n13m3y3r@gmail.com, rsc@golang.org
(cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[gpaul, 2011-12-01 11:48:37]

On 2011/12/01 10:52:23, dfc wrote:
> Hello mailto:gustav.paul@gmail.com, mailto:agl@golang.org,
mailto:n13m3y3r@gmail.com, mailto:rsc@golang.org
> (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

LGTM

[niemeyer, 2011-12-01 12:31:31]

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
File src/pkg/exp/ssh/client_auth_test.go (right):

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:65: panic("known key type")
un? :-)

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:105: var rsakey *rsa.PrivateKey
Would be nicer as a single var block: var ( ... )

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:108: var pw = password("tiger")
The k and pw names are on the short side for being globals.

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:151: t.Fatal(err)
Fatal => Error+return, as we discussed in the prior CL.

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:156: defer c.Close()
s/defer//

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:172: defer c.Close()
s/defer//

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:221: c.Close()
Should be above Fatalf.

[dave_cheney.net, 2011-12-01 22:21:18]

Thanks for your comments Gustavo. PTAL

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
File src/pkg/exp/ssh/client_auth_test.go (right):

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:65: panic("known key type")
On 2011/12/01 12:31:32, niemeyer wrote:
> un? :-)

Done.

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:105: var rsakey *rsa.PrivateKey
On 2011/12/01 12:31:32, niemeyer wrote:
> Would be nicer as a single var block: var ( ... )

Done.

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:108: var pw = password("tiger")
On 2011/12/01 12:31:32, niemeyer wrote:
> The k and pw names are on the short side for being globals.

Done.

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:151: t.Fatal(err)
On 2011/12/01 12:31:32, niemeyer wrote:
> Fatal => Error+return, as we discussed in the prior CL.

Done.

https://codereview.appspot.com/5447049/diff/5002/src/pkg/exp/ssh/client_auth_...
src/pkg/exp/ssh/client_auth_test.go:156: defer c.Close()
On 2011/12/01 12:31:32, niemeyer wrote:
> s/defer//

Done.

[dave_cheney.net, 2011-12-01 22:21:52]

Hello gustav.paul@gmail.com, agl@golang.org, n13m3y3r@gmail.com, rsc@golang.org
(cc: golang-dev@googlegroups.com),

Please take another look.

[niemeyer, 2011-12-01 22:27:36]

LGTM

Leaving for agl or rsc.

[dave_cheney.net, 2011-12-04 03:07:56]

The latest patch set changes the name of the mock server func from
authServerListen to newMockAuthServer.

[dave_cheney.net, 2011-12-06 20:25:24]

Hello gustav.paul@gmail.com, agl@golang.org, n13m3y3r@gmail.com, rsc@golang.org
(cc: golang-dev@googlegroups.com),

Please take another look.

[agl1, 2011-12-06 23:13:32]

*** Submitted as http://code.google.com/p/go/source/detail?r=f6fdc83852fd ***

exp/ssh: cleanup client auth tests

This CL cleans up the client auth tests, making the
individual test body more manageable.

Also, adds tests for rsa and dsa key negotiation.

Finally, remove the package level use of the variable
strings, which avoids conflicting with the strings pkg.

R=gustav.paul, agl, n13m3y3r, rsc
CC=golang-dev
http://codereview.appspot.com/5447049

Committer: Adam Langley <agl@golang.org>

http://codereview.appspot.com/5447049/diff/2005/src/pkg/exp/ssh/client_auth_t...
File src/pkg/exp/ssh/client_auth_test.go (right):

http://codereview.appspot.com/5447049/diff/2005/src/pkg/exp/ssh/client_auth_t...
src/pkg/exp/ssh/client_auth_test.go:23: const _pem = `-----BEGIN RSA PRIVATE
KEY-----
renamed to testServerPrivateKey to make it clear which is the server key and
which the client when I remove rsa.Generate.

http://codereview.appspot.com/5447049/diff/2005/src/pkg/exp/ssh/client_auth_t...
src/pkg/exp/ssh/client_auth_test.go:70: hashFunc := crypto.SHA1
I added an "import _ "crypto/sha1"" to make sure that this New call works.

http://codereview.appspot.com/5447049/diff/2005/src/pkg/exp/ssh/client_auth_t...
src/pkg/exp/ssh/client_auth_test.go:129: rsakey, err =
rsa.GenerateKey(rand.Reader, 512)
generating even a 512-bit key is expensive on our ARM builders, so I switched
this to using a static key.

http://codereview.appspot.com/5447049/diff/2005/src/pkg/exp/ssh/client_auth_t...
src/pkg/exp/ssh/client_auth_test.go:164: c.Close()
made this a defer higher up so that c doesn't leak