bug 867473 - (4/4) remove nsIX509Cert.issuer and getChain r?jcj,fkiefer draft
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 17 Apr 2018 13:07:52 -0700
changeset 783976 4c028d01a67936bc54099260c3e4e686dbde9f0d
parent 783972 d73d03d1addac1c7d1624da88607c995bcbf1f67
push id106829
push userbmo:dkeeler@mozilla.com
push dateTue, 17 Apr 2018 23:24:38 +0000
reviewersjcj, fkiefer
bugs867473
milestone61.0a1
bug 867473 - (4/4) remove nsIX509Cert.issuer and getChain r?jcj,fkiefer These functions cause main-thread certificate verifications, which is bad for performance. In general, nsIX509CertDB.asyncVerifyCertAtTime should be used instead. MozReview-Commit-ID: 9nkUDmyFY0k
security/manager/ssl/nsIX509Cert.idl
security/manager/ssl/nsNSSCertificate.cpp
security/manager/ssl/tests/unit/moz.build
security/manager/ssl/tests/unit/test_getchain.js
security/manager/ssl/tests/unit/test_getchain/ca-1.pem
security/manager/ssl/tests/unit/test_getchain/ca-1.pem.certspec
security/manager/ssl/tests/unit/test_getchain/ca-2.pem
security/manager/ssl/tests/unit/test_getchain/ca-2.pem.certspec
security/manager/ssl/tests/unit/test_getchain/ee.pem
security/manager/ssl/tests/unit/test_getchain/ee.pem.certspec
security/manager/ssl/tests/unit/test_getchain/moz.build
security/manager/ssl/tests/unit/xpcshell.ini
--- a/security/manager/ssl/nsIX509Cert.idl
+++ b/security/manager/ssl/nsIX509Cert.idl
@@ -132,22 +132,16 @@ interface nsIX509Cert : nsISupports {
 
   /**
    *  The issuer subject's organizational unit.
    */
   [must_use]
   readonly attribute AString issuerOrganizationUnit;
 
   /**
-   *  The certificate used by the issuer to sign this certificate.
-   */
-  [must_use]
-  readonly attribute nsIX509Cert issuer;
-
-  /**
    *  This certificate's validity period.
    */
   readonly attribute nsIX509CertValidity validity;
 
   /**
    *  A unique identifier of this certificate within the local storage.
    */
   [must_use]
@@ -184,26 +178,16 @@ interface nsIX509Cert : nsISupports {
   /**
    *  Constants for specifying the chain mode when exporting a certificate
    */
   const unsigned long CMS_CHAIN_MODE_CertOnly = 1;
   const unsigned long CMS_CHAIN_MODE_CertChain = 2;
   const unsigned long CMS_CHAIN_MODE_CertChainWithRoot = 3;
 
   /**
-   *  Obtain a list of certificates that contains this certificate
-   *  and the issuing certificates of all involved issuers,
-   *  up to the root issuer.
-   *
-   *  @return The chain of certifficates including the issuers.
-   */
-  [must_use]
-  nsIArray getChain();
-
-  /**
    * A comma separated list of localized strings representing the contents of
    * the certificate's key usage extension, if present. The empty string if the
    * certificate doesn't have the key usage extension, or has an empty extension.
    */
   [must_use]
   readonly attribute AString keyUsages;
 
   /**
--- a/security/manager/ssl/nsNSSCertificate.cpp
+++ b/security/manager/ssl/nsNSSCertificate.cpp
@@ -517,123 +517,28 @@ nsNSSCertificate::GetIssuerOrganizationU
     if (organizationUnit) {
       aOrganizationUnit = NS_ConvertUTF8toUTF16(organizationUnit.get());
     }
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsNSSCertificate::GetIssuer(nsIX509Cert** aIssuer)
-{
-  NS_ENSURE_ARG(aIssuer);
-  *aIssuer = nullptr;
-
-  nsCOMPtr<nsIArray> chain;
-  nsresult rv;
-  rv = GetChain(getter_AddRefs(chain));
-  NS_ENSURE_SUCCESS(rv, rv);
-  uint32_t length;
-  if (!chain || NS_FAILED(chain->GetLength(&length)) || length == 0) {
-    return NS_ERROR_UNEXPECTED;
-  }
-  if (length == 1) { // No known issuer
-    return NS_OK;
-  }
-  nsCOMPtr<nsIX509Cert> cert;
-  chain->QueryElementAt(1, NS_GET_IID(nsIX509Cert), getter_AddRefs(cert));
-  if (!cert) {
-    return NS_ERROR_UNEXPECTED;
-  }
-  cert.forget(aIssuer);
-  return NS_OK;
-}
-
-NS_IMETHODIMP
 nsNSSCertificate::GetOrganizationalUnit(nsAString& aOrganizationalUnit)
 {
   aOrganizationalUnit.Truncate();
   if (mCert) {
     UniquePORTString orgunit(CERT_GetOrgUnitName(&mCert->subject));
     if (orgunit) {
       aOrganizationalUnit = NS_ConvertUTF8toUTF16(orgunit.get());
     }
   }
   return NS_OK;
 }
 
-static nsresult
-UniqueCERTCertListToMutableArray(/*in*/ UniqueCERTCertList& nssChain,
-                                /*out*/ nsIArray** x509CertArray)
-{
-  if (!x509CertArray) {
-    return NS_ERROR_INVALID_ARG;
-  }
-
-  nsCOMPtr<nsIMutableArray> array = nsArrayBase::Create();
-  if (!array) {
-    return NS_ERROR_FAILURE;
-  }
-
-  CERTCertListNode* node;
-  for (node = CERT_LIST_HEAD(nssChain.get());
-       !CERT_LIST_END(node, nssChain.get());
-       node = CERT_LIST_NEXT(node)) {
-    nsCOMPtr<nsIX509Cert> cert = nsNSSCertificate::Create(node->cert);
-    nsresult rv = array->AppendElement(cert);
-    if (NS_FAILED(rv)) {
-      return rv;
-    }
-  }
-
-  array.forget(x509CertArray);
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-nsNSSCertificate::GetChain(nsIArray** _rvChain)
-{
-  NS_ENSURE_ARG(_rvChain);
-
-  mozilla::pkix::Time now(mozilla::pkix::Now());
-
-  RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier());
-  NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED);
-
-  UniqueCERTCertList nssChain;
-  // We want to test all usages supported by the certificate verifier, but we
-  // start with TLS server because most of the time Firefox users care about
-  // server certs.
-  const int usagesToTest[] = { certificateUsageSSLServer,
-                               certificateUsageSSLClient,
-                               certificateUsageSSLCA,
-                               certificateUsageEmailSigner,
-                               certificateUsageEmailRecipient };
-  for (auto usage : usagesToTest) {
-    if (certVerifier->VerifyCert(mCert.get(), usage, now,
-                                 nullptr, /*XXX fixme*/
-                                 nullptr, /*hostname*/
-                                 nssChain,
-                                 CertVerifier::FLAG_LOCAL_ONLY)
-          == mozilla::pkix::Success) {
-      return UniqueCERTCertListToMutableArray(nssChain, _rvChain);
-    }
-  }
-
-  // There is no verified path for the chain, however we still want to
-  // present to the user as much of a possible chain as possible, in the case
-  // where there was a problem with the cert or the issuers.
-  nssChain = UniqueCERTCertList(
-    CERT_GetCertChainFromCert(mCert.get(), PR_Now(), certUsageSSLClient));
-  if (!nssChain) {
-    return NS_ERROR_FAILURE;
-  }
-  return UniqueCERTCertListToMutableArray(nssChain, _rvChain);
-}
-
 NS_IMETHODIMP
 nsNSSCertificate::GetSubjectName(nsAString& _subjectName)
 {
   _subjectName.Truncate();
   if (mCert->subjectName) {
     _subjectName = NS_ConvertUTF8toUTF16(mCert->subjectName);
   }
   return NS_OK;
--- a/security/manager/ssl/tests/unit/moz.build
+++ b/security/manager/ssl/tests/unit/moz.build
@@ -19,17 +19,16 @@ TEST_DIRS += [
     'test_cert_sha1',
     'test_cert_signatures',
     'test_cert_trust',
     'test_cert_version',
     'test_certDB_import',
     'test_content_signing',
     'test_ct',
     'test_ev_certs',
-    'test_getchain',
     'test_intermediate_basic_usage_constraints',
     'test_keysize',
     'test_keysize_ev',
     'test_missing_intermediate',
     'test_name_constraints',
     'test_ocsp_fetch_method',
     'test_ocsp_url',
     'test_onecrl',
deleted file mode 100644
--- a/security/manager/ssl/tests/unit/test_getchain.js
+++ /dev/null
@@ -1,85 +0,0 @@
-// -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
-// This Source Code Form is subject to the terms of the Mozilla Public
-// License, v. 2.0. If a copy of the MPL was not distributed with this
-// file, You can obtain one at http://mozilla.org/MPL/2.0/.
-
-"use strict";
-
-do_get_profile(); // must be called before getting nsIX509CertDB
-const certdb  = Cc["@mozilla.org/security/x509certdb;1"]
-                  .getService(Ci.nsIX509CertDB);
-// This is the list of certificates needed for the test.
-var certList = [
-  "ee",
-  "ca-1",
-  "ca-2",
-];
-
-// Since all the ca's are identical expect for the serial number
-// I have to grab them by enumerating all the certs and then finding
-// the ones that I am interested in.
-function get_ca_array() {
-  let ret_array = [];
-  let allCerts = certdb.getCerts();
-  let enumerator = allCerts.getEnumerator();
-  while (enumerator.hasMoreElements()) {
-    let cert = enumerator.getNext().QueryInterface(Ci.nsIX509Cert);
-    if (cert.commonName == "ca") {
-      ret_array[parseInt(cert.serialNumber, 16)] = cert;
-    }
-  }
-  return ret_array;
-}
-
-
-function check_matching_issuer_and_getchain(expected_issuer_serial, cert) {
-  const nsIX509Cert = Ci.nsIX509Cert;
-
-  equal(expected_issuer_serial, cert.issuer.serialNumber,
-        "Expected and actual issuer serial numbers should match");
-  let chain = cert.getChain();
-  let issuer_via_getchain = chain.queryElementAt(1, nsIX509Cert);
-  // The issuer returned by cert.issuer or cert.getchain should be consistent.
-  equal(cert.issuer.serialNumber, issuer_via_getchain.serialNumber,
-        "Serial numbers via cert.issuer and via getChain() should match");
-}
-
-function check_getchain(ee_cert, ssl_ca, email_ca) {
-  // A certificate should first build a chain/issuer to
-  // a SSL trust domain, then an EMAIL trust domain and then
-  // an object signer trust domain.
-
-  const nsIX509Cert = Ci.nsIX509Cert;
-  certdb.setCertTrust(ssl_ca, nsIX509Cert.CA_CERT,
-                      Ci.nsIX509CertDB.TRUSTED_SSL);
-  certdb.setCertTrust(email_ca, nsIX509Cert.CA_CERT,
-                      Ci.nsIX509CertDB.TRUSTED_EMAIL);
-  check_matching_issuer_and_getchain(ssl_ca.serialNumber, ee_cert);
-  certdb.setCertTrust(ssl_ca, nsIX509Cert.CA_CERT, 0);
-  check_matching_issuer_and_getchain(email_ca.serialNumber, ee_cert);
-  certdb.setCertTrust(email_ca, nsIX509Cert.CA_CERT, 0);
-  // Do a final test on the case of no trust. The results must
-  // be consistent (the actual value is non-deterministic).
-  check_matching_issuer_and_getchain(ee_cert.issuer.serialNumber, ee_cert);
-}
-
-function run_test() {
-  clearOCSPCache();
-  clearSessionCache();
-
-  let ee_cert = null;
-  for (let cert of certList) {
-    let result = addCertFromFile(certdb, `test_getchain/${cert}.pem`, ",,");
-    if (cert == "ee") {
-      ee_cert = result;
-    }
-  }
-
-  notEqual(ee_cert, null, "EE cert should have successfully loaded");
-
-  let ca = get_ca_array();
-
-  check_getchain(ee_cert, ca[1], ca[2]);
-  // Swap ca certs to deal alternate trust settings.
-  check_getchain(ee_cert, ca[2], ca[1]);
-}
deleted file mode 100644
--- a/security/manager/ssl/tests/unit/test_getchain/ca-1.pem
+++ /dev/null
@@ -1,17 +0,0 @@
------BEGIN CERTIFICATE-----
-MIICtjCCAZ6gAwIBAgIBATANBgkqhkiG9w0BAQsFADANMQswCQYDVQQDDAJjYTAi
-GA8yMDE2MTEyNzAwMDAwMFoYDzIwMTkwMjA1MDAwMDAwWjANMQswCQYDVQQDDAJj
-YTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALqIUahEjhbWQf1utogG
-NhA9PBPZ6uQ1SrTs9WhXbCR7wcclqODYH72xnAabbhqG8mvir1p1a2pkcQh6pVqn
-RYf3HNUknAJ+zUP8HmnQOCApk6sgw0nk27lMwmtsDu0Vgg/xfq1pGrHTAjqLKkHu
-p3DgDw2N/WYLK7AkkqR9uYhheZCxV5A90jvF4LhIH6g304hD7ycW2FW3ZlqqfgKQ
-Lzp7EIAGJMwcbJetlmFbt+KWEsB1MaMMkd20yvf8rR0l0wnvuRcOp2jhs3svIm9p
-47SKlWEd7ibWJZ2rkQhONsscJAQsvxaLL+Xxj5kXMbiz/kkj+nJRxDHVA6zaGAo1
-7Y0CAwEAAaMdMBswDAYDVR0TBAUwAwEB/zALBgNVHQ8EBAMCAQYwDQYJKoZIhvcN
-AQELBQADggEBAFKB/sAo+kzOcOJD28nchLpfgq9pzqZoKPoGZWmYiQSQVWwlLk3z
-ZbIm8Qwioz7fCbjuH/Ilo4RDwt1t96q2tvi34rTs0DgkZSKX20JxQpUVzudUHYwc
-0SsZxwYU8/DySqOl/c0YyLXPdPr10H4fKSu7rdbTet8xPL5nD5JibwGi7LVtFLjv
-KDm/7fNg1kb5ntwPdVotDssBMvGgDzn9PPFScdL7ulTAzMlUFFPu/EfADGCx8XcY
-+1UJkDSOwmyOjLbjiN4fZdAhJ1M26O/3zjdJt4hq951Uncho2OmmaXPnfLqA9mbM
-ElgU2kCE9whztnNcNrxhvduj2wMTdSaAl0k=
------END CERTIFICATE-----
deleted file mode 100644
--- a/security/manager/ssl/tests/unit/test_getchain/ca-1.pem.certspec
+++ /dev/null
@@ -1,5 +0,0 @@
-issuer:ca
-subject:ca
-extension:basicConstraints:cA,
-extension:keyUsage:cRLSign,keyCertSign
-serialNumber:1
deleted file mode 100644
--- a/security/manager/ssl/tests/unit/test_getchain/ca-2.pem
+++ /dev/null
@@ -1,17 +0,0 @@
------BEGIN CERTIFICATE-----
-MIICtjCCAZ6gAwIBAgIBAjANBgkqhkiG9w0BAQsFADANMQswCQYDVQQDDAJjYTAi
-GA8yMDE2MTEyNzAwMDAwMFoYDzIwMTkwMjA1MDAwMDAwWjANMQswCQYDVQQDDAJj
-YTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALqIUahEjhbWQf1utogG
-NhA9PBPZ6uQ1SrTs9WhXbCR7wcclqODYH72xnAabbhqG8mvir1p1a2pkcQh6pVqn
-RYf3HNUknAJ+zUP8HmnQOCApk6sgw0nk27lMwmtsDu0Vgg/xfq1pGrHTAjqLKkHu
-p3DgDw2N/WYLK7AkkqR9uYhheZCxV5A90jvF4LhIH6g304hD7ycW2FW3ZlqqfgKQ
-Lzp7EIAGJMwcbJetlmFbt+KWEsB1MaMMkd20yvf8rR0l0wnvuRcOp2jhs3svIm9p
-47SKlWEd7ibWJZ2rkQhONsscJAQsvxaLL+Xxj5kXMbiz/kkj+nJRxDHVA6zaGAo1
-7Y0CAwEAAaMdMBswDAYDVR0TBAUwAwEB/zALBgNVHQ8EBAMCAQYwDQYJKoZIhvcN
-AQELBQADggEBAA5GrT0S6pZYYLJv05JDzNy4ax/0aBrkm6DQlwadbD1ZjJbivT7S
-fyDiIxNub0eMsp5QRKI36PV3KToN26khNoIgRualq9oSoZ5kbH1477nZXhCVIZk2
-sfC/M1uR8t3ziHPXGDpKMszmrCqLJPaEMpWDWklHMz99SCldi41sz+dg6BcdTRys
-ayWSZDGyUOM1EgS2MN+VZ6ucuXqvSnpn9l1e97XaSs/z3kPF8CpVFmL50xbBIHYl
-4fa9uiswzL0quczA3vNMkihHF4/pPG2Vc4chdNtTigrLbcjA0X42/vomLzNlkcQC
-7GKaYdFiclzUzLKwux8cTa1dJtHDyFkVLqg=
------END CERTIFICATE-----
deleted file mode 100644
--- a/security/manager/ssl/tests/unit/test_getchain/ca-2.pem.certspec
+++ /dev/null
@@ -1,5 +0,0 @@
-issuer:ca
-subject:ca
-extension:basicConstraints:cA,
-extension:keyUsage:cRLSign,keyCertSign
-serialNumber:2
deleted file mode 100644
--- a/security/manager/ssl/tests/unit/test_getchain/ee.pem
+++ /dev/null
@@ -1,17 +0,0 @@
------BEGIN CERTIFICATE-----
-MIICqjCCAZKgAwIBAgIUKQ7jtHr5Bp6UboOGF1ZvwHp9v3AwDQYJKoZIhvcNAQEL
-BQAwDTELMAkGA1UEAwwCY2EwIhgPMjAxNjExMjcwMDAwMDBaGA8yMDE5MDIwNTAw
-MDAwMFowDTELMAkGA1UEAwwCZWUwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEK
-AoIBAQC6iFGoRI4W1kH9braIBjYQPTwT2erkNUq07PVoV2wke8HHJajg2B+9sZwG
-m24ahvJr4q9adWtqZHEIeqVap0WH9xzVJJwCfs1D/B5p0DggKZOrIMNJ5Nu5TMJr
-bA7tFYIP8X6taRqx0wI6iypB7qdw4A8Njf1mCyuwJJKkfbmIYXmQsVeQPdI7xeC4
-SB+oN9OIQ+8nFthVt2Zaqn4CkC86exCABiTMHGyXrZZhW7filhLAdTGjDJHdtMr3
-/K0dJdMJ77kXDqdo4bN7LyJvaeO0ipVhHe4m1iWdq5EITjbLHCQELL8Wiy/l8Y+Z
-FzG4s/5JI/pyUcQx1QOs2hgKNe2NAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAF+s
-c8Q9YfGiE9uAc4+OArgF5b9Li9euJIBIdyicdhk2FIpGSUX/bds2QXNjokvkG/2M
-i0TW2jlPkFb4GRXznqo/zFzHNpVArYjbNlfqBBy5EUmbjtzJJhqn/z6Je2Gl92PU
-yWKPSHmK72jZx1GdbIk8fAp/mRwKqjVZ7tbDR7uXAXZppw7L9XMPTMycqSQ3v1ou
-Kj0lopCPA80XbFkOdhKNx0oNapZCswohzXr46AnfBKZUTkHskhv/LS3BkLR+CFQN
-qm6wiEhzeZwqLlpAJHlcPpDaB5L8zcgFv48fVw+XMX3kPVbrPpRsx9mLJT8kWP29
-cDQySy9cWzC85GvHia0=
------END CERTIFICATE-----
deleted file mode 100644
--- a/security/manager/ssl/tests/unit/test_getchain/ee.pem.certspec
+++ /dev/null
@@ -1,2 +0,0 @@
-issuer:ca
-subject:ee
deleted file mode 100644
--- a/security/manager/ssl/tests/unit/test_getchain/moz.build
+++ /dev/null
@@ -1,15 +0,0 @@
-# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*-
-# vim: set filetype=python:
-# This Source Code Form is subject to the terms of the Mozilla Public
-# License, v. 2.0. If a copy of the MPL was not distributed with this
-# file, You can obtain one at http://mozilla.org/MPL/2.0/.
-
-# Temporarily disabled. See bug 1256495.
-#test_certificates = (
-#    'ca-1.pem',
-#    'ca-2.pem',
-#    'ee.pem',
-#)
-#
-#for test_certificate in test_certificates:
-#    GeneratedTestCertificate(test_certificate)
--- a/security/manager/ssl/tests/unit/xpcshell.ini
+++ b/security/manager/ssl/tests/unit/xpcshell.ini
@@ -15,17 +15,16 @@ support-files =
   test_cert_signatures/**
   test_cert_trust/**
   test_cert_version/**
   test_certDB_import/**
   test_certviewer_invalid_oids/**
   test_content_signing/**
   test_ct/**
   test_ev_certs/**
-  test_getchain/**
   test_intermediate_basic_usage_constraints/**
   test_keysize/**
   test_keysize_ev/**
   test_missing_intermediate/**
   test_name_constraints/**
   test_ocsp_fetch_method/**
   test_ocsp_url/**
   test_onecrl/**
@@ -84,17 +83,16 @@ skip-if = toolkit == 'android'
 [test_der.js]
 [test_enterprise_roots.js]
 skip-if = os != 'win' # tests a Windows-specific feature
 [test_ev_certs.js]
 tags = blocklist
 run-sequentially = hardcoded ports
 [test_forget_about_site_security_headers.js]
 skip-if = toolkit == 'android'
-[test_getchain.js]
 [test_hash_algorithms.js]
 [test_hash_algorithms_wrap.js]
 # bug 1124289 - run_test_in_child violates the sandbox on android
 skip-if = toolkit == 'android'
 [test_hmac.js]
 [test_intermediate_basic_usage_constraints.js]
 [test_imminent_distrust.js]
 run-sequentially = hardcoded ports