[refactor] Extract the platform-specific TrustStore instantiations and

[refactor] Extract the platform-specific TrustStore instantiations and
setup for path building.

Has the side-effect of adding the Mac trust store integration to
CertVerifyProcBuiltin (however doesn't enable the tests yet as some
more work needed).

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

[eroman, 2017-04-19 19:19:04]

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

[commit-bot: I haz the power, 2017-04-19 19:19:27]

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-19 19:34:41]

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

[eroman, 2017-04-19 19:34:49]

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

[commit-bot: I haz the power, 2017-04-19 19:34:53]

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

[mattm, 2017-04-19 20:26:04]

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
File net/cert/internal/system_trust_store.cc (right):

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
net/cert/internal/system_trust_store.cc:133: // TODO(eroman): Should this return
something?
yeah, I just didn't implement the CertIssuerSourceMac yet..

I'm not entirely happy with the way that works on the NSS impl (having two
separate classes that do almost the same thing).  Not sure if we should change
the interfaces, or just merge the implementations (either having both classes
use some common helper, or having a single class that implements both
interfaces.)

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
File net/cert/internal/system_trust_store.h (right):

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
net/cert/internal/system_trust_store.h:44: // unsupported platforms. In the case
where this returns true, the trust store
true here should be false?

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
net/cert/internal/system_trust_store.h:69: // manner that has a global effect
within the process.
Is there a use case in mind for having one with a global effect?

[commit-bot: I haz the power, 2017-04-19 20:41:35]

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

[commit-bot: I haz the power, 2017-04-19 20:41:36]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-04-19 21:10:42]

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
File net/cert/internal/system_trust_store.cc (right):

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
net/cert/internal/system_trust_store.cc:133: // TODO(eroman): Should this return
something?
On 2017/04/19 20:26:04, mattm wrote:
> yeah, I just didn't implement the CertIssuerSourceMac yet..
> 
> I'm not entirely happy with the way that works on the NSS impl (having two
> separate classes that do almost the same thing).  Not sure if we should change
> the interfaces, or just merge the implementations (either having both classes
> use some common helper, or having a single class that implements both
> interfaces.)

Agreed.

I will circle back and re-visit the TrustStore interface separately (there are
TODOs for this).

Some things to address are:
  * Need to be able to indicate distrust
  * Expose intermediates

One possible API is to have an all in-one "LookupCertAndIssuer(cert, ....)"
which returns:
  * matching trust anchors
  * matching (indeterminate trust) intermediates
  * whether |cert| itself is distrusted

This would allow for a more efficient NSS implementation, however would need
some modifications to the path builder.

Where this gets a bit uglier is for composing TrustStores. Right now
TrustStoreCollection is simple since it is clear what it means to combine
TrustAnchor results. In the case of distrustedness, we would need a policy for
precedence of trustedness/distrustedness (I am thinking a blacklisted cert takes
precedence).

Are you OK with handling this as a separate change?

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
File net/cert/internal/system_trust_store.h (right):

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
net/cert/internal/system_trust_store.h:44: // unsupported platforms. In the case
where this returns true, the trust store
On 2017/04/19 20:26:04, mattm wrote:
> true here should be false?

Done.

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
net/cert/internal/system_trust_store.h:69: // manner that has a global effect
within the process.
On 2017/04/19 20:26:04, mattm wrote:
> Is there a use case in mind for having one with a global effect?

I am just documenting how things currently work (for instance in the
pre-existing NSS implementation).

I don't think it *should* work that way, however in practice it does (because
the ScopedTestRoot manipulates the global in-memory store).

[eroman, 2017-04-19 21:11:32]

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

[commit-bot: I haz the power, 2017-04-19 21:12:07]

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

[mattm, 2017-04-19 21:36:00]

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
File net/cert/internal/system_trust_store.cc (right):

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
net/cert/internal/system_trust_store.cc:133: // TODO(eroman): Should this return
something?
On 2017/04/19 21:10:41, eroman wrote:
> On 2017/04/19 20:26:04, mattm wrote:
> > yeah, I just didn't implement the CertIssuerSourceMac yet..
> > 
> > I'm not entirely happy with the way that works on the NSS impl (having two
> > separate classes that do almost the same thing).  Not sure if we should
change
> > the interfaces, or just merge the implementations (either having both
classes
> > use some common helper, or having a single class that implements both
> > interfaces.)
> 
> Agreed.
> 
> I will circle back and re-visit the TrustStore interface separately (there are
> TODOs for this).
> 
> Some things to address are:
>   * Need to be able to indicate distrust
>   * Expose intermediates
> 
> One possible API is to have an all in-one "LookupCertAndIssuer(cert, ....)"
> which returns:
>   * matching trust anchors
>   * matching (indeterminate trust) intermediates
>   * whether |cert| itself is distrusted
> 
> This would allow for a more efficient NSS implementation, however would need
> some modifications to the path builder.
> 
> Where this gets a bit uglier is for composing TrustStores. Right now
> TrustStoreCollection is simple since it is clear what it means to combine
> TrustAnchor results. In the case of distrustedness, we would need a policy for
> precedence of trustedness/distrustedness (I am thinking a blacklisted cert
takes
> precedence).
> 
> Are you OK with handling this as a separate change?

