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

Issue 843005: Adds support for the <keygen> element to Windows, matching support present on... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 1 month ago by rsleevi-old
Modified:
2 years, 11 months ago
Reviewers:
wtc, Jens Alfke
CC:
chromium-reviews_chromium.org, pam+watch_chromium.org, John Grabowski, darin-cc_chromium.org, PaweĊ‚ Hajdan Jr.
Visibility:
Public.

Description

[Replaced by http://codereview.chromium.org/1591006 ]

Adds support for the <keygen> element to Windows, matching support present on OS X and Linux.

Contributed by ryan.sleevi@gmail.com

BUG=148
TEST=KeygenHandler.SmokeTest

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : Allow generated keys/certificates to be used for SChannel servers, not just clients #

Total comments: 31

Patch Set 6 : Fix style nits #

Total comments: 23

Patch Set 7 : '' #

Total comments: 57

Patch Set 8 : Addressing #

Patch Set 9 : Fixing remaining issues #

Unified diffs Side-by-side diffs Delta from patch set Stats (+616 lines, -33 lines) Lint Patch
M AUTHORS View 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M net/base/cert_database.h View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -2 lines 0 comments 0 errors Download
M net/base/cert_database_mac.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -4 lines 0 comments 0 errors Download
M net/base/cert_database_nss.cc View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -6 lines 0 comments 0 errors Download
M net/base/cert_database_win.cc View 1 2 3 4 5 6 7 8 1 chunk +100 lines, -8 lines 0 comments 0 errors Download
M net/base/keygen_handler.h View 1 2 3 4 5 6 7 8 2 chunks +51 lines, -0 lines 0 comments 0 errors Download
A net/base/keygen_handler.cc View 1 2 3 4 5 6 7 1 chunk +36 lines, -0 lines 0 comments 1 errors Download
M net/base/keygen_handler_mac.cc View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments 0 errors Download
M net/base/keygen_handler_nss.cc View 1 2 3 4 5 6 7 8 2 chunks +18 lines, -5 lines 0 comments 1 errors Download
M net/base/keygen_handler_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +42 lines, -0 lines 0 comments 0 errors Download
M net/base/keygen_handler_win.cc View 1 2 3 4 5 6 7 8 1 chunk +353 lines, -3 lines 0 comments 0 errors Download
M net/base/net_error_list.h View 6 7 8 1 chunk +1 line, -1 line 0 comments 0 errors Download
M net/net.gyp View 1 2 3 4 5 6 7 8 2 chunks +1 line, -4 lines 0 comments 0 errors Download
Trybot results:
Commit:

Messages

Total messages: 19
rsleevi-old
In line with Jens' work on the Mac side, I was trying to bring the ...
4 years, 1 month ago #1
wtc
Ryan: Thanks a lot for your patch. Your email request for code review looks like ...
4 years, 1 month ago #2
rsleevi-old
Wan-Teh, While I absolutely agree that there should be some process to inform the user ...
4 years, 1 month ago #3
rsleevi-old
I suppose I should have added Option 4), which I didn't think was acceptable, but ...
4 years, 1 month ago #4
rsleevi-old
As a further clarification to your request for prompting prior to key generation: Do you ...
4 years, 1 month ago #5
Jens Alfke
On 2010/03/15 18:11:42, wtc wrote: > Re: prompting the user: we should prompt the user ...
4 years, 1 month ago #6
rsleevi-old
> Hm. I tend to disagree. I can't think of any security implications of sending ...
4 years, 1 month ago #7
rsleevi-old
Jens or Wan-Teh, if I have correctly followed http://dev.chromium.org/developers/contributing-code#TOC-If-you-are-not-a-committer-yet , and assuming the prompting issue ...
4 years, 1 month ago #8
rsleevi-old
The most recent modification is to ensure that the key itself is generated in a ...
4 years, 1 month ago #9
wtc
This is great. Thanks! I still need to review keygen_handler_win.cc again. I also decided to ...
4 years, 1 month ago #10
rsleevi-old
http://codereview.chromium.org/843005/diff/45001/46005 File net/base/cert_database_win.cc (right): http://codereview.chromium.org/843005/diff/45001/46005#newcode20 net/base/cert_database_win.cc:20: // Returns an encoded version SubjectPublicKeyInfo from |cert| that ...
4 years, 1 month ago #11
wtc
Ryan, Here are some more comments. I still haven't reviewed the new Windows code, but ...
4 years, 1 month ago #12
wtc
I forgot to point out my most important comment in the previous review: should KeyCache ...
4 years, 1 month ago #13
rsleevi-old
Wan-Teh, I hope I've addressed your feedback in the proper manner - I wasn't sure ...
4 years ago #14
wtc
Ryan, I finished the review. It's a long CL! Thank you very much for contributing ...
4 years ago #15
wtc
Ryan, Here is my response to your last comments. http://codereview.chromium.org/843005/diff/60001/61006 File net/base/keygen_handler.cc (right): http://codereview.chromium.org/843005/diff/60001/61006#newcode27 net/base/keygen_handler.cc:27: ...
4 years ago #16
rsleevi-old
Still a few outstanding questions for you, Wan-Teh, but hopefully this will be finished soon. ...
4 years ago #17
wtc
Ryan, Here is my response to your last reply. Let's try to wrap this up ...
4 years ago #18
rsleevi-old
4 years ago #19
Hopefully final draft

