Remove residual support for SHA-1 public key pins.

Remove residual support for SHA-1 public key pins.

BUG=564281
Review-Url: https://codereview.chromium.org/2951343002
Cr-Commit-Position: refs/heads/master@{#482473}
Committed: https://chromium.googlesource.com/chromium/src/+/08b2e4295cc88d1a306d75ca7d152b4b83516ffd

[palmer, 2017-06-22 21:08:14]

palmer@chromium.org changed reviewers:
+ davidben@chromium.org

[palmer, 2017-06-22 21:08:28]

Should be a quick and simple one. PTAL?

[palmer, 2017-06-22 21:08:36]

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

[commit-bot: I haz the power, 2017-06-22 21:09:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2017-06-22 21:33:47]

https://codereview.chromium.org/2951343002/diff/1/net/http/transport_security...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/2951343002/diff/1/net/http/transport_security...
net/http/transport_security_state.cc:155: continue;
This is just removing a bit in the reporting side of things, right? It looks
like we'd still enforce any SHA-1 pins we have lying around in the user's
profile directory; there's still residual code in HashValue::FromString (used by
the parser) and CertVerifyProc continues to produce SHA-1 hashes.

We probably want to unwind all of that (which I'm all for doing) before taking
this piece out. I bet we can even take HASH_VALUE_SHA1 out altogether. I see
net/cert/internal/parse_ocsp.cc uses it, but not in any useful away as far as I
can tell. (I can unwind that part if you like.)

[commit-bot: I haz the power, 2017-06-22 21:35:48]

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

[commit-bot: I haz the power, 2017-06-22 21:35:49]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[davidben, 2017-06-22 21:51:43]

https://codereview.chromium.org/2951343002/diff/1/net/http/transport_security...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/2951343002/diff/1/net/http/transport_security...
net/http/transport_security_state.cc:155: continue;
On 2017/06/22 21:33:47, davidben wrote:
> This is just removing a bit in the reporting side of things, right? It looks
> like we'd still enforce any SHA-1 pins we have lying around in the user's
> profile directory; there's still residual code in HashValue::FromString (used
by
> the parser) and CertVerifyProc continues to produce SHA-1 hashes.
> 
> We probably want to unwind all of that (which I'm all for doing) before taking
> this piece out. I bet we can even take HASH_VALUE_SHA1 out altogether. I see
> net/cert/internal/parse_ocsp.cc uses it, but not in any useful away as far as
I
> can tell. (I can unwind that part if you like.)

Here's taking it out of parse_ocsp.cc:
https://chromium-review.googlesource.com/545017

[palmer, 2017-06-23 00:57:59]

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

[commit-bot: I haz the power, 2017-06-23 00:58:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[palmer, 2017-06-23 00:59:24]

There's 1 last HASH_VALUE_SHA1 usage site: The name-constrained CAs list. Would
you rather I migrate that to SHA-256 now, or in a follow-up CL?

[commit-bot: I haz the power, 2017-06-23 01:29:20]

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

[commit-bot: I haz the power, 2017-06-23 01:29:21]

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

[palmer, 2017-06-23 19:42:54]

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

[palmer, 2017-06-23 19:43:08]

> There's 1 last HASH_VALUE_SHA1 usage site: The name-constrained CAs list.
Would you rather I migrate that to SHA-256 now, or in a follow-up CL?

I did it in patchset 4.

[commit-bot: I haz the power, 2017-06-23 19:43:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 20:58:23]

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

[commit-bot: I haz the power, 2017-06-23 20:58:24]

Dry run: This issue passed the CQ dry run.

[davidben, 2017-06-26 20:15:56]

https://codereview.chromium.org/2951343002/diff/60001/net/base/hash_value.cc
File net/base/hash_value.cc (right):

https://codereview.chromium.org/2951343002/diff/60001/net/base/hash_value.cc#...
net/base/hash_value.cc:61: return std::string("sha1/") + base64_str;
Out of curiosity, what are the remaining HASH_VALUE_SHA1 uses? I think it's fine
to leave it for now, but I'm curious what's left. Perhaps we shouldn't have had
HPKP's tagged hash type be shared with random other parts of the codebase. Ah
well. :-/ Apologies for the scope creep in the CL. It probably should have
occurred to me that would have been kind of involved.

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc.cc (left):

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:802: 0xae, 0x27, 0x1a, 0x6d, 0x94, 0xf4, 0x14,
0xd1, 0xa8, 0x73},
(confirmed the file matches)

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:809: 0xcf, 0x88, 0x3f, 0x1e, 0xc0, 0x58, 0x27,
0xd8, 0xb8, 0xe4},
(confirmed the file matches)

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:816: 0x52, 0x56, 0xed, 0x07, 0xa8, 0x63, 0xf2,
0xdb, 0x1c, 0xdf},
(confirmed the file matches)

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:823: 0x5a, 0x0a, 0x42, 0xb8, 0xc5, 0xcf, 0x6d,
0xb3, 0x57, 0xe1},
(confirmed the file matches)

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:805: 0x27, 0xbe, 0xfb, 0x2c, 0x4f, 0x4b, 0x04,
0xd0, 0x44, 0x96},
(confirmed)

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:815: 0xcf, 0xd3, 0x98, 0xc3, 0xac, 0x1f, 0x0d,
0xbb, 0x75, 0x4b},
(confirmed)

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:825: 0x8f, 0x6b, 0xb2, 0x10, 0xe1, 0x23, 0xfd,
0x07, 0x57, 0x93},
(confirmed)

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:835: 0xe6, 0x6a, 0x4b, 0xa7, 0x74, 0x46, 0xee,
0x2b, 0xd1, 0xf7},
(confirmed)

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_builtin.cc (right):

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:92: // Appends the SHA1 and SHA256 hashes
of |spki_bytes| to |*hashes|.
Nit: No longer accurate.

