SSLCertRequestInfo: Add |valid_cas| and |valid_key_types|

SSLCertRequestInfo: Add |valid_cas| and |valid_key_types|

On Android, it is not possible to determine the list of compatible
client certificates from a given server CertificateRequest message
without prompting the user.

Due to this, add new fields to SSLCertRequestInfo:

  - |no_client_certs| to indicate that |client_certs| should
    be ignored. Note that |client_certs| is not removed to
    keep existing unit tests working.

  - |valid_cas| the list of valid certificate authorities
    passed by the server.

  - |valid_key_types| the list of valid certificate signing
    key types passed by the server.

This patch introduces a new X509Certificate method
(IsValidClientCertificate) to check a given certificate against
a given SSLCertRequestInfo. This uses either the |client_certs|
list of the |valid_cas|/|valid_key_types| one.

Future patches will use these new fields to query the Android
platform APIs for the right certificate chain and private key
alias.

BUG=134418

[digit1, 2012-12-11 17:50:51]

This definitely is a first try to implement the changes that were discussed
recently to properly support client certificates on Android.

I have tested that this builds on Android, and doesn't break the net_unittests,
but I'll run other tests later.

In the meantime, I'd appreciate your comments and recommendations. To be more
precise, is there a good way to check a certificate against a |valid_cas| list?
It looks like comparing X509_NAME with OpenSSL is frought with perils.

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

My biggest concern here is the shared-code that is having #if
defined(USE_OPENSSL) introduced to it.

What we discussed on e-mail was how all platforms can share this pattern. I'm
concerned with this CL because it does not make it clear who will own that
refactoring cost, which means that we now have to maintain two,
slightly-incompatible implementations for some unknown period of time. That was
the point of my e-mail - that this behaviour you're implementing for Android is
actually desirable for all platforms, so it would be good to simply do it for
all platforms. It should not require much (any?) platform-specific knowledge to
do so.

If you are planning on doing this for all platforms, then I think the concerns
are:
1) We should be trying not to introduce more #ifdefs within .h files. In
general, an #ifdef in a .h indicates a broken cross-platform abstraction, and we
should strive to unify that when possible.
2) We should not be using CertPrincipal for anything other than end-user
presentation
3) We should make sure that layering dependencies, even within a path
(net/base), should be adhered to.


Rather than bringing up a parallel interface for Android first, and then trying
to fit the other code to it, I would suggest that it might be easier to adapt
the existing code (non-openssl) into the end-goal that you're desiring, and then
implement Android in that. If anything, it will make sure that we're not having
to maintain parallel test infrastructures, since you will have already updated
the old code by the time you're introducing new code.

https://codereview.chromium.org/11458012/diff/1/net/base/ssl_cert_request_info.h
File net/base/ssl_cert_request_info.h (right):

https://codereview.chromium.org/11458012/diff/1/net/base/ssl_cert_request_inf...
net/base/ssl_cert_request_info.h:39: bool no_client_certs;
I don't think we should be expressing this value on the SSLCertRequestInfo
itself.

It should be in the code, and it SHOULD be fixed so that all platforms behave
the same (at some later point).

You should put a TODO here to reference the bug (or, ideally, attempt the
refactoring yourself, since all the implementation exists for the other
platforms and just needs to be brought out)

https://codereview.chromium.org/11458012/diff/1/net/base/ssl_cert_request_inf...
net/base/ssl_cert_request_info.h:57: #if defined(USE_OPENSSL)
I strongly dislike #ifdefs for shared code like this.

https://codereview.chromium.org/11458012/diff/1/net/base/ssl_cert_request_inf...
File net/base/ssl_cert_request_info_openssl.cc (right):

https://codereview.chromium.org/11458012/diff/1/net/base/ssl_cert_request_inf...
net/base/ssl_cert_request_info_openssl.cc:3: // found in the LICENSE file.
We should not be creating a new platform-specific file here.

https://codereview.chromium.org/11458012/diff/1/net/base/x509_cert_types.h
File net/base/x509_cert_types.h (right):

https://codereview.chromium.org/11458012/diff/1/net/base/x509_cert_types.h#ne...
net/base/x509_cert_types.h:124: #if defined(OS_MACOSX) || defined(USE_OPENSSL)
NACK on this. CertPrincipal::Compare() should NOT be encouraged or used.

