Address post-review feedback for r81702

net/base/x509_certificate_mac.cc

Index: net/base/x509_certificate_mac.cc
diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc
index 4bb9adf66b58371d8d2d1b94b5a2a2a8163142cc..845249c2ad60fc8c020def36936f69fdab0d271f 100644
--- a/net/base/x509_certificate_mac.cc
+++ b/net/base/x509_certificate_mac.cc
@@ -289,13 +289,19 @@ OSStatus CreatePolicy(const CSSM_OID* policy_OID,
 }
 
 // Creates a series of SecPolicyRefs to be added to a SecTrustRef used to
-// validate a certificate for an SSL peer. |hostname| contains the name of
-// the SSL peer that the certificate should be verified against. |flags| is
+// validate a certificate for an SSL server. |hostname| contains the name of
+// the SSL server that the certificate should be verified against. |flags| is
 // a bitwise-OR of VerifyFlags that can further alter how trust is
 // validated, such as how revocation is checked. If successful, returns
 // noErr, and stores the resultant array of SecPolicyRefs in |policies|.
 OSStatus CreateTrustPolicies(const std::string& hostname, int flags,
                              ScopedCFTypeRef<CFArrayRef>* policies) {
+  CFMutableArrayRef local_policies =
+      CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks);
+  if (!local_policies)
+    return memFullErr;
+  ScopedCFTypeRef<CFMutableArrayRef> scoped_local_policies(local_policies);

[wtc, 2011/04/21 22:04:38]

Please rewrite this as:
  ScopedCFTypeRef<CFMutableArrayRef> local_policies(
      CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));
  if (!local_policies)
    return memFullErr;

+
   // Create an SSL SecPolicyRef, and configure it to perform hostname
   // validation. The hostname check does 99% of what we want, with the
   // exception of dotted IPv4 addreses, which we handle ourselves below.
@@ -310,35 +316,29 @@ OSStatus CreateTrustPolicies(const std::string& hostname, int flags,
                                  sizeof(tp_ssl_options), &ssl_policy);
   if (status)
     return status;
-  ScopedCFTypeRef<SecPolicyRef> scoped_ssl_policy(ssl_policy);
+  CFArrayAppendValue(scoped_local_policies, ssl_policy);
+  CFRelease(ssl_policy);
 
-  // Manually add OCSP and CRL policies. If neither an OCSP or CRL policy is
-  // specified, the Apple TP module will add whatever the system settings
-  // are, which is not desirable here.
-  //
-  // Note that this causes any locally configured OCSP responder URL to be
-  // ignored.
+  // Manually add revocation policies. If no revocation policy is specified,
+  // the Apple TP module will add whatever the system settings are, which is
+  // not desirable here.

[wtc, 2011/04/21 22:04:38]

It would be nice to stress that we need to specify a revocation
policy (an OCSP policy that suppresses access to both
network and cache) to disable revocation checking.

[Ryan Sleevi, 2011/04/21 23:59:17]

Wording has been updated.

   CSSM_APPLE_TP_OCSP_OPTIONS tp_ocsp_options;
   memset(&tp_ocsp_options, 0, sizeof(tp_ocsp_options));
   tp_ocsp_options.Version = CSSM_APPLE_TP_OCSP_OPTS_VERSION;
 
