net: add OCSP tests.

net: add OCSP tests.

I was getting increasingly unhappy altering EV and revocation checking
semantics without any tests. We historically haven't had tests because online
revocation checking is inherently flaky so I amended testserver with the
minimum code to be able to sign and vend OCSP responses.

These tests do not test the final EV/CRLSet/revocation checking semantics. They
are intended to be altered in future CLs.

Adding the EV policy is a little messy, but then so is altering the EV
policy database to be mutable in unittests. Since nobody outside of the the
unittests will trust the "Testing CA", the additional OID should be moot.
(The policy OID has been allocated from Google's arc.)

BUG=none
TEST=net_unittests

[Ryan Sleevi, 2012-03-09 22:07:53]

Adam:

I think it's a good idea to add tests, but I have some concerns. I think for the
Python code, it would be good to get a Python reviewer - but I think it would be
good to go back through the code with the Python style guide:

http://google-styleguide.googlecode.com/svn/trunk/pyguide.html

While I know OCSP is very much a low-level detail, and so the systems-approach
to programming is often easiest, it does feel like it hinders readability a bit.
Can I suggest that you split into three files - ASN.1 encoding, OCSP response
generation, and certificate generation - and add some unit tests?

I'd also give a little push back to see if it can't be done in a style that will
also permit us to do AIA chases and CRL fetches. While I know you're not a fan
of CRLs/OCSP, I think the AIA portion would be especially helpful for some of
the SSL test corner cases.

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/test/test_server.cc
File net/test/test_server.cc (right):

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/test/test_server...
net/test/test_server.cc:82: }
Can you rework this not to require these sorts of magic strings?

What's wrong with having lines 399-412 simply look at the certificate type and
appending custom args to test_server.py like
--ocsp-responder=[ok,revoked,invalid] ?

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/tools/testserver...
File net/tools/testserver/testserver.py (right):

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/tools/testserver...
net/tools/testserver/testserver.py:121: name = "OCSPServerThread")
Could you explain why the threading is necessary here? So far TestServer has
avoided any threading.

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/tools/testserver...
net/tools/testserver/testserver.py:1897: handlers, handlers)
Seems inappropriate to pass handlers for the connect, head, and put handlers.

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/tools/testserver...
net/tools/testserver/testserver.py:1991: if options.cert.startswith('__'):
As I mentioned in the C++ code, I strongly feel we should avoid such magic
values, and instead have well-defined command-line options here.

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/url_request/url_...
File net/url_request/url_request_unittest.cc (right):

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/url_request/url_...
net/url_request/url_request_unittest.cc:1453: // have that ability on other
platforms.
For OS X, we should. What errors are you seeing?

https://chromiumcodereview.appspot.com/9663017/diff/1011/.DEPS.git
File .DEPS.git (right):

https://chromiumcodereview.appspot.com/9663017/diff/1011/.DEPS.git#newcode9
.DEPS.git:9: '@fa137f83758e9d18239059bfa66dbb5408f744ef'
?

https://chromiumcodereview.appspot.com/9663017/diff/1011/DEPS
File DEPS (right):

https://chromiumcodereview.appspot.com/9663017/diff/1011/DEPS#newcode8
DEPS:8: "webkit_revision": "110246",
?

https://chromiumcodereview.appspot.com/9663017/diff/1011/net/base/ev_root_ca_...
File net/base/ev_root_ca_metadata.cc (right):

https://chromiumcodereview.appspot.com/9663017/diff/1011/net/base/ev_root_ca_...
net/base/ev_root_ca_metadata.cc:295: },
Can we not do something like we do for TestRootCerts here? I think that would
avoid any chance of affecting release binaries.

https://chromiumcodereview.appspot.com/9663017/diff/1011/net/base/ev_root_ca_...
net/base/ev_root_ca_metadata.cc:372: 
eg:

bool EVRootCAMetadata::IsEVPolicyOID(PolicyOID policy_oid) const {
  if (TestRootCerts::HasInstance() &&
TestRooCerts::GetInstance()->IsEVPolicyOID(policy_oid)) {
    return true;
  }
  return IsIVPolicyInternal(policy_oid)
}

