Closed Bug 1465251 Opened 6 years ago Closed 6 years ago

use PrioEncoder DOM function to encode values for Telemetry

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: rhelmer, Assigned: rhelmer)

References

Details

Attachments

(1 file)

Once the PrioEncoder DOM function from bug 1465249 is available, use it from chrome-only JS to encrypt a few boolean values sent in with Telemetry.

For the initial implementation, we want to sent both the clear-text and also prio-encoded version of the same values, so we can run a test on the server-side to verify that Prio works as intended.
Rob, are you tracking this and know who will work on this?

For the current testing stage, will this just record encrypted data into Telemetry or is Telemetry expected to do the encrypting?
Flags: needinfo?(rhelmer)
(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> Rob, are you tracking this and know who will work on this?

Ursula, are you able to take this on still?

I am going to put the PrioEncoder chrome-only DOM function up for review in bug 1465249 soon (hopefully today or else early next week)

> For the current testing stage, will this just record encrypted data into
> Telemetry or is Telemetry expected to do the encrypting?

The plan is to use the new PrioEncoder DOM method mentioned above to do the encryption, and just add the prio-encrypted payload to the normal Telemetry payload. We can post-process the data from the telemetry server (bug 1465252) to verify if the Prio approach is working.
Flags: needinfo?(rhelmer) → needinfo?(usarracini)
Yup, I can still do this.
Flags: needinfo?(usarracini)
Assignee: nobody → usarracini
(In reply to Robert Helmer [:rhelmer] from comment #2)
> > For the current testing stage, will this just record encrypted data into
> > Telemetry or is Telemetry expected to do the encrypting?
> 
> The plan is to use the new PrioEncoder DOM method mentioned above to do the
> encryption, and just add the prio-encrypted payload to the normal Telemetry
> payload. We can post-process the data from the telemetry server (bug
> 1465252) to verify if the Prio approach is working.

Where are you looking to add this to the normal payload?
If it's going somewhere outside of standard data types like histograms/scalars, it would be good to connect quickly on the approach.
Priority: -- → P3
(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> (In reply to Robert Helmer [:rhelmer] from comment #2)
> > > For the current testing stage, will this just record encrypted data into
> > > Telemetry or is Telemetry expected to do the encrypting?
> > 
> > The plan is to use the new PrioEncoder DOM method mentioned above to do the
> > encryption, and just add the prio-encrypted payload to the normal Telemetry
> > payload. We can post-process the data from the telemetry server (bug
> > 1465252) to verify if the Prio approach is working.
> 
> Where are you looking to add this to the normal payload?
> If it's going somewhere outside of standard data types like
> histograms/scalars, it would be good to connect quickly on the approach.

Yes it should go in the normal payload.
(In reply to Robert Helmer [:rhelmer] from comment #5)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> > (In reply to Robert Helmer [:rhelmer] from comment #2)
> > > > For the current testing stage, will this just record encrypted data into
> > > > Telemetry or is Telemetry expected to do the encrypting?
> > > 
> > > The plan is to use the new PrioEncoder DOM method mentioned above to do the
> > > encryption, and just add the prio-encrypted payload to the normal Telemetry
> > > payload. We can post-process the data from the telemetry server (bug
> > > 1465252) to verify if the Prio approach is working.
> > 
> > Where are you looking to add this to the normal payload?

Oops, I'm sorry I misread "Where are you" as "Were you".

> > If it's going somewhere outside of standard data types like
> > histograms/scalars, it would be good to connect quickly on the approach.
> 
> Yes it should go in the normal payload.

This is probably more a question for Ursula!
Flags: needinfo?(usarracini)
No longer blocks: 1421501
No longer depends on: 1465249
Depends on: 1421501
I believe we the API finalized in bug 1421501 and that the bug is close to landing, so I wanted to give you a heads-up and also describe how to use Prio from Firefox JS.

The WebIDL is:

---
[ChromeOnly, Exposed=(Window,System)]
namespace PrioEncoder {
  readonly attribute DOMString version;

  [Throws, NewObject]
  Promise<PrioEncodedData> encode(ByteString batchID, PrioParams params);
};

dictionary PrioParams {
  required boolean startupCrashDetected;
  required boolean safeModeUsage;
  required boolean browserIsUserDefault;
};

dictionary PrioEncodedData {
  ByteString a;
  ByteString b;
};
---

Example usage:

---
async function prioEncode() {
  // batchID can be anything, just needs to be unique each time this is called
  let batchID = new Date().getTime();

  // These are the only values supported at the moment, PrioEncoder.encode() will throw if these are not all specified
  let params = {browserIsUserDefault: false, safeModeUsage: true, startupCrashDetected: false};

  // This returns a promise which resolves to an object containing {a: <bytestring>, b: <bytestring>}
  let result = await PrioEncoder.encode(batchID, params);

  // You can use TextEncoder to turn the bytestrings into Uint8Array typed ArrayBuffers
  let t = new TextEncoder();
  console.log(t.encode(result.a), t.encode(result.b))
}
--- 

The result above is what we need to get into the existing Telemetry packet. I believe that's JSON and I'm not sure of the best way to encode binary data in there is, probably want to talk to Telemetry folks (do you have any thoughts mreid?)
Flags: needinfo?(usarracini) → needinfo?(mreid)
> async function prioEncode() {
>   // batchID can be anything, just needs to be unique each time this is
> called
>   let batchID = new Date().getTime();

One comment about this: We will only be able to compute aggregate statistics over users who submit data using the _same_ batch ID. So if the browser sends one telemetry ping per hour, for example, the batchID should be the concatenation of the date and hour. If the browser sends one ping per day, then the batchID should just be the date. Etc, etc.
So it needs to be the same batch id across a user population? Is there any reason we don't use just the concatenated names of the measurements as the batchID so we can aggregate across everyone?
(In reply to Chris H-C :chutten from comment #9)
> So it needs to be the same batch id across a user population?

Yes, that's right.

> Is there any
> reason we don't use just the concatenated names of the measurements as the
> batchID so we can aggregate across everyone?

If each client uses a different batchID per ping, then the servers that are computing the aggregate statistics can use these batchIDs to prevent certain types of replay attacks by a malicious server. In particular, using unique-per-submission batch IDs ensures that a client's submission for day X doesn't get included in the computation of the statistic for day X+1, even if there is an attacker (or adversarial server) sitting between the client and the servers.

For the pilot, I'm not sure that we need to worry about this type of attack (in which case the batchID can be the concatenated measurement names or some other constant value). If it's easy, having the date or date+hour as the batch ID would probably be good too.
(In reply to Robert Helmer [:rhelmer] from comment #7)
>   // You can use TextEncoder to turn the bytestrings into Uint8Array typed
> ArrayBuffers
>   let t = new TextEncoder();
>   console.log(t.encode(result.a), t.encode(result.b))
> }

Oh just FYI on the above - Henry noticed TextEncoder wasn't working as expected, I'm not 100% sure why.

This API returns `ByteString`s:
https://developer.mozilla.org/en-US/docs/Web/API/ByteString

But TextEncoder says it takes a `DOMString`:
https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder/encode

Not sure if that alone accounts for the difference, 

If you do need to convert this to a typed array, this seems to work and passes the libprio tests:

```
const toTypedArray = byteString => {
  let u8Array = new Uint8Array(byteString.length);
  for (let i in byteString) {
      u8Array[i] = byteString.charCodeAt(i);
  }
  return u8Array;
}
```
Quick update - so we did manage to figure out how to get Uint8Array returned from PrioEncoder.encode() directly, so you don't need that `toTypedArray()` in comment 11.

The C++ and WebIDL bits have r+ now (just waiting on one of the vendoring patches), the WebIDL is a bit simpler now (no version field and PrioEncodedData contains Uint8Arrays):

```
[ChromeOnly, Exposed=(Window,System)]
namespace PrioEncoder {
  [Throws, NewObject]
  Promise<PrioEncodedData> encode(ByteString batchID, PrioParams params);
};

dictionary PrioParams {
  required boolean startupCrashDetected;
  required boolean safeModeUsage;
  required boolean browserIsUserDefault;
};

dictionary PrioEncodedData {
  Uint8Array a;
  Uint8Array b;
};
```

Taking into account Henry's comments about batchID in comment 10, an example usage would be:

```
async function prioEncode() {
  // use current UTC day as batchID - see bug 1465251
  let batchID = `${now.getUTCFullYear()}/${now.getUTCMonth()}/${now.getUTCDate()}`

  // These are the only values supported at the moment, PrioEncoder.encode() will throw if these are not all specified
  let params = {browserIsUserDefault: false, safeModeUsage: true, startupCrashDetected: false};

  // This returns a promise which resolves to an object containing {a: <Uint8Array>, b: <Uint8Array>}
  let result = await PrioEncoder.encode(batchID, params);

  // This is the Prio-encoded data for server A and server B respectively
  console.log(result.a, result.b);
}
```
One thing we'll need to do as part of this bug is to regenerate the public/private keypair for the "A" and "B" servers. A plan for this was discussed in bug 1421501 comment 16, but I think it makes more sense to do that as part of landing the Telemetry client changes in this bug.

The public keys will be stored in the prefs `prio.publicKeyA` and `prio.publicKeyB` when bug 1421501 lands, and the private keys should only be available to the parts of the Telemetry pipeline that need them.

Also - I think it'd be ideal if we had Prio-encoded Telemetry always running for Nightly users, but pref'd off for release. I think the way to do this is with preprocessor directives (`#if defined(NIGHTLY_BUILD)` in https://searchfox.org/mozilla-central/source/browser/app/profile/firefox.js)

For beta/release users, we can run studies and/or do phased roll-out using the pref. Just getting the Nightly data rolling in will be huge progress though, thanks for everyones help on this so far!
(In reply to Robert Helmer [:rhelmer] from comment #7)
>   // This returns a promise which resolves to an object containing {a:
> <bytestring>, b: <bytestring>}
>   let result = await PrioEncoder.encode(batchID, params);
> 
>   // You can use TextEncoder to turn the bytestrings into Uint8Array typed
> ArrayBuffers
>   let t = new TextEncoder();
>   console.log(t.encode(result.a), t.encode(result.b))
> }
> --- 
> 
> The result above is what we need to get into the existing Telemetry packet.
> I believe that's JSON and I'm not sure of the best way to encode binary data
> in there is, probably want to talk to Telemetry folks (do you have any
> thoughts mreid?)

Can you give an example of the output here?
Flags: needinfo?(rhelmer)
(In reply to Georg Fritzsche [:gfritzsche] from comment #14)
> (In reply to Robert Helmer [:rhelmer] from comment #7)
> >   // This returns a promise which resolves to an object containing {a:
> > <bytestring>, b: <bytestring>}
> >   let result = await PrioEncoder.encode(batchID, params);
> > 
> >   // You can use TextEncoder to turn the bytestrings into Uint8Array typed
> > ArrayBuffers
> >   let t = new TextEncoder();
> >   console.log(t.encode(result.a), t.encode(result.b))
> > }
> > --- 
> > 
> > The result above is what we need to get into the existing Telemetry packet.
> > I believe that's JSON and I'm not sure of the best way to encode binary data
> > in there is, probably want to talk to Telemetry folks (do you have any
> > thoughts mreid?)
> 
> Can you give an example of the output here?

Sure, comment 12 has an example of the finalized API. It now returns TypedArrays (Uint8Array) directly, so there's no need to TextEncode or toTypedArray() as in previous comments (sorry for confusion there, I didn't think we'd get that API change done in time!)

The code in the above comment should work (assuming you assign "now" to a Date()), however I just ran this directly in the Browser Console as a more stripped-down example:

PrioEncoder.encode("...", {browserIsUserDefault:true, safeModeUsage: false, startupCrashDetected:true})
    .then(result => console.log(result))

The output of the console.log above is:

Object { a: Uint8Array(223), b: Uint8Array(154) }

So this is two Uint8Arrays, one for server A with size 223 and the other for server B of size 154 (it's expected that these sizes will vary slightly in different runs of PrioEncoder.encode())

TypedArrays have a lot of the same convenience methods as regular JS Arrays, so you should have lots of options for encoding this in JSON in some way that is acceptable for Telemetry: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array

Let me know if that helps! The canonical reference for the API would be the WebIDL:
https://hg.mozilla.org/mozilla-central/file/190b827aaa2b/dom/chrome-webidl/PrioEncoder.webidl

Happy to add comments to that and/or additional documentation elsewhere if that would be helpful.
Flags: needinfo?(rhelmer)
Just FYI - the bustage fix from bug 1485946 accidentally disabled PrioEncoder on *all* platforms, and not just the old Tier-2 Windows builder.

There'll be a patch landing momentarily, so make sure you're using the latest mozilla-central if you don't see it (or pull the patch from the request if you need it sooner), it's just a small change to one of the build files (toolkit/moz.configure)
Depends on: 1485946
No longer depends on: 1485946
Depends on: 1485946
Sorry for the slow reply - I don't have strong opinions on how to encode the binary data on the client. I'm fine with whatever :ursula decides.
Flags: needinfo?(mreid)
First stab at this; I am not sure if we want to use a smarter encoding for
the Uint8Arrays, or if we only want to use the first value from each histogram,
we want to coordinate this with mreid.

It's a bit unfortunate that this is all synchronous code since
PrioEncoder.encode() returns a promise... since we're in chrome code we can
just spin the event loop until it resolves, but it'd be ideal to make this
all async someday unless there's a reason not to :)
Taking this for now, happy to hand off if anyone has cycles to take over.
Assignee: usarracini → rhelmer
Status: NEW → ASSIGNED
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a40f6566e7f7f8118fd6678a6bfa1d106e0fa5d&selectedJob=197903546

Franziskus, this failure just started appearing as of this try run on Windows debug only:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a40f6566e7f7f8118fd6678a6bfa1d106e0fa5d&selectedJob=197903546

GECKO(3536) | Assertion failure: *outlen == 0, at z:/build/build/src/security/nss/lib/freebl/ctr.c:225

This is the first patch where we actually call PrioEncoder.encode() during tests, so not totally surprising, but I am having trouble figuring out exactly what this assertion means:

https://searchfox.org/mozilla-central/rev/a41fd8cb947266ea2e3f463fc6e31c88bfab9d41/security/nss/lib/freebl/ctr.c#225

Is this a sign we're not cleaning something up, and if so what? Also why would it be Windows-only?
Flags: needinfo?(franziskuskiefer)
(In reply to Robert Helmer [:rhelmer] from comment #20)
> Try run:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9a40f6566e7f7f8118fd6678a6bfa1d106e0fa5d&selectedJob=1
> 97903546
> 
> Franziskus, this failure just started appearing as of this try run on
> Windows debug only:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9a40f6566e7f7f8118fd6678a6bfa1d106e0fa5d&selectedJob=1
> 97903546
> 
> GECKO(3536) | Assertion failure: *outlen == 0, at
> z:/build/build/src/security/nss/lib/freebl/ctr.c:225

FWIW I grabbed an opt build and it looks like it's working OK, I'll run the output through the libprio integration test to see.
Comment on attachment 9006714 [details]
Bug 1465251 - use PrioEncoder to encode Telemetry values for pilot project r?gfritzsche

Kris Maglione [:kmag] has approved the revision.
Attachment #9006714 - Flags: review+
(In reply to Robert Helmer [:rhelmer] from comment #21)
> (In reply to Robert Helmer [:rhelmer] from comment #20)
>
> > GECKO(3536) | Assertion failure: *outlen == 0, at
> > z:/build/build/src/security/nss/lib/freebl/ctr.c:225
> 
> FWIW I grabbed an opt build and it looks like it's working OK, I'll run the
> output through the libprio integration test to see.

OK well to get this moving in the meantime (looks like it's failing down in some hardware-specific AES asm, yay?) I am going to disable libprio on Windows until it's resolved.

As mentioned above, PrioEncoder produces output in a Windows opt build, although I haven't had a chance to run it through the libprio integration test yet. I don't really understand the assertion but the outlen being 0 from that function seems bad...
Moving the Windows NSS assertion over to bug 1489691 to clear the way for this one.
Flags: needinfo?(franziskuskiefer)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2590ffaf2850761814cb3a60ef1cebc4211550c9 lgtm, MOZ_LIBPRIO is turned off on Windows until bug 1489691 is resolved but I don't think that should block landing this one.
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c55a3d07f0c
use PrioEncoder to encode Telemetry values for pilot project r=kmag
https://hg.mozilla.org/mozilla-central/rev/8c55a3d07f0c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1492638
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: