Allow the TrustStore interface to return matching intermediates, and identify distrusted certs.

Allow the TrustStore interface to return matching intermediates, and identify distrusted certs.

* Make TrustStore implement CertIssuerSource
* Add a method for getting trust/distrust of a certificate
* Remove the TrustAnchor abstraction

All the integrations (including CertVerifyProc) required full certificates anyway, so TrustAnchor ended up being more of a hindrance than benefit.

BUG=649017
TBR=dougsteed@chromium.org

Review-Url: https://codereview.chromium.org/2832703002
Cr-Commit-Position: refs/heads/master@{#468175}
Committed: https://chromium.googlesource.com/chromium/src/+/2a938c3d286c77d7b769a9b7dc31bab8a309d19b

[eroman, 2017-04-20 01:06:12]

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

[commit-bot: I haz the power, 2017-04-20 01:07:01]

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

[eroman, 2017-04-20 01:11:45]

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

[commit-bot: I haz the power, 2017-04-20 01:12:02]

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

[eroman, 2017-04-20 01:16:09]

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

[commit-bot: I haz the power, 2017-04-20 02:16:25]

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

[commit-bot: I haz the power, 2017-04-20 02:16:26]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-04-20 03:19:07]

https://codereview.chromium.org/2832703002/diff/20001/net/cert/internal/trust...
File net/cert/internal/trust_store.h (right):

https://codereview.chromium.org/2832703002/diff/20001/net/cert/internal/trust...
net/cert/internal/trust_store.h:123: // [1] The identified trust anchors and
itermediates are merely "candidates"
intermediates

https://codereview.chromium.org/2832703002/diff/20001/net/cert/internal/trust...
net/cert/internal/trust_store.h:131: virtual bool IsBlacklisted(const
scoped_refptr<ParsedCertificate>& cert) const
If we still need to do a separate trust store check for each cert to check
blacklisting, I wonder if it makes sense to have FindIssuers only return certs
(not even do the trust tests, basically just making it another CertIssuerSource)
and then have a GetTrustStatus or something instead of IsBlacklisted. Then we
wouldn't end up doing the trust status check on each cert twice.

I guess that would complicate path builder a bit to prioritize trying the paths
with trust anchors first, we'd have to GetTrustStatus on all the intermediates
we had, and sort them based on the trust value..

https://codereview.chromium.org/2832703002/diff/20001/net/cert/internal/trust...
File net/cert/internal/trust_store_collection_unittest.cc (right):

https://codereview.chromium.org/2832703002/diff/20001/net/cert/internal/trust...
net/cert/internal/trust_store_collection_unittest.cc:81:
collection.FindIssuers(newintermediate_, &anchors, &intermediates);
These tests should all check the returned intermediates too.

[eroman, 2017-04-20 18:20:11]

https://codereview.chromium.org/2832703002/diff/20001/net/cert/internal/trust...
File net/cert/internal/trust_store.h (right):

https://codereview.chromium.org/2832703002/diff/20001/net/cert/internal/trust...
net/cert/internal/trust_store.h:131: virtual bool IsBlacklisted(const
scoped_refptr<ParsedCertificate>& cert) const
On 2017/04/20 03:19:07, mattm wrote:
> If we still need to do a separate trust store check for each cert to check
> blacklisting, I wonder if it makes sense to have FindIssuers only return certs
> (not even do the trust tests, basically just making it another
CertIssuerSource)
> and then have a GetTrustStatus or something instead of IsBlacklisted. Then we
> wouldn't end up doing the trust status check on each cert twice.
> 
> I guess that would complicate path builder a bit to prioritize trying the
paths
> with trust anchors first, we'd have to GetTrustStatus on all the intermediates
> we had, and sort them based on the trust value..

I generally like that, however if we go that route, I will need to revisit yet
again the TrustAnchor abstraction -- since we would always need a Certificate
per TrustAnchor. Maybe I should just give up on that abstraction, since
CertVerifyProc requires a Certificate anyways and all our trust stores represent
anchors by certificates...

Another idea for efficiency (although harmful for code readability) would be to
have TrustStore::FindIssuers() return a list of (cert, trust_level). In this
manner the trust lookups are not lost and can be batched internally while having
the OS cert handle. PathBuilder would however still need to query trustedness
for the target certificate, as well as intermediates obtained from locations
other than TrustStore, so this approach is kind of meh.

I can experiment with the first option.

[eroman, 2017-04-27 18:31:54]

Description was changed from

==========
Allow the TrustStore interface to return matching intermediates.

Previously this was handled by having a separate CertIssuerSource.

Also adds a method to check if a certificate is distrusted.

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

to

==========
Allow the TrustStore interface to return matching intermediates, and identify
distrusted certs.

* Make TrustStore implement CertIssuerSource
* Add a method for getting trust/distrust of a certificate
* Remove the TrustAnchor abstraction

All the integrations (including CertVerifyProc) required full certificates
anyway, so TrustAnchor ended up being more of a hindrance.

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

[eroman, 2017-04-27 18:32:18]

Description was changed from

==========
Allow the TrustStore interface to return matching intermediates, and identify
distrusted certs.

* Make TrustStore implement CertIssuerSource
* Add a method for getting trust/distrust of a certificate
* Remove the TrustAnchor abstraction

All the integrations (including CertVerifyProc) required full certificates
anyway, so TrustAnchor ended up being more of a hindrance.

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

to

==========
Allow the TrustStore interface to return matching intermediates, and identify
distrusted certs.

* Make TrustStore implement CertIssuerSource
* Add a method for getting trust/distrust of a certificate
* Remove the TrustAnchor abstraction

All the integrations (including CertVerifyProc) required full certificates
anyway, so TrustAnchor ended up being more of a hindrance than benefit.

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

[eroman, 2017-04-27 18:35:28]

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

[commit-bot: I haz the power, 2017-04-27 18:36:06]

Dry run: CQ is trying da patch.

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

[eroman, 2017-04-27 18:45:57]

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

[eroman, 2017-04-27 18:46:33]

PTAL, I updated per our discussion.

[commit-bot: I haz the power, 2017-04-27 18:46:51]

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-04-27 21:09:17]

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

[commit-bot: I haz the power, 2017-04-27 21:09:18]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-04-28 20:26:47]

https://codereview.chromium.org/2832703002/diff/60001/components/cast_certifi...
File components/cast_certificate/cast_cert_validator_unittest.cc (right):

