descriptor: recognise sha512, tweak wording #609

jonboulle · 2017-03-10T15:50:48Z

As previously discussed in #589, implementations may wish to use SHA-512 for
performance/security reasons, and since our endorsed digest package also
supports it, let's add the algorithm identifier to the recognised ones in the
spec.

At the same time, we keep wording to clarify (and indeed mandate) that SHA-256
MUST be supported by implementations and is preferred for interoperability
reasons.

wking · 2017-03-10T17:50:36Z

descriptor.md


-The value of the digest property, the _digest string_, is a serialized hash result, consisting of an _algorithm_ portion and a _hex_ portion.
-The algorithm identifies the methodology used to calculate the digest; the hex portion is the lowercase hex-encoded result of the hash.
+The value of the digest property, the _digest string_, is a serialized hash result, consisting of an _algorithm_ portion (the "algorithm identifier") and a _hex_ portion (the digest).


I would prefer we refer to the whole string as the “digest” and the part after the semicolon as the “hash” or the “hex” to match our definition and implementation. Calling the hash/hex portion the “digest” is confusing if the whole thing is so similar (the “digest string”) and is represented by the Digest type.

It's hard to find consistency in the terminology use in this area, but I agree this can be improved; see 23af834

vbatts · 2017-03-13T12:53:42Z

LGTM

wking · 2017-03-13T16:51:32Z

descriptor.md

@@ -76,20 +76,17 @@ hex         := /[a-f0-9]+/

 Some example digest strings include the following:

-digest                                                                  | algorithm           |
+digest string                                                           | algorithm           |


With 23af834 moving us towards consistently using digest/algorithm (identifier)/hex matching our ABNF, I think we may want to stick to “digest” instead of using “digest string”. I don't mind if existing instances of “digest string” are changed to “digest” in this PR or not, but I think this line (and the “Before consuming…” line below) should be left alone in this PR.

It's fine like this in context. If you want then submit a follow up that changes those AND the preceding two references.

This is just referred to as a "digest". String here is redundant.

jbouzane

I'll buy that we can resolve the digest vs. digest string thing separately.

wking · 2017-03-15T22:06:11Z

@jbouzane approved these changes an hour ago

This is another ignored-by-PullApprove approval ;). Did you want to LGTM this PR?

vbatts · 2017-04-03T23:23:11Z

needs a rebase

Besides the mailing list thread referenced in the diff, this algorithm identifier is supported in go-digest [1]. [1]: https://github.com/opencontainers/go-digest/blob/v1.0.0-rc0/algorithm.go#L33 Signed-off-by: W. Trevor King <wking@tremily.us>

Signed-off-by: Jonathan Boulle <jonathanboulle@gmail.com>

jonboulle · 2017-04-04T14:23:09Z

rebased

vbatts · 2017-04-05T14:50:41Z

LGTM

jonboulle · 2017-04-06T13:44:48Z

bump (before something happens that requires another rebase!)

philips · 2017-05-03T23:17:13Z

LGTM

stevvooe · 2017-05-04T18:57:53Z

Would have liked to have reviewed this one before going forward...

stevvooe · 2017-05-04T18:58:53Z

descriptor.md

