Mac: implement <keygen> support, including adding generated cert to the Keychain.

Mac: implement <keygen> support, including adding generated cert to the Keychain.
BUG=34607
TEST=KeygenHandlerTest.SmokeTest

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

[Jens Alfke, 2010-02-23 20:09:58]

Important note: I can't actually commit this until I get legal approval for the
newly added 3rd party Apple code (which uses a different license, the APSL.)
This is pending.

[Jens Alfke, 2010-02-23 22:28:03]

I removed the Apple code (which was really just four struct declarations).

[wtc, 2010-02-25 00:53:34]

LGTM.  Please note the important issues below, marked with
"BUG", "UI DESIGN ISSUE", and "API DESIGN ISSUE".  Also I'm
having second thoughts about the change to
X509Certificate::CreateFromBytes().  I know you asked me
before you made the change.  Sorry about that.

You can address some issues in a follow-up CL or just add
TODO comments for them.

Thank you for writing this CL!

http://codereview.chromium.org/652137/diff/2001/2002
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/652137/diff/2001/2002#newcode2743
chrome/app/generated_resources.grd:2743: <message name="IDS_ADD_CERT_DIALOG_ADD"
desc="Title of the button that adds the certificate in the Add Client
Certificate dialog">
UI DESIGN ISSUE: We should not ask the user to approve the
addition of the client cert.

Most users aren't capable of examinig the generated cert
and making this decision.

Moreover, if the user clicks the wrong button by mistake,
the generated certificate will be wasted, and the key
store/keychain will have an orphaned key pair.

We just need a message dialog that tells the user the cert
has been added successfully, or we could not store the cert.

http://codereview.chromium.org/652137/diff/2001/2002#newcode2752
chrome/app/generated_resources.grd:2752: <message name="IDS_ADD_CERT_ERR_FAILED"
desc="Generic error message for a failure to add the generated certificate to
the database/keychain">
Nit: database => certificate store

