descriptor: Define the 'sha256' algo identifier #587
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before this commit, there wasn't something obvious to point to if you wanted to explain the sha256 identifier. The "SHOULD be submitted" wording follows runtime-spec's example [1]. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc4/config.md#platform Signed-off-by: W. Trevor King <[email protected]>
Contributor
|
The spec has left the algorithm undefined, purposefully. Defining disallows implementations from choosing other hashes. Is there anywhere else where we make this distinction? |
Contributor
Author
|
On Tue, Feb 28, 2017 at 04:19:23PM -0800, Stephen Day wrote:
The spec has left the algorithm undefined, purposefully. Defining
disallows implementations from choosing other hashes.
The intent here is not to limit algorithm choices (can you point to
the wording that gives you that impression?).
The idea is that when I use the ‘sha256’ algorithm identifier, folks
are clear that I mean the SHA-256 algorithm as specified in [1]. I
don't want folks hashing with SHA-224 or some such instead and then
getting confused when the hashes don't match. Before this commit,
there is nothing besides examples to make the connection between the
‘sha256’ identifier and the SHA-256 algorithm as specified in [1].
This commit makes the ‘sha256’ algorithm identifier reliable.
A second benefit of an algorithm-identifier registry is that digest
authors are more likely to reuse existing identifiers. Without a
registry (or a bunch of examples acting as a quasi-registry), Alice
might create digest with ‘sha-256’ as the algorithm identifier for
SHA-256. With this commit she's still allowed to do that, but she's
more likely to discover the ‘sha256’ identifier and use that instead
of inventing her own identifier. That increases the odds that digests
will use reliable algorithm identifiers.
And again, there is only a SHOULD-level request for algorithm
identifier registration. If you want to use ‘my-secret-algorithm’ and
not register it, go right ahead. Just don't be surprised when Bob's
digester chokes with “unrecognized algorithm: my-secret-algorithm”.
[1]: https://tools.ietf.org/html/rfc4634#section-4.1
|
Contributor
|
LGTM |
Contributor
Saying one thing exists creates the presumption that unmentioned things don't exist. The format is already defined and documented here. While I wasn't quite accurate above, we already call out a whole section on I think the same effect can be had by simply adding a sentence after the following: If you think a table with one item is great, then fine but this seems better. |
Contributor
Author
|
On Wed, Mar 01, 2017 at 11:23:48AM -0800, Stephen Day wrote:
> I also don't see how it disallows/limits anything.
Saying one thing exists creates the presumption that unmentioned
things don't exist.
I feel like the “If a useful algorithm is not included in the above
table…” provides a sufficient hint that the table is not exhaustive.
If you feel like it needs additional/alternative wording to make that
point clear, what sort of wording would you like to see?
The format is already defined and documented here.
We already define the ‘algorithm ":" hex’ format [1], but that does
not define any values for the algorithm component. Defining at least
the ‘sha256’ value is what I'm trying to add here.
While I wasn't quite accurate above, we already call out a whole
section on `sha256`.
Not quite. We have a section on SHA-256 the algorithm [2], but there
is no mention in that section on ‘sha256’ the identifier.
This PR just adds more unnecessary indirection. If this actually
added value or context, I would be in favor. As it stands, it just
plops another table in a confusing spot.
Maybe. If ‘sha256’ is the only algorithm identifier we're ever going
to document, I agree that the table is overkill and your proposed
wording seems fine. However, go-digest already defines two additional
algorithm identifiers [3]. I feel like at least sha512 should be
registered as well, although that seemed out of scope for this PR.
Using a table here sets the stage for subsequent registrations like
that. But again, if there is no intention of registering other
algorithm identifiers, let me know and I'll update this PR.
[1]: https://github.com/opencontainers/image-spec/blame/v1.0.0-rc5/descriptor.md#L72
[2]: https://github.com/opencontainers/image-spec/blob/v1.0.0-rc5/descriptor.md#sha-256
[3]: https://github.com/opencontainers/go-digest/blob/v1.0.0-rc0/algorithm.go#L32-L33
|
Contributor
|
LGTM @wking Fair enough. I think we can maintain this as a restricted set, but we need to be careful about adding new algorithms. While there are hooks for making it happen, adding support for a new algorithm should not be casual. What sha256 is broken, it will be a once in a decade event. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this commit, there wasn't something obvious to point to if you wanted to explain the
sha256identifier.The “SHOULD be submitted” wording follows runtime-spec's example.