https://chromiumcodereview.appspot.com/9663017/diff/1011/net/base/ev_root_ca_...
net/base/ev_root_ca_metadata.cc:374: PolicyOID policy_oid) const {
eg:

if (TestRootCerts::HasInstance() &&
TestRootCerts::GetInstance()->HasEVPolicyOID(fingerprint, oid)) {
  return true;
}

https://chromiumcodereview.appspot.com/9663017/diff/1011/net/base/transport_s...
File net/base/transport_security_state.cc (left):

https://chromiumcodereview.appspot.com/9663017/diff/1011/net/base/transport_s...
net/base/transport_security_state.cc:1447: DOMAIN_NOT_PINNED },
Unrelated?

https://chromiumcodereview.appspot.com/9663017/diff/1011/net/base/transport_s...
File net/base/transport_security_state_unittest.cc (left):

https://chromiumcodereview.appspot.com/9663017/diff/1011/net/base/transport_s...
net/base/transport_security_state_unittest.cc:821:
EXPECT_TRUE(ShouldRedirect("foo.bigshinylock.minazo.net"));
?

[agl, 2012-03-13 22:24:28]

I've split the ASN.1 Python code off into it's own file but haven't split the
certificate and OCSP generation. They're very much linked. I think that it
should be easy to add AIA chasing tests in the future but, for now, I'd like to
keep it simple.

The Python style guide you linked to is just wrong in several important
respects. By wrong I mean that it doesn't document the Python style that Google
actually uses and I'm not sure where it came from. See any internal, Google
Python code.

There is quite a chain of patches inflight ahead of this now, but I think that
this can be reviewed in isolation.

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/test/test_server.cc
File net/test/test_server.cc (right):

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/test/test_server...
net/test/test_server.cc:82: }
On 2012/03/09 22:07:53, Ryan Sleevi wrote:
> Can you rework this not to require these sorts of magic strings?

I had to split the certificate and key filename from the --https option to do
this. See how you like it.

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/tools/testserver...
File net/tools/testserver/testserver.py (right):

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/tools/testserver...
net/tools/testserver/testserver.py:121: name = "OCSPServerThread")
On 2012/03/09 22:07:53, Ryan Sleevi wrote:
> Could you explain why the threading is necessary here? So far TestServer has
> avoided any threading.

For the OCSP tests we need testserver to listen for both HTTPS and HTTP
connections (the latter is for the OCSP fetches). I'm not a Python person, but I
don't believe that BaseHTTPServer uses a message loop that can easily be hooked.

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/tools/testserver...
net/tools/testserver/testserver.py:1897: handlers, handlers)
On 2012/03/09 22:07:53, Ryan Sleevi wrote:
> Seems inappropriate to pass handlers for the connect, head, and put handlers.

Done.

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/tools/testserver...
net/tools/testserver/testserver.py:1991: if options.cert.startswith('__'):
On 2012/03/09 22:07:53, Ryan Sleevi wrote:
> As I mentioned in the C++ code, I strongly feel we should avoid such magic
> values, and instead have well-defined command-line options here.

Done.

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/url_request/url_...
File net/url_request/url_request_unittest.cc (right):

https://chromiumcodereview.appspot.com/9663017/diff/5001/net/url_request/url_...
net/url_request/url_request_unittest.cc:1453: // have that ability on other
platforms.
On 2012/03/09 22:07:53, Ryan Sleevi wrote:
> For OS X, we should. What errors are you seeing?

I think the test passed on Mac when I tried it. Certainly I'll try it again
before landing, but Mac doesn't appear to set
CERT_STATUS_UNABLE_TO_CHECK_REVOCATION.

[Ryan Sleevi, 2012-03-13 23:06:39]

Sorry, you're right - the Chromium style is PEP-8 + two spaces (instead of 4) +
MixedCase for methods/functions

http://dev.chromium.org/developers/coding-style

