Disable SHA-1 for Enterprise Certs

Disable SHA-1 for Enterprise Certs

Disable SHA-1 unless CertVerifier::VERIFY_ENABLE_SHA1_LOCAL_ANCHORS
is set. This flag is set based on the EnableSha1ForLocalAnchors
policy for Chrome users.

BUG=673036
Committed: https://crrev.com/d3d4039a2c95869ba38aa69b6bb542362e5f4574
Cr-Commit-Position: refs/heads/master@{#438399}

[Ryan Sleevi, 2016-12-09 23:07:39]

rsleevi@chromium.org changed reviewers:
+ eroman@chromium.org

[Ryan Sleevi, 2016-12-09 23:07:40]

Eric: Do you have time to take a look at this? I can always punt to Matt or
David if not

[Ryan Sleevi, 2016-12-09 23:07:50]

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

[commit-bot: I haz the power, 2016-12-09 23:08:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Sleevi, 2016-12-13 00:19:26]

rsleevi@chromium.org changed reviewers:
+ mattm@chromium.org
- eroman@chromium.org

[Ryan Sleevi, 2016-12-13 00:19:27]

Punting to Matt :)

[eroman, 2016-12-13 00:46:38]

eroman@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2016-12-13 00:46:38]

Oh hmm, i didn't even notice this.
Looking...

[mattm, 2016-12-13 01:17:17]

lgtm

[Ryan Sleevi, 2016-12-13 01:19:48]

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

[commit-bot: I haz the power, 2016-12-13 01:21:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2016-12-13 01:22:47]

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

https://codereview.chromium.org/2560343002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:482: // disabled on this date, but enterprises need
more time to transition.
Should this comment be updated?

https://codereview.chromium.org/2560343002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:490: // - ... unless it's in the intermediate and
SHA-1 intermediates are
This comment also seems incomplete now.

https://codereview.chromium.org/2560343002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:492: (!sha1_legacy_mode_enabled &&
This big nested if-statement is really hard to read. I had to do several
pass-throughs to gain confidence it does what it claims to do.

How about splitting this into smaller sections using short-circuits, and less
nesting? Say:

bool UsesWeakSignature(...) {
  if (verify_result->has_md5)
    return true;

  if (...)
    return true;

  ...

  return false;
}

Another point of confusion when reading this is the inconsistency between
"sha1_legacy_mode_enabled" being an affirmative boolean for sha1 being allowed,
whereas the per-request version, "disable_sha1" is in the opposite direction.

[Ryan Sleevi, 2016-12-13 01:29:57]

Just trying to understand more to figure out what can be done to clarity here,
since readability is the ultimate goal. I'm not sure I understand your concerns,
but please don't take the request for more feedback as a rejection of your
suggestion.

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

https://codereview.chromium.org/2560343002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:482: // disabled on this date, but enterprises need
more time to transition.
On 2016/12/13 01:22:47, eroman (slow) wrote:
> Should this comment be updated?

I'm not sure it should be, but I'm admittedly thinking in terms of "We wouldn't
even offer the flag" (it's scheduled for 2019 to be removed)

https://codereview.chromium.org/2560343002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:490: // - ... unless it's in the intermediate and
SHA-1 intermediates are
On 2016/12/13 01:22:47, eroman (slow) wrote:
> This comment also seems incomplete now.

Why does it seem incomplete?

It's consistent gramatically with 489, and logically with lines 494-495.

https://codereview.chromium.org/2560343002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:492: (!sha1_legacy_mode_enabled &&
On 2016/12/13 01:22:47, eroman (slow) wrote:
> This big nested if-statement is really hard to read. I had to do several
> pass-throughs to gain confidence it does what it claims to do.
> 
> How about splitting this into smaller sections using short-circuits, and less
> nesting? Say:

I explicitly wanted to avoid this. I'd be curious to better understand what
caused trouble, because I tried to match both the textual description and the
condition, and to holistically group all of the necessary conditions together
(legacy mode vs non-legacy mode)

> Another point of confusion when reading this is the inconsistency between
> "sha1_legacy_mode_enabled" being an affirmative boolean for sha1 being
allowed,
> whereas the per-request version, "disable_sha1" is in the opposite direction.

I'm not sure I fully grok this. Certainly, legacy_mode is temporary (it was
going to be removed in M57, but after the CT timebomb, I'm more inclined to wait
until 56 has fully hit stable before removing this), but I don't think having
two booleans is fundamentally at odds, nor is it necessary to have both their
default states equal the same conditional.

[eroman, 2016-12-13 01:49:33]

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

https://codereview.chromium.org/2560343002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:490: // - ... unless it's in the intermediate and
SHA-1 intermediates are
On 2016/12/13 01:29:57, Ryan Sleevi wrote:
> On 2016/12/13 01:22:47, eroman (slow) wrote:
> > This comment also seems incomplete now.
> 
> Why does it seem incomplete?
> 
> It's consistent gramatically with 489, and logically with lines 494-495.

It doesn't note the per-request override.

