Add policies support to VerifyCertificateChain().

Add policies support to VerifyCertificateChain().

Support is compliant with RFC 5280 and supports all the policy
extensions specified therein:

 * Inhibit Any Policy
 * Policy Constraints
 * Policies
 * Policy Mappings

Testing is done solely using the PKITS test suite, which has fairly good
coverage of these extensions:

  4.8 (Certificate Policies)
  4.9 (Require Explicit Policy)
  4.10 (Policy Mappings)
  4.11 (Inhibit Policy Mapping)
  4.12 (Inhibit Any Policy)

BUG=634456, 634453, 634452
Review-Url: https://codereview.chromium.org/2903283002
Cr-Commit-Position: refs/heads/master@{#476513}
Committed: https://chromium.googlesource.com/chromium/src/+/bcca0368ac116828cecc6c68a381bfc5147f8c98

[eroman, 2017-05-25 18:41:46]

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

[commit-bot: I haz the power, 2017-05-25 18:42:46]

Dry run: CQ is trying da patch.

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

[eroman, 2017-05-25 18:43:36]

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

[commit-bot: I haz the power, 2017-05-25 20:10:24]

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

[commit-bot: I haz the power, 2017-05-25 20:10:24]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-05-25 23:21:49]

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:431: //      certificate is
self-issued, then:
paste more RFC instead of leaving the cliffhanger "then: ..."?

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:440: valid_policies_ =
std::move(new_valid_policies);
so is step (3) unnecessary because we replace valid_policies_ with
new_valid_policies instead of keeping the whole policy tree and updating in
place? Maybe add a comment about that?

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:461: //      (a)  If a policy
mappings extension is present, verify that the
Preceed this with what RFC section you're referencing now

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:467:
errors->AddError(kPolicyMappingAnyPolicy);
Is it intentionally not returning here for the "try to accumulate all errors"
thing?
(As the rfc says it should terminate if this fails)

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:494: //                  
equivalent to ID-P by the policy mappings extension.
Man the policy mapping stuff is impossible to comprehend. It doesn't seem like
this is done though?

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:496: //            (ii)   If there
is a node in the valid_policy_tree of depth
comment out of place? (this is b.2.ii?)

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:509: mapping.issuer_domain_policy
== AnyPolicy()) {
so this is an extrapolation for what to do when continuing through the error
case of having AnyPolicy as an issuerDomainPolicy?
or was that supposed to be orig_policy == AnyPolicy()?

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:828: // constraints.
should these todos be done?

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
File net/cert/internal/verify_certificate_chain.h (right):

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.h:172: //  * Policy Mappings:      
No
yes?

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.h:187: bool
initial_any_policy_inhibit,
maybe use enums instead of passing a bunch of bools?

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.h:188: CertPathErrors* errors);
Add a todo for outputting the final valid_policy_tree?

[eroman, 2017-05-26 18:50:00]

Thanks for the feedback!

Before I respond to all the comments, I am going to try and modify the CL so
that it also outputs "user-constrained-policy-set".

In the process I hope to address an issue with the current policy mapping
handlings where a mapping is lost if the certificate doesn't also assert that
policy.

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
File net/cert/internal/verify_certificate_chain.h (right):

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.h:172: //  * Policy Mappings:      
No
On 2017/05/25 23:21:49, mattm wrote:
> yes?

The "No" is because it is not specified in rfc 5937. I have been trying to match
5937's notion of trust anchor constraints here, which in essence just does a
preprocessing step and changes the inputs to RFC 5937.

I have some ideas on improving this, but don't want to do so in this (which is
why I left the the others as "Not currently, TODO").

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.h:187: bool
initial_any_policy_inhibit,
On 2017/05/25 23:21:49, mattm wrote:
> maybe use enums instead of passing a bunch of bools?

sgtm

https://codereview.chromium.org/2903283002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.h:188: CertPathErrors* errors);
On 2017/05/25 23:21:49, mattm wrote:
> Add a todo for outputting the final valid_policy_tree?

