Add OCSPVerifyResult for tracking stapled OCSP responses cross-platform.

Extract OCSPCertStatus::Status to standalone OCSPRevocationStatus, and add OCSPVerifyResult for tracking stapled OCSP responses cross-platform. OCSPVerifyResult is populated by CertVerifyProc, but is currently unused. In the future, it will be consumed by Expect-Staple reports.

This CL also updates mini-CA and the spawned test server to be able to send a wider range of OCSP responses. Since OCSP responses are short lived, test the new functionality in url_request_unittest.cc and dynamically generate OCSP responses.

BUG=598021
Committed: https://crrev.com/612337a1d7cadc52d0217b9f399eb1fab445d3e2
Cr-Commit-Position: refs/heads/master@{#406699}

[dadrian, 2016-06-27 22:43:02]

dadrian@google.com changed reviewers:
+ estark@chromium.org, rsleevi@chromium.org, svaldez@chromium.org

[dadrian, 2016-06-27 22:43:03]

This is the next step in splitting up
https://codereview.chromium.org/2040513003/. It adds the OCSPVerifyResult, and
corresponding tests using minica.py and the scaffolding in
url_request_unittest.cc. It'd be nice to add in a few more tests, but I'm
tossing it up a bit early because of some policy/compatibility issues that came
up while testing. I added some open questions inline that I'm looking for
feedback on, especially from rsleevi@.

https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
File net/tools/testserver/minica.py (right):

https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
net/tools/testserver/minica.py:295: producedAt = thisUpdate
I had to modify producedAt because apparently NSS enforces some level of
recentness of producedAt relative to thisUpdate. I'm not sure how recent it has
to be, just that the previous value was definitely too old, and NSS validation
was failing when thisUpdate and nextUpdate were dynamically set, but producedAt
was not. Probably a question for rsleevi, but do we know what restrictions other
platforms enforce on producedAt? The current date validation ignores it, and I
don't believe any particular policy is specified in RFC 6960.

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9611: class OCSPErrorTestDelegate :
public TestDelegate {
This approach is definitely less than ideal, and needlessly splits up the
OCSPVerify tests into two classes. One class for "successful" responses, and one
class for "failure" responses. This was the first solution svaldez and I came up
with, but I'm curious if anyone else has ideas, especially given the further
issues described below.

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9640:
SpawnedTestServer::SSLOptions::OCSP_OLD, OCSPVerifyResult::INVALID_DATE,
Arguably, this test case should not cause a failure, since the response is old,
and therefore invalid. However, it does fail (at least with NSS). I'm not sure
how other platforms will behave, but I wouldn't be surprised if some will treat
this as GOOD, and some as REVOKED. I can run the CL against the try bots once
the dependent CL lands, but I'm anticipating issues from this.

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9645:
SpawnedTestServer::SSLOptions::OCSP_LONG,
Similar comment about this test case.

[svaldez, 2016-06-29 14:41:23]

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:185: bool CompareCertIDToCertificate(const
OCSPCertID& cert_id,
Might need a different name, since this only returns equality, rather than other
comparisons.

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:189: return serial == cert_id.serial_number;
nit: Swap to match method name.

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:227: if
(!ParseOCSPSingleResponse(single_response_der, &single_response))
Should either of these be setting response_status? Seems like we also want to
keep metrics on "invalid" OCSP responses, even if they don't match the
certificate.

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:242: OCSPCertStatus::Status::GOOD)) {
This really should use the same ordering from parse_ocsp.cc:519. Otherwise
UNKNOWN status will occlude a REVOKED status.

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_re...
File net/cert/cert_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_re...
net/cert/cert_verify_result.h:13: #include "net/cert/ocsp_verify_result.h"
Can you forward seclar OCSPVerifyResult?

https://codereview.chromium.org/2100303002/diff/20001/net/cert/ocsp_verify_re...
File net/cert/ocsp_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/20001/net/cert/ocsp_verify_re...
net/cert/ocsp_verify_result.h:52: // |response_status| = PROVIDED.
*is

Maybe explicitly call out what "strictest" CertStatus means.

https://codereview.chromium.org/2100303002/diff/20001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/2100303002/diff/20001/net/net.gypi#newcode121
net/net.gypi:121: 'cert/ocsp_verify_result.cc',
order.

https://codereview.chromium.org/2100303002/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.cc (left):

https://codereview.chromium.org/2100303002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:784: 
Typically keep the space after conditionals?

https://codereview.chromium.org/2100303002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1304: 
stray format?

https://codereview.chromium.org/2100303002/diff/20001/net/ssl/ssl_info.h
File net/ssl/ssl_info.h (right):

https://codereview.chromium.org/2100303002/diff/20001/net/ssl/ssl_info.h#newc...
net/ssl/ssl_info.h:14: #include "net/cert/ocsp_verify_result.h"
Forward Declare?

https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
File net/tools/testserver/minica.py (right):

https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
net/tools/testserver/minica.py:295: producedAt = thisUpdate
On 2016/06/27 22:43:03, dadrian wrote:
> I had to modify producedAt because apparently NSS enforces some level of
> recentness of producedAt relative to thisUpdate. I'm not sure how recent it
has
> to be, just that the previous value was definitely too old, and NSS validation
> was failing when thisUpdate and nextUpdate were dynamically set, but
producedAt
> was not. Probably a question for rsleevi, but do we know what restrictions
other
> platforms enforce on producedAt? The current date validation ignores it, and I
> don't believe any particular policy is specified in RFC 6960.

From the RFC, the only requirement on producedAt is that it should be within the
validity period of the certificate that signed the response.

Unfortunately it looks like NSS adds a bunch of additional constraints, which
aren't necessarily valid
(https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/certhigh/ocsp...).

We should probably still allow responses where producedAt < thisUpdate, since
pre-signing is allowed by the spec.

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9250: ::testing::AssertionResult
DoConnection(
Still unclear whether you need the return change.

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9611: class OCSPErrorTestDelegate :
public TestDelegate {
On 2016/06/27 22:43:03, dadrian wrote:
> This approach is definitely less than ideal, and needlessly splits up the
> OCSPVerify tests into two classes. One class for "successful" responses, and
one
> class for "failure" responses. This was the first solution svaldez and I came
up
> with, but I'm curious if anyone else has ideas, especially given the further
> issues described below.

Arguably, we might actually want a flag on the cert verifier that doesn't pass
OCSP information into the system verifier, which would prevent certificate
errors as a result of OCSP, though I don't know whether we actually want that
much plumbing.

Alternatively, should OCSP stapling mean we don't cache results, or make sure
the lifetime is only valid within the OCSP lifetime?

[dadrian, 2016-06-30 21:52:43]

Extra bump for rsleevi@'s thoughts on tests

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:185: bool CompareCertIDToCertificate(const
OCSPCertID& cert_id,
On 2016/06/29 14:41:22, svaldez wrote:
> Might need a different name, since this only returns equality, rather than
other
> comparisons.

Done.

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:189: return serial == cert_id.serial_number;
On 2016/06/29 14:41:22, svaldez wrote:
> nit: Swap to match method name.

Done.

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:242: OCSPCertStatus::Status::GOOD)) {
On 2016/06/29 14:41:22, svaldez wrote:
> This really should use the same ordering from parse_ocsp.cc:519. Otherwise
> UNKNOWN status will occlude a REVOKED status.

Done.

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_re...
File net/cert/cert_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/20001/net/cert/cert_verify_re...
net/cert/cert_verify_result.h:13: #include "net/cert/ocsp_verify_result.h"
On 2016/06/29 14:41:22, svaldez wrote:
> Can you forward seclar OCSPVerifyResult?

Not without making it a pointer on line 70.

https://codereview.chromium.org/2100303002/diff/20001/net/cert/ocsp_verify_re...
File net/cert/ocsp_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/20001/net/cert/ocsp_verify_re...
net/cert/ocsp_verify_result.h:52: // |response_status| = PROVIDED.
On 2016/06/29 14:41:22, svaldez wrote:
> *is
> 
> Maybe explicitly call out what "strictest" CertStatus means.

Done.

https://codereview.chromium.org/2100303002/diff/20001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/2100303002/diff/20001/net/net.gypi#newcode121
net/net.gypi:121: 'cert/ocsp_verify_result.cc',
On 2016/06/29 14:41:22, svaldez wrote:
> order.

Done.

https://codereview.chromium.org/2100303002/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.cc (left):

https://codereview.chromium.org/2100303002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:784: 
On 2016/06/29 14:41:22, svaldez wrote:
> Typically keep the space after conditionals?

Done.

https://codereview.chromium.org/2100303002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:1304: 
On 2016/06/29 14:41:22, svaldez wrote:
> stray format?

Done.

https://codereview.chromium.org/2100303002/diff/20001/net/ssl/ssl_info.h
File net/ssl/ssl_info.h (right):

https://codereview.chromium.org/2100303002/diff/20001/net/ssl/ssl_info.h#newc...
net/ssl/ssl_info.h:14: #include "net/cert/ocsp_verify_result.h"
On 2016/06/29 14:41:22, svaldez wrote:
> Forward Declare?

Would need to make |ocsp| a pointer.

https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
File net/tools/testserver/minica.py (right):

https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
net/tools/testserver/minica.py:295: producedAt = thisUpdate
On 2016/06/29 14:41:23, svaldez wrote:
> On 2016/06/27 22:43:03, dadrian wrote:
> > I had to modify producedAt because apparently NSS enforces some level of
> > recentness of producedAt relative to thisUpdate. I'm not sure how recent it
> has
> > to be, just that the previous value was definitely too old, and NSS
validation
> > was failing when thisUpdate and nextUpdate were dynamically set, but
> producedAt
> > was not. Probably a question for rsleevi, but do we know what restrictions
> other
> > platforms enforce on producedAt? The current date validation ignores it, and
I
> > don't believe any particular policy is specified in RFC 6960.
> 
> From the RFC, the only requirement on producedAt is that it should be within
the
> validity period of the certificate that signed the response.
> 
> Unfortunately it looks like NSS adds a bunch of additional constraints, which
> aren't necessarily valid
>
(https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/certhigh/ocsp...).
> 
> We should probably still allow responses where producedAt < thisUpdate, since
> pre-signing is allowed by the spec.

I added a check to make sure notBefore <= producedAt <= notAfter.

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9250: ::testing::AssertionResult
DoConnection(
On 2016/06/29 14:41:23, svaldez wrote:
> Still unclear whether you need the return change.

I got flak for not making this change on another review.

[Ryan Sleevi, 2016-06-30 22:14:51]

Hopefully I addressed the specific questions, but LMK if I missed something

https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
File net/tools/testserver/minica.py (right):

https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
net/tools/testserver/minica.py:295: producedAt = thisUpdate
On 2016/06/29 14:41:23, svaldez wrote:
> We should probably still allow responses where producedAt < thisUpdate, since
> pre-signing is allowed by the spec.

There were some security bugs about this. From an ops side, CAs presigning
creates substantial risk for compromise, so we haven't been too keen to allow a
large window here. I'd have to dig up bugzilla for the details.

For Windows/OS X, I'd have to check the implementation, I don't know off the top
of my head. How pressing is it?

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9250: ::testing::AssertionResult
DoConnection(
On 2016/06/30 21:52:43, dadrian wrote:
> I got flak for not making this change on another review.

It may not have been clear why the change.

It matters if you're doing ASSERTs (which abort the method), as you need to have
a return value that allows them to escape.

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
covers this in more detail, but judging by it, you don't need the
::testing::AssertionResult for any of these; it only becomes an issue if you're
trying to abort early.

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9519: static const OCSPVerifyTestData
kOCSPVerifyData[] = {
We tend to just combine these definitions

static const struct OCSPVerifyTestData {
  ...
} kOCSPVerifyData[] = {
  ...
};

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9611: class OCSPErrorTestDelegate :
public TestDelegate {
On 2016/06/29 14:41:23, svaldez wrote:
> On 2016/06/27 22:43:03, dadrian wrote:
> > This approach is definitely less than ideal, and needlessly splits up the
> > OCSPVerify tests into two classes. One class for "successful" responses, and
> one
> > class for "failure" responses. This was the first solution svaldez and I
came
> up
> > with, but I'm curious if anyone else has ideas, especially given the further
> > issues described below.
> 
> Arguably, we might actually want a flag on the cert verifier that doesn't pass
> OCSP information into the system verifier, which would prevent certificate
> errors as a result of OCSP, though I don't know whether we actually want that
> much plumbing.

I'm not sure I understand what you're proposing?

> Alternatively, should OCSP stapling mean we don't cache results, or make sure
> the lifetime is only valid within the OCSP lifetime?

For disk cache? I would argue we shouldn't cache the OCSP response; we don't
re-verify certs as they come out of the disk cache, so the OCSP status is
irrelevant. Nor do we need to limit the disk cache response (doing so gets to
the more fundamental discussion about whether to re-validate certs as they come
out of the cache, which we don't, but limiting cache lifetime to OCSP lifetime
implies that)

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9611: class OCSPErrorTestDelegate :
public TestDelegate {
On 2016/06/27 22:43:03, dadrian wrote:
> This approach is definitely less than ideal, and needlessly splits up the
> OCSPVerify tests into two classes. One class for "successful" responses, and
one
> class for "failure" responses. This was the first solution svaldez and I came
up
> with, but I'm curious if anyone else has ideas, especially given the further
> issues described below.

Have you traced through with gdb to figure out who is calling this?
https://cs.chromium.org/chromium/src/net/url_request/url_request.h?rcl=146730...
is the magic point on the delegate,
https://cs.chromium.org/chromium/src/net/url_request/url_request.h?rcl=146730...
on the URLRequest

[dadrian, 2016-07-08 22:17:30]

This should now be ready for real review.

https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
File net/tools/testserver/minica.py (right):

https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
net/tools/testserver/minica.py:295: producedAt = thisUpdate
On 2016/06/30 22:14:50, Ryan Sleevi (extremely slow) wrote:
> On 2016/06/29 14:41:23, svaldez wrote:
> > We should probably still allow responses where producedAt < thisUpdate,
since
> > pre-signing is allowed by the spec.
> 
> There were some security bugs about this. From an ops side, CAs presigning
> creates substantial risk for compromise, so we haven't been too keen to allow
a
> large window here. I'd have to dig up bugzilla for the details.
> 
> For Windows/OS X, I'd have to check the implementation, I don't know off the
top
> of my head. How pressing is it?

I mangled these down to one test class that _should_ be platform independent.
We'll see what happens once these hit the try bots after the dependent CL lands.

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9250: ::testing::AssertionResult
DoConnection(
On 2016/06/30 22:14:51, Ryan Sleevi (extremely slow) wrote:
> On 2016/06/30 21:52:43, dadrian wrote:
> > I got flak for not making this change on another review.
> 
> It may not have been clear why the change.
> 
> It matters if you're doing ASSERTs (which abort the method), as you need to
have
> a return value that allows them to escape.
> 
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> covers this in more detail, but judging by it, you don't need the
> ::testing::AssertionResult for any of these; it only becomes an issue if
you're
> trying to abort early.

Reverted, but not sure I follow. You can't use an ASSERT in a function that
returns a ::testing::AssertionResult, just EXCEPT.

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9519: static const OCSPVerifyTestData
kOCSPVerifyData[] = {
On 2016/06/30 22:14:51, Ryan Sleevi (extremely slow) wrote:
> We tend to just combine these definitions
> 
> static const struct OCSPVerifyTestData {
>   ...
> } kOCSPVerifyData[] = {
>   ...
> };

I had kept them separate since the type was used in more than one location, but
this is no longer the case.

https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:9611: class OCSPErrorTestDelegate :
public TestDelegate {
On 2016/06/30 22:14:51, Ryan Sleevi (extremely slow) wrote:
> On 2016/06/27 22:43:03, dadrian wrote:
> > This approach is definitely less than ideal, and needlessly splits up the
> > OCSPVerify tests into two classes. One class for "successful" responses, and
> one
> > class for "failure" responses. This was the first solution svaldez and I
came
> up
> > with, but I'm curious if anyone else has ideas, especially given the further
> > issues described below.
> 
> Have you traced through with gdb to figure out who is calling this?
>
https://cs.chromium.org/chromium/src/net/url_request/url_request.h?rcl=146730...
> is the magic point on the delegate,
>
https://cs.chromium.org/chromium/src/net/url_request/url_request.h?rcl=146730...
> on the URLRequest

I worked around it, but it was getting modified by
https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc....

[dadrian, 2016-07-08 22:18:03]

Description was changed from

==========
Add OCSPVerifyResult for tracking stapled OCSP responses cross-platform.

The result is currently unused, but will be consumed by Expect-Staple reports.

BUG=598021
==========

to

==========
Add OCSPVerifyResult for tracking stapled OCSP responses cross-platform.

The result is currently unused, but will be consumed by Expect-Staple reports.
It is populated by CertVerifyProc.

BUG=598021
==========

[dadrian, 2016-07-13 17:33:06]

The CQ bit was checked by dadrian@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 17:33:50]

Dry run: 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-07-13 18:18:44]

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

[commit-bot: I haz the power, 2016-07-13 18:18:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dadrian, 2016-07-13 18:51:53]

The CQ bit was checked by dadrian@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 18:53:13]

Dry run: 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-07-13 19:31:08]

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

[commit-bot: I haz the power, 2016-07-13 19:31:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dadrian, 2016-07-13 20:04:05]

The CQ bit was checked by dadrian@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 20:06:09]

Dry run: 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-07-13 21:17:08]

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

[commit-bot: I haz the power, 2016-07-13 21:17:09]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dadrian, 2016-07-14 01:39:03]

On 2016/07/08 22:17:30, dadrian wrote:
> This should now be ready for real review.
> 
>
https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
> File net/tools/testserver/minica.py (right):
> 
>
https://codereview.chromium.org/2100303002/diff/20001/net/tools/testserver/mi...
> net/tools/testserver/minica.py:295: producedAt = thisUpdate
> On 2016/06/30 22:14:50, Ryan Sleevi (extremely slow) wrote:
> > On 2016/06/29 14:41:23, svaldez wrote:
> > > We should probably still allow responses where producedAt < thisUpdate,
> since
> > > pre-signing is allowed by the spec.
> > 
> > There were some security bugs about this. From an ops side, CAs presigning
> > creates substantial risk for compromise, so we haven't been too keen to
allow
> a
> > large window here. I'd have to dig up bugzilla for the details.
> > 
> > For Windows/OS X, I'd have to check the implementation, I don't know off the
> top
> > of my head. How pressing is it?
> 
> I mangled these down to one test class that _should_ be platform independent.
> We'll see what happens once these hit the try bots after the dependent CL
lands.
> 
>
https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
> File net/url_request/url_request_unittest.cc (right):
> 
>
https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
> net/url_request/url_request_unittest.cc:9250: ::testing::AssertionResult
> DoConnection(
> On 2016/06/30 22:14:51, Ryan Sleevi (extremely slow) wrote:
> > On 2016/06/30 21:52:43, dadrian wrote:
> > > I got flak for not making this change on another review.
> > 
> > It may not have been clear why the change.
> > 
> > It matters if you're doing ASSERTs (which abort the method), as you need to
> have
> > a return value that allows them to escape.
> > 
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> > covers this in more detail, but judging by it, you don't need the
> > ::testing::AssertionResult for any of these; it only becomes an issue if
> you're
> > trying to abort early.
> 
> Reverted, but not sure I follow. You can't use an ASSERT in a function that
> returns a ::testing::AssertionResult, just EXCEPT.
> 
>
https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
> net/url_request/url_request_unittest.cc:9519: static const OCSPVerifyTestData
> kOCSPVerifyData[] = {
> On 2016/06/30 22:14:51, Ryan Sleevi (extremely slow) wrote:
> > We tend to just combine these definitions
> > 
> > static const struct OCSPVerifyTestData {
> >   ...
> > } kOCSPVerifyData[] = {
> >   ...
> > };
> 
> I had kept them separate since the type was used in more than one location,
but
> this is no longer the case.
> 
>
https://codereview.chromium.org/2100303002/diff/20001/net/url_request/url_req...
> net/url_request/url_request_unittest.cc:9611: class OCSPErrorTestDelegate :
> public TestDelegate {
> On 2016/06/30 22:14:51, Ryan Sleevi (extremely slow) wrote:
> > On 2016/06/27 22:43:03, dadrian wrote:
> > > This approach is definitely less than ideal, and needlessly splits up the
> > > OCSPVerify tests into two classes. One class for "successful" responses,
and
> > one
> > > class for "failure" responses. This was the first solution svaldez and I
> came
> > up
> > > with, but I'm curious if anyone else has ideas, especially given the
further
> > > issues described below.
> > 
> > Have you traced through with gdb to figure out who is calling this?
> >
>
https://cs.chromium.org/chromium/src/net/url_request/url_request.h?rcl=146730...
> > is the magic point on the delegate,
> >
>
https://cs.chromium.org/chromium/src/net/url_request/url_request.h?rcl=146730...
> > on the URLRequest
> 
> I worked around it, but it was getting modified by
>
https://cs.chromium.org/chromium/src/net/http/http_stream_factory_impl_job.cc....

This is now actually compiling and passing tests on all platforms (yay debugging
against the try bots). rsleevi@, going to need review from you, although estark@
and svaldez@ can also chime in :)

[estark, 2016-07-14 21:14:58]

(Oops, my comments span two different patchsets because apparently I had some
from days ago that I had never mailed.)

I think you've told me at least 10 times and I keep forgetting... so let me get
it in writing: why do we need the test server to generate/serve the various OCSP
responses? (That is, why are the unit tests not cert_verify_proc_unittests that
read the OCSP responses from files and pass them in to the Verify()?)

https://codereview.chromium.org/2100303002/diff/180001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2100303002/diff/180001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:347: // Check OCSP information
nit: non-useful comment, I think you could just delete it.

question: I'm wondering if it's correct that we don't call CheckOCSP() in the
event of a blacklisted cert (line 327). I suppose that won't end up mattering
because we plan to only send Expect-Staple reports for certificates that
validate without errors, is that correct? Still, I would lean towards
consistency (either always have verify_result.ocsp populated, or never populate
it in the event of a validation error).

https://codereview.chromium.org/2100303002/diff/180001/net/test/spawned_test_...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/2100303002/diff/180001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.h:93: // testserver can produce
nit: period

https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
File net/cert/ocsp_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:16: struct NET_EXPORT OCSPVerifyResult {
probably could use some documentation on this class

https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:30: // The stapled OCSP response did not have a
SUCCESFUL status.
missing an S in SUCCESSFUL

https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:57: base::Optional<OCSPRevocationStatus>
cert_status;
Hmm, I wonder if this should be called |revocation_status| or
|cert_revocation_status| instead? In reading the code I keep getting it confused
with the other use of |cert_status|.

https://codereview.chromium.org/2100303002/diff/260001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2100303002/diff/260001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:9012: void DoConnection(const
SpawnedTestServer::SSLOptions& ssl_options,
nit: I think overloads are generally discouraged-ish?
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Maybe rename the above to DoConnectionWithDelegate()

[dadrian, 2016-07-15 01:00:34]

The CQ bit was checked by dadrian@google.com to run a CQ dry run

[dadrian, 2016-07-15 01:00:50]

https://codereview.chromium.org/2100303002/diff/180001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2100303002/diff/180001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:347: // Check OCSP information
On 2016/07/14 21:14:58, estark wrote:
> nit: non-useful comment, I think you could just delete it.
> 
Done.

> question: I'm wondering if it's correct that we don't call CheckOCSP() in the
> event of a blacklisted cert (line 327). I suppose that won't end up mattering
> because we plan to only send Expect-Staple reports for certificates that
> validate without errors, is that correct? Still, I would lean towards
> consistency (either always have verify_result.ocsp populated, or never
populate
> it in the event of a validation error).

Hmmm. I'm a fan of consistency, but we also don't do any of the other error
checking when a certificate is blacklisted (e.g. weak keys, name constraints). I
could see almost any approach being rational. /shrug

https://codereview.chromium.org/2100303002/diff/180001/net/test/spawned_test_...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/2100303002/diff/180001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.h:93: // testserver can produce
On 2016/07/14 21:14:58, estark wrote:
> nit: period

Done.

https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
File net/cert/ocsp_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:16: struct NET_EXPORT OCSPVerifyResult {
On 2016/07/14 21:14:58, estark wrote:
> probably could use some documentation on this class

Done.

https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:30: // The stapled OCSP response did not have a
SUCCESFUL status.
On 2016/07/14 21:14:58, estark wrote:
> missing an S in SUCCESSFUL

I'm an adult who can spell things, I swear!

https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:57: base::Optional<OCSPRevocationStatus>
cert_status;
On 2016/07/14 21:14:58, estark wrote:
> Hmm, I wonder if this should be called |revocation_status| or
> |cert_revocation_status| instead? In reading the code I keep getting it
confused
> with the other use of |cert_status|.

It should probably be |revocation_status|. It was called |cert_status| when it
corresponded with the OCSPCertStatus ASN.1 object, but it turns out that's only
_sometimes_ just an enum, because they helpfully attach revocation reason and
timestamp to REVOKED responses. The RFC also doesn't name the enum itself, which
is why in the original OCSP parsing code, it was referred to as a
CertStatus::Status.

tl;dr I'll make my own abstractions! With blackjack and |revocation_status|!

https://codereview.chromium.org/2100303002/diff/260001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2100303002/diff/260001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:9012: void DoConnection(const
SpawnedTestServer::SSLOptions& ssl_options,
On 2016/07/14 21:14:58, estark wrote:
> nit: I think overloads are generally discouraged-ish?
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> 
> Maybe rename the above to DoConnectionWithDelegate()

Done.

[commit-bot: I haz the power, 2016-07-15 01:01:49]

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

[dadrian, 2016-07-15 01:03:37]

On 2016/07/14 21:14:58, estark wrote:
> (Oops, my comments span two different patchsets because apparently I had some
> from days ago that I had never mailed.)
> 
> I think you've told me at least 10 times and I keep forgetting... so let me
get
> it in writing: why do we need the test server to generate/serve the various
OCSP
> responses? (That is, why are the unit tests not cert_verify_proc_unittests
that
> read the OCSP responses from files and pass them in to the Verify()?)

I discussed this with rsleevi@ at length. There's a few reasons:

1. OCSP responses are short-lived, and don't want to have to regenerate them
weekly.
2. We don't want to inject fake times to compensate for expired responses.
3. It's easier to update for policy changes.

Ideally these would use EmbeddedTestServer over SpawnedTestServer, but after
discussing some with svaldez@, Embedded isn't quite there yet.

> 
>
https://codereview.chromium.org/2100303002/diff/180001/net/cert/cert_verify_p...
> File net/cert/cert_verify_proc.cc (right):
> 
>
https://codereview.chromium.org/2100303002/diff/180001/net/cert/cert_verify_p...
> net/cert/cert_verify_proc.cc:347: // Check OCSP information
> nit: non-useful comment, I think you could just delete it.
> 
> question: I'm wondering if it's correct that we don't call CheckOCSP() in the
> event of a blacklisted cert (line 327). I suppose that won't end up mattering
> because we plan to only send Expect-Staple reports for certificates that
> validate without errors, is that correct? Still, I would lean towards
> consistency (either always have verify_result.ocsp populated, or never
populate
> it in the event of a validation error).
> 
>
https://codereview.chromium.org/2100303002/diff/180001/net/test/spawned_test_...
> File net/test/spawned_test_server/base_test_server.h (right):
> 
>
https://codereview.chromium.org/2100303002/diff/180001/net/test/spawned_test_...
> net/test/spawned_test_server/base_test_server.h:93: // testserver can produce
> nit: period
> 
>
https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
> File net/cert/ocsp_verify_result.h (right):
> 
>
https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
> net/cert/ocsp_verify_result.h:16: struct NET_EXPORT OCSPVerifyResult {
> probably could use some documentation on this class
> 
>
https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
> net/cert/ocsp_verify_result.h:30: // The stapled OCSP response did not have a
> SUCCESFUL status.
> missing an S in SUCCESSFUL
> 
>
https://codereview.chromium.org/2100303002/diff/260001/net/cert/ocsp_verify_r...
> net/cert/ocsp_verify_result.h:57: base::Optional<OCSPRevocationStatus>
> cert_status;
> Hmm, I wonder if this should be called |revocation_status| or
> |cert_revocation_status| instead? In reading the code I keep getting it
confused
> with the other use of |cert_status|.
> 
>
https://codereview.chromium.org/2100303002/diff/260001/net/url_request/url_re...
> File net/url_request/url_request_unittest.cc (right):
> 
>
https://codereview.chromium.org/2100303002/diff/260001/net/url_request/url_re...
> net/url_request/url_request_unittest.cc:9012: void DoConnection(const
> SpawnedTestServer::SSLOptions& ssl_options,
> nit: I think overloads are generally discouraged-ish?
>
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> 
> Maybe rename the above to DoConnectionWithDelegate()

[commit-bot: I haz the power, 2016-07-15 02:16:49]

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

[commit-bot: I haz the power, 2016-07-15 02:16:50]

Dry run: This issue passed the CQ dry run.

[svaldez, 2016-07-16 08:51:53]

Mostly looks good, can you call out the OCSPRevocationStatus being moved out of
the parse_ocsp code in the CL description and the OCSP change to the testserver?

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
File net/cert/ocsp_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:64: base::Optional<OCSPRevocationStatus>
revocation_status;
I think there's some disagreement over whether we should be using base::Optional
in net. Might want to check with sleevi?

[dadrian, 2016-07-18 17:21:40]

Description was changed from

==========
Add OCSPVerifyResult for tracking stapled OCSP responses cross-platform.

The result is currently unused, but will be consumed by Expect-Staple reports.
It is populated by CertVerifyProc.

BUG=598021
==========

to

==========
Extract OCSPCertStatus::Status to standalone OCSPRevocationStatus, and add
OCSPVerifyResult for tracking stapled OCSP responses cross-platform.
OCSPVerifyResult is populated by CertVerifyProc, but is currently unused. In the
future, it will be consumed by Expect-Staple reports.

This CL also updates mini-CA and the spawned test server to be able to send a
wider range of OCSP responses. Since OCSP responses are short lived, test the
new functionality in url_request_unittest.cc and dynamically generate OCSP
responses.

BUG=598021
==========

[dadrian, 2016-07-18 17:30:19]

On 2016/07/16 08:51:53, svaldez wrote:
> Mostly looks good, can you call out the OCSPRevocationStatus being moved out
of
> the parse_ocsp code in the CL description and the OCSP change to the
testserver?

Description updated.
> 
>
https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
> File net/cert/ocsp_verify_result.h (right):
> 
>
https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
> net/cert/ocsp_verify_result.h:64: base::Optional<OCSPRevocationStatus>
> revocation_status;
> I think there's some disagreement over whether we should be using
base::Optional
> in net. Might want to check with sleevi?

rsleevi@, thoughts?

[Ryan Sleevi, 2016-07-18 20:08:08]

I largely skipped the test server/unittest code to focus on the implementation
for expediency sake.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:188: bool CheckCertIDMatchesCertificate(const
OCSPCertID& cert_id,
Document

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:195: void CheckOCSP(const std::string&
raw_response,
Document

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:196: CertVerifyResult* verify_result) {
Is CertVerifyResult really the right dependency? It looks like you're using it
to smuggle two parameters - the output OCSP response, and the cert under
inspection.

I'd suggest making those inputs explicit (that is, rather than taking the
CertVerifyResult directly)

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:197: verify_result->ocsp.Reset();
Do we need an explicit reset? Can't you just do a by-val assignment? Is this
just cargo-culted from CertVerifyResult?

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:203: der::Input response_der(&raw_response);
move the newline from 204 to 203 :)

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:207: verify_result->ocsp.response_status =
OCSPVerifyResult::PARSE_RESPONSE;
Is PARSE_RESPONSE a good name for the enum? How does it distinguish from
BAD_RESPONSE?

Should it be PARSE_ERROR?

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:212: // data.
Could you explain the "why" more? Why is this a 'bad' response?

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:219: if (!ParseOCSPResponseData(response.data,
&response_data)) {
This could benefit from documentation, because ParseOCSPResponse and
ParseOCSPResponseData aren't immediately clear about the distinction between
these two. For example, citing spec text (as you can see in the other RFC 5280
validation code) is useful to help refresh readers' context about what's
happening.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:230: verify_result->verified_cert->valid_expiry(),
&not_after)) {
Just to make sure I understand: We parse the certificate validity, and then
re-encode the certificate validity?

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:240: // TODO(svaldez): Unify with
GetOCSPCertStatus.
Is there a Bug #?

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:242: base::TimeDelta max_age =
base::TimeDelta::FromDays(7);
Should this be a function-level static constant so it's clearly documented the
magic age?

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:244: for (const auto& single_response_der :
response_data.responses) {
You could do with some high-level documentation about the logic for matching,
and why it's incomplete here.

For example

for (const auto& single_response_der : response_data.responses) {
  // Although in most cases there should only be a single response (for the
certificate requested), it's
  // possible the OCSP responder provided statuses for multiple certificates.
Look through all the
  // responses to see if there's a response that matches the
|verify_result->verified_cert|'s certificate.
  // This matching is ...

}

etc

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:254: if (verify_result->ocsp.response_status !=
OCSPVerifyResult::PROVIDED)
This could benefit from documentation

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:263: // In the case that we receive multiple
responses, we keep only the
Avoid "we" in comments, especially new code.

// In the event multiple responses are received, keep the strictest status
(REVOKED > UNKNOWN > GOOD)

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_r...
File net/cert/cert_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_r...
net/cert/cert_verify_result.h:72: OCSPVerifyResult ocsp;
NAMING: ocsp_status / ocsp_result ? Simply naming the variable "ocsp" doesn't
feel very descriptive/like a good name.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
File net/cert/ocsp_verify_result.cc (right):

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.cc:14: OCSPVerifyResult::~OCSPVerifyResult() {}
= default

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
File net/cert/ocsp_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:28: void Reset();
See remarks about Reset()

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:60: ResponseStatus response_status;
From an API perspective, I'd argue that the C++11 syntax of

ResponseStatus response_status = MISSING;

In this header represents an acceptable API contract (that is, it's not
"leaking" implementation details into the header, and serves as reasonable
documentation about the default initialization state)

This would allow you to =default in the .cc completely.

The rubric I use is largely borrowed from Lakos' "Large Scale C++ design" -
which is, what's the expected rate of change in this header, and would
everything that depends on it reasonably need to be recompiled if it changed.
The goal is to minimize changes to headers as much as possible, and keep as much
implementation status in the .cc - If it's an implementation detail, dependents
shouldn't have to be recompiled.

I would argue that this (the default = MISSING) represents a reasonable API
contract, it's not something we'd change, and we *would* want things to be
recompiled if we changed the default value, so... go ahead and C++11 it :)

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:64: base::Optional<OCSPRevocationStatus>
revocation_status;
On 2016/07/16 08:51:53, svaldez wrote:
> I think there's some disagreement over whether we should be using
base::Optional
> in net. Might want to check with sleevi?

Aye, I'm a hater of it, because I've generally seen it abused more than helpful.
But it's part of base and it's part of C++, so I am willing to accept I'm in the
minority here.

I'm not sure this really buys us anything by being optional, either from storage
or API clarity.

https://codereview.chromium.org/2100303002/diff/280001/net/ssl/ssl_info.h
File net/ssl/ssl_info.h (right):

https://codereview.chromium.org/2100303002/diff/280001/net/ssl/ssl_info.h#new...
net/ssl/ssl_info.h:150: OCSPVerifyResult ocsp;
same remark re: naming

https://codereview.chromium.org/2100303002/diff/280001/net/test/spawned_test_...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/2100303002/diff/280001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.cc:147: return "";
std::string, not ""

https://codereview.chromium.org/2100303002/diff/280001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.cc:163: return "";
std::string() not ""

https://codereview.chromium.org/2100303002/diff/280001/net/test/spawned_test_...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/2100303002/diff/280001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.h:103: struct SingleResponse {
Naming: OCSPSingleResponse - since this is very much an OCSP concept.

[dadrian, 2016-07-18 22:23:33]

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:188: bool CheckCertIDMatchesCertificate(const
OCSPCertID& cert_id,
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> Document

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:195: void CheckOCSP(const std::string&
raw_response,
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> Document

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:196: CertVerifyResult* verify_result) {
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> Is CertVerifyResult really the right dependency? It looks like you're using it
> to smuggle two parameters - the output OCSP response, and the cert under
> inspection.
> 
> I'd suggest making those inputs explicit (that is, rather than taking the
> CertVerifyResult directly)

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:197: verify_result->ocsp.Reset();
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> Do we need an explicit reset? Can't you just do a by-val assignment? Is this
> just cargo-culted from CertVerifyResult?

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:203: der::Input response_der(&raw_response);
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> move the newline from 204 to 203 :)

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:207: verify_result->ocsp.response_status =
OCSPVerifyResult::PARSE_RESPONSE;
On 2016/07/18 20:08:07, Ryan Sleevi (extremely slow) wrote:
> Is PARSE_RESPONSE a good name for the enum? How does it distinguish from
> BAD_RESPONSE?
> 
> Should it be PARSE_ERROR?

I changed it to PARSE_RESPONSE_ERROR and PARSE_RESPONSE_DATA_ERROR, to
distinguish the two cases, while still making it clear it's a parsing error.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:212: // data.
On 2016/07/18 20:08:07, Ryan Sleevi (extremely slow) wrote:
> Could you explain the "why" more? Why is this a 'bad' response?

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:219: if (!ParseOCSPResponseData(response.data,
&response_data)) {
On 2016/07/18 20:08:07, Ryan Sleevi (extremely slow) wrote:
> This could benefit from documentation, because ParseOCSPResponse and
> ParseOCSPResponseData aren't immediately clear about the distinction between
> these two. For example, citing spec text (as you can see in the other RFC 5280
> validation code) is useful to help refresh readers' context about what's
> happening.

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:230: verify_result->verified_cert->valid_expiry(),
&not_after)) {
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> Just to make sure I understand: We parse the certificate validity, and then
> re-encode the certificate validity?

Aren't computers great? :) 

The other option would be to implement der::EncodeGeneralizedTimeToTime(), but
based on previous experience, this would literally take months.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:240: // TODO(svaldez): Unify with
GetOCSPCertStatus.
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> Is there a Bug #?

There is now. https://crbug.com/629249

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:242: base::TimeDelta max_age =
base::TimeDelta::FromDays(7);
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> Should this be a function-level static constant so it's clearly documented the
> magic age?

Sure? Not sure what the rules are here, but this is buried pretty deep. I'll try
to draw it out.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:244: for (const auto& single_response_der :
response_data.responses) {
On 2016/07/18 20:08:07, Ryan Sleevi (extremely slow) wrote:
> You could do with some high-level documentation about the logic for matching,
> and why it's incomplete here.
> 
> For example
> 
> for (const auto& single_response_der : response_data.responses) {
>   // Although in most cases there should only be a single response (for the
> certificate requested), it's
>   // possible the OCSP responder provided statuses for multiple certificates.
> Look through all the
>   // responses to see if there's a response that matches the
> |verify_result->verified_cert|'s certificate.
>   // This matching is ...
> 
> }
> 
> etc

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:254: if (verify_result->ocsp.response_status !=
OCSPVerifyResult::PROVIDED)
On 2016/07/18 20:08:07, Ryan Sleevi (extremely slow) wrote:
> This could benefit from documentation

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:263: // In the case that we receive multiple
responses, we keep only the
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> Avoid "we" in comments, especially new code.
> 
> // In the event multiple responses are received, keep the strictest status
> (REVOKED > UNKNOWN > GOOD)

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_r...
File net/cert/cert_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/280001/net/cert/cert_verify_r...
net/cert/cert_verify_result.h:72: OCSPVerifyResult ocsp;
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> NAMING: ocsp_status / ocsp_result ? Simply naming the variable "ocsp" doesn't
> feel very descriptive/like a good name.

Done. Switched to |ocsp_result|.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
File net/cert/ocsp_verify_result.cc (right):

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.cc:14: OCSPVerifyResult::~OCSPVerifyResult() {}
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> = default

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
File net/cert/ocsp_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:28: void Reset();
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> See remarks about Reset()

Removed.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:60: ResponseStatus response_status;
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> From an API perspective, I'd argue that the C++11 syntax of
> 
> ResponseStatus response_status = MISSING;
> 
> In this header represents an acceptable API contract (that is, it's not
> "leaking" implementation details into the header, and serves as reasonable
> documentation about the default initialization state)
> 
> This would allow you to =default in the .cc completely.
> 
> The rubric I use is largely borrowed from Lakos' "Large Scale C++ design" -
> which is, what's the expected rate of change in this header, and would
> everything that depends on it reasonably need to be recompiled if it changed.
> The goal is to minimize changes to headers as much as possible, and keep as
much
> implementation status in the .cc - If it's an implementation detail,
dependents
> shouldn't have to be recompiled.
> 
> I would argue that this (the default = MISSING) represents a reasonable API
> contract, it's not something we'd change, and we *would* want things to be
> recompiled if we changed the default value, so... go ahead and C++11 it :)

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:64: base::Optional<OCSPRevocationStatus>
revocation_status;
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> On 2016/07/16 08:51:53, svaldez wrote:
> > I think there's some disagreement over whether we should be using
> base::Optional
> > in net. Might want to check with sleevi?
> 
> Aye, I'm a hater of it, because I've generally seen it abused more than
helpful.
> But it's part of base and it's part of C++, so I am willing to accept I'm in
the
> minority here.
> 
> I'm not sure this really buys us anything by being optional, either from
storage
> or API clarity.

Works pretty well with no optional and just a value.

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/test/spawned_test_...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/2100303002/diff/280001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.cc:147: return "";
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> std::string, not ""

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.cc:163: return "";
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> std::string() not ""

Done.

https://codereview.chromium.org/2100303002/diff/280001/net/test/spawned_test_...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/2100303002/diff/280001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.h:103: struct SingleResponse {
On 2016/07/18 20:08:08, Ryan Sleevi (extremely slow) wrote:
> Naming: OCSPSingleResponse - since this is very much an OCSP concept.

Done.

Although now the full type is SpawnedTestServer::SSLOptions::OCSPSingleResponse,
which is a bit of mouthful.

[Ryan Sleevi, 2016-07-18 22:56:37]

LGTM

*super* good work on the documentation. Made it so much easier to follow and
parse the code :)

https://codereview.chromium.org/2100303002/diff/300001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2100303002/diff/300001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:285: // The SingleResponse matches the certificate,
but may be out of date.  Out
s/  / / (single space)

https://codereview.chromium.org/2100303002/diff/300001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:286: // of date responses are matching responses
are noted seperate from
grammar: are matching response are ?

https://codereview.chromium.org/2100303002/diff/300001/net/cert/ocsp_verify_r...
File net/cert/ocsp_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/300001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:10: #include "base/optional.h"
unneeded

https://codereview.chromium.org/2100303002/diff/300001/net/test/spawned_test_...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/2100303002/diff/300001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.h:101: // SingleResponse is used
when specifying multiple stapled responses, each
s/SingleResponse/OCSPSingleResponse/

https://codereview.chromium.org/2100303002/diff/300001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2100303002/diff/300001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:9260: OCSPVerifyResult::ResponseStatus
response_status;
It's arguably more typing, but if it makes you feel any better, you could also
do

using OCSPSingleResponse = SpawnedTestServer::SSLOptions::OCSPSingleResponse;
using OCSPProduced = SpawnedTestServer::SSLOptions::OCSPProduced;
using ResponseStatus = OCSPVerifyResult::ResponseStatus

std::vector<OCSPSingleResponse> ocsp_responses;

(etc)

https://codereview.chromium.org/2100303002/diff/300001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:9488: virtual ~HTTPSOCSPVerifyTest() {}
Pretty sure you can omit both of these, so that it's just

...
public testing::WithParamInterface<OCSPVerifyTestData> {
};

https://codereview.chromium.org/2100303002/diff/300001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:9516: }
omit braces

[dadrian, 2016-07-18 23:20:26]

https://codereview.chromium.org/2100303002/diff/300001/net/cert/cert_verify_p...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2100303002/diff/300001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:285: // The SingleResponse matches the certificate,
but may be out of date.  Out
On 2016/07/18 22:56:37, Ryan Sleevi (extremely slow) wrote:
> s/  / / (single space)

Done.

Vim auto-line-breaker has a love of extra spaces because it hates people.

https://codereview.chromium.org/2100303002/diff/300001/net/cert/cert_verify_p...
net/cert/cert_verify_proc.cc:286: // of date responses are matching responses
are noted seperate from
On 2016/07/18 22:56:37, Ryan Sleevi (extremely slow) wrote:
> grammar: are matching response are ?

Done.

https://codereview.chromium.org/2100303002/diff/300001/net/cert/ocsp_verify_r...
File net/cert/ocsp_verify_result.h (right):

https://codereview.chromium.org/2100303002/diff/300001/net/cert/ocsp_verify_r...
net/cert/ocsp_verify_result.h:10: #include "base/optional.h"
On 2016/07/18 22:56:37, Ryan Sleevi (extremely slow) wrote:
> unneeded

Done.

https://codereview.chromium.org/2100303002/diff/300001/net/test/spawned_test_...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/2100303002/diff/300001/net/test/spawned_test_...
net/test/spawned_test_server/base_test_server.h:101: // SingleResponse is used
when specifying multiple stapled responses, each
On 2016/07/18 22:56:37, Ryan Sleevi (extremely slow) wrote:
> s/SingleResponse/OCSPSingleResponse/

Done.

https://codereview.chromium.org/2100303002/diff/300001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2100303002/diff/300001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:9260: OCSPVerifyResult::ResponseStatus
response_status;
On 2016/07/18 22:56:37, Ryan Sleevi (extremely slow) wrote:
> It's arguably more typing, but if it makes you feel any better, you could also
> do
> 
> using OCSPSingleResponse = SpawnedTestServer::SSLOptions::OCSPSingleResponse;
> using OCSPProduced = SpawnedTestServer::SSLOptions::OCSPProduced;
> using ResponseStatus = OCSPVerifyResult::ResponseStatus
> 
> std::vector<OCSPSingleResponse> ocsp_responses;
> 
> (etc)

If I never have to touch this array again, it'll still be too soon.

https://codereview.chromium.org/2100303002/diff/300001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:9488: virtual ~HTTPSOCSPVerifyTest() {}
On 2016/07/18 22:56:37, Ryan Sleevi (extremely slow) wrote:
> Pretty sure you can omit both of these, so that it's just
> 
> ...
> public testing::WithParamInterface<OCSPVerifyTestData> {
> };

Done.

https://codereview.chromium.org/2100303002/diff/300001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:9516: }
On 2016/07/18 22:56:37, Ryan Sleevi (extremely slow) wrote:
> omit braces

headdesk dot gif.

Done.

[dadrian, 2016-07-18 23:23:57]

dadrian@google.com changed reviewers:
+ dzhioev@chromium.org

[dadrian, 2016-07-18 23:23:58]

dzhioev@chromium.org: Looking for review on the compatibility change to the
ChromeOS test file. Thanks!

[dadrian, 2016-07-20 20:10:48]

dadrian@google.com changed reviewers:
+ piman@chromium.org

[dadrian, 2016-07-20 20:14:07]

On 2016/07/18 23:23:58, dadrian wrote:
> mailto:dzhioev@chromium.org: Looking for review on the compatibility change to
the
> ChromeOS test file. Thanks!

piman@, dzhioev@: Looking for review on
//chrome/browser/chromeos/login/test/https_forwarder.py, we're trying to push
through a few CL's by the end of the week that are all blocking on this one, and
we'd appreciate verification from a chromeos/login OWNER. Thanks!

[dzhioev (left Google), 2016-07-20 21:01:40]

c/b/chromeos/login LGTM

[dadrian, 2016-07-20 21:07:48]

The CQ bit was checked by dadrian@google.com

[dadrian, 2016-07-20 21:07:48]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2100303002/#ps320001
(title: "Remaining nits.")

[commit-bot: I haz the power, 2016-07-20 21:08:12]

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-07-20 22:37:13]

Description was changed from

==========
Extract OCSPCertStatus::Status to standalone OCSPRevocationStatus, and add
OCSPVerifyResult for tracking stapled OCSP responses cross-platform.
OCSPVerifyResult is populated by CertVerifyProc, but is currently unused. In the
future, it will be consumed by Expect-Staple reports.

This CL also updates mini-CA and the spawned test server to be able to send a
wider range of OCSP responses. Since OCSP responses are short lived, test the
new functionality in url_request_unittest.cc and dynamically generate OCSP
responses.

BUG=598021
==========

to

==========
Extract OCSPCertStatus::Status to standalone OCSPRevocationStatus, and add
OCSPVerifyResult for tracking stapled OCSP responses cross-platform.
OCSPVerifyResult is populated by CertVerifyProc, but is currently unused. In the
future, it will be consumed by Expect-Staple reports.

This CL also updates mini-CA and the spawned test server to be able to send a
wider range of OCSP responses. Since OCSP responses are short lived, test the
new functionality in url_request_unittest.cc and dynamically generate OCSP
responses.

BUG=598021
==========

[commit-bot: I haz the power, 2016-07-20 22:37:14]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-07-20 22:39:09]

Description was changed from

==========
Extract OCSPCertStatus::Status to standalone OCSPRevocationStatus, and add
OCSPVerifyResult for tracking stapled OCSP responses cross-platform.
OCSPVerifyResult is populated by CertVerifyProc, but is currently unused. In the
future, it will be consumed by Expect-Staple reports.

This CL also updates mini-CA and the spawned test server to be able to send a
wider range of OCSP responses. Since OCSP responses are short lived, test the
new functionality in url_request_unittest.cc and dynamically generate OCSP
responses.

BUG=598021
==========

to

==========
Extract OCSPCertStatus::Status to standalone OCSPRevocationStatus, and add
OCSPVerifyResult for tracking stapled OCSP responses cross-platform.
OCSPVerifyResult is populated by CertVerifyProc, but is currently unused. In the
future, it will be consumed by Expect-Staple reports.

This CL also updates mini-CA and the spawned test server to be able to send a
wider range of OCSP responses. Since OCSP responses are short lived, test the
new functionality in url_request_unittest.cc and dynamically generate OCSP
responses.

BUG=598021

Committed: https://crrev.com/612337a1d7cadc52d0217b9f399eb1fab445d3e2
Cr-Commit-Position: refs/heads/master@{#406699}
==========

[commit-bot: I haz the power, 2016-07-20 22:39:11]

Patchset 17 (id:??) landed as
https://crrev.com/612337a1d7cadc52d0217b9f399eb1fab445d3e2
Cr-Commit-Position: refs/heads/master@{#406699}