Bug 1252882 introduces a content signature service. Let's make use of this to ensure the authenticity of our kinto collections.

As notes, this is the way the patch would look like: https://gist.github.com/leplatrem/c2877cee0bb6c3f5e5612b165eb5ff30 (I took Alexis code and rewrote it using generators)

Do we want different certificates for each collection? (Alexis mentionned that here [0]) [0] https://github.com/Kinto/kinto-signer/pull/52#issuecomment-209691656

(In reply to Mathieu Leplatre (:leplatrem) from comment #2) > Do we want different certificates for each collection? (Alexis mentionned > that here [0]) > > [0] https://github.com/Kinto/kinto-signer/pull/52#issuecomment-209691656 Good question. I think we probably do. At the moment, the content signature service assumes one of two signers (for remote-newtab - which doesn't use Kinto collections - and oneCRL) - it'd be trivial to add more.

Some notes on testing. We need both valid and invalid signatures for various states of a collection to test this effectively. A valid signature also implies that we need a test certificate that is trusted by the content signature verifier for the purposes of the test. It seems sensible to make use of the keys included for the PSM xpshell tests and the certificates, which use these keys, created for the existing content signature xpcshell tests (as seen in security/manager/ssl/tests/unit/test_content_signing). For mocking server responses, we can do something similar to the existing kinto tests (e.g. the certificate blocklist test here: services/common/tests/unit/test_kintoCertBlocklist.js - since this will be our first signed blocklist). To get sample messages we can do one of two things: a) Create messages by hand or b) set up autograph (with the aforementioned keypair) and kinto and intercept responses.

Here is a good place to start for understanding how the services hang together: https://github.com/Kinto/kinto-signer

The implementation in services/common/KintoBlocklist.js needs work on how we handle invalid signatures: in particular, we should try to retrieve a full collection should the canonical form of the merged data appear incorrect. We also need: * A way to enable / disable content signature checks * Telemetry (maybe not in this bug) The tests should include: * checks for modification of local data prior to a sync (thus forcing a verification failure and re-fetch). * checks with bad signatures * checks with broken / missing certificate chains * checks for incorrect subject / SAN in the call to verifyContentSignature There are also various things that can be done to make the tests work better. In particular, all of the data (cert chain, signatures) is hardcoded. It would be nice to move toward test-time generation of some of these things (maybe pull in the chain from the PSM tests or something)

Add functionality to retrieve a full collection if signature verification fails

> Do we want different certificates for each collection? Yes. Autograph currently supports RemoteNewtab (non-kinto) and OneCRL (kinto). When we add more collections (like AMO Blocklist), we'll need more certificates.

Review commit: https://reviewboard.mozilla.org/r/49755/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/49755/

https://reviewboard.mozilla.org/r/49755/#review46537 ::: services/common/KintoBlocklist.js:162 (Diff revision 1) > this.updateLastCheck(serverTime); > return; > } > // Fetch changes from server. > + try { > + yield collection.sync(); I just remembered: we need to check for success here. Opening an issue and making a note so I know to do this when addressing other feedback. ::: services/common/KintoBlocklist.js:166 (Diff revision 1) > + try { > + yield collection.sync(); > + } catch (e) { > + if (e.message == INVALID_SIGNATURE) { > + yield collection.clear(); > - yield collection.sync(); > + yield collection.sync(); Again, check success.

