crypto: add ContextSigner and use in crypto/tls

[rittneje]

This was previously proposed in #28427 but rejected. However, I don't think it was given proper consideration.

As mentioned in #28427, it is possible for a crypto.Signer to require remote calls to some external service such as AWS KMS. Currently the Sign method does not take a context.Context, meaning that the timeout/cancellation and any logging/monitoring metadata cannot make it across the method call. @bcmills mentioned using currying to simulate this but that does not work well with the crypto/tls API (and possibly others). Instead the desire there would be to forward the context from HandshakeContext across.

I propose the following changes. First add a new interface to crypto.

type ContextSigner interface {
	Signer
	SignContext(ctx context.Context, rand io.Reader, digest []byte, opts SignerOpts) (signature []byte, err error)
}

Then add a helper function to crypto. (Not strictly required but will make people's lives easier.)

func SignContext(ctx context.Context, s Signer, rand io.Reader, digest []byte, opts SignerOpts) (signature []byte, err error) {
    cs, ok := s.(ContextSigner)
    if !ok {
        return s.Sign(rand, digest, opts)
    }
    return cs.SignContext(ctx, rand, digest, opts)
}

Then the internals of net/tls can be amended to call the new crypto.SignContext function from HandshakeContext.

[seankhliao]

cc @golang/security

[ianlancetaylor]

Can you explain the problem with currying in more detail? Thanks.

[rittneje]

@ianlancetaylor In crypto/tls, the crypto.Signer is inside the tls.Certificate, which in turn is inside the tls.Config. So only way for the currying approach to work is to make a separate tls.Config for each connection. In addition, without this change tls.Conn.HandshakeContext cannot uphold its contract properly, since it has no way of canceling the call to Sign, meaning the client has to be sure to pass the same context to both, which is pretty awkward.

[apparentlymart]

I see that ContextSigner embeds Signer and so any implementation of that type would need to offer both methods. That seems reasonable and necessary for backward-compatibility.

Would you anticipate that all ContextSigner implementations would have an essentially-boilerplate Sign that would just forward over to SignContext with a placeholder context?

type exampleSigner struct {}

func (s *exampleSigner) SignContext(ctx context.Context, rand io.Reader, digest []byte, opts SignerOpts) (signature []byte, err error) {
    // ...
}

func (s *exampleSigner) Sign(rand io.Reader, digest []byte, opts SignerOpts) (signature []byte, err error) {
    return s.SignContext(context.TODO(), rand, digest, opts)
}

This is just a question, not an objection. I'm curious as to whether you or someone else has an idea about how to avoid this boilerplate, but if not then at a least doesn't seem too arduous. Perhaps the boilerplate example could be included in the godoc for the new interface.

[rittneje]

Would you anticipate that all ContextSigner implementations would have an essentially-boilerplate Sign that would just forward over to SignContext with a placeholder context?

Yes, although it is ultimately up to each implementation what exactly that context is. In most cases I would expect context.Background() but that is not strictly required.

I'm curious as to whether you or someone else has an idea about how to avoid this boilerplate

You kind of can, but I don't think it is worth it in this particular case, as you would be trading one line of trivial boilerplate for another. For example:

type ContextSigner interface {
	Public() PublicKey
	SignContext(ctx context.Context, rand io.Reader, digest []byte, opts SignerOpts) (signature []byte, err error)
}

type ContextSignerAdapter struct {
    ContextSigner
}

func (s ContextSignerAdapter) Sign(rand io.Reader, digest []byte, opts SignerOpts) (signature []byte, err error) {
    return s.SignContext(context.Background(), rand, digest, opts)
}

Also, if you did this then other methods implemented by your ContextSigner would not be "visible" via type assertion, which could be a problem for some use cases.

[bcmills]

The obvious place to add a Context parameter in the existing Sign method is in the SignerOpts argument. Unfortunately (and perplexingly!), SignerOpts is an interface type, so it cannot be extended without breaking compatibility. 😩

[bcmills]

@apparentlymart, in theory that boilerplate could be implemented with a generic wrapper. However, a wrapper that preserves additional extension methods is not currently possible due to #43621. (If it were allowed, it might look like this: https://go.dev/play/p/Hp-kP_UM9c5)

However, if we are ok with dropping extension methods, it can be implemented as a generic wrapper today: https://go.dev/play/p/IJpyNKgbgE2

(Note that the wrapper that drops extension methods doesn't even need to be generic — but making it generic allows for a future change to preserve extension methods.)

[apparentlymart]

It does seem unfortunate that the SignerOpts is an interface, but that does seem to suggest that a similar technique to this proposal could be applied to SignerOpts but in a way that perhaps encapsulates the new behavior better.

package crypto

func SignerOptsContext(opts SignerOpts) context.Context {
    if withCtx, ok := opts.(SignerOptsContext); ok {
        return withCtx.SignerOptsContext()
    }
    return context.Background()
}

A signer that would benefit from a context can pass the given opts to this function and hopefully get a useful context, but if not then get a harmless useless one.

My assumption in proposing this is that there would be relatively few SignerOpts implementations to update for this to be useful, and the extra special case is just an extra call inside the existing Signer method rather than a new method and a fowarder from the old one, so less boilerplate for Signer implementers.

[rittneje]

@apparentlymart Unfortunately, the situation with SignerOpts is complicated because it fundamentally has always required signers to type assert. Generally it will either be an instance of crypto.Hash or *rsa.PSSOptions, and implementations must check for these to function.

Since crypto.Hash is just an int there is nowhere to put a context, so that would require a wrapper. That alone is a breaking change for anyone that was type asserting to crypto.Hash. But even if we ignore that, we cannot use the same wrapper for *rsa.PSSOptions or it will definitely break signers as there is no other way to check for PSS. So *rsa.PSSOptions could not be wrapped but rather that struct would need a context.Context field. Then for backwards compatibility all signers would have to check for a nil context and treat it (presumably) as context.Background.

Had SignerOpts been a struct instead of an interface it would have worked much better.

[bcmills]

So, another option might be to define the Context support as an extension interface on SignerOpts. Perhaps something like:

// ContextSignerOpts extends SignerOpts with a Context method.
//
// Signer implementations are encouraged to check whether the SignerOpts
// implement ContextSignerOpts and make use of its Context method.
type ContextSignerOpts {
	Context() context.Context
	SignerOpts
}

// SignerContext returns the Context from opts, if present, or else context.TODO().
func SignerContext(opts SignerOpts) context.Context {
	ctxOpts, ok := opts.(ContextSignerOpts)
	if !ok {
		return context.TODO()
	}
	return ctxOpts.Context()
}

...and then also update *rsa.PSSOptions and *ed25519.Options to implement ContextSignerOpts, and perhaps add a struct type to the crypto package to allow one to augment a crypto.Hash with a Context (this time in a struct type, in case further extensions are needed in the future 😅).