https://codereview.chromium.org/11458012/diff/1/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

https://codereview.chromium.org/11458012/diff/1/net/base/x509_certificate.cc#...
net/base/x509_certificate.cc:706: DCHECK(cert_info.no_client_certs == false);
While this impl is NACKed, as a style nit, this DCHECK style is discouraged.

DCHECK_EQ(false, cert_info.no_client_certs)

which quickly reveals it's better as

DCHECK(!cert_info.no_client_certs);

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

https://codereview.chromium.org/11458012/diff/1/net/base/x509_certificate.h#n...
net/base/x509_certificate.h:310: bool IsValidClientCertificate(const
SSLCertRequestInfo& cert_info);
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)

https://codereview.chromium.org/11458012/diff/1/net/base/x509_certificate_ope...
File net/base/x509_certificate_openssl.cc (right):

https://codereview.chromium.org/11458012/diff/1/net/base/x509_certificate_ope...
net/base/x509_certificate_openssl.cc:517: #endif
While NACKed, I don't think we should add unreachable code via #if 0. If it's
not being implemented in this go-around, it's better to leave it out than to
suggest it would work, since it may not at some later point, and if anyone
changes things, they may think they have to keep this code up to date when doing
so.

Further, TODOs like this should reference a bug.

https://codereview.chromium.org/11458012/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/11458012/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_openssl.cc:663:
cert_request_info->valid_cas.push_back(std::string());
I think this comment is ambiguous - it suggests that the assignment is not
copying |encoded_name|, even though there's no standards-compliant way it could.

Further, it discounts compiler-optimizations with regards to RVO and placements.
That is, it's legal for it to recognize that

.push_back(std::string(reinterpret_cast<...>, ...)) is creating a temporary, and
to have the std::string() invoked as part of the new type used for push_back

*in general*, such cleverness is strongly avoided in Chromium code unless it's a
demonstrated necessity. The cognitive overhead (and both the comment and the
assign required me to double check what's going on here) is not worth the
perceived benefits.

https://codereview.chromium.org/11458012/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_openssl.cc:667: count++;
post-increment

https://codereview.chromium.org/11458012/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_openssl.cc:675: // RSA, which is by far the most
common one. crbug.com/165446
http:// linkify

[digit1, 2012-12-11 22:48:20]

On 2012/12/11 21:30:24, Ryan Sleevi wrote:
> My biggest concern here is the shared-code that is having #if
> defined(USE_OPENSSL) introduced to it.
> 
> What we discussed on e-mail was how all platforms can share this pattern. I'm
> concerned with this CL because it does not make it clear who will own that
> refactoring cost, which means that we now have to maintain two,
> slightly-incompatible implementations for some unknown period of time. That
was
> the point of my e-mail - that this behaviour you're implementing for Android
is
> actually desirable for all platforms, so it would be good to simply do it for
> all platforms. It should not require much (any?) platform-specific knowledge
to
> do so.
> 
My priority is to get client certificate support working on Android for M25
asap, and I'm wary that trying to implement the "generic" solution up-front will
drag this effort too much. From my perspective, it's really hard to quantify
whether such effort will result in difficult or invasive changes to the code
base. I know that the #ifdefs are not welcomed but at least they give me the
guarantee that I'm not going to screw all other platforms due to subtle
platform-specific issues that are fairly difficult to foresee, imho.

However, I'll take your work for it, so let's discuss the general solution here.

I think it's clear that |valid_cas| and |valid_key_types| must be exposed in
SSLCertRequestInfo, since there is no way to populate |client_certs| properly on
Android.

This would hint at providing a new platform-specific function that would map a
(valid_cas,valid_key_types) tuple to a (client_certs) list, for example:

  // Return into |out| a list of compatible pre-installed client certificates
  // matching this SSLCertRequestInfo. Returns true on success.
  //
  // Note that the function will return successfully with an empty list to
indicate
  // that there is no matching pre-installed certificate. Failure corresponds to
the
  // case where the platform-specific APIs cannot produce such list (e.g.
Android).
  // 
  bool net::SSLCertRequestInfo::GetClientCertificates(
      std::vector<scoped_refptr<X509Certificate> >* out);

