code review 6776043: crypto/tls: add support for loading EC X.509 key pairs

crypto/tls: add support for loading EC X.509 key pairs

Add support for loading X.509 key pairs that consist of a certificate
with an EC public key and its corresponding EC private key.

[jsing, 2012-11-14 14:18:52]

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-11-14 16:32:59]

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go
File src/pkg/crypto/tls/tls.go (right):

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:158: if strings.HasSuffix(keyDERBlock.Type, "PRIVATE
KEY") {
" PRIVATE KEY"

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:175: switch x509Cert.PublicKeyAlgorithm {
It would seem cleaner to do a type switch on x509Cert.PublicKey. It would save
having to cast it too.

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:178: priv := cert.PrivateKey.(*rsa.PrivateKey)
This will crash if a non-RSA key is provided with an RSA cert.

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:185: priv := cert.PrivateKey.(*ecdsa.PrivateKey)
Ditto, but vice-versa.

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:208: return key.(*rsa.PrivateKey), nil
Can just do switch key := key.(type) and avoid the casts here.

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:212: return nil, errors.New("crypto/tls: found unknown
private key in PKCS#8 wrapping")
unknown private key /type/

[jsing, 2012-11-15 14:03:37]

PTAL

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go
File src/pkg/crypto/tls/tls.go (right):

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:158: if strings.HasSuffix(keyDERBlock.Type, "PRIVATE
KEY") {
On 2012/11/14 16:32:59, agl1 wrote:
> " PRIVATE KEY"

Done.

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:175: switch x509Cert.PublicKeyAlgorithm {
On 2012/11/14 16:32:59, agl1 wrote:
> It would seem cleaner to do a type switch on x509Cert.PublicKey. It would save
> having to cast it too.

Done.

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:178: priv := cert.PrivateKey.(*rsa.PrivateKey)
On 2012/11/14 16:32:59, agl1 wrote:
> This will crash if a non-RSA key is provided with an RSA cert.

Indeed. I've fixed this and have added a test case for it.

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:185: priv := cert.PrivateKey.(*ecdsa.PrivateKey)
On 2012/11/14 16:32:59, agl1 wrote:
> Ditto, but vice-versa.

Fixed.

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:208: return key.(*rsa.PrivateKey), nil
On 2012/11/14 16:32:59, agl1 wrote:
> Can just do switch key := key.(type) and avoid the casts here.

Yes, much nicer.

https://codereview.appspot.com/6776043/diff/8001/src/pkg/crypto/tls/tls.go#ne...
src/pkg/crypto/tls/tls.go:212: return nil, errors.New("crypto/tls: found unknown
private key in PKCS#8 wrapping")
On 2012/11/14 16:32:59, agl1 wrote:
> unknown private key /type/

Done.

[agl1, 2012-11-15 17:57:29]

LGTM

[jsing, 2012-11-16 08:40:46]

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

crypto/tls: add support for loading EC X.509 key pairs

Add support for loading X.509 key pairs that consist of a certificate
with an EC public key and its corresponding EC private key.

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