https://codereview.chromium.org/2951343002/diff/60001/net/data/ssl/blacklist/...
File net/data/ssl/blacklist/README.md (right):

https://codereview.chromium.org/2951343002/diff/60001/net/data/ssl/blacklist/...
net/data/ssl/blacklist/README.md:77: *
[f375e2f77a108bacc4234894a9af308edeca1acd8fbde0e7aaa9634e9daf7e1c.pem](f375e2f77a108bacc4234894a9af308edeca1acd8fbde0e7aaa9634e9daf7e1c.pem)
So currently everything in this directory corresponds to an entry in
cert_verify_proc_blacklist.inc (although I don't see a script to generate it
anywhere...). These aren't blacklisted but name-constrained. Previously the
wosign certs were in a separate net/data/ssl/wosign directory and then folded in
here when they converted to a straight-up blacklist.

Perhaps move these to net/data/ssl/name_constrained/?

https://codereview.chromium.org/2951343002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/2951343002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.cc:160: continue;
Might be better to leave it case HASH_VALUE_SHA1 (at least until that enum value
is taken out altogether) with a NOTREACHED() + continue. That we can keep the
compiler warning us if we forget a case in the future.

[davidben, 2017-06-26 20:17:00]

https://codereview.chromium.org/2951343002/diff/60001/net/base/hash_value.cc
File net/base/hash_value.cc (right):

https://codereview.chromium.org/2951343002/diff/60001/net/base/hash_value.cc#...
net/base/hash_value.cc:61: return std::string("sha1/") + base64_str;
On 2017/06/26 20:15:55, davidben wrote:
> Out of curiosity, what are the remaining HASH_VALUE_SHA1 uses? I think it's
fine
> to leave it for now, but I'm curious what's left. Perhaps we shouldn't have
had
> HPKP's tagged hash type be shared with random other parts of the codebase. Ah
> well. :-/ Apologies for the scope creep in the CL. It probably should have
> occurred to me that would have been kind of involved.

