Mac: Ignoring optional client-cert requests from server

Mac: Ignoring optional client-cert requests from server
BUG=37765
TEST=none

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

[Jens Alfke, 2010-03-09 22:56:38]

I'd forgotten to make sure the user gets prompted if the server requests certs
during initial handshake but socket hasn't been given any yet. Also, I made the
code not break if it gets the cert-request during the first call to
SSLHandshake. And I added some logging, with an easy way to boost its priority
during debugging.

[wtc, 2010-03-10 20:33:23]

This seems correct, but I'm worried about the
continuation after client_cert_requested_ in
DoVerifyCertComplete.  I'd like to read the code again.
Let me send my comments to you first.

http://codereview.chromium.org/746002/diff/2001/3001
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/746002/diff/2001/3001#newcode102
net/socket/ssl_client_socket_mac.cc:102: #define SSLOG LOG(INFO) << "SSL: "
Nit: should this be SSL_LOG?  (There should be two L's.)

http://codereview.chromium.org/746002/diff/2001/3001#newcode659
net/socket/ssl_client_socket_mac.cc:659: CFArrayRef allowedIssuerNames = NULL;
Nit: rename this allowed_issuer_names to follow our naming
convention.

http://codereview.chromium.org/746002/diff/2001/3001#newcode939
net/socket/ssl_client_socket_mac.cc:939: // TODO(hawk): we verify the
certificate chain even on resumed sessions
Nit: Move this TODO comment back to its original location
because it applies to the 'noErr' case, too.  (It applies
when we set next_handshake_state_ to STATE_VERIFY_CERT.)

http://codereview.chromium.org/746002/diff/2001/3001#newcode955
net/socket/ssl_client_socket_mac.cc:955: client_cert_requested_ = true;
I think we also need to set handshake_interrupted_ to true,
so that in DoVerifyCertComplete we will go on to the
STATE_HANDSHAKE_FINISH state.

http://codereview.chromium.org/746002/diff/2001/3001#newcode998
net/socket/ssl_client_socket_mac.cc:998: // Server asked for client cert before
we'd had a chance to verify its own.
This comment is confusing.  How about:
  We verified the server cert early because the server asked
  for client cert.

http://codereview.chromium.org/746002/diff/2001/3001#newcode1105
net/socket/ssl_client_socket_mac.cc:1105: SSLOG << "Server renegotiating for
client cert; restarting...";
The "for client cert" part of this log message is wrong
for the errSSLServerAuthCompleted case.

The "Server renegotiating" part is correct for both cases.

[Jens Alfke, 2010-03-11 16:03:10]

I've made the changes you requested. Please see my note below about
handshake_interrupted_. Thanks!

http://codereview.chromium.org/746002/diff/2001/3001
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/746002/diff/2001/3001#newcode955
net/socket/ssl_client_socket_mac.cc:955: client_cert_requested_ = true;
On 2010/03/10 20:33:23, wtc wrote:
> I think we also need to set handshake_interrupted_ to true,
> so that in DoVerifyCertComplete we will go on to the
> STATE_HANDSHAKE_FINISH state.

I can't figure out if this is necessary or not, but you understand the state
machine better than I do, so I've made the change.

In testing it, I found that I can't get this code path to be reached — what
happens instead is that on the first connection attempt send_client_cert is not
yet set, so the method returns ERR_SSL_CLIENT_AUTH_CERT_NEEDED on line 951
above. Then on subsequent connections after the user's picked a cert, the cert
is stored in the session before the connection starts (in the Initialize method)
instead of enabling the break-on-cert-request mode.

[wtc, 2010-03-11 22:31:16]

Hi Jens,

Thanks for the new patch set.  Now that I understand the
code more, I think it still needs work (hopefully simplified).
Please see my comments below.

http://codereview.chromium.org/746002/diff/7001/8001
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/746002/diff/7001/8001#newcode556
net/socket/ssl_client_socket_mac.cc:556: SSL_LOG << "----- DISCONNECTED";
Nit: should we put this SSL_LOG statement outside the
"if" block, so that it's always executed whenever Disconnect()
is called?

http://codereview.chromium.org/746002/diff/7001/8001#newcode663
net/socket/ssl_client_socket_mac.cc:663: " allowed issuer names";
Nit: format this SSL_LOG statment like this:

    SSL_LOG << "Server has " << CFArrayGetCount(allowed_issuer_names)
            << " allowed issuer names";

See http://www.chromium.org/developers/coding-style#Code_Formatting
(This issue is not discussed in the public Google C++ Style
Guide because the authors consider "base/logging.h" to be
Chromium-specific.)

This suggested change also applies to the other multi-line
SSL_LOG statements in this file.

http://codereview.chromium.org/746002/diff/7001/8001#newcode756
net/socket/ssl_client_socket_mac.cc:756: if (ssl_set_session_options &&
!ssl_config_.send_client_cert) {
I think we can remove the !ssl_config_.send_client_cert
test now, because you added code to verify the server cert
before sending the client cert.

Note: removing the !ssl_config_.send_client_cert test
will exercise different code paths, so please test carefully.

http://codereview.chromium.org/746002/diff/7001/8001#newcode957
net/socket/ssl_client_socket_mac.cc:957: next_handshake_state_ =
STATE_VERIFY_CERT;
Since we enable both break-on-server-auth and break-on-cert-requested,
if we get errSSLClientCertRequested. we must have gotten
errSSLServerAuthCompleted earlier, and therefore must have
verified the server cert, so we should not go to
STATE_VERIFY_CERT again.  Otherwise we'll verify the cert
twice.

Actually, I believe we won't get errSSLClientCertRequested
here.  Instead, we should only get errSSLClientCertRequested
in DoHandshakeFinish().  So this case should be dead code
and can be replaced with a NOTREACHED() or even CHECK(0).

This should also mean client_cert_requested_ can be removed.

http://codereview.chromium.org/746002/diff/7001/8001#newcode1111
net/socket/ssl_client_socket_mac.cc:1111: LOG(INFO) << "Server renegotiating;
assuming it wants a client cert...";
Do we need both SSL_LOG and LOG(INFO) of the same message?

[Jens, 2010-03-12 02:00:03]

http://codereview.chromium.org/746002/diff/7001/8001
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/746002/diff/7001/8001#newcode556
net/socket/ssl_client_socket_mac.cc:556: SSL_LOG << "----- DISCONNECTED";
On 2010/03/11 22:31:16, wtc wrote:
> Nit: should we put this SSL_LOG statement outside the
> "if" block, so that it's always executed whenever Disconnect()
> is called?

I had it that way at first, but then it often gets logged twice in a row,
because the call to Disconnect in the destructor is often redundant. This way
it'll only get called once.

http://codereview.chromium.org/746002/diff/7001/8001#newcode756
net/socket/ssl_client_socket_mac.cc:756: if (ssl_set_session_options &&
!ssl_config_.send_client_cert) {
On 2010/03/11 22:31:16, wtc wrote:
> I think we can remove the !ssl_config_.send_client_cert
> test now, because you added code to verify the server cert
> before sending the client cert.

I don't think so. I put that condition in to work around the renegotiation
problem with SecureTransport. This way we never have to renegotiate: if we don't
have a cert, then when the server tries to renegotiate we have to close the
session anyway to ask the user. And if we do have a cert, we provide it up front
instead of enabling break-on-auth, because break-on-auth would screw up upon
renegotiation.

http://codereview.chromium.org/746002/diff/7001/8001#newcode957
net/socket/ssl_client_socket_mac.cc:957: next_handshake_state_ =
STATE_VERIFY_CERT;
On 2010/03/11 22:31:16, wtc wrote:
> Actually, I believe we won't get errSSLClientCertRequested
> here.  Instead, we should only get errSSLClientCertRequested
> in DoHandshakeFinish().  So this case should be dead code
> and can be replaced with a NOTREACHED() or even CHECK(0).

Nope -- it really happens, when logging into foaf.me, that's why I added this
code! Otherwise it was aborting the connection because it got treated as an
unrecognized error.

[wtc, 2010-03-13 01:23:38]

Sorry about the delay in replying.

I think we need to set breakpoints on all of the
errSSLServerAuthCompleted and errSSLClientCertRequested
cases in this file and see which ones get hit.  I believe
the errSSLClientCertRequested case in DoHandshakeStart()
is unreachable.

In any case, I believe that the code in the two
errSSLClientCertRequested cases after the
"return ERR_SSL_CLIENT_AUTH_CERT_NEEDED" statements
is unreachable.  So please at least delete that.

http://codereview.chromium.org/746002/diff/7001/8001
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/746002/diff/7001/8001#newcode556
net/socket/ssl_client_socket_mac.cc:556: SSL_LOG << "----- DISCONNECTED";
On 2010/03/12 02:00:03, Jens wrote:
>
> I had it that way at first, but then it often gets logged twice in a row,
> because the call to Disconnect in the destructor is often redundant. This way
> it'll only get called once.

I see.  Perhaps this log message should say "DISPOSED"
or "Dispose SSLContext".

http://codereview.chromium.org/746002/diff/7001/8001#newcode957
net/socket/ssl_client_socket_mac.cc:957: next_handshake_state_ =
STATE_VERIFY_CERT;
This is strange.  I just read the Secure Transport source
code of Mac OS X 10.5.8.  When the server requests client
auth, we should get both errSSLServerAuthCompleted and
errSSLClientCertRequested.  So I don't understand why we
get errSSLClientCertRequested here in DoHandshakeStart()
rather than in DoHandshakeFinish().  Perhaps I'm missing
something.

If we get errSSLClientCertRequested here, do we get
errSSLServerAuthCompleted first?  If so, we still need
to avoid going to the STATE_VERIFY_CERT state twice.

Note: with this patch, when we get errSSLClientCertRequested here,
we will return ERR_SSL_CLIENT_AUTH_CERT_NEEDED on line
954.  So lines 955-960 are unreachable.

http://codereview.chromium.org/746002/diff/7001/8001#newcode1048
net/socket/ssl_client_socket_mac.cc:1048: status = SetClientCert();
Same here: with this CL, lines 1048-1049 are unreachable
because we must have returned ERR_SSL_CLIENT_AUTH_CERT_NEEDED
on line 1047.

Also note that we should set 'status' to noErr before
the 'break' statement, otherwise errSSLClientCertRequested
will be mapped to ERR_FAILED in the
NetErrorFromOSStatus(status) call on line 1079 below.

[Jens Alfke, 2010-03-15 19:04:05]

http://codereview.chromium.org/746002/diff/7001/8001
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/746002/diff/7001/8001#newcode957
net/socket/ssl_client_socket_mac.cc:957: next_handshake_state_ =
STATE_VERIFY_CERT;
On 2010/03/13 01:23:38, wtc wrote:
> If we get errSSLClientCertRequested here, do we get
> errSSLServerAuthCompleted first?

No, not that I've seen. Here's the flow when logging into foaf.me, with some
additional log calls I just added. If there had been an
errSSLServerAuthCompleted there would have been a line in the log for it.

ssl_client_socket_mac.cc(682)] SSL: ----- InitializeSSLContext
ssl_client_socket_mac.cc(760)] SSL: Setting
kSSLSessionOptionBreakOnCertRequested
ssl_client_socket_mac.cc(952)] SSL: Received client cert request in
DoHandshakeStart
ssl_client_socket_mac.cc(671)] SSL: Asking user to choose between 8 client
certs...
ssl_client_socket_mac.cc(555)] SSL: ----- Disposed SSLContext
ssl_client_socket_mac.cc(682)] SSL: ----- InitializeSSLContext
ssl_client_socket_mac.cc(790)] SSL: Setting client cert in advance because
send_client_cert is set
ssl_client_socket_mac.cc(1022)] SSL: SSLSetCertificate(1 certs)
ssl_client_socket_mac.cc(979)] SSL: DoVerifyCert...
ssl_client_socket_mac.cc(995)] SSL: ...DoVerifyCertComplete (result=0)
ssl_client_socket_mac.cc(1008)] SSL: Handshake finished! (DoVerifyCertComplete)
ssl_client_socket_mac.cc(555)] SSL: ----- Disposed SSLContext

