descriptor: Register sha512

[wking]

Besides the mailing list thread referenced in the diff, this algorithm identifier is supported in go-digest.

[stevvooe]

I don't see any reason to call this out as a supportable digest. This will only create divergence in implementations that will try to differentiate themselves through hash selection. For the most part, sha256 is fast enough and unique enough to be sufficient for this use case.

Making this decision based on performance isn't even sound. Not all of the tests take into all variations. For example, even though sha512 should be more performant on 64-bit, here are the results on my laptop:

BenchmarkHash8Bytes-4   	 5000000	       302 ns/op	  26.46 MB/s
BenchmarkHash1K-4       	  500000	      3514 ns/op	 291.37 MB/s
BenchmarkHash8K-4       	   50000	     25935 ns/op	 315.86 MB/s
PASS
ok  	crypto/sha256	5.200s

BenchmarkHash8Bytes-4   	 3000000	       582 ns/op	  13.72 MB/s
BenchmarkHash1K-4       	  300000	      4297 ns/op	 238.29 MB/s
BenchmarkHash8K-4       	   50000	     29743 ns/op	 275.42 MB/s
PASS
ok  	crypto/sha512	5.460s

Giving a choice here really will only generate arguments and not compatible images.

Migrating a digest algorithm should be a once in a decade event. Let's do this when sha256 is broken and not waste a second of time before that happens.

[wking]

On Mon, Mar 06, 2017 at 03:50:18PM -0800, Stephen Day wrote: Migrating a digest algorithm should be a once in a decade event. Let's do this when `sha256` is broken and not waste a second of time before that happens.

Migrating hashes should *not* be a flag-day event. The point of the algorithm identifier in digests is that image producers/consumers can add support for other alorithms as they see fit, and the ecosystem can gradually evolve between algorithms. The paranoid folks can add safer (and possibly more expensive) hashes, support for consuming them will percolate across the ecosystem, as support grows there is less need to use the old hashes, and eventually local policy is “we no longer accept digests using $OLD_VULNERABLE_ALGO”. This is not a new idea, and can be seen in all sorts of crypto contexts (e.g. [1,2]). I'm fine if image-spec maintainers want implementation notes like: Implementers concerned about portability SHOULD prefer algorithm identifiers which MUST be supported (like [`sha256`](#sha-256)). or: Implementers concerned about performance SHOULD prefer [`sha256`](#sha-256), because it makes [deduplication](considerations.md#canonicalization) more likely. or whatever. But I don't think “we'll add a new algorithm identifier when *we* think SHA-256 is broken” is a good approach. And I don't think “we'll gate-keep algorithms by only registering algorithms that we want to encourage” is a good approach. I think the right registration criterion is “is this algorithm widely implemented?”, and anything under [3] probably meets that criterion. [1]: http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_ciphers [2]: https://www.openssl.org/docs/manmaster/man1/ciphers.html#CIPHER-STRINGS [3]: https://golang.org/pkg/crypto/#pkg-subdirectories

[stevvooe]

@wking Since you refused to address my actual premise and points, I'm going to close this PR.

Thank you for your submission!

[wking]

On Mon, Mar 06, 2017 at 05:15:05PM -0800, Stephen Day wrote: @wking Since you refused to address my actual premise and points, I'm going to close this PR.

I tried to make a case for what I think is the appropriate yardstick for registering algorithm identifiers. But if you didn't find that convincing, I can address your other points. Mon, Mar 06, 2017 at 03:50:18PM -0800, Stephen Day:

I don't see any reason to call this out as a supportable digest.

The reasons for registering any algorithm identifier were covered in #587. Basically, if you expect any two people to want to share digests made with a given algorithm, you increase the odds of them picking the same algorithm identifier by registering the identifier here.

This will only create divergence in implementations that will try to differentiate themselves through hash selection.

If folks want to use SHA-512 (or whatever), they can *already* use SHA-512. There are trade-offs to be considered (some of which I outlined in [1]), but some digest producers will likely be better off with SHA-512 after considering those trade-offs. What this PR does is make it more likely for those people to use ‘sha512’ as their identifier (instead of ‘sha-512’ or whatever).

For the most part, sha256 is fast enough and unique enough to be sufficient for this use case.

For you, maybe. But why block registration based on your assessment? It seems to me that the critical feature is how hard it is to discover an attack payload which collides with the intended content. I'm not enough of a crypo person, and would be surprised if any image-spec maintainers are either, to want to be in the position of making that call for consumers. And again [1], you really don't want to be registering your second algorithm identifier on the day that the first SHA-256 exploit goes public.

Making this decision based on performance isn't even sound. Not all of the tests take into all variations. For example, even though sha512 should be more performant on 64-bit, here are the results on my laptop:

I've run ‘openssh test sha256’ and ‘openssh test sha512’ on my work machines and come to the same “SHA-512 is faster” conclusion that @jbouzane reported for a 2015 Skylake [2]. If I'm distributing an image internally, why would I care about your laptop? Of course, if I'm distributing an image for general consumption, picking the MUST-be-implemented sha256 is clearly a better call than the MAY-be-supported sha512. But not all images are intended for general consumption.

Giving a choice here really will only generate arguments…

Arguments like this one? I think getting clear criteria for algorithm identifier registration is a useful discussion to have. I don't expect there to be significant arguments within publishing groups about which algorithm to use, since you can lay out the points on each side, and whoever makes the calls can make the call. That applies to this PR too. I'm laying out the arguments in favor of registering ‘sha512’, and if the image-spec maintainers feel that the arguments against registering it are stronger, they can close the PR.

… and not compatible images.

