code review 13966043: go.crypto/ssh: parse DSA private keys too.

go.crypto/ssh: parse DSA private keys too.

[hanwen-google, 2013-09-26 12:19:41]

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

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

[hanwen-google, 2013-09-26 12:20:49]

I guess this should be in the x509 module, but the standard lib is in release
freeze now?

[agl1, 2013-09-26 15:00:07]

Yes, I don't think we're adding anything to the standard lib at the moment,
although this is a reasonable thing to move across in the future.

https://codereview.appspot.com/13966043/diff/2001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13966043/diff/2001/ssh/keys.go#newcode596
ssh/keys.go:596: 
ASN.1

https://codereview.appspot.com/13966043/diff/2001/ssh/keys.go#newcode597
ssh/keys.go:597: 
Are you trying to refer to the OpenSSL manpage here?

https://codereview.appspot.com/13966043/diff/2001/ssh/keys.go#newcode609
ssh/keys.go:609: 
return nil, errors.New("ssh: trailing garbage after DSA private key")

https://codereview.appspot.com/13966043/diff/2001/ssh/keys.go#newcode611
ssh/keys.go:611: 
This test should be above the if len(rest) condition because, if asn1 returns an
explicit error, it's probably more useful.

Also, it might be good to return something like errors.New("ssh: failed to parse
DSA private key: " + err.Error())

[hanwen-google, 2013-09-26 15:12:19]

On Thu, Sep 26, 2013 at 5:00 PM,  <agl@golang.org> wrote:
> Yes, I don't think we're adding anything to the standard lib at the
> moment, although this is a reasonable thing to move across in the
> future.

added a TODO

> https://codereview.appspot.com/13966043/diff/2001/ssh/keys.go
> File ssh/keys.go (right):
>
> https://codereview.appspot.com/13966043/diff/2001/ssh/keys.go#newcode596
> ssh/keys.go:596:
> ASN.1
>

done.

> https://codereview.appspot.com/13966043/diff/2001/ssh/keys.go#newcode597
> ssh/keys.go:597:
> Are you trying to refer to the OpenSSL manpage here?

yes. (Is there a more authoritative source for this?)

> https://codereview.appspot.com/13966043/diff/2001/ssh/keys.go#newcode609
> ssh/keys.go:609:
> return nil, errors.New("ssh: trailing garbage after DSA private key")

done.


> https://codereview.appspot.com/13966043/diff/2001/ssh/keys.go#newcode611
> ssh/keys.go:611:
> This test should be above the if len(rest) condition because, if asn1
> returns an explicit error, it's probably more useful.
>
> Also, it might be good to return something like errors.New("ssh: failed
> to parse DSA private key: " + err.Error())

done.

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

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

[agl1, 2013-09-26 15:18:07]

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

go.crypto/ssh: parse DSA private keys too.

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

Committer: Adam Langley <agl@golang.org>

https://codereview.appspot.com/13966043/diff/20001/ssh/keys_test.go
File ssh/keys_test.go (right):

https://codereview.appspot.com/13966043/diff/20001/ssh/keys_test.go#newcode152
ssh/keys_test.go:152: t.Fatalf("ParsePrivateKey", err)
I'll a "%s" to this Fatalf call before landing.

[hanwen-google, 2013-09-26 15:24:24]

*** Abandoned ***