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

Add BLAKE3 hash function #61

Merged
merged 2 commits into from
May 18, 2021
Merged

Add BLAKE3 hash function #61

merged 2 commits into from
May 18, 2021

Conversation

sargun
Copy link
Contributor

@sargun sargun commented Feb 15, 2021

This adds the default BLAKE3 hash function with a default output size of 256-bit. If we want to add support for other sizes in the future, I suggest that we do something like blake3-512 or blake3_512. Given that BLAKE3 can have an arbitrary output size, I'm unsure which "variants" we should add.

@sargun
Copy link
Contributor Author

sargun commented Feb 15, 2021

See: opencontainers/image-spec#819

@vbatts
Copy link
Member

vbatts commented Feb 25, 2021

to what extent is github.com/zeebo/blake3 still being vetted?
Even comparing to golang.org/x/crypto/blake2b as a more reliable option?

@sargun
Copy link
Contributor Author

sargun commented Mar 1, 2021

@vbatts that's blake2b which is != blake3. I can also add blake2b, but it's a fundamentally different hash.

@sargun
Copy link
Contributor Author

sargun commented Mar 1, 2021

We use zeebo/blake3 in prod. Blake2 doesn't have formal specifications for XOF, or a a tree hash, whereas blake3 does. IIRC, there are variants of blake2b that support XOF and tree hash, but AFAIK, they're not formalized.

@vbatts
Copy link
Member

vbatts commented Mar 4, 2021

ah, i wasn't saying that "2b" and "3" were the same or equivalent.
I was more commenting on how established the implementation is, for the golang team to have an implementation of blake2b that they "support", vs a community member's implementation of blake3

blake3.go Outdated
import (
"hash"

"github.com/zeebo/blake3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want blake3 dependency in this repo (until blake3 becomes popular in the OCI ecosystem).

Instead, can we add functions for dynamically adding algorithms?

e.g.,

package digest

func Register(algorithm string, siz int, newHash func ()hash.Hash)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh clever

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that refactor.

@sargun
Copy link
Contributor Author

sargun commented Apr 21, 2021

Please see:
#62

This PR can move forward if we add a "sub-module" that has the blake3 bits, and people can import that in order to get that function to register.

@AkihiroSuda AkihiroSuda marked this pull request as draft April 21, 2021 08:39
@AkihiroSuda
Copy link
Member

Can we have blake3.go in a separate pacakge: github.com/opencontainers/go-digest/blake3

@sargun
Copy link
Contributor Author

sargun commented Apr 22, 2021

That's my plan.

@sargun sargun force-pushed the blake3 branch 2 times, most recently from fc04afd to eec210a Compare April 22, 2021 06:28
@sargun sargun requested review from vbatts and AkihiroSuda April 22, 2021 08:09
blake3/blake3_test.go Outdated Show resolved Hide resolved
"hash"

"github.com/opencontainers/go-digest"
"github.com/zeebo/blake3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any chance that this implementation will substantively change if there is a canonical (e.g. golang.org/x/crypto/...) package for blake3?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The cryptoHash interface is a subset of the hash interface in standard Go.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I meant the hash algorithm itself, not the interface itself.:-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm itself has been released and specified. It's in production use in a bunch of places. I'm pretty sure it's considered stable at this point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@vbatts
Copy link
Member

vbatts commented Apr 24, 2021

Is this still a draft?

@sargun sargun marked this pull request as ready for review April 27, 2021 23:19
@sargun
Copy link
Contributor Author

sargun commented Apr 27, 2021

@vbatts it's good to go. I have one small additional change of moving around the definition of the digest (name), but that can be next.

This adds support for the BLAKE3 hash family with a default output
size of 256-bit.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Because blake3 is a different go module, go test ./... doesn't
traverse into it, so it needs a separate stanza in the github
actions to test it.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
@sargun
Copy link
Contributor Author

sargun commented May 7, 2021

@vbatts This should be ready for review.
@AkihiroSuda Changes since last time:

  • Fixed test code
  • Moved the declaration of the blake3 "name" into the root module.

Copy link
Member

@vbatts vbatts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the iterations on this.

LGTM

@sargun
Copy link
Contributor Author

sargun commented May 17, 2021

@AkihiroSuda Do you have any outstanding concerns?

@AkihiroSuda
Copy link
Member

@dmcgowan PTAL

@dmcgowan dmcgowan merged commit b0b31a4 into opencontainers:master May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants