Add CertIssuerSourceAia: authorityInfoAccess fetching for CertPathBuilder.

Add CertIssuerSourceAia: authorityInfoAccess fetching for CertPathBuilder.

BUG=410574
Committed: https://crrev.com/8d7481465225d87d54154a3c5561983e116b0cf1
Cr-Commit-Position: refs/heads/master@{#399838}

[mattm, 2016-06-03 02:57:22]

Patchset #1 (id:1) has been deleted

[mattm, 2016-06-03 02:57:28]

Patchset #1 (id:20001) has been deleted

[mattm, 2016-06-03 02:57:43]

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

[eroman, 2016-06-03 16:28:53]

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

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:49: // XXX should HTTPS be
disallowed?
XXX --> TODO(...)

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:51: if (url.is_valid() &&
url.SchemeIsHTTPOrHTTPS())
We only support http:// for the fetching of AIA [1]

Shouldn't encode that fetching policy in here, but rather leave it to the
CertNetFetcher implementation.
Possible solutions:
  * Simply pass all URLs to the fetcher and let it fail them if it doesn't like
  * Add a synchronous failure mode for FetchCaIssuers()
  * Add a method to CertNetFetcher's interface similar to
"SupportsFetchingUrl(GURL)"

Of those I suggest the first option for this CL.
The second option might be reasonable, but as a separate change.
And the third option I don't much care fore.

[1]
https://cs.chromium.org/chromium/src/net/cert_net/cert_net_fetcher_impl.cc?sq...

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:54: if (urls.empty())
I think we also should have a failsafe that bounds how many AIA's we will
consider for a single certificate.

We can run some numbers to see what is reasonable, but I would imagine 5-10 is
already more than will be used in practice

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:100: LOG(ERROR) <<
"AiaRequest::FetchCallback got error " << error;
Please add a TODO here. Rather than swallowing the error, we will want to
propagate this through.

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:111:
ParsedCertificate::DataSource::INTERNAL_COPY, &results_)) {
Can you add a TODO to avoid copying the bytes?

(We can change the CertNetFetcher interface to pass ownership of the bytes
instead, and also enable passing into ParsedCertificate).

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:112: LOG(ERROR) << "Error parsing
AIA data";
Also add a TODO here about the error propagation.

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

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.h:8: #include <unordered_map>
unused

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.h:52: int pending_requests_ = 0;
can this be unsigned?

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

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:57: explicit
Request(RequestManager* manager,
nit: no need for explicit

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:434: std::vector<base::StringPiece>*
out_ca_issuers_uris,
why stringpiece rather than der::Input ?

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:435: std::vector<base::StringPiece>*
out_ocsp_uris);
warn_unused_result to match other functions

https://codereview.chromium.org/2036033002/diff/40001/net/data/cert_issuer_so...
File net/data/cert_issuer_source_aia_unittest/generate-certs.py (right):

https://codereview.chromium.org/2036033002/diff/40001/net/data/cert_issuer_so...
net/data/cert_issuer_source_aia_unittest/generate-certs.py:8: sys.path +=
[os.path.join('..', 'verify_certificate_chain_unittest')]
I wonder if we should move this somewhere more generic?

[mattm, 2016-06-03 21:27:09]

Patchset #2 (id:60001) has been deleted

[mattm, 2016-06-03 21:27:27]

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

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:49: // XXX should HTTPS be
disallowed?
On 2016/06/03 16:28:52, eroman wrote:
> XXX --> TODO(...)

Acknowledged.

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:51: if (url.is_valid() &&
url.SchemeIsHTTPOrHTTPS())
On 2016/06/03 16:28:52, eroman wrote:
> We only support http:// for the fetching of AIA [1]
> 
> Shouldn't encode that fetching policy in here, but rather leave it to the
> CertNetFetcher implementation.
> Possible solutions:
>   * Simply pass all URLs to the fetcher and let it fail them if it doesn't
like
>   * Add a synchronous failure mode for FetchCaIssuers()
>   * Add a method to CertNetFetcher's interface similar to
> "SupportsFetchingUrl(GURL)"
> 
> Of those I suggest the first option for this CL.
> The second option might be reasonable, but as a separate change.
> And the third option I don't much care fore.
> 
> [1]
>
https://cs.chromium.org/chromium/src/net/cert_net/cert_net_fetcher_impl.cc?sq...

Agreed. I've done #1 for this CL.

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:54: if (urls.empty())
On 2016/06/03 16:28:52, eroman wrote:
> I think we also should have a failsafe that bounds how many AIA's we will
> consider for a single certificate.
> 
> We can run some numbers to see what is reasonable, but I would imagine 5-10 is
> already more than will be used in practice

Done.

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:100: LOG(ERROR) <<
"AiaRequest::FetchCallback got error " << error;
On 2016/06/03 16:28:52, eroman wrote:
> Please add a TODO here. Rather than swallowing the error, we will want to
> propagate this through.

Done.

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:111:
ParsedCertificate::DataSource::INTERNAL_COPY, &results_)) {
On 2016/06/03 16:28:52, eroman wrote:
> Can you add a TODO to avoid copying the bytes?
> 
> (We can change the CertNetFetcher interface to pass ownership of the bytes
> instead, and also enable passing into ParsedCertificate).

Done.

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:112: LOG(ERROR) << "Error parsing
AIA data";
On 2016/06/03 16:28:52, eroman wrote:
> Also add a TODO here about the error propagation.

Done.

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

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.h:8: #include <unordered_map>
On 2016/06/03 16:28:52, eroman (OOO till June 9) wrote:
> unused

Done.

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.h:52: int pending_requests_ = 0;
On 2016/06/03 16:28:52, eroman (OOO till June 9) wrote:
> can this be unsigned?

yeah. Done.

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

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:57: explicit
Request(RequestManager* manager,
On 2016/06/03 16:28:52, eroman wrote:
> nit: no need for explicit

Done.

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:434: std::vector<base::StringPiece>*
out_ca_issuers_uris,
On 2016/06/03 16:28:52, eroman (OOO till June 9) wrote:
> why stringpiece rather than der::Input ?

These are ASCII string data, so StringPiece seems appropriate.

https://codereview.chromium.org/2036033002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:435: std::vector<base::StringPiece>*
out_ocsp_uris);
On 2016/06/03 16:28:52, eroman (OOO till June 9) wrote:
> warn_unused_result to match other functions

Done.

https://codereview.chromium.org/2036033002/diff/40001/net/data/cert_issuer_so...
File net/data/cert_issuer_source_aia_unittest/generate-certs.py (right):

https://codereview.chromium.org/2036033002/diff/40001/net/data/cert_issuer_so...
net/data/cert_issuer_source_aia_unittest/generate-certs.py:8: sys.path +=
[os.path.join('..', 'verify_certificate_chain_unittest')]
On 2016/06/03 16:28:52, eroman (OOO till June 9) wrote:
> I wonder if we should move this somewhere more generic?

Probably. I'm not sure where though.

[eroman, 2016-06-04 01:08:48]

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
File net/cert/internal/cert_issuer_source_aia.cc (right):

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:102:
cert_fetcher_requests_.push_back(std::move(cert_fetcher_request));
Should this DCHECK that cert_fetcher_request != nullptr?

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:106: void
CertIssuerSourceAia::AiaRequest::FetchCallback(
optional nit: Might I suggest OnFetchCompleted() or some such thing?

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:109: DCHECK(pending_requests_ > 0);
nit: DCHECK_GT

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:122: // TODO(mattm): Is supporting
CMS message format important?
My expectation is no, but TODO sgtm until we can document the reason.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:139: (current_result_ <
results_.size() || pending_requests_ == 0)) {
I suggest extracting this to a helper. Maybe something like:

bool HasNext() const {
  return current_result_ < results_.size();
}

(By my count there are at least 3 places that could use it).

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
File net/cert/internal/cert_issuer_source_aia.h (right):

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.h:32: class AiaRequest : public Request
{
Can you move this into the .cc file?

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.h:50: unsigned pending_requests_ = 0;
size_t ?

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
File net/cert/internal/cert_issuer_source_aia_unittest.cc (right):

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:53: class RequestManager {
Please add a comment for this class.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:69: ~RequestManager() {
CHECK_EQ(nullptr, request_); }
ASSERT_FALSE ?

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:73: CHECK_EQ(nullptr,
request_);
EXPECT_FALSE() ?

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:113:
CHECK(request_map_.find(url) == request_map_.end());
I think the ASSERT_* / EXPECT_* stuff is preferable to CHECK in unit tests
(provided doesn't lead to a subsequent crash anyway)

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:143: // std::map<GURL,
RequestImpl*> request_map_;
uncomment or delete.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:159:
TEST(CertIssuerSourceAiaUnittest, NoSyncResults) {
nit: Rather than suffix of Unittest --> suffix of Test is more common.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:599: // AuthorityInfoAccess
with a single HTTP url pointing to a single DER cert,
nice set of test cases btw!

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:659: OK,
std::vector<uint8_t>(data, data + arraysize(data)));
i believe you can use uniform initialization notation now, and just {1, 2, 3, 4,
5} inline.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:421: // |out_ca_issuers_uris| is filled
with the accessLocations of type
note: it is unclear from this whether the intent is to append or replace. My
reading of "fill" is that it replaces the contents, but the implementation does
an append.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/parse...
File net/cert/internal/parsed_certificate.h (right):

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/parse...
net/cert/internal/parsed_certificate.h:161: // Returns any caIssuers URIs from
the AuthorityInfoAccess extension.
nit: space after closing bracket (and elsewhere). Not sure if the style guide
requires this, but reads more easily for me and I think it is more common in our
code, but feel free to challenge.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/parse...
net/cert/internal/parsed_certificate.h:165: // Returns any ocsp URIs from the
AuthorityInfoAccess extension.
nit: ocsp --> OCSP

[mattm, 2016-06-04 03:21:27]

Patchset #4 (id:120001) has been deleted

[mattm, 2016-06-04 03:24:09]

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
File net/cert/internal/cert_issuer_source_aia.cc (right):

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:102:
cert_fetcher_requests_.push_back(std::move(cert_fetcher_request));
On 2016/06/04 01:08:47, eroman (OOO till June 9) wrote:
> Should this DCHECK that cert_fetcher_request != nullptr?

Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:106: void
CertIssuerSourceAia::AiaRequest::FetchCallback(
On 2016/06/04 01:08:47, eroman (OOO till June 9) wrote:
> optional nit: Might I suggest OnFetchCompleted() or some such thing?

Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:109: DCHECK(pending_requests_ > 0);
On 2016/06/04 01:08:47, eroman (OOO till June 9) wrote:
> nit: DCHECK_GT

Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:122: // TODO(mattm): Is supporting
CMS message format important?
On 2016/06/04 01:08:47, eroman (OOO till June 9) wrote:
> My expectation is no, but TODO sgtm until we can document the reason.

Acknowledged.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.cc:139: (current_result_ <
results_.size() || pending_requests_ == 0)) {
On 2016/06/04 01:08:47, eroman (OOO till June 9) wrote:
> I suggest extracting this to a helper. Maybe something like:
> 
> bool HasNext() const {
>   return current_result_ < results_.size();
> }
> 
> (By my count there are at least 3 places that could use it).

Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
File net/cert/internal/cert_issuer_source_aia.h (right):

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.h:32: class AiaRequest : public Request
{
On 2016/06/04 01:08:47, eroman (OOO till June 9) wrote:
> Can you move this into the .cc file?

Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia.h:50: unsigned pending_requests_ = 0;
On 2016/06/04 01:08:47, eroman (OOO till June 9) wrote:
> size_t ?

oh yeah, that would make sense since they're getting stuck in a vector. done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
File net/cert/internal/cert_issuer_source_aia_unittest.cc (right):

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:53: class RequestManager {
On 2016/06/04 01:08:48, eroman (OOO till June 9) wrote:
> Please add a comment for this class.

Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:69: ~RequestManager() {
CHECK_EQ(nullptr, request_); }
On 2016/06/04 01:08:48, eroman (OOO till June 9) wrote:
> ASSERT_FALSE ?

If the request is still alive it'll try to access the RequestManager in its
destructor, so I figure CHECK is better than getting random memory corruption
later. Changed it to CHECK(!request_) though.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:73: CHECK_EQ(nullptr,
request_);
On 2016/06/04 01:08:47, eroman (OOO till June 9) wrote:
> EXPECT_FALSE() ?

Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:113:
CHECK(request_map_.find(url) == request_map_.end());
On 2016/06/04 01:08:48, eroman (OOO till June 9) wrote:
> I think the ASSERT_* / EXPECT_* stuff is preferable to CHECK in unit tests
> (provided doesn't lead to a subsequent crash anyway)

Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:143: // std::map<GURL,
RequestImpl*> request_map_;
On 2016/06/04 01:08:48, eroman (OOO till June 9) wrote:
> uncomment or delete.

Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:159:
TEST(CertIssuerSourceAiaUnittest, NoSyncResults) {
On 2016/06/04 01:08:47, eroman (OOO till June 9) wrote:
> nit: Rather than suffix of Unittest --> suffix of Test is more common.

Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:599: // AuthorityInfoAccess
with a single HTTP url pointing to a single DER cert,
On 2016/06/04 01:08:47, eroman (OOO till June 9) wrote:
> nice set of test cases btw!

Thanks :)

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/cert_...
net/cert/internal/cert_issuer_source_aia_unittest.cc:659: OK,
std::vector<uint8_t>(data, data + arraysize(data)));
On 2016/06/04 01:08:47, eroman (OOO till June 9) wrote:
> i believe you can use uniform initialization notation now, and just {1, 2, 3,
4,
> 5} inline.

Ah, cool. Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/parse...
File net/cert/internal/parse_certificate.h (right):

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/parse...
net/cert/internal/parse_certificate.h:421: // |out_ca_issuers_uris| is filled
with the accessLocations of type
On 2016/06/04 01:08:48, eroman (OOO till June 9) wrote:
> note: it is unclear from this whether the intent is to append or replace. My
> reading of "fill" is that it replaces the contents, but the implementation
does
> an append.

Changed the function to clear them first.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/parse...
File net/cert/internal/parsed_certificate.h (right):

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/parse...
net/cert/internal/parsed_certificate.h:161: // Returns any caIssuers URIs from
the AuthorityInfoAccess extension.
On 2016/06/04 01:08:48, eroman (OOO till June 9) wrote:
> nit: space after closing bracket (and elsewhere). Not sure if the style guide
> requires this, but reads more easily for me and I think it is more common in
our
> code, but feel free to challenge.

Done.

https://codereview.chromium.org/2036033002/diff/80001/net/cert/internal/parse...
net/cert/internal/parsed_certificate.h:165: // Returns any ocsp URIs from the
AuthorityInfoAccess extension.
On 2016/06/04 01:08:48, eroman (OOO till June 9) wrote:
> nit: ocsp --> OCSP

Done.

[mattm, 2016-06-07 23:37:29]

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

[mattm, 2016-06-07 23:37:29]

+Ryan in case you wanted to look at it. Hadn't actually added you before.

[eroman, 2016-06-14 00:22:22]

lgtm

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_aia.cc (right):

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_aia.cc:32: void OnFetchCompleted(Error,
const std::vector<uint8_t>&);
nit: I believe the chrome style is to include the variable names, always.

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_aia.cc:123: return;
Should we reset out_req  for these various no-result cases?

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_aia.h (right):

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_aia.h:19: // CertIssuerSourceAia. 
CertIssuerSourceAia must be created and used only on
nit: can this be rephrased to avoid speaking about a "network thread" ?

For instance by saying it lives on a single thread, or the CertnetFetcher is
operated from that thread.
Or alternately expressing in the code by inheriting NonThreadSafe or using
ThreadChecker.

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_aia_unittest.cc (right):

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_aia_unittest.cc:84: bool is_request_alive()
const { return request_; }
fyi: may need to write this as request_ != null (i think msvc warns on implicit
bool conversions).

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_aia_unittest.cc:187: // If the
AuthorityInfoAccess extension only contains non-http URIs,
nit: non-HTTP

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
File net/data/verify_certificate_chain_unittest/common.py (right):

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
net/data/verify_certificate_chain_unittest/common.py:75: def __init__(self,
name, cert_type, issuer, key_from=None):
Rather than making this an optional parameter to init, can it be a helper
function?

For instance:
  set_key_path(path)

Or is it needed during initialization?

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
net/data/verify_certificate_chain_unittest/common.py:81: # If specified, use
they key from the given Certificate object instead of
they --> the

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
net/data/verify_certificate_chain_unittest/common.py:140: ['openssl', 'genrsa',
'-out', self.get_key_path(), str(size_bits)])
Can you add some safety-measures to prevent over-writing a key file that is not
"owned" by this certificate?

Maybe an assertion that key_from is None.

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
net/data/verify_certificate_chain_unittest/common.py:147: ['openssl', 'ecparam',
'-out', self.get_key_path(),
same here.

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
net/data/verify_certificate_chain_unittest/common.py:225: if not
os.path.isfile(self.get_key_path()):
Can you similarly add a safety measure here?

(Or maybe we just need to re-work this abstraction and have the key be a passed
in explicit dependency to the Certificate, leaving generation and key selection
entirely up to caller).

WDYT?

[mattm, 2016-06-14 21:37:30]

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_aia.cc (right):

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_aia.cc:32: void OnFetchCompleted(Error,
const std::vector<uint8_t>&);
On 2016/06/14 00:22:21, eroman wrote:
> nit: I believe the chrome style is to include the variable names, always.

Done.

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_aia.cc:123: return;
On 2016/06/14 00:22:21, eroman wrote:
> Should we reset out_req  for these various no-result cases?

Done.

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_aia.h (right):

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_aia.h:19: // CertIssuerSourceAia. 
CertIssuerSourceAia must be created and used only on
On 2016/06/14 00:22:21, eroman wrote:
> nit: can this be rephrased to avoid speaking about a "network thread" ?
> 
> For instance by saying it lives on a single thread, or the CertnetFetcher is
> operated from that thread.
> Or alternately expressing in the code by inheriting NonThreadSafe or using
> ThreadChecker.

Done.

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
File net/cert/internal/cert_issuer_source_aia_unittest.cc (right):

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_aia_unittest.cc:84: bool is_request_alive()
const { return request_; }
On 2016/06/14 00:22:22, eroman wrote:
> fyi: may need to write this as request_ != null (i think msvc warns on
implicit
> bool conversions).

Ack. Trybots were happy so I'll see what happens..

https://codereview.chromium.org/2036033002/diff/160001/net/cert/internal/cert...
net/cert/internal/cert_issuer_source_aia_unittest.cc:187: // If the
AuthorityInfoAccess extension only contains non-http URIs,
On 2016/06/14 00:22:22, eroman wrote:
> nit: non-HTTP

Done.

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
File net/data/verify_certificate_chain_unittest/common.py (right):

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
net/data/verify_certificate_chain_unittest/common.py:75: def __init__(self,
name, cert_type, issuer, key_from=None):
On 2016/06/14 00:22:22, eroman wrote:
> Rather than making this an optional parameter to init, can it be a helper
> function?
> 
> For instance:
>   set_key_path(path)

done.

> Or is it needed during initialization?

Doesn't need to be, but that does need to update the path in the config too.

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
net/data/verify_certificate_chain_unittest/common.py:81: # If specified, use
they key from the given Certificate object instead of
On 2016/06/14 00:22:22, eroman wrote:
> they --> the

Done.

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
net/data/verify_certificate_chain_unittest/common.py:140: ['openssl', 'genrsa',
'-out', self.get_key_path(), str(size_bits)])
On 2016/06/14 00:22:22, eroman wrote:
> Can you add some safety-measures to prevent over-writing a key file that is
not
> "owned" by this certificate?
> 
> Maybe an assertion that key_from is None.

Done.

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
net/data/verify_certificate_chain_unittest/common.py:147: ['openssl', 'ecparam',
'-out', self.get_key_path(),
On 2016/06/14 00:22:22, eroman wrote:
> same here.

Done.

https://codereview.chromium.org/2036033002/diff/160001/net/data/verify_certif...
net/data/verify_certificate_chain_unittest/common.py:225: if not
os.path.isfile(self.get_key_path()):
On 2016/06/14 00:22:22, eroman wrote:
> Can you similarly add a safety measure here?

calls generate_rsa_key which has the assert in it, so I don' think it needs to
be checked here too.

> (Or maybe we just need to re-work this abstraction and have the key be a
passed
> in explicit dependency to the Certificate, leaving generation and key
selection
> entirely up to caller).
> 
> WDYT?

I kinda like that, though it would require a bunch of extra boilerplate for all
the existing places that just want the default. Also would require extra work if
we wanted the key filename to match the cert filename for the normal cases. Just
going with set_key_path for now.

[mattm, 2016-06-15 03:18:35]

Patchset #7 (id:200001) has been deleted

[mattm, 2016-06-15 03:21:54]

The CQ bit was checked by mattm@chromium.org

[mattm, 2016-06-15 03:21:55]

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/2036033002/#ps220001
(title: "remove orphaned kw_args change, remove g_cur_path_id change from this
cl")

[commit-bot: I haz the power, 2016-06-15 03:22:18]

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

[commit-bot: I haz the power, 2016-06-15 04:34:40]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2016-06-15 04:34:47]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-15 04:37:31]

Description was changed from

==========
Add CertIssuerSourceAia: authorityInfoAccess fetching for CertPathBuilder.

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

to

==========
Add CertIssuerSourceAia: authorityInfoAccess fetching for CertPathBuilder.

BUG=410574

Committed: https://crrev.com/8d7481465225d87d54154a3c5561983e116b0cf1
Cr-Commit-Position: refs/heads/master@{#399838}
==========

[commit-bot: I haz the power, 2016-06-15 04:37:32]

Patchset 7 (id:??) landed as
https://crrev.com/8d7481465225d87d54154a3c5561983e116b0cf1
Cr-Commit-Position: refs/heads/master@{#399838}