Don't block the UI thread for OCSP/CRLs when viewing a cert on Mac.

Don't block the UI thread for OCSP/CRLs when viewing a cert on Mac.

When viewing certificates in the system certificate viewer on Mac, the
certificate chain is re-evaluated with the system revocation checking
policies. During this evaluation, the UI thread is blocked, potentially
causing significant delays for slow OCSP responders.

Instead of allowing the default policies, explicitly disable revocation
checking when invoking the viewer. This does not affect revocation checking
from happening in the network stack (if the user has enabled it), only from
the user being able to view exactly which certificate is revoked when a chain
contains revoked certificate.

BUG=79950
TEST=Access a site with a slow OCSP responder (see bug for an example).
Clicking "Certificate Information" should cause the certificate chain to be
displayed immediately, rather than after ~25 seconds.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90146

[Ryan Sleevi, 2011-05-29 09:46:42]

wtc: Please take a look.

I'm not thrilled with the design of hanging these off the X509Certificate, lest
it become an even larger-mishmash of platform-specific #ifdefs.

My preference would be to move these static methods into a helper namespace such
as net::x509[_util]. This matches somewhat with net::x509_openssl_util (which
could be renamed). However, in a previous CL, you expressed reservations about
this design, so I've kept the practice of hanging them on the X509Certificate.

[wtc, 2011-06-03 01:58:04]

LGTM.

High-Level Comments

1. It would be more readable to have separate CreateSSLServerPolicy
and CreateSSLClientPolicy functions.

2. I didn't object to creating a namespace for the
certificate utility functions.  I remember I only objected
to using "x509" in the namespace name because I think
"X.509" is meaningless to someone unfamiliar with
certificates.  But I don't want us to get bogged down
by naming discussions, and I seem to have already lost.
So, please add x509_mac_util.{h,cc}.  (My preference
would be mac_cert_util.{h,cc}, or cert_util_mac.{h,cc}.)

3. The description of the CL says:
  This behaviour matches the viewer behaviour on Linux and
  Windows.

I am not sure if that's true.  If it's true, it is NOT
intentional.  So I think we should not mention it.

4. Nit: please try to fold long lines in CL descriptions.
Here is a guideline that I like:

* The first line of the CL description should be a brief
  (< 50 characters), single-sentence summary of the change.

* Follow this with one blank line.

* There are no restrictions on the remainder of the
  description.  We usually write one or more paragraphs
  that contain a more detailed summary of the change.
  However, you should manually word wrap your description
  at 80 columns.

Details comments follow.

http://codereview.chromium.org/7082031/diff/1/chrome/browser/ui/cocoa/certifi...
File chrome/browser/ui/cocoa/certificate_viewer.mm (right):

http://codereview.chromium.org/7082031/diff/1/chrome/browser/ui/cocoa/certifi...
chrome/browser/ui/cocoa/certificate_viewer.mm:41: // made in such a way as to
stall the UI while doing so. If an OCSP
Nit: just say "However, the call stalls the UI."

http://codereview.chromium.org/7082031/diff/1/chrome/browser/ui/cocoa/certifi...
chrome/browser/ui/cocoa/certificate_viewer.mm:52: // with revocation checking
enabled. For the time being, this is an
Nit: remove "For the time being".

(This comment block is a little long, so I'm looking for simple
ways to shorten it.)

http://codereview.chromium.org/7082031/diff/1/chrome/browser/ui/cocoa/certifi...
chrome/browser/ui/cocoa/certificate_viewer.mm:67: // hostname and name
identifiers) will not be displayed in the certificate
Nit: just say "certificate name mismatch".  "name identifiers"
isn't a common term.

http://codereview.chromium.org/7082031/diff/1/chrome/browser/ui/cocoa/certifi...
chrome/browser/ui/cocoa/certificate_viewer.mm:68: // viewer if the user is
viewing an SSL server certificate.
Does the ordering of the policies matter?  I wonder if we
should add the basic X.509 policy first.

http://codereview.chromium.org/7082031/diff/1/chrome/browser/ui/cocoa/certifi...
chrome/browser/ui/cocoa/certificate_viewer.mm:69: if (!status) {
Nit: use
  if (status == noErr)
and
  if (status != noErr)
in this file.  This is Mark Mentovai's preferred style.

http://codereview.chromium.org/7082031/diff/1/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/7082031/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:271: // Creates a security policy for basix X.509
validation. If the policy is
Typo: basix => basic

http://codereview.chromium.org/7082031/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:281: // |policies|.
Do we need to mention "and ownership transferred to the caller"?

http://codereview.chromium.org/7082031/diff/1/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/7082031/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:315: // Explicitly add revocation policies, in
order to override system checks.
Nit: it's not clear what "system checks" means.

http://codereview.chromium.org/7082031/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:318: local_policies);
Add
  if (status)
    return status;

http://codereview.chromium.org/7082031/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:1158: tp_ssl_options.ServerName =
hostname.c_str();
Use hostname.data(), which does not require conversion to
a C string.  (See line 310 of the original code.)

[Ryan Sleevi, 2011-06-21 03:52:25]

As it looks like joth@ is planning some refactoring around Verify(), I left off
the move to the x509_util namespace until some of that can be worked out.

PTAL, as all other comments should be addressed.

http://codereview.chromium.org/7082031/diff/1/chrome/browser/ui/cocoa/certifi...
File chrome/browser/ui/cocoa/certificate_viewer.mm (right):

http://codereview.chromium.org/7082031/diff/1/chrome/browser/ui/cocoa/certifi...
chrome/browser/ui/cocoa/certificate_viewer.mm:68: // viewer if the user is
viewing an SSL server certificate.
On 2011/06/03 01:58:04, wtc wrote:
> Does the ordering of the policies matter?  I wonder if we
> should add the basic X.509 policy first.

Functionally, it doesn't, but I went ahead and made that change to see if it
would help readability.

http://codereview.chromium.org/7082031/diff/1/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/7082031/diff/1/net/base/x509_certificate.h#new...
net/base/x509_certificate.h:281: // |policies|.
On 2011/06/03 01:58:04, wtc wrote:
> Do we need to mention "and ownership transferred to the caller"?

I don't think so. It should be well understood that appending to a CFArray will
cause the value to be CFRetain'd (well, use the Retain callback of the array
invoked, which by default is CFRetain), so ownership transfer is implied,
similar to storing in a scoped_refptr<>*.

[wtc, 2011-06-22 21:10:18]

LGTM.  Thanks!

[commit-bot: I haz the power, 2011-06-22 23:38:13]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for diner.
Ping the relevant dude on a on-needed basis.

[commit-bot: I haz the power, 2011-06-23 00:51:26]

Change committed as 90146