Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ValarDragon opened this issue Mar 27, 2018 · 7 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ValarDragon
Copy link

ValarDragon commented Mar 27, 2018

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 gopherbot added this to the Proposal milestone Mar 27, 2018
@gopherbot
Copy link
Contributor

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

@aead
Copy link
Contributor

aead commented Mar 27, 2018

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
Copy link
Author

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
Copy link
Contributor

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.

@FiloSottile FiloSottile changed the title proposal: x/crypto/blake2b: Add a method to copy the internal state for blake2b x/crypto/{blake2b,blake2s}: implement encoding.BinaryMarshaler and encoding.BinaryUnmarshaler Mar 27, 2018
@FiloSottile FiloSottile modified the milestones: Proposal, Unreleased Mar 27, 2018
@FiloSottile FiloSottile added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Mar 27, 2018
@ValarDragon
Copy link
Author

ValarDragon commented Mar 28, 2018

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
Copy link
Contributor

aead commented Mar 28, 2018

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

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/103241 mentions this issue: blake2: implement BinaryMarshaler, BinaryUnmarshaler

@golang golang locked and limited conversation to collaborators Mar 30, 2019
mikroskeem pushed a commit to mikroskeem/golang-blake2s that referenced this issue Apr 4, 2021
The marshal method allows the hash's internal state to be serialized and
unmarshaled at a later time, without having the re-write the entire stream
of data that was already written to the hash.

Fixes golang/go#24548

Change-Id: I82358c34181fc815f85d5d1509fb2fe0e62e40bd
Reviewed-on: https://go-review.googlesource.com/103241
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
The marshal method allows the hash's internal state to be serialized and
unmarshaled at a later time, without having the re-write the entire stream
of data that was already written to the hash.

Fixes golang/go#24548

Change-Id: I82358c34181fc815f85d5d1509fb2fe0e62e40bd
Reviewed-on: https://go-review.googlesource.com/103241
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
The marshal method allows the hash's internal state to be serialized and
unmarshaled at a later time, without having the re-write the entire stream
of data that was already written to the hash.

Fixes golang/go#24548

Change-Id: I82358c34181fc815f85d5d1509fb2fe0e62e40bd
Reviewed-on: https://go-review.googlesource.com/103241
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
The marshal method allows the hash's internal state to be serialized and
unmarshaled at a later time, without having the re-write the entire stream
of data that was already written to the hash.

Fixes golang/go#24548

Change-Id: I82358c34181fc815f85d5d1509fb2fe0e62e40bd
Reviewed-on: https://go-review.googlesource.com/103241
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
The marshal method allows the hash's internal state to be serialized and
unmarshaled at a later time, without having the re-write the entire stream
of data that was already written to the hash.

Fixes golang/go#24548

Change-Id: I82358c34181fc815f85d5d1509fb2fe0e62e40bd
Reviewed-on: https://go-review.googlesource.com/103241
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
The marshal method allows the hash's internal state to be serialized and
unmarshaled at a later time, without having the re-write the entire stream
of data that was already written to the hash.

Fixes golang/go#24548

Change-Id: I82358c34181fc815f85d5d1509fb2fe0e62e40bd
Reviewed-on: https://go-review.googlesource.com/103241
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
desdeel2d0m added a commit to desdeel2d0m/crypto that referenced this issue Jul 1, 2024
The marshal method allows the hash's internal state to be serialized and
unmarshaled at a later time, without having the re-write the entire stream
of data that was already written to the hash.

Fixes golang/go#24548

Change-Id: I82358c34181fc815f85d5d1509fb2fe0e62e40bd
Reviewed-on: https://go-review.googlesource.com/103241
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants