Introduce unit test for NSS OCSP/AIA fetching

Introduce unit test for NSS OCSP/AIA fetching

BUG=176541
R=erikwright, wtc

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201984

[Ryan Sleevi, 2013-05-15 23:14:17]

Paul: I'm hoping you can take lead on this, given your work with the
URLRequest::Interceptors and friends, and make sure this is the 'right' way to
be hooking the stack. I'm certainly interested in any improvements you can
suggest - especially if I want to be able to start varying test responses. Do I
need to create a new factory/job type for each / introduce
yet-another-singleton?

wtc: Please check the cert generation portion and let me know if you have any
concerns or additional edges to be tested.

[pauljensen, 2013-05-16 13:50:43]

So rather than have anonymous-namespaced global data to get information between
the test and the URLRequestJob creator, I'd recommend creating a
URLRequestJobFactory::ProtocolHandler with
URLRequestFilter::AddHostnameProtocolHandler().  The ProtocolHandler is really
just a URLRequest::ProtocolFactory wrapped in a class, you just implement
MaybeCreateJob() as you would your ProtocolFactory.  The ProtocolFactory way is
essentially deprecated (see how URLRequest::RegisterProtocolFactory() is in a
Deprecated namespace), maybe we should explicitly deprecate
URLFilter::Add{Hostname,Url}Handler.

If you want to add more tests with different responses, you could just construct
new instances of your ProtocolHandler maybe passing the perspective responses to
the constructor.

Alternately, URLRequestPrepackagedInterceptor might work for you though it would
have to be moved from content/ to net/, which would probably just require
breaking it into two classes, one of which is abstracted on the thread.

Alternately, could you just use the testserver and add a .mock-http-headers file
to specify your headers.

https://codereview.chromium.org/15080007/diff/1/net/data/ssl/scripts/generate...
File net/data/ssl/scripts/generate-aia-certs.sh (right):

https://codereview.chromium.org/15080007/diff/1/net/data/ssl/scripts/generate...
net/data/ssl/scripts/generate-aia-certs.sh:12: $@ || exit 1
I prefer "set -e" as it guarantees I didn't forget "try" in one spot; but I'm
cool with your code.

https://codereview.chromium.org/15080007/diff/1/net/ocsp/nss_ocsp_unittest.cc
File net/ocsp/nss_ocsp_unittest.cc (right):

https://codereview.chromium.org/15080007/diff/1/net/ocsp/nss_ocsp_unittest.cc...
net/ocsp/nss_ocsp_unittest.cc:52: request->url().host() != kAiaHost) {
The URLRequestFilter should never use this to handle schemes or hosts other than
what it was registered for, so you could just DCHECK here.

https://codereview.chromium.org/15080007/diff/1/net/ocsp/nss_ocsp_unittest.cc...
net/ocsp/nss_ocsp_unittest.cc:128: int flags =
CertVerifier::VERIFY_CERT_IO_ENABLED;
Any reason you don't pass this directly? (i.e. instead of indirectly through a
variable)

https://codereview.chromium.org/15080007/diff/1/net/ocsp/nss_ocsp_unittest.cc...
net/ocsp/nss_ocsp_unittest.cc:131: test_callback.callback(), &request_handle,
BoundNetLog());
The style guide says one argument per line when the indent-by-4 function call
format is used.

[wtc, 2013-05-16 19:21:13]

Patch set 1 LGTM.

https://codereview.chromium.org/15080007/diff/1/net/data/ssl/certificates/aia...
File net/data/ssl/certificates/aia-root.pem (right):

https://codereview.chromium.org/15080007/diff/1/net/data/ssl/certificates/aia...
net/data/ssl/certificates/aia-root.pem:1: -----BEGIN CERTIFICATE-----

This .pem file is missing the human-readable part.

https://codereview.chromium.org/15080007/diff/1/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/15080007/diff/1/net/net.gyp#newcode1602
net/net.gyp:1602: 'ocsp/nss_ocsp_unittest.cc',

nss_ocsp_unittest.cc is only testing id-ad-caIssuers, not
id-ad-ocsp, so the file name is misleading, unless you plan
to add OCSP tests to the file in the future.

[Ryan Sleevi, 2013-05-16 21:25:38]

So, I'm not sure I fully understood your recommendation for
URLRequestJobFactory. I actually tried to specifically avoid that, based on the
TODO(shalev) that suggests that the JobFactory would be moved into the
implementation file.

Is that comment out of date? Given the Factorys and Handlers in play, it'd be
nice to know clearly what the "one true way" is going forward - is URLFilter
even the right place (I took your comments to be suggesting 'maybe not'?)

https://codereview.chromium.org/15080007/diff/1/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/15080007/diff/1/net/net.gyp#newcode1602
net/net.gyp:1602: 'ocsp/nss_ocsp_unittest.cc',
On 2013/05/16 19:21:13, wtc wrote:
> 
> nss_ocsp_unittest.cc is only testing id-ad-caIssuers, not
> id-ad-ocsp, so the file name is misleading, unless you plan
> to add OCSP tests to the file in the future.

This is intentional, to match the naming of the file - nss_ocsp.h - even though
it handles all of NSS's IO functions.

I recall there was some discussion about this file naming last time, and during
phajdan.jr's moving, but it was decided to keep it the same.

In particular, following the sort of "directory as components" model, we don't
want to stick this into cert/ because we want to have this able to depend on
concepts like URLRequest (which cert/ should not)

https://codereview.chromium.org/15080007/diff/1/net/ocsp/nss_ocsp_unittest.cc
File net/ocsp/nss_ocsp_unittest.cc (right):

https://codereview.chromium.org/15080007/diff/1/net/ocsp/nss_ocsp_unittest.cc...
net/ocsp/nss_ocsp_unittest.cc:52: request->url().host() != kAiaHost) {
On 2013/05/16 13:50:43, pauljensen wrote:
> The URLRequestFilter should never use this to handle schemes or hosts other
than
> what it was registered for, so you could just DCHECK here.

Fair enough. I don't suppose there's a way to setup a filter for *all*
URLRequests, is there? That is, similar how we can intercept all DNS, intercept
all URL requests...

https://codereview.chromium.org/15080007/diff/1/net/ocsp/nss_ocsp_unittest.cc...
net/ocsp/nss_ocsp_unittest.cc:128: int flags =
CertVerifier::VERIFY_CERT_IO_ENABLED;
On 2013/05/16 13:50:43, pauljensen wrote:
> Any reason you don't pass this directly? (i.e. instead of indirectly through a
> variable)

Mostly, consistency with most of our other calls to verify (eg:
cert_verify_proc_unittest and friends), which is largely trying for ease of
readability (particularly when flags is '0')

https://codereview.chromium.org/15080007/diff/1/net/ocsp/nss_ocsp_unittest.cc...
net/ocsp/nss_ocsp_unittest.cc:131: test_callback.callback(), &request_handle,
BoundNetLog());
On 2013/05/16 13:50:43, pauljensen wrote:
> The style guide says one argument per line when the indent-by-4 function call
> format is used.

Could swore I clang-format'd this, but fixed now.

[pauljensen, 2013-05-17 03:45:22]

Ryan, I'm going on vacation for 10 days so my response time may be significant. 
Erik Wright knows about these interfaces too.

https://codereview.chromium.org/15080007/diff/1/net/ocsp/nss_ocsp_unittest.cc
File net/ocsp/nss_ocsp_unittest.cc (right):

