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

Issue 3455019: Support for using OS-native certificates for SSL client auth.... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 6 months ago by wtc
Modified:
2 years, 11 months ago
Reviewers:
Ryan Sleevi
CC:
chromium-reviews_chromium.org, cbentzel+watch_chromium.org, darin-cc_chromium.org
Visibility:
Public.

Description

Support for using OS-native certificates for SSL client
auth.

Known Limitations:
- Only SSL3/TLS1.0 handshakes are supported. It's unlikely
SSLv2 will/should ever be implemented. NSS does not yet
support TLS1.1/1.2.
- On Windows, only CryptoAPI keys are supported. Keys that
can only be accessed via CNG will fail.

Technical Notes:
Windows:
- Only the AT_KEYEXCHANGE key is used, per
http://msdn.microsoft.com/en-us/library/aa387461(VS.85).aspx
- CryptSetHashParam is used to directly set the hash value.
This *should* be supported by all CSPs that are compatible
with RSA/SChannel, AFAICT, but testing is needed.

NSS:
- The define NSS_PLATFORM_CLIENT_AUTH is used to guard all
of the new/patched code. The primary implementation
details are in sslplatf.c.

Patch author: Ryan Sleevi <rsleevi@chromium.org>;
Original review URL: http://codereview.chromium.org/2828002

BUG=148, 37560, 45369
TEST=Attempt to authenticate with a site that requires SSL
client authentication (e.g., https://foaf.me/simpleLogin.php
with a FOAF+SSL client certificate).

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

Patch Set 1 #

Total comments: 11

Patch Set 2 : Reviewed the whole CL for the first time #

Patch Set 3 : Sync'ed to current trunk. Attempt to fix Mac compilation errors. #

Patch Set 4 : Make it work on Mac OS X #

Patch Set 5 : Upload before checkin #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1008 lines, -39 lines) Lint Patch
M net/socket/client_socket_factory.cc View 4 2 chunks +0 lines, -7 lines 0 comments 0 errors Download
M net/socket/ssl_client_socket_nss.h View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments 0 errors Download
M net/socket/ssl_client_socket_nss.cc View 1 2 3 4 9 chunks +166 lines, -19 lines 0 comments 0 errors Download
M net/socket/ssl_client_socket_nss_factory.cc View 1 2 3 4 2 chunks +0 lines, -12 lines 0 comments 0 errors Download
M net/third_party/nss/ssl.gyp View 1 2 3 4 4 chunks +14 lines, -0 lines 0 comments 0 errors Download
M net/third_party/nss/ssl/ssl.h View 1 2 3 4 1 chunk +29 lines, -0 lines 0 comments 0 errors Download
M net/third_party/nss/ssl/ssl3con.c View 1 2 3 4 13 chunks +103 lines, -0 lines 0 comments 7 errors Download
M net/third_party/nss/ssl/ssl3ext.c View 4 1 chunk +1 line, -1 line 0 comments 1 errors Download
M net/third_party/nss/ssl/sslauth.c View 1 2 3 4 1 chunk +22 lines, -0 lines 0 comments 5 errors Download
M net/third_party/nss/ssl/sslimpl.h View 1 2 3 4 7 chunks +80 lines, -0 lines 0 comments 5 errors Download
M net/third_party/nss/ssl/sslnonce.c View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments 1 errors Download
A net/third_party/nss/ssl/sslplatf.c View 1 2 3 4 1 chunk +561 lines, -0 lines 0 comments 55 errors Download
M net/third_party/nss/ssl/sslsnce.c View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments 2 errors Download
M net/third_party/nss/ssl/sslsock.c View 1 2 3 4 3 chunks +14 lines, -0 lines 0 comments 4 errors Download
Trybot results:
Commit:

Messages

Total messages: 5
wtc
rsleevi: this is essentially your Patch Set 5, updated to the current trunk. I made ...
3 years, 6 months ago #1
wtc
Status update: I have reviewed everything except sslplatf.c. I should be able to finish the ...
3 years, 6 months ago #2
Ryan Sleevi
Thanks for the status update. I've addressed your comments below and look forward to seeing ...
3 years, 6 months ago #3
wtc
rsleevi: OK, I made one pass through the CL. Please review the differences between Patch ...
3 years, 6 months ago #4
Ryan Sleevi
3 years, 6 months ago #5
I'll keep future comments over there, but I'll address the last notes here for
thread-readability.

Primary reference for the CSSM/CDSA functions:
http://www.opensource.apple.com/source/Security/Security-37594/doc/

Secondary/cross-reference:
http://www.opengroup.org/onlinepubs/009609799/index.htm

You can also find a PDF on the spec buried there somewhere in the site. I have
it saved locally, but it was a free registration to download.

The Apple stuff I'm constantly having to pull out of archives, as Apple revamped
developer.apple.com to focus on IOS, and much of the older documentation is now
404'ing. MSDN has the same problem now and then, but with Apple it's been really
bad with the Security functions. Good luck trying to find a copy of the 10.5 +
10.6 API deprecations and additions to Security.framework (archive.org doesn't
help there, so some of it's from memory)

Also, as with much of Apple APIs, a good portion of additional documentation is
stuck within the public headers themselves. I still haven't decided where I
appreciate it or hate it (as it never appears anywhere else) :)

For the actual comments on the issue, I'm working on merging Patch Set 7 (which
were just a few one-line changes to the UI bits), but I think you'll find a
better explanation about the UI concerns / API concern addressed in the message
that went out with that patchset.

http://codereview.chromium.org/3455019/diff/1/11
File net/third_party/nss/ssl/sslplatf.c (right):

http://codereview.chromium.org/3455019/diff/1/11#newcode453
net/third_party/nss/ssl/sslplatf.c:453: // TODO(rsleevi): Should it be
kSecCredentialTypeNoUI? In Win32, at least,
On 2010/10/16 00:29:04, wtc wrote:
> We want UI here.  Did I miss something?

The main concern here was just GUI blocking (all of) the IO thread. The main
concern is that something like http://crbug.com/16979 is going to resurface -
unless a series of asynchronous functions are also added to NSS to indicate
"Hey, get the key/privileges/prompt". I thought I remembered davidben doing some
work on the PKCS#11-prompting side of this problem.

http://codereview.chromium.org/3455019/diff/1/11#newcode509
net/third_party/nss/ssl/sslplatf.c:509: if (cssmSignature)
On 2010/10/16 00:29:04, wtc wrote:
> BUG: should we also delete cspHandle, cssmKey, and cssmCreds here?

The documentation for SecKeyGetCSPHandle, SecKeyGetCSSMHandle, and
SecKeyGetCredentials all state that the output remains valid until the key
reference is released and shouldn't be modified.

So not a bug :)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1275:d14800f88434