Comment on attachment 8747195 [details] MozReview Request: Bug 1263602 - Verify kinto collection signatures using the content signature service. r=MattN, r=leplatrem https://reviewboard.mozilla.org/r/49755/#review46939 I'm sorry, I underlined a great number of nitpicks. But the most important improvements is with regards to sync result and the *«retry»* strategy. ::: services/common/KintoBlocklist.js:49 (Diff revision 1) > + return Cc["@mozilla.org/security/contentsignatureverifier;1"] > + .createInstance(Ci.nsIContentSignatureVerifier); > +} > + > +function mergeChanges(localRecords, changes) { > + const records = {}; nit: this declaration could be moved below the declaration of stripPrivateProps ::: services/common/KintoBlocklist.js:87 (Diff revision 1) > +function validateCollectionSignature(payload, collection) { > + return Task.spawn(function* () { > + // this is a content-signature field from an autograph response. > + const {x5u, signature} = yield fetchCollectionMetadata(collection); > + const certChain = yield fetch(x5u).then(function(res){return res.text()}); > + You can use short array function: ``.then((res) => res.text())`` ::: services/common/KintoBlocklist.js:91 (Diff revision 1) > + const certChain = yield fetch(x5u).then(function(res){return res.text()}); > + > + const localRecords = (yield collection.list()).data; > + const merged = mergeChanges(localRecords, payload.changes); > + const serialized = CanonicalJSON.stringify(merged); > + nit: can be moved on top to *group* the signature variables all together ::: services/common/KintoBlocklist.js:93 (Diff revision 1) > + const localRecords = (yield collection.list()).data; > + const merged = mergeChanges(localRecords, payload.changes); > + const serialized = CanonicalJSON.stringify(merged); > + > + let verifier = getSignatureVerifier(); > + nit: can be `const` ::: services/common/KintoBlocklist.js:97 (Diff revision 1) > + let verifier = getSignatureVerifier(); > + > + if (verifier.verifyContentSignature(serialized, "p384ecdsa="+signature, > + certChain, > + "oneCRL-signer.mozilla.org")) { > + // In case the hash is valid, apply the changes locally. The name `"oneCRL-signer.mozilla.org"` should come from parameters since it is specific to each KintoClient instance (make it a class attribute?) ::: services/common/KintoBlocklist.js:163 (Diff revision 1) > return; > } > // Fetch changes from server. > + try { > + yield collection.sync(); > + } catch (e) { As we have seen, the Kinto.js sync result object contains information about potential errors. http://kintojs.readthedocs.io/en/latest/api/#the-synchronization-result-object Here we should test that the sync was successful. ```js const result = yield collection.sync() if (!result.ok) { ... } ``` ::: services/common/KintoBlocklist.js:167 (Diff revision 1) > + yield collection.sync(); > + } catch (e) { > + if (e.message == INVALID_SIGNATURE) { > + yield collection.clear(); > - yield collection.sync(); > + yield collection.sync(); > + } else { What happens here if sync() fails again ? The collection will be empty. You can add a comment to mention that we may have to load from JSON on disk. ::: services/common/KintoBlocklist.js:170 (Diff revision 1) > + yield collection.clear(); > - yield collection.sync(); > + yield collection.sync(); > + } else { > + throw e; > + } > + } I wonder if it wouldn't be cleaner with a loop like: ```js while(!success && nbRetries > 0) { nbRetries--; try { const result = yield collection.sync(); success = result.ok; } catch(e) { if (e.message == INVALID_SIGNATURE) { yield collection.clear(); success = false; } } } ``` ::: services/common/tests/unit/test_kinto_signatures.js:74 (Diff revision 1) > + "last_modified": lastModified, > + "signature": { > + "x5u": `http://localhost:${port}/test_kinto_signatures/test_cert_chain.pem`, > + "public_key": "fake", > + "content-signature": "x5u=https://bucket.example.net/appkey1.pem;p384ecdsa=lJj7PfrLvvLcDBPBWQrV10rY5s1OlUAITx9UT-K_wzxmgEgS7vy8LzJQh5-rdpXHfZW5lKM5itpYwyscV9LkJSuVaozITP81_5zg8Pw6OifmqHcvBE81AtRv0r_eBVd0", > + "signature_encoding": "rs_base64url", don't you want to use `signature` here too ? ::: services/common/tests/unit/test_kinto_signatures.js:156 (Diff revision 1) > + > + > + // set some initial values so we can check these are updated appropriately > + Services.prefs.setIntPref(PREF_LAST_UPDATE, 0); > + Services.prefs.clearUserPref(PREF_LAST_ETAG); > + These look like like leftovers (no assertion later on) ::: services/common/tests/unit/test_kinto_signatures.js:239 (Diff revision 1) > + "Access-Control-Expose-Headers: Retry-After, Content-Length, Alert, Backoff", > + "Content-Type: application/json; charset=UTF-8", > + "Server: waitress" > + ], > + "status": {status: 200, statusText: "OK"}, > + "responseBody": JSON.stringify({"settings":{"cliquet.batch_max_requests":25}, "url":`http://localhost:${port}/v1/`, "documentation":"https://kinto.readthedocs.org/", "version":"1.5.1", "commit":"cbc6f58", "hello":"kinto"}) use `{"batch_max_requests":25}` instead. The cliquet prefix was deprecated ::: services/common/tests/unit/test_kinto_signatures.js:289 (Diff revision 1) > + const RESPONSE_TWO_ADDED = { > + "comment": "RESPONSE_TWO_ADDED", > + "sampleHeaders": [ > + "Content-Type: application/json; charset=UTF-8", > + "ETag: \"3000\"" > + ], nit: indentation ::: services/common/tests/unit/test_kinto_signatures.js:409 (Diff revision 1) > + > +function run_test() { > + // get a signature verifier to ensure nsNSSComponent is initialized > + Cc["@mozilla.org/security/contentsignatureverifier;1"] > + .createInstance(Ci.nsIContentSignatureVerifier); > + Is this still necessary since one of the first things done in the test is loading a verifier ?