Certainly, looking through the code in GYP, src/tools, and build/, the code
there definitely looks internally consistent and with much more documentation
and verbosity than included here. I would defer to one of the Pythonic people -
I tend to defer to Nico Weber/Marc-Antoine Ruel, but that is by no means
everyone.

While not trying to bikeshed, it also seems wrong to hardcode in the
public/private keypair in the Python itself.

Did you mean to include the changes to net/ocsp/nss_ocsp.cc ? They seem
unrelated, but I'm curious if they're related to some bug you discovered with
these tests. It certainly looks like there are typos/bugs in it, so I wasn't
sure if it was just WIP.

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/ocsp/nss_ocsp.cc
File net/ocsp/nss_ocsp.cc (right):

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/ocsp/nss_ocsp.c...
net/ocsp/nss_ocsp.cc:526: while (!requests_.empty()) {
This is surely wrong - the swap on line 522 should have requests_.empty() always
be true.

Further, I'm not sure how the original code was bugged, since mutating
|requests_| would be fine, since we swapped on line 528 (original) and only
iterate the stack copy (line 530).

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
File net/tools/testserver/minica.py (right):

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/minica.py:49: prefix =
'3021300906052b0e03021a05000414'.decode('hex')
nit: This should be some form of constant.

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/minica.py:149: 
As should all of these

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/minica.py:167: sha1_with_rsa_encryption = asn1.OID([1, 2,
840, 113549, 1, 1, 5])
And these, renamed in ALL_CAPS per the style.

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
File net/tools/testserver/testserver.py (right):

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/testserver.py:1999: print 'OCSP server on port ' +
str(ocsp_server.server_port)
nit: 
print 'OCSP server started on %s:%d' % (host, ocsp_server.server_port)

(Per the other 'server started' prints)

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/testserver.py:2018: ocsp_url = "http://127.0.0.1:%d/ocsp" %
ocsp_server.server_port,
bug? 127.0.0.1 -> host

We've avoided hardcoding in localhost into testserver.py - I've certainly used
it to test inter-machine behaviours, so it'd be good to keep this keyed on
'host'

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/testserver.py:2158: 'automatically generated certificate.
One of of '
nit: 'of of' -> 'of'

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/url_request/url...
File net/url_request/url_request_unittest.cc (right):

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/url_request/url...
net/url_request/url_request_unittest.cc:1389: // This is the policy OID
contained in the certificates that testserver
nit: testserver -> test server

[agl, 2012-03-13 23:44:03]

Hardcoding the key into testserver.py is a little unfortunate. But loading it
(and the cert) from disk means more complexity and more for the next person to
understand. If it becomes worthwhile in the future then it's doable, but I don't
think it's worthwhile at the moment.

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/ocsp/nss_ocsp.cc
File net/ocsp/nss_ocsp.cc (right):

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/ocsp/nss_ocsp.c...
net/ocsp/nss_ocsp.cc:526: while (!requests_.empty()) {
On 2012/03/13 23:06:39, Ryan Sleevi wrote:
> This is surely wrong - the swap on line 522 should have requests_.empty()
always
> be true.

Yes, sorry. That was a slip with git add -p, it shouldn't have been staged. This
was me misunderstanding what was going wrong. The rest of the changes in this
file are the bug fix. In short:

1) We would grab the MessageLoop at construction time, which broke if we did an
Init/Shutdown several times with a different MessageLoop.

2) The code in RemoveRequest to ignore removals at shutdown time would mean that
they were left in requests_ and triggered a use-after-free when we shut it down
again.

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
File net/tools/testserver/minica.py (right):

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/minica.py:49: prefix =
'3021300906052b0e03021a05000414'.decode('hex')
On 2012/03/13 23:06:39, Ryan Sleevi wrote:
> nit: This should be some form of constant.

Done.

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/minica.py:149: 
On 2012/03/13 23:06:39, Ryan Sleevi wrote:
> As should all of these

