Route OCSP stapling through CertVerifier.

Route OCSP stapling through CertVerifier.

This moves the platform-specific OCSP stapling code out of SSLClientSocket,
which enables OCSP stapling on the chimera build. It'll also make it easier to
support OCSP stapling on OS X, where a stapled OCSP response is a property of
the certificate verification (as it should be) and not the certificate.

BUG=430714, 479034
Committed: https://crrev.com/15f5713c41825ea900cd6dc4f6de4a6d3fff538d
Cr-Commit-Position: refs/heads/master@{#327070}

[Ryan Sleevi, 2015-04-22 22:53:37]

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

[Ryan Sleevi, 2015-04-22 22:53:37]

Looks like remoting might be supplying a NULL certverifier in violation of the
"expected" API. Preferably fix that in a separable CL?

Otherwise, this LG, mod the issue of just moving the modification on Win down
and the problem goes away.

https://codereview.chromium.org/1081913003/diff/1/net/cert/cert_verify_proc_w...
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/1081913003/diff/1/net/cert/cert_verify_proc_w...
net/cert/cert_verify_proc_win.cc:595: }
Move this down to line 659 and your problems go away

[davidben, 2015-04-23 01:02:33]

Dealt with remoting being silly in https://codereview.chromium.org/1103603003/
and rebased on top of it.

https://codereview.chromium.org/1081913003/diff/1/net/cert/cert_verify_proc_w...
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/1081913003/diff/1/net/cert/cert_verify_proc_w...
net/cert/cert_verify_proc_win.cc:595: }
On 2015/04/22 22:53:37, Ryan Sleevi wrote:
> Move this down to line 659 and your problems go away

Done.

[Ryan Sleevi, 2015-04-23 01:41:23]

This LGTM, but the caveat is that I think the CertVerifier solution is
short-term correct (keeps the unfortunate current behaviour of the code), but
long-term wrong (my remarks on optionality).

However, since I know you actually go back and eventually clean up code in a
timely fashion, I'm happy to stamp this, rather than block on fixing those API
callers.

Still, several nits below before landing.

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_nss.cc (right):

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.cc:765: cache_ocsp_response_from_side_channel_ =
In order to const, you have to move to a more challenging form:
#if defined(USE_NSS_CERTS)
  : cache_ocsp_response_from_side_channel_(
  reinterpret_cast<CacheOCSPResponseFromSideChannelFunction>(dlsym(...)))
#endif
  {

Yes, I unquestionably agree it's less readable, but the above at least tries to
address several issues:
1) const-explicitness for the pointer (CacheOCSPResponseFromSideChannelFunction*
const) since it will be hit on multiple threads
2) casting

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.cc:779: return
cache_ocsp_response_from_side_channel_ != nullptr;
Drop the explicit "!= nullptr"; this follows the idiomatic form for all of our
pointer types of allowing bool coercion according to the type (raw pointers
promote to bool, scoped_ptr has a converter, scoped_refptr will eventually)

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.cc:806: cache_ocsp_response_from_side_channel_ !=
nullptr) {
ditto bool conversion. Drop the != nullptr

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.cc:807: // Note: NSS uses a global hash table, so
this call will affect any
s/a global/a thread-safe global/

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_nss.h (right):

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.h:54: CacheOCSPResponseFromSideChannelFunction
Const-this, since it will definitely be hit on multiple threads?

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_win.cc:657: VLOG(1) << "Failed to set OCSP response
property: " << GetLastError();
Time to nuke this; we've never once used it, and we've had it for years.

(Line 656-658)

[davidben, 2015-04-23 20:41:47]

Rebased on top of new remoting CL which fixes their null CertVerifier

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_nss.cc (right):

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.cc:765: cache_ocsp_response_from_side_channel_ =
On 2015/04/23 01:41:23, Ryan Sleevi wrote:
> In order to const, you have to move to a more challenging form:
> #if defined(USE_NSS_CERTS)
>   : cache_ocsp_response_from_side_channel_(
>   reinterpret_cast<CacheOCSPResponseFromSideChannelFunction>(dlsym(...)))
> #endif
>   {
> 
> Yes, I unquestionably agree it's less readable, but the above at least tries
to
> address several issues:
> 1) const-explicitness for the pointer
(CacheOCSPResponseFromSideChannelFunction*
> const) since it will be hit on multiple threads
> 2) casting

