Closed Bug 1359428 Opened 7 years ago Closed 6 years ago

Remove preference to select OneCRL update mechanism

Categories

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

Product:
Core
Component:
Security: PSM
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: leplatrem, Assigned: mgoodwin)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file, 1 obsolete file)

Bug 1359428 - Remove preference to select OneCRL update mechanism
59 bytes, text/x-review-board-request
rhelmer
: review+
Details 
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).
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!
Flags: needinfo?(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).
Attachment #8861481 - Flags: review?(mgoodwin) → review+
(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.
Flags: needinfo?(mgoodwin)
Assignee: mathieu → mgoodwin
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!
Priority: -- → P1
Whiteboard: [psm-assigned]
This blocks a Quantum Flow P1 bug and we're wondering what the status is. Any updates you can share? Thank you.
Flags: needinfo?(mgoodwin)
Attachment #8861481 - Attachment is obsolete: true
Flags: needinfo?(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).
Flags: needinfo?(mathieu)
Flags: needinfo?(dkeeler)
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.
Attachment #8904577 - Flags: review?(dkeeler) → review+
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 :)
Attachment #8904577 - Flags: review+
Flags: needinfo?(mathieu)
(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.
(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.
Attachment #8904577 - Flags: review?(MattN+bmo) → review?(dtownsend)
Attachment #8904577 - Flags: review?(dtownsend) → review?(rhelmer)
Comment on attachment 8904577 [details]
Bug 1359428 - Remove preference to select OneCRL update mechanism

https://reviewboard.mozilla.org/r/176422/#review187660
Attachment #8904577 - Flags: review?(rhelmer) → review+
Pushed by mgoodwin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a889835e4003
Remove preference to select OneCRL update mechanism r=keeler,leplatrem,rhelmer
https://hg.mozilla.org/mozilla-central/rev/a889835e4003
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1404577
Blocks: 1435609
You need to log in before you can comment on or make changes to this bug.