Good idea.

I will go ahead and add "user-constrained-policy-set" as an output which will be
useful in some other CLs.
I don't think valid_policy_tree itself is worth outputting (or calculating for
that matter).
Let me see how well just the set works.

[eroman, 2017-05-30 06:56:04]

PTAL.

 * Updated it to include support for "user_constrained_policy_set"
 * More closely match the procedure given in RFC 5280

[eroman, 2017-05-30 06:56:11]

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

[commit-bot: I haz the power, 2017-05-30 06:56:28]

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-05-30 08:02:55]

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

[commit-bot: I haz the power, 2017-05-30 08:02:56]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-05-30 18:31:17]

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

[commit-bot: I haz the power, 2017-05-30 18:32:09]

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-05-30 20:04:02]

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

[commit-bot: I haz the power, 2017-05-30 20:04:03]

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...)

[mattm, 2017-05-31 23:03:34]

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:243: // ValidPolicyTree differs
slightly from RFC 5280's description in that:
do you think the PKITS tests are comprehensive enough to be confident this
alternative implementation is equivalent to the 5280 version, from a black box
perspective?

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:248: //  (b) It only stores the
most recent level of the policy tree rather than
nit: should be (2), or the point above should be (a) ?

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:312: void
GetValidPolicySet(std::set<der::Input>* policy_set) {
I think the method name could be clearer about what it does.. at least from the
name alone I would expect it to be looking at valid_policy values, not
root_policy.

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:345: ((parent.root_policy ==
AnyPolicy()) && (policy_oid != AnyPolicy()))
does the  && (policy_oid != AnyPolicy()) here do anything? Seems like the result
would be the same in all cases without it.

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain_unittest.cc (right):

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain_unittest.cc:26:
test.initial_any_policy_inhibit, nullptr, &errors);
/* argument name comment */

[eroman, 2017-06-01 00:46:34]

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

[eroman, 2017-06-01 00:46:36]

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:243: // ValidPolicyTree differs
slightly from RFC 5280's description in that:
On 2017/05/31 23:03:33, mattm wrote:
> do you think the PKITS tests are comprehensive enough to be confident this
> alternative implementation is equivalent to the 5280 version, from a black box
> perspective?

Good question...

I think the PKITS tests are pretty comprehensive when it comes to policies. I
will give them another read through and see if there is any coverage I think is
missing.

Are there some changes I can make to the code that would help to address this
concern?

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:248: //  (b) It only stores the
most recent level of the policy tree rather than
On 2017/05/31 23:03:33, mattm wrote:
> nit: should be (2), or the point above should be (a) ?

Done.

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:312: void
GetValidPolicySet(std::set<der::Input>* policy_set) {
On 2017/05/31 23:03:33, mattm wrote:
> I think the method name could be clearer about what it does.. at least from
the
> name alone I would expect it to be looking at valid_policy values, not
> root_policy.

Good point. Renamed to GetValidRootPolicySet() and added some better
documentation.

However, another option I am thinking now may be better given your concerns on
explainability/equivalence, is to just rename this to
GetAuthoritiesConstrainedPolicySet(), and remove my two biggest deviations in
the process:

 (a) initializing valid policy tree to user-initial-policy-set rather than
anyPolicy
 (b) Not doing intersection as part of WrapUp()

WDYT?

I'll go ahead and prepare this option as a new patchset.

The main reason I initially went with the approach in the current patchset, was
a desire to:
  (1) Avoid unnecessary computation of policies by intersecting from the start
  (2) Attribute policy errors to the first certificate that violates them
(rather than attributing them to the target cert).

But thinking about it now, those are probably less important than explainability

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:345: ((parent.root_policy ==
AnyPolicy()) && (policy_oid != AnyPolicy()))
On 2017/05/31 23:03:33, mattm wrote:
> does the  && (policy_oid != AnyPolicy()) here do anything? Seems like the
result
> would be the same in all cases without it.

You are correct. Done.

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain_unittest.cc (right):

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain_unittest.cc:26:
test.initial_any_policy_inhibit, nullptr, &errors);
On 2017/05/31 23:03:34, mattm wrote:
> /* argument name comment */