Done.

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.cc:779: return
cache_ocsp_response_from_side_channel_ != nullptr;
On 2015/04/23 01:41:23, Ryan Sleevi wrote:
> Drop the explicit "!= nullptr"; this follows the idiomatic form for all of our
> pointer types of allowing bool coercion according to the type (raw pointers
> promote to bool, scoped_ptr has a converter, scoped_refptr will eventually)

Done.

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.cc:806: cache_ocsp_response_from_side_channel_ !=
nullptr) {
On 2015/04/23 01:41:23, Ryan Sleevi wrote:
> ditto bool conversion. Drop the != nullptr

Done.

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.cc:807: // Note: NSS uses a global hash table, so
this call will affect any
On 2015/04/23 01:41:23, Ryan Sleevi wrote:
> s/a global/a thread-safe global/

Done.

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_nss.h (right):

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_nss.h:54: CacheOCSPResponseFromSideChannelFunction
On 2015/04/23 01:41:23, Ryan Sleevi wrote:
> Const-this, since it will definitely be hit on multiple threads?

Done.

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_win.cc (right):

https://codereview.chromium.org/1081913003/diff/40001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_win.cc:657: VLOG(1) << "Failed to set OCSP response
property: " << GetLastError();
On 2015/04/23 01:41:23, Ryan Sleevi wrote:
> Time to nuke this; we've never once used it, and we've had it for years.
> 
> (Line 656-658)

Done.

[davidben, 2015-04-24 21:49:45]

davidben@chromium.org changed reviewers:
+ pneubeck@chromium.org, rogerta@chromium.org

[davidben, 2015-04-24 21:49:45]

+pneubeck for chrome/browser/chromeos/policy
+rogerta for google_apis

[davidben, 2015-04-24 23:20:08]

davidben@chromium.org changed reviewers:
+ sergeyu@chromium.org

[davidben, 2015-04-24 23:20:09]

And +sergeyu for remoting

Also, Ryan, PTAL at the new changes to net/cert/cert_verifier.cc in case you
object to them. I think this is the sanest solution to:
- Having a CertVerifier::CreateDefault API is probably a reasonable thing to
want.
- net_nacl has a need for the CertVerifier interface, but no need for the
particular CertVerifier we've selected as default for the normal build of the
library.

(Problem is that net_nacl doesn't include the C files for
MultiThreadedCertVerifier.)

[Ryan Sleevi, 2015-04-24 23:29:13]

On 2015/04/24 23:20:09, David Benjamin (OOO sick) wrote:
> - Having a CertVerifier::CreateDefault API is probably a reasonable thing to
> want.

Still LGTM, although hopefully someday we'll remove this and require explicit
construction rather than a factory like this; that is, the "ideal" world is that
the CertVerifier is always instantiated and propagated from an appropriate
layer. This API was just a bodge reflecting the old X509Certificate::Verify()
being the single and sole way to verify certs.

[Sergey Ulanov, 2015-04-24 23:34:33]

remoting lgtm

[davidben, 2015-04-24 23:36:54]

On 2015/04/24 23:29:13, Ryan Sleevi wrote:
> On 2015/04/24 23:20:09, David Benjamin (OOO sick) wrote:
> > - Having a CertVerifier::CreateDefault API is probably a reasonable thing to
> > want.
> 
> Still LGTM, although hopefully someday we'll remove this and require explicit
> construction rather than a factory like this; that is, the "ideal" world is
that
> the CertVerifier is always instantiated and propagated from an appropriate
> layer. This API was just a bodge reflecting the old X509Certificate::Verify()
> being the single and sole way to verify certs.

We're not too far from that, to be honest. Skimming through codesearch, the
places which do this seem relatively top-level.
content/browser/renderer_host/pepper/ssl_context_helper.h is a little weird.

[pneubeck (no reviews), 2015-04-27 09:07:23]

chrome/browser/chromeos/policy lgtm

[Roger Tawa OOO till Jul 10th, 2015-04-27 15:19:10]

lgtm google_apis

[davidben, 2015-04-27 16:09:11]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-04-27 16:09:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1081913003/#ps200001
(title: "yet another CrOS-only Verify call")

[commit-bot: I haz the power, 2015-04-27 16:09:22]

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

[commit-bot: I haz the power, 2015-04-27 18:08:45]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-04-27 18:09:43]

Patchset 11 (id:??) landed as
https://crrev.com/15f5713c41825ea900cd6dc4f6de4a6d3fff538d
Cr-Commit-Position: refs/heads/master@{#327070}