code review 114680043: crypto: add Signer

crypto: add 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 interfaces for representing them and
alters crypto/tls so that client certificates can use
opaque keys.

[agl1, 2014-08-13 19:10:11]

NSS and OpenSSL did a terrible job of supporting opaque keys (PKCS#11 and
ENGINE) which is why I've shied away from them in the past. But there are quite
a lot of uses for them piling up now.

I don't know whether opaque keys inherently cause an API disaster or whether
it's possible to avoid that by being careful. So please give much thought to the
API in this case.

(Note: there are probably some places where I need to update the comments in
crypto/tls and haven't yet. However, this change is on the wrong computer and
it'll be a while until I'll be at it again so I'm hoping to start the review
now.)

[jdeprez, 2014-08-14 02:23:39]

https://codereview.appspot.com/114680043/diff/20001/src/pkg/crypto/tls/handsh...
File src/pkg/crypto/tls/handshake_client.go (right):

https://codereview.appspot.com/114680043/diff/20001/src/pkg/crypto/tls/handsh...
src/pkg/crypto/tls/handshake_client.go:418: key, err :=
opaquekey.WrapSigner(c.config.Certificates[0].PrivateKey)
Something that's bugging me about both the existing code and this CL, and I'm
not sure if I've missed something: why c.config.Certificates[0] is used here
when the findCert loop above carefully determines chainToSend as the matching
certificate chain. You could pass two certs in c.config, match the second one,
and end up signing the digest with the key for the first.

[bradfitz, 2014-08-14 20:30:33]

https://codereview.appspot.com/114680043/diff/20001/src/pkg/crypto/opaquekey/...
File src/pkg/crypto/opaquekey/opaquekey.go (right):

https://codereview.appspot.com/114680043/diff/20001/src/pkg/crypto/opaquekey/...
src/pkg/crypto/opaquekey/opaquekey.go:21: type Signer interface {
what if this and SignerOpts were just in package crypto and the WrapSigner were
split into two functions in each of crypto/ecdsa and crypto/rsa?

This package seems too small and yet also too large in that just mostly depends
on other things (and presumably more in the future?)

Alternatively, WrapSigner could also be in package crypto, but ecdsa and rsa
could register wrapSigner funcs  with the package in their inits.

[agl1, 2014-08-29 19:09:12]

https://codereview.appspot.com/114680043/diff/20001/src/pkg/crypto/opaquekey/...
File src/pkg/crypto/opaquekey/opaquekey.go (right):

https://codereview.appspot.com/114680043/diff/20001/src/pkg/crypto/opaquekey/...
src/pkg/crypto/opaquekey/opaquekey.go:21: type Signer interface {
On 2014/08/14 20:30:33, bradfitz wrote:
> what if this and SignerOpts were just in package crypto and the WrapSigner
were
> split into two functions in each of crypto/ecdsa and crypto/rsa?

I've tried to do something like this. One of the nasty problems is that the
SignerOpts struct can need to contain algorithm specific things (like
rsa.PSSOptions). If it's in crypto/ then it can't. So SignerOpts has turned into
an interface and PSSOptions implements that interface.

I also made ecdsa and rsa PrivateKeys implement Signer directly rather than
having them register functions for WrapSigner. (So WrapSigner is gone.)

https://codereview.appspot.com/114680043/diff/20001/src/pkg/crypto/tls/handsh...
File src/pkg/crypto/tls/handshake_client.go (right):

https://codereview.appspot.com/114680043/diff/20001/src/pkg/crypto/tls/handsh...
src/pkg/crypto/tls/handshake_client.go:418: key, err :=
opaquekey.WrapSigner(c.config.Certificates[0].PrivateKey)
On 2014/08/14 02:23:39, jdeprez wrote:
> Something that's bugging me about both the existing code and this CL, and I'm
> not sure if I've missed something: why c.config.Certificates[0] is used here

I think that's a very good point. I guess nobody uses multiple client
certificates! Fixed.

[bradfitz, 2014-08-29 19:22:02]

CL description needs updating. No more opaquekey.

https://codereview.appspot.com/114680043/diff/60001/src/pkg/crypto/crypto.go
File src/pkg/crypto/crypto.go (right):

https://codereview.appspot.com/114680043/diff/60001/src/pkg/crypto/crypto.go#...
src/pkg/crypto/crypto.go:18: // HashFunc simply returns the value of h in order
that a Hash implement
s/in order that/so/ ?
s/implement/implements/ ?

[bradfitz, 2014-08-29 19:22:27]

LGTM

[agl1, 2014-08-29 19:31:52]

https://codereview.appspot.com/114680043/diff/60001/src/pkg/crypto/crypto.go
File src/pkg/crypto/crypto.go (right):

https://codereview.appspot.com/114680043/diff/60001/src/pkg/crypto/crypto.go#...
src/pkg/crypto/crypto.go:18: // HashFunc simply returns the value of h in order
that a Hash implement
On 2014/08/29 19:22:01, bradfitz wrote:
> s/in order that/so/ ?
> s/implement/implements/ ?

Done.

[agl1, 2014-08-29 19:36:27]

Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com,
jdeprez@google.com),

I'd like you to review this change to
https://code.google.com/p/go/

[agl1, 2014-08-29 19:36:43]

*** Submitted as https://code.google.com/p/go/source/detail?r=40906c63a04e ***

crypto: add 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 interfaces for representing them and
alters crypto/tls so that client certificates can use
opaque keys.

LGTM=bradfitz
R=bradfitz
CC=golang-codereviews, jdeprez
https://codereview.appspot.com/114680043

[gobot, 2014-08-29 19:53:09]

This CL appears to have broken the windows-386 builder.
See http://build.golang.org/log/4c8f3d532877b6b9413cd9708bb1974bc9d562ec

[agl1, 2014-08-29 20:39:18]

On Fri, Aug 29, 2014 at 12:53 PM,  <gobot@golang.org> wrote:
> This CL appears to have broken the windows-386 builder.

Not real, just lag:

sleep_test.go:230: attempt 0 failed: After(40ms) arrived at 243ms,
expected [30ms,240ms]


Cheers

AGL

[minux, 2014-08-30 03:32:08]

Please update doc/go1.4.txt.