Do you mean “my sha256-only unpacker is choking on your sha512 digest”? I agree that this could happen (because sha512 support is only MAY in this PR). But I don't think it's a problem. That MAY support was clear to the digest author when they made the decision to use sha512. They decided to use sha512 anyway, which probably means they didn't care about interop with your unpacker. That's a valid position. For example, anyone who uses an image index feels the benefits outweigh the need for compat with all OCI-compliant consumers, because we only SHOULD index support [3]. And again, I think this PR is mostly shifting digesters between sha512 and sha-512, etc. and less about shifting digesters between sha512 and sha265. [1]: #589 (comment) [2]: https://groups.google.com/a/opencontainers.org/d/msg/dev/hsMw7cAwrZE/UiI1lJ_EDgAJ Subject: Re: Hash Algorithms and Performance Date: Wed, 4 Nov 2015 11:37:11 -0800 Message-ID: <CAFi6z1Eb4K0eppRq1rG08jOfniq2RfognSuczeQfo2TNxu06Kg@mail.gmail.com> [3]: https://github.com/opencontainers/image-spec/blame/v1.0.0-rc5/image-index.md#L4

[jonboulle]

+1 overall, I don't understand the contention.

This will only create divergence in implementations that will try to differentiate themselves through hash selection.

if implementations try to differentiate themselves through hash selection, that's entirely their prerogative

For the most part, sha256 is fast enough and unique enough to be sufficient for this use case.

That's fine; we already (and still) stipulate that all implementations should support this. I would even be happier to turn this into a MUST if that would make you more comfortable (or just one of wking's slightly stronger SHOULD variations).

Migrating a digest algorithm should be a once in a decade event. Let's do this when sha256 is broken and not waste a second of time before that happens.

as already said this should not be a flag-day operation.

Frankly I don't see the fuss, since SHA256 and SHA512 are so similar I imagine most libraries people are using in practice support both.

[jonboulle]

SHA-512?

[wking]

Oops, thanks :). Fixed with 09b5988 → 781425f.

[jonboulle]

resistant

[wking]

Thanks. Fixed with 09b5988 → 781425f.

[wking]

On Tue, Mar 07, 2017 at 07:23:05AM -0800, Jonathan Boulle wrote: > For the most part, sha256 is fast enough and unique enough to be > sufficient for this use case. That's fine; we already (and still) stipulate that all implementations should support this. I would even be happier to turn this into a MUST if that would make you more comfortable (or just one of wking's slightly stronger SHOULD variations).

Are you saying you'd like to MUST sha512? I don't see a point to that, since folks who *want* to support it are free to do so and we can use sha256 (which we already MUST [1]) for basic OCI-compliance testing. But sha512 wouldn't be hard to implement either, so I don't have major problems with MUSTing it if folks feel strongly about it. [1]: https://github.com/opencontainers/image-spec/blame/v1.0.0-rc5/descriptor.md#L115

[jonboulle]

no, I meant sha256, to mitigate the apparent concern about divergence.

[wking]

no, I meant sha256...

If you don't read the already-in-master wording as doing that now, you should probably file a separate PR to fix that. The current wording there may have too much space between "SHA-256" and "sha256", but it seems orthogonal to this PR.

[jonboulle]

@wking ahh, I see the source of my confusion - we have a SHOULD here. I guess the current master wording is okay.

[wking]

On Tue, Mar 07, 2017 at 08:40:16AM -0800, Jonathan Boulle wrote: ahh, I see the source of my confusion - we have a SHOULD [here](https://github.com/opencontainers/image-spec/blame/master/descriptor.md#L90).

Yeah, I think *that* SHOULD is aimed at digest producers, while the MUST in [1] is aimed at digest consumers. [1]: https://github.com/opencontainers/image-spec/blame/v1.0.0-rc5/descriptor.md#L115

[stevvooe]

@jonboulle He has been asked a number of times to limit the length and scope of his responses to be on point and constructive. At this time, he has made no concerted effort to do this. His continued verbosity has ended up wasting a lot of time in a number of cases, including my own submissions. On my PR for chain ID clarifications, he made something like 46 comments in 24 hours, on a proposal that has been available for at least a month. Not only that, but after his points were clarified, he continued to argue his position, continuing to ignore premises and arguments of others. As an active maintainer and contributor to this project, his incessant badgering borders on harassment.

Even worse, his continued verbosity led to having @philips step down from pushing releases and the delay of a 1.0 release of this specification. Had so much time not been wasted on arguing minutia, we would probably have released a 1.0 now.

In this case, the immediate closure of this issue was due to the fact that he did not respond to the feedback provided in my review comment. My original concern was this:

This will only create divergence in implementations that will try to differentiate themselves through hash selection.

Instead, he chose to argue about breaking hashes, which was a minor point.

On its own, this PR is probably okay, but in context of the thousands of comments issued by this individual, on a specification which companies are actually betting on, this behavior is unacceptable.

@jonboulle If you think this is a reasonable addition, please submit a PR, where we can discuss the merits and stipulations to make this work in practice without diverging implementations. Until then, this PR will remain closed.

[wking]

Instead, he chose to argue about breaking hashes, which was a minor point.

I think pushing back against “Let's do this when sha256 is broken and not waste a second of time before that happens.” is a major point, but shrug. Skipping your other points in my initial response was part of an attempt to decrease verbosity, and then you asked me to address those too, so I did. And you don't seem to have found the very brief initial post particularly convincing either.

… the immediate closure of this issue was due to the fact that he did not respond to the feedback provided in my review comment…

I think the deduplication and portability issues (which I did address in my initial response) are the main issues with “divergence”. I'm fine mentioning those issues in the spec (as I said earlier), but I'm pretty sure not registering sha512 is the wrong way to go about addressing them.

[stevvooe]

@wking I acknowledge those points. If another contributor would like to take on this task, then they are welcome to address these issues.