https://codereview.chromium.org/2560343002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:492: (!sha1_legacy_mode_enabled &&
On 2016/12/13 01:29:57, Ryan Sleevi wrote:
> On 2016/12/13 01:22:47, eroman (slow) wrote:
> > This big nested if-statement is really hard to read. I had to do several
> > pass-throughs to gain confidence it does what it claims to do.
> > 
> > How about splitting this into smaller sections using short-circuits, and
less
> > nesting? Say:
> 
> I explicitly wanted to avoid this. I'd be curious to better understand what
> caused trouble, because I tried to match both the textual description and the
> condition, and to holistically group all of the necessary conditions together
> (legacy mode vs non-legacy mode)
> 
> > Another point of confusion when reading this is the inconsistency between
> > "sha1_legacy_mode_enabled" being an affirmative boolean for sha1 being
> allowed,
> > whereas the per-request version, "disable_sha1" is in the opposite
direction.
> 
> I'm not sure I fully grok this. Certainly, legacy_mode is temporary (it was
> going to be removed in M57, but after the CT timebomb, I'm more inclined to
wait
> until 56 has fully hit stable before removing this), but I don't think having
> two booleans is fundamentally at odds, nor is it necessary to have both their
> default states equal the same conditional.

Similar concerns as hilghted in this article:
https://goto.google.com/nxqkx

Large nested expressions with parens, or, and, take more effort to visually
parse. Readers tend to rely on the whitespace indentation to decipher, which can
also be the source of bugs. Forget some parens around comment-delineated
subexpressions and the meaning changes.

If your goal is clarity I would definitely break this up.

[Ryan Sleevi, 2016-12-13 02:22:47]

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

https://codereview.chromium.org/2560343002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:492: (!sha1_legacy_mode_enabled &&
On 2016/12/13 01:49:33, eroman (slow) wrote:
> If your goal is clarity I would definitely break this up.

I guess this doesn't really help me understand why.

That is, there are quite a few ways to botch this conditional by simplifying,
and the current structure seems to me to be clearer because it describes the
three states, and it makes sure prose and code are kept in sync and close by. At
least judging by the CT policy bugs, this was a source of issues in the past -
by 'simplifying' the conditional, several CT policy bugs were introduced.

For example, upon re-reading this, there's a bug in that legacy mode (our
emergency fallback for any SHA-1 issues) would have fallen back to fully
disabling SHA-1 after 2016-01-01, rather than truly representing the 'legacy'
mode.

[Ryan Sleevi, 2016-12-13 02:22:53]

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

[commit-bot: I haz the power, 2016-12-13 02:23:43]

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, 2016-12-13 03:58:24]

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

[commit-bot: I haz the power, 2016-12-13 03:58:25]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-12-14 00:03:38]

Eric: Curious for your thoughts with the latest patch. Does that strike the
balance in readability?

[eroman, 2016-12-14 01:15:37]

LGTM

Yes thanks, that is more readable to me.

[Ryan Sleevi, 2016-12-14 01:35:04]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-12-14 01:35:05]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2560343002/#ps60001
(title: "Retweaked")

[commit-bot: I haz the power, 2016-12-14 01:35:35]

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, 2016-12-14 02:40:10]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481679304898450,
"parent_rev": "6bfb6be36344d0b40cf480eb232512a2a594b468", "commit_rev":
"64e1f9bafe332373f0742a6c6b80f057b651b9a2"}

[commit-bot: I haz the power, 2016-12-14 02:40:36]

Description was changed from

==========
Disable SHA-1 for Enterprise Certs

Disable SHA-1 unless CertVerifier::VERIFY_ENABLE_SHA1_LOCAL_ANCHORS
is set. This flag is set based on the EnableSha1ForLocalAnchors
policy for Chrome users.

BUG=673036
==========

to

==========
Disable SHA-1 for Enterprise Certs

Disable SHA-1 unless CertVerifier::VERIFY_ENABLE_SHA1_LOCAL_ANCHORS
is set. This flag is set based on the EnableSha1ForLocalAnchors
policy for Chrome users.

BUG=673036

Review-Url: https://codereview.chromium.org/2560343002
==========

[commit-bot: I haz the power, 2016-12-14 02:40:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-14 02:44:38]

Description was changed from

==========
Disable SHA-1 for Enterprise Certs

Disable SHA-1 unless CertVerifier::VERIFY_ENABLE_SHA1_LOCAL_ANCHORS
is set. This flag is set based on the EnableSha1ForLocalAnchors
policy for Chrome users.

BUG=673036

Review-Url: https://codereview.chromium.org/2560343002
==========

to

==========
Disable SHA-1 for Enterprise Certs

Disable SHA-1 unless CertVerifier::VERIFY_ENABLE_SHA1_LOCAL_ANCHORS
is set. This flag is set based on the EnableSha1ForLocalAnchors
policy for Chrome users.

BUG=673036

Committed: https://crrev.com/d3d4039a2c95869ba38aa69b6bb542362e5f4574
Cr-Commit-Position: refs/heads/master@{#438399}
==========

[commit-bot: I haz the power, 2016-12-14 02:44:39]

Patchset 4 (id:??) landed as
https://crrev.com/d3d4039a2c95869ba38aa69b6bb542362e5f4574
Cr-Commit-Position: refs/heads/master@{#438399}