http://codereview.chromium.org/652137/diff/2001/2003
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/652137/diff/2001/2003#newcode985
chrome/browser/renderer_host/resource_dispatcher_host.cc:985: void
ResourceDispatcherHost::OnClientCertificateGenerated(
The test shell (something like SimpleResourceLoaderBridge)
should probably also implement this method.

http://codereview.chromium.org/652137/diff/2001/2005
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/652137/diff/2001/2005#newcode1336
chrome/browser/renderer_host/resource_message_filter.cc:1336: key_size_in_bits =
2048;
BUG: missing "break" statement for case 0 and case 1.

How can keygen possibly work with this bug?!

http://codereview.chromium.org/652137/diff/2001/2006
File chrome/browser/renderer_host/x509_user_cert_resource_handler.cc (right):

http://codereview.chromium.org/652137/diff/2001/2006#newcode87
chrome/browser/renderer_host/x509_user_cert_resource_handler.cc:87:
net::X509Certificate::CreateFromBytes(resource_buffer_->data(),
On second thought, I think it's better to do the base64
decoding here, and then pass the decoded binary bytes to
net::X509Certificate::CreateFromBytes, so that
net::X509Certificate::CreateFromBytes only needs to support
binary input.

http://codereview.chromium.org/652137/diff/2001/2006#newcode89
chrome/browser/renderer_host/x509_user_cert_resource_handler.cc:89:
request_->delegate()->OnClientCertificateGenerated(request_, cert);
API DESIGN ISSUE: why do you want to make this a
URLRequest::Delegate method?

http://codereview.chromium.org/652137/diff/2001/2007
File chrome/browser/ssl/ssl_add_cert_handler.cc (right):

http://codereview.chromium.org/652137/diff/2001/2007#newcode31
chrome/browser/ssl/ssl_add_cert_handler.cc:31: {
Is this block (curly braces) necessary?

http://codereview.chromium.org/652137/diff/2001/2007#newcode35
chrome/browser/ssl/ssl_add_cert_handler.cc:35: if (cert_error) {
Nit: cert_error != OK

http://codereview.chromium.org/652137/diff/2001/2007#newcode37
chrome/browser/ssl/ssl_add_cert_handler.cc:37:
ShowError(l10n_util::GetStringUTF16(IDS_ADD_CERT_ERR_INVALID_CERT));
IDS_ADD_CERT_ERR_INVALID_CERT is not the right error message
for this error condition.

http://codereview.chromium.org/652137/diff/2001/2007#newcode44
chrome/browser/ssl/ssl_add_cert_handler.cc:44: #if !defined (OS_MACOSX)
Nit: remove the space after '!defined'.

Actually, should we move this stub implementation to
new ssl_add_cert_handler_win.cc and
ssl_add_cert_handler_gtk.cc files?

http://codereview.chromium.org/652137/diff/2001/2007#newcode46
chrome/browser/ssl/ssl_add_cert_handler.cc:46: // TODO(somebody): Add Windows
and GTK implementations with UI.
Nit: I believe the style guide says TODO(wtc) doesn't mean
wtc signed up to do the work.  So you can just put your userid
in it.

http://codereview.chromium.org/652137/diff/2001/2008
File chrome/browser/ssl/ssl_add_cert_handler.h (right):

http://codereview.chromium.org/652137/diff/2001/2008#newcode28
chrome/browser/ssl/ssl_add_cert_handler.h:28: // If |addCert| is true, the cert
will be added to the CertDatabase.
Nit: the argument name 'addCert' doesn't conform to the
style guide.

http://codereview.chromium.org/652137/diff/2001/2008#newcode42
chrome/browser/ssl/ssl_add_cert_handler.h:42: void ShowError(string16 error);
Nit: the input argument should be const string16&.

http://codereview.chromium.org/652137/diff/2001/2009
File chrome/browser/ssl/ssl_add_cert_handler_mac.mm (right):

http://codereview.chromium.org/652137/diff/2001/2009#newcode68
chrome/browser/ssl/ssl_add_cert_handler_mac.mm:68: [panel
setDefaultButtonTitle:l10n_util::GetNSString(IDS_ADD_CERT_DIALOG_ADD)];
Some of the lines in this file look longer than 80 characters.

http://codereview.chromium.org/652137/diff/2001/2009#newcode74
chrome/browser/ssl/ssl_add_cert_handler_mac.mm:74: // Open the cert panel as a
sheet on the browser window.
I can't figure out the indentation rules for lines 76-80
below, so I'm not sure if any of those lines is incorrectly
indented.

http://codereview.chromium.org/652137/diff/2001/2011
File net/base/cert_database.h (right):

http://codereview.chromium.org/652137/diff/2001/2011#newcode27
net/base/cert_database.h:27: // Store user (client) certificate. Assumes
CheckUserCert has already passed.
Alteratively, we can have AddUserCert call CheckUserCert,
and make CheckUserCert a private method.

http://codereview.chromium.org/652137/diff/2001/2011#newcode28
net/base/cert_database.h:28: // Returns OK or a network error code such as
ERR_FAILED.
We should avoid returning ERR_FAILED because it doesn't tell
us why the function failed.

If ERR_FAILED is the only possible error code, this function
should just return bool.

http://codereview.chromium.org/652137/diff/2001/2012
File net/base/cert_database_mac.cc (right):

http://codereview.chromium.org/652137/diff/2001/2012#newcode22
net/base/cert_database_mac.cc:22: return ERR_CERT_INVALID;
Note: my comments on this function also apply to
the same function in cert_database_nss.cc.

This should be ERR_INVALID_ARGUMENT.

http://codereview.chromium.org/652137/diff/2001/2012#newcode26
net/base/cert_database_mac.cc:26: return ERR_CERT_CONTAINS_ERRORS;
Let's use ERR_CERT_INVALID instead.  I'm trying to avoid
using ERR_CERT_CONTAINS_ERRORS.  Let's add a comment to
net_error_list.h to note this.

Background: we got the ERR_CERT_CONTAINS_ERRORS and
ERR_CERT_INVALID from WinInet, but I never knew how they're
different, so I'm now using only ERR_CERT_INVALID.

http://codereview.chromium.org/652137/diff/2001/2012#newcode34
net/base/cert_database_mac.cc:34: return ERR_CERT_CONTAINS_ERRORS;
We should add a new error code for this condition (there is
no matching private key for the certificate).

http://codereview.chromium.org/652137/diff/2001/2013
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/652137/diff/2001/2013#newcode47
net/base/cert_database_nss.cc:47: // TODO(gauravsh): We also need to make sure
another certificate
We probably should remove this TODO comment.  It's possible
to "renew" a certificate without changing the key pair.
(I don't think <keygen> supports this though.)

http://codereview.chromium.org/652137/diff/2001/2014
File net/base/cert_database_win.cc (right):

http://codereview.chromium.org/652137/diff/2001/2014#newcode18
net/base/cert_database_win.cc:18: return ERR_FAILED;
Return ERR_NOT_IMPLEMENTED here and also in AddUserCert below.

http://codereview.chromium.org/652137/diff/2001/2015
File net/base/keygen_handler.h (right):

http://codereview.chromium.org/652137/diff/2001/2015#newcode14
net/base/keygen_handler.h:14: //
<https://developer.mozilla.org/En/HTML/HTML_Extensions/KEYGEN_Tag>
The <kengen> tag also made it into the HTML 5 spec, in spite
of criticisms that <kengen> sucks:

http://dev.w3.org/html5/spec/Overview.html#the-keygen-element

Please also add the HTML 5 URL here.

http://codereview.chromium.org/652137/diff/2001/2015#newcode20
net/base/keygen_handler.h:20: inline KeygenHandler(int key_size_in_bits, const
std::string& challenge);
Is the "inline" keyword necessary?  It's uncommon in our code
base.

http://codereview.chromium.org/652137/diff/2001/2015#newcode27
net/base/keygen_handler.h:27: void setStoreKey(bool store) { store_key_ =
store;}
Nit: use all lowercase set_store_key name, which is the
prevalent convention for setters in the net module.

http://codereview.chromium.org/652137/diff/2001/2016
File net/base/keygen_handler_mac.cc (right):

http://codereview.chromium.org/652137/diff/2001/2016#newcode83
net/base/keygen_handler_mac.cc:83: static OSStatus createRSAKeyPair(int
size_in_bits,
These two function names should follow our naming convention,
capitalized.

http://codereview.chromium.org/652137/diff/2001/2016#newcode111
net/base/keygen_handler_mac.cc:111: scoped_cftyperef<CFDataRef>
scoped_publicKeyData(key_data);
Nit: the variable name scoped_publicKeyData does not conform
to our style guide.

http://codereview.chromium.org/652137/diff/2001/2016#newcode119
net/base/keygen_handler_mac.cc:119: SignedPublicKeyAndChallenge spkac = {
Nit: This initializer is hard to understand.  It may be better
to use assignment statements for the struct members instead.

http://codereview.chromium.org/652137/diff/2001/2016#newcode132
net/base/keygen_handler_mac.cc:132:
reinterpret_cast<uint8_t*>(const_cast<char*>(challenge_.c_str()))
Nit: you may be able to use challenge_.data() instead here,
if the string doesn't need to be null-terminated.

http://codereview.chromium.org/652137/diff/2001/2016#newcode163
net/base/keygen_handler_mac.cc:163: err = -1;
Is there some Mac error code that we can use instead of -1?

http://codereview.chromium.org/652137/diff/2001/2016#newcode182
net/base/keygen_handler_mac.cc:182: free(signature.Data);
Is 'free' the right function to free the result of
CSSM_SignData?

The documentation says CSSM_SignData uses "the
application's declared memory allocation functions".
Are malloc and free the default?

http://codereview.chromium.org/652137/diff/2001/2016#newcode209
net/base/keygen_handler_mac.cc:209: CSSM_KEYUSE_ANY,                            
    // private key
Why don't we use matching key uses for private key?
CSSM_KEYUSE_DECRYPT | CSSM_KEYUSE_SIGN | CSSM_KEYUSE_UNWRAP

http://codereview.chromium.org/652137/diff/2001/2016#newcode219
net/base/keygen_handler_mac.cc:219: CSSM_RETURN err;
Nit: Since 'err' receives the return value of SecKeyXXX
functions, shouldn't 'err' be OSStatus?

http://codereview.chromium.org/652137/diff/2001/2017
File net/base/keygen_handler_nss.cc (right):

http://codereview.chromium.org/652137/diff/2001/2017#newcode190
net/base/keygen_handler_nss.cc:190: std::string
input(reinterpret_cast<char*>(signedItem.data), signedItem.len);
Nit: consider moving the declaration of 'input' to the
beginning of this function (before any 'goto' statement) to
avoid this block (the curly braces).  (This function was
written in the C style.)

I guess you don't want to waste a default std::string
constructor call?

By the way, I verified that NSSBase64_EncodeItem uses a
line length of 64 characters.

http://codereview.chromium.org/652137/diff/2001/2018
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/652137/diff/2001/2018#newcode5
net/base/keygen_handler_unittest.cc:5: #include "net/base/keygen_handler.h"
Nit: add a blank line.

http://codereview.chromium.org/652137/diff/2001/2018#newcode8
net/base/keygen_handler_unittest.cc:8: #include "base/base64.h"
Nit: prune the headers.  This file isn't using all of them.

http://codereview.chromium.org/652137/diff/2001/2018#newcode32
net/base/keygen_handler_unittest.cc:32: LOG(INFO) << "KeygenHandler produced: <"
<< result << ">";
Can we remove "<" and ">" from the output?  They look like
they're part of the result (which is base64 encoded).

http://codereview.chromium.org/652137/diff/2001/2018#newcode65
net/base/keygen_handler_unittest.cc:65: }  // namespace
Nit: add a blank line.

http://codereview.chromium.org/652137/diff/2001/2020
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/652137/diff/2001/2020#newcode229
net/base/x509_certificate.h:229: bool SupportsSSLClientAuth();
Remove x509_certificate.h and x509_certificate_mac.cc
from this CL.  They are part of another CL
(http://codereview.chromium.org/651090).

http://codereview.chromium.org/652137/diff/2001/2022
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/652137/diff/2001/2022#newcode625
net/base/x509_certificate_nss.cc:625: CERTCertificate* cert =
CERT_DecodeCertFromPackage(data_copy, length);
This change is fine if we allow CreateOSCertHandleFromBytes
to take base64-encoded input.

The problem with using CERT_DecodeCertFromPackage is that it
is extremely flexible.  I am worried that it may be more
flexible than the Mac implementation, which will cause
platform-specific bugs (some inputs are accepted on Linux
but not on Mac).

http://codereview.chromium.org/652137/diff/2001/2022#newcode628
net/base/x509_certificate_nss.cc:628: LOG(ERROR) << "Couldn't parse a
certificate from " << length << " bytes";
Delete this log message.

http://codereview.chromium.org/652137/diff/2001/2024
File net/url_request/url_request.h (right):

http://codereview.chromium.org/652137/diff/2001/2024#newcode172
net/url_request/url_request.h:172: virtual void OnCertificateRequested(
We should rename this method OnClientCertificateRequested
to be consistent with the new method's name.  (rvargas
requested this renaming when I added OnCertificateRequested
but I haven't got around to doing that.)

You don't need to make this change.  Adding a TODO comment
would be nice.

[Jens Alfke, 2010-02-26 19:13:40]

Thanks for the feedback. It probably wasn't necessary for me to add a URLRequest
delegate method; shall I simplify the code to have the user cert resource
handler just directly invoke the add-cert handler?

http://codereview.chromium.org/652137/diff/2001/2005
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/652137/diff/2001/2005#newcode1336
chrome/browser/renderer_host/resource_message_filter.cc:1336: key_size_in_bits =
2048;
On 2010/02/25 00:53:35, wtc wrote:
> BUG: missing "break" statement for case 0 and case 1.
> How can keygen possibly work with this bug?!

Aw, crap! This was the last change I made before uploading, and I didn't realize
that it wouldn't break the unit test (which doesn't go through HTTP), so I
didn't do the whole manual test in the browser. Sorry about that.

http://codereview.chromium.org/652137/diff/2001/2006
File chrome/browser/renderer_host/x509_user_cert_resource_handler.cc (right):

http://codereview.chromium.org/652137/diff/2001/2006#newcode87
chrome/browser/renderer_host/x509_user_cert_resource_handler.cc:87:
net::X509Certificate::CreateFromBytes(resource_buffer_->data(),
On 2010/02/25 00:53:35, wtc wrote:
> On second thought, I think it's better to do the base64
> decoding here, and then pass the decoded binary bytes to
> net::X509Certificate::CreateFromBytes, so that
> net::X509Certificate::CreateFromBytes only needs to support
> binary input.

This HTTP response isn't base64-encoded; it's the exact cert data in MIME type
"application/x-x509-user-cert".

http://codereview.chromium.org/652137/diff/2001/2006#newcode89
chrome/browser/renderer_host/x509_user_cert_resource_handler.cc:89:
request_->delegate()->OnClientCertificateGenerated(request_, cert);
On 2010/02/25 00:53:35, wtc wrote:
> API DESIGN ISSUE: why do you want to make this a
> URLRequest::Delegate method?

I think I was just following the way client auth is handled, through an
OnCertificateRequested delegate method. But I don't know the Chrome APIs well,
so maybe it would be better to have this method just call SSLAddCertHandler
directly. Do you agree?

http://codereview.chromium.org/652137/diff/2001/2007
File chrome/browser/ssl/ssl_add_cert_handler.cc (right):

http://codereview.chromium.org/652137/diff/2001/2007#newcode31
chrome/browser/ssl/ssl_add_cert_handler.cc:31: {
On 2010/02/25 00:53:35, wtc wrote:
> Is this block (curly braces) necessary?

I just wanted the temporary CertDatabase object to get cleaned up ASAP, before
the potentially-lengthy calls to ShowError or AskToAddCert. Probably not an
issue in our implementations of CertDatabase, but it was just two curly braces.

http://codereview.chromium.org/652137/diff/2001/2007#newcode35
chrome/browser/ssl/ssl_add_cert_handler.cc:35: if (cert_error) {
On 2010/02/25 00:53:35, wtc wrote:
> Nit: cert_error != OK

Done.

http://codereview.chromium.org/652137/diff/2001/2007#newcode37
chrome/browser/ssl/ssl_add_cert_handler.cc:37:
ShowError(l10n_util::GetStringUTF16(IDS_ADD_CERT_ERR_INVALID_CERT));
On 2010/02/25 00:53:35, wtc wrote:
> IDS_ADD_CERT_ERR_INVALID_CERT is not the right error message
> for this error condition.

?? It's a new error string I just added for this one situation. Do you mean that
the string itself should be worded differently?

http://codereview.chromium.org/652137/diff/2001/2007#newcode44
chrome/browser/ssl/ssl_add_cert_handler.cc:44: #if !defined (OS_MACOSX)
On 2010/02/25 00:53:35, wtc wrote:
> Nit: remove the space after '!defined'.

Done.

> Actually, should we move this stub implementation to
> new ssl_add_cert_handler_win.cc and
> ssl_add_cert_handler_gtk.cc files?

I'll just let other people add those files when they're implemented, instead of
cluttering up the build with two files that don't do anything yet.

http://codereview.chromium.org/652137/diff/2001/2007#newcode46
chrome/browser/ssl/ssl_add_cert_handler.cc:46: // TODO(somebody): Add Windows
and GTK implementations with UI.
On 2010/02/25 00:53:35, wtc wrote:
> Nit: I believe the style guide says TODO(wtc) doesn't mean
> wtc signed up to do the work.  So you can just put your userid
> in it.

Done.

http://codereview.chromium.org/652137/diff/2001/2008
File chrome/browser/ssl/ssl_add_cert_handler.h (right):

http://codereview.chromium.org/652137/diff/2001/2008#newcode28
chrome/browser/ssl/ssl_add_cert_handler.h:28: // If |addCert| is true, the cert
will be added to the CertDatabase.
On 2010/02/25 00:53:35, wtc wrote:
> Nit: the argument name 'addCert' doesn't conform to the
> style guide.

Done.

http://codereview.chromium.org/652137/diff/2001/2008#newcode42
chrome/browser/ssl/ssl_add_cert_handler.h:42: void ShowError(string16 error);
On 2010/02/25 00:53:35, wtc wrote:
> Nit: the input argument should be const string16&.

Done.

http://codereview.chromium.org/652137/diff/2001/2009
File chrome/browser/ssl/ssl_add_cert_handler_mac.mm (right):

http://codereview.chromium.org/652137/diff/2001/2009#newcode68
chrome/browser/ssl/ssl_add_cert_handler_mac.mm:68: [panel
setDefaultButtonTitle:l10n_util::GetNSString(IDS_ADD_CERT_DIALOG_ADD)];
On 2010/02/25 00:53:35, wtc wrote:
> Some of the lines in this file look longer than 80 characters.

Nope; the two longest lines are exactly 80.

http://codereview.chromium.org/652137/diff/2001/2009#newcode74
chrome/browser/ssl/ssl_add_cert_handler_mac.mm:74: // Open the cert panel as a
sheet on the browser window.
On 2010/02/25 00:53:35, wtc wrote:
> I can't figure out the indentation rules for lines 76-80
> below, so I'm not sure if any of those lines is incorrectly
> indented.

In Objective-C you line up the ":"s on message-send keywords.
It's easier to see if there's a space after the ":", but for some reason our
style guide doesn't allow that...

http://codereview.chromium.org/652137/diff/2001/2011
File net/base/cert_database.h (right):

http://codereview.chromium.org/652137/diff/2001/2011#newcode27
net/base/cert_database.h:27: // Store user (client) certificate. Assumes
CheckUserCert has already passed.
On 2010/02/25 00:53:35, wtc wrote:
> Alteratively, we can have AddUserCert call CheckUserCert,
> and make CheckUserCert a private method.

No, I broke it out this way so the UI can call this first and put up an error
before asking the user if s/he wants to add the cert. Otherwise you'd have to
hit OK first and then get an error message.

http://codereview.chromium.org/652137/diff/2001/2011#newcode28
net/base/cert_database.h:28: // Returns OK or a network error code such as
ERR_FAILED.
On 2010/02/25 00:53:35, wtc wrote:
> We should avoid returning ERR_FAILED because it doesn't tell
> us why the function failed.

It's functionally equivalent to the bool that this used to return, but it leaves
open future improvement that returns more specific codes. I dislike APIs that
have multiple failure modes but return only a true/false flag.

> If ERR_FAILED is the only possible error code, this function
> should just return bool.

The implementations may only return one code so far, but the API should support
returning others.

http://codereview.chromium.org/652137/diff/2001/2012
File net/base/cert_database_mac.cc (right):

http://codereview.chromium.org/652137/diff/2001/2012#newcode22
net/base/cert_database_mac.cc:22: return ERR_CERT_INVALID;
On 2010/02/25 00:53:35, wtc wrote:
> Note: my comments on this function also apply to
> the same function in cert_database_nss.cc.
> 
> This should be ERR_INVALID_ARGUMENT.

No, because this happens when the server response fails to parse at all. The
AddCertHandler gets called, because we need UI, but it has to be passed NULL
because there is no X509Certificate object.

http://codereview.chromium.org/652137/diff/2001/2012#newcode26
net/base/cert_database_mac.cc:26: return ERR_CERT_CONTAINS_ERRORS;
On 2010/02/25 00:53:35, wtc wrote:
> Let's use ERR_CERT_INVALID instead.  I'm trying to avoid
> using ERR_CERT_CONTAINS_ERRORS.  Let's add a comment to
> net_error_list.h to note this.
> 
> Background: we got the ERR_CERT_CONTAINS_ERRORS and
> ERR_CERT_INVALID from WinInet, but I never knew how they're
> different, so I'm now using only ERR_CERT_INVALID.

Done.

http://codereview.chromium.org/652137/diff/2001/2012#newcode34
net/base/cert_database_mac.cc:34: return ERR_CERT_CONTAINS_ERRORS;
On 2010/02/25 00:53:35, wtc wrote:
> We should add a new error code for this condition (there is
> no matching private key for the certificate).

OK — I've added ERR_CERT_NO_PRIVATE_KEY.

http://codereview.chromium.org/652137/diff/2001/2013
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/652137/diff/2001/2013#newcode47
net/base/cert_database_nss.cc:47: // TODO(gauravsh): We also need to make sure
another certificate
On 2010/02/25 00:53:35, wtc wrote:
> We probably should remove this TODO comment.  It's possible
> to "renew" a certificate without changing the key pair.
> (I don't think <keygen> supports this though.)

Done.

http://codereview.chromium.org/652137/diff/2001/2014
File net/base/cert_database_win.cc (right):

http://codereview.chromium.org/652137/diff/2001/2014#newcode18
net/base/cert_database_win.cc:18: return ERR_FAILED;
On 2010/02/25 00:53:35, wtc wrote:
> Return ERR_NOT_IMPLEMENTED here and also in AddUserCert below.

Done.

http://codereview.chromium.org/652137/diff/2001/2015
File net/base/keygen_handler.h (right):

http://codereview.chromium.org/652137/diff/2001/2015#newcode14
net/base/keygen_handler.h:14: //
<https://developer.mozilla.org/En/HTML/HTML_Extensions/KEYGEN_Tag>
On 2010/02/25 00:53:35, wtc wrote:
> The <kengen> tag also made it into the HTML 5 spec, in spite
> of criticisms that <kengen> sucks:
> 
> http://dev.w3.org/html5/spec/Overview.html#the-keygen-element
> 
> Please also add the HTML 5 URL here.

Done.

http://codereview.chromium.org/652137/diff/2001/2015#newcode20
net/base/keygen_handler.h:20: inline KeygenHandler(int key_size_in_bits, const
std::string& challenge);
On 2010/02/25 00:53:35, wtc wrote:
> Is the "inline" keyword necessary?  It's uncommon in our code
> base.

It's necessary when the method implementation is external to the class
declaration, as this one is (I put it down below because it's multiple lines.)

http://codereview.chromium.org/652137/diff/2001/2015#newcode27
net/base/keygen_handler.h:27: void setStoreKey(bool store) { store_key_ =
store;}
On 2010/02/25 00:53:35, wtc wrote:
> Nit: use all lowercase set_store_key name, which is the
> prevalent convention for setters in the net module.

Done.

http://codereview.chromium.org/652137/diff/2001/2016
File net/base/keygen_handler_mac.cc (right):

http://codereview.chromium.org/652137/diff/2001/2016#newcode83
net/base/keygen_handler_mac.cc:83: static OSStatus createRSAKeyPair(int
size_in_bits,
On 2010/02/25 00:53:35, wtc wrote:
> These two function names should follow our naming convention,
> capitalized.

Done.

http://codereview.chromium.org/652137/diff/2001/2016#newcode111
net/base/keygen_handler_mac.cc:111: scoped_cftyperef<CFDataRef>
scoped_publicKeyData(key_data);
On 2010/02/25 00:53:35, wtc wrote:
> Nit: the variable name scoped_publicKeyData does not conform
> to our style guide.

Done.

http://codereview.chromium.org/652137/diff/2001/2016#newcode119
net/base/keygen_handler_mac.cc:119: SignedPublicKeyAndChallenge spkac = {
On 2010/02/25 00:53:35, wtc wrote:
> Nit: This initializer is hard to understand.  It may be better
> to use assignment statements for the struct members instead.

Done.

http://codereview.chromium.org/652137/diff/2001/2016#newcode132
net/base/keygen_handler_mac.cc:132:
reinterpret_cast<uint8_t*>(const_cast<char*>(challenge_.c_str()))
On 2010/02/25 00:53:35, wtc wrote:
> Nit: you may be able to use challenge_.data() instead here,
> if the string doesn't need to be null-terminated.

Done.

http://codereview.chromium.org/652137/diff/2001/2016#newcode163
net/base/keygen_handler_mac.cc:163: err = -1;
On 2010/02/25 00:53:35, wtc wrote:
> Is there some Mac error code that we can use instead of -1?

I looked at the implementation and it's actually impossible for it to fail
(modp_base64_encode has no failure condition, it always returns >= 0.) So I've
taken out the test.

http://codereview.chromium.org/652137/diff/2001/2016#newcode182
net/base/keygen_handler_mac.cc:182: free(signature.Data);
On 2010/02/25 00:53:35, wtc wrote:
> The documentation says CSSM_SignData uses "the
> application's declared memory allocation functions".
> Are malloc and free the default?

All the CSSM sample code I've seen just uses free() on CSSM_DATA. There isn't a
specific CSSM API call to free it.

http://codereview.chromium.org/652137/diff/2001/2016#newcode209
net/base/keygen_handler_mac.cc:209: CSSM_KEYUSE_ANY,                            
    // private key
On 2010/02/25 00:53:35, wtc wrote:
> Why don't we use matching key uses for private key?
> CSSM_KEYUSE_DECRYPT | CSSM_KEYUSE_SIGN | CSSM_KEYUSE_UNWRAP

Good point. I copied this code from an earlier project of mine, but I don't
remember why I used those flags. I'll try changing them and see if everything
still works.

http://codereview.chromium.org/652137/diff/2001/2016#newcode219
net/base/keygen_handler_mac.cc:219: CSSM_RETURN err;
On 2010/02/25 00:53:35, wtc wrote:
> Nit: Since 'err' receives the return value of SecKeyXXX
> functions, shouldn't 'err' be OSStatus?

Done.

http://codereview.chromium.org/652137/diff/2001/2017
File net/base/keygen_handler_nss.cc (right):

http://codereview.chromium.org/652137/diff/2001/2017#newcode190
net/base/keygen_handler_nss.cc:190: std::string
input(reinterpret_cast<char*>(signedItem.data), signedItem.len);
On 2010/02/25 00:53:35, wtc wrote:
> Nit: consider moving the declaration of 'input' to the
> beginning of this function (before any 'goto' statement) to
> avoid this block (the curly braces).  (This function was
> written in the C style.)
> 
> I guess you don't want to waste a default std::string
> constructor call?
> 
> By the way, I verified that NSSBase64_EncodeItem uses a
> line length of 64 characters.

OK. I just moved the string constructor into the Encode() call so there's no
local variable at all.

http://codereview.chromium.org/652137/diff/2001/2018
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/652137/diff/2001/2018#newcode8
net/base/keygen_handler_unittest.cc:8: #include "base/base64.h"
On 2010/02/25 00:53:35, wtc wrote:
> Nit: prune the headers.  This file isn't using all of them.

Done.

http://codereview.chromium.org/652137/diff/2001/2018#newcode32
net/base/keygen_handler_unittest.cc:32: LOG(INFO) << "KeygenHandler produced: <"
<< result << ">";
On 2010/02/25 00:53:35, wtc wrote:
> Can we remove "<" and ">" from the output?  They look like
> they're part of the result (which is base64 encoded).

Done.

[wtc, 2010-03-01 21:52:34]

Hi Jens,

I responded to your comments below.  I'll review the new
Patch Set 4 later.

(In the future, please don't use "Done" on my suggested
changes, especially if I suggested a lot of changes.  This
will make it easier for me to go through your responses.
Thanks!)

http://codereview.chromium.org/652137/diff/2001/2006
File chrome/browser/renderer_host/x509_user_cert_resource_handler.cc (right):

http://codereview.chromium.org/652137/diff/2001/2006#newcode87
chrome/browser/renderer_host/x509_user_cert_resource_handler.cc:87:
net::X509Certificate::CreateFromBytes(resource_buffer_->data(),
On 2010/02/26 19:13:40, Jens Alfke wrote:
>
> This HTTP response isn't base64-encoded; it's the exact cert data in MIME type
> "application/x-x509-user-cert".

Do you know if application/x-x509-user-cert may contain
the intermediate CA certs?  The web page
https://developer.mozilla.org/En/HTML/HTML_Extensions/KEYGEN_Tag
doesn't specify the format of the response.  Presumably
that's outside the scope of the keygen tag.

I found documentation of application/x-x509-user-cert here:
http://www.redhat.com/docs/manuals/cert-system/admin/app_dwnld.htm
which implies that there may be intermediate CA certs
following the user cert.

CERT_DecodeCertFromPackage handles an input consisting
of multiple certs, but it will only import the first cert.
So the intermediate CA certs, if any, will be ignored.

We should file a bug report to look into this issue.

http://codereview.chromium.org/652137/diff/2001/2006#newcode89
chrome/browser/renderer_host/x509_user_cert_resource_handler.cc:89:
request_->delegate()->OnClientCertificateGenerated(request_, cert);
On 2010/02/26 19:13:40, Jens Alfke wrote:
>
> I think I was just following the way client auth is handled, through an
> OnCertificateRequested delegate method. But I don't know the Chrome APIs well,
> so maybe it would be better to have this method just call SSLAddCertHandler
> directly. Do you agree?

Yes, I agree.  The reason client auth must be handled as
a URLRequest::Delegate method is that URLRequest itself
(in url_request_http_job.cc) needs to call this method to
handle a client cert request.  This is not the case here
(this is not url_request.cc or url_request_xxx_job.cc), so
we can just call SSLAddCertHandler directly.

http://codereview.chromium.org/652137/diff/2001/2007
File chrome/browser/ssl/ssl_add_cert_handler.cc (right):

http://codereview.chromium.org/652137/diff/2001/2007#newcode31
chrome/browser/ssl/ssl_add_cert_handler.cc:31: {
On 2010/02/26 19:13:40, Jens Alfke wrote:
> 
> I just wanted the temporary CertDatabase object to get cleaned up ASAP, before
> the potentially-lengthy calls to ShowError or AskToAddCert. Probably not an
> issue in our implementations of CertDatabase, but it was just two curly
braces.

I see. This is fine.  Perhaps we should add a
net::CertDatabase::Close method to make this intention
obvious.

http://codereview.chromium.org/652137/diff/2001/2007#newcode37
chrome/browser/ssl/ssl_add_cert_handler.cc:37:
ShowError(l10n_util::GetStringUTF16(IDS_ADD_CERT_ERR_INVALID_CERT));
On 2010/02/26 19:13:40, Jens Alfke wrote:
>
> ?? It's a new error string I just added for this one situation. Do you mean
that
> the string itself should be worded differently?

OK, then it's fine.  (I may have mistaken this for an
existing error message for ERR_CERT_INVALID.)

http://codereview.chromium.org/652137/diff/2001/2011
File net/base/cert_database.h (right):

http://codereview.chromium.org/652137/diff/2001/2011#newcode28
net/base/cert_database.h:28: // Returns OK or a network error code such as
ERR_FAILED.
On 2010/02/26 19:13:40, Jens Alfke wrote:
>
> The implementations may only return one code so far, but the API should
support
> returning others.

In that case, please return a more specific error code.
If none of the existing error codes is suitable, add a
new one.  In the worst case, you can always add
ERR_ADD_CLIENT_CERT_FAILED.

When we get a bug report with ERR_FAILED in it, we have
no idea what went wrong.  On the other hand, if the error
code is more specific, such as ERR_ADD_CLIENT_CERT_FAILED,
that narrows down the possible places of failure.

[wtc, 2010-03-01 23:05:40]

LGTM.  Here are my comments on Patch Set 4.

http://codereview.chromium.org/652137/diff/4010/4025
File net/base/keygen_handler_mac.cc (right):

http://codereview.chromium.org/652137/diff/4010/4025#newcode142
net/base/keygen_handler_mac.cc:142: spkac.signature_algorithm.algorithm =
CSSMOID_MD5WithRSA;
Can we use SHA1WithRSA instead of MD5WithRSA?  (This needs to
match what you specify on line 241 below.)

I know this comes from the NSS-based implementation and is
specified in http://dev.w3.org/html5/spec/Overview.html#the-keygen-element
but not in https://developer.mozilla.org/En/HTML/HTML_Extensions/KEYGEN_Tag

For now, let's just add a TODO comment and link to the
Mozilla bug I just filed:
https://bugzilla.mozilla.org/show_bug.cgi?id=549460

http://codereview.chromium.org/652137/diff/4010/4029
File net/base/net_error_list.h (right):

http://codereview.chromium.org/652137/diff/4010/4029#newcode194
net/base/net_error_list.h:194: // NOTE: It's unclear how this differs from
CERT_INVALID. For consistency,
Nit: CERT_INVALID => ERR_CERT_INVALID

http://codereview.chromium.org/652137/diff/4010/4029#newcode233
net/base/net_error_list.h:233: NET_ERROR(CERT_NO_PRIVATE_KEY, -209)
This is not a certificate error (the certificate itself is
good).  So we need to add this error code elsewhere.
(The error code ranges are documented at line 8.)

Let's add it to the -5xx range, and name it
NO_PRIVATE_KEY_FOR_CERT.

Please remember to change CERT_END back to -209.