code review 6873060: go.crypto/ssh: Add support for ECDSA keys and certs.

go.crypto/ssh: Add support for ECDSA keys and certs.

[jmpittman, 2012-12-11 01:12:00]

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

[jmpittman, 2012-12-11 01:16:03]

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

Please take another look.

[jmpittman, 2012-12-11 14:33:24]

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

Please take another look.

[agl1, 2012-12-11 15:32:16]

LGTM. I'll wait a bit to see whether dfc has anything to note.

If I forget about it in 48 hours, please ping :)

https://codereview.appspot.com/6873060/diff/3004/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/6873060/diff/3004/ssh/certs.go#newcode8
ssh/certs.go:8: package ssh
I don't think the References comment should live above the package line.

[jmpittman, 2012-12-11 15:52:02]

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

Please take another look.

[jmpittman, 2012-12-11 15:52:07]

Thanks for the quick response.  I think I covered most everything with the last
two updates to the CL.  I left the Authorized Keys stuff alone (for now) for the
parsing (where it clearly indicates not supporting beyond ssh-rsa and ssh-dss). 
I think that could be saved for a later CL.

https://codereview.appspot.com/6873060/diff/3004/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/6873060/diff/3004/ssh/certs.go#newcode8
ssh/certs.go:8: package ssh
On 2012/12/11 15:32:16, agl1 wrote:
> I don't think the References comment should live above the package line.

Unless you feel it not necessary, I moved it up so that it would show up in
package documentation via godoc.  Godoc seems to create a nice little box for
references, but only when they appear above the package line.  An example is
http://golang.org/pkg/archive/tar/.

Although, after double checking, it is not showing up when I run a local copy of
godoc.  It may be that I would have to put the reference comment in another
file.  I will move it back for now.

[dfc, 2012-12-11 21:52:00]

Sorry I haven't contributed much in the way of reviewing ssh patches,
this is very poor form on my part, I'll have some time after work
today to review.

On Wed, Dec 12, 2012 at 2:52 AM,  <jmpittman@google.com> wrote:
> Thanks for the quick response.  I think I covered most everything with
> the last two updates to the CL.  I left the Authorized Keys stuff alone
> (for now) for the parsing (where it clearly indicates not supporting
> beyond ssh-rsa and ssh-dss).  I think that could be saved for a later
> CL.
>
>
>
> https://codereview.appspot.com/6873060/diff/3004/ssh/certs.go
> File ssh/certs.go (right):
>
> https://codereview.appspot.com/6873060/diff/3004/ssh/certs.go#newcode8
> ssh/certs.go:8: package ssh
> On 2012/12/11 15:32:16, agl1 wrote:
>>
>> I don't think the References comment should live above the package
>
> line.
>
> Unless you feel it not necessary, I moved it up so that it would show up
> in package documentation via godoc.  Godoc seems to create a nice little
> box for references, but only when they appear above the package line.
> An example is http://golang.org/pkg/archive/tar/.
>
> Although, after double checking, it is not showing up when I run a local
> copy of godoc.  It may be that I would have to put the reference comment
> in another file.  I will move it back for now.
>
> https://codereview.appspot.com/6873060/

[dfc, 2012-12-13 11:16:03]

LGTM, very nice, thank you. A few minor comments if you feel like addressing
them in this CL.

https://codereview.appspot.com/6873060/diff/12001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/6873060/diff/12001/ssh/certs.go#newcode47
ssh/certs.go:47: // and ecdsa-sha2-nistp521-cert-v01@openssh.com.
Suggestion: Maybe the constants above should be exported, then we can move this
part of the comment to describe those directly.

https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go#newcode124
ssh/keys.go:124: return key, in, ok
/s/return.*/return/

https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go#newcode347
ssh/keys.go:347: panic("unexpected key type")
there are a lot of panics that all do the same thing. Is a label useful here ?

[jmpittman, 2012-12-13 18:38:49]

https://codereview.appspot.com/6873060/diff/12001/ssh/certs.go
File ssh/certs.go (right):

https://codereview.appspot.com/6873060/diff/12001/ssh/certs.go#newcode47
ssh/certs.go:47: // and ecdsa-sha2-nistp521-cert-v01@openssh.com.
On 2012/12/13 11:16:03, dfc wrote:
> Suggestion: Maybe the constants above should be exported, then we can move
this
> part of the comment to describe those directly.

Will do in a follow up CL.

https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go#newcode124
ssh/keys.go:124: return key, in, ok
On 2012/12/13 11:16:03, dfc wrote:
> /s/return.*/return/

Just return will only return out, rest, and ok which will be nil, nil, true. 
Here I am following the convention we have with the other parse functions and do
an explicit return of key, in, ok so they should have some value beyond nil.

https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go#newcode347
ssh/keys.go:347: panic("unexpected key type")
On 2012/12/13 11:16:03, dfc wrote:
> there are a lot of panics that all do the same thing. Is a label useful here ?