They are constants, but I've renamed them ALL_CAPS.

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/minica.py:167: sha1_with_rsa_encryption = asn1.OID([1, 2,
840, 113549, 1, 1, 5])
On 2012/03/13 23:06:39, Ryan Sleevi wrote:
> And these, renamed in ALL_CAPS per the style.

Done.

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
File net/tools/testserver/testserver.py (right):

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/testserver.py:1999: print 'OCSP server on port ' +
str(ocsp_server.server_port)
On 2012/03/13 23:06:39, Ryan Sleevi wrote:
> nit: 
> print 'OCSP server started on %s:%d' % (host, ocsp_server.server_port)
> 
> (Per the other 'server started' prints)

Done.

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/testserver.py:2018: ocsp_url = "http://127.0.0.1:%d/ocsp" %
ocsp_server.server_port,
On 2012/03/13 23:06:39, Ryan Sleevi wrote:
> bug? 127.0.0.1 -> host
> 
> We've avoided hardcoding in localhost into testserver.py - I've certainly used
> it to test inter-machine behaviours, so it'd be good to keep this keyed on
> 'host'

Done.

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/tools/testserve...
net/tools/testserver/testserver.py:2158: 'automatically generated certificate.
One of of '
On 2012/03/13 23:06:39, Ryan Sleevi wrote:
> nit: 'of of' -> 'of'

Done.

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/url_request/url...
File net/url_request/url_request_unittest.cc (right):

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/url_request/url...
net/url_request/url_request_unittest.cc:1389: // This is the policy OID
contained in the certificates that testserver
On 2012/03/13 23:06:39, Ryan Sleevi wrote:
> nit: testserver -> test server

It really is called testserver.

[Ryan Sleevi, 2012-03-13 23:50:19]

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/ocsp/nss_ocsp.cc
File net/ocsp/nss_ocsp.cc (right):

https://chromiumcodereview.appspot.com/9663017/diff/12001/net/ocsp/nss_ocsp.c...
net/ocsp/nss_ocsp.cc:526: while (!requests_.empty()) {
On 2012/03/13 23:44:03, agl wrote:
> On 2012/03/13 23:06:39, Ryan Sleevi wrote:
> > This is surely wrong - the swap on line 522 should have requests_.empty()
> always
> > be true.
> 
> Yes, sorry. That was a slip with git add -p, it shouldn't have been staged.
This
> was me misunderstanding what was going wrong. The rest of the changes in this
> file are the bug fix. In short:
> 
> 1) We would grab the MessageLoop at construction time, which broke if we did
an
> Init/Shutdown several times with a different MessageLoop.

For unit tests that are affected by such globals, you're supposed to construct a
base::ShadowingAtExitManager within the test (fixture or individual test).

This should allow you to avoid most of the changes, mod this one.

> 2) The code in RemoveRequest to ignore removals at shutdown time would mean
that
> they were left in requests_ and triggered a use-after-free when we shut it
down
> again.

Is this unnecessary if using a ShadowingAtExitManager?

I'm just trying to figure out where the root bug is.

[agl, 2012-03-13 23:54:36]

On Tue, Mar 13, 2012 at 7:50 PM,  <rsleevi@chromium.org> wrote:
> For unit tests that are affected by such globals, you're supposed to
> construct a base::ShadowingAtExitManager within the test (fixture or
> individual test).
>
> This should allow you to avoid most of the changes, mod this one.

A ShadowingAtExitManager probably doesn't help (although I've not
tried it) because the OCSPIOLoop is leaky:

base::LazyInstance<OCSPIOLoop>::Leaky
    g_ocsp_io_loop = LAZY_INSTANCE_INITIALIZER;

In any case, since we have a Shutdown call I'd rather make it work.


Cheers

AGL

[Ryan Sleevi, 2012-03-20 18:51:08]

agl: Looks like you landed in http://crrev.com/127680 - should this review be
closed?

[agl, 2012-03-20 18:52:36]

On 2012/03/20 18:51:08, Ryan Sleevi wrote:
> agl: Looks like you landed in http://crrev.com/127680 - should this review be
> closed?

Yep, sorry. With the reverts and such it didn't happen automatically.