1227956 - Implement Kinto.js OneCRL client

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect)

Description

[Mark Goodwin [:mgoodwin]]

Implement a client that uses Kinto instead of the addons blocklist to do the OneCRL data sync.

Kinto has a mechanism which allows a client to discover the most recent version of each collection in a given bucket. Because of this, we can have an updater which pings periodically to discover the current version of all collections, then hands that version information to individual clients that know how to fetch and handle that data.

The clients simply expose a method like maybeSync(lastModified) which the updater invokes when it has data.

This is one such client.

Comment 1

[Mark Goodwin [:mgoodwin]]

Attachment: MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

Bug 1227956 - Implement Kinto.js OneCRL client r?mossop

Comment 2

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8691949 [details]
MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

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

Comment 3

[Mark Goodwin [:mgoodwin]]

I'm pretty sure this ultimately shouldn't live in toolkit/mozapps/extensions - but while we're migrating from nsBlocklistService it kind of makes sense if all of the state-updater bits are together.

Comment 4

[Dave Townsend [:mossop]]

(In reply to Mark Goodwin [:mgoodwin] from comment #3)
> I'm pretty sure this ultimately shouldn't live in toolkit/mozapps/extensions
> - but while we're migrating from nsBlocklistService it kind of makes sense
> if all of the state-updater bits are together.

I'd it shouldn't live here long-term and I'd rather we just put it in its real home now. Tracking file moves in hg isn't too bad but hgweb still makes it harder than it needs to be so if we know something should be somewhere else to begin with we should put it there.

So should this go under services somewhere? Does that get me off the hook for reviewing it? ;)

Comment 5

[Mark Goodwin [:mgoodwin]]

(In reply to Dave Townsend [:mossop] from comment #4)
> So should this go under services somewhere? 

Maybe services. Or maybe security/manager somewhere (though we have no JS there currently, not that that's a good reason). On the one hand, this uses services stuff. On the other, it's feeding PSM code.

Keeler, Richard; thoughts?

> Does that get me off the hook for reviewing it? ;)

I guess so :)

Comment 6

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

I think maybe services is a good idea. Similar code is already there (in particular, moz-kinto-client.js itself).

Comment 7

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8691949 [details]
MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

Would you like to take a first pass at this before I move it to /services/common?

Comment 8

[Richard Newman [:rnewman]]

(In reply to Mark Goodwin [:mgoodwin] from comment #7)
> Comment on attachment 8691949 [details]
> MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?mossop
> 
> Would you like to take a first pass at this before I move it to
> /services/common?

Nah, move it, then ping me with an updated MozReview request! Make sure all the tests pass in its final destination and moz.build settings…

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8691949 [details]
MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

Reminder to re-ping after moving.

Comment 10

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8691949 [details]
MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

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

Comment 11

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a908c749cce1

Comment 12

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58cb797c0a21

Comment 13

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8691949 [details]
MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

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

Comment 14

[Mark Goodwin [:mgoodwin]]

(In reply to Richard Newman [:rnewman] from comment #9)
> Reminder to re-ping after moving.

ping!

Comment 15

[Mark Goodwin [:mgoodwin]]

Gentle ping...

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8691949 [details]
MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

https://reviewboard.mozilla.org/r/26211/#review25769

::: services/common/KintoCertificateBlocklist.js:35
(Diff revision 4)
> +    validate: function(id) {

`validate: RE_UUID.test,` probably suffices?

::: services/common/KintoCertificateBlocklist.js:75
(Diff revision 4)
> +      let now = Math.round(Date.now() / 1000);

I have a very strong preference that this timestamp be supplied by the server, ideally exactly matching the timestamps in the data.

Client clocks are always wrong, and so you'll have some percentage of users who will go for long periods of time without checking.

::: services/common/KintoCertificateBlocklist.js:93
(Diff revision 4)
> +            certList.revokeCertByIssuerAndSerial(item.issuerName,

Can these operations fail?

If they fail, what should we do?

This looks pretty good, but I think the use of timestamps needs to be rethought.

Comment 17

[Richard Newman [:rnewman]]

A thousand apologies for the delayed review. Things have been hectic.

Comment 18

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8691949 [details]
MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

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

Comment 19

[Mark Goodwin [:mgoodwin]]

https://reviewboard.mozilla.org/r/26211/#review25769

> `validate: RE_UUID.test,` probably suffices?

Not quite; though RE_UUID.test.bind(RE_UUID) will?

> Can these operations fail?
> 
> If they fail, what should we do?

They can - and if they do they'll throw - which means updateLastCheck() is not called (so we'll try again next time maybeSync is called).

Comment 20

[Mark Goodwin [:mgoodwin]]

Gentle ping.

Comment 21

[Richard Newman [:rnewman]]

Comment on attachment 8691949 [details]
MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

https://reviewboard.mozilla.org/r/26211/#review30629

::: services/common/KintoCertificateBlocklist.js:24
(Diff revision 5)
> +const PREF_KINTO_ONECRL_CHECKED = "services.kinto.onecrl.checked";

This is in seconds since, epoch, right? If so, let's put `_SECONDS` in here.

::: services/common/KintoCertificateBlocklist.js:47
(Diff revision 5)
> +  this.maybeSync = function(lastModified, serverTime) {

Comment that both of these are in milliseconds since epoch, and both are server timestamps?

::: services/common/KintoCertificateBlocklist.js:56
(Diff revision 5)
> +    let bucket = Services.prefs.getCharPref(PREF_KINTO_BUCKET);

Might as well move these two lines to the start of `maybeSync` -- if the prefs are missing they'll throw.

::: services/common/KintoCertificateBlocklist.js:63
(Diff revision 5)
> +      adapter:FirefoxAdapter

Nit: space after colon, add trailing comma.

::: services/common/KintoCertificateBlocklist.js:73
(Diff revision 5)
> +      let now = Math.round(serverTime / 1000);

`now` -> `checkedServerTimeInSeconds`

::: services/common/KintoCertificateBlocklist.js:83
(Diff revision 5)
> +        if (lastModified <= collectionLastModified) {

If the server modified time is earlier than local, rather than being the same, does that mean something terrible has happened?

::: services/common/tests/unit/test_kintoCertBlocklist.js:74
(Diff revision 5)
> +  let result = yield OneCRLClient.maybeSync(1448378812000, Date.now());

I have a gentle preference for making these fixed and obviously-not-local values (e.g., from very long ago).

Comment 22

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8691949 [details]
MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

I can't f? on mozreview, so trying here.

Comment 23

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

https://reviewboard.mozilla.org/r/26211/#review30685

Seems reasonable.

::: services/common/tests/unit/test_kintoCertBlocklist.js:175
(Diff revision 5)
> +        "serialNumber":"ATFpsA==",

Maybe also have a test for subject/public key hash?

Comment 24

[Mark Goodwin [:mgoodwin]]

https://reviewboard.mozilla.org/r/26211/#review30629

> If the server modified time is earlier than local, rather than being the same, does that mean something terrible has happened?

Not currently - and when it does, it'll be because we have collection signatures (which would take precedence over time in any case).

Comment 25

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f2d174df2b1

Comment 26

[Mark Goodwin [:mgoodwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56e66f43d7ee4e52da7fc7add39ca86743225cc2
Bug 1227956 - Implement Kinto.js OneCRL client r=rnewman

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56e66f43d7ee