Yeah, I agree. It does seem a bit excessive considering it just says the same
thing.  A label would be good.  We could also change the message for each of
these to give something more useful with respect to the context at hand.  For
example...

panic("unexpected bit size for *ecdsa.PublicKey")
panic("unexpected public key type for ssh certificate")

Or we could change this function to return an error along with the []byte.

Which do you think works and reads best?  I am fine with doing any of those.

[dfc, 2012-12-13 18:45:11]

On 14 Dec 2012 05:38, <jmpittman@google.com> wrote:
>
>
> https://codereview.appspot.com/6873060/diff/12001/ssh/certs.go
> File ssh/certs.go (right):
>
> https://codereview.appspot.com/6873060/diff/12001/ssh/certs.go#newcode47
> ssh/certs.go:47: // and ecdsa-sha2-nistp521-cert-v01@openssh.com.
> On 2012/12/13 11:16:03, dfc wrote:
>>
>> Suggestion: Maybe the constants above should be exported, then we can
>
> move this
>>
>> part of the comment to describe those directly.
>
>
> Will do in a follow up CL.
>
>
> https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go
> File ssh/keys.go (right):
>
> https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go#newcode124
> ssh/keys.go:124: return key, in, ok
> On 2012/12/13 11:16:03, dfc wrote:
>>
>> /s/return.*/return/
>
>
> Just return will only return out, rest, and ok which will be nil, nil,
> true.  Here I am following the convention we have with the other parse
> functions and do an explicit return of key, in, ok so they should have
> some value beyond nil.

My bad, I'm trying to learn to not hate on named return vars.

>
>
> https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go#newcode347
> ssh/keys.go:347: panic("unexpected key type")
> On 2012/12/13 11:16:03, dfc wrote:
>>
>> there are a lot of panics that all do the same thing. Is a label
>
> useful here ?
>
> Yeah, I agree. It does seem a bit excessive considering it just says the
> same thing.  A label would be good.  We could also change the message
> for each of these to give something more useful with respect to the
> context at hand.  For example...
>
> panic("unexpected bit size for *ecdsa.PublicKey")
> panic("unexpected public key type for ssh certificate")
>
> Or we could change this function to return an error along with the
> []byte.
>
> Which do you think works and reads best?  I am fine with doing any of
> those.

What is there is fine, let's do a cleanup in the next CL. Would you like me
to submit this ?

>
> https://codereview.appspot.com/6873060/

[jmpittman, 2012-12-13 18:46:23]

On 2012/12/13 18:45:11, dfc wrote:
> On 14 Dec 2012 05:38, <mailto:jmpittman@google.com> wrote:
> >
> >
> > https://codereview.appspot.com/6873060/diff/12001/ssh/certs.go
> > File ssh/certs.go (right):
> >
> > https://codereview.appspot.com/6873060/diff/12001/ssh/certs.go#newcode47
> > ssh/certs.go:47: // and mailto:ecdsa-sha2-nistp521-cert-v01@openssh.com.
> > On 2012/12/13 11:16:03, dfc wrote:
> >>
> >> Suggestion: Maybe the constants above should be exported, then we can
> >
> > move this
> >>
> >> part of the comment to describe those directly.
> >
> >
> > Will do in a follow up CL.
> >
> >
> > https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go
> > File ssh/keys.go (right):
> >
> > https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go#newcode124
> > ssh/keys.go:124: return key, in, ok
> > On 2012/12/13 11:16:03, dfc wrote:
> >>
> >> /s/return.*/return/
> >
> >
> > Just return will only return out, rest, and ok which will be nil, nil,
> > true.  Here I am following the convention we have with the other parse
> > functions and do an explicit return of key, in, ok so they should have
> > some value beyond nil.
> 
> My bad, I'm trying to learn to not hate on named return vars.
> 
> >
> >
> > https://codereview.appspot.com/6873060/diff/12001/ssh/keys.go#newcode347
> > ssh/keys.go:347: panic("unexpected key type")
> > On 2012/12/13 11:16:03, dfc wrote:
> >>
> >> there are a lot of panics that all do the same thing. Is a label
> >
> > useful here ?
> >
> > Yeah, I agree. It does seem a bit excessive considering it just says the
> > same thing.  A label would be good.  We could also change the message
> > for each of these to give something more useful with respect to the
> > context at hand.  For example...
> >
> > panic("unexpected bit size for *ecdsa.PublicKey")
> > panic("unexpected public key type for ssh certificate")
> >
> > Or we could change this function to return an error along with the
> > []byte.
> >
> > Which do you think works and reads best?  I am fine with doing any of
> > those.
> 
> What is there is fine, let's do a cleanup in the next CL. Would you like me
> to submit this ?
> 
> >
> > https://codereview.appspot.com/6873060/

Please.  Thank you!

[dfc, 2012-12-13 18:52:35]

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

go.crypto/ssh: Add support for ECDSA keys and certs.

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

Committer: Dave Cheney <dave@cheney.net>

[jmpittman, 2012-12-13 21:56:44]

*** Abandoned ***