cc @golang/security

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

@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.

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.

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.

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. 😩

@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.)

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.

@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.

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 😅).

As I mentioned, adding a wrapper for crypto.Hash will break any existing signer that type asserts to crypto.Hash. I don't know how common that is, but there is no way for the caller to tell in advance without adding another method to crypto.Signer, and at that point we are back to my original proposal.

By the way, if we were designing the crypto package from scratch, I think double dispatch would have made much more sense.

package crypto

type Signer interface {
    Public() PublicKey
    Sign(ctx context.Context, rand io.Reader, digest []byte, hash crypto.Hash) (signature []byte, err error)
}

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

type Hash int

func (h Hash) Sign(ctx context.Context, rand io.Reader, digest []byte, s Signer) (signature []byte, err error) {
    return s.Sign(ctx, rand, digest, h)
}

package rsa

type PSSSigner interface {
    crypto.Signer
    SignPSS(ctx context.Context, rand io.Reader, digest []byte, opts PSSOptions) (signature []byte, err error)
}

type PSSOptions struct {
    SaltLength int
    Hash crypto.Hash
}

func (opts PSSOptions) Sign(ctx context.Context, rand io.Reader, digest []byte, s Signer) (signature []byte, err error) {
    ps, ok := s.(PSSSigner)
    if !ok {
        return nil, errors.New(...)
    }
    return ps.SignPSS(ctx, rand, digest, opts)
}

Maybe it would be worth combing both fixes into one?

package crypto

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

// I don't have a better name for this right now.
type SignerOptsV2 interface {
    Sign(ctx context.Context, s Signer, rand io.Reader, digest []byte) (signature []byte, err error)
}

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

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

package rsa

type PSSSigner interface {
    crypto.ContextSigner
    SignPSS(ctx context.Context, rand io.Reader, digest []byte, opts *PSSOptions) (signature []byte, err error)
}

func (opts *PSSOptions) Sign(ctx context.Context, s crypto.Signer, rand io.Reader, digest []byte) (signature []byte, err error) {
    ps, ok := s.(PSSSigner)
    if !ok {
        return s.Sign(rand, digest, opts)
    }
    return ps.SignPSS(ctx, rand, digest, opts)
}

func (priv *PrivateKey) SignContext(ctx context.Context, rand io.Reader, digest []byte, hash crypto.Hash) (signature []byte, err error) {
    ...
}

func (priv *PrivateKey) SignPSS(ctx context.Context, rand io.Reader, digest []byte, opts *PSSOptions) (signature []byte, err error) {
    ...
}

crypto.Signer.Sign and crypto.SignerOpts are then deprecated. For people implementing crypto.Signer, they can just hard-code some error return if they know if won't be called via a legacy path.

Another option would be this:

package crypto

type SignerOptsV2 interface {
    Sign(ctx context.Context, s Signer, rand io.Reader, digest []byte) (signature []byte, err error)
}

type HashSigner interface {
    crypto.Signer
    SignHash(ctx context.Context, rand io.Reader, digest []byte, h Hash) (signature []byte, err error)
}

type SignerWrapper interface {
    SignWithOpts(ctx context.Context, rand io.Reader, digest []byte, opts SignerOptsV2) (signature []byte, err error)
}

func (h Hash) Sign(ctx context.Context, s Signer, rand io.Reader, digest []byte) (signature []byte, err error) {
    hs, ok := s.(HashSigner)
    if !ok {
        return s.Sign(rand, digest, h)
    }
    return hs.SignHash(ctx, rand, digest, h)
}

func Sign(ctx context.Context, s Signer, rand io.Reader, digest []byte, opts SignerOpts) (signature []byte, err error) {
    ov2, ok := opts.(SignerOptsV2)
    if !ok {
        return s.Sign(rand, digest, opts)
    }
    w, ok := s.(SignerWrapper)
    if !ok {
        return ov2.Sign(ctx, s, rand, digest)
    }
    return w.SignWithOpts(ctx, rand, digest, ov2)
}

package rsa

type PSSSigner interface {
    crypto.Signer
    SignPSS(ctx context.Context, rand io.Reader, digest []byte, opts *PSSOptions) (signature []byte, err error)
}

