Implement the backend of SSL client authentication for...

Implement the backend of SSL client authentication for
Windows.

Create Schannel SSPI CredHandles with certificates for
SSL client authentication.

Remember the client certificates that the user selected
so that we don't ask the user again and again.

R=rvargas,eroman
BUG=http://crbug.com/318
TEST=none

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

[wtc, 2009-06-19 01:02:55]

eroman,rvargas: please review the entire CL.

rvargas: please focus on the ssl_client_socket_win.cc.

eroman: please focus on http_network_transaction.cc.  Do you think I should
change SSLClientAuthCache so that its methods take const GURL& instead of const
std::string& ?

[rvargas (doing something else), 2009-06-19 18:57:50]

LGTM

http://codereview.chromium.org/131086/diff/1003/1004
File net/base/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/131086/diff/1003/1004#newcode124
Line 124: // CredHandleMapKey is a std::pair consistig of these two components:
nit: consisting.

http://codereview.chromium.org/131086/diff/1003/1004#newcode346
Line 346: CERT_CHAIN_FIND_BY_ISSUER_PARA find_by_issuer_para;
nit: can we move this block (341 - 352) to after calling CerOpenSystemStore
(360)

http://codereview.chromium.org/131086/diff/1003/1004#newcode350
Line 350: find_by_issuer_para.dwKeySpec = 0;
nit: no need to set this to 0

http://codereview.chromium.org/131086/diff/1003/1004#newcode355
Line 355: HCERTSTORE my_cert_store = CertOpenSystemStore(0, L"MY");
nit: use NULL instead of 0

[eroman, 2009-06-19 21:42:24]

Sorry I didn't get to this earlier, was OOO this morning.

http://codereview.chromium.org/131086/diff/2002/3001
File net/base/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/131086/diff/2002/3001#newcode93
Line 93: if (dwLower || dwUpper) {
heh, been a while since i seen hungarian notation :)

http://codereview.chromium.org/131086/diff/2002/3001#newcode108
Line 108: DCHECK(0 < ssl_version_mask &&
nit: I would suggest writing this as two DCHECKs:

DCHECK_LT(0, ssl_version_mask);
DCHECK_LT(ssl_version_mask, arraysize(anonymous_creds_));

(makes the output a bit more useful if ever it does fail).

http://codereview.chromium.org/131086/diff/2002/3001#newcode129
Line 129: typedef std::map<CredHandleMapKey, CredHandleClass> CredHandleMap;
Hmm, I dunno about this...

Most of the STL containers rely on copy-constructors, since when you insert an
entry it may need to grow the backing store. For example when a std::vector is
at capacity and you insert an element, it re-allocates a contiguous range of
heap space, and copies all the old entries into their new spots.

This has two important consequences:
(1) The address of &vector[i] will be changed
(2) The copy constructor, and destructor, were called on all of the entries.

Now, I don't know how std::map is implemented, but I suspect it has the same
behavior. Doesn't seem like CredHandleClass is going to be happy with that
model:

CredHandleClass new;
{
  CredHandleClass orig = ...;
  new = orig;
}

Once |orig| goes out of scope, it will call FreeCredentialHandle(), which I
assume invalidates the data which has been copied into |new|.

Similarly, I notice in GetHandle() that it returns the address of a value in the
map. I haven't looked at the callers of this function yet, but this pointer is
short-lived so hopefully they don't hold on to following any mutations made to
client_cert_creds_ .....

http://codereview.chromium.org/131086/diff/2002/3002
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/131086/diff/2002/3002#newcode1293
Line 1293: if (memcmp(&client_cert->fingerprint(),
I suggesting moving this into operator==.

struct Fingerprint {
  unsigned char data[20];

  bool operator==(const Fingerprint& other) const {
    return memcmp(...) == 0;
  }
};

[eroman, 2009-06-19 22:00:35]

Also, are there any unit-tests for this?

[wtc, 2009-06-19 23:08:21]

On 2009/06/19 22:00:35, eroman wrote:
> Also, are there any unit-tests for this?

I will add unit tests next week or in July.

[wtc, 2009-06-19 23:11:53]

eroman: thanks for the review.  I will make the changes
you suggested except the one below.

http://codereview.chromium.org/131086/diff/2002/3001
File net/base/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/131086/diff/2002/3001#newcode108
Line 108: DCHECK(0 < ssl_version_mask &&
On 2009/06/19 21:42:24, eroman wrote:
> nit: I would suggest writing this as two DCHECKs:
> 
> DCHECK_LT(0, ssl_version_mask);
> DCHECK_LT(ssl_version_mask, arraysize(anonymous_creds_));

I didn't make this change because the second DCHECK_LT
would require a static_cast to make both arguments the
same type (int or signed int).  I don't think the clutter
of the resulting code is worth the trouble.