1437671 - Apply content signing to Tippytop in Activity Stream

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P1)

Description

[Nan Jiang [:nanj]]

This intends to add an extra layer of protection to Tippytop service, which serves hi-res icons for Topsites in Activity Stream.

We already have an in-house signing service called Autograph running and being maintained by the Security team, see https://github.com/mozilla-services/autograph. Both Normandy and Kinto are active users of this service.

Content signing wasn't new to us either, Splice used to leverage Autograph to secure the remote newtab page, and we know exactly what the benefit is from exercising it.

It makes sense to equip Activity Stream with content signing, particularly for those resources being hosted and served from outside of Firefox. As a first step, I propose to enable it to secure the Tippytop feed. Specifically, we can break this work down into following steps.

1) Update the Splice content signing module to match the latest Autograph. Per ulfr, this should be just some slight changes to Splice.
2) Sign each version of Tippytop manifest via Splice, and serve the signed content to all AS users.
3) Add content signature verification to AS. We can reference Kinto and Normandy on this. See https://searchfox.org/mozilla-central/search?q=symbol:%23verifyContentSignature&redirect=false

Tippytop is a relatively simple external feed than other bigger ones in AS, such as Pocket. Once we get it working and smooth out all the rough edges, we can then apply content signing to other feeds accordingly.

Comment 1

[Julien Vehent]

Nan: I'll be going on leave for 6 weeks soon, so I'm cc-ing Greg Guthe who can help you with this. Do you want to meet over the next week to kick this off?

Comment 2

[Ed Lee :Mardak]

Sounds like one possible approach is to put tippy top data with RemoteSettings where content is signed.

Comment 3

[Nan Jiang [:nanj]]

Julien: sounds good. We can grab a chat to touch base with Greg before your leave. As a quick update, we've been working with the RemoteSetting folks on migrating TippyTop manifest to RemoteSetting. We believe that leveraging RemoteSettings makes a lot more sense than rolling out another content signing/management service for Activity Stream.

Comment 4

[Nan Jiang [:nanj]]

leplatrem: we're also facing a similar situation while migrating TippyTop to RemoteSettings as you commented at 
https://bugzilla.mozilla.org/show_bug.cgi?id=1083971#c14.

So the current TippyTop manifest, about 5 MB after compression, is a single JSON file with approx. 1000 entries. Usually we update this manifest every two months.

After studying the documents of RemoteSettings, we're looking to create one record in the TippyTop bucket on RemoteSettings, and store the version of the manifest as the value of the record, then treat the manifest file as the attachment of this record. The upside of doing so is that it resembles the current workflow (i.e. bulk update), and only requires a few minor changes in Activity Stream on the client side.

A few questions about the attachment:
* does it have 8 MB hard limit on the size?
* is it also signed and verified upon upload & download? 

Other reasons of using attachment other than breaking it down into individual records are:
* we barely update the individual values of the TippyTop manifest, rather we just periodically update all of them as a whole
* the multi-record mechanism would need some significant code changes on the client side

Does that make sense to you?

Also, I was curious about how blocklist handled this, I've noticed that it also maintains a wide range of items on RemoteSettings. Could you shed some light on that for me please?

Comment 5

[Julien Vehent]

> Usually we update this manifest every two months.

This may sound silly, but why not check it in tree and follow the 6 weeks firefox release schedule if you're only updating it every 10 weeks?

Comment 6

[Ed Lee :Mardak]

There have been issues in the past where it was valuable to be able to adjust/remove entries sooner without requiring a dot release.

Comment 7

[Mathieu Leplatre [:leplatrem]]

> After studying the documents of RemoteSettings, we're looking to create one record in the TippyTop bucket on 
> RemoteSettings, and store the version of the manifest as the value of the record

That's not the ideal approach for optimal syncing, but OK... 

BTW I don't think that storing the version is necessary since RemoteSettings already provides one (last_modified)

> A few questions about the attachment:
> * does it have 8 MB hard limit on the size?
> * is it also signed and verified upon upload & download? 

The upload size limit can be increased if necessary. The only limit is on the NGinx in front the writer instance I think.