[Jens Alfke, 2010-03-15 19:04:57]

Oh, and BTW, I made the changes you suggested (removing unreachable code.)
Please review.

[wtc, 2010-03-16 00:42:48]

LGTM.  Thanks a lot for the event trace.  I have some
comments.

http://codereview.chromium.org/746002/diff/16001/17001
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/746002/diff/16001/17001#newcode662
net/socket/ssl_client_socket_mac.cc:662: << " allowed issuer names";
Nit: please align "<<" with the first "<<" on the previous
line.  Please make the same change to lines 672 and 1057
below.

http://codereview.chromium.org/746002/diff/16001/17001#newcode951
net/socket/ssl_client_socket_mac.cc:951: case errSSLClientCertRequested:
IMPORTANT:

1. Please document that if we get errSSLClientCertRequested,
we won't get errSSLServerAuthCompleted.  This is not obvious
at all.

2. This means we still end up sending the client certificate
before verifying the server certificate.  Can you verify
this?

The only solution I have is:
- Enable breakOnServerAuth and breakOnCertRequested for
  the initial handshake, but disable them after the initial
  handshake is done, so that renegotiation, if any, won't
  be interrupted.
- During initial handshake, when we get
  errSSLClientCertRequested, verify the server certificate
  first.

If you want to try this solution, please check in this CL
first, so that this code review won't drag on for too long.

[Jens Alfke, 2010-03-16 18:50:10]

Checked in, with a warning comment added and nits fixed. Thanks!

http://codereview.chromium.org/746002/diff/16001/17001
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/746002/diff/16001/17001#newcode951
net/socket/ssl_client_socket_mac.cc:951: case errSSLClientCertRequested:
On 2010/03/16 00:42:48, wtc wrote:
> 2. This means we still end up sending the client certificate
> before verifying the server certificate.

No, because we always abort the connection in this case (by returning
ERR_SSL_CLIENT_AUTH_CERT_NEEDED.) The unreachable code you had me delete in the
previous round of review took care of the case where we would proceed with this
connection; it deferred sending the client cert till the verification succeeded.
But as things are now we won't ever proceed.