@@ -59,10 +59,10 @@ Extended _Descriptor_ field additions proposed in other OCI specifications SHOUL

 The _digest_ property of a Descriptor acts as a content identifier, enabling [content addressability](http://en.wikipedia.org/wiki/Content-addressable_storage).
 It uniquely identifies content by taking a [collision-resistant hash](https://en.wikipedia.org/wiki/Cryptographic_hash_function) of the bytes.
-If the identifier can be communicated in a secure manner, one can retrieve the content from an insecure source, calculate the digest independently, and be certain that the correct content was obtained.
+If the digest can be communicated in a secure manner, one can retrieve the content from an insecure source, recalculate the digest independently, and be certain that the correct content was obtained.


Changing to digest here is incorrect. We are trying to communicate the concept of calculating a common identifier.

stevvooe · 2017-05-04T19:01:09Z

descriptor.md

 * Before calculating the digest, the size of the content SHOULD be verified to reduce hash collision space.
 * Heavy processing before calculating a hash SHOULD be avoided.
-* Implementations MAY employ some canonicalization of the underlying content to ensure stable content identifiers.
+* Implementations MAY employ [canonicalization](canonicalization.md) of the underlying content to ensure stable content identifiers.


This implies that the canonicalization is limited to those described in the document. Implementations may employ any kind of canonicalization they want in the generation of content.

stevvooe · 2017-05-04T19:01:56Z

descriptor.md


-While the _algorithm_ component of the digest does allow one to utilize a wide variety of algorithms, compliant implementations SHOULD use [SHA-256](#sha-256).


While the algorithm component of the digest does allow one to utilize a wide variety of algorithms, compliant implementations SHOULD use those specified here.

Nevermind, this moved down.

stevvooe · 2017-05-04T19:06:59Z

descriptor.md

 Implementations MUST implement SHA-256 digest verification for use in descriptors.

+#### SHA-512
+
+[SHA-512][rfc4634-s4.2] is a collision-resistant hash function which [may be more perfomant][sha256-vs-sha512] than [SHA-256](#sha-256) on some CPUs.


This implies that performance is a good reason to select one digest algorithm over another, when that is not the case at all. The most important factors in the selection of a digest algorithm is that it is common to all implementations.

In practice, the use of sha512 will likely cause the introduction of incompatible images.

stephenrwalli · 2017-05-04T20:29:25Z

This request is a late change as you try to close the Image Spec.
It is defined in implementation specific language about performance.
This essentially looks like folks trying to ensure their implementation specific change is allowed by the specification, while possibly preventing other implementation changes in the space.
It's a product versus specification timing issue.

If you are focused on finishing a spec I would suggest you either:

Try to keep it pure. Mandate the simple. Say nothing about other possible implementations.
Accept this is a space with a lot of change still possible. Mandate the simple. Specifically call out that implementations MAY do other things, thereby warning implementers and users alike that this space can't be guaranteed.

vbatts · 2017-05-04T21:42:50Z

@stephenrwalli I get calling out that implementations MAY choose others, but this is not a late change. Calling out a hash like sha512 has been a couple year lingering conversation. Besides sha256 isn't immediately going away. This is not a product vs. spec issue.

wking reviewed Mar 10, 2017

View reviewed changes

wking reviewed Mar 13, 2017

View reviewed changes

jbouzane approved these changes Mar 15, 2017

View reviewed changes

jonboulle mentioned this pull request Mar 16, 2017

.pullapprove.yml: List project maintainers separately opencontainers/project-template#29

Merged

wking mentioned this pull request Mar 17, 2017

.pullapprove.yml: Switch to v2 and other project-template updates #616

Merged

descriptor: Register sha512

ed86220

Besides the mailing list thread referenced in the diff, this algorithm identifier is supported in go-digest [1]. [1]: https://github.com/opencontainers/go-digest/blob/v1.0.0-rc0/algorithm.go#L33 Signed-off-by: W. Trevor King <wking@tremily.us>

jonboulle force-pushed the algorithms branch from 23af834 to 83f6ed0 Compare April 4, 2017 14:21

descriptor: improve consistency in use of "digest"

bbe399d

Signed-off-by: Jonathan Boulle <jonathanboulle@gmail.com>

jonboulle force-pushed the algorithms branch from 83f6ed0 to bbe399d Compare April 4, 2017 14:22

wking mentioned this pull request May 3, 2017

schema: allow compound algorithm specifiers in digests #654

Merged

vbatts merged commit d92d3ed into opencontainers:master May 3, 2017

stevvooe reviewed May 4, 2017

View reviewed changes

vbatts mentioned this pull request May 19, 2017

Bump version to rc6 #681

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

descriptor: recognise sha512, tweak wording #609

descriptor: recognise sha512, tweak wording #609

jonboulle commented Mar 10, 2017

wking Mar 10, 2017 •

edited

Loading

jonboulle Mar 13, 2017

vbatts commented Mar 13, 2017 •

edited by caniszczyk

Loading

wking Mar 13, 2017 •

edited

Loading

jonboulle Mar 13, 2017

stevvooe May 4, 2017

jbouzane left a comment

wking commented Mar 15, 2017

vbatts commented Apr 3, 2017

jonboulle commented Apr 4, 2017

vbatts commented Apr 5, 2017 •

edited by caniszczyk

Loading

jonboulle commented Apr 6, 2017

philips commented May 3, 2017 •

edited by caniszczyk

Loading

stevvooe commented May 4, 2017

stevvooe May 4, 2017

stevvooe May 4, 2017

stevvooe May 4, 2017

stevvooe May 4, 2017

stevvooe May 4, 2017

stephenrwalli commented May 4, 2017

vbatts commented May 4, 2017


		While the _algorithm_ component of the digest does allow one to utilize a wide variety of algorithms, compliant implementations SHOULD use [SHA-256](#sha-256).

descriptor: recognise sha512, tweak wording #609

descriptor: recognise sha512, tweak wording #609

Conversation

jonboulle commented Mar 10, 2017

wking Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbatts commented Mar 13, 2017 • edited by caniszczyk Loading

wking Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbouzane left a comment

Choose a reason for hiding this comment

wking commented Mar 15, 2017

vbatts commented Apr 3, 2017

jonboulle commented Apr 4, 2017

vbatts commented Apr 5, 2017 • edited by caniszczyk Loading

jonboulle commented Apr 6, 2017

philips commented May 3, 2017 • edited by caniszczyk Loading

stevvooe commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenrwalli commented May 4, 2017

vbatts commented May 4, 2017

wking Mar 10, 2017 •

edited

Loading

vbatts commented Mar 13, 2017 •

edited by caniszczyk

Loading

wking Mar 13, 2017 •

edited

Loading

vbatts commented Apr 5, 2017 •

edited by caniszczyk

Loading

philips commented May 3, 2017 •

edited by caniszczyk

Loading