Remember if a user declines to provide a server with a client certificate

Remember if a user declines to provide a server with a client certificate

When an SSL/TLS server requests a client certificate, the user may opt to not send any certificate. This will work if the server does not require a client certificate to be sent in order to load the resource - for example, to fall-back to forms-based authentication or when client certificates are configured as optional.

If the user declines to provide a client certificate, remember that choice in the SSLClientAuthCache so that they will not be repeatedly prompted to select a certificate (declining each time) for every sub-resource that loads, similar to how the SSLClientAuthCache remembers an explicit certificate selected by a user.

If the server requires a client certificate, it is expected that it will abort the TLS handshake with an appropriate TLS error code. When the server aborts and the error code is processed, the existing cached selection will be removed, and the user will be re-prompted to select a certificate on their next connection to that server.

If the server requires a client certificate to continue, but does not use the TLS stack to indicate that requirement - for example, to return a "friendlier" error page or an HTTP error code like 403, the selection will not be evicted from the cache, and the user must restart the browser before they will be prompted again for a certificate from that server. This is the same lifetime and behaviour as would happen if the user actively selected a certificate, rather than declined to provide one.

BUG=56177
TEST=SSLClientAuthCacheTest.LookupNullPreference . Also see the Apache configuration in http://crbug.com/56177. Access a site that requests, but does not require, client authentication, which will attempt to load multiple sub-resources, and which does not permit HTTP KeepAlives.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66931

[Ryan Sleevi, 2010-11-06 00:20:45]

Could you please take a look at this, and let me know if you agree with this
approach?

This CL changes the SSL client auth cache to also cache if the user *declines*
to provide a certificate. The ssl_config_ used for SSLClientSocket* has three
possible states:

1) Don't send a client cert
2) Send a client cert, but send an empty one.
3) Send a client cert, but send a specific one.

If the server sends a CertificateRequest message when the config is in state #1,
then an ERR_SSL_CLIENT_AUTH_CERT_NEEDED will be raised by the socket.

If the server sends a CertificateRequest message when the config is in state #2,
then the client will proceed through the CertificateRequest and not send any
cert. If the server aborts the handshake, ERR_BAD_SSL_CLIENT_AUTH_CERT will be
raised, indicating the server really wanted a client certificate. If the server
does not send an alert, then the connection will continue.

If the server sends a CertificateRequest message when the config is in State #3,
then it behaves the same as in State #2.

The current SSLClientAuthCache only caches State #3 decisions - when the user
selects a specific cert. If the user declines to choose a specific certificate,
nothing is cached. As a result, every connection returns
ERR_SSL_CLIENT_AUTH_CERT_NEEDED, and the user is prompted for every one of those
connections (and must decline, for every one of those connections)

This CL changes the behaviour to also cache if the user declined to provide a
cert. If the cache was "wrong", and the server required a certificate, then when
it aborts the handshake, the cache entry will be removed and the user prompted
at the next connection.

Overall, the user experience should be no worse than the existing user
experience if the user positively selects the "wrong" certificate (ie: one the
server rejects), and overall it should be better if the user has a client
certificate that they do not wish to send, and they're talking to a server whose
CertificateRequest message does not indicate any preferred CAs. 

For what it's worth, the UX issue is separately tracked at
http://crbug.com/29784 and requires a more involved solution.

[Ryan Sleevi, 2010-11-06 00:28:31]

Also, if this is an acceptable approach, can you let me know whether or not this
should be merged into any of the branches. This is related to
http://crbug.com/56019, but unlike that issue, this is not a specific regression
as far as I can tell.

[Ryan Hamilton, 2010-11-09 16:36:42]

This approach seems reasonable to me, and it looks like you got the logic right.
 I have two small suggestions that you can feel free to take or leave...

Do you think it might be easier to read if SSLClientAuthCache::Lookup returned
an enum that was one of: UNKNOWN, DO_NOT_SEND_CERT, SEND_CERT (or some such). 
And only if SEND_CERT was returned would the certificate pointer be non-null. 
This feels like it would be easier to read, but that's just my preference.

Also, would it make sense to add some tests of HttpNetworkTransaction that
exercise all of the codepaths?

[agl, 2010-11-09 17:04:06]

