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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by rsleevi-old
Modified:
2 years, 11 months ago
Reviewers:
wtc, Ryan Sleevi
CC:
chromium-reviews_chromium.org, cbentzel+watch_chromium.org, darin-cc_chromium.org, David Benjamin
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.

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

Patch Set 1 #

Patch Set 2 : Whitespace for NSS, leakfix for client auth handler, intermediate cert for win/osx #

Total comments: 1

Patch Set 3 : Mac handling of smart card ejection and a few Mac typos I spotted reviewing #

Patch Set 4 : Enable ECDSA signatures, match SChannel's behaviour for signing, and omit root certs from the chain #

Patch Set 5 : License fix #

Total comments: 1

Patch Set 6 : Copy from 3455019 - Patchset 1 (rebase to trunk) #

Patch Set 7 : White space and don't prompt to insert smart card if ejected on Windows #

Total comments: 3

Patch Set 8 : Rebase to trunk #

Patch Set 9 : Copy from 3455019 - Patchset 2 #

Patch Set 10 : Merge in Patchset 7 - Switch to CRYPT_SILENT #

Patch Set 11 : Fix Mac compiles post-copy #

Total comments: 3

Patch Set 12 : Copy from 3455019 - Patchset 4 #

Patch Set 13 : Add a short-circuit when the CSP reports the container is not removable #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1018 lines, -39 lines) Lint Patch
M net/socket/client_socket_factory.cc View 2 chunks +0 lines, -7 lines 0 comments 0 errors Download
M net/socket/ssl_client_socket_nss.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +8 lines, -0 lines 0 comments ? errors Download
M net/socket/ssl_client_socket_nss.cc View 1 2 3 4 5 6 7 8 9 10 11 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 5 6 7 8 9 10 11 2 chunks +0 lines, -12 lines 0 comments 0 errors Download
M net/third_party/nss/ssl.gyp View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +14 lines, -0 lines 0 comments ? errors Download
M net/third_party/nss/ssl/ssl.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +29 lines, -0 lines 0 comments 0 errors Download
M net/third_party/nss/ssl/ssl3con.c View 1 2 3 4 5 6 7 8 9 10 11 13 chunks +103 lines, -0 lines 0 comments 7 errors Download
M net/third_party/nss/ssl/ssl3ext.c View 1 chunk +1 line, -1 line 0 comments 1 errors Download
M net/third_party/nss/ssl/sslauth.c View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +22 lines, -0 lines 0 comments 5 errors Download
M net/third_party/nss/ssl/sslimpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +81 lines, -0 lines 0 comments 5 errors Download
M net/third_party/nss/ssl/sslnonce.c View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments 1 errors Download
A net/third_party/nss/ssl/sslplatf.c View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +570 lines, -0 lines 0 comments 56 errors Download
M net/third_party/nss/ssl/sslsnce.c View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments 2 errors Download
M net/third_party/nss/ssl/sslsock.c View 1 2 3 4 5 6 7 8 3 chunks +14 lines, -0 lines 0 comments 4 errors Download
Trybot results:
Commit:

Messages

Total messages: 16
rsleevi-old
Two other questions I didn't mention in the description: Preferred license for sslplatf.c Style for ...
3 years, 10 months ago #1
wtc
Ryan: thanks a lot for this CL! I will review this CL before your other ...
3 years, 10 months ago #2
rsleevi-old
Now that I've got your attention, I can go forward with the soul-crushing technical discussion ...
3 years, 10 months ago #3
rsleevi-old
This patchset adds support for sending the certificate chain as returned by the OS. This ...
3 years, 10 months ago #4
rsleevi-old
Patchset 3 should contain the last of the must-implement TODOs, by adding detection of the ...
3 years, 10 months ago #5
Ryan Sleevi
Wan-Teh, Have you had a chance to look at this? Seeing that you moved native ...
3 years, 7 months ago #6
wtc
Ryan, This fell through the cracks. I'm very sorry. Too busy! I am very interested ...
3 years, 7 months ago #7
wtc
rsleevi: I have applied your patch to a current source tree. I will upload an ...
3 years, 7 months ago #8
Ryan Sleevi
On 2010/09/23 23:14:26, wtc wrote: > rsleevi: I have applied your patch to a current ...
3 years, 7 months ago #9
wtc
rsleevi: "gcl update" doesn't allow me to upload a new snapshot of a CL owned ...
3 years, 7 months ago #10
Ryan Sleevi
Hopefully I'm doing this right. I used git cl patch to grab your patch and ...
3 years, 7 months ago #11
wtc
rsleevi: I'm getting ready to check in this patch (using my copy of the CL ...
3 years, 5 months ago #12
rsleevi-old
http://codereview.chromium.org/2828002/diff/95014/119010 File net/third_party/nss/ssl/sslplatf.c (right): http://codereview.chromium.org/2828002/diff/95014/119010#newcode144 net/third_party/nss/ssl/sslplatf.c:144: info->provType, CRYPT_SILENT)) On 2010/11/03 23:30:06, wtc wrote: > I ...
3 years, 5 months ago #13
Ryan Sleevi
@wtc: I was able to reproduce using medium/high security with a locally imported key. Unfortunately, ...
3 years, 5 months ago #14
wtc
rsleevi: thanks for the detailed reply. CAPI CSPs behave very differently from PKCS #11 libraries ...
3 years, 5 months ago #15
wtc
3 years, 5 months ago #16
On 2010/11/04 02:05:24, rsleevi wrote:
>
> The alternative, which I believe you've explored, is to remove the checks
> altogether as to whether the card is still present. The *existing* SSL/TLS
> session will remain valid, but no new sessions should be possible. From a FIPS
> standpoint, this may not be acceptable, but it does simplify the
implementation.

I didn't look into what ssl_PlatformAuthTokenPresent is
used for, so I didn't consider this alternative.

I just studied how NSS uses ssl3_ClientAuthTokenPresent,
and found that it actually tests more than whether the client
auth token is present; it also tests whether the token was
ever removed since it was used for client auth:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3...

So the current ssl_PlatformAuthTokenPresent implementation
does not fully meet the requirement.

CVS history reminded me why ssl3_ClientAuthTokenPresent was added:
https://bugzilla.mozilla.org/show_bug.cgi?id=167756

It had nothing to do with FIPS.  It was added to implement
a useful behavior, where a client-authenticated SSL connection
will be broken if NSS detects that the token has been removed.

If we don't care about this behavior, we can define
ssl_PlatformAuthTokenPresent to simply return PR_TRUE.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6