Add CertIssuerSource interface and CertIssuerSourceStatic implementation.

Add CertIssuerSource interface and CertIssuerSourceStatic implementation.

The CertIssuerSource interface will provide an abstraction for the path builder to
lookup issuers without needing to know the details of how.

CertIssuerSourceStatic is a simple implementation useful for providing a fixed set,
for example the certs received during the TLS handshake.

BUG=410574
Committed: https://crrev.com/137c967af0ab9f8cd49eaf3bd735acf2d0320832
Cr-Commit-Position: refs/heads/master@{#397899}

[mattm, 2016-06-01 20:15:45]

Patchset #1 (id:1) has been deleted

[mattm, 2016-06-01 20:15:52]

Patchset #1 (id:20001) has been deleted

[mattm, 2016-06-01 20:18:59]

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

[mattm, 2016-06-01 20:19:00]

Next part of https://codereview.chromium.org/1923433002/.
Child of https://codereview.chromium.org/1976433002/.

Note that the interface for AsyncGetIssuersOf changed a bit. I was working on
the AIA fetching and realized we'd need to support multiple async callbacks for
the same CertSource if there are several AIA urls in the cert.

[eroman, 2016-06-01 20:52:10]

CL description: typo: "implemntation"

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
File net/cert/internal/cert_source.h (right):

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_source.h:19: // Interface for looking up issuers of a
certificate during path building.
Can you clarify in the comments and interface, how callers differentiate between
sync vs async implementations?

It looks like they both return void, making it unclear how an implementation
would consume without knowing if the source provides synchronously or not.

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_source.h:24: class NET_EXPORT CertSource {
How about working "issuer" into the name somewhere.

Like CertIssuerSource, CertIssuerProvider, CertIssuerFinder, CertIssuerFetcher,
etc.

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_source.h:40: // SYNC, the Request is complete and the
|issuers_callback| will not be
not clear that this is referring to the outer one for the provisioning
CertSource

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_source.h:52: virtual ~CertSource() {}
= default; ?

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
File net/cert/internal/cert_source_static_unittest.cc (right):

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_source_static_unittest.cc:67:
scoped_refptr<ParsedCertificate> root_, b1_1_, b1_2_, b2_1_, c1_1_, c1_2_,
one per line please -- i believe that is the usual chrome style, even if more
verbose.

https://codereview.chromium.org/2030693002/diff/40001/net/data/cert_source_st...
File net/data/cert_source_static_unittest/b1_1.pem (right):

https://codereview.chromium.org/2030693002/diff/40001/net/data/cert_source_st...
net/data/cert_source_static_unittest/b1_1.pem:8: B1  b1  B2
Not sure what B1, b1, B2 C, C1, C1'' etc are referring to.

https://codereview.chromium.org/2030693002/diff/40001/net/data/cert_source_st...
File net/data/cert_source_static_unittest/generate-certs.py (right):

https://codereview.chromium.org/2030693002/diff/40001/net/data/cert_source_st...
net/data/cert_source_static_unittest/generate-certs.py:47:
write_cert_to_file(B1_1, 'b1_1.pem')
I didn't find the names easy to follow between the CN/diagram, and the
filenames.

Can you use names that have a direct mapping between the diagram and file name?
(Esp B1 vs b1 where capitalization is the only difference).

Before seeing the generator script I was also confused when reading the .pem
files. A more helpful comment might be to just redirect readers in the .pem file
to the generator script, and have the description of how they interact in here.

Or alternately put all the certificates into a single PEM file, with the current
comment block at the top (but it now refers to local objects of that file).

[mattm, 2016-06-01 21:06:11]

Description was changed from

==========
Add CertSource interface and CertSourceStatic implementation.

The CertSource interface will provide an abstraction for the path builder to
lookup issuers without needing to know the details of how.

CertSourceStatic is a simple implemntation useful for providing a fixed set,
for example the certs received during the TLS handshake.

BUG=410574
==========

to

==========
Add CertSource interface and CertSourceStatic implementation.

The CertSource interface will provide an abstraction for the path builder to
lookup issuers without needing to know the details of how.

CertSourceStatic is a simple implementation useful for providing a fixed set,
for example the certs received during the TLS handshake.

BUG=410574
==========

[mattm, 2016-06-01 22:10:50]

Patchset #2 (id:60001) has been deleted

[mattm, 2016-06-01 22:13:10]

Patchset #2 (id:80001) has been deleted

[mattm, 2016-06-01 22:20:41]

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
File net/cert/internal/cert_source.h (right):

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_source.h:19: // Interface for looking up issuers of a
certificate during path building.
On 2016/06/01 20:52:10, eroman wrote:
> Can you clarify in the comments and interface, how callers differentiate
between
> sync vs async implementations?
> 
> It looks like they both return void, making it unclear how an implementation
> would consume without knowing if the source provides synchronously or not.

Done.

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_source.h:24: class NET_EXPORT CertSource {
On 2016/06/01 20:52:10, eroman wrote:
> How about working "issuer" into the name somewhere.
> 
> Like CertIssuerSource, CertIssuerProvider, CertIssuerFinder,
CertIssuerFetcher,
> etc.

Done.

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_source.h:40: // SYNC, the Request is complete and the
|issuers_callback| will not be
On 2016/06/01 20:52:10, eroman wrote:
> not clear that this is referring to the outer one for the provisioning
> CertSource

Done.

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_source.h:52: virtual ~CertSource() {}
On 2016/06/01 20:52:10, eroman wrote:
> = default; ?

Done.

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
File net/cert/internal/cert_source_static_unittest.cc (right):

https://codereview.chromium.org/2030693002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_source_static_unittest.cc:67:
scoped_refptr<ParsedCertificate> root_, b1_1_, b1_2_, b2_1_, c1_1_, c1_2_,
On 2016/06/01 20:52:10, eroman wrote:
> one per line please -- i believe that is the usual chrome style, even if more
> verbose.

Done.

https://codereview.chromium.org/2030693002/diff/40001/net/data/cert_source_st...
File net/data/cert_source_static_unittest/b1_1.pem (right):

https://codereview.chromium.org/2030693002/diff/40001/net/data/cert_source_st...
net/data/cert_source_static_unittest/b1_1.pem:8: B1  b1  B2
On 2016/06/01 20:52:10, eroman wrote:
> Not sure what B1, b1, B2 C, C1, C1'' etc are referring to.

Done.

https://codereview.chromium.org/2030693002/diff/40001/net/data/cert_source_st...
File net/data/cert_source_static_unittest/generate-certs.py (right):

https://codereview.chromium.org/2030693002/diff/40001/net/data/cert_source_st...
net/data/cert_source_static_unittest/generate-certs.py:47:
write_cert_to_file(B1_1, 'b1_1.pem')
On 2016/06/01 20:52:10, eroman wrote:
> I didn't find the names easy to follow between the CN/diagram, and the
> filenames.
> 
> Can you use names that have a direct mapping between the diagram and file
name?
> (Esp B1 vs b1 where capitalization is the only difference).

Done.

> Before seeing the generator script I was also confused when reading the .pem
> files. A more helpful comment might be to just redirect readers in the .pem
file
> to the generator script, and have the description of how they interact in
here.

Done.

> Or alternately put all the certificates into a single PEM file, with the
current
> comment block at the top (but it now refers to local objects of that file).

Putting them all in a single PEM file makes it a bit more annoying in the test
code to figure out which cert is which.

[eroman, 2016-06-01 23:30:23]

lgtm after updating the CL description (to reflect new naming)

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source.h (right):

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source.h:24: // An implementations may choose to
return results from either one of the Get
i think the interface may need more specificity for conveying completion
synchronously, and being more opinionated on whether async is a superset of sync
results etc.

But can leave those comments to the integration CL :)

Has the WIP been updated to use this?

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_static.cc (right):

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_static.cc:33: out_req->reset();
should we reset out_req too?

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_static_unittest.cc (right):

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_static_unittest.cc:57: void
AddAllCerts(CertIssuerSourceStatic* source) {
optional: might read more easily as StaticCertIssuerSource

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_static_unittest.cc:135:
source.AsyncGetIssuersOf(c1_.get(), base::Bind(&NotCalled), &request);
Note because there is no indication of whether an async result is pending, this
could also just be testing request cancellation.
Since if this were to somehow return async, it probably wouldn't invoke the
callback until running the message loop

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/comp...
File net/cert/internal/completion_status.h (right):

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/comp...
net/cert/internal/completion_status.h:10: enum class CompletionStatus {
can't really review this in the absence of consumers

https://codereview.chromium.org/2030693002/diff/120001/net/data/cert_issuer_s...
File net/data/cert_issuer_source_static_unittest/generate-certs.py (right):

https://codereview.chromium.org/2030693002/diff/120001/net/data/cert_issuer_s...
net/data/cert_issuer_source_static_unittest/generate-certs.py:23: I1 (i1_1.pem)
and i1 (i1_2.pem) have subjects that are equal after
are these names updated? (I don't see i1 in the diagram above)

[mattm, 2016-06-01 23:48:23]

Description was changed from

==========
Add CertSource interface and CertSourceStatic implementation.

The CertSource interface will provide an abstraction for the path builder to
lookup issuers without needing to know the details of how.

CertSourceStatic is a simple implementation useful for providing a fixed set,
for example the certs received during the TLS handshake.

BUG=410574
==========

to

==========
Add CertIssuerSource interface and CertIssuerSourceStatic implementation.

The CertIssuerSource interface will provide an abstraction for the path builder
to
lookup issuers without needing to know the details of how.

CertIssuerSourceStatic is a simple implementation useful for providing a fixed
set,
for example the certs received during the TLS handshake.

BUG=410574
==========

[mattm, 2016-06-01 23:48:34]

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source.h (right):

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source.h:24: // An implementations may choose to
return results from either one of the Get
On 2016/06/01 23:30:23, eroman wrote:
> i think the interface may need more specificity for conveying completion
> synchronously, and being more opinionated on whether async is a superset of
sync
> results etc.
> 
> But can leave those comments to the integration CL :)

ack.

> Has the WIP been updated to use this?

Just uploaded a new patchset.

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_static.cc (right):

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_static.cc:33: out_req->reset();
On 2016/06/01 23:30:23, eroman wrote:
> should we reset out_req too?

Isn't that what it's doing?

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_static_unittest.cc (right):

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_static_unittest.cc:57: void
AddAllCerts(CertIssuerSourceStatic* source) {
On 2016/06/01 23:30:23, eroman wrote:
> optional: might read more easily as StaticCertIssuerSource

Reads easier, but I like the suffix since it sorts all the related files
together.

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_static_unittest.cc:135:
source.AsyncGetIssuersOf(c1_.get(), base::Bind(&NotCalled), &request);
On 2016/06/01 23:30:23, eroman wrote:
> Note because there is no indication of whether an async result is pending

request being null is the indication that no async result is pending.

>, this
> could also just be testing request cancellation.
> Since if this were to somehow return async, it probably wouldn't invoke the
> callback until running the message loop

Yeah, but it's basically true of any unittest that you can't completely test
that a callback wont wrongly being called asynchronously in some weird way.

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/comp...
File net/cert/internal/completion_status.h (right):

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/comp...
net/cert/internal/completion_status.h:10: enum class CompletionStatus {
On 2016/06/01 23:30:23, eroman wrote:
> can't really review this in the absence of consumers

(See updated WIP cl)

https://codereview.chromium.org/2030693002/diff/120001/net/data/cert_issuer_s...
File net/data/cert_issuer_source_static_unittest/generate-certs.py (right):

https://codereview.chromium.org/2030693002/diff/120001/net/data/cert_issuer_s...
net/data/cert_issuer_source_static_unittest/generate-certs.py:23: I1 (i1_1.pem)
and i1 (i1_2.pem) have subjects that are equal after
On 2016/06/01 23:30:23, eroman wrote:
> are these names updated? (I don't see i1 in the diagram above)

That's i1_2 in the diagram. I can't have the diagram match both the subject name
and the file name at the same time due to case insensitive filesystems.

[eroman, 2016-06-01 23:53:06]

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_static.cc (right):

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_static.cc:33: out_req->reset();
On 2016/06/01 23:48:33, mattm wrote:
> On 2016/06/01 23:30:23, eroman wrote:
> > should we reset out_req too?
> 
> Isn't that what it's doing? 

.... yeah.....
sorry, dunno what my brain did right there

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_static_unittest.cc (right):

https://codereview.chromium.org/2030693002/diff/120001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_static_unittest.cc:135:
source.AsyncGetIssuersOf(c1_.get(), base::Bind(&NotCalled), &request);
On 2016/06/01 23:48:33, mattm wrote:
> On 2016/06/01 23:30:23, eroman wrote:
> > Note because there is no indication of whether an async result is pending
> 
> request being null is the indication that no async result is pending.
> 
> >, this
> > could also just be testing request cancellation.
> > Since if this were to somehow return async, it probably wouldn't invoke the
> > callback until running the message loop
> 
> Yeah, but it's basically true of any unittest that you can't completely test
> that a callback wont wrongly being called asynchronously in some weird way.
> 

I missed the |request| test, per my earlier stupid comment.
With the line below this test makes sense to me :)

[mattm, 2016-06-04 01:02:03]

The CQ bit was checked by mattm@chromium.org

[mattm, 2016-06-04 01:02:04]

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/2030693002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-04 01:02:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030693002/160001

[commit-bot: I haz the power, 2016-06-04 02:02:54]

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

[commit-bot: I haz the power, 2016-06-04 02:02:55]

Try jobs failed on following builders:
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[mattm, 2016-06-04 02:13:56]

The CQ bit was checked by mattm@chromium.org

[mattm, 2016-06-04 02:13:56]

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/2030693002/#ps180001
(title: "update and re-run net/data/update_net_gypi.py")

[commit-bot: I haz the power, 2016-06-04 02:14:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030693002/180001

[commit-bot: I haz the power, 2016-06-04 03:33:03]

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

[commit-bot: I haz the power, 2016-06-04 03:33:06]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-06-04 03:33:37]

Description was changed from

==========
Add CertIssuerSource interface and CertIssuerSourceStatic implementation.

The CertIssuerSource interface will provide an abstraction for the path builder
to
lookup issuers without needing to know the details of how.

CertIssuerSourceStatic is a simple implementation useful for providing a fixed
set,
for example the certs received during the TLS handshake.

BUG=410574
==========

to

==========
Add CertIssuerSource interface and CertIssuerSourceStatic implementation.

The CertIssuerSource interface will provide an abstraction for the path builder
to
lookup issuers without needing to know the details of how.

CertIssuerSourceStatic is a simple implementation useful for providing a fixed
set,
for example the certs received during the TLS handshake.

BUG=410574

Committed: https://crrev.com/137c967af0ab9f8cd49eaf3bd735acf2d0320832
Cr-Commit-Position: refs/heads/master@{#397899}
==========

[commit-bot: I haz the power, 2016-06-04 03:33:38]

Patchset 6 (id:??) landed as
https://crrev.com/137c967af0ab9f8cd49eaf3bd735acf2d0320832
Cr-Commit-Position: refs/heads/master@{#397899}