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.

Sign up for GitHub

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

*: clarify we only deal with hex-encoded digests #32

Merged
merged 2 commits into from
Jan 8, 2020
Merged

*: clarify we only deal with hex-encoded digests #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b74b840
*: clarify we only deal with hex-encoded digests
jonboulle Mar 9, 2017
d1caf20
doc: tweak wording around algorithms
jonboulle Mar 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ Please see the [godoc](https://godoc.org/github.com/opencontainers/go-digest) fo

# What is a digest?

A digest is just a hash.
A digest is just a [hash](https://en.wikipedia.org/wiki/Hash_function).

The most common use case for a digest is to create a content
identifier for use in [Content Addressable Storage](https://en.wikipedia.org/wiki/Content-addressable_storage)
Expand Down Expand Up @@ -60,18 +60,21 @@ out when using this package.
```go
import (
_ "crypto/sha256"
_ "crypto/sha512"
_ "crypto/sha512"
)
```
This may seem inconvenient but it allows you replace the hash
implementations with others, such as https://github.com/stevvooe/resumable.

2. Even though `digest.Digest` may be assemable as a string, _always_
2. Even though `digest.Digest` may be assemblable as a string, _always_
verify your input with `digest.Parse` or use `Digest.Validate`
when accepting untrusted input. While there are measures to
avoid common problems, this will ensure you have valid digests
in the rest of your application.

3. While alternative encodings of hash values (digests) are possible (for
example, base64), this package deals exclusively with hex-encoded digests.
Copy link
Contributor

@wking wking Mar 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I can tell, with d1caf20 the package position will be:

  1. The algorithm identifier (e.g. ‘sha256’) identifies both the hash algorithm (e.g. SHA-256) and the encoding (e.g. lowercase base16).
  2. Algorithm identifiers which use encodings other than lowercase base 16 are allowed.
  3. This package will reject pull requests which would allow support for algorithm identifiers which use encodings other than lowercase base 16.
  4. This package will reject pull requests which would allow library consumers to register their own externally-defined Algorithm unless that externally-defined Algorithm uses a lowercase base 16 encoding.

Putting that all together, that's “you can define a new algorithm identifier using an encoding other than lowercase base 16, but if you do you won't be able to use go-digest at all, and we're not interested in providing a library that will work with you”. I find that position unfortunate, when the alternatives (either requiring hex encoding for all algorithm identifiers, or providing a framework that would support algorithm identifiers which used other encodings) seem so easy to adopt. But maybe I'm misunderstanding something about the package position?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that summarises the state of the world today. I don't have the effort to fight #31 so I'm just trying to clarify the current situation.

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this section. The doc update clarifies the point and the interface has been updated to be less opinionated about encoding. Other sections look good.

Copy link
Contributor

@wking wking May 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The doc update clarifies the point and the interface has been updated to be less opinionated about encoding.

The doc update in this PR says that all current algorithms in this package are hex (true). This README line is getting at “and this package currently has no interest in supporting users who want custom, non-hex algorithms”.

And while the interface seems less opinionated now that there is an Algorithm.Encode, that's currently hardcoded to assume hex, Algorithm is still not an interface (like I proposed in #30), and there is no way for consumers to register or use non-hex algorithms with this package. If there is a way to do that, I'd recommend pushing docs for “how you register your custom non-hex algorithm” to doc.go. With #30, you'd do it by pushing your Algorithm-interface implementing object into the Algorithms map.

Other sections look good.

Pulling the typo-fixes and such out (if they still apply) into less contentious PRs might help get them landed faster.

Copy link
Member

Choose a reason for hiding this comment

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

Can you link that doc?

Look down

Pulling the typo-fixes and such out (if they still apply) into less contentious PRs might help get them landed faster.

I am requesting the other way around, otherwise would just carry this PR with those changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Look down

Yup, sorry. I've edited my earlier comment.

I am requesting the other way around.

That's fine too, as long as we remove Fixes #31.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine too, as long as we remove Fixes #31.

Fair


# Stability

The Go API, at this stage, is considered stable, unless otherwise noted.
Expand Down
9 changes: 7 additions & 2 deletions doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,13 @@
//
// sha256:7173b809ca12ec5dee4506cd86be934c4596dd234ee82c0662eac04a8c2c71dc
//
// In this case, the string "sha256" is the algorithm and the hex bytes are
// the "digest".
// The "algorithm" portion defines both the hashing algorithm used to calculate
// the digest and the encoding of the resulting digest, which defaults to "hex"
// if not otherwise specified. Currently, all supported algorithms have their
// digests encoded in hex strings.
//
// In the example above, the string "sha256" is the algorithm and the hex bytes
// are the "digest".
//
// Because the Digest type is simply a string, once a valid Digest is
// obtained, comparisons are cheap, quick and simple to express with the
Expand Down