Implement Kinto.js OneCRL client

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mgoodwin, Assigned: mgoodwin)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8691949 [details]
MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

Bug 1227956 - Implement Kinto.js OneCRL client r?mossop
Attachment #8691949 - Flags: review?(dtownsend)
(Assignee)

Updated

3 years ago
Blocks: 1227970
(Assignee)

Comment 2

3 years ago
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/
(Assignee)

Comment 3

3 years ago
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.
(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? ;)
(Assignee)

Comment 5

3 years ago
(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 :)
Flags: needinfo?(rnewman)
Flags: needinfo?(dkeeler)
I think maybe services is a good idea. Similar code is already there (in particular, moz-kinto-client.js itself).
Flags: needinfo?(dkeeler)
(Assignee)

Comment 7

3 years ago
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?
Attachment #8691949 - Flags: review?(dtownsend) → feedback?(rnewman)
(Assignee)

Updated

3 years ago
Component: Security: PSM → Firefox: Common
Product: Core → Cloud Services
(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…
Status: NEW → ASSIGNED
Flags: needinfo?(rnewman)
OS: Unspecified → All
Hardware: Unspecified → All
Comment on attachment 8691949 [details]
MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman

Reminder to re-ping after moving.
Flags: needinfo?(mgoodwin)
Attachment #8691949 - Flags: feedback?(rnewman)
(Assignee)

Comment 10

3 years ago
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/
Attachment #8691949 - Attachment description: MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?mossop → MozReview Request: Bug 1227956 - Implement Kinto.js OneCRL client r?rnewman
Attachment #8691949 - Flags: review?(rnewman)
(Assignee)

Comment 13

3 years ago
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/
(Assignee)

Comment 14

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

ping!
Flags: needinfo?(mgoodwin) → needinfo?(rnewman)
(Assignee)

Comment 15

3 years ago
Gentle ping...
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.
Attachment #8691949 - Flags: review?(rnewman)
A thousand apologies for the delayed review. Things have been hectic.
Flags: needinfo?(rnewman)
(Assignee)

Comment 18

3 years ago
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/
Attachment #8691949 - Flags: review?(rnewman)
(Assignee)

Comment 19

3 years ago
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).
(Assignee)

Comment 20

3 years ago
Gentle ping.
Flags: needinfo?(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).
Attachment #8691949 - Flags: review?(rnewman) → review+
(Assignee)

Comment 22

3 years ago
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.
Attachment #8691949 - Flags: feedback?(dkeeler)
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?
Attachment #8691949 - Flags: feedback?(dkeeler) → feedback+
(Assignee)

Comment 24

3 years ago
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).
Flags: needinfo?(rnewman)

Comment 27

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/56e66f43d7ee
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Blocks: 1247662
See Also: → bug 1257565

Updated

3 years ago
Blocks: 1259947
You need to log in before you can comment on or make changes to this bug.