Bug 1552446 - Leverage Remote Settings attachments utils for intermediates r=glasserc,jcj
authorMathieu Leplatre <mathieu@mozilla.com>
Mon, 16 Dec 2019 17:01:37 +0000
changeset 507177 8b5bcdecb7ad9e1c0d33ec04b809cf3fea65b03c
parent 507176 52c86d6ee05b74ad2c6e0b7964c3f24ea445a915
child 507178 551bc504b93ee06f9931924987ec28dcdddb96a2
push id36923
push userccoroiu@mozilla.com
push dateMon, 16 Dec 2019 21:47:33 +0000
treeherdermozilla-central@c238b47d7d57 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglasserc, jcj
bugs1552446
milestone73.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 1552446 - Leverage Remote Settings attachments utils for intermediates r=glasserc,jcj Differential Revision: https://phabricator.services.mozilla.com/D54727
security/manager/ssl/RemoteSecuritySettings.jsm
security/manager/ssl/tests/unit/test_intermediate_preloads.js
--- a/security/manager/ssl/RemoteSecuritySettings.jsm
+++ b/security/manager/ssl/RemoteSecuritySettings.jsm
@@ -66,66 +66,27 @@ const CRLITE_FILTERS_ENABLED_PREF =
   "security.remote_settings.crlite_filters.enabled";
 const CRLITE_FILTERS_SIGNER_PREF =
   "security.remote_settings.crlite_filters.signer";
 
 XPCOMUtils.defineLazyGlobalGetters(this, ["fetch"]);
 
 XPCOMUtils.defineLazyGetter(this, "gTextDecoder", () => new TextDecoder());
 
-XPCOMUtils.defineLazyGetter(this, "baseAttachmentsURL", async () => {
-  const server = Services.prefs.getCharPref("services.settings.server");
-  const serverInfo = await (await fetch(`${server}/`, {
-    credentials: "omit",
-  })).json();
-  const {
-    capabilities: {
-      attachments: { base_url },
-    },
-  } = serverInfo;
-  return base_url;
-});
-
 XPCOMUtils.defineLazyGetter(this, "log", () => {
   let { ConsoleAPI } = ChromeUtils.import("resource://gre/modules/Console.jsm");
   return new ConsoleAPI({
     prefix: "RemoteSecuritySettings.jsm",
     // tip: set maxLogLevel to "debug" and use log.debug() to create detailed
     // messages during development. See LOG_LEVELS in Console.jsm for details.
     maxLogLevel: "error",
     maxLogLevelPref: LOGLEVEL_PREF,
   });
 });
 
