Adding User Certificate (.crt) Import to Certificate Manager

Adding User Certificate (.crt) Import to Certificate Manager

BUG=514767
Committed: https://crrev.com/3e98a71a93160c9585aed8f6a6f93adbc532e9d1
Cr-Commit-Position: refs/heads/master@{#361115}

[svaldez, 2015-11-02 18:32:37]

svaldez@chromium.org changed reviewers:
+ davidben@chromium.org, rsleevi@chromium.org

[svaldez, 2015-11-02 18:32:38]

The certificate manager half of the x509-user-cert UI gating.

[Ryan Sleevi, 2015-11-04 03:28:42]

rsleevi@chromium.org changed reviewers:
+ mattm@chromium.org
- rsleevi@chromium.org

[Ryan Sleevi, 2015-11-04 03:28:42]

I'm going to suggest mattm here as an excellent reviewer, and familiar with the
code. Despite his best efforts otherwise ;)

[mattm, 2015-11-06 00:24:21]

On 2015/11/02 18:32:38, svaldez wrote:
> The certificate manager half of the x509-user-cert UI gating.

So, in a later change CertDatabase::AddUserCert and related code would be
removed? How long would they co-exist?

[svaldez, 2015-11-06 14:05:16]

On 2015/11/06 00:24:21, mattm wrote:
> On 2015/11/02 18:32:38, svaldez wrote:
> > The certificate manager half of the x509-user-cert UI gating.
> 
> So, in a later change CertDatabase::AddUserCert and related code would be
> removed? How long would they co-exist?

Its unclear. While we could remove the Certificate Resource Handler calls to
AddUserCert given this CL and the removal of the mime-type handling. ChromeOS
also has the Enterprise Platform Keys API that still calls it, though we could
probably re-write it to use the new NSSCertDatabase method.

[mattm, 2015-11-06 23:13:00]

On 2015/11/06 14:05:16, svaldez wrote:
> On 2015/11/06 00:24:21, mattm wrote:
> > On 2015/11/02 18:32:38, svaldez wrote:
> > > The certificate manager half of the x509-user-cert UI gating.
> > 
> > So, in a later change CertDatabase::AddUserCert and related code would be
> > removed? How long would they co-exist?
> 
> Its unclear. While we could remove the Certificate Resource Handler calls to
> AddUserCert given this CL and the removal of the mime-type handling. ChromeOS
> also has the Enterprise Platform Keys API that still calls it, though we could
> probably re-write it to use the new NSSCertDatabase method.

Ok. In the short term (ie, this CL), at least change the NSS version of
CertDatabase::AddUserCert to call the same psm::ImportUserCert function.

Since the Enterprise Platform Keys API is chromeos only, it would definitely
make sense to make that use the NSSCertDatabase method and remove the resource
handler stuff and the CertDatabase::AddUserCert code (since that is implemented
on multiple platforms where it would no longer be used).

https://codereview.chromium.org/1423333006/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/1423333006/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:761:
l10n_util::GetStringUTF8(IDS_CERT_MANAGER_PKCS12_IMPORT_ERROR_TITLE),
this would be the wrong message in the case of .crt

https://codereview.chromium.org/1423333006/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:786: } else {
This case needs to do the cleanup and overlay dismiss stuff.

https://codereview.chromium.org/1423333006/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:803:
l10n_util::GetStringUTF8(IDS_CERT_MANAGER_PKCS12_IMPORT_ERROR_TITLE),
same comment as above

https://codereview.chromium.org/1423333006/diff/1/net/cert/nss_cert_database.h
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/1423333006/diff/1/net/cert/nss_cert_database....
net/cert/nss_cert_database.h:195: // Import user certificates.
"Import a user certificate", since it only does one.
Also include details about the necessary pre-conditions, etc.

https://codereview.chromium.org/1423333006/diff/1/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

