Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
843005:
Adds support for the <keygen> element to Windows, matching support present on... (Closed)
|
Created:
10 years, 9 months ago by rsleevi-old Modified:
9 years, 7 months ago CC:
chromium-reviews, pam+watch_chromium.org, John Grabowski, darin-cc_chromium.org, Paweł Hajdan Jr. Base URL:
http://src.chromium.org/svn/trunk/src/ 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 #
Messages
Total messages: 19 (0 generated)
In line with Jens' work on the Mac side, I was trying to bring the Windows side up to parity. The actual importing of the certificate presently happens without prompting the user. My understanding from the comments at http://codereview.chromium.org/652137, specifically http://codereview.chromium.org/652137/diff/2001/2002#newcode2743 , was that this is probably acceptable. The UI can show the user after the fact if something was imported successfully (or if there was an error). I originally looked at the CryptUI series of functions for displaying, but they tended to not like the certs I was testing with (foaf.me) because of a lack of issuer. I was concerned this might seem a negative status to users, even though things were successfully generated. In addition, the CryptUI functions block, but continuing pumping messages (see client cert handler), so it was a no-go. That's why I initially had the .grd file but backed it out. One of the big limitations of this check-in I feel is the lack of smart card support. I'm just not comfortable enough with the whole GUI message lifecycle to feel comfortable introducing any changes, but in order to implement the feature, the user should be prompted during <keygen> about 1) What CSP they want to use 2) What container they want to use [new or existing] 3) Whether they want the key to be exportable 4) Whether they want the key to be "strong" [requiring a pin/UI to unlock for usage] For the NSS libs, this would be equivalent to selecting the slot to generate the key in. The rationale for the Key Cache was because each key container on Windows needs to be unique, and I didn't see any way to associate the related information from the generation and have that followed back to the URLRequest. Further, I didn't think it was guaranteed that the certificate would be the first response received (eg: the user may choose to download the cert separately) I equally wasn't sure if the keys being generated should be cleaned up when the cache goes out of scope, or even persisted as identifiers to a secondary file. I know the NSS code uses the internal slot, while Mac uses the Keychain. I know the Keychain persists settings long term, but I wasn't sure if the NSS internal slot was a per-session temporary or if it was backed by permanent storage, so I wasn't sure what the expected behaviour was. Certainly, removing/enumerating generated keys is much more difficult on the Windows side, so if clutter is a concern, there is probably more work that can be done.
Ryan: Thanks a lot for your patch. Your email request for code review looks like a reply (with "Re:" in the Subject), so I thought you're just commenting on my review of Jens' Mac changelist. Re: prompting the user: we should prompt the user at the beginning of the <keygen> process rather than at the very end, because the purpose of the prompting is more of an approval than a verification of the generated cert (which most users aren't qualified to do). We can bring up UI to display the imported new certificate to make it easy for qualified users to inspect the certificate. I will look at your changelist later. (I see your electronically submitted CLA form.)
Wan-Teh, While I absolutely agree that there should be some process to inform the user and confirm the generation, I'm a little unsure of how to reasonably address that. The backtrace of the call, based on Rev 41389, is something akin to this (using a notation where I list function [code that calls the function:line]) RenderBrowserHost I/O Thread: (OS implementation) [net/base/keygen_handler_(platform).cc] net::KeygenHandler::GenKeyAndSignChallenge() [chrome/browser/renderer_host/resource_message_filter.cc:1357] ResourceMessageFilter::OnKeygen() [chrome/browser/renderer_host/resource_message_filter.cc:523] ResourceMessageFilter::OnMessageReceived() [...] <IPC stuff snipped> RenderProcess Render Thread: RenderThread::Send() [chrome/renderer/renderer_webkit_impl.cc:334] RendererWebKitClientImpl::signedPublicKeyAndChallengeString() [third_party/WebKit/WebKit/chromium/src/ChromiumBridge.cpp:410] ChromiumBridge::signedPublicKeyAndChallengeString() [third_party\WebKit\WebCore\platform\chromium\SSLKeyGeneratorChromium.cpp:54] WebCore::signedPublicKeyAndChallengeString() [third_party\WebKit\WebCore\html\HTMLKeygenElement.cpp:80] HTMLKeygenElement::appendFormData() [third_party\WebKit\WebCore\html\HTMLFormElement.cpp:208] HTMLFormElement::createFormData() [third_party\WebKit\WebCore\html\HTMLFormElement.cpp:323] HTMLFormElement::prepareSubmit() [third_party\WebKit\WebCore\html\HTMLFormElement.cpp:237 or 267 or 269] The point in providing this stack trace was to point that there isn't a readily available way, that I can see at least, to decouple the key generation, which may need to block on user input. Webkit itself blocks on the SPKAC portion, so there is no way I can change the IPC message map to be an IPC_MESSAGE_HANDLER_DELAY_REPLY or something nice and cozy like that. The best way I can currently think to address that particular issue is to change how WebKit handles the Keygen tag, which I'm not sure what the impact of that would be. Option 1) Change the rendering of the <keygen> tag so that, in addition to the <select> element currently rendered [third_party\WebKit\WebCore\html\HTMLKeygenElement.cpp:50-54], a <button> element is rendered to "Generate Key". When this <button> is clicked, it calls the sPKAC method as necessary, passing in the keygen element (via delegate). When the platform-specific implementation has generated the key, it can call setValue() on the element, and it's that value that is returned during appendFormData(). This has the downside of requiring a second level of user interaction before a key can be generated. Option 2) Change the behaviour of WebKit's FormDataList so that, in addition to appendData()/appendFile(), it has appendKeygen(), passing in a Keygen RefPtr. The Keygen object contains the logical values associated with the <keygen> tag (http://www.whatwg.org/specs/web-apps/current-work/#the-keygen-element), such as algorithm, parameters, and the user-selected keysize (as the user selected via the drop-down). This change then propagates through the glue code, such as weburlloader_impl, just as the filenames do, eventually making it to the URLRequest. This has the downside of feeling extremely hack-y, although it's reasonable to assume that <keygen>s may actually be a long-running operation, like an IO file load, since it may involve communication with one or more hardware devices (or even networked devices) to assert all of the authorizations necessary to generate and store the key. Option 3) Change the behaviour of WebKit's form building, so that, like Firefox, it can be restarted/resumed multiple times (akin to the Task passing that goes on everywhere). Dispatching can then happen to the IO thread, which can then push it around as necessary. To even further gum up the works, it's entirely reasonable during a key generation, at least on Windows, that the KSP/CSP may have their own UI that needs to be shown, in addition to whatever "Hey, you, someone wants to generate a key for you". For example, a Smart Card selection dialog (if using the Smart Card CSP/KSP) or a PIN entry dialog. Naturally, this brings even further interactions into consideration, similar to the client cert selection dialog issues that exist already. I mention all of this in this CL to get direction and feedback on the design choices, as I'm more than happy to continue work on this if the prompting behaviour is a must. For what it's worth, I'm absolutely with you on a desire to inform the user (before the action has happened) as well as given them an opportunity to fine tune the parameters (exportable or not, which KSP to use on Win/PK11_Slot on NSS/Keychain file on OS X), I'm just not sure if it's readily addressable without upstream WebKit changes. In examining other browsers: - Firefox: Prompts, as it allows you to select the PKCS#11 slot to tuck the key in, as well as handle some of the other (non-RSA) key generation operations, as well as selecting whether or not the key will be exportable. - Safari: Simply generates and imports the key, no prompting whatsoever (similar to this implementation). The key is exportable. - Jens' Mac implementation: Last I checked, it generated the key automatically, and only prompts about importing the certificate (but does not ask the user where to store the certificate - or the key). The key is exportable. - This implementation: Generates the key automatically using the default CSP/KSP for RSA, disabling any prompts for that CSP/KSP (which will cause it to fail gracefully if the default provider requires a UI). It imports the certificate into the users' personal certificate store. The private key is exportable. - Opera: Have not checked
I suppose I should have added Option 4), which I didn't think was acceptable, but for the sake of thoroughness: Use IPC_MESSAGE_HANDLER_DELAY_REPLY and simply block the renderer thread until it receives a reply. Certainly, of all of the solutions, it ranks as the easiest to implement, if such behaviours are permissible.
As a further clarification to your request for prompting prior to key generation: Do you see the prompting logic being associated with the URL making the request for key generation (the page hosting the form) or the target URL of the form? If the argument is for the latter, then since both the action URL of the form can be changed and the <keygen> element itself can be reparented to another form, it would eliminate the possibility of pursuing Option 1. The unacceptable example would be two forms on a page, one http://good, the other http://bad. The form submitting to http://good has the <keygen>, the user clicks the new UI button to generate the key, which prompts the user to generate a key for http://good, and then the hosting page manipulates the DOM to move the keygen to http://bad (or change good to bad).
On 2010/03/15 18:11:42, wtc wrote: > Re: prompting the user: we should prompt the user at the > beginning of the <keygen> process rather than at the very > end, because the purpose of the prompting is more of an > approval than a verification of the generated cert Hm. I tend to disagree. I can't think of any security implications of sending a CSR to a server; there's no information in the CSR besides a randomly-generated public key. It might be 'nice to have', but the difficulty of doing it (as Ryan points out) outweighs the usefulness.
> Hm. I tend to disagree. I can't think of any security implications of sending a > CSR to a server; there's no information in the CSR besides a randomly-generated > public key. It might be 'nice to have', but the difficulty of doing it (as Ryan > points out) outweighs the usefulness. In a hypothetical world, I suppose there could be security implications if the CSP/KSP improperly implemented a piece of crypto. For example, could the site run timing attacks / denial of service / exhaust entropy using <keygen> tags and JavaScript? Ideally, I'd like to think whatever CSP/KSP the user was using was robust enough that it wouldn't suffer from such exploits. I'd like to think the prompting/control behavior is a second, separate issue than the support for a naive keygen implementation, as supplied in this implementation (which only serves to match the functionality of the other platforms, now that 2 out of 3 have implementations) The inclusion of <keygen> in HTML5, and the discussion related to it, appears more in line with documenting "what exists today" than "what is a usable and useful feature" - and it appears that this patch matches the other vendors' implementations (with the exclusion of Firefox, who originated the tag) Some of the relevant discussions to <keygen> and it's usability: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-January/024759.html http://blog.whatwg.org/this-week-in-html5-episode-35 http://lists.foaf-project.org/pipermail/foaf-protocols/2010-January/001488.html I think longer term, the means to address the perceived deficiencies of <keygen> is to: - Work upstream on WebCore to explore delegate/platform methods of performing such operations asynchronously. This work is likely inevitable if future keytypes other than RSA are to be added, which is a perfectly reasonable assumption, and by designing asynchronously, any number of blocking operations may be required safely (dialogs, hardware device connections, etc) - Consider JavaScript bindings and implement the (non-standard) generateCRMFRequest and potentially other portions of window.crypto, as implemented by Mozilla, as it's the only browser-vendor developed alternative to CertEnroll/XEnroll (Microsoft) or CA-specific implementations (traditionally, Java applets). Reference is at https://developer.mozilla.org/en/JavaScript_crypto . This works by means of JavaScript callbacks to notify when/if a request is generated, and so fits nicely with the requirement/desire to prompt the user. Some of the discussion relevant from WHATWG can be found at http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-June/020110.html
Jens or Wan-Teh, if I have correctly followed http://dev.chromium.org/developers/contributing-code#TOC-If-you-are-not-a-com... , and assuming the prompting issue is not a deal-breaker for this CL, would one of you submit to the try-server and/or commit? I'm not sure why lint is upset with keygen_handler.cc - I've verified the (local) file has newlines, has svn:eol-style LF on it, and my local cpplint.py / gcl lint finds no issue with it.
The most recent modification is to ensure that the key itself is generated in a manner appropriate for CryptoAPI/SChannel to use it when acting as a TLS server, not just as a client certificate. However, due to the lack of CSP configuration/prompting/selection, the resulting key will not end up stored in the CSP that SChannel needs (PROV_RSA_SCHANNEL is needed, instead of the current PROV_RSA_FULL). It unfortunately falls to the user to export the key and re-import it to the SChannel CSP to be viable for server authentication. For client authentication, it is acceptable as is. Seeing as <keygen> was added to HTML5 as a normative reference to existing implementations, and the expected use cases are not well defined AFAIK, this may or may not be an acceptable end-user requirement. Further documentation on the CryptoAPI/SChannel requirements can be had at http://msdn.microsoft.com/en-us/library/aa375195%28VS.85%29.aspx (CryptoAPI 2.0 Private Keys, in case MSDN moves yet again)
This is great. Thanks! I still need to review keygen_handler_win.cc again. I also decided to fix the formatting nits when I commit this CL for you. You can then look at the changes I make to learn about our formatting style. It's more efficient to communicate the style issues that way. Here are the preliminary comments. 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 is Nit: add "of" after "version". The input parameter |cert| can be declared "const". http://codereview.chromium.org/843005/diff/45001/46005#newcode32 net/base/cert_database_win.cc:32: PCERT_PUBLIC_KEY_INFO cert_info = |cert_info| is poorly named because it points to SubjectPublicKeyInfo. You can rename it |key_info|. (In NSS, this variable is commonly named |spki|, but Google style guide discourages using abbreviations as variable names.) http://codereview.chromium.org/843005/diff/45001/46005#newcode33 net/base/cert_database_win.cc:33: &(cert->os_cert_handle()->pCertInfo->SubjectPublicKeyInfo); Nit: when a long line is wrapped, the continuation lines are indented by 4. Same for lines 59 and 61 below. http://codereview.chromium.org/843005/diff/45001/46005#newcode42 net/base/cert_database_win.cc:42: &output.front(), &size); Nit: use &output[0] instead of &output.front(). This is an arbitrary preference. Same for line 46 below. Actually, we have a WriteInto template that you can use to let CryptEncodeObject directly write into the |result| string. http://codereview.chromium.org/843005/diff/45001/46005#newcode70 net/base/cert_database_win.cc:70: return ok == TRUE; Nit: this should be return ok != 0; http://codereview.chromium.org/843005/diff/45001/46005#newcode78 net/base/cert_database_win.cc:78: void CertDatabase::Init() { Move the Init() method back to the end of this file. Google style guide wants us to try to define methods in the same order they're declared in the header files. http://codereview.chromium.org/843005/diff/45001/46005#newcode98 net/base/cert_database_win.cc:98: &key_spec, &should_free); |should_free| is not used. Why? http://codereview.chromium.org/843005/diff/45001/46005#newcode101 net/base/cert_database_win.cc:101: // os_cert_handle() will auto-close it Nit: close the sentence with a period (.). http://codereview.chromium.org/843005/diff/45001/46005#newcode131 net/base/cert_database_win.cc:131: return ERR_ERR_ADD_USER_CERT_FAILED; ERR_ERR means we have an extra ERR in net_error_list.h for this error code. Could you fix that? http://codereview.chromium.org/843005/diff/45001/46007 File net/base/keygen_handler.h (right): http://codereview.chromium.org/843005/diff/45001/46007#newcode33 net/base/keygen_handler.h:33: std::string keychain_path; Nit: indentation off by one. http://codereview.chromium.org/843005/diff/45001/46008 File net/base/keygen_handler_mac.cc (right): http://codereview.chromium.org/843005/diff/45001/46008#newcode92 net/base/keygen_handler_mac.cc:92: const net::KeygenHandler::KeyCache::KeyLocation& location) const { Indent the parameter list by 4. http://codereview.chromium.org/843005/diff/45001/46009 File net/base/keygen_handler_nss.cc (right): http://codereview.chromium.org/843005/diff/45001/46009#newcode55 net/base/keygen_handler_nss.cc:55: const net::KeygenHandler::KeyCache::KeyLocation& location) const { Nit: indent the parameter list by 4. http://codereview.chromium.org/843005/diff/45001/46011 File net/base/keygen_handler_win.cc (right): http://codereview.chromium.org/843005/diff/45001/46011#newcode70 net/base/keygen_handler_win.cc:70: &output->at(old_size), &size); Nit: use &(*output)[old_size] http://codereview.chromium.org/843005/diff/45001/46011#newcode219 net/base/keygen_handler_win.cc:219: // FIXME(rsleevi): Is there a less-hackish way to get this through to convert Nit: FIXME => TODO http://codereview.chromium.org/843005/diff/45001/46011#newcode277 net/base/keygen_handler_win.cc:277: const net::KeygenHandler::KeyCache::KeyLocation& location) const { Indent by 4.l
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 is On 2010/03/25 07:54:29, wtc wrote: > Nit: add "of" after "version". > > The input parameter |cert| can be declared "const". Done. http://codereview.chromium.org/843005/diff/45001/46005#newcode32 net/base/cert_database_win.cc:32: PCERT_PUBLIC_KEY_INFO cert_info = On 2010/03/25 07:54:29, wtc wrote: > |cert_info| is poorly named because it points to > SubjectPublicKeyInfo. You can rename it |key_info|. > (In NSS, this variable is commonly named |spki|, but > Google style guide discourages using abbreviations as > variable names.) Done. http://codereview.chromium.org/843005/diff/45001/46005#newcode33 net/base/cert_database_win.cc:33: &(cert->os_cert_handle()->pCertInfo->SubjectPublicKeyInfo); On 2010/03/25 07:54:29, wtc wrote: > Nit: when a long line is wrapped, the continuation lines > are indented by 4. Same for lines 59 and 61 below. Done. http://codereview.chromium.org/843005/diff/45001/46005#newcode42 net/base/cert_database_win.cc:42: &output.front(), &size); On 2010/03/25 07:54:29, wtc wrote: > Nit: use &output[0] instead of &output.front(). This is an > arbitrary preference. Same for line 46 below. > > Actually, we have a WriteInto template that you can use to > let CryptEncodeObject directly write into the |result| string. Done. http://codereview.chromium.org/843005/diff/45001/46005#newcode70 net/base/cert_database_win.cc:70: return ok == TRUE; On 2010/03/25 07:54:29, wtc wrote: > Nit: this should be > return ok != 0; Done. http://codereview.chromium.org/843005/diff/45001/46005#newcode78 net/base/cert_database_win.cc:78: void CertDatabase::Init() { On 2010/03/25 07:54:29, wtc wrote: > Move the Init() method back to the end of this file. > Google style guide wants us to try to define methods in the > same order they're declared in the header files. Done. http://codereview.chromium.org/843005/diff/45001/46005#newcode98 net/base/cert_database_win.cc:98: &key_spec, &should_free); On 2010/03/25 07:54:29, wtc wrote: > |should_free| is not used. Why? should_free will always be false, because of the CRYPT_ACQUIRE_CACHE_FLAG. Per MSDN: pfCallerFreeProvOrNCryptKey [out] The address of a BOOL variable that receives a value that indicates whether the caller must free the handle returned in the phCryptProvOrNCryptKey variable. This receives FALSE if any of the following is true: * Public key acquisition or comparison fails. * The dwFlags parameter contains the CRYPT_ACQUIRE_CACHE_FLAG flag. * The dwFlags parameter contains the CRYPT_ACQUIRE_USE_PROV_INFO_FLAG flag, the certificate context property is set to CERT_KEY_PROV_INFO_PROP_ID with the CRYPT_KEY_PROV_INFO structure, and the dwFlags member of the CRYPT_KEY_PROV_INFO structure is set to CERT_SET_KEY_CONTEXT_PROP_ID. So in a successful retrieval, it will be false because of the flag, and in a failure scenario, it will be false because acquisition/comparison failed. http://codereview.chromium.org/843005/diff/45001/46005#newcode101 net/base/cert_database_win.cc:101: // os_cert_handle() will auto-close it On 2010/03/25 07:54:29, wtc wrote: > Nit: close the sentence with a period (.). Done. http://codereview.chromium.org/843005/diff/45001/46005#newcode131 net/base/cert_database_win.cc:131: return ERR_ERR_ADD_USER_CERT_FAILED; On 2010/03/25 07:54:29, wtc wrote: > ERR_ERR means we have an extra ERR in net_error_list.h for > this error code. Could you fix that? Done. http://codereview.chromium.org/843005/diff/45001/46007 File net/base/keygen_handler.h (right): http://codereview.chromium.org/843005/diff/45001/46007#newcode33 net/base/keygen_handler.h:33: std::string keychain_path; On 2010/03/25 07:54:29, wtc wrote: > Nit: indentation off by one. Done. http://codereview.chromium.org/843005/diff/45001/46008 File net/base/keygen_handler_mac.cc (right): http://codereview.chromium.org/843005/diff/45001/46008#newcode92 net/base/keygen_handler_mac.cc:92: const net::KeygenHandler::KeyCache::KeyLocation& location) const { On 2010/03/25 07:54:29, wtc wrote: > Indent the parameter list by 4. Done. http://codereview.chromium.org/843005/diff/45001/46009 File net/base/keygen_handler_nss.cc (right): http://codereview.chromium.org/843005/diff/45001/46009#newcode55 net/base/keygen_handler_nss.cc:55: const net::KeygenHandler::KeyCache::KeyLocation& location) const { On 2010/03/25 07:54:29, wtc wrote: > Nit: indent the parameter list by 4. Done. http://codereview.chromium.org/843005/diff/45001/46011 File net/base/keygen_handler_win.cc (right): http://codereview.chromium.org/843005/diff/45001/46011#newcode70 net/base/keygen_handler_win.cc:70: &output->at(old_size), &size); On 2010/03/25 07:54:29, wtc wrote: > Nit: use &(*output)[old_size] Done. http://codereview.chromium.org/843005/diff/45001/46011#newcode219 net/base/keygen_handler_win.cc:219: // FIXME(rsleevi): Is there a less-hackish way to get this through to convert On 2010/03/25 07:54:29, wtc wrote: > Nit: FIXME => TODO Done. http://codereview.chromium.org/843005/diff/45001/46011#newcode277 net/base/keygen_handler_win.cc:277: const net::KeygenHandler::KeyCache::KeyLocation& location) const { On 2010/03/25 07:54:29, wtc wrote: > Indent by 4.l Done.
Ryan, Here are some more comments. I still haven't reviewed the new Windows code, but I reviewed the KeyCache and KeyLocation code (didn't really do that last night). I tend to write a lot of comments, so please don't rely with "Done" to my comments. I will need to work on HTTP NTLM/Negotiate auth at work today, so I may not review this CL again until tonight or tomorrow. Thanks again for the good work! http://codereview.chromium.org/843005/diff/45001/46005 File net/base/cert_database_win.cc (right): http://codereview.chromium.org/843005/diff/45001/46005#newcode98 net/base/cert_database_win.cc:98: &key_spec, &should_free); On 2010/03/25 09:11:43, rsleevi wrote: > > So in a successful retrieval, it will be false because of the flag, and in a > failure scenario, it will be false because acquisition/comparison failed. Thanks for the explanation. Could you add the above as a comment, and a DCHECK_EQ(should_free, FALSE)? 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: KeyLocationMap::iterator key_pos(cache_.find(public_key_info)); Nit: use assignment (=) instead of constructor initialization. This variable is commonly named |iter| in our code base. http://codereview.chromium.org/843005/diff/60001/61007 File net/base/keygen_handler.h (right): http://codereview.chromium.org/843005/diff/60001/61007#newcode24 net/base/keygen_handler.h:24: // generated, so that the private key can be properly associated with any Nit: "private keys" (plural) http://codereview.chromium.org/843005/diff/60001/61007#newcode26 net/base/keygen_handler.h:26: class KeyCache { The nested types have very long names. If they will remain nested, we can remove "Key" from the names, so we'll say: KeygenHandler::Cache KeygenHandler::Cache::Location Another solution is to move KeyLocation outside KeyCache, so we can say KeygenHandler::KeyLocation. Or a combination of both solutions: KeygenHandler::Cache KeygenHandler::Location http://codereview.chromium.org/843005/diff/60001/61007#newcode39 net/base/keygen_handler.h:39: bool Equals(const KeyLocation& location) const; Does the compiler generate an equality operator (==) for us? http://codereview.chromium.org/843005/diff/60001/61007#newcode43 net/base/keygen_handler.h:43: void Insert(const std::string& public_key_info, Should KeyCache have a Remove() method? We want to remove a location when keygen completes (successfully or with a failure), right? http://codereview.chromium.org/843005/diff/60001/61007#newcode47 net/base/keygen_handler.h:47: // |location| Nit: |location| => |*location| http://codereview.chromium.org/843005/diff/60001/61008 File net/base/keygen_handler_mac.cc (right): http://codereview.chromium.org/843005/diff/60001/61008#newcode91 net/base/keygen_handler_mac.cc:91: bool KeygenHandler::KeyCache::KeyLocation::Equals( We're not using KeygenHandler::KeyCache::Insert() in this Mac file. Should we add a TODO comment? http://codereview.chromium.org/843005/diff/60001/61009 File net/base/keygen_handler_nss.cc (right): http://codereview.chromium.org/843005/diff/60001/61009#newcode217 net/base/keygen_handler_nss.cc:217: char* slot_name = PK11_GetSlotName(slot); |slot_name| should be declared "const". I don't know why PK11_GetSlotName() returns a non-const char pointer. Sigh. Remove the null check for |slot_name| below because it cannot be NULL: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap... http://codereview.chromium.org/843005/diff/60001/61009#newcode224 net/base/keygen_handler_nss.cc:224: // On successful keygen we need to keep the private key, of course, I believe this comment should be moved into the "else" block above. http://codereview.chromium.org/843005/diff/60001/61010 File net/base/keygen_handler_unittest.cc (right): http://codereview.chromium.org/843005/diff/60001/61010#newcode54 net/base/keygen_handler_unittest.cc:54: KeygenHandler::KeyCache::KeyLocation ValidLocation() { Nit: Move the ValidLocation() and DefaultLocation() functions before the first unit test. http://codereview.chromium.org/843005/diff/60001/61010#newcode68 net/base/keygen_handler_unittest.cc:68: KeygenHandler::KeyCache::KeyLocation DefaultLocation() { Do we really need the DefaultLocation() function? It's nothing more than the default constructor. http://codereview.chromium.org/843005/diff/60001/61010#newcode85 net/base/keygen_handler_unittest.cc:85: // should be transitive Nit: transitive => reflexive Transitivity means a=b and b=c implies a=c. http://codereview.chromium.org/843005/diff/60001/61010#newcode93 net/base/keygen_handler_unittest.cc:93: // The cache should miss for an unregistered hit Nit: "unregistered hit" => "unregister key"
I forgot to point out my most important comment in the previous review: should KeyCache have a Remove() method?
Wan-Teh, I hope I've addressed your feedback in the proper manner - I wasn't sure if I was supposed to comment on the existing code or in the newly submitted code. I opted for the former, so apologies if this was incorrect. 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: KeyLocationMap::iterator key_pos(cache_.find(public_key_info)); On 2010/03/25 14:44:43, wtc wrote: > Nit: use assignment (=) instead of constructor initialization. > > This variable is commonly named |iter| in our code base. Done. Both the naming and the assignment were inherited from the style of x509_certificate.cc, 106-107, so I'm not sure if those need to be changed at some point. http://codereview.chromium.org/843005/diff/60001/61007 File net/base/keygen_handler.h (right): http://codereview.chromium.org/843005/diff/60001/61007#newcode26 net/base/keygen_handler.h:26: class KeyCache { > Or a combination of both solutions: > KeygenHandler::Cache > KeygenHandler::Location Renamed to this scheme, and changed to classes with public rather than structs, per Chromium project (but not Google C++ Style Guide) standards, if I understood correctly. http://codereview.chromium.org/843005/diff/60001/61007#newcode39 net/base/keygen_handler.h:39: bool Equals(const KeyLocation& location) const; On 2010/03/25 14:44:43, wtc wrote: > Does the compiler generate an equality operator (==) for us? No, at least not a suitable one (non-POD) I wasn't sure what the unit test guidelines were. Because it's only used by the unit tests, it seems to me like it would make sense to push it into the test file, but I didn't want to decouple the platform-specific field definitions from the actual tests. http://codereview.chromium.org/843005/diff/60001/61007#newcode43 net/base/keygen_handler.h:43: void Insert(const std::string& public_key_info, On 2010/03/25 14:44:43, wtc wrote: > Should KeyCache have a Remove() method? We want to remove > a location when keygen completes (successfully or with a > failure), right? I'm not sure if Remove() is necessary. I was modeling after the X.509 cert cache, and had trouble finding a sensible explanation for what Remove() should mean. Should Remove() actually remove the generated private key (by using CRYPT_DELETEKEYSET and the equivalents in NSS/Keychain), or should it simply "forget" that this key ever existed - leaving it on the system, but otherwise unreachable. Further, what are the conditions one would forget about a key/remove it? After a certificate had been successfully imported? What if the user of the <keygen> tag intended for more than one certificate to be generated (for example, in a federated PKI without bridge CAs, where instead multiple certificates, each signed by a different CA, were generated?) And if a certificate failed to import, could we be certain the user would not need the key? I was already uncomfortable with the cache as is, because if I understand the Chrome process model correctly, there exist multiple Browser Hosts per logical user session, in which case, the Key Cache won't end up being shared across processes. Of course, if there is "one parent to rule them all", and if this cache lives within that memory space, then this isn't a problem. I'm still getting familiar with the IPC model being used across the platforms. Likewise, I had concerns for the Find() implementation - should Find() only consult the LocationMap, or should it enumerate all CSPs, and all containers, exporting any public key info blocks that it can from both the AT_SIGNATURE and the AT_KEYEXCHANGE slots, and comparing them against the target key? Should searching terminate at the first encounter, or should it continue, in the event the user has the key stored in multiple locations (eg: physical storage and network storage) This method would fail if the key was stored on a smart card and the card was not currently inserted, so how would a false Find() be interpreted - that the key never existed, or that it might exist, just not accessible yet? Would this process need to be repeated every time the key was looked up, in the event the card is now inserted, and what would the performance implications be? Would a SmartcardChangeObserver (similar to a NetworkChangeObserver) need to be implemented cross-platform to monitor for such events? For these reasons, I opted to keep the implementation simple - keys are stored permanently at generation, their locations are stored in the cache, and the cache only consults for keys that have been generated with <keygen> If necessary, I'd be happy to commit further improvements: 1) An object to represent a key autodeleter, so that unless the trigger is disabled, will automatically delete the key on destruction (presumably, at process shutdown) 2) The cache scans CSPs & containers for keys, and not just the ones it has generated http://codereview.chromium.org/843005/diff/60001/61008 File net/base/keygen_handler_mac.cc (right): http://codereview.chromium.org/843005/diff/60001/61008#newcode91 net/base/keygen_handler_mac.cc:91: bool KeygenHandler::KeyCache::KeyLocation::Equals( On 2010/03/25 14:44:43, wtc wrote: > We're not using KeygenHandler::KeyCache::Insert() in this Mac > file. Should we add a TODO comment? If you think so. We're not using Find() in the cert_database_mac, because currently both KeygenHandler and CertDatabase presume the default keychain (a NULL string), and uses the Keychain APIs that are in front of the CSSM DL/DB APIs to do the key locating. The way I see it, there are two implementation paths for handling certificates: 1) Only allow certificates with keys generated via <keygen> to be imported. This matches the Win32 code (as proposed), but not the OS X/NSS code. The OS X/NSS code would simply need to make sure it Insert()s on <keygen> and Find()s on import 2) Allow certificates for any keys to be imported. This matches the (current) OS X/NSS code, AFAICT. To make all platforms unified, the Win32 code would need to use the previously mentioned CSP/Container searching code
Ryan, I finished the review. It's a long CL! Thank you very much for contributing it. I have a lot of comments below. Please don't respond with "Done", so that your reply can be shorter. After you address this round of comments, I will check this in. http://codereview.chromium.org/843005/diff/73001/74005 File net/base/cert_database_win.cc (right): http://codereview.chromium.org/843005/diff/73001/74005#newcode41 net/base/cert_database_win.cc:41: reinterpret_cast<BYTE*>(WriteInto(&result, size)), I believe we need to pass |size + 1| as the second argument to WriteInto(), otherwise the str->resize() call in the WriteInto() template will be passed |size - 1|. http://codereview.chromium.org/843005/diff/73001/74005#newcode57 net/base/cert_database_win.cc:57: bool UpdateCert(X509Certificate* cert, This function could use a better name. "UpdateCert" is vague. Perhaps something like UpdateCertWithKeyContainer or LinkCertToKeyContainer? http://codereview.chromium.org/843005/diff/73001/74005#newcode58 net/base/cert_database_win.cc:58: KeygenHandler::Location location) { Now I think "KeyLocation" is better than "Location". I can take care of this renaming. http://codereview.chromium.org/843005/diff/73001/74005#newcode69 net/base/cert_database_win.cc:69: prov_info.dwKeySpec = AT_KEYEXCHANGE; Nit: I know this value doesn't matter, but let's set it to AT_SIGNATURE because in SSL client auth, the RSA private key is used to sign handshake hashes. 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; Are you sure we should pass the CRYPT_ACQUIRE_SILENT_FLAG flag? 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 don't understand your comment "This call will never succeed if the cert has been built from a byte stream, which is /almost/ always the case." Doesn't that mean this call will almost always fail? http://codereview.chromium.org/843005/diff/73001/74005#newcode130 net/base/cert_database_win.cc:130: // construction parameters (Keychain filepath on OS X, Slot/Reader on NSS, Nit: "Slot/Reader" => "PKCS #11 slot" http://codereview.chromium.org/843005/diff/73001/74005#newcode134 net/base/cert_database_win.cc:134: HCERTSTORE cert_db = CertOpenStore(CERT_STORE_PROV_SYSTEM, Why don't we just call CertOpenSystemStore(NULL, L"MY")? Then we won't need to worry about open_flags. See also the example at the bottom of http://msdn.microsoft.com/en-us/library/aa376559(v=VS.85).aspx http://codereview.chromium.org/843005/diff/73001/74005#newcode147 net/base/cert_database_win.cc:147: CertCloseStore(cert_db, 0); Move this CertCloseStore call before the if (!added) statement, otherwise we leak cert_db on the error path. http://codereview.chromium.org/843005/diff/73001/74005#newcode152 net/base/cert_database_win.cc:152: void CertDatabase::Init() { We should do something about the Init() method. Either remove Init(), or define the CertDatabase constructor to call Init() on all platforms. http://codereview.chromium.org/843005/diff/73001/74007 File net/base/keygen_handler.h (right): http://codereview.chromium.org/843005/diff/73001/74007#newcode23 net/base/keygen_handler.h:23: // This class stores the relative location for a given private key. It does Nit: remove "relative"? Is there an absolute location? The fact that the smart card that holds the private key may not be present seems irrelevant here. Do we need to mention that? 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 think we should move this block of code to line 201 above, right before the |failure:| label, because we should execute this block of code only if |isSuccess| is true, regardless of the value of |stores_key_|. http://codereview.chromium.org/843005/diff/73001/74011 File net/base/keygen_handler_win.cc (right): http://codereview.chromium.org/843005/diff/73001/74011#newcode75 net/base/keygen_handler_win.cc:75: // Sometimes the initial call to CryptEncodeObject was wrong about the size, Nit: was wrong about => gave a generous estimate of http://codereview.chromium.org/843005/diff/73001/74011#newcode85 net/base/keygen_handler_win.cc:85: std::wstring wide_challenge = UTF8ToWide(challenge); We should be able to use ASCIIToWide if |challenge| contains a DER IA5String. Does |challenge| begin with the tag and length? Can we use X509_ANY_STRING to avoid the conversion to wide? http://codereview.chromium.org/843005/diff/73001/74011#newcode92 net/base/keygen_handler_win.cc:92: reinterpret_cast<const BYTE*>(wide_challenge.c_str())); We could use wide_challenge.data() here and sizeof(wchar_t) * wide_challenge.size() (instead of 0) below. http://codereview.chromium.org/843005/diff/73001/74011#newcode105 net/base/keygen_handler_win.cc:105: // From the private key stored in HCRYPTKEY, obtain the public key, stored as Should "HCRYPTKEY" be "HCRYPTPROV"? http://codereview.chromium.org/843005/diff/73001/74011#newcode108 net/base/keygen_handler_win.cc:108: ok = CryptExportPublicKeyInfoEx(prov, AT_KEYEXCHANGE, X509_ASN_ENCODING, Change AT_KEYEXCHANGE to AT_SIGNATURE if you make my suggested change to cert_database_win.cc. http://codereview.chromium.org/843005/diff/73001/74011#newcode123 net/base/keygen_handler_win.cc:123: Should we call public_key_info.resize(size)? http://codereview.chromium.org/843005/diff/73001/74011#newcode169 net/base/keygen_handler_win.cc:169: // This happens to be the same naive type as an SPKAC, so this works. Nice! :-) http://codereview.chromium.org/843005/diff/73001/74011#newcode172 net/base/keygen_handler_win.cc:172: info.ToBeSigned.pbData = &pkac.front(); Nit: &pkac.front() => &pkac[0] Please fix all occurrences of &xxx.front() in this file. http://codereview.chromium.org/843005/diff/73001/74011#newcode173 net/base/keygen_handler_win.cc:173: info.SignatureAlgorithm.pszObjId = szOID_RSA_MD5RSA; Do you know if we can sign with SHA-1-RSA? Just curious. http://codereview.chromium.org/843005/diff/73001/74011#newcode227 net/base/keygen_handler_win.cc:227: void StoreKeyInCache(HCRYPTPROV prov) { Nit: StoreKeyInCache => StoreKeyLocationInCache because it's the location that's stored. http://codereview.chromium.org/843005/diff/73001/74011#newcode249 net/base/keygen_handler_win.cc:249: UTF8ToWide(reinterpret_cast<char*>(&buffer.front()), size, Can we assume the cotainer and provider names are UTF-8? http://codereview.chromium.org/843005/diff/73001/74011#newcode285 net/base/keygen_handler_win.cc:285: bool isSuccess = true; Nit: isSuccess => is_success I know you copied this variable name from keygen_handler_nss.cc, which did not follow the naming convention. It's better to initialize is_success to false, and set it to true only after everything has succeeded, at line 333. http://codereview.chromium.org/843005/diff/73001/74011#newcode306 net/base/keygen_handler_win.cc:306: if (GetLastError() != NTE_BAD_KEYSET || IMPORTANT: It seems that we should retry with a new key id if the first CryptAcquireContext call succeeds, otherwise we'll overwrite the existing keypair in the key container, right? http://codereview.chromium.org/843005/diff/73001/74011#newcode316 net/base/keygen_handler_win.cc:316: if (!CryptGenKey(prov, CALG_RSA_KEYX, flags, &key)) { Nit: move the declaration of |flags| here. Nit: CALG_RSA_KEYX => CALG_RSA_SIGN ? http://codereview.chromium.org/843005/diff/73001/74011#newcode342 net/base/keygen_handler_win.cc:342: StoreKeyInCache(prov); Move the StoreKeyInCache call to line 333 above, right before the |failure:| label. http://codereview.chromium.org/843005/diff/73001/74011#newcode344 net/base/keygen_handler_win.cc:344: // Securely destroys the handle, but leaves the underlying key alone. The This comment is inaccurate or needs updating because we do destroy the underlying key at line 354 below if stores_key_ is false. Nit: KeyCache location => key location http://codereview.chromium.org/843005/diff/73001/74011#newcode350 net/base/keygen_handler_win.cc:350: if (stores_key_) { I think lines 350-356 should be: CryptReleaseContext(prov, 0); prov = NULL; if (!stores_key_) { // Fully destroys any of the keys that were created and releases prov CryptAcquireContext(&prov, new_key_id.c_str(), NULL, PROV_RSA_FULL, CRYPT_DELETEKEYSET); } Otherwise the original |prov| is leaked.
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: KeyLocationMap::iterator key_pos(cache_.find(public_key_info)); On 2010/03/30 23:14:15, rsleevi wrote: > > Both the naming and the assignment were inherited from the style of > x509_certificate.cc, 106-107, so I'm not sure if those need to be changed at > some point. I see. This is probably a matter of personal preference. No need to change x509_certificate.cc. http://codereview.chromium.org/843005/diff/60001/61007 File net/base/keygen_handler.h (right): http://codereview.chromium.org/843005/diff/60001/61007#newcode26 net/base/keygen_handler.h:26: class KeyCache { On 2010/03/30 23:14:15, rsleevi wrote: > ..., and changed to classes with public rather than structs, > per Chromium project (but not Google C++ Style Guide) standards, if I understood > correctly. Yes. This is so that we can always use "class Foo" in a forward declaration without having to check the header file. http://codereview.chromium.org/843005/diff/60001/61007#newcode39 net/base/keygen_handler.h:39: bool Equals(const KeyLocation& location) const; It is perfectly fine to add a method that's only used by unit tests. You can consider putting the method inside #if defined(UNIT_TEST) #endif Since this method is short, I would not bother. You can remove the "FIXME" now. Just say: // This is only for unit tests. http://codereview.chromium.org/843005/diff/60001/61007#newcode43 net/base/keygen_handler.h:43: void Insert(const std::string& public_key_info, Your reply to my question on a Remove() method is very long. It's 7:30pm now and I am tired, so I didn't read your reply carefully. The following is my response to your comments on Remove() and whether the Mac code should call Insert(): I think the goal of the key location cache is to prevent the injection of the certificate that we didn't ask the CA to issue. So the key location cache stores all the outstanding keygen requests. When a keygen request completes successfully and the new cert is successfully imported, we can remove the corresponding entry from the cache. On Windows, the key location cache also serves the purpose of locating the key container. Perhaps this is actually what you designed the cache for. Right now I'd like to check in what you have now (after addressing the latest round of comments) and work on cleanup or enhancements in follow-up changelists.
Still a few outstanding questions for you, Wan-Teh, but hopefully this will be finished soon. http://codereview.chromium.org/843005/diff/60001/61007 File net/base/keygen_handler.h (right): http://codereview.chromium.org/843005/diff/60001/61007#newcode43 net/base/keygen_handler.h:43: void Insert(const std::string& public_key_info, My goal was solely to serve as a location cache. This is absolutely necessary for Windows, but if prompting for the location/store were implemented, would be necessary across all platforms given the decoupling between the <keygen> request (handled during WebKit form submission) and the certificate loading (handled at some arbitrary point in the future) I was not trying to use it as an outstanding queue, simply because it seemed a legitimate use case that there could be a 1-to-many relationship between keys and certificates. As such, treating it as a pending queue would cause, on Windows at least, each n certificate, where n is > 1, to fail to be imported properly (because the location will have been forgotten) That's why I omitted including Remove(), since the browser cannot know if more certificates are incoming. I omitted Insert() on Mac because I understood a NULL Keychain location to be meaningful and valid, and not like the Windows/NSS code, where it indicates "default", where "default" may change during the course of program operation. http://codereview.chromium.org/843005/diff/73001/74005 File net/base/cert_database_win.cc (right): http://codereview.chromium.org/843005/diff/73001/74005#newcode41 net/base/cert_database_win.cc:41: reinterpret_cast<BYTE*>(WriteInto(&result, size)), On 2010/03/31 02:22:47, wtc wrote: > I believe we need to pass |size + 1| as the second argument > to WriteInto(), otherwise the str->resize() call in the > WriteInto() template will be passed |size - 1|. Thanks, I should have paid attention to "length_with_null" http://codereview.chromium.org/843005/diff/73001/74005#newcode69 net/base/cert_database_win.cc:69: prov_info.dwKeySpec = AT_KEYEXCHANGE; On 2010/03/31 02:22:47, wtc wrote: > Nit: I know this value doesn't matter, but let's set it to > AT_SIGNATURE because in SSL client auth, the RSA private key > is used to sign handshake hashes. AT_KEYEXCHANGE actually incorporates AT_SIGNATURE, at least when using the RSA provider. The justification for why I switched to AT_KEYEXCHANGE, when originally using AT_SIGNATURE, is explained in the notes for Patch Set 5. Basically, it's to permit server certificate enrollment, and not just limit it to client certificates. 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 02:22:47, wtc wrote: > Are you sure we should pass the CRYPT_ACQUIRE_SILENT_FLAG flag? If the default CSP for RSA has been assigned to a third-party CSP, then a GUI dialog may be spawned by the CSP if we don't. An example would be replacing the default CSP with a vendor-specific smart-card provider, and the GUI being "Please insert Smartcard X,Y,Z" Since this code currently makes no assumptions about which thread it is running on, and lives in net/base (rather than in chrome), I didn't want to bother trying to find an acceptable GUI thread to swing over to. If it's an acceptable precondition to make this code only callable on the GUI thread, and if the possibility of a CSP pumping messages is acceptable, then the silent flag could be turned off. 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, On 2010/03/31 02:22:47, wtc wrote: > I don't understand your comment "This call will never succeed if the cert has > been built from a byte stream, which is > /almost/ always the case." > > Doesn't that mean this call will almost always fail? Almost ;) The nuance comes from the possibility of a server supplying an encoding for a certificate which already exists, which depending on the parameters used to construct that certificate from the byte stream, may result in a handle being passed by the OS to an already open certificate. The other possibility is a caller passing in a handle previously obtained from the operating system. In the /current/ implementation of the X.509 loading, this isn't the case. However, my goal was to ensure that I wasn't coupling the specifics of CertDatabase to the <keygen> tag, since the comments by guaravsh in cert_database.h seemed to indicate that the intent was for this to be used outside just supporting <keygen> Certainly, this call can be removed, but it's presence exists as a short-circuit for the potentially more time-consuming serializing/searching/updating when we already have the key associated. 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 02:22:47, wtc wrote: > Why don't we just call CertOpenSystemStore(NULL, L"MY")? > Then we won't need to worry about open_flags. See also the > example at the bottom of > http://msdn.microsoft.com/en-us/library/aa376559%28v=VS.85%29.aspx Mainly to be able to explicitly pass the CERT_STORE_MAXIMUM_ALLOWED_FLAG, since we know that, if things work, we'll be modifying the store, and that store may be (and often is) a registry store. http://codereview.chromium.org/843005/diff/73001/74005#newcode152 net/base/cert_database_win.cc:152: void CertDatabase::Init() { On 2010/03/31 02:22:47, wtc wrote: > We should do something about the Init() method. > > Either remove Init(), or define the CertDatabase constructor > to call Init() on all platforms. I opted to remove Init(), since you suggested it - I just wasn't sure if this violates the Style Guide since the NSS implementation has to ensure NSS is Init. If this does need to be moved to an explicit Init() function, what is the style guideline for calling Init() on all platforms? Is it acceptable/desirable to declare the ctor body in the .h? http://codereview.chromium.org/843005/diff/73001/74007 File net/base/keygen_handler.h (right): http://codereview.chromium.org/843005/diff/73001/74007#newcode23 net/base/keygen_handler.h:23: // This class stores the relative location for a given private key. It does On 2010/03/31 02:22:47, wtc wrote: > Nit: remove "relative"? Is there an absolute location? > > The fact that the smart card that holds the private key > may not be present seems irrelevant here. Do we need to > mention that? My intent was merely to communicate that a KeyLocation doesn't guarantee acquisition will succeed, even when present in the cache. This differs from the X.509 cache, where presence in the cache guarantees acquisition of the certificate (since the cache itself holds the certificate open) 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(); On 2010/03/31 02:22:47, wtc wrote: > I think we should move this block of code to line 201 above, > right before the |failure:| label, because we should execute > this block of code only if |isSuccess| is true, regardless of > the value of |stores_key_|. I'm not sure I follow. If |stores_key_| is false, my understanding is that the underlying key itself will be destroyed - not just released. This means that acquisition moves from a general "may fail" to a guaranteed "will fail" when |stores_key_| is not set. Since acquisition will always fail, and the underlying key will be irretrievable, why bother inserting into the cache at all? http://codereview.chromium.org/843005/diff/73001/74011 File net/base/keygen_handler_win.cc (right): http://codereview.chromium.org/843005/diff/73001/74011#newcode85 net/base/keygen_handler_win.cc:85: std::wstring wide_challenge = UTF8ToWide(challenge); On 2010/03/31 02:22:47, wtc wrote: > We should be able to use ASCIIToWide if |challenge| contains > a DER IA5String. Does |challenge| begin with the tag and > length? > > Can we use X509_ANY_STRING to avoid the conversion to wide? Honestly, I'm not sure. Per http://msdn.microsoft.com/en-us/library/aa378145%28VS.85%29.aspx "Decode noncharacter strings by using a lpszStructType of X509_ANY_STRING." Empirically, it seems to work - although I haven't bothered to trace to see if under the hood Windows is just converting back to Unicode during the API call tree, as it tends to do frequently. http://codereview.chromium.org/843005/diff/73001/74011#newcode92 net/base/keygen_handler_win.cc:92: reinterpret_cast<const BYTE*>(wide_challenge.c_str())); On 2010/03/31 02:22:47, wtc wrote: > We could use wide_challenge.data() here and > sizeof(wchar_t) * wide_challenge.size() (instead of 0) below. Good point, in the event challenge contains embedded NULLs. http://codereview.chromium.org/843005/diff/73001/74011#newcode173 net/base/keygen_handler_win.cc:173: info.SignatureAlgorithm.pszObjId = szOID_RSA_MD5RSA; On 2010/03/31 02:22:47, wtc wrote: > Do you know if we can sign with SHA-1-RSA? Just curious. Yeah. I've been following the upstream bug you created and the specification, hoping that MD5 will be abandoned as well. I had the same concern when I started looking at Jens' implementation, until I realized it was explicitly stated in the spec. http://codereview.chromium.org/843005/diff/73001/74011#newcode249 net/base/keygen_handler_win.cc:249: UTF8ToWide(reinterpret_cast<char*>(&buffer.front()), size, On 2010/03/31 02:22:47, wtc wrote: > Can we assume the cotainer and provider names are UTF-8? From MSDN: http://msdn.microsoft.com/en-us/library/aa380196%28VS.85%29.aspx The name of the current key container as a null-terminated CHAR string. This string is exactly the same as the one passed in the pszContainer parameter of the CryptAcquireContext function to specify the key container to use. The pszContainer parameter can be read to determine the name of the default key container. I can see it going either way. I felt it safer to presume UTF8, rather than ASCII, because on acquisition, the default function takes the name as a UTF-16 string (LPCWSTR). If one can put Unicode in, I would presume one will get Unicode out. http://codereview.chromium.org/843005/diff/73001/74011#newcode306 net/base/keygen_handler_win.cc:306: if (GetLastError() != NTE_BAD_KEYSET || On 2010/03/31 02:22:47, wtc wrote: > IMPORTANT: It seems that we should retry with a new key id if > the first CryptAcquireContext call succeeds, otherwise we'll > overwrite the existing keypair in the key container, right? Yeah, I was thinking about that last night. From an implementation side, GetNewKeyContainerId is using a GUID, so it would seem the collision probability is on an order of nanoseconds if I remember the specification for MSFT GUID's time correctly, but that still leaves a certain window. An alternate implementation I would think would be to loop through several random key container IDs until one succeeded or the sanity check exhausted. Does that implementation, with a check of say, 5 iterations, work for you? http://codereview.chromium.org/843005/diff/73001/74011#newcode316 net/base/keygen_handler_win.cc:316: if (!CryptGenKey(prov, CALG_RSA_KEYX, flags, &key)) { On 2010/03/31 02:22:47, wtc wrote: > Nit: move the declaration of |flags| here. > > Nit: CALG_RSA_KEYX => CALG_RSA_SIGN ? The declaration was to prevent the goto on 312 from potentially skipping the initialization. http://codereview.chromium.org/843005/diff/73001/74011#newcode342 net/base/keygen_handler_win.cc:342: StoreKeyInCache(prov); On 2010/03/31 02:22:47, wtc wrote: > Move the StoreKeyInCache call to line 333 above, right > before the |failure:| label. See the response to the NSS comments of the same nature - I'm not sure I understand this one.
Ryan, Here is my response to your last reply. Let's try to wrap this up today because I need some time to prepare it for checkin. I can make any remaining changes if you don't have time. Thanks! 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; My experience with dwKeySpec is that it is merely a way to identify one of the two key pairs in a key container. It is up to the user to decide what operations he will use a key pair for. CryptoAPI doesn't care. I found that client certificates issued by real CAs have both "Digital Signature, Key Encipherment (a0)" in the Key Usage extension, so it is not a stretch to call them AT_KEYEXCHANGE, even though in SSL client auth we use them to sign only. <keygen> is not used for server certificate enrollment. In the interest of wrapping up this changelist, I won't insist on changing dwKeySpec to AT_SIGNATURE. 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; 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. 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, Thanks for the explanation. I'm still not sure if I fully understand it. Now I know this CryptAcquireCertificatePrivateKey call is a shortcut and is optional. But I still don't know why |cert| may have been built from an existing OS handle. It seems that you're trying to handle a rare corner case. If so, I'd say it's not worth optimizing. In any case, please update the comment with your clarification. http://codereview.chromium.org/843005/diff/73001/74005#newcode134 net/base/cert_database_win.cc:134: HCERTSTORE cert_db = CertOpenStore(CERT_STORE_PROV_SYSTEM, 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. http://codereview.chromium.org/843005/diff/73001/74005#newcode152 net/base/cert_database_win.cc:152: void CertDatabase::Init() { Yes, you can define the constructor body in the header file. http://codereview.chromium.org/843005/diff/73001/74009 File net/base/keygen_handler_nss.cc (right): http://codereview.chromium.org/843005/diff/73001/74009#newcode213 net/base/keygen_handler_nss.cc:213: SECKEY_DestroyPrivateKey(privateKey); BUG: I believe (ignoring your new cache->Insert code) this should be: if (!isSuccess || !stores_key_) PK11_DestroyTokenObject(privateKey->pkcs11Slot, privateKey->pkcs11ID); SECKEY_DestroyPrivateKey(privateKey); The key handle should always be closed, and the underlying key should be deleted if !isSuccess || !stores_key_. http://codereview.chromium.org/843005/diff/73001/74009#newcode217 net/base/keygen_handler_nss.cc:217: Cache* cache = KeyCache::GetInstance(); stores_key_ is set to false only in some unit tests, to avoid leaving behind keys after running the tests. To answer your question, we need to study those unit tests and see whether inserting the key location into the cache matters to the tests. My suggestion of moving the block of code up, to the place where the entire keygen sequence has succeeded, is to make the code easier to understand. http://codereview.chromium.org/843005/diff/73001/74011 File net/base/keygen_handler_win.cc (right): http://codereview.chromium.org/843005/diff/73001/74011#newcode306 net/base/keygen_handler_win.cc:306: if (GetLastError() != NTE_BAD_KEYSET || On 2010/03/31 06:53:36, rsleevi wrote: > > An alternate implementation I would think would be to loop through several > random key container IDs until one succeeded or the sanity check exhausted. Does > that implementation, with a check of say, 5 iterations, work for you? Yes.
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