func (opts *PSSOptions) Sign(ctx context.Context, s crypto.Signer, rand io.Reader, digest []byte) (signature []byte, err error) {
    ps, ok := s.(PSSSigner)
    if !ok {
        return s.Sign(rand, digest, opts)
    }
    return ps.SignPSS(ctx, rand, digest, opts)
}

func (priv *PrivateKey) SignHash(ctx context.Context, rand io.Reader, digest []byte, h crypto.Hash) (signature []byte, err error) {
    ...
}

func (priv *PrivateKey) SignPSS(ctx context.Context, rand io.Reader, digest []byte, opts *PSSOptions) (signature []byte, err error) {
    ...
}

In this case, SignWithOpts will allow wrapper signer implementations to function properly since they can just pass the options down to the underlying signer without having to implement SignHash, SignPSS, etc. themselves.

adding a wrapper for crypto.Hash will break any existing signer that type asserts to crypto.Hash

Merely defining a new wrapper type (without changing Hash itself) cannot break any existing program, because actually using that wrapper type would require a separate code change. (Programs will not implicitly change to use the wrapper where today they use a crypto.Hash.)

(Moreover, since the SignerOpts interface includes an accessor method that returns the Hash directly, I would expect essentially every implementation to call that method instead of using a type assertion. 😅)

Merely defining a new wrapper type (without changing Hash itself) cannot break any existing program

Sure but any attempt by crypto/tls (or others) to use to new type to support context.Context would be a breaking change, which defeats the whole point.

Moreover, since the SignerOpts interface includes an accessor method that returns the Hash directly, I would expect essentially every implementation to call that method instead of using a type assertion.

Because of the way that crypto.SignerOpts works today, the only way for a signer to know they are properly understanding the opts is to type assert. For example, an RSA signer that only only does PKCS might type assert because it does not support PSS or any other type of signing that might be added in the future. (This is the reason I feel that crypto.SignerOpts was a mistake and that double dispatch is a better approach.)

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

It sounds like there are really two parts to this proposal:

1. Add SignContext
2. Use it in crypto/tls if the Signer implements SignContext, to pass a context known only to TLS into the signer. That's not possible with currying because the context is not available to be curried.

Are there other pieces of code that would also change, besides crypto/tls? We should enumerate the full set to understand the concrete details of the proposal. Thanks.

The only places that we use crypto.Signer are:

1. in crypto/tls for the tls.Certificate
2. in crypto/x509 for signing new certificates

As crypto/x509 is not context-aware it is not affected by this proposal. (Probably using the currying approach here is still good enough.) I see x/crypto/ssh also appears not to be context-aware. I am not familiar with any other APIs using crypto.Signer.

OK, so it sounds like we understand the proposal. Does anyone object to this proposal?
Thoughts from @rolandshoemaker or @FiloSottile or @golang/security?

crypto/tls is the only place I can think of off in the standard library where we'd want to upgrade I think.

I've seen the currying pattern a handful of times outside of the standard library (in particular when using HSMs, and other cloud key management services, for signing), so there is clearly a desire for something like this, although it is clearly doable when you're using an API you can route the context/signer through cleanly yourself.

I have no real objection, seems like it'd allow some cleaner API usage, and has relatively low complexity/risk.

Currying is technically possible, and creating a new Config for each connection is a pretty established pattern through GetConfigForClient, but I can see that being cumbersome when operating through net/http, which is the main user of HandshakeContext.

Adding a context-aware interface and upgrading to it in crypto/tls sounds good.

The SignContext helper is kinda counter-intuitive to me, as it might or might not use the context, and hides the receiver. Do we need it?

(I'll note that technically also Decrypter would need a context-aware version, but that's only used for the legacy RSA key exchange, and I am totally fine with leaving that behind.)

I don't think we need the helper either. I hadn't noticed it. I believe the proposal is simply to add

package crypto

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

and then use that interface in crypto/tls. No other API additions.

@FiloSottile @rsc As I mentioned in the original proposal, the SignContext helper function was just to make people's lives easier if they would be straddling both the old and new APIs, and was not strictly necessary. If crypto/tls would be the only consumer it can be omitted (for now).

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group