https://codereview.chromium.org/1423333006/diff/1/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:210: int
ImportUserCert(const net::CertificateList& certificates) {
Change the NSS version of CertDatabase::AddUserCert to call this also.

https://codereview.chromium.org/1423333006/diff/1/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:214: const
scoped_refptr<net::X509Certificate>& cert = certificates[0];
If this only uses the first cert, just pass that in instead of a list.

https://codereview.chromium.org/1423333006/diff/1/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:216:
PK11SlotInfo* slot =
crypto::ScopedPK11Slot

https://codereview.chromium.org/1423333006/diff/1/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:227:
cert->GetDefaultNickname(net::SERVER_CERT),
USER_CERT?

[svaldez, 2015-11-10 15:07:50]

https://codereview.chromium.org/1423333006/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/1423333006/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:761:
l10n_util::GetStringUTF8(IDS_CERT_MANAGER_PKCS12_IMPORT_ERROR_TITLE),
On 2015/11/06 23:13:00, mattm wrote:
> this would be the wrong message in the case of .crt

Done.

https://codereview.chromium.org/1423333006/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:786: } else {
On 2015/11/06 23:12:59, mattm wrote:
> This case needs to do the cleanup and overlay dismiss stuff.

Done.

https://codereview.chromium.org/1423333006/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:803:
l10n_util::GetStringUTF8(IDS_CERT_MANAGER_PKCS12_IMPORT_ERROR_TITLE),
On 2015/11/06 23:13:00, mattm wrote:
> same comment as above

Done.

https://codereview.chromium.org/1423333006/diff/1/net/cert/nss_cert_database.h
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/1423333006/diff/1/net/cert/nss_cert_database....
net/cert/nss_cert_database.h:195: // Import user certificates.
On 2015/11/06 23:13:00, mattm wrote:
> "Import a user certificate", since it only does one.
> Also include details about the necessary pre-conditions, etc.

Done.

https://codereview.chromium.org/1423333006/diff/1/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right):

https://codereview.chromium.org/1423333006/diff/1/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:210: int
ImportUserCert(const net::CertificateList& certificates) {
On 2015/11/06 23:13:00, mattm wrote:
> Change the NSS version of CertDatabase::AddUserCert to call this also.

Done.

https://codereview.chromium.org/1423333006/diff/1/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:214: const
scoped_refptr<net::X509Certificate>& cert = certificates[0];
On 2015/11/06 23:13:00, mattm wrote:
> If this only uses the first cert, just pass that in instead of a list.

We may want to eventually verify the user cert cert chain, and it seems cleaner
to have this method choose the specific cert, instead of having to make all the
callers select the first cert to pass in.

https://codereview.chromium.org/1423333006/diff/1/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:216:
PK11SlotInfo* slot =
On 2015/11/06 23:13:00, mattm wrote:
> crypto::ScopedPK11Slot

Done.

https://codereview.chromium.org/1423333006/diff/1/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:227:
cert->GetDefaultNickname(net::SERVER_CERT),
On 2015/11/06 23:13:00, mattm wrote:
> USER_CERT?

Done.

[mattm, 2015-11-10 21:59:42]

https://codereview.chromium.org/1423333006/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1423333006/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:3303: +          There was an error while
trying to read the file: <ph name="ERROR_TEXT">$1<ex>File not found.</ex></ph>.
Seems a bit long for a title. And the uses of it in the CL don't pass in any
format argument.
Also it seems a bit odd that IDS_CERT_MANAGER_PKCS12_IMPORT_ERROR_TITLE still
exists and gets used in one specific case while this new one is used in other
cases (even for .p12).

I'd just replace IDS_CERT_MANAGER_PKCS12_IMPORT_ERROR_TITLE with
IDS_CERT_MANAGER_USER_IMPORT_ERROR_TITLE and give it a more generic title like
"User Certificate Import Error".

https://codereview.chromium.org/1423333006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/1423333006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:7: #include
<errno.h>
unused?

[svaldez, 2015-11-11 14:53:14]

https://codereview.chromium.org/1423333006/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1423333006/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:3303: +          There was an error while
trying to read the file: <ph name="ERROR_TEXT">$1<ex>File not found.</ex></ph>.
On 2015/11/10 21:59:41, mattm wrote:
> Seems a bit long for a title. And the uses of it in the CL don't pass in any
> format argument.
> Also it seems a bit odd that IDS_CERT_MANAGER_PKCS12_IMPORT_ERROR_TITLE still
> exists and gets used in one specific case while this new one is used in other
> cases (even for .p12).
> 
> I'd just replace IDS_CERT_MANAGER_PKCS12_IMPORT_ERROR_TITLE with
> IDS_CERT_MANAGER_USER_IMPORT_ERROR_TITLE and give it a more generic title like
> "User Certificate Import Error".

Done.

https://codereview.chromium.org/1423333006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/1423333006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:7: #include
<errno.h>
On 2015/11/10 21:59:41, mattm wrote:
> unused?

Done.

[mattm, 2015-11-11 22:14:39]

lgtm if you re-add the errno include.

https://codereview.chromium.org/1423333006/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/1423333006/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:7: #include
<errno.h>
On 2015/11/11 14:53:14, svaldez wrote:
> On 2015/11/10 21:59:41, mattm wrote:
> > unused?
> 
> Done.

Nevermind, looks like that actually came in from a rebase. In the future I would
recommend uploading a rebase in a separate patchset from any of your own
changes.

[svaldez, 2015-11-12 17:58:44]

svaldez@chromium.org changed reviewers:
+ jhawkins@chromium.org

[svaldez, 2015-11-12 17:58:46]

Hi,

This CL is part of a set to move the user certificate import from being a
mime-type handler and instead to allow the user to import it through the
settings UI.

[svaldez, 2015-11-19 15:58:34]

svaldez@chromium.org changed reviewers:
+ thestig@chromium.org
- jhawkins@chromium.org

[svaldez, 2015-11-19 15:58:34]

thestig: Any chance you could take a look at the certificate manager changes
under chrome/browser/ and review?

[Lei Zhang, 2015-11-19 22:23:59]

https://codereview.chromium.org/1423333006/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/1423333006/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:733: } else {
Just to be clear, is this the .crt file case? Assuming yes, do you care to check
the files have a .crt extension or does it not really matter?

[svaldez, 2015-11-20 15:01:53]

https://codereview.chromium.org/1423333006/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/1423333006/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:733: } else {
On 2015/11/19 22:23:59, Lei Zhang wrote:
> Just to be clear, is this the .crt file case? Assuming yes, do you care to
check
> the files have a .crt extension or does it not really matter?

This is for the '.crt' case, however there are other non-standard extensions
used for certificate files that while we shouldn't explicitly add to the file
list, since they aren't standard, should probably go through the .crt code path
(ex: .cer, .der, ...). Using this path for other files also means we'll attempt
to import them and then error out, rather than having a password prompt show up
which isn't relevant for non-p12 files.

[Lei Zhang, 2015-11-21 01:55:15]

lgtm with some nits.

https://codereview.chromium.org/1423333006/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/1423333006/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:732:
"CertificateManager.importPersonalAskPassword");
You may also consider adding a return here, and then you can drop the else
keyword and everything in the else block can be shifted 2 chars to the left.
Like in ImportPersonalPasswordSelected() below.

Ditto for ImportPersonalFileRead().

https://codereview.chromium.org/1423333006/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:733: } else {
On 2015/11/20 15:01:52, svaldez wrote:
> On 2015/11/19 22:23:59, Lei Zhang wrote:
> > Just to be clear, is this the .crt file case? Assuming yes, do you care to
> check
> > the files have a .crt extension or does it not really matter?
> 
> This is for the '.crt' case, however there are other non-standard extensions
> used for certificate files that while we shouldn't explicitly add to the file
> list, since they aren't standard, should probably go through the .crt code
path
> (ex: .cer, .der, ...). Using this path for other files also means we'll
attempt
> to import them and then error out, rather than having a password prompt show
up
> which isn't relevant for non-p12 files.

Got it. Can you write a comment here to explain the situation, and in
CertificateManagerHandler::ImportPersonalFileRead(), which I presume is similar?

[svaldez, 2015-11-23 15:17:54]

https://codereview.chromium.org/1423333006/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/1423333006/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:732:
"CertificateManager.importPersonalAskPassword");
On 2015/11/21 01:55:15, Lei Zhang wrote:
> You may also consider adding a return here, and then you can drop the else
> keyword and everything in the else block can be shifted 2 chars to the left.
> Like in ImportPersonalPasswordSelected() below.
> 
> Ditto for ImportPersonalFileRead().

