bug 867473 - (4/4) remove nsIX509Cert.issuer and getChain r=jcj
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 17 Apr 2018 13:07:52 -0700
changeset 468037 05d72da7c0a39b461ea836143028cb992602f101
parent 468036 2cf15e5e3918f4b06b25e0803ddb2af6cc09b50d
child 468038 16d084813aa7b8c24a42c08279b8ece88ec3ad57
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjcj
bugs867473
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 867473 - (4/4) remove nsIX509Cert.issuer and getChain r=jcj 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