Code review - Issue 6776043: code review 6776043: crypto/tls: add support for loading EC X.509 key pairshttps://codereview.appspot.com/2012-11-16T08:40:46+00:00rietveld
Message from unknown
2012-11-14T14:15:28+00:00jsingurn:md5:fc407dd70384ff049edcafa135413a52
Message from unknown
2012-11-14T14:18:44+00:00jsingurn:md5:1dc66faf9ff26df4873a6da8c30ff978
Message from jsing@google.com
2012-11-14T14:18:52+00:00jsingurn:md5:35e57542fbf3215230ee92b206b9ffc7
Hello agl@golang.org (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://go.googlecode.com/hg/
Message from agl@golang.org
2012-11-14T16:32:59+00:00agl1urn:md5:674431474e774de9b857c5865b3958ab
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#newcode158
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#newcode175
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#newcode178
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#newcode185
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#newcode208
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#newcode212
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/
Message from unknown
2012-11-14T16:57:13+00:00jsingurn:md5:8fec978c5f6fc0afc1f09ec56deb1999
Message from jsing@google.com
2012-11-15T14:03:37+00:00jsingurn:md5:1235e1ec1cc7bfe6a1f684347ea6dc4c
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#newcode158
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#newcode175
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#newcode178
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#newcode185
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#newcode208
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#newcode212
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.
Message from agl@golang.org
2012-11-15T17:57:29+00:00agl1urn:md5:10e3223270081683d38d0fb4754cbb53
LGTM
Message from unknown
2012-11-16T08:35:44+00:00jsingurn:md5:617ea0c426346a2fcec3b32296b2db87
Message from jsing@google.com
2012-11-16T08:40:46+00:00jsingurn:md5:0a69552753c306694739ce0376f1c897
*** 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