pki library: Add CertIssuerSourceNSS that retrieves intermediate certs from NSS.

pki library: Add CertIssuerSourceNSS that retrieves intermediate certs from NSS.

BUG=635203
Committed: https://crrev.com/4abbcdbf78201fd2f8447ba6934537770b99e4f1
Cr-Commit-Position: refs/heads/master@{#435398}

[mattm, 2016-11-29 03:35:47]

Patchset #1 (id:1) has been deleted

[mattm, 2016-11-29 03:36:09]

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

[commit-bot: I haz the power, 2016-11-29 03:36:33]

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

[mattm, 2016-11-29 03:40:26]

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

[mattm, 2016-11-29 03:40:27]

most of the lines of change are from shifting the unittest to a parameterized
one and adding/regenerating the test cert files. I can try to split the test
parameterization into a preceding CL if you would find that easier to review.

[commit-bot: I haz the power, 2016-11-29 04:12:46]

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

[commit-bot: I haz the power, 2016-11-29 04:12:47]

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, 2016-11-29 19:54:53]

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

[commit-bot: I haz the power, 2016-11-29 19:55:19]

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

[eroman, 2016-11-29 20:18:27]

lgtm

https://codereview.chromium.org/2535733003/diff/20001/net/cert/internal/cert_...
File net/cert/internal/cert_issuer_source_nss.h (right):

https://codereview.chromium.org/2535733003/diff/20001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_nss.h:13: // Returns issuers from NSS.
Always returns results syncronously.
spelling:  synchronously

Can you also add a little bit more information to the description on what these
certificates correspond with, and how it relates to TrustStoreNss ?

In particular there may be duplication of certificates (that are also trust
anchors), and it also I believe will include explicitly distrusted certs.

We may want to re-visit the interface later to combine the trust store accesses.
But layering-wise having separate interfaces for querying trustedness of a cert,
trust anchor, and intermediates feels legit too.

https://codereview.chromium.org/2535733003/diff/20001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_nss.h:22: void AsyncGetIssuersOf(const
ParsedCertificate* cert,
Side-note, and thinking out loud here (not related to this CL): I wonder if it
would make sense to remove the async part of this interface and make it purely
sync. The only current consumer of async is for AIA. 

Assuming we won't have need of any async certificate providers (other than AIA),
Path builder could use the CertNetFetcher directly and have a less generic async
path.

https://codereview.chromium.org/2535733003/diff/20001/net/cert/internal/cert_...
File net/cert/internal/cert_issuer_source_nss_unittest.cc (right):

https://codereview.chromium.org/2535733003/diff/20001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_nss_unittest.cc:47: unsigned
nickname_counter_ = 0;
nit: "unsigned int"

[commit-bot: I haz the power, 2016-11-29 22:28:46]

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

[commit-bot: I haz the power, 2016-11-29 22:28:47]

Dry run: This issue passed the CQ dry run.

[mattm, 2016-11-30 03:07:19]

https://codereview.chromium.org/2535733003/diff/20001/net/cert/internal/cert_...
File net/cert/internal/cert_issuer_source_nss.h (right):

https://codereview.chromium.org/2535733003/diff/20001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_nss.h:13: // Returns issuers from NSS.
Always returns results syncronously.
On 2016/11/29 20:18:27, eroman (slow) wrote:
> spelling:  synchronously

Done.

> Can you also add a little bit more information to the description on what
these
> certificates correspond with, and how it relates to TrustStoreNss ?
> 
> In particular there may be duplication of certificates (that are also trust
> anchors), and it also I believe will include explicitly distrusted certs.

Done. (Although there is still a TODO about actually handling distrusted certs.)

Note also that there is the possibility that this could hide problems in our AIA
implementation if NSS already has a cached cert from its own AIA lookup. I
discussed it a bit with Ryan and although there could be some alternatives we
decided to go with this for now. Can discuss more if you want.

> We may want to re-visit the interface later to combine the trust store
accesses.
> But layering-wise having separate interfaces for querying trustedness of a
cert,
> trust anchor, and intermediates feels legit too.

Yeah, I think duplicating the lookup isn't a big burden for now, but we may want
to revisit later.

https://codereview.chromium.org/2535733003/diff/20001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_nss.h:22: void AsyncGetIssuersOf(const
ParsedCertificate* cert,
On 2016/11/29 20:18:27, eroman (slow) wrote:
> Side-note, and thinking out loud here (not related to this CL): I wonder if it
> would make sense to remove the async part of this interface and make it purely
> sync. The only current consumer of async is for AIA. 
> 
> Assuming we won't have need of any async certificate providers (other than
AIA),
> Path builder could use the CertNetFetcher directly and have a less generic
async
> path.

Possibly, if we find there aren't any other use cases.

https://codereview.chromium.org/2535733003/diff/20001/net/cert/internal/cert_...
File net/cert/internal/cert_issuer_source_nss_unittest.cc (right):

https://codereview.chromium.org/2535733003/diff/20001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_nss_unittest.cc:47: unsigned
nickname_counter_ = 0;
On 2016/11/29 20:18:27, eroman (slow) wrote:
> nit: "unsigned int"

Done.

[mattm, 2016-11-30 03:08:12]

The CQ bit was checked by mattm@chromium.org

[mattm, 2016-11-30 03:08:12]

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/2535733003/#ps60001
(title: "review changes")

[commit-bot: I haz the power, 2016-11-30 03:09:01]

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-11-30 05:10:23]

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

[commit-bot: I haz the power, 2016-11-30 05:10:25]

Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[mattm, 2016-11-30 19:34:39]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2016-11-30 19:35:21]

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-11-30 20:23:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480534479003730,
"parent_rev": "787ee1fb20f613826669496864e5ae1c57a736af", "commit_rev":
"da0c589e9dbbef16f576bd817af83c1771835623"}

[commit-bot: I haz the power, 2016-11-30 20:24:02]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-11-30 20:27:49]

Description was changed from

==========
pki library: Add CertIssuerSourceNSS that retrieves intermediate certs from NSS.

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

to

==========
pki library: Add CertIssuerSourceNSS that retrieves intermediate certs from NSS.

BUG=635203

Committed: https://crrev.com/4abbcdbf78201fd2f8447ba6934537770b99e4f1
Cr-Commit-Position: refs/heads/master@{#435398}
==========

[commit-bot: I haz the power, 2016-11-30 20:27:50]

Patchset 3 (id:??) landed as
https://crrev.com/4abbcdbf78201fd2f8447ba6934537770b99e4f1
Cr-Commit-Position: refs/heads/master@{#435398}