The record on which the file is attached will have some metadata like the md5sum.
Since the collection that contains the record is signed, the content attachment is indirectly signed.

We verify nothing during upload (VPN only) and you will be in charge of verifying the md5sum after download (as well as managing disk space, resuming interrupted downloads etc.). We would obviously be very interested to have some helpers in services/settings/ for that :)

> we barely update the individual values of the TippyTop manifest, rather we just periodically update all of them as a whole

Does it mean all records change? You could a job that only overwrites the records that have changed, couldn't you?

> the multi-record mechanism would need some significant code changes on the client side

I would be interested to see how it affects your client code. We might face this same situation in other contexts and it could be worth adjusting the API if necessary.

Comment 8

[Nan Jiang [:nanj]]

(In reply to Mathieu Leplatre (:leplatrem) from comment #7)

> BTW I don't think that storing the version is necessary since RemoteSettings
> already provides one (last_modified)

Speaking of the versioning, how do we handle the version changes in RemoteSettings? Say if we were to change the format of TippyTop manifest and need to bump up the version. Assuming that we can do so by creating a new collection (or bucket?) in RemoteSettings for the new version, and leave the old one around to serve the users with the old version. Is there anything else needed on the client side? Such as wiping out the local data of the old version upon update.    

> Does it mean all records change? You could a job that only overwrites the
> records that have changed, couldn't you?

Not really, but currently we just update the whole manifest even if there is only one line change in it.
 
> I would be interested to see how it affects your client code. We might face
> this same situation in other contexts and it could be worth adjusting the
> API if necessary.

The current TippyTop Service in Activity Stream works as follows:
1. it loads the manifest to memory when A-S initializes. If it's missing, it'll fetch it from remote
2. it periodically checks the status (by comparing the S3 Etag) of manifest hosted on S3 
3. once a new version is found, it downloads the new manifest, then replaces the old one in memory, and persists it to a local file for the future use

So if we keep the current manifest format, and use the attachment feature, we would just need to change the step 2 by using the onChange feature of RemoteSettings. The rest remains pretty much the same.

Alternatively, if we break the manifest down for the optimal syncing, I believe we can get rid of the copy in memory in step 1 as well as the persisted file in step 3, and the lookup code would look like

const data = await RemoteSettings("some-key").get();

Is there any performance or disk impact in Firefox for having thousands "keys" in a RemoteSettings bucket?

Comment 9

[Nan Jiang [:nanj]]

Julien: looks like we still need a signer for this on RemoteSettings :)

Although looks like all the signers for blocklists are sharing the same value "remote-settings.content-signature.mozilla.org". Can we just follow the same pattern here? I.e. add a new pref "browser.newtabpage.activity-stream.tippytop.signer", and assign "remote-settings.content-signature.mozilla.org" to it.

Comment 10

[Mathieu Leplatre [:leplatrem]]

> Julien: looks like we still need a signer for this on RemoteSettings :)

If you use Remote Settings you don't have to manage signers. The blocklist code is two years old and has its specificities. It should not be a model for the new stuff ;)


> Speaking of the versioning, how do we handle the version changes in RemoteSettings? 

You should see remote settings as a pipe. We are in charge of moving data from the server to the clients. We don't manage domain specific stuff like schema versioning etc.
Obviously if your schema change is backward compatible you don't have to do anything. If not, then indeed you can create another collection, and modify the client to fetch from it. The local data is isolated by collection, so in that case nothing else would have to be done.

> Not really, but currently we just update the whole manifest even if there is only one line change in it.

Based on what you described in comment #8, I would really recommend to split into individual records.

- The job that updates the records on the server can only update the records that changed
- If you have thousands of records for a few megabytes, we could even try to base64 them directly in the JSON
- You could indeed lookup individual entries using .get() and filters (see API docs [0]):
    ``RemoteSettings("tippytop").get({ filters: { domain: "website.org" } });
- Having a thousand keys is not an issue, the synchronization and IndexedDB can handle that easily
- And it would not prevent you to rebuild the single big manifest if you like, with something like this:

```
    RemoteSettings("tippytop").on("sync", async event => {
      const {
        data: { created, updated, deleted }
      } = event;
      // Load current manifest
      const manifest = await Manifest.load();
      // Download every new/updated icon
      const toDownload = created.concat(updated.map(u => u.new));
      const newIcons = await Promise.all(
        toDownload.map(async record => {
          const icon = await downloadIcon(record.attachment);
          return { record, icon };
        })
      );
      // Update the manifest with the content
      newIcons.forEach(({ record, icon }) => manifest.save(record.id, icon));
      // Remove every removed icon
      const toRemove = deleted.concat(updated.map(u => u.old));
      toRemove.forEach(({ record }) => manifest.remove(record.id));
    });

```



[0] https://firefox-source-docs.mozilla.org/main/latest/services/common/docs/services/RemoteSettings.html

Comment 11

[Nan Jiang [:nanj]]

Thanks for those insights, :leplatrem! That's very helpful.

With your clarifications above as well as referencing the blocklist-client's way of managing multiple blocklists in Remote Settings, I agree that working with individual records is more favorable than the opposite.

Will give it a try in stage with this game plan. Stay tuned.

Comment 12

[Julien Vehent]

+1 to everything :leplatrem said, we don't need a custom signer for tippytop

Comment 13

[Nan Jiang [:nanj]]

:leplatrem is there an endpoint for "bulk create" in RemoteSettings by any chance? Otherwise we'll have to create the record one by one like,

```
for record in records
do
  curl --request POST $SERVER/v1/buckets/main-workspace/collections/tippytop/records   \
      -d '{"data":"$record"}' -H 'Content-Type:application/json'                       \
      -u '[e-mail]:your-pass'
end
```

It'd be nice to have such feature when dealing with a large number of records for a collection.

Comment 14

[Mathieu Leplatre [:leplatrem]]

Yes, there is :)

You can POST a list of requests on /v1/batch as explained here:
http://kinto.readthedocs.io/en/stable/api/1.x/batch.html

Using the Python or Javascript clients brings some convenience:

https://github.com/Kinto/kinto-http.py/#batching-operations
https://github.com/Kinto/kinto-http.js/#batching-operations

The server is configured to receive batches of 25 requests. Let us know if you need us to increase that number.

Comment 15

[Nan Jiang [:nanj]]

Nice, that's what I am looking for. Thanks for pointing out the kinto's document, I totally forgot about that :)

Comment 16

[Nan Jiang [:nanj]]

Hooray, managed to upload the whole manifest to kinto with the batch API. Also :r1cky approved the request and all the records got signed.

:leplatrem, how can I make Firefox talk to the stage now? I couldn't find any doc for that.

Comment 17

[Mathieu Leplatre [:leplatrem]]

Great!

With regards to switching environments, we had the about:remotesettings extension but you're  right, I'll make the docs clearer.
Meanwhile you can just execute those two lines:
https://github.com/leplatrem/remotesettings-pi/blob/84dd019022afa2aacdc7ac3d247b9fd19f09fd7f/data/script.js#L42
Have fun!

Comment 18

[Nan Jiang [:nanj]]

Bug 1476309 landed in nightly, TippyTop has succesfully switched to Remote Settings. \o/

Comment 19

[Mathieu Leplatre [:leplatrem]]

Congratulations :) Don't hesitate to get in touch if you have questions!

Comment 20

[Nan Jiang [:nanj]]

(In reply to Mathieu Leplatre (:leplatrem) PTO from comment #19)
> Congratulations :) Don't hesitate to get in touch if you have questions!

Will do, this is just the start, looking forward to equipping more AS remote contents with RS soon :)

Thanks a ton for your incredible help during this migration, Mathieu!

Comment 21

[Nan Jiang [:nanj]]

Closing this via bug 1476309, bug 1476292, and bug 1476934.

Everything looks good in nightly so far with the last two TippyTop releases (1.3 and 1.4). Will continue monitoring the TippyTop icon coverage at https://sql.telemetry.mozilla.org/queries/57594#150059