code review 6767045: crypto/x509: add support for SEC1/EC private keys.

crypto/x509: add support for SEC1/EC private keys.

Add support for parsing SEC1 EC private keys and PKCS8 encapsulated
EC private key structures.

[jsing, 2012-10-25 13:25:57]

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

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

[agl1, 2012-10-25 14:19:03]

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go
File src/pkg/crypto/x509/sec1.go (right):

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:32: func ParseSEC1PrivateKey(der []byte) (key
*ecdsa.PrivateKey, err error) {
Where is this format from? It's odd to have two ASN.1 values concatenated,
rather than in a SEQUENCE and SEC1 C.2 doesn't seem to specify this format.

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:52: return nil, fmt.Errorf("crypto/x509: failed to
parse EC private key: %v", err)
errors.New("crypto/x509: failed to parse EC private key: " + err.Error())

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:52: return nil, fmt.Errorf("crypto/x509: failed to
parse EC private key: %v", err)
errors.New("crypto/x509: failed to parse EC private key: " + err.Error())

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:55: return nil, fmt.Errorf("crypto/x509: unknown
version %d\n", privKey.Version)
no \n at the end

s/unknown version/unknown EC private key version/

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:58: var namedCurve elliptic.Curve
s/namedCurve/curve/

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:69: priv := new(ecdsa.PrivateKey)
if k.Cmp(curve.Params().N) >= 0 {
  return nil, errors.New("crypto/x509: invalid elliptic curve private key
value")
}

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:72: priv.X, priv.Y =
namedCurve.ScalarBaseMult(k.Bytes())
s/k.Bytes()/privKey/PrivateKey/

[jsing, 2012-10-25 15:20:57]

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go
File src/pkg/crypto/x509/sec1.go (right):

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:32: func ParseSEC1PrivateKey(der []byte) (key
*ecdsa.PrivateKey, err error) {
On 2012/10/25 14:19:03, agl1 wrote:
> Where is this format from? It's odd to have two ASN.1 values concatenated,
> rather than in a SEQUENCE and SEC1 C.2 doesn't seem to specify this format.

Agreed. It appears that 'openssl ecparam' writes out an ECDomainParameters
immediately followed by the ECPrivateKey structure. This is similar to what it
does in the PEM case, where there are two PEM blocks in the same file (the first
an 'EC PARAMS' block, the second an 'EC PRIVATE KEY' block). In the case of a
named curve, the ASN.1 can simply be an OID. I'm not entirely sure how we should
handle this...

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:52: return nil, fmt.Errorf("crypto/x509: failed to
parse EC private key: %v", err)
On 2012/10/25 14:19:03, agl1 wrote:
> errors.New("crypto/x509: failed to parse EC private key: " + err.Error())

Done.

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:52: return nil, fmt.Errorf("crypto/x509: failed to
parse EC private key: %v", err)
On 2012/10/25 14:19:03, agl1 wrote:
> errors.New("crypto/x509: failed to parse EC private key: " + err.Error())

Done.

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:55: return nil, fmt.Errorf("crypto/x509: unknown
version %d\n", privKey.Version)
On 2012/10/25 14:19:03, agl1 wrote:
> no \n at the end
> 
> s/unknown version/unknown EC private key version/
> 

Done.

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:58: var namedCurve elliptic.Curve
On 2012/10/25 14:19:03, agl1 wrote:
> s/namedCurve/curve/

Done.

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:69: priv := new(ecdsa.PrivateKey)
On 2012/10/25 14:19:03, agl1 wrote:
> if k.Cmp(curve.Params().N) >= 0 {
>   return nil, errors.New("crypto/x509: invalid elliptic curve private key
> value")
> }

Done.

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:72: priv.X, priv.Y =
namedCurve.ScalarBaseMult(k.Bytes())
On 2012/10/25 14:19:03, agl1 wrote:
> s/k.Bytes()/privKey/PrivateKey/

Done.

[agl1, 2012-10-29 13:46:37]

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go
File src/pkg/crypto/x509/sec1.go (right):

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
src/pkg/crypto/x509/sec1.go:32: func ParseSEC1PrivateKey(der []byte) (key
*ecdsa.PrivateKey, err error) {
On 2012/10/25 15:20:58, jsing wrote:
> On 2012/10/25 14:19:03, agl1 wrote:
> > Where is this format from? It's odd to have two ASN.1 values concatenated,
> > rather than in a SEQUENCE and SEC1 C.2 doesn't seem to specify this format.
> 
> Agreed. It appears that 'openssl ecparam' writes out an ECDomainParameters
> immediately followed by the ECPrivateKey structure. This is similar to what it
> does in the PEM case, where there are two PEM blocks in the same file (the
first
> an 'EC PARAMS' block, the second an 'EC PRIVATE KEY' block). In the case of a
> named curve, the ASN.1 can simply be an OID. I'm not entirely sure how we
should
> handle this...

I don't think that we should handle it. I think it's basically a problem with
OpenSSL. It makes sense in the PEM case, but it's nonsense for DER.

If we eliminate this method, we can remove the current ParseECPrivateKey wrapper
too, right?

[jsing, 2012-11-13 14:03:13]

On 2012/10/29 13:46:37, agl1 wrote:
> https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go
> File src/pkg/crypto/x509/sec1.go (right):
> 
>
https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#...
> src/pkg/crypto/x509/sec1.go:32: func ParseSEC1PrivateKey(der []byte) (key
> *ecdsa.PrivateKey, err error) {
> On 2012/10/25 15:20:58, jsing wrote:
> > On 2012/10/25 14:19:03, agl1 wrote:
> > > Where is this format from? It's odd to have two ASN.1 values concatenated,
> > > rather than in a SEQUENCE and SEC1 C.2 doesn't seem to specify this
format.
> > 
> > Agreed. It appears that 'openssl ecparam' writes out an ECDomainParameters
> > immediately followed by the ECPrivateKey structure. This is similar to what
it
> > does in the PEM case, where there are two PEM blocks in the same file (the
> first
> > an 'EC PARAMS' block, the second an 'EC PRIVATE KEY' block). In the case of
a
> > named curve, the ASN.1 can simply be an OID. I'm not entirely sure how we
> should
> > handle this...
> 
> I don't think that we should handle it. I think it's basically a problem with
> OpenSSL. It makes sense in the PEM case, but it's nonsense for DER.

Apologies for the delay in getting back to this.

Generally speaking, I agree with your sentiment. OpenSSL seems to take the
approach that EC keys can be generated with non-named parameters, which means
that the curve parameters need to be included separately. This is obvious in the
PEM case where there are two PEM blocks, however their DER output also includes
two ASN.1 values (this occurs even in the named curve case). If we choose not
support this then we will not be able to load DER EC keys as currently generated
by OpenSSL.

> If we eliminate this method, we can remove the current ParseECPrivateKey
wrapper
> too, right?

Yes.

[agl1, 2012-11-13 15:43:12]

https://codereview.appspot.com/6767045/diff/11001/src/pkg/crypto/x509/sec1.go
File src/pkg/crypto/x509/sec1.go (right):

https://codereview.appspot.com/6767045/diff/11001/src/pkg/crypto/x509/sec1.go...
src/pkg/crypto/x509/sec1.go:41: func ParseECPrivateKey(der []byte) (key
*ecdsa.PrivateKey, err error) {
I thought this was going away?

[jsing, 2012-11-13 15:57:02]

On 14 November 2012 02:43,  <agl@golang.org> wrote:
>
> https://codereview.appspot.com/6767045/diff/11001/src/pkg/crypto/x509/sec1.go
> File src/pkg/crypto/x509/sec1.go (right):
>
>
https://codereview.appspot.com/6767045/diff/11001/src/pkg/crypto/x509/sec1.go...
> src/pkg/crypto/x509/sec1.go:41: func ParseECPrivateKey(der []byte) (key
> *ecdsa.PrivateKey, err error) {
> I thought this was going away?

If the decision is to not to support DER generated via OpenSSL ecparam
then we can drop ParseSEC1PrivateKey. The ParseECPrivateKey() function
is still needed for PEM EC PRIVATE KEY blocks, with parseECPrivateKey
being necessary to handle PKCS8 encapsulated EC keys.

[agl1, 2012-11-13 16:35:12]

On Tue, Nov 13, 2012 at 10:57 AM, Joel Sing <jsing@google.com> wrote:
> If the decision is to not to support DER generated via OpenSSL ecparam
> then we can drop ParseSEC1PrivateKey. The ParseECPrivateKey() function
> is still needed for PEM EC PRIVATE KEY blocks, with parseECPrivateKey
> being necessary to handle PKCS8 encapsulated EC keys.

I'm pretty certain that the DER output of ecparam is a bug. It makes
some sense in PEM format because they are two separate PEM blocks, but
ramming two DER streams together is completely non-standard.


Cheers

AGL

[jsing, 2012-11-14 10:39:56]

On 2012/11/13 16:35:12, agl1 wrote:
> On Tue, Nov 13, 2012 at 10:57 AM, Joel Sing <mailto:jsing@google.com> wrote:
> > If the decision is to not to support DER generated via OpenSSL ecparam
> > then we can drop ParseSEC1PrivateKey. The ParseECPrivateKey() function
> > is still needed for PEM EC PRIVATE KEY blocks, with parseECPrivateKey
> > being necessary to handle PKCS8 encapsulated EC keys.
> 
> I'm pretty certain that the DER output of ecparam is a bug. It makes
> some sense in PEM format because they are two separate PEM blocks, but
> ramming two DER streams together is completely non-standard.

Agreed. I've removed ParseSEC1PrivateKey() and the associated test.

PTAL.

[agl1, 2012-11-14 16:27:48]

LGTM.

(Let me know if you need me to land it in the tree.)

[jsing, 2012-11-14 16:41:00]

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

crypto/x509: add support for SEC1/EC private keys.

Add support for parsing SEC1 EC private keys and PKCS8 encapsulated
EC private key structures.

R=agl
CC=golang-dev
http://codereview.appspot.com/6767045