Comment on attachment 8747195 [details] MozReview Request: Bug 1263602 - Verify kinto collection signatures using the content signature service. r=MattN, r=leplatrem https://reviewboard.mozilla.org/r/49755/#review47611 Here are some things from a quick skim but it seems like it would be best to review after Mathieu's issues are addressed. ::: services/common/KintoBlocklist.js:20 (Diff revision 1) > const { classes: Cc, interfaces: Ci, utils: Cu } = Components; > > Cu.import("resource://gre/modules/Services.jsm"); > const { Task } = Cu.import("resource://gre/modules/Task.jsm"); > const { OS } = Cu.import("resource://gre/modules/osfile.jsm"); > +Cu.importGlobalProperties(['fetch']); Nit: double quotes ::: services/common/KintoBlocklist.js:94 (Diff revision 1) > + const merged = mergeChanges(localRecords, payload.changes); > + const serialized = CanonicalJSON.stringify(merged); > + > + let verifier = getSignatureVerifier(); > + > + if (verifier.verifyContentSignature(serialized, "p384ecdsa="+signature, Nit: surround operators with spaces ::: services/common/KintoBlocklist.js:145 (Diff revision 1) > * @param {Date} serverTime the current date return by the server. > * @return {Promise} which rejects on sync or process failure. > */ > maybeSync(lastModified, serverTime) { > let db = kintoClient(); > - let collection = db.collection(this.collectionName); > + let collection = db.collection(this.collectionName, { hooks : { Nit: No space before the `:` ::: services/common/tests/unit/test_kinto_signatures.js:1 (Diff revision 1) > +/* 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"; Nit: tests are normally PD licensed and they are now PD by default so you can remove the header. ::: services/common/tests/unit/test_kinto_signatures.js:13 (Diff revision 1) > + > +var server; > + Always use `let` instead of `var`

https://reviewboard.mozilla.org/r/49755/#review46939 > I wonder if it wouldn't be cleaner with a loop like: > > ```js > > while(!success && nbRetries > 0) { > nbRetries--; > try { > const result = yield collection.sync(); > success = result.ok; > } catch(e) { > if (e.message == INVALID_SIGNATURE) { > yield collection.clear(); > success = false; > } > } > } > ``` I don't think that retrying more than pnce gives us any benefit. Another sync will be retried in 24h in any case - and if the signature on the remote collection is bad, retrying won't help us at all. We should: * attempt to sync * If the signature is bad, attempt to fetch the remote collection * if that signature is bad, forget about it for today * if that signature is good, clear local collection and loadDump from the complete remote collection > Is this still necessary since one of the first things done in the test is loading a verifier ? Yes. We need to ensure that nsNSSComponent is initialized before the root pref is set.

Comment on attachment 8747195 [details] MozReview Request: Bug 1263602 - Verify kinto collection signatures using the content signature service. r=MattN, r=leplatrem Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49755/diff/1-2/

The renaming of some files (KintoBlocklist.js and the tests for this bug) have played havoc with the reviewboard interdiff. My apologies for this!

Comment on attachment 8747195 [details] MozReview Request: Bug 1263602 - Verify kinto collection signatures using the content signature service. r=MattN, r=leplatrem https://reviewboard.mozilla.org/r/49755/#review52438 ::: services/common/blocklist-clients.js:159 (Diff revision 2) > */ > maybeSync(lastModified, serverTime) { > let db = kintoClient(); > - let collection = db.collection(this.collectionName); > + let collection = db.collection(this.collectionName, { hooks: { > + "incoming-changes": [this.validateCollectionSignature.bind(this)] > + }}); nitpick: we could add this hook only if `this.signerSAN` is defined.

It occurs to me, we should also add a pref that allows us to turn this off for testing purposes (and land with the feature disabled initially).

Comment on attachment 8747195 [details] MozReview Request: Bug 1263602 - Verify kinto collection signatures using the content signature service. r=MattN, r=leplatrem Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49755/diff/2-3/