Done.

https://codereview.chromium.org/1423333006/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:733: } else {
On 2015/11/21 01:55:15, Lei Zhang wrote:
> On 2015/11/20 15:01:52, svaldez wrote:
> > On 2015/11/19 22:23:59, Lei Zhang wrote:
> > > Just to be clear, is this the .crt file case? Assuming yes, do you care to
> > check
> > > the files have a .crt extension or does it not really matter?
> > 
> > This is for the '.crt' case, however there are other non-standard extensions
> > used for certificate files that while we shouldn't explicitly add to the
file
> > list, since they aren't standard, should probably go through the .crt code
> path
> > (ex: .cer, .der, ...). Using this path for other files also means we'll
> attempt
> > to import them and then error out, rather than having a password prompt show
> up
> > which isn't relevant for non-p12 files.
> 
> Got it. Can you write a comment here to explain the situation, and in
> CertificateManagerHandler::ImportPersonalFileRead(), which I presume is
similar?

Done.

[svaldez, 2015-11-23 15:18:03]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-11-23 15:18:04]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1423333006/#ps100001
(title: "Updating comment.s")

[commit-bot: I haz the power, 2015-11-23 15:19:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423333006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423333006/100001

[commit-bot: I haz the power, 2015-11-23 16:22:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-23 16:23:08]

Patchset 6 (id:??) landed as
https://crrev.com/3e98a71a93160c9585aed8f6a6f93adbc532e9d1
Cr-Commit-Position: refs/heads/master@{#361115}