Refactor the assignment of CertVerifyResult::has_md2, etc.

Refactor the assignment of CertVerifyResult::has_md2, etc.

This allows unconditionally enabling the tests in
cert_verify_proc_unittest.cc,

Previously the assignment of weak hash algorithms was done by each
CertVerifyProc::VerifyInternal() implementation, whereas now it is
done internally by CertVerifyProc::Verify() after VerifyInternal() has
run.

The downside to this approach is that at this layer there is ambiguity
as to which certificates are trusted and hence should be skipped for
determining if the chain contains weak hash algorithms.

This ambiguity results in some  differences in the reporting of
"has_md2", "has_md4", "has_md5", "hash_sha1", "has_sha1_leaf" when
verification has failed (The final intermediate is assumed to be the
trust anchor and is skipped).

BUG=649017
Review-Url: https://codereview.chromium.org/2627523002
Cr-Commit-Position: refs/heads/master@{#442522}
Committed: https://chromium.googlesource.com/chromium/src/+/accb81312b4555356dd49253d156e4a8b9eac784

[eroman, 2017-01-10 01:26:26]

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

[commit-bot: I haz the power, 2017-01-10 01:27:56]

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

[eroman, 2017-01-10 01:31:14]

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

[Ryan Sleevi, 2017-01-10 01:36:57]

Description was changed from

==========
Refactor the assignment of CertVerifyResult::has_md2, etc.

This allows unconditionally enabling the tests in cert_verify_proc_unittest.cc,

Previously the assignment of weak hash algorithms was done by each
CertVerifyProc::VerifyInternal() implementation, whereas now it is done
internally by CertVerifyProc::Verify() after VerifyInternal() has run.

The downside to this approach is that at this layer there is ambiguity as to
which certificates are trusted and hence should be skipped for determining if
the chain contains weak hash algorithms.

This ambiguity results in some  differences in the reporting of "has_md2",
"has_md4", "has_md5", "hash_sha1", "has_sha1_leaf" when verification has failed
(The final intermediate is assumed to be the trust anchor and is skipped).

BUG=649017
==========

to

==========
Refactor the assignment of CertVerifyResult::has_md2, etc.

This allows unconditionally enabling the tests in
cert_verify_proc_unittest.cc,

Previously the assignment of weak hash algorithms was done by each
CertVerifyProc::VerifyInternal() implementation, whereas now it is
done internally by CertVerifyProc::Verify() after VerifyInternal() has
run.

The downside to this approach is that at this layer there is ambiguity
as to which certificates are trusted and hence should be skipped for
determining if the chain contains weak hash algorithms.

This ambiguity results in some  differences in the reporting of
"has_md2", "has_md4", "has_md5", "hash_sha1", "has_sha1_leaf" when
verification has failed (The final intermediate is assumed to be the
trust anchor and is skipped).

BUG=649017
==========

[Ryan Sleevi, 2017-01-10 01:53:24]

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc.cc
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:375: bool is_leaf,
Two suggested changes:
1) It seems like using the X509Certificate::SignatureHashAlgorithm for this
helper function may reduce the "What this function does". It's a minor quibble
though.
2) You can remove the is_leaf parameter with the simplification below.

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:401: void CheckWeakHashAlgorithms(CertVerifyResult*
verify_result) {
Naming wise, I think CheckWeakHashAlgorithms and
FillCertVerifyResultHashAlgorithms are probably... overly long and unfortunately
not as descriptive.

To me, "check" implies some conditional behaviour, but instead, it's populating.
FillCertVerifyResultHashAlgorithms is, well, a mouthful :)

ComputeSignatureAlgorithms(CertVerifyResult* verify_result);
MapCertHashAlgorithm ?


I suppose one dimension here is I have never really thought of the "has_*" as
the "weak signature fields" so much as, well, just details about the signature.

