Add TrustStoreNSS and TrustStoreCollection

Add TrustStoreNSS and TrustStoreCollection

BUG=635203
Committed: https://crrev.com/9c63d444f0af74f7379839939412f5bd8341b590
Cr-Commit-Position: refs/heads/master@{#416417}

[mattm, 2016-08-23 01:30:07]

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

[commit-bot: I haz the power, 2016-08-23 01:30:44]

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

[mattm, 2016-08-23 01:31:42]

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

[mattm, 2016-08-23 01:31:43]

This is a followup to https://codereview.chromium.org/2266333002/, so look at
that first.

[commit-bot: I haz the power, 2016-08-23 01:40:07]

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

[commit-bot: I haz the power, 2016-08-23 01:40:09]

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

[eroman, 2016-08-23 18:48:54]

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
File net/cert/internal/trust_store_collection.cc (right):

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.cc:13: DCHECK(!primary_store_);
suggest also adding DCHECK(store) -- or do you want the API to permit nulling it
through this?

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.cc:31: std::unique_ptr<Request>
unused_req;
should nullptr be allowed in the case where null callback is passed?

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
File net/cert/internal/trust_store_collection.h (right):

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.h:26: // multiple batches.)
Can you mention something about the order of results that this combined store
returns?

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.h:32: // Include results from |store|
in the combined output. Both sync and async
Include --> Includes

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.h:37: // Include results from |store|
in the combined output. |store| will only be
Includes

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.h:42: void FindTrustAnchorsForCert(
Do you think this API will ever need to communicate errors?

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.h:44: const TrustAnchorCallback&
callback,
side-comment (belongs on other CL): "TrustAnchorCallback" is a bit general -- in
my mind that would be a callback that returns a single TrustAnchor. I suggest
exploring other options like:
  * TrustAnchorsCallback
  * FindTrustAnchorsCallback
  * MatchingTrustAnchorsCallback
  * FoundTrustAnchorsCallback

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
File net/cert/internal/trust_store_nss.cc (right):

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_nss.cc:59:
ParsedCertificate::DataSource::INTERNAL_COPY, {});
side-comment: I am thinking we may want to refactor this at some point
(cumbersome to type and a bit confusing). I am thinking we should either get rid
of the non-copy version, or make it a bit more hidden since it is pretty niche
and tricky to use correctly.

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_nss.cc:126: req->Start(cert->tbs().issuer_tlv,
nss_task_runner_.get());
This could be a problem in the case of cancellation -- I believe |cert| might be
deleted before the task completes, and the task is holding onto a der::Input
which just aliases the data.

I suggest instead passing a reference of the cert to the task, or copying the
issuer name out here.

[mattm, 2016-08-26 02:23:20]

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

[commit-bot: I haz the power, 2016-08-26 02:23:48]

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-08-26 02:33:50]

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

[commit-bot: I haz the power, 2016-08-26 02:33:50]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[mattm, 2016-08-26 22:17:08]

Patchset #5 (id:80001) has been deleted

[mattm, 2016-08-26 22:38:49]

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

[commit-bot: I haz the power, 2016-08-26 22:39:43]

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

[mattm, 2016-08-26 23:41:30]

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

[commit-bot: I haz the power, 2016-08-26 23:42:05]

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

[mattm, 2016-08-27 00:30:28]

Patchset #6 (id:120001) has been deleted

[mattm, 2016-08-27 00:37:02]

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
File net/cert/internal/trust_store_collection.cc (right):

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.cc:13: DCHECK(!primary_store_);
On 2016/08/23 18:48:54, eroman wrote:
> suggest also adding DCHECK(store) -- or do you want the API to permit nulling
it
> through this?

Done.

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.cc:31: std::unique_ptr<Request>
unused_req;
On 2016/08/23 18:48:54, eroman wrote:
> should nullptr be allowed in the case where null callback is passed?