-function hexify(data) {
-  return Array.from(data, (c, i) =>
-    data
-      .charCodeAt(i)
-      .toString(16)
-      .padStart(2, "0")
-  ).join("");
-}
-
-// Hash a UTF-8 string into a hex string with SHA256
-function getHash(str) {
-  // return the two-digit hexadecimal code for a byte
-  let hasher = Cc["@mozilla.org/security/hash;1"].createInstance(
-    Ci.nsICryptoHash
-  );
-  hasher.init(Ci.nsICryptoHash.SHA256);
-  let stringStream = Cc["@mozilla.org/io/string-input-stream;1"].createInstance(
-    Ci.nsIStringInputStream
-  );
-  stringStream.data = str;
-  hasher.updateFromStream(stringStream, -1);
-
-  // convert the binary hash data to a hex string.
-  return hexify(hasher.finish(false));
-}
-
 // Converts a JS string to an array of bytes consisting of the char code at each
 // index in the string.
 function stringToBytes(s) {
   let b = [];
   for (let i = 0; i < s.length; i++) {
     b.push(s.charCodeAt(i));
   }
   return b;
@@ -611,120 +572,52 @@ class IntermediatePreloads {
             : Ci.nsICertStorage.STATE_UNSET
         )
       );
     }
     await new Promise(resolve => certStorage.setCRLiteState(entries, resolve));
   }
 
   /**
-   * Downloads the attachment data of the given record. Does not retry,
-   * leaving that to the caller.
-   * @param  {AttachmentRecord} record The data to obtain
-   * @return {Promise}          resolves to a Uint8Array on success
-   */
-  async _downloadAttachmentBytes(record) {
-    const {
-      attachment: { location },
-    } = record;
-    const remoteFilePath = (await baseAttachmentsURL) + location;
-    const headers = new Headers();
-    headers.set("Accept-Encoding", "gzip");
-
-    return fetch(remoteFilePath, {
-      headers,
-      credentials: "omit",
-    })
-      .then(resp => {
-        log.debug(`Download fetch completed: ${resp.ok} ${resp.status}`);
-        if (!resp.ok) {
-          Cu.reportError(`Failed to fetch ${remoteFilePath}: ${resp.status}`);
-
-          Services.telemetry
-            .getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-            .add("failedToFetch");
-
-          return Promise.reject();
-        }
-        return resp.arrayBuffer();
-      })
-      .then(buffer => new Uint8Array(buffer));
-  }
-
-  /**
    * Attempts to download the attachment, assuming it's not been processed
    * already. Does not retry, and always resolves (e.g., does not reject upon
    * failure.) Errors are reported via Cu.reportError.
    * @param  {AttachmentRecord} record defines which data to obtain
    * @return {Promise}          a Promise that will resolve to an object with the properties
    *                            record, cert, and subject. record is the original record.
    *                            cert is the base64-encoded bytes of the downloaded certificate (if
    *                            downloading was successful), and null otherwise.
    *                            subject is the base64-encoded bytes of the subject distinguished
    *                            name of the same.
    */
   async maybeDownloadAttachment(record) {
-    const {
-      attachment: { hash, size },
-    } = record;
     let result = { record, cert: null, subject: null };
 
-    let attachmentData;
+    let dataAsString = null;
     try {
-      attachmentData = await this._downloadAttachmentBytes(record);
+      let buffer = await this.client.attachments.downloadAsBytes(record, {
+        retries: 0,
+      });
+      dataAsString = gTextDecoder.decode(new Uint8Array(buffer));
     } catch (err) {
-      Cu.reportError(`Failed to download attachment: ${err}`);
-      Services.telemetry
-        .getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("failedToDownloadMisc");
-      return result;
-    }
-
-    if (!attachmentData || attachmentData.length == 0) {
       // Bug 1519273 - Log telemetry for these rejections
-      log.debug(`Empty attachment. Hash=${hash}`);
-
-      Services.telemetry
-        .getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("emptyAttachment");
-
+      if (err.name == "BadContentError") {
+        log.debug(`Bad attachment content.`);
+        Services.telemetry
+          .getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
+          .add("unexpectedHash");
+      } else {
+        Cu.reportError(`Failed to download attachment: ${err}`);
+        Services.telemetry
+          .getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
+          .add("failedToDownloadMisc");
+      }
       return result;
     }
 
-    // check the length
-    if (attachmentData.length !== size) {
-      log.debug(
-        `Unexpected attachment length. Hash=${hash} Lengths ${
-          attachmentData.length
-        } != ${size}`
-      );
-
-      Services.telemetry
-        .getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("unexpectedLength");
-
-      return result;
-    }
-
-    // check the hash
-    let dataAsString = gTextDecoder.decode(attachmentData);
-    let calculatedHash = getHash(dataAsString);
-    if (calculatedHash !== hash) {
-      log.warn(
-        `Invalid hash. CalculatedHash=${calculatedHash}, Hash=${hash}, data=${dataAsString}`
-      );
-
-      Services.telemetry
-        .getHistogramById(INTERMEDIATES_ERRORS_TELEMETRY)
-        .add("unexpectedHash");
-
-      return result;
-    }
-    log.debug(`downloaded cert with hash=${hash}, size=${size}`);
-
     let certBase64;
     let subjectBase64;
     try {
       // split off the header and footer
       certBase64 = dataAsString.split("-----")[2].replace(/\s/g, "");
       // get an array of bytes so we can use X509.jsm
       let certBytes = stringToBytes(atob(certBase64));
       let cert = new X509.Certificate();
--- a/security/manager/ssl/tests/unit/test_intermediate_preloads.js
+++ b/security/manager/ssl/tests/unit/test_intermediate_preloads.js
@@ -293,19 +293,19 @@ add_task(
       .snapshot();
 
     equal(
       countTelemetryReports(errors_histogram),
       1,
       "There should be only one error report"
     );
     equal(
-      errors_histogram.values[8],
+      errors_histogram.values[7],
       1,
-      "There should be one invalid length error"
+      "There should be one invalid content hash error"
     );
 
     equal(
       (await locallyDownloaded()).length,
       0,
       "There should be no local entry"
     );