hash: add Clone

[FiloSottile]

There isn't a general way to clone the state of a hash.Hash, but #20573 introduced the concept of hash.Hash implementations also implementing encoding.BinaryMarshaler and encoding.BinaryUnmarshaler, and the hash.Hash docs commit our implementations to doing that.

Hash implementations in the standard library (e.g. hash/crc32 and crypto/sha256) implement the encoding.BinaryMarshaler and encoding.BinaryUnmarshaler interfaces.

That allows cloning the hash state without recomputing it, as done in HMAC.

go/src/crypto/hmac/hmac.go

Lines 96 to 103 in db40d1a

	marshalableInner, innerOK := h.inner.(marshalable)
	if !innerOK {
	return
	}
	marshalableOuter, outerOK := h.outer.(marshalable)
	if !outerOK {
	return
	}

However, it's obscure and pretty clunky to use.

I propose we add a hash.Clone helper function.

package hash

// Clone returns a separate Hash instance with the same state as h.
//
// h must implement encoding.BinaryMarshaler and encoding.BinaryUnmarshaler,
// or be provided by the Go standard library. Otherwise, Clone returns an error.
func Clone(h Hash) (Hash, error)

In practice, we should only fallback to BinaryMarshaler + BinaryUnmarshaler for the general case, while for standard library implementations we can do an undocumented interface upgrade to interface { Clone() Hash }. In that sense, hash.Clone is a way to hide the interface upgrade as a more discoverable and easier to use function.

(Yet another example of why we should be returning concrete types everywhere rather than interfaces.)

CloneXOF

If #69518 is accepted, I propose we also add hash.CloneXOF.

package hash

// CloneXOF returns a separate XOF instance with the same state as h.
//
// h must implement encoding.BinaryMarshaler and encoding.BinaryUnmarshaler,
// or be provided by the Go standard library or by the golang.org/x/crypto module
// (starting at version v0.x.y). Otherwise, Clone returns an error.
func CloneXOF(h XOF) (XOF, error)

None of our XOFs actually implement BinaryMarshaler + BinaryUnmarshaler, but they have their own interface methods Clone() ShakeHash and Clone() XOF that each return an interface. I can't really think of a way to use them from CloneXOF, so instead we can add hidden methods CloneXOF() hash.XOF and interface upgrade to them.

