Bug 1453690 - Fail nicely when RemoteSetting collection is not signed r?mgoodwin
MozReview-Commit-ID: ESP7GXfmYzU
--- a/services/common/remote-settings.js
+++ b/services/common/remote-settings.js
@@ -30,16 +30,17 @@ const PREF_SETTINGS_LAST_UPDATE =
const PREF_SETTINGS_LAST_ETAG = "services.settings.last_etag";
const PREF_SETTINGS_CLOCK_SKEW_SECONDS = "services.settings.clock_skew_seconds";
const PREF_SETTINGS_LOAD_DUMP = "services.settings.load_dump";
// Telemetry update source identifier.
const TELEMETRY_HISTOGRAM_KEY = "settings-changes-monitoring";
const INVALID_SIGNATURE = "Invalid content/signature";
+const MISSING_SIGNATURE = "Missing signature";
function mergeChanges(collection, localRecords, changes) {
const records = {};
// Local records by id.
localRecords.forEach((record) => records[record.id] = collection.cleanLocalFields(record));
// All existing records are replaced by the version from the server.
changes.forEach((record) => records[record.id] = record);
@@ -52,25 +53,25 @@ function mergeChanges(collection, localR
if (a.id < b.id) {
return -1;
}
return a.id > b.id ? 1 : 0;
});
}
-function fetchCollectionMetadata(remote, collection) {
+async function fetchCollectionMetadata(remote, collection) {
const client = new KintoHttpClient(remote);
- return client.bucket(collection.bucket).collection(collection.name).getData()
- .then(result => {
- return result.signature;
- });
+ const { signature } = await client.bucket(collection.bucket)
+ .collection(collection.name)
+ .getData();
+ return signature;
}
-function fetchRemoteCollection(remote, collection) {
+async function fetchRemoteCollection(remote, collection) {
const client = new KintoHttpClient(remote);
return client.bucket(collection.bucket)
.collection(collection.name)
.listRecords({sort: "id"});
}
async function fetchLatestChanges(url, lastEtag) {
//
@@ -297,18 +298,21 @@ class RemoteSettingsClient {
syncResult.created.push(r);
}
}
// Records that remain in our map now are those missing from remote
syncResult.deleted = Array.from(oldById.values());
}
} else {
- // The sync has thrown, it can be a network or a general error.
- if (/NetworkError/.test(e.message)) {
+ // The sync has thrown, it can be related to metadata, network or a general error.
+ if (e.message == MISSING_SIGNATURE) {
+ // Collection metadata has no signature info, no need to retry.
+ reportStatus = UptakeTelemetry.STATUS.SIGNATURE_ERROR;
+ } else if (/NetworkError/.test(e.message)) {
reportStatus = UptakeTelemetry.STATUS.NETWORK_ERROR;
} else if (/Backoff/.test(e.message)) {
reportStatus = UptakeTelemetry.STATUS.BACKOFF;
} else {
reportStatus = UptakeTelemetry.STATUS.SYNC_ERROR;
}
throw e;
}
@@ -362,19 +366,22 @@ class RemoteSettingsClient {
throw new Error(`Could not read from '${fileURI}'`);
}
// Will be rejected if JSON is invalid.
return response.json();
}
async _validateCollectionSignature(remote, payload, collection, options = {}) {
const {ignoreLocal} = options;
-
// this is a content-signature field from an autograph response.
- const {x5u, signature} = await fetchCollectionMetadata(remote, collection);
+ const signaturePayload = await fetchCollectionMetadata(remote, collection);
+ if (!signaturePayload) {
+ throw new Error(MISSING_SIGNATURE);
+ }
+ const {x5u, signature} = signaturePayload;
const certChainResponse = await fetch(x5u);
const certChain = await certChainResponse.text();
const verifier = Cc["@mozilla.org/security/contentsignatureverifier;1"]
.createInstance(Ci.nsIContentSignatureVerifier);
let toSerialize;
if (ignoreLocal) {
--- a/services/common/tests/unit/test_blocklist_signatures.js
+++ b/services/common/tests/unit/test_blocklist_signatures.js
@@ -257,16 +257,29 @@ add_task(async function test_check_signa
],
status: {status: 200, statusText: "OK"},
responseBody: JSON.stringify({"data": []})
};
const RESPONSE_BODY_META_EMPTY_SIG = makeMetaResponseBody(1000,
"vxuAg5rDCB-1pul4a91vqSBQRXJG_j7WOYUTswxRSMltdYmbhLRH8R8brQ9YKuNDF56F-w6pn4HWxb076qgKPwgcEBtUeZAO_RtaHXRkRUUgVzAr86yQL4-aJTbv3D6u");
+ const RESPONSE_META_NO_SIG = {
+ sampleHeaders: [
+ "Content-Type: application/json; charset=UTF-8",
+ `ETag: \"123456\"`
+ ],
+ status: {status: 200, statusText: "OK"},
+ responseBody: JSON.stringify({
+ data: {
+ last_modified: 123456
+ }
+ })
+ };
+
// The collection metadata containing the signature for the empty
// collection.
const RESPONSE_META_EMPTY_SIG =
makeMetaResponse(1000, RESPONSE_BODY_META_EMPTY_SIG,
"RESPONSE_META_EMPTY_SIG");
// Here, we map request method and path to the available responses
const emptyCollectionResponses = {
@@ -558,16 +571,41 @@ add_task(async function test_check_signa
} catch (e) {
await checkRecordCount(OneCRLBlocklistClient, 2);
}
// Ensure that the failure is reflected in the accumulated telemetry:
endHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY);
expectedIncrements = {[UptakeTelemetry.STATUS.SIGNATURE_RETRY_ERROR]: 1};
checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements);
+
+
+ const missingSigResponses = {
+ // In this test, we deliberately serve metadata without the signature attribute.
+ // As if the collection was not signed.
+ "GET:/v1/buckets/blocklists/collections/certificates?":
+ [RESPONSE_META_NO_SIG],
+ };
+
+ startHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY);
+ registerHandlers(missingSigResponses);
+ try {
+ await OneCRLBlocklistClient.maybeSync(6000, startTime);
+ do_throw("Sync should fail (the signature is missing)");
+ } catch (e) {
+ await checkRecordCount(OneCRLBlocklistClient, 2);
+ }
+
+ // Ensure that the failure is reflected in the accumulated telemetry:
+ endHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY);
+ expectedIncrements = {
+ [UptakeTelemetry.STATUS.SIGNATURE_ERROR]: 1,
+ [UptakeTelemetry.STATUS.SIGNATURE_RETRY_ERROR]: 0 // Not retried since missing.
+ };
+ checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements);
});
function run_test() {
BlocklistClients.initialize();
// ensure signatures are enforced
Services.prefs.setBoolPref(PREF_SETTINGS_VERIFY_SIGNATURE, true);