I would say this is largely a minor nit, but the wording feels pretty stilted.

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:425: if (!intermediates.empty()) {
Does it make sense to make this error-short circuit more explicit

// Explanation of why this short-circuit
if (intermediates.empty())
  return;

FillCertVerifyResultWeakHashAlgorithms(verify_result->verified_cert->os_cert_handle(),
verify_result);
verify_result->has_sha1_leaf = verify_result->has_sha1;

for (const auto& os_cert : intermediates) {
  FillCertVerifyResultWEakHashAlgorithms(os_cert, verify_result);
}

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:489: // subsequent code can rely on it being
meaningful.
Seems like an unnecessary comment (or perhaps unnecessarily verbose)

// Fill in the verify_result->has_* booleans
...

Namely, it should be obvious "so that subsequent code can rely on it being
meaningful" is implied from position. I would even go as far as suggesting no
comment needed :)

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc_u...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc_u...
net/cert/cert_verify_proc_unittest.cc:1743: proc->Verify(ee_chain.get(),
"127.0.0.1", std::string(), flags, NULL,
nullptr?

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc_u...
net/cert/cert_verify_proc_unittest.cc:1796: // know whether the final
intermediate is a trust anchor or not.
This comment doesn't seem entirely accurate - incomplete chains do report the
status of the intermediate, just not the *last* intermediate.

You sort of capture this in 1812-1815

[commit-bot: I haz the power, 2017-01-10 01:58:40]

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

[commit-bot: I haz the power, 2017-01-10 01:58:40]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[eroman, 2017-01-10 02:48:33]

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

[eroman, 2017-01-10 02:48:46]

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc.cc
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:375: bool is_leaf,
On 2017/01/10 01:53:24, Ryan Sleevi wrote:
> Two suggested changes:
> 1) It seems like using the X509Certificate::SignatureHashAlgorithm for this
> helper function may reduce the "What this function does". It's a minor quibble
> though.
> 2) You can remove the is_leaf parameter with the simplification below.

Done.

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:401: void CheckWeakHashAlgorithms(CertVerifyResult*
verify_result) {
On 2017/01/10 01:53:24, Ryan Sleevi wrote:
> Naming wise, I think CheckWeakHashAlgorithms and
> FillCertVerifyResultHashAlgorithms are probably... overly long and
unfortunately
> not as descriptive.
> 
> To me, "check" implies some conditional behaviour, but instead, it's
populating.
> FillCertVerifyResultHashAlgorithms is, well, a mouthful :)
> 
> ComputeSignatureAlgorithms(CertVerifyResult* verify_result);
> MapCertHashAlgorithm ?
> 
> 
> I suppose one dimension here is I have never really thought of the "has_*" as
> the "weak signature fields" so much as, well, just details about the
signature.
> 
> I would say this is largely a minor nit, but the wording feels pretty stilted.

Done.

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:425: if (!intermediates.empty()) {
On 2017/01/10 01:53:24, Ryan Sleevi wrote:
> Does it make sense to make this error-short circuit more explicit
> 
> // Explanation of why this short-circuit
> if (intermediates.empty())
>   return;
> 
>
FillCertVerifyResultWeakHashAlgorithms(verify_result->verified_cert->os_cert_handle(),
> verify_result);
> verify_result->has_sha1_leaf = verify_result->has_sha1;
> 
> for (const auto& os_cert : intermediates) {
>   FillCertVerifyResultWEakHashAlgorithms(os_cert, verify_result);
> }

Done.

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc.c...
net/cert/cert_verify_proc.cc:489: // subsequent code can rely on it being
meaningful.
On 2017/01/10 01:53:24, Ryan Sleevi wrote:
> Seems like an unnecessary comment (or perhaps unnecessarily verbose)
> 
> // Fill in the verify_result->has_* booleans
> ...
> 
> Namely, it should be obvious "so that subsequent code can rely on it being
> meaningful" is implied from position. I would even go as far as suggesting no
> comment needed :)

