SSLCertRequestInfo: Add |valid_cas| and |valid_key_types|

net/base/x509_certificate.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_BASE_X509_CERTIFICATE_H_
 #define NET_BASE_X509_CERTIFICATE_H_
 
 #include <string.h>
 
 #include <string>
@@ skipping 27 matching lines @@
 class PickleIterator;
 
 namespace crypto {
 class RSAPrivateKey;
 }  // namespace crypto
 
 namespace net {
 
 class CRLSet;
 class CertVerifyResult;
+class SSLCertRequestInfo;
 
 typedef std::vector<scoped_refptr<X509Certificate> > CertificateList;
 
 // X509Certificate represents a X.509 certificate, which is comprised a
 // particular identity or end-entity certificate, such as an SSL server
 // identity or an SSL client certificate, and zero or more intermediate
 // certificates that may be used to build a path to a root certificate.
 class NET_EXPORT X509Certificate
     : public base::RefCountedThreadSafe<X509Certificate> {
  public:
@@ skipping 239 matching lines @@
   CFArrayRef CreateClientCertificateChain() const;
 
   // Returns a new CFArrayRef containing this certificate and its intermediate
   // certificates in the form expected by Security.framework and Keychain
   // Services, or NULL on failure.
   // The first item in the array will be this certificate, followed by its
   // intermediates, if any.
   CFArrayRef CreateOSCertChainForCert() const;
 #endif
 
+  // Does this certificate matches the SSL CertificateRequest parameters
+  // stored in |cert_info|.
+  bool IsValidClientCertificate(const SSLCertRequestInfo& cert_info);

[Ryan Sleevi, 2012/12/11 21:30:24]

NACK on this.

Comparison for client certificates is done based on constructed chains, and is a
function of chain building, not an instrinsic function of certificates.

We should also NOT be depending on SSLCertRequestInfo in X509Certificate (in
general). It's a design layering violation, given their circular dependency
(SSLCertRequestInfo holds X509Certificates, therefore it's at a design layer
"higher" then X509Certificate)

[digit1, 2012/12/11 23:05:31]

The comment is misleading, sorry. This only moved some existing code that checks
the certificate against |client_certs|. As far as I understand, it's not doing
pure certificate validation, just checking that the certificate is already
listed in |client_certs|. Of course, this is meaningless on Android.

Also, I refrained from implementing the actual |valid_cas| verification for
openssl, given that clearly it looks like there are many dragons in this field
(the code just checks the key types).

Just for the record, what's the purpose of the original check? Wouldn't the
server be able to reject a bad certificate anyway? (and it doesn't seem the
error returned would provide any useful customization of the UI).
Point taken. I don't think the location of the function matters much. Could this
be a method of SSLCertRequestInfo(), or put into x509_util.h, or somewhere else?

I still think a standalone function is better than having multiple clients check
|client_certs| manually.

[Ryan Sleevi, 2012/12/12 00:05:40]

The server's CertificateRequest indicates who the client cert should be signed
by. The existing implementation ('suboptimal') first gathers all the possible
client certificates and THEN checks to see if the one it was using was still in
the list.

The 'ideal' way is simply: if we have a client certificate, check that there is
an intersection between:

Set(Issuers of X509Certificate* + X509Certificate->GetIntermediateCerts());
Set(client_cas)

Using the appropriate name equality function (X509_NAME_cmp[OpenSSL],
CERT_CompareName[NSS, OS X], CertCompareCertificateName[Win]) to do the
comparison. 

Further optimization (not necessary) would be caching the name-normalized form,
at which point we could just do a binary compare, but that's neither here nor
there.
I think we have two places today, and one of them is actively being refactored
away (the WebSocket guys are trying to get on board with HttpNetworkTransaction
and to refactor that code)

If we're willing to assume that |this| contains an already constructed chain
(rather then being used to *discover* a chain), I don't think there's a
particular issue putting this sort of logic in X509Certificate, especially since
(as I mentioned) it relies on some platform dependent types & library
comparisons. But it should not depend directly on SSLCertRequestInfo, and
arguably should not have the cert type check.

IsIssuedBy(...) [like we have on Mac] could be exposed as a member function for
all platforms

+
 #if defined(OS_WIN)
   // Returns a new PCCERT_CONTEXT containing this certificate and its
   // intermediate certificates, or NULL on failure. The returned
   // PCCERT_CONTEXT *MUST NOT* be stored in an X509Certificate, as this will
   // cause os_cert_handle() to return incorrect results. This function is only
   // necessary if the CERT_CONTEXT.hCertStore member will be accessed or
   // enumerated, which is generally true for any CryptoAPI functions involving
   // certificate chains, including validation or certificate display.
   //
   // Remarks:
@@ skipping 194 matching lines @@
   // based on the type of the certificate.
   std::string default_nickname_;
 #endif
 
   DISALLOW_COPY_AND_ASSIGN(X509Certificate);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_X509_CERTIFICATE_H_