golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.18k stars 17.57k forks source link

x/crypto/{md4,ripemd160,sha3}: implement encoding.BinaryMarshaler and encoding.BinaryUnmarshaler #24617

Open ValarDragon opened 6 years ago

ValarDragon commented 6 years ago

This is the same idea as #24548, make these hash functions in crypto implement encoding.BinaryMarshaler and BinaryUnmarshaler to allow for state reuse. The password hashes aren't listed here, since they should be using a unique salt per hash, and therefore they wouldn't benefit from state reuse. (Also it seems that the password hashes don't implement hash.Hash)

FiloSottile commented 6 years ago

Wow, we seriously have md4 in there. Don't bother adding support to that.

sabin-rapan commented 6 years ago

@ValarDragon are you implementing those interfaces for all the mentioned packages? I'd like to join :grin:.

ValarDragon commented 6 years ago

I have it all implemented (even md4 since I had that done before I saw FiloSottile's comment), I'll push a change for code-review later today, I just need to make the test error messages more verbose. However I think there are some optimizations that can be done for the way I'm marshaling sha3. I think the only use case for marshaling it is when its in the absorbing state, not when its squeezing. (Even the tests for cloning don't cover the case where the state is squeezing) Currently I'm storing a byte for this case, but I'm not sure its needed. (Also, in the case that its in the squeezing state, I actually have to store an additional byte. Right now I am only storing that byte when its needed)

gopherbot commented 6 years ago

Change https://golang.org/cl/104576 mentions this issue: sha3,md4,ripemd160: implement BinaryMarshaler, BinaryUnmarshaler

ValarDragon commented 6 years ago

Is there anything else that needs to be done for this proposal on gerrit? (Asking because there haven't been any comments in gerrit, and its been awhile)

suhjohn commented 2 years ago

What's the reason for this not being merged

gopherbot commented 8 hours ago

Change https://go.dev/cl/616635 mentions this issue: sha3: add MarshalBinary, AppendBinary, and UnmarshalBinary