Done.

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
File net/cert/internal/trust_store_collection.h (right):

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.h:26: // multiple batches.)
On 2016/08/23 18:48:54, eroman wrote:
> Can you mention something about the order of results that this combined store
> returns?

Done.

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.h:32: // Include results from |store|
in the combined output. Both sync and async
On 2016/08/23 18:48:54, eroman wrote:
> Include --> Includes

Done.

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.h:37: // Include results from |store|
in the combined output. |store| will only be
On 2016/08/23 18:48:54, eroman wrote:
> Includes

Done.

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.h:42: void FindTrustAnchorsForCert(
On 2016/08/23 18:48:54, eroman wrote:
> Do you think this API will ever need to communicate errors?

I think we may need something as far as logging / debugging error reporting. For
example if we can't parse a cert we get out of the OS, would be good to include
that in the debug info about maybe why we couldn't build a chain.

But I don't currently see a need to have a error output that the caller would
actually act on (if there is an error there's not really anything the caller can
do other than continue on and see if other paths succeed. Wouldn't want to fail
completely just because one possible issuer failed to parse or something like
that.)

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_collection.h:44: const TrustAnchorCallback&
callback,
On 2016/08/23 18:48:54, eroman wrote:
> side-comment (belongs on other CL): "TrustAnchorCallback" is a bit general --
in
> my mind that would be a callback that returns a single TrustAnchor. I suggest
> exploring other options like:
>   * TrustAnchorsCallback
>   * FindTrustAnchorsCallback
>   * MatchingTrustAnchorsCallback
>   * FoundTrustAnchorsCallback

Done.

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
File net/cert/internal/trust_store_nss.cc (right):

https://codereview.chromium.org/2272493002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_nss.cc:126: req->Start(cert->tbs().issuer_tlv,
nss_task_runner_.get());
On 2016/08/23 18:48:54, eroman wrote:
> This could be a problem in the case of cancellation -- I believe |cert| might
be
> deleted before the task completes, and the task is holding onto a der::Input
> which just aliases the data.
> 
> I suggest instead passing a reference of the cert to the task, or copying the
> issuer name out here.

it was making a copy of the name inside Start() before posting a task. But I
think that design was still sort of just a holdover from the previous interface
being FindTrustAnchorsByNormalizedName. I've changed it to take a reference to
the cert.

[mattm, 2016-08-27 00:40:08]

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

[commit-bot: I haz the power, 2016-08-27 00:40:42]

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-27 01:53:39]

