Give keys friendly names in NSS and OS X

Give keys friendly names in NSS and OS X

This will make them browsing them easier in Keychain Access and certutil.
It also gives them a more useful name in Keychain permission dialogs.

The friendly name is currently just the hostname which requested
the key. It would be nice to have some surrounding text with
localization, but this works for now.

BUG=none
TEST=none

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

[davidben, 2010-07-08 18:59:41]

+wtc, since he's back from vacation now.

[agl, 2010-07-08 19:05:33]

Sorry, it appears that I missed this CL. (Please email me if I don't reply in a
couple of working days.)

http://codereview.chromium.org/2806045/diff/1/5
File net/base/keygen_handler_mac.cc (right):

http://codereview.chromium.org/2806045/diff/1/5#newcode113
net/base/keygen_handler_mac.cc:113: std::string label = StringPrintf("Key
generated for %s",
i18n? However, I don't think i18n belongs in net/, suggesting that only the URL
should be stored here and the human readable wrapping should be added at display
time.

http://codereview.chromium.org/2806045/diff/1/7
File net/third_party/mozilla_security_manager/nsKeygenHandler.cpp (right):

http://codereview.chromium.org/2806045/diff/1/7#newcode180
net/third_party/mozilla_security_manager/nsKeygenHandler.cpp:180: std::string
label = StringPrintf("Key generated for %s",
ditto

[davidben (use chromium.org), 2010-07-08 19:13:16]

http://codereview.chromium.org/2806045/diff/1/5
File net/base/keygen_handler_mac.cc (right):

http://codereview.chromium.org/2806045/diff/1/5#newcode113
net/base/keygen_handler_mac.cc:113: std::string label = StringPrintf("Key
generated for %s",
On 2010/07/08 19:05:34, agl wrote:
> i18n? However, I don't think i18n belongs in net/, suggesting that only the
URL
> should be stored here and the human readable wrapping should be added at
display
> time.

Yeah, I'm definitely intending to store the hostname by itself later for other
purposes anyway, though it's not clear where...

The i18n thing bothered me, but I'm really not sure what to do about it, short
of adding some callback that seemed a little overkill. We don't control the
display of the key. That's Keychain's business. Perhaps KeygenHandler could be
passed a template string or so.

[agl, 2010-07-08 21:06:05]

Sorry, I didn't understand that it would be Keychain displaying these names, not
Chrome. In which case I don't have a better answer for the i18n problem, so
LGTM.

[davidben, 2010-08-04 19:28:39]

On 2010/07/08 21:06:05, agl wrote:
> Sorry, I didn't understand that it would be Keychain displaying these names,
not
> Chrome. In which case I don't have a better answer for the i18n problem, so
> LGTM.

This has been sitting around for a while. I did a quick check in Safari and it
does localize the key name when inserting it. While it doesn't rename them if
you later change languages, it'd probably be good of us not to follow suit; it
occurs to me that the key name is also displayed in a Keychain prompt (again,
not under out control), should one attempt to access a website in Safari through
a certificate and key generated by Chrome.

Having the text in English is certainly better than '<key>' as we currently have
it, but localizing it seems a good idea. I dunno. There is a TODO to localize
the strings, so I can land this as-is. We could also find some way to get that
string localized, though it might be messy. As I understand, l10n_util functions
can only be called on the UI thread, whereas this code lives entirely on the IO
thread and a worker thread.

We could also just name the key by the domain without any other words. As
fallout from the previously noted experiment, my Mac's keychain dialogs are
convinced that I'm French, but it appears that the relevant dialog /does/ say
"key" in them; Google translate says "Safari wants to sign in using the key
'$KEYNAME' from your keychain."

[davidben, 2010-08-04 19:29:38]

On 2010/08/04 19:28:39, David Benjamin wrote:
> it'd probably be good of us not to follow suit; it

Er... s/not //

[wtc, 2010-08-06 22:56:02]

LGTM.

I, too, am not sure about how to localize the friendly
names for keys.  Perhaps we just use the URL...

http://codereview.chromium.org/2806045/diff/11001/12003
File net/base/keygen_handler.h (right):

http://codereview.chromium.org/2806045/diff/11001/12003#newcode22
net/base/keygen_handler.h:22: // and incorporate the |challenge| into the
Netscape SPKAC structure.
Document what the |url| parameter is.

http://codereview.chromium.org/2806045/diff/11001/12003#newcode36
net/base/keygen_handler.h:36: GURL url_;  // the url that requested the key
Nit: url => URL

http://codereview.chromium.org/2806045/diff/11001/12004
File net/base/keygen_handler_mac.cc (right):

http://codereview.chromium.org/2806045/diff/11001/12004#newcode112
net/base/keygen_handler_mac.cc:112: // Give them friendly names.
Nit: them => the keys

http://codereview.chromium.org/2806045/diff/11001/12004#newcode114
net/base/keygen_handler_mac.cc:114: std::string host = url_.host();
Should 'host' be a reference to avoid a copy constructor
call?

http://codereview.chromium.org/2806045/diff/11001/12007
File net/third_party/mozilla_security_manager/nsKeygenHandler.cpp (right):

http://codereview.chromium.org/2806045/diff/11001/12007#newcode54
net/third_party/mozilla_security_manager/nsKeygenHandler.cpp:54: #include
"base/logging.h"
Nit: move "base/logging.h" up.

http://codereview.chromium.org/2806045/diff/11001/12007#newcode177
net/third_party/mozilla_security_manager/nsKeygenHandler.cpp:177: // Set
friendly names for the keys
Nit: add a period at end of sentence.

Merge the two blocks at lines 183-190 into one.

http://codereview.chromium.org/2806045/diff/11001/12008
File net/third_party/mozilla_security_manager/nsKeygenHandler.h (right):

http://codereview.chromium.org/2806045/diff/11001/12008#newcode56
net/third_party/mozilla_security_manager/nsKeygenHandler.h:56: //   challenge:
challenge string sent by server
Document the new 'url' parameter.

[davidben, 2010-08-09 23:42:00]

http://codereview.chromium.org/2806045/diff/11001/12003
File net/base/keygen_handler.h (right):

http://codereview.chromium.org/2806045/diff/11001/12003#newcode22
net/base/keygen_handler.h:22: // and incorporate the |challenge| into the
Netscape SPKAC structure.
On 2010/08/06 22:56:02, wtc wrote:
> Document what the |url| parameter is.

Done.

http://codereview.chromium.org/2806045/diff/11001/12003#newcode36
net/base/keygen_handler.h:36: GURL url_;  // the url that requested the key
On 2010/08/06 22:56:02, wtc wrote:
> Nit: url => URL

Done.

http://codereview.chromium.org/2806045/diff/11001/12004
File net/base/keygen_handler_mac.cc (right):

http://codereview.chromium.org/2806045/diff/11001/12004#newcode112
net/base/keygen_handler_mac.cc:112: // Give them friendly names.
On 2010/08/06 22:56:02, wtc wrote:
> Nit: them => the keys

Done.

http://codereview.chromium.org/2806045/diff/11001/12004#newcode114
net/base/keygen_handler_mac.cc:114: std::string host = url_.host();
On 2010/08/06 22:56:02, wtc wrote:
> Should 'host' be a reference to avoid a copy constructor
> call?

Done.

http://codereview.chromium.org/2806045/diff/11001/12007
File net/third_party/mozilla_security_manager/nsKeygenHandler.cpp (right):

http://codereview.chromium.org/2806045/diff/11001/12007#newcode54
net/third_party/mozilla_security_manager/nsKeygenHandler.cpp:54: #include
"base/logging.h"
On 2010/08/06 22:56:02, wtc wrote:
> Nit: move "base/logging.h" up.

Done.

http://codereview.chromium.org/2806045/diff/11001/12007#newcode177
net/third_party/mozilla_security_manager/nsKeygenHandler.cpp:177: // Set
friendly names for the keys
On 2010/08/06 22:56:02, wtc wrote:
> Nit: add a period at end of sentence.
> 
> Merge the two blocks at lines 183-190 into one.

Done.

http://codereview.chromium.org/2806045/diff/11001/12008
File net/third_party/mozilla_security_manager/nsKeygenHandler.h (right):

http://codereview.chromium.org/2806045/diff/11001/12008#newcode56
net/third_party/mozilla_security_manager/nsKeygenHandler.h:56: //   challenge:
challenge string sent by server
On 2010/08/06 22:56:02, wtc wrote:
> Document the new 'url' parameter.

Done.

[wtc, 2010-08-17 23:50:49]

LGTM.  Just one suggestion below.  Alternatively, use the
scheme://host:port part of the 'url' as the nickname.

http://codereview.chromium.org/2806045/diff/19001/20004
File net/base/keygen_handler_mac.cc (right):

http://codereview.chromium.org/2806045/diff/19001/20004#newcode115
net/base/keygen_handler_mac.cc:115: // TODO(davidben): Localize this.
Please file a bug report and cite the bug number here.

http://codereview.chromium.org/2806045/diff/19001/20007
File net/third_party/mozilla_security_manager/nsKeygenHandler.cpp (right):

http://codereview.chromium.org/2806045/diff/19001/20007#newcode180
net/third_party/mozilla_security_manager/nsKeygenHandler.cpp:180: //
TODO(davidben): Localize this.
Same here: cite the bug number.

[davidben, 2010-08-18 20:52:39]

I made it just send the URL.

Also, during testing, I had to rewrite the OS X code. It appears that there are
actually /two/ labels here, one of which is not visible from the UI but is used
in the dialogs. So now we create an initial SecAccessRef with the appropriate
descriptor argument. The other fields are bootstrapped from this one.

That code should probably get a once-over before I land this.

[wtc, 2010-08-18 22:14:26]

LGTM.  Just some nits on comments and function parameter names.

http://codereview.chromium.org/2806045/diff/30001/31003
File net/base/keygen_handler.h (right):

http://codereview.chromium.org/2806045/diff/30001/31003#newcode25
net/base/keygen_handler.h:25: inline KeygenHandler(int key_size_in_bits, const
std::string& challenge,
Nit: list one parameter on each line, given brettw's email
yesterday.

http://codereview.chromium.org/2806045/diff/30001/31004
File net/base/keygen_handler_mac.cc (right):

http://codereview.chromium.org/2806045/diff/30001/31004#newcode94
net/base/keygen_handler_mac.cc:94: SecAccessRef initial_access,
It may be better to name this parameter "initial_access_control_list",
"initial_acl", or "access_control_list".  I had no idea what
"initial access" means.

http://codereview.chromium.org/2806045/diff/30001/31004#newcode117
net/base/keygen_handler_mac.cc:117: // descriptor; it must be set on the
SecAccessRef to appear in
Nit: what is "it"?  I think you meant the "descriptor"
argument, right?

http://codereview.chromium.org/2806045/diff/30001/31004#newcode118
net/base/keygen_handler_mac.cc:118: // Keychain dialogs. Pass NULL to use the
default trusted list.
Nit: Pass NULL => Pass NULL as the second argument

You may want to replace "use the default trusted list" to
"allow only the application creating the item to access the
item without triggering confirmation dialogs".  (That's my
paraphrase of Apple's documentation of SecAccessCreate.)

ISSUE: the default ACL doesn't seem to be what we want, as
that will trigger confirmation dialogs if Safari wants to
use the private key for SSL client auth.  But I can't find
a away to give access to all applications.

http://codereview.chromium.org/2806045/diff/30001/31004#newcode120
net/base/keygen_handler_mac.cc:120: // This will also set kSecKeyPrintName.
I had no idea what kSecKeyPrintName is until I looked at
the previous version of this CL.

If kSecKeyPrintName is not well-known to Mac programmers,
I'd remove this comment.

By the way, while doing a web search for kSecKeyPrintName,
I found snej's library: http://bitbucket.org/snej/mycrypto/wiki/Home

http://codereview.chromium.org/2806045/diff/30001/31006
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/2806045/diff/30001/31006#newcode76
net/base/keygen_handler_unittest.cc:76: KeygenHandler handler(768, "some
challenge", GURL());
Should we use something like GURL("www.example.com") instead?
Just wondering...

[davidben, 2010-08-18 22:34:44]

http://codereview.chromium.org/2806045/diff/30001/31003
File net/base/keygen_handler.h (right):

http://codereview.chromium.org/2806045/diff/30001/31003#newcode25
net/base/keygen_handler.h:25: inline KeygenHandler(int key_size_in_bits, const
std::string& challenge,
On 2010/08/18 22:14:26, wtc wrote:
> Nit: list one parameter on each line, given brettw's email
> yesterday.

Done.

http://codereview.chromium.org/2806045/diff/30001/31004
File net/base/keygen_handler_mac.cc (right):

http://codereview.chromium.org/2806045/diff/30001/31004#newcode94
net/base/keygen_handler_mac.cc:94: SecAccessRef initial_access,
On 2010/08/18 22:14:26, wtc wrote:
> It may be better to name this parameter "initial_access_control_list",
> "initial_acl", or "access_control_list".  I had no idea what
> "initial access" means.

initial_access is what Apple calls the parameter (well, initialAccess). The APIs
that manipulate a SecAccessRef are all called SecAccess*, whereas the ones that
maniulate a single ACL entry are called SecACL* and SecACLRef, so initial_acl
might not be that accurate.

http://codereview.chromium.org/2806045/diff/30001/31004#newcode117
net/base/keygen_handler_mac.cc:117: // descriptor; it must be set on the
SecAccessRef to appear in
On 2010/08/18 22:14:26, wtc wrote:
> Nit: what is "it"?  I think you meant the "descriptor"
> argument, right?

Yeah. I've reworded it.

// Create an initial access object to set the SecAccessRef. This
// sets a label on the Keychain dialogs.

http://codereview.chromium.org/2806045/diff/30001/31004#newcode118
net/base/keygen_handler_mac.cc:118: // Keychain dialogs. Pass NULL to use the
default trusted list.
On 2010/08/18 22:14:26, wtc wrote:
> Nit: Pass NULL => Pass NULL as the second argument

Done.

> You may want to replace "use the default trusted list" to
> "allow only the application creating the item to access the
> item without triggering confirmation dialogs".  (That's my
> paraphrase of Apple's documentation of SecAccessCreate.)

Done.

// [...] Pass NULL as the second
// argument to use the default trusted list; only allow the
// current application to access without confirmation.

> ISSUE: the default ACL doesn't seem to be what we want, as
> that will trigger confirmation dialogs if Safari wants to
> use the private key for SSL client auth.  But I can't find
> a away to give access to all applications.

Hrm. There should be some mechanism, but this matches the current behavior.
Also, Safari's keys are generated in the same way. I believe this is also how
saved passwords work, so it's probably the expected behavior on the platform.

http://codereview.chromium.org/2806045/diff/30001/31004#newcode120
net/base/keygen_handler_mac.cc:120: // This will also set kSecKeyPrintName.
On 2010/08/18 22:14:26, wtc wrote:
> I had no idea what kSecKeyPrintName is until I looked at
> the previous version of this CL.
> 
> If kSecKeyPrintName is not well-known to Mac programmers,
> I'd remove this comment.
> 
> By the way, while doing a web search for kSecKeyPrintName,
> I found snej's library: http://bitbucket.org/snej/mycrypto/wiki/Home
> 

Done.

http://codereview.chromium.org/2806045/diff/30001/31006
File net/base/keygen_handler_unittest.cc (right):

http://codereview.chromium.org/2806045/diff/30001/31006#newcode76
net/base/keygen_handler_unittest.cc:76: KeygenHandler handler(768, "some
challenge", GURL());
On 2010/08/18 22:14:26, wtc wrote:
> Should we use something like GURL("www.example.com") instead?
> Just wondering...

Probably. It'd stress more codepaths. Done. (I used http://www.example.com)

[wtc, 2010-08-18 22:39:41]

LGTM.