Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
174102:
Enable SSLClientSocketTest unit tests on Mac OS X by implementing our own cer... (Closed)
|
Created:
11 years, 4 months ago by hawk Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, John Grabowski, darin (slow to review), brettw, willchan no longer on Chromium, Ben Goodger (Google) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionEnable SSLClientSocketTest unit tests on Mac OS X by implementing our own certificate validation code. This gives us proper hostname matching, multiple error codes (e.g., before a certificate could be marked as expired or untrusted, but not both), revocation checking, and EV certificate checking.
BUG=19286, 10910, 14733
TEST=https://www.paypal.com should work without warning. https://paypal.com should get a warning about a hostname mismatch. https://test-ssev.verisign.com:1443/test-SSEV-expired-verisign.html should give a warning about an expired certificate.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=24625
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 72
Patch Set 4 : '' #
Total comments: 1
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 21
Patch Set 7 : '' #
Total comments: 4
Patch Set 8 : '' #
Total comments: 9
Messages
Total messages: 25 (0 generated)
The Mac try failures in the SSL unit tests are expected -- they're due to the need to install the test certificate as described in the CL description.
Haven't fully read the cert code (need some aspirin before doing so) but have some initial comments. http://codereview.chromium.org/174102/diff/1014/21 File net/base/x509_certificate.h (right): http://codereview.chromium.org/174102/diff/1014/21#newcode67 Line 67: typedef CFTypeRef OSCertHandle; Feeling weird about this. http://codereview.chromium.org/174102/diff/1014/22 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/22#newcode34 Line 34: // building a la SecTrustCreateWithCertificates() and friends. That's a pretty big change to be out of sync with the other platforms. Can we do better with an API change? http://codereview.chromium.org/174102/diff/1014/22#newcode186 Line 186: class ScopedCFObject { We have this already; see base/scoped_cftyperef.h http://codereview.chromium.org/174102/diff/1014/22#newcode593 Line 593: return false; You might not have to implement parsing here, but you _do_ need to return a correct value.
Cool! I may not finish the review until tomorrow. Look forward to learning how all this can be done on the Mac. Thanks a lot for your help.
http://codereview.chromium.org/174102/diff/1014/22 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/22#newcode34 Line 34: // building a la SecTrustCreateWithCertificates() and friends. On 2009/08/20 22:17:44, Avi wrote: > That's a pretty big change to be out of sync with the other platforms. Can we do > better with an API change? I wasn't thrilled with it either, but we need those intermediate certificates in order to do chain building. With NSS, the SSL provider is implicitly storing the intermediate certs in its certificate DB, and I assume that Windows does something similar (else it would not be able to verify any peer certificate that wasn't signed directly by one of the trusted anchors). Secure Transport doesn't help us out that way. How about this: I can add a second form of X509Certificate::Verify() that takes a fourth parameter, a vector of X509Certificates that can be used for chain building. On the Mac, this will take the place CFArrayRef and OSCertHandle will go back to being a SecCertificateRef. On Windows and Linux, the new four parameter Verify() will just call the three param Verify(), since the extra certs aren't needed by them. http://codereview.chromium.org/174102/diff/1014/22#newcode186 Line 186: class ScopedCFObject { On 2009/08/20 22:17:44, Avi wrote: > We have this already; see base/scoped_cftyperef.h Done. http://codereview.chromium.org/174102/diff/1014/22#newcode593 Line 593: return false; On 2009/08/20 22:17:44, Avi wrote: > You might not have to implement parsing here, but you _do_ need to return a > correct value. Since it's a private method that's not called by anyone (on Linux and Windows it's used internally by the Verify() method), how about I just remove it for the Mac (and put an appropriate #if around its declaration in x509_certificate.h)?
On 2009/08/21 00:15:28, hawk wrote: > How about this: I can add a second form of X509Certificate::Verify() that takes > a fourth parameter, a vector of X509Certificates that can be used for chain > building. On the Mac, this will take the place CFArrayRef and OSCertHandle will > go back to being a SecCertificateRef. On Windows and Linux, the new four > parameter Verify() will just call the three param Verify(), since the extra > certs aren't needed by them. That sounds like a much cleaner design. > Since it's a private method [...] how about I just remove it for the > Mac (and put an appropriate #if around its declaration in x509_certificate.h)? Yes. And I see that there's no comment for it in the .h file so explain why it's not needed for the Mac while you're at it.
Chris, I have reviewed everything except X509Certificate::Verify. I will review that method next. Let me send the comments on the rest of the CL first. Thank you very much for taking this on. I would not be able to figure out how to do many of these. http://codereview.chromium.org/174102/diff/1014/21 File net/base/x509_certificate.h (right): http://codereview.chromium.org/174102/diff/1014/21#newcode67 Line 67: typedef CFTypeRef OSCertHandle; CFTypeRef is typedef const void * CFTypeRef; It is untyped. Is this change necessary? Nit: If so, should we include CFBase.h instead of Security/Security.h ? http://codereview.chromium.org/174102/diff/1014/22 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/22#newcode14 Line 14: #include "net/base/ev_root_ca_metadata.h" You can remove this header since you're using Mac OS X's built-in EV root CA metadata. http://codereview.chromium.org/174102/diff/1014/22#newcode34 Line 34: // building a la SecTrustCreateWithCertificates() and friends. I suggest that we store the CFArrayRef in a member of X509Certificate: CFArrayRef cert_chain_; Mozilla used to have a (in-memory) hashtable of the intermediate CA certs. They only recently started to store the intermediate CA certs in their certificate DB, partly because IE does that and it helps support misconfigured websites that only send the end-entity certs. So another option is to store the intermediate CA certs in a std::vector or some other kind of container. http://codereview.chromium.org/174102/diff/1014/22#newcode86 Line 86: // Failure was due to something Chromium doesn't define a Are there OSStatus codes that are equivalent to CERT_STATUS_NO_REVOCATION_MECHANISM or Is it possible for 'status' to be not related to certificates at all? http://codereview.chromium.org/174102/diff/1014/22#newcode93 Line 93: bool OverrideHostnameMismatch(const std::string &hostname, Nit: put & and * next to the type. We need to be careful in this function, because dns_names is a lossy conversion of the common name field and subjectAltName extension. We lose the type info. This function should only allow dns_names that are of the iPAddress type. http://codereview.chromium.org/174102/diff/1014/22#newcode109 Line 109: for (std::vector<std::string>::iterator name = dns_names->begin(); Nit: use a const_iterator. http://codereview.chromium.org/174102/diff/1014/22#newcode111 Line 111: override_hostname_mismatch = *name == hostname; Nit: add parentheses around *name == hostname. http://codereview.chromium.org/174102/diff/1014/22#newcode592 Line 592: NOTIMPLEMENTED(); Replace NOTIMPLEMENTED() by NOTREACHED() ? You can remove the block comment on lines 582-587 or move it into the Verify() method above. http://codereview.chromium.org/174102/diff/1014/19 File net/socket/ssl_client_socket_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/19#newcode326 Line 326: status = SSLSetEnableCertVerify(ssl_context_, false); Add a comment saying that we validate the certificate chain ourselves. http://codereview.chromium.org/174102/diff/1014/19#newcode471 Line 471: // Kick off server certificate validation Nit: add periods (.) at the end of these comments. http://codereview.chromium.org/174102/diff/1014/19#newcode508 Line 508: if (!status && certs) { Nit: use the same test for "success" in this function. I suggest "status == noErr", which is used a few lines above. http://codereview.chromium.org/174102/diff/1014/19#newcode515 Line 515: return ERR_CERT_INVALID; We probably should fail with a different error code because there is no certificate at all. ERR_CERT_INVALID would be confusing if the user couldn't inspect the cert to see what's wrong... Nit: our style is to return early, so we don't need to indent the rest of the code, like this: if (status || !certs) return ERR_CERT_INVALID; // some other error code DCHECK(CFArrayGetCount(certs) > 0); // server_cert_ takes ownership of certs ... http://codereview.chromium.org/174102/diff/1014/19#newcode542 Line 542: ssl_config_.IsAllowedBadCert(server_cert_)) Nit: identation is slightly off.
LGTM. You can upload a new Patch Set to address the issues, or do that in a follow-up CL. Some global comments. 1. Please put * and & after type names. 2. You often return ERR_UNEXPECTED when a Security Framework function fails. We should use ERR_UNEXPECTED only when the function is not expected to fail (because we're sure the input arguments are all valid, etc.). Otherwise, it is best to add an error code mapping function that maps OSStatus to our error code. 3. Nit: please add periods (.) to the end of comments. 4. You often return CERT_STATUS_INVALID by default. Please verify that when you do that, we have a certificate and the error is related to that certificate (as opposed to out of memory or IO errors, etc.). 5. Let's update the test setup instructions with the correct command to install the test root CA cert, so that we don't need to mention that in the Description of this CL. Thanks again for writing this CL. It is nontrivial. http://codereview.chromium.org/174102/diff/1014/22 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/22#newcode365 Line 365: GetCertGeneralNamesForOID(cert_handle_, CSSMOID_SubjectAltName, GNT_DNSName, Hmm... so we get only GNT_DNSName, but not GNT_IPAddress here. Perhaps this means our test server certificate was generated incorrectly. I bet we put "127.0.0.1" in the subject's common name. We should have used an iPAddress in the subjectAltName extension. http://codereview.chromium.org/174102/diff/1014/22#newcode384 Line 384: if (status || !ssl_policy_search_ref) You do this in several places -- even with a successful 'status', you still do a null check of the returned result. Should we trust the system libraries more? http://codereview.chromium.org/174102/diff/1014/22#newcode385 Line 385: return ERR_UNEXPECTED; We may want to use an error mapping function here, unless these functions are not expected to fail on valid inputs. http://codereview.chromium.org/174102/diff/1014/22#newcode392 Line 392: tp_ssl_options.ServerName = hostname.c_str(); Nit: we should be able to use hostname.data(), to avoid the conversion to a C string. http://codereview.chromium.org/174102/diff/1014/22#newcode410 Line 410: const void *cert = cert_handle_; Nit: use CFTypeRef instead of const void * here? http://codereview.chromium.org/174102/diff/1014/22#newcode414 Line 414: return ERR_UNEXPECTED; return ERR_OUT_OF_MEMORY instead. http://codereview.chromium.org/174102/diff/1014/22#newcode423 Line 423: // to apply OCSP and CRL checking, but we're still subject to the global I see. This means we should only pass the VERIFY_REV_CHECKING_ENABLED flag to this method when the global setting for revocation checking is enabled. We should do that when we port the SSLConfigService class to Mac OS X. http://codereview.chromium.org/174102/diff/1014/22#newcode425 Line 425: // the Certificates tab of the Preferences dialog). If the user has Nit: we try not to refer to the Chromium browser in the net module. If "the Certificates tab of the Preferences dialog" means the Preferences dialog of Chromium, please remove this. http://codereview.chromium.org/174102/diff/1014/22#newcode428 Line 428: // with one of a number of sub error codes indicating that revocation Nit: "revocation" => "revocation checking" http://codereview.chromium.org/174102/diff/1014/22#newcode440 Line 440: return ERR_UNEXPECTED; Return ERR_OUT_OF_MEMORY here? http://codereview.chromium.org/174102/diff/1014/22#newcode452 Line 452: // chain, not that the chain contains no errors. We need to examine the Did you mean not that the chain contain no errors or not that the chain contain errors ? http://codereview.chromium.org/174102/diff/1014/22#newcode473 Line 473: // the user has not explicitly set a trust setting) What does it mean that the user has not set a trust setting? http://codereview.chromium.org/174102/diff/1014/22#newcode477 Line 477: case kSecTrustResultConfirm: Why do "Deny" and "Confirm" mean the same thing? http://codereview.chromium.org/174102/diff/1014/22#newcode532 Line 532: verify_result->cert_status |= CERT_STATUS_INVALID; Perhaps a NOTREACHED() assertion, so we can debug this? http://codereview.chromium.org/174102/diff/1014/22#newcode560 Line 560: CFSTR("SecTrustCopyExtendedResult"))); SecTrustCopyExtendedResult is declared in SecTrustPriv.h, which is "Private part of SecTrust.h". Are you sure it's OK to use it? http://codereview.chromium.org/174102/diff/1014/22#newcode565 Line 565: // The returned dictionary contains the organization name from the Nit: add "EV" before "organization name"? Should we verify that the returned dictionary contains the kSecEVOrganizationName key?
http://codereview.chromium.org/174102/diff/1014/21 File net/base/x509_certificate.h (right): http://codereview.chromium.org/174102/diff/1014/21#newcode67 Line 67: typedef CFTypeRef OSCertHandle; On 2009/08/21 21:38:23, wtc wrote: > CFTypeRef is > typedef const void * CFTypeRef; > > It is untyped. Is this change necessary? > > Nit: If so, should we include CFBase.h instead of > Security/Security.h ? Reverted back to SecSecurityRef. http://codereview.chromium.org/174102/diff/1014/22 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/22#newcode14 Line 14: #include "net/base/ev_root_ca_metadata.h" On 2009/08/21 21:38:23, wtc wrote: > You can remove this header since you're using Mac OS X's > built-in EV root CA metadata. Done. http://codereview.chromium.org/174102/diff/1014/22#newcode34 Line 34: // building a la SecTrustCreateWithCertificates() and friends. On 2009/08/21 21:38:23, wtc wrote: > I suggest that we store the CFArrayRef in a member of > X509Certificate: > CFArrayRef cert_chain_; > > Mozilla used to have a (in-memory) hashtable of the > intermediate CA certs. They only recently started to > store the intermediate CA certs in their certificate > DB, partly because IE does that and it helps support > misconfigured websites that only send the end-entity > certs. > > So another option is to store the intermediate CA certs > in a std::vector or some other kind of container. What I ended up doing what loading the intermediate certificates in X509Certificate objects, which puts them into the certificate cache. When it comes time to verify, we load the intermediate certificates from the cache. http://codereview.chromium.org/174102/diff/1014/22#newcode86 Line 86: // Failure was due to something Chromium doesn't define a On 2009/08/21 21:38:23, wtc wrote: > Are there OSStatus codes that are equivalent to > CERT_STATUS_NO_REVOCATION_MECHANISM or CSSMERR_APPLETP_CRL_NOT_FOUND is close; there's no OCSP status code that means "OCSP not checked because there was no responder URL in the certificate" > Is it possible for 'status' to be not related to certificates > at all? Not from where this function is called (i.e., on the result of SecTrustGetCssmResultCode() and the status codes in CSSM_TP_APPLE_EVIDENCE_INFO) http://codereview.chromium.org/174102/diff/1014/22#newcode109 Line 109: for (std::vector<std::string>::iterator name = dns_names->begin(); On 2009/08/21 21:38:23, wtc wrote: > Nit: use a const_iterator. Done. http://codereview.chromium.org/174102/diff/1014/22#newcode111 Line 111: override_hostname_mismatch = *name == hostname; On 2009/08/21 21:38:23, wtc wrote: > Nit: add parentheses around *name == hostname. Done. http://codereview.chromium.org/174102/diff/1014/22#newcode365 Line 365: GetCertGeneralNamesForOID(cert_handle_, CSSMOID_SubjectAltName, GNT_DNSName, On 2009/08/21 22:27:40, wtc wrote: > Hmm... so we get only GNT_DNSName, but not GNT_IPAddress > here. > > Perhaps this means our test server certificate was > generated incorrectly. I bet we put "127.0.0.1" in > the subject's common name. We should have used an > iPAddress in the subjectAltName extension. The CN of the test cert is indeed 127.0.0.1. http://codereview.chromium.org/174102/diff/1014/22#newcode384 Line 384: if (status || !ssl_policy_search_ref) On 2009/08/21 22:27:40, wtc wrote: > You do this in several places -- even with a successful > 'status', you still do a null check of the returned result. > Should we trust the system libraries more? Sorry, I have a habit of not trusting any return value in security-sensitive code. Removed (here and elsewhere). http://codereview.chromium.org/174102/diff/1014/22#newcode385 Line 385: return ERR_UNEXPECTED; On 2009/08/21 22:27:40, wtc wrote: > We may want to use an error mapping function here, > unless these functions are not expected to fail on > valid inputs. Done. http://codereview.chromium.org/174102/diff/1014/22#newcode392 Line 392: tp_ssl_options.ServerName = hostname.c_str(); On 2009/08/21 22:27:40, wtc wrote: > Nit: we should be able to use hostname.data(), to avoid > the conversion to a C string. Done. http://codereview.chromium.org/174102/diff/1014/22#newcode410 Line 410: const void *cert = cert_handle_; On 2009/08/21 22:27:40, wtc wrote: > Nit: use CFTypeRef instead of const void * here? Removed. http://codereview.chromium.org/174102/diff/1014/22#newcode414 Line 414: return ERR_UNEXPECTED; On 2009/08/21 22:27:40, wtc wrote: > return ERR_OUT_OF_MEMORY instead. Done. http://codereview.chromium.org/174102/diff/1014/22#newcode423 Line 423: // to apply OCSP and CRL checking, but we're still subject to the global On 2009/08/21 22:27:40, wtc wrote: > I see. This means we should only pass the > VERIFY_REV_CHECKING_ENABLED flag to this method when > the global setting for revocation checking is enabled. > We should do that when we port the SSLConfigService class > to Mac OS X. My use of "global" might be a little misleading here; revocation checking is set by the user using Keychain Access and that setting propagates to all Mac OS X apps that use SecTrust functions. http://codereview.chromium.org/174102/diff/1014/22#newcode425 Line 425: // the Certificates tab of the Preferences dialog). If the user has On 2009/08/21 22:27:40, wtc wrote: > Nit: we try not to refer to the Chromium browser in the net > module. > > If "the Certificates tab of the Preferences dialog" means > the Preferences dialog of Chromium, please remove this. It means the the prefs dialog of Keychain Access.app. http://codereview.chromium.org/174102/diff/1014/22#newcode428 Line 428: // with one of a number of sub error codes indicating that revocation On 2009/08/21 22:27:40, wtc wrote: > Nit: "revocation" => "revocation checking" Done. http://codereview.chromium.org/174102/diff/1014/22#newcode440 Line 440: return ERR_UNEXPECTED; On 2009/08/21 22:27:40, wtc wrote: > Return ERR_OUT_OF_MEMORY here? Done. http://codereview.chromium.org/174102/diff/1014/22#newcode452 Line 452: // chain, not that the chain contains no errors. We need to examine the On 2009/08/21 22:27:40, wtc wrote: > Did you mean > not that the chain contain no errors > or > not that the chain contain errors > ? I reworded this a little bit. An error returned here means that an error occurred while processing the chain (like out of memory), and no error returned means that the chain was processed, but the chain may have problems (like an expired cert). http://codereview.chromium.org/174102/diff/1014/22#newcode473 Line 473: // the user has not explicitly set a trust setting) On 2009/08/21 22:27:40, wtc wrote: > What does it mean that the user has not set a trust setting? In their keychains, users can assign "Always"/"Never Trust" for each certificate, for various policies (like SSL). If the user has never done so (which is the case for most users and most certificates), the default is "no value specified" http://codereview.chromium.org/174102/diff/1014/22#newcode477 Line 477: case kSecTrustResultConfirm: On 2009/08/21 22:27:40, wtc wrote: > Why do "Deny" and "Confirm" mean the same thing? "Deny" means that the user set "Never Trust" for the certificate. "Confirm" means that the user has set “Ask Permission" (at least according to the docs, that option is not available in Leopard). For kSecTrustResultConfirm, we're following what Secure Transport does, which is treat it as "Deny". http://codereview.chromium.org/174102/diff/1014/22#newcode532 Line 532: verify_result->cert_status |= CERT_STATUS_INVALID; On 2009/08/21 22:27:40, wtc wrote: > Perhaps a NOTREACHED() assertion, so we can debug this? Done. http://codereview.chromium.org/174102/diff/1014/22#newcode560 Line 560: CFSTR("SecTrustCopyExtendedResult"))); On 2009/08/21 22:27:40, wtc wrote: > SecTrustCopyExtendedResult is declared in SecTrustPriv.h, > which is "Private part of SecTrust.h". > > Are you sure it's OK to use it? It carries the same risk as any SPI. I'll follow up offline. http://codereview.chromium.org/174102/diff/1014/22#newcode565 Line 565: // The returned dictionary contains the organization name from the On 2009/08/21 22:27:40, wtc wrote: > Nit: add "EV" before "organization name"? Done. > Should we verify that the returned dictionary contains > the kSecEVOrganizationName key? The current SecTrust code won't return a dictionary that doesn't have it. http://codereview.chromium.org/174102/diff/1014/22#newcode592 Line 592: NOTIMPLEMENTED(); On 2009/08/21 21:38:23, wtc wrote: > Replace NOTIMPLEMENTED() by NOTREACHED() ? > > You can remove the block comment on lines 582-587 or move > it into the Verify() method above. Done. http://codereview.chromium.org/174102/diff/1014/19 File net/socket/ssl_client_socket_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/19#newcode326 Line 326: status = SSLSetEnableCertVerify(ssl_context_, false); On 2009/08/21 21:38:23, wtc wrote: > Add a comment saying that we validate the certificate chain > ourselves. Done. http://codereview.chromium.org/174102/diff/1014/19#newcode471 Line 471: // Kick off server certificate validation On 2009/08/21 21:38:23, wtc wrote: > Nit: add periods (.) at the end of these comments. Done. http://codereview.chromium.org/174102/diff/1014/19#newcode508 Line 508: if (!status && certs) { On 2009/08/21 21:38:23, wtc wrote: > Nit: use the same test for "success" in this function. > I suggest "status == noErr", which is used a few lines > above. Done. http://codereview.chromium.org/174102/diff/1014/19#newcode515 Line 515: return ERR_CERT_INVALID; On 2009/08/21 21:38:23, wtc wrote: > We probably should fail with a different error code > because there is no certificate at all. > > ERR_CERT_INVALID would be confusing if the user couldn't > inspect the cert to see what's wrong... > > Nit: our style is to return early, so we don't need to > indent the rest of the code, like this: > > if (status || !certs) > return ERR_CERT_INVALID; // some other error code > > DCHECK(CFArrayGetCount(certs) > 0); > // server_cert_ takes ownership of certs > ... > Switched to return ERR_SSL_PROTOCOL_ERROR, since we don't support anonymous cipher suites, the server should always have a certificate. http://codereview.chromium.org/174102/diff/1014/19#newcode542 Line 542: ssl_config_.IsAllowedBadCert(server_cert_)) On 2009/08/21 21:38:23, wtc wrote: > Nit: identation is slightly off. Done.
I would prefer that the tests used something like SSLSetTrustedRoots rather than requiring people to install the certificates to their system root keychains. That will make the test more useful without having to worry about obscure setup instructions.
Mark is right. It's better to implement the equivalent of LoadTemporaryCert in src/net/socket/ssl_test_util.cc for the Mac. I have an open issue http://crbug.com/8470 to do that for Windows.
On 2009/08/24 19:52:34, wtc wrote: > Mark is right. It's better to implement the equivalent of > LoadTemporaryCert in src/net/socket/ssl_test_util.cc for the > Mac. I have an open issue http://crbug.com/8470 to do that > for Windows. Working on it, will upload new changes shortly.
LGTM http://codereview.chromium.org/174102/diff/32/1024 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/32/1024#newcode577 Line 577: // it's defined in x509_certificate.h. We perform EV checking in the Would prefer to ifdef the function definition in the .h file but this comment is OK.
Hi Chris, I just realized that you may be waiting for me to review the latest Patch Set. Sorry about that. I have some suggested changes below. Please make at least the small changes in this CL. There are two big requested changes: 1. Use a different way to pass our test root CA cert to X509Certificate::Verify. 2. Don't convert the intermediate certs to X509Certificate objects. You can make these two changes in a follow-up CL. That'll be easier to review and hopefully will also be more convenient for you. http://codereview.chromium.org/174102/diff/1036/1040 File net/base/x509_certificate.h (right): http://codereview.chromium.org/174102/diff/1036/1040#newcode137 Line 137: SOURCE_TRUSTED_STORE = 1, // From the trusted anchor certificate We probably don't need this new SOURCE_TRUSTED_STORE enum value. The Source enum type is a hack. It's really a boolean type ("lone cert import" vs. "from network"), plus an "unused" value. I think it's better to add a separate cache of certs from the trusted anchor cert store, rather than using the existing X509Certificate::Cache (which is intended to reduce the number of X509Certificate objects we create). http://codereview.chromium.org/174102/diff/1036/1041 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1036/1041#newcode41 Line 41: default: Let's add a LOG(ERROR) statement with the value of 'status' that we map to ERR_FAILED, so that we can improve the NetErrorFromOSStatus function. ERR_FAILED is our default error code. It doesn't tell us anything, so it's painful to see the ERR_FAILED error code in a bug report. http://codereview.chromium.org/174102/diff/1036/1041#newcode414 Line 414: // Set the trusted anchor certificates for the SecTrustRef by merging the I guess you're doing this to support the test root CA cert? Perhaps we should just add a global function that returns the test root CA cert for unit tests (we can use #ifdef UNIT_TEST for the code that merge the system trust anchors with our test root CA.). http://codereview.chromium.org/174102/diff/1036/1041#newcode418 Line 418: status = SecTrustCopyAnchorCertificates(&anchor_array); I hope there is a better way to do this. The number of system trust anchors may be large. Is it expensive to copy them every time we verify a cert? For example, we call AddToArray(SOURCE_TRUSTED_STORE) first. If it returns no trust anchors, then we skip the SecTrustSetAnchorCertificates call. Otherwise (presumably for unit tests only) we take the slow code path and merge the system trust anchors with the trust anchors from the cache. It seems that we can call SecTrustSetAnchorCertificatesOnly to avoid merging the trust anchors ourselves. http://codereview.chromium.org/174102/diff/1036/1038 File net/socket/ssl_client_socket_mac.cc (right): http://codereview.chromium.org/174102/diff/1036/1038#newcode255 Line 255: if (!CFArrayGetCount(certs)) Note: please merge with my CL http://codereview.chromium.org/173328 carefully here. I found that the problem is that 'certs' is NULL, not that the array count is 0. So I suggest that we leave the DCHECK_GT assertion unchanged. http://codereview.chromium.org/174102/diff/1036/1038#newcode512 Line 512: return ERR_SSL_PROTOCOL_ERROR; We should use another error code here. ERR_SSL_PROTOCOL_ERROR during handshake means the handshake failed. Here the handshake succeeded, just that there's no server cert. Perhaps return ERR_UNEXPECTED or invent a new error code (ERR_ANONYMOUS_CIPHER_SUITE?). Note: When TLS is enabled, ERR_SSL_PROTOCOL_ERROR during handshake will trigger a retry with TLS disabled in our HTTP layer. All browsers do this to support "TLS_intolerant servers", which don't support a ClientHello message requesting version 3.1. http://codereview.chromium.org/174102/diff/1036/1038#newcode522 Line 522: if (!server_cert_) I think the original DCHECK is good enough here. We can only enter this state from DoHandshake after server_cert_ is set to a non-NULL value. In any case, the error code should match the error code used on line 512 in DoHandshake. http://codereview.chromium.org/174102/diff/1036/1038#newcode527 Line 527: // and makes them available X509Certificate::Verify() for chain building. Nit: add "to" after "available". http://codereview.chromium.org/174102/diff/1036/1039 File net/socket/ssl_client_socket_mac.h (right): http://codereview.chromium.org/174102/diff/1036/1039#newcode95 Line 95: std::vector<scoped_refptr<X509Certificate> > intermediate_certs_; Nit: intermediate_cas_? That seems to look worse :-) Since we don't use the intermediate CA certs directly, they don't need to be converted to X509Certificate. We can just use an CFArrayRef here. Note: I actually suggested that you put the intermediate CAs in the X509Certificate object for the end-entity cert. But I think your choice of putting them in SSL client sockets is superior. http://codereview.chromium.org/174102/diff/1036/1042 File net/socket/ssl_test_util.cc (right): http://codereview.chromium.org/174102/diff/1036/1042#newcode393 Line 393: #elif defined(OS_MACOSX) Let's add a TODO comment to make the same code work for Linux and Mac.
Some more comments... http://codereview.chromium.org/174102/diff/1014/22 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/22#newcode86 Line 86: // Failure was due to something Chromium doesn't define a On 2009/08/22 01:43:45, hawk wrote: > On 2009/08/21 21:38:23, wtc wrote: > > Are there OSStatus codes that are equivalent to > > CERT_STATUS_NO_REVOCATION_MECHANISM or > > CSSMERR_APPLETP_CRL_NOT_FOUND is close; there's no OCSP status code that means > "OCSP not checked because there was no responder URL in the certificate" Is it possible to map CSSMERR_APPLETP_CRL_NOT_FOUND to our CERT_STATUS_NO_REVOCATION_MECHANISM? http://codereview.chromium.org/174102/diff/1014/22#newcode423 Line 423: // to apply OCSP and CRL checking, but we're still subject to the global On 2009/08/22 01:43:45, hawk wrote: > > My use of "global" might be a little misleading here; revocation checking is set > by the user using Keychain Access and that setting propagates to all Mac OS X > apps that use SecTrust functions. Perhaps reword it to say "system settings"? http://codereview.chromium.org/174102/diff/1014/22#newcode477 Line 477: case kSecTrustResultConfirm: On 2009/08/22 01:43:45, hawk wrote: > For kSecTrustResultConfirm, we're > following what Secure Transport does, which is treat it as "Deny". Would be nice to add this to the source code as a comment. http://codereview.chromium.org/174102/diff/1036/1041#newcode406 Line 406: if (CFGetTypeID(cert_handle_) == CFArrayGetTypeID()) { Hmm... it seems like an expensive, roundabout solution. We will also get unrelated end-entity and intermediate certs into cert_array. Perhaps we should just stick the CFArrayRef returned by SSLCopyPeerCertificates in the X509Certificate object, and add an OS_MACOSX-specific getter for it? http://codereview.chromium.org/174102/diff/1036/1041#newcode438 Line 438: sizeof(tp_action_data)); Is it possible to find out programmatically whether the user has revocation enabled or disabled? http://codereview.chromium.org/174102/diff/1036/1041#newcode442 Line 442: if (SecTrustSetParameters(trust_ref, CSSM_TP_ACTION_DEFAULT, Could you show me where we "set our own result to include CERT_STATUS_UNABLE_TO_CHECK_REVOCATION"? Note that CERT_STATUS_UNABLE_TO_CHECK_REVOCATION causes Chromium to show a warning info-bar and some sort of warning indicator, so we should not include CERT_STATUS_UNABLE_TO_CHECK_REVOCATION when the user has revocation disabled.
http://codereview.chromium.org/174102/diff/1036/1041 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1036/1041#newcode438 Line 438: // revocation disabled (which is the default), then we will get On 2009/08/25 19:08:38, wtc wrote: > Is it possible to find out programmatically whether the > user has revocation enabled or disabled? Yes, the settings are stored in ~/Library/Preferences/com.apple.security.revocation.plist. Should we use just the system-wide settings for revocation checking and not SSLConfig? I say yes: we'll get the same behavior as other applications, and less user confusion (since we're subject to the settings anyway, this will eliminate the problem of an SSLConfig with rev_checking_enabled set to true but the system-wide settings set to false giving unexpected results). http://codereview.chromium.org/174102/diff/1036/1041#newcode442 Line 442: // CERT_STATUS_UNABLE_TO_CHECK_REVOCATION (note that this does not apply On 2009/08/25 19:08:38, wtc wrote: > Could you show me where we "set our own result to include > CERT_STATUS_UNABLE_TO_CHECK_REVOCATION"? CertStatusFromOSStatus() performs the mapping. > Note that CERT_STATUS_UNABLE_TO_CHECK_REVOCATION causes > Chromium to show a warning info-bar and some sort of warning > indicator, so we should not include > CERT_STATUS_UNABLE_TO_CHECK_REVOCATION when the user has > revocation disabled. Will do (want to get an answer on the system-wide revocation checking before I dive into more changes)
http://codereview.chromium.org/174102/diff/1014/22 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/22#newcode86 Line 86: // Failure was due to something Chromium doesn't define a On 2009/08/25 19:08:38, wtc wrote: > On 2009/08/22 01:43:45, hawk wrote: > > On 2009/08/21 21:38:23, wtc wrote: > > > Are there OSStatus codes that are equivalent to > > > CERT_STATUS_NO_REVOCATION_MECHANISM or > > > > CSSMERR_APPLETP_CRL_NOT_FOUND is close; there's no OCSP status code that means > > "OCSP not checked because there was no responder URL in the certificate" > > Is it possible to map CSSMERR_APPLETP_CRL_NOT_FOUND to > our CERT_STATUS_NO_REVOCATION_MECHANISM? Will do. What's the proper CERT_STATUS for "there's no revocation information associated with this certificate chain" i.e., there's no way to check revocation, even though the SSLConfig asked for it? After some experimentation, we get CSSMERR_APPLETP_INCOMPLETE_REVOCATION_CHECK back from SecTrustEvaluate() in this situation (e.g., when running the unit tests). As of CL 23998 the SSLConfig used by the unit tests has enable_rev_checking set to true. Based on that flag, we try to perform revocation checking, fail, and return CERT_STATUS_NO_REVOCATION_MECHANISM, thus failing the unit tests. http://codereview.chromium.org/174102/diff/1014/22#newcode477 Line 477: case kSecTrustResultConfirm: On 2009/08/25 19:08:38, wtc wrote: > On 2009/08/22 01:43:45, hawk wrote: > > For kSecTrustResultConfirm, we're > > following what Secure Transport does, which is treat it as "Deny". > > Would be nice to add this to the source code as a comment. > > Done.
Because this CL is very long, I'm starting to have code review fatigue :-) We should check it in when it's at a good checkpoint, and make improvements in follow-up CLs. http://codereview.chromium.org/174102/diff/1014/22 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/22#newcode86 Line 86: // Failure was due to something Chromium doesn't define a On 2009/08/25 21:53:50, hawk wrote: > What's the proper CERT_STATUS for "there's no revocation information > associated with this certificate chain" i.e., there's no way to check > revocation, even though the SSLConfig asked for it? That's CERT_STATUS_NO_REVOCATION_MECHANISM. It essentially means the cert has no CRL distribution point extension and no AIA extension (that contains the OCSP responder URL). Chromium ignores the CERT_STATUS_NO_REVOCATION_MECHANISM status, like other browsers. > After some experimentation, > we get CSSMERR_APPLETP_INCOMPLETE_REVOCATION_CHECK back from SecTrustEvaluate() > in this situation (e.g., when running the unit tests). As of CL 23998 the > SSLConfig used by the unit tests has enable_rev_checking set to true. Yeah, that's because the SSLConfigService used on the Mac returns hardcoded default values right now. See SSLConfigService::CreateSystemSSLConfigService(). It needs to be fixed to return the system setting for revocation checking instead. http://codereview.chromium.org/174102/diff/1036/1041#newcode438 Line 438: sizeof(tp_action_data)); On 2009/08/25 21:12:48, hawk wrote: > > Yes, the settings are stored in > ~/Library/Preferences/com.apple.security.revocation.plist. > > Should we use just the system-wide settings for revocation checking and not > SSLConfig? Yes, but it's best if SSLConfig has the same value as the system-wide setting for revocation checking. The members of SSLConfig are set by the SSLConfigService object. So the Mac port of SSLConfigService should set the rev_checking_enabled member of SSLConfig to the system setting for revocation checking.
http://codereview.chromium.org/174102/diff/1014/22 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/22#newcode86 Line 86: // Failure was due to something Chromium doesn't define a On 2009/08/25 21:53:50, hawk wrote: > As of CL 23998 the > SSLConfig used by the unit tests has enable_rev_checking set to true. We can revisit that change. I want rev_checking_enabled to default to true on Linux. (Linux doesn't have system settings for revocation checking.) That can be accomplished in another way.
OK, hopefully this finished things up :-) Changes since the last time involve removing the use of the certificate cache for intermediate certs and the test root. The intermediate certs are now added to the server cert's X509Certificate object, so they are kept where they are needed and only where they are needed. The trusted root is kept in a nascent trusted certificate store that's specific to Mac, a la the NSS certificate DB. Right now, this store does just enough to hold the test root and nothing else. I have no plans to add to it, but it's easy enough to do if the need arises in the future. Revocation checking is present, but is never called. This is due to the change in SSLConfig that caused revocation to always be requested on the Mac (which breaks the unit tests, as well as most https sites). There's a TODO in there now to enable it, and I'll get to that shortly (not in this CL, of course!) http://codereview.chromium.org/174102/diff/1014/22 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1014/22#newcode86 Line 86: // Failure was due to something Chromium doesn't define a On 2009/08/25 22:15:13, wtc wrote: > On 2009/08/25 21:53:50, hawk wrote: > > As of CL 23998 the > > SSLConfig used by the unit tests has enable_rev_checking set to true. > > We can revisit that change. I want rev_checking_enabled > to default to true on Linux. (Linux doesn't have system > settings for revocation checking.) That can be accomplished > in another way. I'll disable revocation checking in this CL and re-enable it in a follow up CL, based on the system-wide settings. http://codereview.chromium.org/174102/diff/1036/1041#newcode41 Line 41: CFArrayGetValueAtIndex(reinterpret_cast<CFArrayRef>( On 2009/08/25 18:26:07, wtc wrote: > Let's add a LOG(ERROR) statement with the value of 'status' > that we map to ERR_FAILED, so that we can improve the > NetErrorFromOSStatus function. ERR_FAILED is our default > error code. It doesn't tell us anything, so it's painful > to see the ERR_FAILED error code in a bug report. Done. http://codereview.chromium.org/174102/diff/1036/1038 File net/socket/ssl_client_socket_mac.cc (right): http://codereview.chromium.org/174102/diff/1036/1038#newcode255 Line 255: if (!CFArrayGetCount(certs)) On 2009/08/25 18:26:07, wtc wrote: > Note: please merge with my CL > http://codereview.chromium.org/173328 > carefully here. I found that the problem is that 'certs' > is NULL, not that the array count is 0. So I suggest > that we leave the DCHECK_GT assertion unchanged. Done. http://codereview.chromium.org/174102/diff/1036/1038#newcode512 Line 512: return ERR_SSL_PROTOCOL_ERROR; On 2009/08/25 18:26:07, wtc wrote: > We should use another error code here. ERR_SSL_PROTOCOL_ERROR > during handshake means the handshake failed. Here the > handshake succeeded, just that there's no server cert. > Perhaps return ERR_UNEXPECTED or invent a new error code > (ERR_ANONYMOUS_CIPHER_SUITE?). > > Note: When TLS is enabled, ERR_SSL_PROTOCOL_ERROR during > handshake will trigger a retry with TLS disabled in our > HTTP layer. All browsers do this to support > "TLS_intolerant servers", which don't support a > ClientHello message requesting version 3.1. Done. http://codereview.chromium.org/174102/diff/1036/1038#newcode522 Line 522: if (!server_cert_) On 2009/08/25 18:26:07, wtc wrote: > I think the original DCHECK is good enough here. > We can only enter this state from DoHandshake after > server_cert_ is set to a non-NULL value. > > In any case, the error code should match the error code > used on line 512 in DoHandshake. Done. http://codereview.chromium.org/174102/diff/1036/1038#newcode527 Line 527: // and makes them available X509Certificate::Verify() for chain building. On 2009/08/25 18:26:07, wtc wrote: > Nit: add "to" after "available". Done.
http://codereview.chromium.org/174102/diff/1050/1054 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1050/1054#newcode448 Line 448: CFMutableArrayRef cert_array = CFArrayCreateMutable(kCFAllocatorDefault, 1, See comment below about the capacity. http://codereview.chromium.org/174102/diff/1050/1054#newcode654 Line 654: intermediate_ca_certs_ = CFArrayCreateMutable(kCFAllocatorDefault, 1, Do we want to cap the size of the intermediate cert array at 1? Don't forget that while the "capacity" of NSMutableArray's arrayWithCapacity: is advisory, the "capacity" of CFArrayCreateMutable is a strict cap. The second parameter should be 0 unless there's some reason to cap it.
http://codereview.chromium.org/174102/diff/1050/1054 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/1050/1054#newcode448 Line 448: CFMutableArrayRef cert_array = CFArrayCreateMutable(kCFAllocatorDefault, 1, On 2009/08/26 23:27:54, Avi wrote: > See comment below about the capacity. Done. http://codereview.chromium.org/174102/diff/1050/1054#newcode654 Line 654: intermediate_ca_certs_ = CFArrayCreateMutable(kCFAllocatorDefault, 1, On 2009/08/26 23:27:54, Avi wrote: > Do we want to cap the size of the intermediate cert array at 1? Don't forget > that while the "capacity" of NSMutableArray's arrayWithCapacity: is advisory, > the "capacity" of CFArrayCreateMutable is a strict cap. The second parameter > should be 0 unless there's some reason to cap it. Ouch, no. I was treating like NSArray. Fixed.
The code LGTM. Re the commit comment, does your change to the test code obviate the need to manually load the test root? If so, please change the commit comment.
On 2009/08/26 23:49:05, Avi wrote: > The code LGTM. Re the commit comment, does your change to the test code obviate > the need to manually load the test root? Yes. > If so, please change the commit comment. Done, and I'll see about getting the instructions updated to reflect the fact that the tests are enabled and you don't need to do anything.
LGTM. Yes, please update the description of the CL before you check this in. You can make the suggested changes below in a follow-up CL. http://codereview.chromium.org/174102/diff/53/1070 File net/base/x509_certificate.h (right): http://codereview.chromium.org/174102/diff/53/1070#newcode213 Line 213: // Adds an untrusted intermediate certificate that may be needed for Nit: Fix indentation. http://codereview.chromium.org/174102/diff/53/1070#newcode311 Line 311: CFMutableArrayRef intermediate_ca_certs_; intermediate_certs_ is fine. I found that Mac OS X documentation uses the term "intermediate certificate", so I'm now used to this term. But actually, why don't we just stick the CFArrayRef returned by SSLCopyPeerCertificates here as CFArrayRef cert_chain_; Then we can just pass the cert_chain_ member directly to SecTrustCreateWithCertificates in X509Certificate::Verify. This avoids the calls to AddIntermediateCertificate and rebuilding cert_array from intermediate_ca_certs_. http://codereview.chromium.org/174102/diff/53/1071 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/53/1071#newcode21 Line 21: class MacTrustedCertificates { See if you can put this class inside #ifdef UNIT_TEST. http://codereview.chromium.org/174102/diff/53/1071#newcode73 Line 73: void SetMacTestCertificate(X509Certificate* cert) { We should declare this function in x509_certificate.h. It can be put inside #ifdef UNIT_TEST. http://codereview.chromium.org/174102/diff/53/1071#newcode469 Line 469: // Set the trusted anchor certificates for the SecTrustRef by merging the See if you can put this block of code (lines 469 - 478) inside #ifdef UNIT_TEST. http://codereview.chromium.org/174102/diff/53/1073 File net/socket/ssl_client_socket_mac.cc (right): http://codereview.chromium.org/174102/diff/53/1073#newcode541 Line 541: // TODO(hawk): set flags based on the SSLConfig, once SSLConfig is Nit: the second "SSLConfig" should be "SSLConfigService".
Some comments on OverrideHostnameMismatch (re: http://crbug.com/20276). http://codereview.chromium.org/174102/diff/53/1071 File net/base/x509_certificate_mac.cc (right): http://codereview.chromium.org/174102/diff/53/1071#newcode156 Line 156: // even if the certificate contains 127.0.0.1 as one of its names. Change "one of its names" to "the commonName in the subject field". It's unknown whether we will still get a hostname mismatch if our test server cert contains 127.0.0.1 as iPAddress in the subjectAltName extension. http://codereview.chromium.org/174102/diff/53/1071#newcode157 Line 157: // We, however, want to allow that behavior. SecTrustEvaluate() Add something like "to be compatible with Windows CryptoAPI and NSS". http://codereview.chromium.org/174102/diff/53/1071#newcode580 Line 580: GetDNSNames(&names); We should use only the common name in the subject field here. GetDNSNames returns either the dNSName's in the subjectAltName extension, or the commonName in the subject field. We shouldn't use the dNSName's in subjectAltName here. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