This also means getting rid of |client_certs| in SSLCertRequestInfo and ensuring
that all code that currently accesses it will use the new
GetClientCertificates() method instead.

However, I'm not sure this will work nicely, when some of the caller code will
be in the UI thread. What do you think about this? Do you have a better
proposal?

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

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/ssl_cert_requ...
File net/base/ssl_cert_request_info.h (right):

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/ssl_cert_requ...
net/base/ssl_cert_request_info.h:39: bool no_client_certs;
On 2012/12/11 21:30:24, Ryan Sleevi wrote:
> I don't think we should be expressing this value on the SSLCertRequestInfo
> itself.
> 
> It should be in the code, and it SHOULD be fixed so that all platforms behave
> the same (at some later point).
> 
> You should put a TODO here to reference the bug (or, ideally, attempt the
> refactoring yourself, since all the implementation exists for the other
> platforms and just needs to be brought out)

There is some code in the content or browser layer that currently assumes that
an empty |client_certs| means there are no matching certificates. In this case,
any certificate selection is short-cutted and nothing works. That's why I had to
add this field.

I assume we could add a ::GetClientCertificates() method here instead, but it
will have to be callable from multiple threads.

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/ssl_cert_requ...
File net/base/ssl_cert_request_info_openssl.cc (right):

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/ssl_cert_requ...
net/base/ssl_cert_request_info_openssl.cc:3: // found in the LICENSE file.
On 2012/12/11 21:30:24, Ryan Sleevi wrote:
> We should not be creating a new platform-specific file here.

This is another consequence of the requirement for the |no_client_certs| flag.
It could probably be lifted if we get rid of it.

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/x509_cert_typ...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:124: #if defined(OS_MACOSX) || defined(USE_OPENSSL)
On 2012/12/11 21:30:24, Ryan Sleevi wrote:
> NACK on this. CertPrincipal::Compare() should NOT be encouraged or used.

Oh point, taken. Actually, that's a left-over, there is no implementation/use
for this method anyway.

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/x509_certific...
File net/base/x509_certificate.cc (right):

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/x509_certific...
net/base/x509_certificate.cc:706: DCHECK(cert_info.no_client_certs == false);
Oh thanks, point taken, will do.

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/x509_certific...
File net/base/x509_certificate.h (right):

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/x509_certific...
net/base/x509_certificate.h:310: bool IsValidClientCertificate(const
SSLCertRequestInfo& cert_info);
On 2012/12/11 21:30:24, Ryan Sleevi wrote:
> 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.
> 
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).

> 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)

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.

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/x509_certific...
File net/base/x509_certificate_openssl.cc (right):

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/x509_certific...
net/base/x509_certificate_openssl.cc:517: #endif
Thanks, I'll remove the #if .. #endif block. A bug would be nice, but I doubt
I'll have the time to work on this any time soon (adding CLIENT_CERT_DSA_SIGN is
trivial, adding the proper unit tests and code coverage is,..., more interesting
:))

https://chromiumcodereview.appspot.com/11458012/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://chromiumcodereview.appspot.com/11458012/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:663:
cert_request_info->valid_cas.push_back(std::string());
On 2012/12/11 21:30:24, Ryan Sleevi wrote:
> I think this comment is ambiguous - it suggests that the assignment is not
> copying |encoded_name|, even though there's no standards-compliant way it
could.
>
I understand, it really means that a
.push_back(std::string(encoded_name,encoded_len)) would generate two memory
copies of the data, in the absence of C++11 move semantics. It will be removed
anyway (see below).

 
> Further, it discounts compiler-optimizations with regards to RVO and
placements.
> That is, it's legal for it to recognize that
> 
> .push_back(std::string(reinterpret_cast<...>, ...)) is creating a temporary,
and
> to have the std::string() invoked as part of the new type used for push_back
> 
> *in general*, such cleverness is strongly avoided in Chromium code unless it's
a
> demonstrated necessity.

Ah, someone actually recommended this technique in a previous unrelated patch.
I'm completely ok doing this the standard way :)


