Issue 145910043: code review 145910043: x509: implement crypto.Signer (Closed)
|
Created:
9 years, 7 months ago by Paul van Brouwershaven Modified:
9 years, 3 months ago Reviewers:
CC:
agl, agl1, bradfitz, golang-codereviews Visibility:
Public. |
Descriptionx509: implement crypto.Signer
Signer is an interface to support opaque private keys.
These keys typically result from being kept in special hardware
(i.e. a TPM) although sometimes operating systems provide a
similar interface using process isolation for security rather
than hardware boundaries.
This changes provides updates implements crypto.Signer in
CreateCRL and CreateCertificate so that they can be used with
opaque keys.
Patch Set 1 #Patch Set 2 : diff -r 6b163ec2122a172030284060788f535ab3b9d0e3 https://code.google.com/p/go #Patch Set 3 : diff -r 6b163ec2122a172030284060788f535ab3b9d0e3 https://code.google.com/p/go #
Total comments: 14
Patch Set 4 : diff -r 6b163ec2122a172030284060788f535ab3b9d0e3 https://code.google.com/p/go #
Total comments: 12
Patch Set 5 : diff -r f60b128afd41217fa34662a5cbe1eb0b8c546e71 https://code.google.com/p/go #MessagesTotal messages: 9
https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go File src/crypto/x509/x509.go (right): https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1388: err = errors.New("x509: only RSA and ECDSA public keys supported") I'd just drop "public" https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1435: // The only supported key types are RSA and ECDSA (*rsa.PublicKey or This part of the comment probably needs to be updated. https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1549: func (c *Certificate) CreateCRL(rand io.Reader, priv interface{}, revokedCerts []pkix.RevokedCertificate, now, expiry time.Time) (crlBytes []byte, err error) { needs updating. https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1664: func CreateCertificateRequest(rand io.Reader, template *CertificateRequest, priv interface{}) (csr []byte, err error) { Why not a Signer here? https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509_test.go File src/crypto/x509/x509_test.go (left): https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509_test... src/crypto/x509/x509_test.go:313: {"RSA/ECDSA", &rsaPriv.PublicKey, ecdsaPriv, false, ECDSAWithSHA384}, These lines can't be removed and there shouldn't be any need to.
Sign in to reply to this message.
https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go File src/crypto/x509/x509.go (right): https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1388: err = errors.New("x509: only RSA and ECDSA public keys supported") On 2014/09/18 17:33:23, agl wrote: > I'd just drop "public" Acknowledged. https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1435: // The only supported key types are RSA and ECDSA (*rsa.PublicKey or On 2014/09/18 17:33:23, agl wrote: > This part of the comment probably needs to be updated. Acknowledged. https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1549: func (c *Certificate) CreateCRL(rand io.Reader, priv interface{}, revokedCerts []pkix.RevokedCertificate, now, expiry time.Time) (crlBytes []byte, err error) { On 2014/09/18 17:33:23, agl wrote: > needs updating. Any idea how we could add a requested signature algorithm without breaking backwards compatibility? We also need to add support for extensions, but that would be a separate change as it has nothing to do with crypto.Signer. https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1664: func CreateCertificateRequest(rand io.Reader, template *CertificateRequest, priv interface{}) (csr []byte, err error) { On 2014/09/18 17:33:23, agl wrote: > Why not a Signer here? In the other functions I'm using the public key to replace the type (rsa, ecdsa) so it can be used for signingParamsForPublicKey (was signingParamsForPrivateKey). This also causes the issue with the mix keys in the test package as the reflection is done on the public key from the pair instead of the private key.
Sign in to reply to this message.
https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go File src/crypto/x509/x509.go (right): https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1664: func CreateCertificateRequest(rand io.Reader, template *CertificateRequest, priv interface{}) (csr []byte, err error) { On 2014/09/18 18:11:59, Paul van Brouwershaven wrote: > In the other functions I'm using the public key to replace the type (rsa, ecdsa) > so it can be used for signingParamsForPublicKey (was > signingParamsForPrivateKey). I can use the Public() from the crypto.Signer interface of course. https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509_test.go File src/crypto/x509/x509_test.go (left): https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509_test... src/crypto/x509/x509_test.go:313: {"RSA/ECDSA", &rsaPriv.PublicKey, ecdsaPriv, false, ECDSAWithSHA384}, On 2014/09/18 17:33:23, agl wrote: > These lines can't be removed and there shouldn't be any need to. I'm using the public key for signingParamsForPublicKey (was signingParamsForPrivateKey). Because I have no access to the details of the private key.
Sign in to reply to this message.
https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go File src/crypto/x509/x509.go (right): https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1388: err = errors.New("x509: only RSA and ECDSA public keys supported") On 2014/09/18 18:11:59, Paul van Brouwershaven wrote: > On 2014/09/18 17:33:23, agl wrote: > > I'd just drop "public" > > Acknowledged. Done. https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1435: // The only supported key types are RSA and ECDSA (*rsa.PublicKey or On 2014/09/18 18:11:59, Paul van Brouwershaven wrote: > On 2014/09/18 17:33:23, agl wrote: > > This part of the comment probably needs to be updated. > > Acknowledged. Done. https://codereview.appspot.com/145910043/diff/40001/src/crypto/x509/x509.go#n... src/crypto/x509/x509.go:1664: func CreateCertificateRequest(rand io.Reader, template *CertificateRequest, priv interface{}) (csr []byte, err error) { On 2014/09/18 17:33:23, agl wrote: > Why not a Signer here? Done.
Sign in to reply to this message.
(Note: this might have to wait until after 1.4 to land.) https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go File src/crypto/x509/x509.go (right): https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go#newco... src/crypto/x509/x509.go:1435: // All keys types that are implemented via crypto.Signer are supported. Append: "(This includes *rsa.PublicKey and *ecdsa.PublicKey.)" https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go#newco... src/crypto/x509/x509.go:1547: // contains the given list of revoked certificates. Copy the same comment about supported key types from above. https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go#newco... src/crypto/x509/x509.go:1661: // All keys types that are implemented via crypto.Signer are supported. append same comment specifically mentioning *rsa/ecdsa.PrivateKey. https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go#newco... src/crypto/x509/x509.go:1672: switch pub := key.Public().(type) { do you need this switch? https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go#newco... src/crypto/x509/x509.go:1688: switch pub := key.Public().(type) { ditto. https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509_test.go File src/crypto/x509/x509_test.go (left): https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509_test.go#... src/crypto/x509/x509_test.go:313: {"RSA/ECDSA", &rsaPriv.PublicKey, ecdsaPriv, false, ECDSAWithSHA384}, this can't be deleted.
Sign in to reply to this message.
https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go File src/crypto/x509/x509.go (right): https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go#newco... src/crypto/x509/x509.go:1435: // All keys types that are implemented via crypto.Signer are supported. On 2014/09/22 22:10:09, agl1 wrote: > Append: > > "(This includes *rsa.PublicKey and *ecdsa.PublicKey.)" Acknowledged. https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go#newco... src/crypto/x509/x509.go:1547: // contains the given list of revoked certificates. On 2014/09/22 22:10:09, agl1 wrote: > Copy the same comment about supported key types from above. Acknowledged. https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go#newco... src/crypto/x509/x509.go:1661: // All keys types that are implemented via crypto.Signer are supported. On 2014/09/22 22:10:09, agl1 wrote: > append same comment specifically mentioning *rsa/ecdsa.PrivateKey. Acknowledged. https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go#newco... src/crypto/x509/x509.go:1672: switch pub := key.Public().(type) { On 2014/09/22 22:10:08, agl1 wrote: > do you need this switch? no we don't, I will remove this https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509.go#newco... src/crypto/x509/x509.go:1688: switch pub := key.Public().(type) { On 2014/09/22 22:10:08, agl1 wrote: > ditto. Acknowledged. https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509_test.go File src/crypto/x509/x509_test.go (left): https://codereview.appspot.com/145910043/diff/4/src/crypto/x509/x509_test.go#... src/crypto/x509/x509_test.go:313: {"RSA/ECDSA", &rsaPriv.PublicKey, ecdsaPriv, false, ECDSAWithSHA384}, On 2014/09/22 22:10:09, agl1 wrote: > this can't be deleted. I renamed signingParamsForPrivateKey to signingParamsForPublicKey as we don't have access to the private key when working with a signer. The reason that I deleted these tests is that while I can obtain the required information from the public key if we have an key pair, I can't when using a different public and private key type like here. In this case I can only obtain that information from the private key (which we don't have). An alternative option could be to rename signingParamsForPublicKey to signingParamsForKey and to keep using the private key when available and fall-back to the public key when they private key is stored in hardware. Then we could keep this test but I would be masking the actual problem and create a different (unexpected) behaviour for hardware keys. Another option could be to remove the auto selection and rely and trust on the requested algorithm. While this would create a problem for x509.CreateCRL which has no option to request an algorithm. Suggestions and alternative ideas are welcome.
Sign in to reply to this message.
Hello agl@chromium.org, agl@golang.org (cc: bradfitz@golang.org, golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
I think that this is probably good, but would still need to wait until after 1.4 is out.
Sign in to reply to this message.
R=close To the author of this CL: The Go project has moved to Gerrit Code Review. If this CL should be continued, please see the latest version of https://golang.org/doc/contribute.html for instructions on how to set up Git and the Go project's Gerrit codereview plugin, and then create a new change with your current code. If there has been discussion on this CL, please give a link to it (golang.org/cl/145910043 is best) in the description in your new CL. Thanks very much.
Sign in to reply to this message.
|