-  CSSM_APPLE_TP_CRL_OPTIONS tp_crl_options;
-  memset(&tp_crl_options, 0, sizeof(tp_crl_options));
-  tp_crl_options.Version = CSSM_APPLE_TP_CRL_OPTS_VERSION;
-
   if (flags & X509Certificate::VERIFY_REV_CHECKING_ENABLED) {
     // If an OCSP responder is available, use it, and avoid fetching any
     // CRLs for that certificate if possible, as they may be much larger.
     tp_ocsp_options.Flags = CSSM_TP_ACTION_OCSP_SUFFICIENT;
-    // Ensure that CRLs can be fetched if a crlDistributionPoint extension
-    // is found. Otherwise, only the local CRL cache will be consulted.
-    tp_crl_options.CrlFlags |= CSSM_TP_ACTION_FETCH_CRL_FROM_NET;
   } else {
-    // Disable OCSP network fetching, but still permit cached OCSP responses
-    // to be used. This is equivalent to the Windows code's usage of
-    // CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY.
-    tp_ocsp_options.Flags = CSSM_TP_ACTION_OCSP_DISABLE_NET;
-    // The default CrlFlags will ensure only cached CRLs are used.
+    // If the Apple TP believes the certificate being verified may be an EV
+    // certificate, it will attempt to force OCSP checking. In order to
+    // reliably disable revocation checking, disable access to both the
+    // network and the cache. Note that when this happens, the Apple TP will
+    // report an error error that OCSP was unavailable, but this will be

[wtc, 2011/04/21 22:04:38]

Typo: error error => error

+    // handled and suppressed in X509Certificate::Verify().
+    tp_ocsp_options.Flags = CSSM_TP_ACTION_OCSP_DISABLE_NET |
+                            CSSM_TP_ACTION_OCSP_CACHE_READ_DISABLE;
   }
 
   SecPolicyRef ocsp_policy;
@@ -346,23 +346,30 @@ OSStatus CreateTrustPolicies(const std::string& hostname, int flags,
                         sizeof(tp_ocsp_options), &ocsp_policy);
   if (status)
     return status;
-  ScopedCFTypeRef<SecPolicyRef> scoped_ocsp_policy(ocsp_policy);
+  CFArrayAppendValue(scoped_local_policies, ocsp_policy);
+  CFRelease(ocsp_policy);
+
+  if (!(flags & X509Certificate::VERIFY_REV_CHECKING_ENABLED)) {
+    policies->reset(scoped_local_policies.release());
+    return noErr;

[wtc, 2011/04/21 22:04:38]

Nit: you can put lines 357-370 inside a
  if (flags & X509Certificate::VERIFY_REV_CHECKING_ENABLED) {
  }
block, then we only need one instance of
  policies->reset(scoped_local_policies.release());
  return noErr;
at the end of this function.

+  }
+
+  CSSM_APPLE_TP_CRL_OPTIONS tp_crl_options;
+  memset(&tp_crl_options, 0, sizeof(tp_crl_options));
+  tp_crl_options.Version = CSSM_APPLE_TP_CRL_OPTS_VERSION;
+  // Ensure that CRLs can be fetched if a crlDistributionPoint extension
+  // is found. Otherwise, only the local CRL cache will be consulted.
+  tp_crl_options.CrlFlags |= CSSM_TP_ACTION_FETCH_CRL_FROM_NET;
 
   SecPolicyRef crl_policy;
   status = CreatePolicy(&CSSMOID_APPLE_TP_REVOCATION_CRL, &tp_crl_options,
                         sizeof(tp_crl_options), &crl_policy);
   if (status)
     return status;
-  ScopedCFTypeRef<SecPolicyRef> scoped_crl_policy(crl_policy);
-
-  CFTypeRef local_policies[] = { ssl_policy, ocsp_policy, crl_policy };
-  CFArrayRef policy_array = CFArrayCreate(kCFAllocatorDefault, local_policies,
-                                          arraysize(local_policies),
-                                          &kCFTypeArrayCallBacks);
-  if (!policy_array)
-    return memFullErr;
+  CFArrayAppendValue(scoped_local_policies, crl_policy);
+  CFRelease(crl_policy);
 
-  policies->reset(policy_array);
+  policies->reset(scoped_local_policies.release());

[wtc, 2011/04/21 22:04:38]

We can use 'swap' here.  Would that be better?

[Ryan Sleevi, 2011/04/21 23:59:17]

We can't. ScopedCFType<CFArrayRef> is a different type than
ScopedCFType<CFMutableArrayRef>, even though the two CF types are exchangeable. 

   return noErr;
 }
 
@@ -856,7 +863,7 @@ int X509Certificate::Verify(const std::string& hostname, int flags,
   } else {
     // EV requires revocation checking.
     // Note, under the hood, SecTrustEvaluate() will modify the OCSP options
-    // so as to attempt OCSP fetching if it believes a certificate may chain
+    // so as to attempt OCSP checking if it believes a certificate may chain
     // to an EV root. However, because network fetches are disabled in
     // CreateTrustPolicies() when revocation checking is disabled, these
     // will only go against the local cache.