> The cognitive overhead (and both the comment and the
> assign required me to double check what's going on here) is not worth the
> perceived benefits.

https://chromiumcodereview.appspot.com/11458012/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:675: // RSA, which is by far the most
common one. crbug.com/165446
Sure, will do.

[Ryan Sleevi, 2012-12-11 23:42:19]

On 2012/12/11 22:48:20, digit1 wrote:
> On 2012/12/11 21:30:24, Ryan Sleevi wrote:
> > My biggest concern here is the shared-code that is having #if
> > defined(USE_OPENSSL) introduced to it.
> > 
> > What we discussed on e-mail was how all platforms can share this pattern.
I'm
> > concerned with this CL because it does not make it clear who will own that
> > refactoring cost, which means that we now have to maintain two,
> > slightly-incompatible implementations for some unknown period of time. That
> was
> > the point of my e-mail - that this behaviour you're implementing for Android
> is
> > actually desirable for all platforms, so it would be good to simply do it
for
> > all platforms. It should not require much (any?) platform-specific knowledge
> to
> > do so.
> > 
> My priority is to get client certificate support working on Android for M25
> asap, and I'm wary that trying to implement the "generic" solution up-front
will
> drag this effort too much. 

In general, comments like this give pause, since the point of the rapid release
schedule was that, if a feature isn't ready for Milestone X, it can make it to
Milestone X+1. My big concern here is the impact (cognitive, quality, test
coverage), and while not necessary to get things "Perfect", I do want to make
sure we're not creating busy work here.

> From my perspective, it's really hard to quantify
> whether such effort will result in difficult or invasive changes to the code
> base. I know that the #ifdefs are not welcomed but at least they give me the
> guarantee that I'm not going to screw all other platforms due to subtle
> platform-specific issues that are fairly difficult to foresee, imho.
> 
> However, I'll take your work for it, so let's discuss the general solution
here.
> 
> I think it's clear that |valid_cas| and |valid_key_types| must be exposed in
> SSLCertRequestInfo, since there is no way to populate |client_certs| properly
on
> Android.

Agreed 100%. I think this information should be exposed on all platforms. And
you'll find that |valid_key_types| ends up needing to be fudged just about
everywhere, just like Android, but as you discovered, we effectively assume RSA
(or more aptly, that 'any' are valid)

> 
> This would hint at providing a new platform-specific function that would map a
> (valid_cas,valid_key_types) tuple to a (client_certs) list, for example:
> 
>   // Return into |out| a list of compatible pre-installed client certificates
>   // matching this SSLCertRequestInfo. Returns true on success.
>   //
>   // Note that the function will return successfully with an empty list to
> indicate
>   // that there is no matching pre-installed certificate. Failure corresponds
to
> the
>   // case where the platform-specific APIs cannot produce such list (e.g.
> Android).
>   // 
>   bool net::SSLCertRequestInfo::GetClientCertificates(
>       std::vector<scoped_refptr<X509Certificate> >* out);
> 
> This also means getting rid of |client_certs| in SSLCertRequestInfo and
ensuring
> that all code that currently accesses it will use the new
> GetClientCertificates() method instead.
> 
> However, I'm not sure this will work nicely, when some of the caller code will
> be in the UI thread. What do you think about this? Do you have a better
> proposal?

We don't want to do it on the UI thread on all platforms, just like we don't
want to do it on the IO thread on all platforms.

Long term, we want to make things async, but I think it may actually be fine to
do sync here (it's no *worse* then what we're already doing today on the IO
thread on all platforms)

class ClientCertStore {
 public:
  // TODO(rsleevi): File a bug to make it async
  virtual void GetClientCertificates(
      const std::vector<std::string>& client_cas,
      const std::vector<net::ClientCertType>& cert_types,
      net::CertificateList* certs) = 0;
};


GetClientCertificates is implemented in terms of the contents of
SSLClientSocket* client cert handlers (eg: PlatformClientAuthHandler /
ClientAuthHandler)

HttpNetworkTransaction changes look similar to what you've already got - it
calls some function and asks, given an X509Certificate*, a set of CAs, and a
cert type, is this still valid?

content/browser/loader/resource_loader.cc gets updated to remove the
client_certs.empty() check

content/browser/ssl/ssl_client_auth_handler.cc doesn't change (I don't think)

Then in chrome/browser/chrome_content_browser_client.cc (eg: Desktop), you add:

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO))
net::CertificateList client_certs;
client_cert_store_->GetClientCerts(request_info.client_cas,
request_info.client_cert_types, &client_certs);

