Add errors per ResultPath for CertPathBuilder.

Add errors per ResultPath for CertPathBuilder.

BUG=634443
Committed: https://crrev.com/9433a3cc5c4f3812a68b737d6e63632c3545002e
Cr-Commit-Position: refs/heads/master@{#416296}

[eroman, 2016-08-30 20:21:01]

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

[commit-bot: I haz the power, 2016-08-30 20:21:37]

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

[eroman, 2016-08-30 20:24:52]

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

[commit-bot: I haz the power, 2016-08-30 21:41:36]

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

[commit-bot: I haz the power, 2016-08-30 21:41:37]

Dry run: This issue passed the CQ dry run.

[mattm, 2016-08-31 19:49:49]

lgtm

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

https://codereview.chromium.org/2292333002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:467: if (!trust_anchor) {
does something hit this currently? or just to be more consistent with the
handling of empty chain?

[eroman, 2016-08-31 21:46:51]

https://codereview.chromium.org/2292333002/diff/1/net/cert/internal/path_buil...
File net/cert/internal/path_builder.cc (left):

https://codereview.chromium.org/2292333002/diff/1/net/cert/internal/path_buil...
net/cert/internal/path_builder.cc:701: next_path_.trust_anchor.get() &&
[1] Reference

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

https://codereview.chromium.org/2292333002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:467: if (!trust_anchor) {
On 2016/08/31 19:49:49, mattm wrote:
> does something hit this currently? or just to be more consistent with the
> handling of empty chain?

Good question, let me run a test to find out.

The way in which this came up is when I was changing the code in path builder
[1], it tests for a null trust anchor, which implies it can reach there with a
null trust anchor.

I didn't think too hard if that is actually the case, but proceeding under the
assumption that it is, I figured it would be simpler to move the error emitting
inside of VerifyCertificateChain() rather than run the risk of crashing.

I am not totally sold on this approach though, so happy to change it based on
your thoughts. WDYT?

[eroman, 2016-08-31 21:55:38]

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

https://codereview.chromium.org/2292333002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:467: if (!trust_anchor) {
On 2016/08/31 21:46:50, eroman wrote:
> On 2016/08/31 19:49:49, mattm wrote:
> > does something hit this currently? or just to be more consistent with the
> > handling of empty chain?
> 
> Good question, let me run a test to find out.
> 
> The way in which this came up is when I was changing the code in path builder
> [1], it tests for a null trust anchor, which implies it can reach there with a
> null trust anchor.
> 
> I didn't think too hard if that is actually the case, but proceeding under the
> assumption that it is, I figured it would be simpler to move the error
emitting
> inside of VerifyCertificateChain() rather than run the risk of crashing.
> 
> I am not totally sold on this approach though, so happy to change it based on
> your thoughts. WDYT?

Ran the tests -- nothing reaches this.

... which suggests that either I don't need it, or I should add some test. I
will probably just remove this since it is weird.

[mattm, 2016-09-01 02:04:39]

On 2016/08/31 21:55:38, eroman wrote:
>
https://codereview.chromium.org/2292333002/diff/1/net/cert/internal/verify_ce...
> File net/cert/internal/verify_certificate_chain.cc (right):
> 
>
https://codereview.chromium.org/2292333002/diff/1/net/cert/internal/verify_ce...
> net/cert/internal/verify_certificate_chain.cc:467: if (!trust_anchor) {
> On 2016/08/31 21:46:50, eroman wrote:
> > On 2016/08/31 19:49:49, mattm wrote:
> > > does something hit this currently? or just to be more consistent with the
> > > handling of empty chain?
> > 
> > Good question, let me run a test to find out.
> > 
> > The way in which this came up is when I was changing the code in path
builder
> > [1], it tests for a null trust anchor, which implies it can reach there with
a
> > null trust anchor.
> > 
> > I didn't think too hard if that is actually the case, but proceeding under
the
> > assumption that it is, I figured it would be simpler to move the error
> emitting
> > inside of VerifyCertificateChain() rather than run the risk of crashing.
> > 
> > I am not totally sold on this approach though, so happy to change it based
on
> > your thoughts. WDYT?
> 
> Ran the tests -- nothing reaches this.
> 
> ... which suggests that either I don't need it, or I should add some test. I
> will probably just remove this since it is weird.

Yeah, the pathbuilder shouldn't be able to call it with a null anchor. (See
CertPathIter::DoGetNextIssuerComplete, it only returns paths if they end in a
trust anchor)

[eroman, 2016-09-01 03:44:52]

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

https://codereview.chromium.org/2292333002/diff/1/net/cert/internal/verify_ce...
net/cert/internal/verify_certificate_chain.cc:467: if (!trust_anchor) {
On 2016/08/31 21:55:37, eroman wrote:
> On 2016/08/31 21:46:50, eroman wrote:
> > On 2016/08/31 19:49:49, mattm wrote:
> > > does something hit this currently? or just to be more consistent with the
> > > handling of empty chain?
> > 
> > Good question, let me run a test to find out.
> > 
> > The way in which this came up is when I was changing the code in path
builder
> > [1], it tests for a null trust anchor, which implies it can reach there with
a
> > null trust anchor.
> > 
> > I didn't think too hard if that is actually the case, but proceeding under
the
> > assumption that it is, I figured it would be simpler to move the error
> emitting
> > inside of VerifyCertificateChain() rather than run the risk of crashing.
> > 
> > I am not totally sold on this approach though, so happy to change it based
on
> > your thoughts. WDYT?
> 
> Ran the tests -- nothing reaches this.
> 
> ... which suggests that either I don't need it, or I should add some test. I
> will probably just remove this since it is weird.

Done -- removed

[eroman, 2016-09-01 03:45:27]

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

[eroman, 2016-09-01 03:45:27]

+sheretov for changes to Cast files

[eroman, 2016-09-01 04:33:09]

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

[commit-bot: I haz the power, 2016-09-01 04:33:24]

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-09-01 05:29:36]

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

[commit-bot: I haz the power, 2016-09-01 05:29:37]

Dry run: This issue passed the CQ dry run.

[sheretov, 2016-09-02 16:14:41]

lgtm

[eroman, 2016-09-02 16:25:23]

The CQ bit was checked by eroman@chromium.org

[eroman, 2016-09-02 16:25:24]

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/2292333002/#ps20001
(title: "remove error for null trust anchor")

[commit-bot: I haz the power, 2016-09-02 16:25:39]

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-09-02 17:45:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-02 17:47:35]

Description was changed from

==========
Add errors per ResultPath for CertPathBuilder.

BUG=634443
==========

to

==========
Add errors per ResultPath for CertPathBuilder.

BUG=634443

Committed: https://crrev.com/9433a3cc5c4f3812a68b737d6e63632c3545002e
Cr-Commit-Position: refs/heads/master@{#416296}
==========

[commit-bot: I haz the power, 2016-09-02 17:47:36]

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