Generate unique certificate nicknames on Linux/CrOS.

Generate unique certificate nicknames on Linux/CrOS.

When importing certificates on Linux/CrOS where the user has a pre-existing
cert, generate a unique certificate nickname if the DER-encoded subjects do
not match, as required by NSS.

This updates the template from being:
  <subject common name>'s <issuer common name> ID
to:
  <subject display name>'s <issuer display name> ID [#d]

Where #d will be appended with an incrementing number until a unique nickname
is found. Note that "display name" represents a gradiation that starts with
common name, then organization name, than organizational unit name.

Note: This does not address PKCS#12 importing - only importing CA certificates
(root and intermediate) and server certificates via the UI, or the handling
of application/x-x509-user-cert (via download).

BUG=237870
TEST=net_unittests added. Additionally, test that server & CA certificates can
still be imported fine through the UI.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201748

[Ryan Sleevi, 2013-05-22 01:46:54]

wtc: This covers all but the PKCS#12 import, for which NSS's API is horrible.
Mostly, I'm trying to avoid any issues for the silent importing being done via
application/x-x509-user-cert.

[wtc, 2013-05-22 20:58:45]

Patch set 3 LGTM.

https://codereview.chromium.org/15315003/diff/6001/net/cert/cert_database_nss.cc
File net/cert/cert_database_nss.cc (right):

https://codereview.chromium.org/15315003/diff/6001/net/cert/cert_database_nss...
net/cert/cert_database_nss.cc:104: LOG(ERROR) << "Couldn't import user
certificate. " << PORT_GetError();

Should we also log an error message on line 90?

https://codereview.chromium.org/15315003/diff/6001/net/cert/nss_cert_database...
File net/cert/nss_cert_database_unittest.cc (right):

https://codereview.chromium.org/15315003/diff/6001/net/cert/nss_cert_database...
net/cert/nss_cert_database_unittest.cc:941: // Importing two certificates with
the same issuer and common name, but

Could you clarify whether the "common name" is the issuer's
or subject's?

https://codereview.chromium.org/15315003/diff/6001/net/cert/nss_cert_database...
net/cert/nss_cert_database_unittest.cc:943: // the second certificate.

The test doesn't check the nicknames are different, right?

https://codereview.chromium.org/15315003/diff/6001/net/cert/nss_cert_database...
net/cert/nss_cert_database_unittest.cc:964: // Now attempt to issue a different
version with the same common name.

What does "issue a different version" mean?

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_certificate_...
File net/cert/x509_certificate_nss.cc (right):

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_certificate_...
net/cert/x509_certificate_nss.cc:94: case USER_CERT: {

Nit: you can remove the curly braces for this case because
the local variables are gone.

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_certificate_...
net/cert/x509_certificate_nss.cc:99: issuer_.GetDisplayName().c_str());

Is this change to fix the problem that the common name may
be empty?

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_util_nss.h
File net/cert/x509_util_nss.h (right):

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_util_nss.h#n...
net/cert/x509_util_nss.h:23: typedef struct PK11SlotInfoStr PK11SlotInfo;

Nit: list this typedef in sorted order. (The original list
is almost sorted.)

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_util_nss.h#n...
net/cert/x509_util_nss.h:101: // be prepended if calling an NSS function that
expects <token>:<nickname>.

Nit: this should be
  ..., and the token name must be prepended ...
           ^^^^^^^^^^^^^^

Nit: document how the function uses the |subject| input.

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_util_nss.h#n...
net/cert/x509_util_nss.h:102: // TODO(gspencer): Internationalize this: it's
wrong to hard-code English.

We need to describe how we make the nickname unique (by
appending "  #%d") to provide some context for this TODO
comment.  I guess #2, #3, etc. are only used in English?
(They are not used in Chinese.)

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_util_nss.h#n...
net/cert/x509_util_nss.h:104: SECItem* subject,

Nit: this should ideally be a const pointer. We will need
a const_cast when we pass this to the NSS function though.

[Ryan Sleevi, 2013-05-22 23:20:19]

https://codereview.chromium.org/15315003/diff/6001/net/cert/cert_database_nss.cc
File net/cert/cert_database_nss.cc (right):

https://codereview.chromium.org/15315003/diff/6001/net/cert/cert_database_nss...
net/cert/cert_database_nss.cc:104: LOG(ERROR) << "Couldn't import user
certificate. " << PORT_GetError();
On 2013/05/22 20:58:45, wtc wrote:
> 
> Should we also log an error message on line 90?

I'm actually not a big fan of the current error message. It's superflous in the
current implementation because it provides no additional context than the
ERR_ADD_USER_CERT_FAILED (which we end up surfacing to the user). I added the
Error code logging just to make it somewhat useful, since PK11_ImportCert has
much more logic/points of failure for it.

https://codereview.chromium.org/15315003/diff/6001/net/cert/nss_cert_database...
File net/cert/nss_cert_database_unittest.cc (right):

https://codereview.chromium.org/15315003/diff/6001/net/cert/nss_cert_database...
net/cert/nss_cert_database_unittest.cc:943: // the second certificate.
On 2013/05/22 20:58:45, wtc wrote:
> 
> The test doesn't check the nicknames are different, right?

Fixed.

https://codereview.chromium.org/15315003/diff/6001/net/cert/nss_cert_database...
net/cert/nss_cert_database_unittest.cc:964: // Now attempt to issue a different
version with the same common name.
On 2013/05/22 20:58:45, wtc wrote:
> 
> What does "issue a different version" mean?

typo
s/issue/import/
s/version/certificate/

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_certificate_...
File net/cert/x509_certificate_nss.cc (right):

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_certificate_...
net/cert/x509_certificate_nss.cc:94: case USER_CERT: {
On 2013/05/22 20:58:45, wtc wrote:
> 
> Nit: you can remove the curly braces for this case because
> the local variables are gone.

Done.

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_certificate_...
net/cert/x509_certificate_nss.cc:99: issuer_.GetDisplayName().c_str());
On 2013/05/22 20:58:45, wtc wrote:
> 
> Is this change to fix the problem that the common name may
> be empty?

Yes. Made the code simpler to fix while I was here, and seemed "obviously" wrong
when compared with FF.

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_util_nss.h
File net/cert/x509_util_nss.h (right):

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_util_nss.h#n...
net/cert/x509_util_nss.h:102: // TODO(gspencer): Internationalize this: it's
wrong to hard-code English.
On 2013/05/22 20:58:45, wtc wrote:
> 
> We need to describe how we make the nickname unique (by
> appending "  #%d") to provide some context for this TODO
> comment.  I guess #2, #3, etc. are only used in English?
> (They are not used in Chinese.)

I wanted to try to avoid describing in the header what the format is, other than
"it's unique"

And the internationalization applies not only to the #1/#2, but things like RTL
and how numbers are expressed.

https://codereview.chromium.org/15315003/diff/6001/net/cert/x509_util_nss.h#n...
net/cert/x509_util_nss.h:104: SECItem* subject,
On 2013/05/22 20:58:45, wtc wrote:
> 
> Nit: this should ideally be a const pointer. We will need
> a const_cast when we pass this to the NSS function though.

Sure, fixed.

[commit-bot: I haz the power, 2013-05-22 23:29:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/15315003/8002

[commit-bot: I haz the power, 2013-05-23 11:07:18]

Change committed as 201748