*: clarify we only deal with hex-encoded digests #32
Conversation
Also fixes a typo and adds one clarifying link in the README. Fixes opencontainers#31 Signed-off-by: Jonathan Boulle <[email protected]>
|
LGTM |
doc.go
Outdated
| @@ -32,6 +32,8 @@ | |||
| // In this case, the string "sha256" is the algorithm and the hex bytes are | |||
| // the "digest". | |||
| // | |||
| // This package exclusively uses hex encoding for all digests. | |||
There was a problem hiding this comment.
... for currently supported algorithms. The hex limitation is about the encoding of digest that is bound to the algorithm. New "algorithms" may support other kinds of encodings.
There was a problem hiding this comment.
IMO this wording is a bit confusing, but I've posted a revised take @ d1caf20, PTAL
Signed-off-by: Jonathan Boulle <[email protected]>
| 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. |
There was a problem hiding this comment.
As far as I can tell, with d1caf20 the package position will be:
- The algorithm identifier (e.g. ‘sha256’) identifies both the hash algorithm (e.g. SHA-256) and the encoding (e.g. lowercase base16).
- Algorithm identifiers which use encodings other than lowercase base 16 are allowed.
- This package will reject pull requests which would allow support for algorithm identifiers which use encodings other than lowercase base 16.
- This package will reject pull requests which would allow library consumers to register their own externally-defined
Algorithmunless that externally-definedAlgorithmuses 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's fine too, as long as we remove Fixes #31.
Fair
|
LGTM |
|
ping @dmcgowan PTAL |
|
LGTM |
Also fixes a typo and adds one clarifying link in the README.
Fixes #31