As we look at moving packages from x/crypto to the standard library (#65269) we should switch x/crypto/sha3 and x/crypto/blake2[bs] from returning interfaces to returning concrete types, at least for XOFs. Then they can have a Clone() method that returns a concrete type, and a CloneXOF() method that returns a hash.XOF interface and enables hash.CloneXOF.

(If anyone has better ideas for how to make this less redundant, I would welcome them. I considered and rejected using reflect to call the existing Clone methods because hash is a pretty core package. This sort of interface-method-that-needs-to-return-a-value-implementing-said-interface scenarios are always annoying.)

/cc @golang/security @cpu @qmuntal (who filed something similar in #69293, as I found while searching refs for this)

[gabyhelp]

Related Issues and Documentation

• proposal: hash: add HashCloner #69293 (closed)
• proposal: hash: add XOF interface #69518
• x/crypto/{blake2b,blake2s}: implement encoding.BinaryMarshaler and encoding.BinaryUnmarshaler #24548 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[magical]

How do you intend to implement the fallback path in Clone? MarshalBinary + UnmarshalBinary only gives you the ability to save and restore a Hash's state, not construct a new instance of it. You might be able to do it with reflect but it sounds like we're trying to avoid reflect.

[magical]

I'll also point out that, when using Clone to optimize repeated hashes with the same prefix, you really want to pair it with a Set method which copies the hash state from one existing instance to another.

e.g.

h := newHash()
h.Write(prefix)
h0 := h.Clone() // allocates
for _, message := range messages {
   h.Set(h0) // doesn't allocate
   h.Write(message)
   fmt.Println(h.Sum(nil))
}

Otherwise, without Set, you end up creating unavoidable garbage on every message.

h0 := newHash()
h0.Write(prefix)
for _, message := range messages {
   h := h0.Clone() // allocates
   h.Write(message)
   fmt.Println(h.Sum(nil))
}

My initial stab at the hmac optimization (before hashes implemented BinaryMarshaler and BinaryUnmarshaler) combined Clone and Set into a single method:

type HashCloner interface {
    hash.Hash

    // Clone returns a copy of its reciever, reusing the provided Hash if possible
    Clone(hash.Hash) hash.Hash
}

It looks a little odd but ends up being fairly ergonomic. One advantage this definition has over separate Clone and Set methods is that there is a nice answer for what to do when the argument and receiver types do not match: just allocate a new value. Whereas Set would have to panic.

// Example implementation for sha1.digest
func (d0 *digest) Clone(h hash.Hash) hash.Hash {
	d, ok := h.(*digest)
	if !ok {
		d = new(digest)
	}
	*d = *d0
	return d
}

h0 := newHash()
h0.Write(prefix)
var h hash.Hash
for _, message := range messages {
   h = h0.Clone(h)  // allocates on first iteration, reuses h on subsequent iterations
   h.Write(message)
   fmt.Println(h.Sum(nil))
}

[qmuntal]

This proposal tackles the same problem that made me start drafting #69293: there is no clean way to clone hashes.

About the proposed hash.Clone function, I had the same concerns as @magical. How do you instantiate a new hash before calling UnmarshalBinary? This I why in #69293 I proposed to define a hash.Cloner interface.

My motivation to have a common way to clone hash objects is to improve the compatibility of several hash implementations that I've implemented using CNG and OpenSSL with those libraries that are currently using MarshalBinary + UnmarshalBinary to clone a hash. The issue is that CNG/OpenSSL don't provide an API to serialize the internal state of a given hash, but they do provide APIs to clone a hash object.

[FiloSottile]

How do you intend to implement the fallback path in Clone? MarshalBinary + UnmarshalBinary only gives you the ability to save and restore a Hash's state, not construct a new instance of it.

Doh. Yes, that doesn't make sense, thank you.

We discussed this with @rsc and it it would make sense to follow the io/fs pattern:

package hash

// Clone returns a separate Hash instance with the same state as h.
//
// h must implement CloneHash, as all Hash implementations in the Go standard library do.
// Otherwise, Clone returns an error.
func Clone(h Hash) (Hash, error)

type CloneHash interface {
    hash.Hash
    Clone() hash.Hash
}

The question is whether we can make it not allocate by a combination of devirtualization and inlining. I think the answer is yes if either the type of the Hash passed to Clone devirtualizes or if it's a concrete type. Notably, I think the crypto/hmac use where a Hash is saved in the struct matches neither case. It would be useful if someone else could go through common cloning use cases

This isn't really urgent for Go 1.24. I don't want to add Clone methods to the new crypto/sha3 types (#69982) until we decide this, because there's a risk we'll make them not implement the interface, but crypto/sha3 can start with just MarshalBinary/UnmarshalBinary like every other stdlib Hash.

[mateusz834]

The question is whether we can make it not allocate by a combination of devirtualization and inlining. I think the answer is yes if either the type of the Hash passed to Clone devirtualizes or if it's a concrete type

Currently it would not work, see #64824.

[mateusz834]

@FiloSottile the io/fs pattern always includes the base interface, so:

type CloneHash interface {
    hash.Hash
    Clone() hash.Hash
}

What do you think?

EDIT: or even:

type CloneHash interface {
    hash.Hash
    Clone() hash.CloneHash
}

[FiloSottile]

Ah yes, that's what I meant to write. Not sure about returning a hash.CloneHash, I think it depends on whether we want users to pass the extended version around or keep it an "implementation detail" of hash.Clone.

[mateusz834]

Not sure about returning a hash.CloneHash, I think it depends on whether we want users to pass the extended version around or keep it an "implementation detail" of hash.Clone.

This way we also make sure that the returned (cloned) hash implements the hash.CloneHash, not doing so would let returning a non-cloneable hash.

[ericlagergren]

Has a generic hash.Clone been ruled out?

[mateusz834]

@FiloSottile Do we plan to implement this new interface by the crypto/hmac, this might influence the API, Clone() hash.Hash or Clone() hash.CloneHash must be allowed to return nil, because the provided hash constructor passed to hmac.New might return an hash.Hash that does not implement the interface (probably hash.Clone should treat that case as an error).

[mateusz834]

Or instead we can check whether the hash implements the new interface in hmac.New and return a different implementation that does not implement hmac.CloneHash in that case.

[FiloSottile]

Good point on crypto/hmac. I think I like the option of choosing the implementation in hmac.New the best.

[mateusz834]

Good point on crypto/hmac. I think I like the option of choosing the implementation in hmac.New the best.

This might cause issues in the future if we would like to make hmac.New alloc-free and devirtualizable. Also wouldn't this be problematic for the fips module (crypto/internal/fips/hmac)? It returns a concrete type now. But considering that crypto/hmac does not yet implement the encoding.BinaryMarshaler and Unmarshaler interfaces i assume that we don't need to make it clonable now.

go/src/crypto/hmac/hmac.go

Lines 34 to 37 in bea9b91

	// Note that unlike other hash implementations in the standard library,
	// the returned Hash does not implement [encoding.BinaryMarshaler]
	// or [encoding.BinaryUnmarshaler].
	func New(h func() hash.Hash, key []byte) hash.Hash {

[aclements]

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]

Talked to @FiloSottile about this and we agreed to leave this for Go 1.25.

[rsc]

It sounds like we agree on:

package hash

// Clone returns a separate Hash instance with the same state as h.
//
// h must implement CloneHash, as all Hash implementations in the Go standard library do.
// Otherwise, Clone returns an error.
func Clone(h Hash) (Hash, error)

type CloneHash interface {
    hash.Hash
    Clone() hash.Hash
}

Do I have that right?

(I'm assuming we ignore CloneXOF until XOF is accepted, and it can be part of the XOF proposal.)

[mateusz834]

@rsc #69521 (comment)

Personally i think that we should treat nil from CloneHash.Clone as an error condition in hash.Clone and just return an error.

[aclements]

Given that we're trying to move toward concrete hash types, it would be nice if you could clone a concrete hash type and get back the same concrete hash type. However, I can't quite figure out how to make this work because CloneHash would have to be parameterized and then I'm not sure how a non-parameterized func Clone(h Hash) (Hash, error) would actually work.

General question: Why do people need to clone hash functions? I think understanding this would help us in evaluating this proposal. @magical mentioned using this to "optimize repeated hashes with the same prefix", but I don't actually know why people would want to do that either.

[rolandshoemaker]

Re the failure case where a hash doesn't implement the interface, is this essentially only going to be the case for hashes implemented outside of the standard library? (Since we explicitly document All hashes in the standard library implement this interface).

I'm not really sure how common this is. There are plausibly hashes we don't implement, but I'm not sure I've ever actually seen anyone do this. Of course I'm not saying it doesn't happen, but I'm wondering how much complexity we should incur to handle this case, especially if it's extremely rare.

[aclements]

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

[qiulaidongfeng]

From CL 644275 ， Regarding crypto/hmac, there is another option. If it cannot be cloned, it should panic, similar to the panic of reflect.Value.Seq over values that cannot be iterated.

[aclements]

It's okay for reflect.Value.Seq to panic because the caller can (and should) check whether it's a sequence before calling that. The caller either knows a priori that it's a sequence, or had to query what kind of value it was before doing much of anything anyway.

For Clone, there's currently no way to check, meaning every generic Clone call would have to defensively handle the panic. If we were to allow Clone to panic, we'd need to add a CanClone() to check, but that's not a good approach either because it's an extra step that also very easy to forget.

[aclements]

The proposal committee discussed this and decided that, while there isn't a perfect choice, returning ErrUnsupported seems like the best option. This is literally the exact situation that's meant for. Panicking isn't ideal because it requires defensively handling the panic, and it would be easy to forget. Returning nil isn't a great option because it would also be very easy to forget to check for and would likely immediately lead to a more confusing panic. It could return a bool, but that's not very self-descriptive or standard.

[aclements]

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

The proposal is to add the following to the hash package:

package hash

// A Cloner is a hash function whose state can be cloned.
// All hashes in the standard library implement this interface.
// If a hash can only determine at runtime if it can be cloned,
// (e.g., if it wraps another hash), it may return [errors.ErrUnsupported].
type Cloner interface {
    Hash
    Clone() (Cloner, error)
}

And to implement the Clone method on all standard library hashes (most, but not all of which, already implement MashalBinary/UnmarshalBinary).

As a follow-on, ideally the compiler would implement optimizations such that, if the concrete type of the hash is fixed, it's possible to write an allocation-free Clone method.

[mateusz834]

As a follow-on, ideally the compiler would implement optimizations such that, if the concrete type of the hash is fixed, it's possible to write an allocation-free Clone method.

I already mailed CL 652036 for this. I added a test case for the new API and it still works. Not sure whether we want this for 1.25 though or whether we should defer that to 1.26.

[gopherbot]

Change https://go.dev/cl/674917 mentions this issue: crypto/internal/fips140: use hash.Hash

[gopherbot]

Change https://go.dev/cl/675197 mentions this issue: crypto,hash: add and implement hash.Cloner

[aclements]

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

The proposal is to add the following to the hash package:

package hash

// A Cloner is a hash function whose state can be cloned.
// All hashes in the standard library implement this interface.
// If a hash can only determine at runtime if it can be cloned,
// (e.g., if it wraps another hash), it may return [errors.ErrUnsupported].
type Cloner interface {
    Hash
    Clone() (Cloner, error)
}

And to implement the Clone method on all standard library hashes (most, but not all of which, already implement MashalBinary/UnmarshalBinary).

[mateusz834]

Unfortunately i was sleeping when the CL was reviewed :), so i haven't got a chance to take a look before it got submitted so i have question:

Cloner is documented as:

go/src/hash/hash.go

Lines 65 to 66 in 53b9eae

	// If a hash can only determine at runtime if it can be cloned,
	// (e.g., if it wraps another hash), it may return [errors.ErrUnsupported].

But ErrUnsupported explicitly mentions that it should be wrapped.

go/src/errors/errors.go

Lines 81 to 86 in 53b9eae

	// Functions and methods should not return this error but should instead
	// return an error including appropriate context that satisfies
	//
	// errors.Is(err, errors.ErrUnsupported)
	//
	// either by directly wrapping ErrUnsupported or by implementing an [Is] method.

Is this fine? The version #69521 (comment) noted that it wraps, but a docs version with an non-wrapping doc comment was accepted.

[FiloSottile]

Good catch, I mailed CL 675555.

[gopherbot]

Change https://go.dev/cl/675555 mentions this issue: crypto/hmac: wrap ErrUnsupported returned by Clone