(Hrm. Codesearch seems to think there actually isn't anything left.)

[palmer, 2017-06-26 21:33:00]

https://codereview.chromium.org/2951343002/diff/1/net/http/transport_security...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/2951343002/diff/1/net/http/transport_security...
net/http/transport_security_state.cc:155: continue;
On 2017/06/22 at 21:33:47, davidben wrote:
> This is just removing a bit in the reporting side of things, right? It looks
like we'd still enforce any SHA-1 pins we have lying around in the user's
profile directory; there's still residual code in HashValue::FromString (used by
the parser) and CertVerifyProc continues to produce SHA-1 hashes.
> 
> We probably want to unwind all of that (which I'm all for doing) before taking
this piece out. I bet we can even take HASH_VALUE_SHA1 out altogether. I see
net/cert/internal/parse_ocsp.cc uses it, but not in any useful away as far as I
can tell. (I can unwind that part if you like.)

Oh, OK, great. I didn't know if you'd want me to remove it from CertVerifyProc,
too; I saw it but hesitated to touch it. I'll put up another patchset with that
removed.

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_builtin.cc (right):

https://codereview.chromium.org/2951343002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_builtin.cc:92: // Appends the SHA1 and SHA256 hashes
of |spki_bytes| to |*hashes|.
> Nit: No longer accurate.

Fixed.

https://codereview.chromium.org/2951343002/diff/60001/net/data/ssl/blacklist/...
File net/data/ssl/blacklist/README.md (right):

https://codereview.chromium.org/2951343002/diff/60001/net/data/ssl/blacklist/...
net/data/ssl/blacklist/README.md:77: *
[f375e2f77a108bacc4234894a9af308edeca1acd8fbde0e7aaa9634e9daf7e1c.pem](f375e2f77a108bacc4234894a9af308edeca1acd8fbde0e7aaa9634e9daf7e1c.pem)
> So currently everything in this directory corresponds to an entry in
cert_verify_proc_blacklist.inc (although I don't see a script to generate it
anywhere...). These aren't blacklisted but name-constrained. Previously the
wosign certs were in a separate net/data/ssl/wosign directory and then folded in
here when they converted to a straight-up blacklist.
> 
> Perhaps move these to net/data/ssl/name_constrained/?

Done.

https://codereview.chromium.org/2951343002/diff/60001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/2951343002/diff/60001/net/http/transport_secu...
net/http/transport_security_state.cc:160: continue;
> Might be better to leave it case HASH_VALUE_SHA1 (at least until that enum
value is taken out altogether) with a NOTREACHED() + continue. That we can keep
the compiler warning us if we forget a case in the future.

I managed to get rid of all HASH_VALUE_SHA1, so I think we're good here now.

[davidben, 2017-06-26 21:36:43]

lgtm. Yay removing code!! :-D

https://codereview.chromium.org/2951343002/diff/80001/net/data/ssl/name_const...
File net/data/ssl/name_constrained/README.md (right):

https://codereview.chromium.org/2951343002/diff/80001/net/data/ssl/name_const...
net/data/ssl/name_constrained/README.md:4: treated as named-constrained during
certiicate validation within Chromium-based
certificate

[palmer, 2017-06-26 21:47:09]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2017-06-26 21:47:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 23:58:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498513629223540,
"parent_rev": "66193f75971bb58ae02aac4cb44f3cdf72479bd4", "commit_rev":
"08b2e4295cc88d1a306d75ca7d152b4b83516ffd"}

[commit-bot: I haz the power, 2017-06-26 23:58:26]

Description was changed from

==========
Remove residual support for SHA-1 public key pins.

BUG=564281
==========

to

==========
Remove residual support for SHA-1 public key pins.

BUG=564281

Review-Url: https://codereview.chromium.org/2951343002
Cr-Commit-Position: refs/heads/master@{#482473}
Committed:
https://chromium.googlesource.com/chromium/src/+/08b2e4295cc88d1a306d75ca7d15...
==========

[commit-bot: I haz the power, 2017-06-26 23:58:28]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/08b2e4295cc88d1a306d75ca7d15...