if (client_certs.empty()) {
  callback.Run(NULL);
  return;
}

/* else, all the existing code that presumes client_certs is known at the time.
Updating every caller from checking the SSLCertRequestInfo::client_certs to
receiving a net::CertificateList directly */

Whereas in your Android content/ client, you can then spawn the UI to do the
selection.


That should have all of the same performance characteristics for desktop (SSL
client certs found on the IO thread), BUT it gives you a sufficient pivot point
to move stuff over the UI thread and do the detection & selection there.

WDYT?

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

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/x509_certific...
File net/base/x509_certificate.h (right):

https://chromiumcodereview.appspot.com/11458012/diff/1/net/base/x509_certific...
net/base/x509_certificate.h:310: bool IsValidClientCertificate(const
SSLCertRequestInfo& cert_info);
On 2012/12/11 23:05:31, digit1 wrote:
> On 2012/12/11 21:30:24, Ryan Sleevi wrote:
> > 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.
> > 
> 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).

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.

> 
> > 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)
> 
> 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.

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

[wtc1, 2012-12-15 00:56:18]

Review comments on patch set 2:

Sorry about the delay. I only reviewed the header file
changes.

https://chromiumcodereview.appspot.com/11458012/diff/8001/net/base/ssl_cert_r...
File net/base/ssl_cert_request_info.h (right):

https://chromiumcodereview.appspot.com/11458012/diff/8001/net/base/ssl_cert_r...
net/base/ssl_cert_request_info.h:39: bool no_client_certs;

(I didn't read the previous discussions in this code review.)
I think we should switch to the new way on all platforms, not
just on Android.

https://chromiumcodereview.appspot.com/11458012/diff/8001/net/base/ssl_cert_r...
net/base/ssl_cert_request_info.h:64: std::vector<SSLClientCertType>
valid_key_types;

I suggest using the names from the TLS protocol (see the
CertificateRequest struct on lines 51-54.

"cas" is especially hard to read because it's all lowercase.

But I agree "key types" is more accurate than "certificate
types"...

https://chromiumcodereview.appspot.com/11458012/diff/8001/net/base/x509_cert_...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/11458012/diff/8001/net/base/x509_cert_...
net/base/x509_cert_types.h:128: // TODO(rsleevi): Remove once Mac client auth
uses NSS for name comparison.

This TODO comment should be updated now that we added
defined(USE_OPENSSL) to the ifdef.

https://chromiumcodereview.appspot.com/11458012/diff/8001/net/base/x509_certi...
File net/base/x509_certificate.h (right):

https://chromiumcodereview.appspot.com/11458012/diff/8001/net/base/x509_certi...
net/base/x509_certificate.h:308: // Does this certificate matches the SSL
CertificateRequest parameters

Typo: matches => match

https://chromiumcodereview.appspot.com/11458012/diff/8001/net/base/x509_certi...
net/base/x509_certificate.h:310: bool IsValidClientCertificate(const
SSLCertRequestInfo& cert_info);

"Valid" is ambiguous. How about "Acceptable"?

Alternatively, this method can be named MatchesClientCertRequest.

[digit1, 2012-12-18 15:19:14]

https://chromiumcodereview.appspot.com/11458012/diff/8001/net/base/ssl_cert_r...
File net/base/ssl_cert_request_info.h (right):

https://chromiumcodereview.appspot.com/11458012/diff/8001/net/base/ssl_cert_r...
net/base/ssl_cert_request_info.h:39: bool no_client_certs;
Yes, this is now the plan, first patch to achieve this is at
https://chromiumcodereview.appspot.com/11579002/, with more to come.

https://chromiumcodereview.appspot.com/11458012/diff/8001/net/base/ssl_cert_r...
net/base/ssl_cert_request_info.h:64: std::vector<SSLClientCertType>
valid_key_types;
I agree, I wasn't really satisfied with these names. I propose the following:

  cert_authorities  (or cert_issuers)
  cert_key_types
  signature_algorithms  (for TLS 1.2)

(to be implemented in a different patch though).

[digit1, 2013-02-07 09:45:44]

This patch is now obsolete, thanks to ppi's work. I'm going to abandon it.