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

proposal: x/crypto/poly1305: implement a subset of the hash.Hash interface #25219

Open
mundaym opened this Issue May 2, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@mundaym
Copy link
Member

mundaym commented May 2, 2018

Currently the interface to poly1305, func Sum(out *[TagSize]byte, msg []byte, key *[32]byte), forces the user to copy everything they want to sum into a single slice (msg). It would be nice to have a way to append multiple slices and then generate the sum.

The main motivation would be to avoid an allocation in the x/crypto/chacha20poly1305 AEAD. We could avoid adding new API surface by implementing this in something like internal/poly1305 but I thought I'd create this proposal to see if we could just add it to the poly1305 package. I'm imagining the API would be used something like (not tested):

zeros := [16]byte{}
h := poly1305.New(key)
h.Write(additionalData)
if len(additionalData) % 16 != 0 {
     h.Write(zeros[:16 - (len(additionalData) % 16)] // pad to 16
}
h.Write(plaintext)
if len(plaintext) % 16 != 0 {
     h.Write(zeros[:16 - (len(plaintext) % 16)] // pad to 16
}
lengths := [16]byte{}
binary.LittleEndian.PutUint64(lengths[0:8], uint64(len(additionalData))
binary.LittleEndian.PutUint64(lengths[8:16], uint64(len(plaintext))
h.Write(lengths[:])
h.Sum(out[:len(plaintext)])

For reference this is the current code:

https://github.com/golang/crypto/blob/613d6eafa307c6881a737a3c35c0e312e8d3a8c5/chacha20poly1305/chacha20poly1305_generic.go#L31-L39

Also, @aead's poly1305 library already implements [EDIT: a subset of] hash.Hash if you want to take a look: https://github.com/aead/poly1305.

@gopherbot gopherbot added this to the Proposal milestone May 2, 2018

@FiloSottile

This comment has been minimized.

Copy link
Member

FiloSottile commented May 2, 2018

Poly1305 is a MAC and not Hash, so the naming would be misleading (although there is precedent in HMAC), it can't be reused with the same key, so Reset would be pointless, and can't be reused after calling Sum as it would be a key reuse.

I don't think hash.Hash is a good fit. Just a simple API with io.Writer and MAC([]byte) []byte would do.

@aead

This comment has been minimized.

Copy link
Contributor

aead commented May 2, 2018

I completely agree with Filippo here. That's exactly the reason why I did not implement hash.Hash for poly1305. IIRC this was already part of the chacha20poly1305 work - but was not really needed after Vald's combined assembler implementation.

@mundaym

This comment has been minimized.

Copy link
Member Author

mundaym commented May 2, 2018

Poly1305 is a MAC and not Hash, so the naming would be misleading (although there is precedent in HMAC), it can't be reused with the same key, so Reset would be pointless, and can't be reused after calling Sum as it would be a key reuse.

I don't think hash.Hash is a good fit. Just a simple API with io.Writer and MAC([]byte) []byte would do.

SGTM. I think I'd use the function name Sum() to match HMAC (that would allow people to make their own MAC interface and switch between the two), but I don't really mind.

IIRC this was already part of the chacha20poly1305 - but was not really needed after Vlad's combined assembler implementation.

Yeah, I'm hoping this and the chacha20 API changes make it possible to get good performance without the complexity of the combined assembly approach.

@FiloSottile

This comment has been minimized.

Copy link
Member

FiloSottile commented May 2, 2018

I think I'd use the function name Sum() to match HMAC

Sure. But panic on anything after it is called once.

Yeah, I'm hoping this and the chacha20 API changes make it possible to get good performance without the complexity of the combined assembly approach.

You have all my support to make that happen BTW. Reducing the need for and amount of assembly is an explicit goal/wishlist item of mine. Let me know if you need anything from my side in that sense.

@mundaym mundaym changed the title proposal: x/crypto/poly1305: implement hash.Hash interface proposal: x/crypto/poly1305: implement a subset of the hash.Hash interface May 3, 2018

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented May 3, 2018

Change https://golang.org/cl/111335 mentions this issue: poly1305: implement a subset of the hash.Hash interface

gopherbot pushed a commit to golang/crypto that referenced this issue Mar 8, 2019

poly1305: implement a subset of the hash.Hash interface
This CL adds the poly1305.MAC type which implements a
subset of the hash.Hash interface. With MAC it is possible
to compute an authentication tag of data without copying
it into a single byte slice.

This commit modifies the reference/generic and the
AMD64 assembler but not the ARM/s390x implementation
to support an io.Writer interface.

Updates golang/go#25219

Change-Id: I7ee5a9eadd43387cf3cd887d734c625575eee47d
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/111335
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.