Done.

[commit-bot: I haz the power, 2017-06-01 00:47:20]

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-01 01:21:32]

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

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

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_...)

[eroman, 2017-06-01 19:26:13]

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

[commit-bot: I haz the power, 2017-06-01 19:27:26]

Dry run: CQ is trying da patch.

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

[eroman, 2017-06-01 19:28:25]

PTAL

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
File net/cert/internal/verify_certificate_chain.cc (right):

https://codereview.chromium.org/2903283002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:312: void
GetValidPolicySet(std::set<der::Input>* policy_set) {
On 2017/06/01 00:46:36, eroman wrote:
> On 2017/05/31 23:03:33, mattm wrote:
> > I think the method name could be clearer about what it does.. at least from
> the
> > name alone I would expect it to be looking at valid_policy values, not
> > root_policy.
> 
> Good point. Renamed to GetValidRootPolicySet() and added some better
> documentation.
> 
> However, another option I am thinking now may be better given your concerns on
> explainability/equivalence, is to just rename this to
> GetAuthoritiesConstrainedPolicySet(), and remove my two biggest deviations in
> the process:
> 
>  (a) initializing valid policy tree to user-initial-policy-set rather than
> anyPolicy
>  (b) Not doing intersection as part of WrapUp()
> 
> WDYT?
> 
> I'll go ahead and prepare this option as a new patchset.
> 
> The main reason I initially went with the approach in the current patchset,
was
> a desire to:
>   (1) Avoid unnecessary computation of policies by intersecting from the start
>   (2) Attribute policy errors to the first certificate that violates them
> (rather than attributing them to the target cert).
> 
> But thinking about it now, those are probably less important than
explainability

I tried this out in Patchset 6 --> 7, but found it to be worse.
So I have restored the earlier approach, but improved the comments.

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

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

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

Dry run: This issue passed the CQ dry run.

[mattm, 2017-06-02 01:11:42]

lgtm

[eroman, 2017-06-02 01:22:07]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2017-06-02 01:22:38]

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-02 01:27:09]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496366527634050,
"parent_rev": "0bf4bd3f8a3111af1a7194607248a3649a1b7baf", "commit_rev":
"bcca0368ac116828cecc6c68a381bfc5147f8c98"}

[commit-bot: I haz the power, 2017-06-02 01:27:17]

Description was changed from

==========
Add policies support to VerifyCertificateChain().

Support is compliant with RFC 5280 and supports all the policy
extensions specified therein:

 * Inhibit Any Policy
 * Policy Constraints
 * Policies
 * Policy Mappings

Testing is done solely using the PKITS test suite, which has fairly good
coverage of these extensions:

  4.8 (Certificate Policies)
  4.9 (Require Explicit Policy)
  4.10 (Policy Mappings)
  4.11 (Inhibit Policy Mapping)
  4.12 (Inhibit Any Policy)

BUG=634456,634453,634452
==========

to

==========
Add policies support to VerifyCertificateChain().

Support is compliant with RFC 5280 and supports all the policy
extensions specified therein:

 * Inhibit Any Policy
 * Policy Constraints
 * Policies
 * Policy Mappings

Testing is done solely using the PKITS test suite, which has fairly good
coverage of these extensions:

  4.8 (Certificate Policies)
  4.9 (Require Explicit Policy)
  4.10 (Policy Mappings)
  4.11 (Inhibit Policy Mapping)
  4.12 (Inhibit Any Policy)

BUG=634456,634453,634452

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

[commit-bot: I haz the power, 2017-06-02 01:27:20]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/bcca0368ac116828cecc6c68a381...

[yoichio, 2017-06-02 04:29:11]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2918063002/ by yoichio@chromium.org.

The reason for reverting is: This patch causes VerifyCertificateChain test
failure on linux:
https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C....