https://codereview.chromium.org/2832703002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_cert_validator_unittest.cc:101: // buil-in Cast
roots).
built-in

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/path_...
File net/cert/internal/path_builder.cc (right):

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/path_...
net/cert/internal/path_builder.cc:235: // TODO(mattm): sort by notbefore, etc
(eg if cert issuer matches a trust
maybe move the TODO over to IssuerEntryComparator?

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/path_...
net/cert/internal/path_builder.cc:476: } else if
(next_issuer_.trust.HasUnspecifiedTrust()) {
just use else without a condition? Or have a final "else {NOTREACHED();}"?

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/trust...
File net/cert/internal/trust_store_collection.cc (right):

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/trust...
net/cert/internal/trust_store_collection.cc:35: // * If multiple stores consider
it trusted, use the trust result from first
doesn't it use the result from the last one?

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/trust...
File net/cert/internal/trust_store_collection_unittest.cc (right):

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/trust...
net/cert/internal/trust_store_collection_unittest.cc:15: class
TrustStoreCollectionTest : public testing::Test {
test GetTrust() too

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

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:513: return;
working_spki and working_normalized_issuer_name aren't initialized in the
non-trusted cases even though it looks like VerifyCertificateChain continues to
try to evaluate the rest of the chain. Won't this lead to spurious
CertPathErrors?

[eroman, 2017-04-28 21:45:36]

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

[commit-bot: I haz the power, 2017-04-28 21:46:21]

Dry run: CQ is trying da patch.

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

[eroman, 2017-04-28 21:48:04]

https://codereview.chromium.org/2832703002/diff/60001/components/cast_certifi...
File components/cast_certificate/cast_cert_validator_unittest.cc (right):

https://codereview.chromium.org/2832703002/diff/60001/components/cast_certifi...
components/cast_certificate/cast_cert_validator_unittest.cc:101: // buil-in Cast
roots).
On 2017/04/28 20:26:47, mattm wrote:
> built-in

Done.

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/path_...
File net/cert/internal/path_builder.cc (right):

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/path_...
net/cert/internal/path_builder.cc:476: } else if
(next_issuer_.trust.HasUnspecifiedTrust()) {
On 2017/04/28 20:26:47, mattm wrote:
> just use else without a condition? Or have a final "else {NOTREACHED();}"?

Changed to a switch statement

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/trust...
File net/cert/internal/trust_store_collection.cc (right):

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/trust...
net/cert/internal/trust_store_collection.cc:35: // * If multiple stores consider
it trusted, use the trust result from first
On 2017/04/28 20:26:47, mattm wrote:
> doesn't it use the result from the last one?

You are correct. Updated the comment.

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/trust...
File net/cert/internal/trust_store_collection_unittest.cc (right):

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/trust...
net/cert/internal/trust_store_collection_unittest.cc:15: class
TrustStoreCollectionTest : public testing::Test {
On 2017/04/28 20:26:47, mattm wrote:
> test GetTrust() too

Done.

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

https://codereview.chromium.org/2832703002/diff/60001/net/cert/internal/verif...
net/cert/internal/verify_certificate_chain.cc:513: return;
On 2017/04/28 20:26:47, mattm wrote:
> working_spki and working_normalized_issuer_name aren't initialized in the
> non-trusted cases even though it looks like VerifyCertificateChain continues
to
> try to evaluate the rest of the chain. Won't this lead to spurious
> CertPathErrors?

Good point.

I don't currently have tests for those cases, and need to follow-up with tests
at least for the distrust case (unspecified is probably not reachable in
practice). I would like to simplify the test format before sticking in the
distrust stuff, since I think this will work better layering on top of the
changes in https://codereview.chromium.org/2805213004/  (which can be further
simplified now)

I will go ahead and initialize the subject and spki for all cases for now.

[mattm, 2017-04-28 22:38:27]

lgtm

[eroman, 2017-04-28 22:40:52]

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

[eroman, 2017-04-28 22:40:53]

TBR dougsteed@ for components/cast_certificate OWNER

There should not be any functional changes; this is just refactoring some of our
interfaces

[eroman, 2017-04-28 23:02:03]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2017-04-28 23:02:07]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2017-04-28 23:03:35]

CQ is trying da patch.

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

[eroman, 2017-04-28 23:07:44]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2017-04-28 23:08:01]

Description was changed from

==========
Allow the TrustStore interface to return matching intermediates, and identify
distrusted certs.

* Make TrustStore implement CertIssuerSource
* Add a method for getting trust/distrust of a certificate
* Remove the TrustAnchor abstraction

All the integrations (including CertVerifyProc) required full certificates
anyway, so TrustAnchor ended up being more of a hindrance than benefit.

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

to

==========
Allow the TrustStore interface to return matching intermediates, and identify
distrusted certs.

* Make TrustStore implement CertIssuerSource
* Add a method for getting trust/distrust of a certificate
* Remove the TrustAnchor abstraction

All the integrations (including CertVerifyProc) required full certificates
anyway, so TrustAnchor ended up being more of a hindrance than benefit.

BUG=649017
TBR=dougsteed@chromium.org
==========

[eroman, 2017-04-28 23:08:10]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2017-04-28 23:08:56]

CQ is trying da patch.

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

[ryanchung, 2017-04-28 23:11:29]

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

[ryanchung, 2017-04-28 23:11:30]

https://codereview.chromium.org/2832703002/diff/80001/components/cast_certifi...
File components/cast_certificate/cast_cert_validator_test_helpers.cc (right):