Comment on attachment 8747195 [details] MozReview Request: Bug 1263602 - Verify kinto collection signatures using the content signature service. r=MattN, r=leplatrem https://reviewboard.mozilla.org/r/49755/#review53318 ::: services/common/blocklist-clients.js:44 (Diff revision 3) > +function getSignatureVerifier() { > + return Cc["@mozilla.org/security/contentsignatureverifier;1"] > + .createInstance(Ci.nsIContentSignatureVerifier); > +} Why is this a function when it's used once? ::: services/common/blocklist-clients.js:54 (Diff revision 3) > +function mergeChanges(localRecords, changes) { > + // Kinto.js adds attributes to local records that aren't present on server. > + // (e.g. _status) > + const stripPrivateProps = (obj) => { > + return Object.keys(obj).reduce((current, key) => { > + if (key.indexOf("_") !== 0) { Nit: String.prototype.startsWith will help with readability. ::: services/common/blocklist-clients.js:67 (Diff revision 3) > + return Object.values(records) > + // Filter out deleted records. > + .filter((record) => record.deleted != true) > + // Sort list by record id. > + .sort((a, b) => a.id < b.id ? -1 : a.id > b.id ? 1 : 0); What data type is the ID? A Number or String? If it's a number you can simply do `a.id - b.id` ::: services/common/blocklist-clients.js:87 (Diff revision 3) > + > +function fetchRemoteCollection(collection) { > + const client = new KintoHttpClient(collection.api.remote); > + return client.bucket(collection.bucket) > + .collection(collection.name) > + .listRecords({"sort": "id"}); Nit: No need to quote "sort" ::: services/common/blocklist-clients.js:115 (Diff revision 3) > - constructor(collectionName, lastCheckTimePref, processCallback) { > + constructor(collectionName, lastCheckTimePref, processCallback, signerSAN) { > this.collectionName = collectionName; > this.lastCheckTimePref = lastCheckTimePref; > this.processCallback = processCallback; > + this.signerSAN = signerSAN; 1) It's not clear what signerSAN is as I don't think SAN is a common-enough acronym. 2) I'm not sure SAN is even correct since it's not clear to me whether the argument needs to be a SAN or whether it could be a CN. The argument to `verifyContentSignature` is just named "name" and doesn't mention SAN. ::: services/common/tests/unit/test_blocklist_signatures.js:61 (Diff revision 3) > + return { > + "data": { > + "id": "certificates", > + "last_modified": lastModified, > + "signature": { > + "x5u": `http://localhost:${port}/test_blocklist_signatures/test_cert_chain.pem`, > + "public_key": "fake", > + "content-signature": `x5u=http://localhost:${port}/test_blocklist_signatures/test_cert_chain.pem;p384ecdsa=${signature}`, > + "signature_encoding": "rs_base64url", > + "signature": signature, > + "hash_algorithm": "sha384", > + "ref": "1yryrnmzou5rf31ou80znpnq8n" > + } Only quote object literal property names when necessary (e.g. if they contain a hyphen) throughout this file.

https://reviewboard.mozilla.org/r/49755/#review53332 ::: services/common/tests/unit/test_blocklist_signatures/moz.build:7 (Diff revision 3) > +# Temporarily disabled. See bug 1256495. > +#test_certificates = ( This bug has since been resolved so I guess this will change?

Comment on attachment 8747195 [details] MozReview Request: Bug 1263602 - Verify kinto collection signatures using the content signature service. r=MattN, r=leplatrem Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49755/diff/3-4/

Comment on attachment 8747195 [details] MozReview Request: Bug 1263602 - Verify kinto collection signatures using the content signature service. r=MattN, r=leplatrem Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49755/diff/4-5/

Thanks for your reviews. I've addressed feedback - I just wanted to mention I've added another test. I noticed there was no test that would actually fail if signature verification were not being enforced, or to ensure that on a sync failure (due to a signature issue) would not corrupt or empty the local collection. This test can be found in test_blocklist_signatures.js (see allBadSigResponses, etc.). Please let me know if there are any issues. Also, the signerName value didn't reflect the SAN that services ops are actually putting in the signer certs so I updated that value.

Comment on attachment 8747195 [details] MozReview Request: Bug 1263602 - Verify kinto collection signatures using the content signature service. r=MattN, r=leplatrem Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49755/diff/5-6/

https://reviewboard.mozilla.org/r/49755/#review53318 > What data type is the ID? A Number or String? If it's a number you can simply do `a.id - b.id` It's a string.

https://reviewboard.mozilla.org/r/49755/#review53732 ::: services/common/tests/unit/test_blocklist_signatures.js:243 (Diff revisions 3 - 6) > - "sampleHeaders": [ > + sampleHeaders: [ > "Content-Type: application/json; charset=UTF-8", > "ETag: \"1000\"" > ], > - "status": {status: 200, statusText: "OK"}, > - "responseBody": JSON.stringify({"data": []}) > + status: {status: 200, statusText: "OK"}, > + responseBody: JSON.stringify({"data": []}) Nit: You still have quotes around "data" throughout this file.

I skimmed the interdiff https://reviewboard.mozilla.org/r/49755/diff/3-6/ and it looks fine. I'll leave a more detailed review of the test to Mathieu.

This looks very good! Ship it! Congratz Mark, that's a huge step forward that had been on the drawing board for a long time!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b83a48d5fab9a296f1522b7e00d31bcb5eb102d4 Bug 1263602 - Verify kinto collection signatures using the content signature service. r=MattN, r=leplatrem