x/crypto/{blake2b,blake2s}: implement encoding.BinaryMarshaler and encoding.BinaryUnmarshaler

[ValarDragon]

It is often useful to be able to copy the internal state for blake2b, such as if you want to compute Blake2b(msg || tag1) and Blake2b(msg || tag2), and you want to avoid double-computing what results from updating msg.

I've added a clone method to do this here. This is an improvement that can't be made in applications using the library, since the digest struct isn't exported.

Since the publicly visible interface for digest is hash.Hash, I made the method for cloning func Clone(h hash.Hash) (*digest, error) instead of func (d *digest) Clone (*digest).

This could alternatively be added as a method that everything which implements the hash.Hash interface must implement, since there have been requests for this before. (Link 1, Link 2)

[gopherbot]

Change https://golang.org/cl/102617 mentions this issue: blake2b: Add Clone method

[aead]

I don't think adding a custom Clone function to every hash function is a proper solution.
I think the idea of this proposal is related #20573 - However there is a fundamental problem using encoding.BinaryMarshaler and encoding.BinaryUnmarshaler for functions that can be used as MAC (like BLAKE2, SHA-3, HMAC):

1. Either we serialize and store the secret key which is IMO not a good idea because of possibly leaking sensitive information (accidentally).
2. Or we exclude the secret key which leads to the following problem:

   key := make([]byte, 32) // secret key
   b2, _ := blake2b.New(key)
   
       var err error
   var b2State []byte
   if marshaler, ok := b2.(encoding.BinaryMarshaler); ok {
   	b2State, err = marshaler.MarshalBinary()
   	if err != nil {
   		fmt.Println(err)
   		return
   	}
   }
   
   if unmarshaler, ok := b2.(encoding.BinaryUnmarshaler); ok {
   	err = unmarshaler.UnmarshalBinary(b2State)
   	if err != nil {
   		fmt.Println(err)
   		return
   	}
   }
   
   b2.Reset() // what should happen here
   fmt.Println(hex.EncodeToString(b2.Sum(nil))) // what should be printed
   

BTW: adding a function to hash.Hash is not possible - backward compatibility.

@ValarDragon Have you considered using a XOF - like BLAKE2X

/cc @FiloSottile

[ValarDragon]

Thanks for explaining the situation with encoding.BinaryMarshaler, I didn't realize that existed. That solves the problem I was having (whereas blake2x wouldn't since I needed compatibility with blake2b hashes in another language).

I agree, adding a custom clone method to every hash is not a good solution. For the HMAC situation, I'm not really sure thats a problem, except for the reset situation as you mentioned. But in principle if you want to re-use the state right after its keyed, you would marshal at that point, and keep on unmarshalling that for the copy of the state. For the reset situation, you're right it is confusing what to do there. I suppose it is an arbitrary decision, so the best option may be to use nil as the key. (Since there is no error return value)

[FiloSottile]

I think for now it would be ok to make MarshalBinary return an error for keyed hashes. Once vgo happens we can think more about the API. Also, we'll know if it's a commonly requested feature and which version of it users expect.

[ValarDragon]

I started an implementation for this, but I was wondering what I should do for test cases. The other hash functions (sha512, sha256, md5) have a TestGolden method, where they test the hash function with a known set of strings. There is no test golden method for blake2b. Should I make the same test golden method here, or is there another preferred way for testing this? The known test cases for blake2b are taken from here, but these were all produced with a key, which means that we would have to enable marshalling for keyed digests to use those test cases.

I'm planning on serializing offset as uint32, and size as uint8. Offset seems to always be positive, and size never exceeds the number 64, so it can fit into 1 byte.

[aead]

Hm - you could follow the same approach but instead of 2 instances of the hash function and a set of precomputed hashes you can use 3 hash instances and use the 3-th as reference.
Like:

h3, _ := New(nil)
h3.Write(testString)

// now do marshal/unmarshal test using h and h2
// compare h and h2 with h3