1359428 - Remove preference to select OneCRL update mechanism

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[Mathieu Leplatre [:leplatrem]]

In Bug 1224467 a preference was introduced to migrate OneCRL from the blocklist XML to the new remote settings solution (aka. Kinto).

The new system has been default for a while now.

In order to avoid having to test many preferences combinations when working on Bug 1257565, I suggest that we get rid of this preference — it is very unlikely that we switch back to the XML system (:mgoodwin agrees).

Comment 1

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 1359428 - Remove preference to select OneCRL update mechanism

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

Comment 2

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8861481 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133458/diff/1-2/

Comment 3

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8861481 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133458/diff/2-3/

Comment 4

[Mathieu Leplatre [:leplatrem]]

I made my best to rewrite ``security/manager/ssl/tests/unit/test_cert_blocklist.js`` to rely on JSON instead of XML. Unfortunately I couldn't go through everything.

Mainly two things:

- With XML there were a way to have several "serialNumber" children. With JSON, it does not make sense anymore IMO. But some tests still assert some behaviour around that.
- When relying on ``blocklist.xml``, we knew that when the "blocklist-updated" event was received, the whole XML had been processed. When relying on Kinto, we have no event that tells when synchronization and its ``updateCertBlocklist()`` callback is finished.

Mark, would you have some time to adjust the test suite and land?

Thanks a lot!

Comment 5

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8861481 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

https://reviewboard.mozilla.org/r/133458/#review138496