https://codereview.chromium.org/15080007/diff/1/net/ocsp/nss_ocsp_unittest.cc...
net/ocsp/nss_ocsp_unittest.cc:52: request->url().host() != kAiaHost) {
On 2013/05/16 21:25:38, Ryan Sleevi wrote:
> On 2013/05/16 13:50:43, pauljensen wrote:
> > The URLRequestFilter should never use this to handle schemes or hosts other
> than
> > what it was registered for, so you could just DCHECK here.
> 
> Fair enough. I don't suppose there's a way to setup a filter for *all*
> URLRequests, is there? That is, similar how we can intercept all DNS,
intercept
> all URL requests...
You can insert the ProtocolHandler into the URLRequestJobFactory rather than
URLRequestFilter.  Some tests already do this; see AddTestInterceptor().

[pauljensen, 2013-05-17 03:58:25]

On 2013/05/16 21:25:38, Ryan Sleevi wrote:
> So, I'm not sure I fully understood your recommendation for
> URLRequestJobFactory. I actually tried to specifically avoid that, based on
the
> TODO(shalev) that suggests that the JobFactory would be moved into the
> implementation file.
> 
> Is that comment out of date? Given the Factorys and Handlers in play, it'd be
> nice to know clearly what the "one true way" is going forward - is URLFilter
> even the right place (I took your comments to be suggesting 'maybe not'?)
URLRequestJobFactory::ProtocolHandler is a really simple interface, really just
one virtual function to implement.  It's meant to be the "one true way" going
forward.  For URLRequestJobFactory::ProtocolHandlers meant to handle *all*
requests for a particular scheme (e.g. http:, ftp:, file:, about:, chrome:,
data: or blob:) they should be added via
URLRequestJobFactoryImpl::SetProtocolHandler() while for tests when they are
meant to just handle some synthetic requests they should be added temporarily
(for the duration of the test) via
URLRequestFilter::AddHostnameProtocolHandler().

That comment about moving the URLRequestJobFactory::ProtocolHandler declaration
into URLRequestJobFactoryImpl is still valid, but this won't change the fact
that many parts of the code base will still use them.  You can think of
URLRequestJobFactory as simply an interface (there are several implementors, not
just URLRequestJobFactoryImpl).  URLRequestJobFactoryImpl is one particular
implementation and it happens to use these ProtocolHandler's to service
requests.  URLFilter happens to use this same ProtocolHandler
definition...really just out of convenience and simplicity, rather than declare
an identical class.

[Ryan Sleevi, 2013-05-18 00:05:51]

Thanks Paul. Moved you to cc (since you're OOO) and added erikwright for review.

[Ryan Sleevi, 2013-05-20 22:02:33]

erikwright: ping?

[erikwright (departed), 2013-05-22 01:18:43]

On 2013/05/20 22:02:33, Ryan Sleevi wrote:
> erikwright: ping?

Ryan, been OOO but looking now.

[erikwright (departed), 2013-05-22 01:29:18]

Usage of ProtocolHandler, JobFactory, etc. LGTM.

https://codereview.chromium.org/15080007/diff/12001/net/ocsp/nss_ocsp_unittes...
File net/ocsp/nss_ocsp_unittest.cc (right):

https://codereview.chromium.org/15080007/diff/12001/net/ocsp/nss_ocsp_unittes...
net/ocsp/nss_ocsp_unittest.cc:86: handler_ = new AiaResponseHandler(kAiaHeaders,
file_contents);
nit: although more verbose, I do find it more correct (and self-documenting) to
assign to a local scoped_ptr, assign local_ptr.get() to the raw member, and then
.Pass() the local ptr to the filter.

The formal reason why it seems better is that over time, with the current
implementation, code might be introduced between lines 86 and 91 that could
cause the pointer to be leaked.

[Ryan Sleevi, 2013-05-22 21:16:47]

https://codereview.chromium.org/15080007/diff/12001/net/ocsp/nss_ocsp_unittes...
File net/ocsp/nss_ocsp_unittest.cc (right):

https://codereview.chromium.org/15080007/diff/12001/net/ocsp/nss_ocsp_unittes...
net/ocsp/nss_ocsp_unittest.cc:86: handler_ = new AiaResponseHandler(kAiaHeaders,
file_contents);
On 2013/05/22 01:29:18, erikwright wrote:
> nit: although more verbose, I do find it more correct (and self-documenting)
to
> assign to a local scoped_ptr, assign local_ptr.get() to the raw member, and
then
> .Pass() the local ptr to the filter.
> 
> The formal reason why it seems better is that over time, with the current
> implementation, code might be introduced between lines 86 and 91 that could
> cause the pointer to be leaked.

Fair point. Updated.

[commit-bot: I haz the power, 2013-05-22 21:16:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/15080007/28001

[commit-bot: I haz the power, 2013-05-22 22:59:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/15080007/37002

[erikwright (departed), 2013-05-22 23:01:12]

https://codereview.chromium.org/15080007/diff/37002/net/ocsp/nss_ocsp_unittes...
File net/ocsp/nss_ocsp_unittest.cc (right):

https://codereview.chromium.org/15080007/diff/37002/net/ocsp/nss_ocsp_unittes...
net/ocsp/nss_ocsp_unittest.cc:84: // Ownership of |handler| is transferred to
the URLRequestFilter, but
Thanks. I think this comment is optional at this point.

[commit-bot: I haz the power, 2013-05-22 23:51:03]

Retried try job too often on ios_dbg_simulator for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-05-24 00:03:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/15080007/69001

[commit-bot: I haz the power, 2013-05-24 06:34:56]

Change committed as 201984