Done.

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc_u...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc_u...
net/cert/cert_verify_proc_unittest.cc:1743: proc->Verify(ee_chain.get(),
"127.0.0.1", std::string(), flags, NULL,
On 2017/01/10 01:53:24, Ryan Sleevi wrote:
> nullptr?

Done.

https://codereview.chromium.org/2627523002/diff/1/net/cert/cert_verify_proc_u...
net/cert/cert_verify_proc_unittest.cc:1796: // know whether the final
intermediate is a trust anchor or not.
On 2017/01/10 01:53:24, Ryan Sleevi wrote:
> This comment doesn't seem entirely accurate - incomplete chains do report the
> status of the intermediate, just not the *last* intermediate.
> 
> You sort of capture this in 1812-1815

Done.

[commit-bot: I haz the power, 2017-01-10 02:49:17]

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

[eroman, 2017-01-10 02:53:10]

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

[commit-bot: I haz the power, 2017-01-10 02:53:53]

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

[Ryan Sleevi, 2017-01-10 03:00:42]

LGTM, great cleanup overall, one supppper tiny nit

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

https://codereview.chromium.org/2627523002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:413: // If there are no intermediates, then the
leaf is trusted.
"then the leaf is trusted or verification failed."

[eroman, 2017-01-10 03:06:12]

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

https://codereview.chromium.org/2627523002/diff/60001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:413: // If there are no intermediates, then the
leaf is trusted.
On 2017/01/10 03:00:41, Ryan Sleevi wrote:
> "then the leaf is trusted or verification failed."

Done.

[eroman, 2017-01-10 03:06:36]

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

[commit-bot: I haz the power, 2017-01-10 03:07:00]

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-01-10 03:30:57]

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

[commit-bot: I haz the power, 2017-01-10 03:30:58]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[eroman, 2017-01-10 04:24:31]

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

[commit-bot: I haz the power, 2017-01-10 04:25:30]

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-01-10 06:18:54]

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

[commit-bot: I haz the power, 2017-01-10 06:18:54]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-01-10 07:09:09]

The CQ bit was checked by eroman@chromium.org

[eroman, 2017-01-10 07:09:10]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2627523002/#ps100001
(title: "fix PrintTo()")

[commit-bot: I haz the power, 2017-01-10 07:09:27]

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-01-10 07:13:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484032149769470,
"parent_rev": "b7b01ac5e2085ad23f1a01b84b2ce27daced0f70", "commit_rev":
"accb81312b4555356dd49253d156e4a8b9eac784"}

[commit-bot: I haz the power, 2017-01-10 07:13:44]

Description was changed from

==========
Refactor the assignment of CertVerifyResult::has_md2, etc.

This allows unconditionally enabling the tests in
cert_verify_proc_unittest.cc,

Previously the assignment of weak hash algorithms was done by each
CertVerifyProc::VerifyInternal() implementation, whereas now it is
done internally by CertVerifyProc::Verify() after VerifyInternal() has
run.

The downside to this approach is that at this layer there is ambiguity
as to which certificates are trusted and hence should be skipped for
determining if the chain contains weak hash algorithms.

This ambiguity results in some  differences in the reporting of
"has_md2", "has_md4", "has_md5", "hash_sha1", "has_sha1_leaf" when
verification has failed (The final intermediate is assumed to be the
trust anchor and is skipped).

BUG=649017
==========

to

==========
Refactor the assignment of CertVerifyResult::has_md2, etc.

This allows unconditionally enabling the tests in
cert_verify_proc_unittest.cc,

Previously the assignment of weak hash algorithms was done by each
CertVerifyProc::VerifyInternal() implementation, whereas now it is
done internally by CertVerifyProc::Verify() after VerifyInternal() has
run.

The downside to this approach is that at this layer there is ambiguity
as to which certificates are trusted and hence should be skipped for
determining if the chain contains weak hash algorithms.

This ambiguity results in some  differences in the reporting of
"has_md2", "has_md4", "has_md5", "hash_sha1", "has_sha1_leaf" when
verification has failed (The final intermediate is assumed to be the
trust anchor and is skipped).

BUG=649017

Review-Url: https://codereview.chromium.org/2627523002
Cr-Commit-Position: refs/heads/master@{#442522}
Committed:
https://chromium.googlesource.com/chromium/src/+/accb81312b4555356dd49253d156...
==========

[commit-bot: I haz the power, 2017-01-10 07:13:50]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/accb81312b4555356dd49253d156...