Yeah, certainly do that separately.

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
File net/cert/internal/system_trust_store.h (right):

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
net/cert/internal/system_trust_store.h:69: // manner that has a global effect
within the process.
On 2017/04/19 21:10:41, eroman wrote:
> On 2017/04/19 20:26:04, mattm wrote:
> > Is there a use case in mind for having one with a global effect?
> 
> I am just documenting how things currently work (for instance in the
> pre-existing NSS implementation).
> 
> I don't think it *should* work that way, however in practice it does (because
> the ScopedTestRoot manipulates the global in-memory store).

Well, I guess it is true that ScopedTestRoot works that way, but I don't think
AddTrustAnchor needs to care (Ie, you might pipe the certs from TestRootCerts
into AddTrustAnchor, but not the other way around.) So as far as this is
concerned, I don't think there is a case where calling AddTrustAnchor should
modify process-global state?

[eroman, 2017-04-19 21:42:53]

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
File net/cert/internal/system_trust_store.h (right):

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
net/cert/internal/system_trust_store.h:69: // manner that has a global effect
within the process.
On 2017/04/19 21:36:00, mattm wrote:
> On 2017/04/19 21:10:41, eroman wrote:
> > On 2017/04/19 20:26:04, mattm wrote:
> > > Is there a use case in mind for having one with a global effect?
> > 
> > I am just documenting how things currently work (for instance in the
> > pre-existing NSS implementation).
> > 
> > I don't think it *should* work that way, however in practice it does
(because
> > the ScopedTestRoot manipulates the global in-memory store).
> 
> Well, I guess it is true that ScopedTestRoot works that way, but I don't think
> AddTrustAnchor needs to care (Ie, you might pipe the certs from TestRootCerts
> into AddTrustAnchor, but not the other way around.) So as far as this is
> concerned, I don't think there is a case where calling AddTrustAnchor should
> modify process-global state?

Oh apologies, yes my comment is wrong I will remove it.

For some reason I was thinking and documenting the TestRootCerts behavior
here...
Yes you are correct we don't ever want that to happen here (should never be
globally visible). 
In fact, the very first sentence of the paragraph contradicts that second part
:)

[eroman, 2017-04-19 21:45:03]

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

[eroman, 2017-04-19 21:45:06]

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
File net/cert/internal/system_trust_store.h (right):

https://codereview.chromium.org/2829783002/diff/10006/net/cert/internal/syste...
net/cert/internal/system_trust_store.h:69: // manner that has a global effect
within the process.
On 2017/04/19 21:42:53, eroman wrote:
> On 2017/04/19 21:36:00, mattm wrote:
> > On 2017/04/19 21:10:41, eroman wrote:
> > > On 2017/04/19 20:26:04, mattm wrote:
> > > > Is there a use case in mind for having one with a global effect?
> > > 
> > > I am just documenting how things currently work (for instance in the
> > > pre-existing NSS implementation).
> > > 
> > > I don't think it *should* work that way, however in practice it does
> (because
> > > the ScopedTestRoot manipulates the global in-memory store).
> > 
> > Well, I guess it is true that ScopedTestRoot works that way, but I don't
think
> > AddTrustAnchor needs to care (Ie, you might pipe the certs from
TestRootCerts
> > into AddTrustAnchor, but not the other way around.) So as far as this is
> > concerned, I don't think there is a case where calling AddTrustAnchor should
> > modify process-global state?
> 
> Oh apologies, yes my comment is wrong I will remove it.
> 
> For some reason I was thinking and documenting the TestRootCerts behavior
> here...
> Yes you are correct we don't ever want that to happen here (should never be
> globally visible). 
> In fact, the very first sentence of the paragraph contradicts that second part
> :)

Done.

[commit-bot: I haz the power, 2017-04-19 21:45:34]

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

[mattm, 2017-04-19 21:48:21]

lgtm

[eroman, 2017-04-19 21:58:00]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2017-04-19 21:58:03]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2017-04-19 21:58:30]

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-19 22:47:30]

CQ is committing da patch.

Bot data: {"patchset_id": 50001, "attempt_start_ts": 1492639083695610,
"parent_rev": "bca0b3fbe6e0c63536e891cf028f3d46a08eefe3", "commit_rev":
"f628d6b4367d88b92e0bcf56886450e26ed58d55"}

[commit-bot: I haz the power, 2017-04-19 22:47:37]

Description was changed from

==========
[refactor] Extract the platform-specific TrustStore instantiations and
setup for path building.

Has the side-effect of adding the Mac trust store integration to
CertVerifyProcBuiltin (however doesn't enable the tests yet as some
more work needed).

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

to

==========
[refactor] Extract the platform-specific TrustStore instantiations and
setup for path building.

Has the side-effect of adding the Mac trust store integration to
CertVerifyProcBuiltin (however doesn't enable the tests yet as some
more work needed).

BUG=649017

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

[commit-bot: I haz the power, 2017-04-19 22:47:37]

Committed patchset #4 (id:50001) as
https://chromium.googlesource.com/chromium/src/+/f628d6b4367d88b92e0bcf568864...