http://codereview.chromium.org/843005/diff/73001/74005
File net/base/cert_database_win.cc (right):

http://codereview.chromium.org/843005/diff/73001/74005#newcode69
net/base/cert_database_win.cc:69: prov_info.dwKeySpec = AT_KEYEXCHANGE;
I didn't think there were any specific requirements for <keygen>, other than it
generates a public/private keypair. Naturally, this keypair /must/ be capable of
signing (SPKAC), but beyond that, the HTML5 just specifies that it /might/ be
used for client certificates, but divests itself of stating the exact
purposes/uses of the key.

http://codereview.chromium.org/843005/diff/73001/74005#newcode91
net/base/cert_database_win.cc:91: DWORD acquire_flags = CRYPT_ACQUIRE_CACHE_FLAG
| CRYPT_ACQUIRE_SILENT_FLAG;
On 2010/03/31 18:04:28, wtc wrote:
> When I worked on a smart card CSP before, I remember my CSP
> code did not run inside the Internet Explorer process.  It
> seems that CryptoAPI ran my CSP code in some daemon/service
> process.  So CSP's GUI may not interfere with Chrome's
> message loops.
> 
> As a first cut, it is fine for the CSP to block the IO thread.
> We already have this issue when performing SSL client auth
> (the CSP may ask the user to enter the PIN/password).  I
> remember I filed a bug about this issue, but I can't find
> it.

Depends. If you were using the Smart Card minidriver, you'd be hosted by the
SCardSvr instance which interfaces the Smart Card CSP with the resource handler.

However, for CSPs in general, they're supposed to use PP_CLIENT_HWND as the
parent window, which the client passes through
http://msdn.microsoft.com/en-us/library/aa380276%28VS.85%29.aspx

For the SSL client auth, I didn't think it blocked the IO thread, as the window
is created/destroyed/blocks only the UI thread
(ssl_client_auth_handler_win.cc:21-23)

Wouldn't it be safer to err on the side of caution, and disable the CSP UI
(which doesn't affect the default MSFT CSPs), causing them to gracefully and
silently error if they need to show a UI, then to deal w/ the possible
side-effects?

http://codereview.chromium.org/843005/diff/73001/74005#newcode93
net/base/cert_database_win.cc:93: // This call will never succeed if the cert
has been built from a byte stream,
I've removed this call.

There were several things being accomplished by this call, but on closer
evaluation and in the name of parity with the other implementations, I removed.

Goal 1) Don't replace the private key of a certificate if it already has one. I
believe this is somewhat true on OS X, as the Keychain stores private keys
uniquely indexed by their SPKI, but I haven't dug into the SecKeychain sources
again to see if the unique index is per-DL/DB handle or across all DL-DB
handles/Keychains. I don't see the equivalent in NSS.

However, this is not the appropriate way to check - since it'd report "no key"
if a key was associated, but lived on an external device not presently
connected.

Goal 2) As a side effect, it would fixup the CERT_KEY_PROV_INFO_PROP_ID if it
was missing and CERT_KEY_CONTEXT_PROP_ID contained a valid key. The only benefit
to doing this would be if Goal 1 was implemented correctly, checking
CERT_KEY_PROV_INFO_PROP_ID could be retrieved and contained a location. Since
PROV_INFO isn't guaranteed to be set, even if CONTEXT is, this call would
synchronize the two and could be done prior to the check for Goal #1.

However, since Goal #1 isn't necessary (at present), I dropped this segment.

http://codereview.chromium.org/843005/diff/73001/74005#newcode134
net/base/cert_database_win.cc:134: HCERTSTORE cert_db =
CertOpenStore(CERT_STORE_PROV_SYSTEM,
On 2010/03/31 18:04:28, wtc wrote:
> Have you tried CertOpenSystemStore(NULL, L"MY")?
> Perhaps it also opens the "MY" system store with write access.
> Otherwise the CertOpenSystemStore MSDN page would warn about
> the read-only access.

I wouldn't count on MSDN being explicit - I've lost count of how many CryptoAPI
documentation bugs I've filed over the years.

It opens it with the default flags (0), at least when tracing on Vista x64 with
http://blogs.msdn.com/alejacma/archive/2007/10/31/cryptoapi-tracer.aspx

Double checking the MSDN references, however, and this call is unnecessary, as
CERT_STORE_READONLY_FLAG is *not* being implicitly set (at least, not that I
could observe during tracing), so it will open with write, and fail if write
access can't be obtained. MAXIMUM_ALLOWED would allow it to gracefully fall back
to read only, but that would just delay the error until 140, so I just replaced
it.

http://codereview.chromium.org/843005/diff/73001/74009
File net/base/keygen_handler_nss.cc (right):

http://codereview.chromium.org/843005/diff/73001/74009#newcode217
net/base/keygen_handler_nss.cc:217: Cache* cache = KeyCache::GetInstance();
I've moved the block up for both NSS and Win, so that they always store the
location, even if they don't store the key.

Combined with the modification to the Win32 code to no longer attempt acquiring
the key before setting it, this should permit unit tests where a key is
generated via <keygen> with stores_key_ as false, and then later import a
certificate for that key, which is what I understand you were wanting.
Sign in to reply to this message.

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