Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 131086: Implement the backend of SSL client authentication for... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 9 months ago by wtc
Modified:
3 years, 10 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Ready for review #

Total comments: 4

Patch Set 3 : Upload before checkin #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+220 lines, -34 lines) Patch
M net/base/ssl_cert_request_info.h View 1 1 chunk +7 lines, -0 lines 0 comments Download
M net/base/ssl_client_socket_win.cc View 1 2 7 chunks +164 lines, -30 lines 4 comments Download
M net/http/http_network_transaction.h View 1 chunk +1 line, -1 line 0 comments Download
M net/http/http_network_transaction.cc View 1 2 5 chunks +48 lines, -3 lines 1 comment Download
Trybot results:
Commit: CQ not working?

Messages

Total messages: 6 (0 generated)
wtc
eroman,rvargas: please review the entire CL. rvargas: please focus on the ssl_client_socket_win.cc. eroman: please focus ...
5 years, 9 months ago (2009-06-19 01:02:55 UTC) #1
rvargas (slow to respond)
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 ...
5 years, 9 months ago (2009-06-19 18:57:50 UTC) #2
eroman
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): ...
5 years, 9 months ago (2009-06-19 21:42:24 UTC) #3
eroman
Also, are there any unit-tests for this?
5 years, 9 months ago (2009-06-19 22:00:35 UTC) #4
wtc
On 2009/06/19 22:00:35, eroman wrote: > Also, are there any unit-tests for this? I will ...
5 years, 9 months ago (2009-06-19 23:08:21 UTC) #5
wtc
5 years, 9 months ago (2009-06-19 23:11:53 UTC) #6
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cf4c24d