LGTM. (Sorry for not replying sooner. I hadn't starred the email. You should
ping me on a review if I haven't replied in one working day.)

r.e. rch's comments: I think SSLClientAuthCache::Lookup is fine.

More tests are always good, but LGTM in any case.

http://codereview.chromium.org/4568002/diff/1004/4004
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4568002/diff/1004/4004#newcode977
net/http/http_network_transaction.cc:977: // If the user selected one of the
certificate in client_certs for this
I think this comment has some grammar issues. (Not your new stuff, the original
comment.)

[Ryan Sleevi, 2010-11-12 05:50:25]

wtc: Ping?

[wtc, 2010-11-17 01:59:24]

rsleevi: I missed this CL, too.  I will review it tomorrow.

[wtc, 2010-11-18 01:33:07]

rsleevi: this looks good.  I suggest that you force the
caller to store the returned X509Certificate pointer in a
scoped_refptr.  What do you think?

http://codereview.chromium.org/4568002/diff/1004/net/base/ssl_client_auth_cac...
File net/base/ssl_client_auth_cache.h (right):

http://codereview.chromium.org/4568002/diff/1004/net/base/ssl_client_auth_cac...
net/base/ssl_client_auth_cache.h:35: // If a certificate preference is not
found, |*certificate| will be NULL.
This should say:
  If a certificate preference is not found, returns false.
It is best to leave |*certificate| unchanged when the function
fails.

http://codereview.chromium.org/4568002/diff/1004/net/base/ssl_client_auth_cac...
net/base/ssl_client_auth_cache.h:36: bool Lookup(const std::string& server,
X509Certificate** certificate);
Declare the output argument as
  scoped_refptr<X509Certificate>* certificate
to force the caller to store the returned X509Certificate
pointer in a scoped_refptr for proper reference counting.

Please update the comment block above accordingly when it
talks about |*certificate|.  (Perhaps the current text also
works for scoped_refptr.)

http://codereview.chromium.org/4568002/diff/1004/net/http/http_network_transa...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/4568002/diff/1004/net/http/http_network_transa...
net/http/http_network_transaction.cc:977: // If the user selected one of the
certificate in client_certs for this
Please fix my grammatical errors.  I guess we should say
"one of the certificates" (plural).  How about:
  If the user selected one of the certificates in client_certs
  or declined to provide one for this server before, use the
  past decision automatically.

http://codereview.chromium.org/4568002/diff/1004/net/http/http_network_transa...
net/http/http_network_transaction.cc:986: // If the user previously selected a
specific certificate, as opposed to
Nit: remove
  If the user previously selected a specific certificate, as opposed to
  declining to provide one,

http://codereview.chromium.org/4568002/diff/1004/net/http/http_network_transa...
net/http/http_network_transaction.cc:995: if
(X509Certificate::IsSameOSCert(client_cert->os_cert_handle(),
Use the new X509Certificate::Equals method:
  if (client_cert->Equals(client_certs[i])) {

[wtc, 2010-11-18 01:35:04]

rsleevi: just to clarify: if you agree to make the
scoped_refptr change that I suggested, I'd like to review
a new Patch Set before you check this in.  Thanks.

[Ryan Sleevi, 2010-11-18 06:32:52]

wtc: Thanks for the feedback. Patchset 3 contains the fixes for the issues you
brought up. I don't have strong feelings for or against scoped_refptr<>, so I
went ahead and implemented it as you requested. Please take another look.

agl, rch: The lack of unit tests for client auth in the HttpNetworkTransaction
is unfortunately a known issue at the moment. The general problem is on my
radar, and davidben has a TODO in that file for it. I took a look through the
classes, to see what it would take, and I think it'd be best served by a
separate CL for the tests, as it'd be non-trivial. Is the lack of test coverage
for this change, which is no worse than our existing lack of test coverage, a
blocker?

[wtc, 2010-11-18 18:55:54]

LGTM.  Thanks.

http://codereview.chromium.org/4568002/diff/11002/net/base/ssl_client_auth_ca...
File net/base/ssl_client_auth_cache_unittest.cc (right):

http://codereview.chromium.org/4568002/diff/11002/net/base/ssl_client_auth_ca...
net/base/ssl_client_auth_cache_unittest.cc:62:
EXPECT_FALSE(cache.Lookup(server1, &cached_cert));
Nit: for consistency, may want to add
  cached_cert = NULL;
before the cache.Lookup() calls here and on line 69.

[Ryan Hamilton, 2010-11-18 19:02:28]

LGTM.  It'd be great if we could add a test, but if it's not trivial,
let's not get into a yak shaving party...

On Wed, Nov 17, 2010 at 10:32 PM, <rsleevi@chromium.org> wrote:
>
> wtc: Thanks for the feedback. Patchset 3 contains the fixes for the issues you
> brought up. I don't have strong feelings for or against scoped_refptr<>, so I
> went ahead and implemented it as you requested. Please take another look.
>
> agl, rch: The lack of unit tests for client auth in the HttpNetworkTransaction
> is unfortunately a known issue at the moment. The general problem is on my
> radar, and davidben has a TODO in that file for it. I took a look through the
> classes, to see what it would take, and I think it'd be best served by a
> separate CL for the tests, as it'd be non-trivial. Is the lack of test
coverage
> for this change, which is no worse than our existing lack of test coverage, a
> blocker?
>
> http://codereview.chromium.org/4568002/