Issue 6208087: code review 6208087: crypto/x509: Add ECDSA support (Closed)
|
Created:
12 years, 9 months ago by b Modified:
12 years, 9 months ago Reviewers:
CC:
golang-dev, agl1, rsc Visibility:
Public. |
Descriptioncrypto/x509: Add ECDSA support
Patch Set 1 #Patch Set 2 : diff -r 3c702dae5f50 https://code.google.com/p/go/ #
Total comments: 5
Patch Set 3 : diff -r 3c702dae5f50 https://code.google.com/p/go/ #
Total comments: 1
Patch Set 4 : diff -r 3c702dae5f50 https://code.google.com/p/go/ #MessagesTotal messages: 13
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509.go File src/pkg/crypto/x509/x509.go (right): http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509.go#n... src/pkg/crypto/x509/x509.go:503: return errors.New("ECDSA signature contained zero or negative values") errors should have a package prefix, i.e. "x509: ECDSA signature...". (The errors above are wrong too.) http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509.go#n... src/pkg/crypto/x509/x509.go:596: return nil, errors.New("unsupported EC named curve") these errors also need the package prefix. http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509.go#n... src/pkg/crypto/x509/x509.go:599: x, y = elliptic.Unmarshal(namedCurve, asn1Data) can use := here. http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509_test.go File src/pkg/crypto/x509/x509_test.go (right): http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509_test... src/pkg/crypto/x509/x509_test.go:27: func dummySHA2Func() (hash.Hash, hash.Hash) { you don't need this to avoid the unused import error. Simply import as: _ "crypto/sha256" http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509_test... src/pkg/crypto/x509/x509_test.go:398: t.Fatalf("Parsed key was not a ECDSA key: %s", err) I think you want to change "%s" to "%#v" and "err" to "cert.PublicKey"
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, agl@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
This looks pretty good, but I'll have to check why the P521 case isn't working first. Hopefully within a day or so. http://codereview.appspot.com/6208087/diff/6001/src/pkg/crypto/x509/x509_test.go File src/pkg/crypto/x509/x509_test.go (right): http://codereview.appspot.com/6208087/diff/6001/src/pkg/crypto/x509/x509_test... src/pkg/crypto/x509/x509_test.go:21: _ "hash" I think you can drop this now.
Sign in to reply to this message.
Thanks for the quick turnaround! I've updated based on your feedback. What is wrong with the signature errors? Are the DSA errors also incorrect? They are also missing a package prefix, regardless. On Mon, May 21, 2012 at 11:36 AM, <agl@golang.org> wrote: > > http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509.go > File src/pkg/crypto/x509/x509.go (right): > > http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509.go#n... > src/pkg/crypto/x509/x509.go:503: return errors.New("ECDSA signature > contained zero or negative values") > errors should have a package prefix, i.e. "x509: ECDSA signature...". > (The errors above are wrong too.) > > http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509.go#n... > src/pkg/crypto/x509/x509.go:596: return nil, errors.New("unsupported EC > named curve") > these errors also need the package prefix. > > http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509.go#n... > src/pkg/crypto/x509/x509.go:599: x, y = elliptic.Unmarshal(namedCurve, > asn1Data) > can use := here. > > http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509_test.go > File src/pkg/crypto/x509/x509_test.go (right): > > http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509_test... > src/pkg/crypto/x509/x509_test.go:27: func dummySHA2Func() (hash.Hash, > hash.Hash) { > you don't need this to avoid the unused import error. Simply import as: > > _ "crypto/sha256" > > http://codereview.appspot.com/6208087/diff/2001/src/pkg/crypto/x509/x509_test... > src/pkg/crypto/x509/x509_test.go:398: t.Fatalf("Parsed key was not a > ECDSA key: %s", err) > I think you want to change "%s" to "%#v" and "err" to "cert.PublicKey" > > http://codereview.appspot.com/6208087/
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, agl@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On Mon, May 21, 2012 at 2:45 PM, Benjamin Black <b@b3k.us> wrote: > Thanks for the quick turnaround! I've updated based on your feedback. > What is wrong with the signature errors? Are the DSA errors also > incorrect? They are also missing a package prefix, regardless. Yes, the DSA errors are also wrong. I should have caught that in review, but didn't. Cheers AGL
Sign in to reply to this message.
The current error prefixes are inconsistent: some are missing entirely, some are x509, some are crypto/x509. What is the preferred structure? On Mon, May 21, 2012 at 11:50 AM, Adam Langley <agl@golang.org> wrote: > On Mon, May 21, 2012 at 2:45 PM, Benjamin Black <b@b3k.us> wrote: >> Thanks for the quick turnaround! I've updated based on your feedback. >> What is wrong with the signature errors? Are the DSA errors also >> incorrect? They are also missing a package prefix, regardless. > > Yes, the DSA errors are also wrong. I should have caught that in > review, but didn't. > > > Cheers > > AGL
Sign in to reply to this message.
Let's not make this CL about error prefixes. Please update the ones you added as per agl's comments, but leave the others alone. Thanks. Russ
Sign in to reply to this message.
Of course, I am not touching the others. Just bringing the problem to agl's attention and asking which format is preferred for this and future changes. On Mon, May 21, 2012 at 11:59 AM, Russ Cox <rsc@golang.org> wrote: > Let's not make this CL about error prefixes. Please update the ones > you added as per agl's comments, but leave the others alone. > > Thanks. > Russ
Sign in to reply to this message.
Found the issue: http://codereview.appspot.com/6215074 On Mon, May 21, 2012 at 11:45 AM, <agl@golang.org> wrote: > This looks pretty good, but I'll have to check why the P521 case isn't > working first. Hopefully within a day or so. > > > http://codereview.appspot.com/6208087/diff/6001/src/pkg/crypto/x509/x509_test.go > File src/pkg/crypto/x509/x509_test.go (right): > > http://codereview.appspot.com/6208087/diff/6001/src/pkg/crypto/x509/x509_test... > src/pkg/crypto/x509/x509_test.go:21: _ "hash" > I think you can drop this now. > > http://codereview.appspot.com/6208087/
Sign in to reply to this message.
I reenabled the P521 test and it now passes. I also reworked the tests a bit to save some duplication. Will submit in a sec. Cheers AGL
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=83f71485e378 *** crypto/x509: Add ECDSA support R=golang-dev, agl, rsc CC=golang-dev http://codereview.appspot.com/6208087 Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.
|
