code review 6938067: go.crypto/ssh: some cleanup

go.crypto/ssh: some cleanup
Simplify MarshalAuthorizedKey by using the algoName func.
Make the algoName func be very specific about supported key types in openssh certs.
Generalize some of the commentary that previously mentioned specific key types.

[jmpittman, 2012-12-15 04:20:37]

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

[dfc, 2012-12-15 05:03:32]

Thank you. Some very minor comments.

Please be aware of 6948057, which may conflict.

https://codereview.appspot.com/6938067/diff/5001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/6938067/diff/5001/ssh/certs.go#newcode15
ssh/certs.go:15: // for certificate types supported in this package.
s/in/by/ ?

https://codereview.appspot.com/6938067/diff/5001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/6938067/diff/5001/ssh/common.go#newcode272
ssh/common.go:272: }
Now we have a list of known supported algos, should there be a test that this
method keeps working if that list grows/shrinks?

https://codereview.appspot.com/6938067/diff/5001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/6938067/diff/5001/ssh/keys.go#newcode17
ssh/keys.go:17: // These constants represent the algorithm names for key types
supported in this
see previous

https://codereview.appspot.com/6938067/diff/5001/ssh/keys.go#newcode343
ssh/keys.go:343: // MarshalPublicKey serializes a supported key or certificate
for use in the
/s/in/by ?

[jmpittman, 2012-12-16 03:55:31]

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

Please take another look.

[jmpittman, 2012-12-16 03:55:49]

For the s/in/by references, I actually had "by" at first, but then changed to
"in."  I should have stuck with my first choice.  :-)

https://codereview.appspot.com/6938067/diff/5001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/6938067/diff/5001/ssh/certs.go#newcode15
ssh/certs.go:15: // for certificate types supported in this package.
On 2012/12/15 05:03:32, dfc wrote:
> s/in/by/ ?

Done.

https://codereview.appspot.com/6938067/diff/5001/ssh/common.go
File ssh/common.go (right):

https://codereview.appspot.com/6938067/diff/5001/ssh/common.go#newcode272
ssh/common.go:272: }
On 2012/12/15 05:03:32, dfc wrote:
> Now we have a list of known supported algos, should there be a test that this
> method keeps working if that list grows/shrinks?

I added a test.  PTAL and let me know if that is satisfactory.  I am a little
iffy about the second test.

https://codereview.appspot.com/6938067/diff/5001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/6938067/diff/5001/ssh/keys.go#newcode17
ssh/keys.go:17: // These constants represent the algorithm names for key types
supported in this
On 2012/12/15 05:03:32, dfc wrote:
> see previous

Done.

https://codereview.appspot.com/6938067/diff/5001/ssh/keys.go#newcode343
ssh/keys.go:343: // MarshalPublicKey serializes a supported key or certificate
for use in the
On 2012/12/15 05:03:32, dfc wrote:
> /s/in/by ?

Done.

[dfc, 2012-12-16 07:58:35]

LGTM. If there is no rush, i'll leave this for a day or so for agl to have a
look.

I see what you mean about the second test. I guess what I wanted was a way to
test if people had added extra constants to that section, but had not added
support to the various decoding routines. I think the first test accurately
captures that, and is simple for others to extend if/when they add additional
algos.

[jmpittman, 2012-12-16 10:45:45]

No rush.  Thanks!

[agl1, 2012-12-16 15:47:22]

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

go.crypto/ssh: some cleanup
Simplify MarshalAuthorizedKey by using the algoName func.
Make the algoName func be very specific about supported key types in openssh
certs.
Generalize some of the commentary that previously mentioned specific key types.

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

Committer: Adam Langley <agl@golang.org>

[remyoudompheng, 2013-07-21 19:59:33]

R=close