[blimp] Change to use SHA-256 instead of SHA-1 for unique ids

[blimp] Change to use SHA-256 instead of SHA-1 for unique ids

Blimp currently uses SHA-1 to uniquely identify resources. There was
no specific reason for this other than the goal of having a unique
ID, so this changes it to instead use SHA-256 from //crypto, since it
is in general better. This does however change the length of the
hash from 20 bytes to 32 bytes.

This concern was raised as a drive-by comment on
https://codereview.chromium.org/1867653002/

This CL also adds functionality to verify that a specific BlobId has
the correct length, and removes all references to SHA1 outside of
the //blimp/common/blob_cache/id_util.[cc|h] files.

In addition, this CL adds a test to ensure that there is certainty
that this unique ID only ever changes intentionally.

BUG=597807, 600719, 603353
Committed: https://crrev.com/5344a6eebfbd65e6a86f2274c69e74ac6ccfe138
Cr-Commit-Position: refs/heads/master@{#389608}

[nyquist, 2016-04-22 22:02:18]

The CQ bit was checked by nyquist@chromium.org to run a CQ dry run

[nyquist, 2016-04-22 22:02:39]

nyquist@chromium.org changed reviewers:
+ davidben@chromium.org, wez@chromium.org

[nyquist, 2016-04-22 22:02:40]

wez, davidben: PTAL

[commit-bot: I haz the power, 2016-04-22 22:02:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1912153004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1912153004/1

[commit-bot: I haz the power, 2016-04-22 22:12:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-22 22:12:37]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Wez, 2016-04-23 00:08:53]

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
File blimp/common/blob_cache/id_util.h (right):

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util.h:17: // to have length crypto::kSHA256Length.
The length of the string is only interesting in terms of it being "valid" as per
the comments below; suggest changing this entire comment to "Returns a unique Id
for the Blob, based on its content." - the caller doesn't need to know the
scheme in-use, nor the length of the Ids, though the fact that they're all the
same length might be worthwhile mentioning.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util.h:21: // Returns a hexadecimal string
representation of a SHA-256 hash. The input is
"... of a BlobId."

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util.h:23: BLIMP_COMMON_EXPORT const std::string
FormatBlobId(const BlobId& id);
Suggest BlobIdToString rather than "format"; format doesn't specify what it's
formatted to, per-se.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util.h:26: BLIMP_COMMON_EXPORT bool
BlobIdHasCorrectLength(const BlobId& id);
Suggest naming this IsValidBlobId - then the FormatBlobId function can say
"input must be a valid BlobId" rather than specifying the length.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
File blimp/common/blob_cache/id_util_unittest.cc (right):

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util_unittest.cc:11: const char kInput[] =
"\xde\xad\xbe\xef\x4b\x1d\xc0\xd3\xff\xfe";
kBlobData?

nit: Blank line between namespace and the const char declaration, for
readability, innit.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util_unittest.cc:15: const char kExpected[] =
kBlobId and kBlobIdString, say?

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util_unittest.cc:21: class IdUtilTest : public
testing::Test {
Why are we overriding testing::Test if we're not actually going to add anything?

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util_unittest.cc:27: TEST_F(IdUtilTest,
VerifyHashCalculationCreatesExpectedHash) {
Suggest calling this just BlobIdIsCorrect or CorrectBlobId or Basic.

Consider also adding a test to verify that different Blobs have different Ids.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util_unittest.cc:35: TEST_F(IdUtilTest,
VerifyLengthChecker) {
This looks like two different tests; one is verifying that different Blobs' Ids
have the correct length, the other that the length-checker is correct - suggest
breaking into separate BlobIdsAreCorrectLength and BlobIdLengthCheck tests.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/in_...
File blimp/common/blob_cache/in_memory_blob_cache.cc (right):

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/in_...
blimp/common/blob_cache/in_memory_blob_cache.cc:9: #include
"base/strings/string_util.h"
Doesn't look like we use either of these headers either?

https://codereview.chromium.org/1912153004/diff/1/blimp/common/compositor/web...
File blimp/common/compositor/webp_decoder.cc (right):

https://codereview.chromium.org/1912153004/diff/1/blimp/common/compositor/web...
blimp/common/compositor/webp_decoder.cc:11: #include
"base/strings/string_util.h"
Do we use string_util.h in here?

https://codereview.chromium.org/1912153004/diff/1/blimp/engine/renderer/engin...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1912153004/diff/1/blimp/engine/renderer/engin...
blimp/engine/renderer/engine_image_serialization_processor.cc:15: #include
"base/strings/string_util.h"
Do we use string_util.h in here?

[nyquist, 2016-04-25 08:11:08]

The CQ bit was checked by nyquist@chromium.org to run a CQ dry run

[nyquist, 2016-04-25 08:11:17]

wez: PTAL

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
File blimp/common/blob_cache/id_util.h (right):

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util.h:17: // to have length crypto::kSHA256Length.
On 2016/04/23 00:08:52, Wez wrote:
> The length of the string is only interesting in terms of it being "valid" as
per
> the comments below; suggest changing this entire comment to "Returns a unique
Id
> for the Blob, based on its content." - the caller doesn't need to know the
> scheme in-use, nor the length of the Ids, though the fact that they're all the
> same length might be worthwhile mentioning.

Agreed! Done.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util.h:21: // Returns a hexadecimal string
representation of a SHA-256 hash. The input is
On 2016/04/23 00:08:52, Wez wrote:
> "... of a BlobId."

Done.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util.h:23: BLIMP_COMMON_EXPORT const std::string
FormatBlobId(const BlobId& id);
On 2016/04/23 00:08:52, Wez wrote:
> Suggest BlobIdToString rather than "format"; format doesn't specify what it's
> formatted to, per-se.

Done.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util.h:26: BLIMP_COMMON_EXPORT bool
BlobIdHasCorrectLength(const BlobId& id);
On 2016/04/23 00:08:52, Wez wrote:
> Suggest naming this IsValidBlobId - then the FormatBlobId function can say
> "input must be a valid BlobId" rather than specifying the length.

Awesome suggestion! Done.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
File blimp/common/blob_cache/id_util_unittest.cc (right):

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util_unittest.cc:11: const char kInput[] =
"\xde\xad\xbe\xef\x4b\x1d\xc0\xd3\xff\xfe";
On 2016/04/23 00:08:52, Wez wrote:
> kBlobData?
> 
> nit: Blank line between namespace and the const char declaration, for
> readability, innit.

Done.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util_unittest.cc:15: const char kExpected[] =
On 2016/04/23 00:08:52, Wez wrote:
> kBlobId and kBlobIdString, say?

Done.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util_unittest.cc:21: class IdUtilTest : public
testing::Test {
On 2016/04/23 00:08:52, Wez wrote:
> Why are we overriding testing::Test if we're not actually going to add
anything?

Done.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util_unittest.cc:27: TEST_F(IdUtilTest,
VerifyHashCalculationCreatesExpectedHash) {
On 2016/04/23 00:08:52, Wez wrote:
> Suggest calling this just BlobIdIsCorrect or CorrectBlobId or Basic.
> 
> Consider also adding a test to verify that different Blobs have different Ids.

Done.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/id_...
blimp/common/blob_cache/id_util_unittest.cc:35: TEST_F(IdUtilTest,
VerifyLengthChecker) {
On 2016/04/23 00:08:52, Wez wrote:
> This looks like two different tests; one is verifying that different Blobs'
Ids
> have the correct length, the other that the length-checker is correct -
suggest
> breaking into separate BlobIdsAreCorrectLength and BlobIdLengthCheck tests.

Done.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/in_...
File blimp/common/blob_cache/in_memory_blob_cache.cc (right):

https://codereview.chromium.org/1912153004/diff/1/blimp/common/blob_cache/in_...
blimp/common/blob_cache/in_memory_blob_cache.cc:9: #include
"base/strings/string_util.h"
On 2016/04/23 00:08:52, Wez wrote:
> Doesn't look like we use either of these headers either?

Done.

https://codereview.chromium.org/1912153004/diff/1/blimp/common/compositor/web...
File blimp/common/compositor/webp_decoder.cc (right):

https://codereview.chromium.org/1912153004/diff/1/blimp/common/compositor/web...
blimp/common/compositor/webp_decoder.cc:11: #include
"base/strings/string_util.h"
On 2016/04/23 00:08:52, Wez wrote:
> Do we use string_util.h in here?

Done.

https://codereview.chromium.org/1912153004/diff/1/blimp/engine/renderer/engin...
File blimp/engine/renderer/engine_image_serialization_processor.cc (right):

https://codereview.chromium.org/1912153004/diff/1/blimp/engine/renderer/engin...
blimp/engine/renderer/engine_image_serialization_processor.cc:15: #include
"base/strings/string_util.h"
On 2016/04/23 00:08:52, Wez wrote:
> Do we use string_util.h in here?

Done.

[commit-bot: I haz the power, 2016-04-25 08:11:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1912153004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1912153004/20001

[commit-bot: I haz the power, 2016-04-25 08:18:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-25 08:18:15]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Wez, 2016-04-25 18:43:02]

lgtm

[davidben, 2016-04-25 18:49:16]

lgtm

[nyquist, 2016-04-25 23:04:41]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2016-04-25 23:05:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1912153004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1912153004/20001

[commit-bot: I haz the power, 2016-04-25 23:29:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-25 23:32:09]

Description was changed from

==========
[blimp] Change to use SHA-256 instead of SHA-1 for unique ids

Blimp currently uses SHA-1 to uniquely identify resources. There was
no specific reason for this other than the goal of having a unique
ID, so this changes it to instead use SHA-256 from //crypto, since it
is in general better. This does however change the length of the
hash from 20 bytes to 32 bytes.

This concern was raised as a drive-by comment on
https://codereview.chromium.org/1867653002/

This CL also adds functionality to verify that a specific BlobId has
the correct length, and removes all references to SHA1 outside of
the //blimp/common/blob_cache/id_util.[cc|h] files.

In addition, this CL adds a test to ensure that there is certainty
that this unique ID only ever changes intentionally.

BUG=597807, 600719, 603353
==========

to

==========
[blimp] Change to use SHA-256 instead of SHA-1 for unique ids

Blimp currently uses SHA-1 to uniquely identify resources. There was
no specific reason for this other than the goal of having a unique
ID, so this changes it to instead use SHA-256 from //crypto, since it
is in general better. This does however change the length of the
hash from 20 bytes to 32 bytes.

This concern was raised as a drive-by comment on
https://codereview.chromium.org/1867653002/

This CL also adds functionality to verify that a specific BlobId has
the correct length, and removes all references to SHA1 outside of
the //blimp/common/blob_cache/id_util.[cc|h] files.

In addition, this CL adds a test to ensure that there is certainty
that this unique ID only ever changes intentionally.

BUG=597807, 600719, 603353

Committed: https://crrev.com/5344a6eebfbd65e6a86f2274c69e74ac6ccfe138
Cr-Commit-Position: refs/heads/master@{#389608}
==========

[commit-bot: I haz the power, 2016-04-25 23:32:11]

Patchset 2 (id:??) landed as
https://crrev.com/5344a6eebfbd65e6a86f2274c69e74ac6ccfe138
Cr-Commit-Position: refs/heads/master@{#389608}