This looks good to me; it might be worth getting a review from Keeler too (since you're touching some PSM code and tests).

Comment 6

[Mark Goodwin [:mgoodwin]]

(In reply to Mathieu Leplatre (:leplatrem) from comment #4)
> Mark, would you have some time to adjust the test suite and land?

I misread; yes. I can do this.

Comment 7

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8861481 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133458/diff/3-4/

Comment 8

[Mathieu Leplatre [:leplatrem]]

Mark,

I rebased and commented out the failing parts of the tests.

I tried my best to fix them myself but I got lost :|

If you have some bandwidth, I'd gladly welcome help here :)

Thanks!

Comment 9

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8861481 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133458/diff/4-5/

Comment 10

[Andrew Overholt [:overholt]]

This blocks a Quantum Flow P1 bug and we're wondering what the status is. Any updates you can share? Thank you.

Comment 11

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1359428 - Remove preference to select OneCRL update mechanism

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

Comment 12

[Mark Goodwin [:mgoodwin]]

Mat; I've made the necessary changes to the tests but I can't review these myself. I r? keeler since the majority of the changes are in test_cert_blocklist.js and I r+ed the rest in https://reviewboard.mozilla.org/r/133458/diff/5#index_header (reviewboard apparently isn't able to reconcile the two as a single set of changes).

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8904577 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

https://reviewboard.mozilla.org/r/176422/#review181394

LGTM - just some minor fixups.

::: security/manager/ssl/CertBlocklist.cpp:649
(Diff revision 1)
>    auto blocklist = static_cast<CertBlocklist*>(aClosure);
>    MutexAutoLock lock(blocklist->mMutex);
>  
>    MOZ_LOG(gCertBlockPRLog, LogLevel::Warning,
>           ("CertBlocklist::PreferenceChanged %s changed", aPref));
> -  if (strcmp(aPref, PREF_BACKGROUND_UPDATE_TIMER) == 0) {
> +  if (strcmp(aPref, PREF_BLOCKLIST_ONECRL_CHECKED) == 0) {

Is the PREF_BACKGROUND_UPDATE_TIMER pref still relevant? (If not, we should probably at least stop observing it, right?)

::: security/manager/ssl/tests/unit/test_cert_blocklist.js:46
(Diff revision 1)
>                 .createInstance(Ci.nsIFileOutputStream);
>  stream.init(blockFile,
>    FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE | FileUtils.MODE_TRUNCATE,
>    FileUtils.PERMS_FILE, 0);
>  
> -var data = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
> +var emptyBlocklistXML = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +

Do we even still need to serve an empty blocklist.xml file?

::: security/manager/ssl/tests/unit/test_cert_blocklist.js:121
(Diff revision 1)
> -    // In this case, the issuer name and the valid serialNumber correspond
> +  // In this case, the issuer name and the valid serialNumber correspond
> -    // to other-test-ca.pem in bad_certs/ (for testing root revocation)
> +  // to other-test-ca.pem in bad_certs/ (for testing root revocation)
> -    "<certItem issuerName='MBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E='>" +
> -    "<serialNumber>exJUIJpq50jgqOwQluhVrAzTF74=</serialNumber></certItem>" +
> -    // This item corresponds to an entry in sample_revocations.txt where:
> +  ` {
> +      "id": "6",
> +      "last_modified": 100000000000000000005,
> +      "issuerName": "MBIxEDAOBgNVBAMMB1Rlc3QgQ0E=",

Shouldn't this be "MBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E="?

::: security/manager/ssl/tests/unit/test_cert_blocklist.js:150
(Diff revision 1)
> -    " pubKeyHash='VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8='>" +
> -    "</certItem></certItems></blocklist>";
> -
> -var updatedBlocklist = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" +
> -    "<blocklist xmlns=\"http://www.mozilla.org/2006/addons-blocklist\">" +
> -    "<certItems>" +
> +      "id": "9",
> +      "last_modified": 100000000000000000007,
> +      "subject": "MCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5",
> +      "pubKeyHash": "VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8="
> +    },` +
> +  // This item revokes other-issuer-ee.pem by issuer and serial number.

If the above entry is fixed, I don't think this one is necessary.

::: services/common/blocklist-updater.js:13
(Diff revision 1)
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.importGlobalProperties(["fetch"]);
>  XPCOMUtils.defineLazyModuleGetter(this, "UptakeTelemetry",
>                                    "resource://services-common/uptake-telemetry.js");

I suppose you'll need a services/toolkit peer to review these files.

Comment 14

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8904577 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

https://reviewboard.mozilla.org/r/176422/#review181706

::: services/common/blocklist-updater.js:21
(Diff revision 1)
>  const PREF_SETTINGS_SERVER_BACKOFF      = "services.settings.server.backoff";
>  const PREF_BLOCKLIST_CHANGES_PATH       = "services.blocklist.changes.path";
>  const PREF_BLOCKLIST_LAST_UPDATE        = "services.blocklist.last_update_seconds";
>  const PREF_BLOCKLIST_LAST_ETAG          = "services.blocklist.last_etag";
>  const PREF_BLOCKLIST_CLOCK_SKEW_SECONDS = "services.blocklist.clock_skew_seconds";
> +const PREF_BLOCKLIST_LOAD_DUMP          = "services.blocklist.load_dump";

Leftover?

::: services/common/blocklist-updater.js:182
(Diff revision 1)
>    // Save current Etag for next poll.
>    if (currentEtag) {
>      Services.prefs.setCharPref(PREF_BLOCKLIST_LAST_ETAG, currentEtag);
>    }
> +
> +  Services.obs.notifyObservers(null, "blocklist-updater-versions-checked");

Could you please add an explicit mention of this in `test_blocklist_updater.js`?
I know it's tested in `test_cert_blocklist.js` but I believe it's a good practice when each module feature has specific assertions in its test file :)

Comment 15

[Mark Goodwin [:mgoodwin]]

(In reply to Mathieu Leplatre (:leplatrem) from comment #14)
> > +const PREF_BLOCKLIST_LOAD_DUMP          = "services.blocklist.load_dump";
> 
> Leftover?

I thought this was one of your changes...

> Could you please add an explicit mention of this in
> `test_blocklist_updater.js`?
> I know it's tested in `test_cert_blocklist.js` but I believe it's a good
> practice when each module feature has specific assertions in its test file :)

Good call. I'll add an explicit test in there.

Comment 16

[Mark Goodwin [:mgoodwin]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #13)
> > -var data = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
> > +var emptyBlocklistXML = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
> 
> Do we even still need to serve an empty blocklist.xml file?

No. But we do need to keep the clobbering of extensions.blocklist.url pref around - for now.

Comment 17

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8904577 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176422/diff/1-2/

Comment 18

[Robert Helmer [:rhelmer]]

Comment on attachment 8904577 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

https://reviewboard.mozilla.org/r/176422/#review187660

Comment 19

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8904577 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176422/diff/2-3/

Comment 20

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8904577 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176422/diff/3-4/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a889835e4003
Remove preference to select OneCRL update mechanism r=keeler,leplatrem,rhelmer

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a889835e4003