Address alv's comments on codereview.chromium.org/119026....

net/base/x509_certificate_nss.cc

Index: net/base/x509_certificate_nss.cc
===================================================================
--- net/base/x509_certificate_nss.cc	(revision 23170)
+++ net/base/x509_certificate_nss.cc	(working copy)
@@ -100,6 +100,23 @@
   DISALLOW_COPY_AND_ASSIGN(ScopedCERTValOutParam);
 };
 
+class ScopedCERTCertificatePolicies {

[wtc, 2009/08/13 00:35:33]

Nit: move this up, under ScopedCERTCertList, because these
two are more related (in NSS).

+ public:
+  explicit ScopedCERTCertificatePolicies(CERTCertificatePolicies* policies)
+      : policies_(policies) {}
+
+  ~ScopedCERTCertificatePolicies() {
+    if (policies_)
+      CERT_DestroyCertificatePoliciesExtension(policies_);
+  }
+
+ private:
+  CERTCertificatePolicies* policies_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedCERTCertificatePolicies);
+};
+
+
 // Map PORT_GetError() return values to our network error codes.
 int MapSecurityError(int err) {
   switch (err) {
@@ -331,8 +348,9 @@
   PRUint64 revocation_method_flags =
       CERT_REV_M_TEST_USING_THIS_METHOD |
       CERT_REV_M_ALLOW_NETWORK_FETCHING |
-      CERT_REV_M_ALLOW_IMPLICIT_DEFAULT_SOURCE |
-      CERT_REV_M_STOP_TESTING_ON_FRESH_INFO;
+      CERT_REV_M_IGNORE_IMPLICIT_DEFAULT_SOURCE |
+      CERT_REV_M_STOP_TESTING_ON_FRESH_INFO |
+      CERT_REV_M_IGNORE_MISSING_FRESH_INFO;

[wtc, 2009/08/13 01:20:58]

Nit: please list CERT_REV_M_IGNORE_MISSING_FRESH_INFO
before CERT_REV_M_STOP_TESTING_ON_FRESH_INFO, to match
the order in which these macros are defined in the NSS
header certt.h.  This makes it easier to verify that we
didn't miss any macros.

   PRUint64 revocation_method_independent_flags =
       CERT_REV_MI_TEST_ALL_LOCAL_INFORMATION_FIRST;
   if (policy_oids && num_policy_oids > 0) {

[wtc, 2009/08/13 00:35:33]

Alexei, I had to add this if statement so that we're
using different flags for normal and EV verifications.
Please verify that these two sets of flags look sane.

@@ -412,6 +430,7 @@
     LOG(ERROR) << "Failed to decode certificate policy.";
     return false;
   }
+  ScopedCERTCertificatePolicies scoped_policies(policies);

[alv, 2009/08/12 18:40:42]

It will work, but it is too fancy and increases size of the code. Since there is
no need to use "goto" I would use a "tag_found" boolean and return it value
after destroying the policies object. You don't even need to have "if" since you
have already made a check at line 429.

[wtc, 2009/08/13 00:38:22]

I, a C programmer, agree with the "too fancy" opinion, but
this is a C++ project, so we need to use the techniques
familiar to C++ programmers.

   CERTPolicyInfo** policy_infos = policies->policyInfos;
   while (*policy_infos != NULL) {
     CERTPolicyInfo* policy_info = *policy_infos++;
@@ -522,9 +541,7 @@
 
 // Studied Mozilla's code (esp. security/manager/ssl/src/nsIdentityChecking.cpp
 // and nsNSSCertHelper.cpp) to learn how to verify EV certificate.
-// TODO(wtc): We may be able to request cert_po_policyOID and just
-// check if any of the returned policies is the EV policy of the trust anchor.
-// Another possible optimization is that we get the trust anchor from
+// TODO(wtc): Possible optimization is that we get the trust anchor from
 // the first PKIXVerifyCert call.  We look up the EV policy for the trust
 // anchor.  If the trust anchor has no EV policy, we know the cert isn't EV.
 // Otherwise, we pass just that EV policy (as opposed to all the EV policies)