https://codereview.chromium.org/2832703002/diff/80001/components/cast_certifi...
components/cast_certificate/cast_cert_validator_test_helpers.cc:96:
trust_store->AddTrustAnchorWithConstraints(cert);
nit: std:move(cert) ?

[commit-bot: I haz the power, 2017-04-28 23:17:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493420890609380,
"parent_rev": "c731464f86c6a5c5c3f2b64890e04b38750896fc", "commit_rev":
"2a938c3d286c77d7b769a9b7dc31bab8a309d19b"}

[commit-bot: I haz the power, 2017-04-28 23:17:14]

Description was changed from

==========
Allow the TrustStore interface to return matching intermediates, and identify
distrusted certs.

* Make TrustStore implement CertIssuerSource
* Add a method for getting trust/distrust of a certificate
* Remove the TrustAnchor abstraction

All the integrations (including CertVerifyProc) required full certificates
anyway, so TrustAnchor ended up being more of a hindrance than benefit.

BUG=649017
TBR=dougsteed@chromium.org
==========

to

==========
Allow the TrustStore interface to return matching intermediates, and identify
distrusted certs.

* Make TrustStore implement CertIssuerSource
* Add a method for getting trust/distrust of a certificate
* Remove the TrustAnchor abstraction

All the integrations (including CertVerifyProc) required full certificates
anyway, so TrustAnchor ended up being more of a hindrance than benefit.

BUG=649017
TBR=dougsteed@chromium.org

Review-Url: https://codereview.chromium.org/2832703002
Cr-Commit-Position: refs/heads/master@{#468175}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a938c3d286c77d7b769a9b7dc31...
==========

[commit-bot: I haz the power, 2017-04-28 23:17:16]

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

[eroman, 2017-04-28 23:51:06]

https://codereview.chromium.org/2832703002/diff/80001/components/cast_certifi...
File components/cast_certificate/cast_cert_validator_test_helpers.cc (right):

https://codereview.chromium.org/2832703002/diff/80001/components/cast_certifi...
components/cast_certificate/cast_cert_validator_test_helpers.cc:96:
trust_store->AddTrustAnchorWithConstraints(cert);
On 2017/04/28 23:11:30, ryanchung wrote:
> nit: std:move(cert) ?

AddTrustAnchorWithConstraints() in this case takes a |const
scoped_refptr<ParsedCertificate>&|, so it wouldn't benefit from move semantics.

That said there is some inconsistency between which methods takes
scoped_refptr<> vs which ones take a const reference, which could be improved.

(In general I find the move() versions to be more fragile, since it invalidates
the caller's variable; mainly only done if we want to squeeze out unnecessary
refcounts).

[ryanchung, 2017-04-28 23:57:37]

https://codereview.chromium.org/2832703002/diff/80001/components/cast_certifi...
File components/cast_certificate/cast_cert_validator_test_helpers.cc (right):

https://codereview.chromium.org/2832703002/diff/80001/components/cast_certifi...
components/cast_certificate/cast_cert_validator_test_helpers.cc:96:
trust_store->AddTrustAnchorWithConstraints(cert);
On 2017/04/28 23:51:06, eroman wrote:
> On 2017/04/28 23:11:30, ryanchung wrote:
> > nit: std:move(cert) ?
> 
> AddTrustAnchorWithConstraints() in this case takes a |const
> scoped_refptr<ParsedCertificate>&|, so it wouldn't benefit from move
semantics.
> 
> That said there is some inconsistency between which methods takes
> scoped_refptr<> vs which ones take a const reference, which could be improved.
> 
> (In general I find the move() versions to be more fragile, since it
invalidates
> the caller's variable; mainly only done if we want to squeeze out unnecessary
> refcounts).

Isn't it AddTrustAnchorWithConstraints(scoped_refptr<ParsedCertificate> cert);

https://cs.chromium.org/chromium/src/net/cert/internal/trust_store_in_memory....

[dougsteed, 2017-04-29 05:22:32]

lgtm