lgtm

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
File net/cert/internal/trust_store.h (right):

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store.h:143: scoped_refptr<ParsedCertificate> cert,
Should we make this a const-reference in case implementations don't need to hold
a reference? (i.e. synchronous-only ones).

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
File net/cert/internal/trust_store_collection.h (right):

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_collection.h:19: // results from multiple other
TrustStores.
nit: multiple other --> "multiple"

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_collection.h:27: // assumption is that async one
would be useful for OS integration, while the
"that async one" --> "that the async one"

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
File net/cert/internal/trust_store_nss.cc (right):

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:18: // TODO(mattm): structure so that
supporting chromeos stuff is doable (
nit on capitalization: ChromeOS ?

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:19: // TrustStoreChromeOS which uses
net::NSSProfileFilterChromeOS.. similar to
Not sure I follow this comment. Was it a note-to-self, or a comment for this
file?

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:32: void
GetAnchors(scoped_refptr<ParsedCertificate> cert,
is our Bind() smart enough to avoid copying from storage to the invocation?

I would probably just express this as const-reference to ensure doesn't do a
spurious add-ref.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:41: PR_Now() /* sorttime */, PR_FALSE /*
validOnly */);
Is validOnly=false necessary? (i.e. are trust anchors going to be invalid
certificates)

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:61: // TODO(mattm): return errors better.
In fact we may need to be less strict and just do a dumb extraction of SPKI and
subject, without all the other enforcements. (food for thought, no change needed
here).

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:66:
out_anchors->push_back(TrustAnchor::CreateFromCertificateNoConstraints(
Does NSS trust store have the notion of attached constraints, or enforcing any
constraints from the certificate?

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:97: auto* anchors_ptr = anchors.get();
heh. gotta love C++'s undefined argument evaluation order :)

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:125: auto
req(base::MakeUnique<GetAnchorsRequest>(callback));
style nit: for consistency with higher up, use "auto req = "

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:126: // Use the original issuer value
instead of the normalized version. NSS does a
How about moving this comment into GetAnchors() since that is where this
decision is being made now.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
File net/cert/internal/trust_store_nss.h (right):

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.h:24: class NET_EXPORT TrustStoreNSS : public
TrustStore {
I have never understood if we are supposed to type things Nss or NSS.

Fine either way, I think we aren't consistent in Chromium code

[commit-bot: I haz the power, 2016-08-27 03:26:07]

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

[commit-bot: I haz the power, 2016-08-27 03:26:08]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mattm, 2016-08-29 20:36:50]

Patchset #8 (id:180001) has been deleted

[mattm, 2016-08-29 20:36:56]

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

[mattm, 2016-08-29 20:38:15]

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

[mattm, 2016-08-29 20:38:17]

Adding Ryan to check the NSS bits. Also see Eric's question about anchor
constraints.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
File net/cert/internal/trust_store.h (right):

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store.h:143: scoped_refptr<ParsedCertificate> cert,
On 2016/08/27 01:53:38, eroman wrote:
> Should we make this a const-reference in case implementations don't need to
hold
> a reference? (i.e. synchronous-only ones).

Done.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
File net/cert/internal/trust_store_collection.h (right):

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_collection.h:19: // results from multiple other
TrustStores.
On 2016/08/27 01:53:38, eroman wrote:
> nit: multiple other --> "multiple"

Done.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_collection.h:27: // assumption is that async one
would be useful for OS integration, while the
On 2016/08/27 01:53:38, eroman wrote:
> "that async one" --> "that the async one"

Done.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
File net/cert/internal/trust_store_nss.cc (right):

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:18: // TODO(mattm): structure so that
supporting chromeos stuff is doable (
On 2016/08/27 01:53:39, eroman wrote:
> nit on capitalization: ChromeOS ?

Done.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:19: // TrustStoreChromeOS which uses
net::NSSProfileFilterChromeOS.. similar to
On 2016/08/27 01:53:38, eroman wrote:
> Not sure I follow this comment. Was it a note-to-self, or a comment for this
> file?

Yeah, I guess it was mostly a note-to-self. I'll remove it.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:32: void
GetAnchors(scoped_refptr<ParsedCertificate> cert,
On 2016/08/27 01:53:38, eroman wrote:
> is our Bind() smart enough to avoid copying from storage to the invocation?
> 
> I would probably just express this as const-reference to ensure doesn't do a
> spurious add-ref.

I think it is, but changed to be safe.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:41: PR_Now() /* sorttime */, PR_FALSE /*
validOnly */);
On 2016/08/27 01:53:38, eroman wrote:
> Is validOnly=false necessary? (i.e. are trust anchors going to be invalid
> certificates)

validOnly here refers to the time validity checking, so we want that to be false
for trust anchors. Added a comment.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:61: // TODO(mattm): return errors better.
On 2016/08/27 01:53:39, eroman wrote:
> In fact we may need to be less strict and just do a dumb extraction of SPKI
and
> subject, without all the other enforcements. (food for thought, no change
needed
> here).

Acknowledged.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:66:
out_anchors->push_back(TrustAnchor::CreateFromCertificateNoConstraints(
On 2016/08/27 01:53:38, eroman wrote:
> Does NSS trust store have the notion of attached constraints, or enforcing any
> constraints from the certificate?

I don't think it does. Ryan?

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:125: auto
req(base::MakeUnique<GetAnchorsRequest>(callback));
On 2016/08/27 01:53:38, eroman wrote:
> style nit: for consistency with higher up, use "auto req = "

Done.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:126: // Use the original issuer value
instead of the normalized version. NSS does a
On 2016/08/27 01:53:38, eroman wrote:
> How about moving this comment into GetAnchors() since that is where this
> decision is being made now.

Done.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
File net/cert/internal/trust_store_nss.h (right):

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.h:24: class NET_EXPORT TrustStoreNSS : public
TrustStore {
On 2016/08/27 01:53:39, eroman wrote:
> I have never understood if we are supposed to type things Nss or NSS.
> 
> Fine either way, I think we aren't consistent in Chromium code

I think the style guide says Nss, but in Chromium it is inconsistent. In
Chromium NSS is usually spelled NSS so I stuck with that.

[commit-bot: I haz the power, 2016-08-29 21:15:33]

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-08-30 03:31:58]

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

[commit-bot: I haz the power, 2016-08-30 03:31:59]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-08-31 17:59:16]

I wonder how much this async design is going to cost us in performance.

Each step in the path it is waiting for this async lookup from NSS. Which if NSS
is being queried from another thread means numerous thread hops back and forth.

Is there any hope of querying NSS trustedness directly from the IO thread?

[Ryan Sleevi, 2016-08-31 20:50:40]

A few pedantry nits but the NSS API looks good % more pedantry

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
File net/cert/internal/trust_store.h (right):

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
net/cert/internal/trust_store.h:143: const scoped_refptr<ParsedCertificate>&
cert,
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

TL;DR: Pass scoped_refptr<> by value, and std::move()

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
File net/cert/internal/trust_store_collection_unittest.cc (right):

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
net/cert/internal/trust_store_collection_unittest.cc:30: };
FWIW, only because Darin sent me a CL where he was using GMock, a Gmock way of
expressing this would be

EXPECT_CALL(*mock, FindTrustAnchorsForCert(_,
Property(&TrustAnchorsCallback::is_null, true), _, _, _));

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
File net/cert/internal/trust_store_nss.cc (right):

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:55: if ((trust.sslFlags & ca_trust) !=
ca_trust)
int flags = SEC_GET_TRUST_FLAG(&trust, trustSSL) is the "official"
source-compatible way. I mean, what you're doing is correct, it's just... yeah

And yeah, it'll be good to handle distrust.

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
File net/cert/internal/trust_store_nss.h (right):

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.h:19: // anchors for path building.
Maybe worth highlighting this is only suitable for SSL. Or should you do what
libpkix and Mozilla do, which is take a SECTrustType to indicate what sort of
trust(s) you're evaluating against?

(I imagine the same for OS X, where we need to indicate an SSL trust policy and
not just an X.509 TP)

[mattm, 2016-09-01 00:18:51]

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

[commit-bot: I haz the power, 2016-09-01 00:19:13]

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 00:26:40]

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

[commit-bot: I haz the power, 2016-09-01 00:26:40]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mattm, 2016-09-01 00:49:45]

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

[commit-bot: I haz the power, 2016-09-01 00:50:20]

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

[mattm, 2016-09-01 00:59:02]

On 2016/08/31 17:59:16, eroman wrote:
> I wonder how much this async design is going to cost us in performance.
> 
> Each step in the path it is waiting for this async lookup from NSS. Which if
NSS
> is being queried from another thread means numerous thread hops back and
forth.
> 
> Is there any hope of querying NSS trustedness directly from the IO thread?

I don't think there's any way to do that without allowing NSS to block the IO
thread.

The other option would be to do the whole path building on the worker thread,
but that means we need to worry about lifetime of all the objects passed into
the pathbuilder, and may end up needing to post back to the IO thread anyway to
do AIA. If it turns out to be an issue we might have to consider that.

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
File net/cert/internal/trust_store_nss.cc (right):

https://codereview.chromium.org/2272493002/diff/160001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:66:
out_anchors->push_back(TrustAnchor::CreateFromCertificateNoConstraints(
On 2016/08/29 20:38:16, mattm wrote:
> On 2016/08/27 01:53:38, eroman wrote:
> > Does NSS trust store have the notion of attached constraints, or enforcing
any
> > constraints from the certificate?
> 
> I don't think it does. Ryan?

I chatted with Ryan about that. The current state is there is only one public
API for this in NSS, CERT_GetImposedNameConstraints (added in NSS 3.19, which we
also don't require yet). It's only used for the hardcoded ANSSI constraints and
doesn't have a way for clients to add additional constraints. So as long as we
do our own enforcement of imposed constraints like we do for CertVerifyProc, we
don't need to worry about that at the moment.

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
File net/cert/internal/trust_store.h (right):

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
net/cert/internal/trust_store.h:143: const scoped_refptr<ParsedCertificate>&
cert,
On 2016/08/31 20:50:39, Ryan Sleevi (slow) wrote:
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> 
> TL;DR: Pass scoped_refptr<> by value, and std::move()

Yea, this does go against the style guide. But following that will lead to
unnecessary reference cycling, since not all the implementations actually need a
reference, and the caller can't move() its own reference.

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
File net/cert/internal/trust_store_collection_unittest.cc (right):

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
net/cert/internal/trust_store_collection_unittest.cc:30: };
On 2016/08/31 20:50:39, Ryan Sleevi (slow) wrote:
> FWIW, only because Darin sent me a CL where he was using GMock, a Gmock way of
> expressing this would be
> 
> EXPECT_CALL(*mock, FindTrustAnchorsForCert(_,
> Property(&TrustAnchorsCallback::is_null, true), _, _, _));

Done.

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
File net/cert/internal/trust_store_nss.cc (right):

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.cc:55: if ((trust.sslFlags & ca_trust) !=
ca_trust)
On 2016/08/31 20:50:39, Ryan Sleevi (slow) wrote:
> int flags = SEC_GET_TRUST_FLAG(&trust, trustSSL) is the "official"
> source-compatible way. I mean, what you're doing is correct, it's just... yeah

done.

> And yeah, it'll be good to handle distrust.

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
File net/cert/internal/trust_store_nss.h (right):

https://codereview.chromium.org/2272493002/diff/200001/net/cert/internal/trus...
net/cert/internal/trust_store_nss.h:19: // anchors for path building.
On 2016/08/31 20:50:39, Ryan Sleevi (slow) wrote:
> Maybe worth highlighting this is only suitable for SSL. Or should you do what
> libpkix and Mozilla do, which is take a SECTrustType to indicate what sort of
> trust(s) you're evaluating against?
> 
> (I imagine the same for OS X, where we need to indicate an SSL trust policy
and
> not just an X.509 TP)

Changed to take a SECTrustType.

[commit-bot: I haz the power, 2016-09-01 01:54:59]

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

[commit-bot: I haz the power, 2016-09-01 01:55:00]

Dry run: This issue passed the CQ dry run.

[mattm, 2016-09-02 23:33:00]

The CQ bit was checked by mattm@chromium.org

[mattm, 2016-09-02 23:33:01]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2272493002/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-02 23:33:29]

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-03 00:46:07]

Committed patchset #12 (id:280001)

[commit-bot: I haz the power, 2016-09-03 00:49:11]

Description was changed from

==========
Add TrustStoreNSS and TrustStoreCollection

BUG=635203
==========

to

==========
Add TrustStoreNSS and TrustStoreCollection

BUG=635203

Committed: https://crrev.com/9c63d444f0af74f7379839939412f5bd8341b590
Cr-Commit-Position: refs/heads/master@{#416417}
==========

[commit-bot: I haz the power, 2016-09-03 00:49:12]

Patchset 12 (id:??) landed as
https://crrev.com/9c63d444f0af74f7379839939412f5bd8341b590
Cr-Commit-Position: refs/heads/master@{#416417}