1299784 - Sync pings should include a device id.

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

Desktop pings aren't recording a device ID, which makes some problems difficult to analyze (eg, bug 1298758) - we can't tell 2 same-OS devices apart from each other when tracking repair behaviour. I think we should hash the fxa device ID and include it in each of our pings.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Ryan, I suspect you have a better grasp of the considerations here - can you think of any operational or privacy concerns with writing a device ID the sync ping? If not, is it worth hashing the ID - the hash function isn't going to be secret so there may be no value in bothering to hash it.

Comment 2

[Ryan Kelly [:rfkelly]]

If unhashed, including the deviceid means we could take a sync ping and look up the corresponding user in FxA, finding out their email address etc.  We probably don't want that.

If hashed using all public data, we couldn't do that direct lookup, but we could start from a specific user in our FxA data, hash their device ids, and then look for any pings from that user in telemetry.  That's still not ideal.

It would be better if this were a HMAC using a secret key, in the same way that we hash the userid.  Then only folks with access to the key could perform that above lookup.  But of course you can't calculate that client-side.

Possible idea: include the metrics_uid in the hash, like SHA256(device_id + metrics_uid) or similar.   That would give you a stable unique identifier that can't be mapped back to the user's FxA identity without knowing the metrics HMAC key.

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1299784 - Include a hashed version of the device ID with the sync ping

Review commit: https://reviewboard.mozilla.org/r/77312/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77312/

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8789035 [details]
Bug 1299784 - Include a hashed version of the device ID with the sync ping

I'm aware that there's still discussion around the right thing to do here and that this will likely change. 

Also, this patch was made under the assumption that metrics_uid is the hashed_fxa_uid we use elsewhere in the ping (it seems to be from looking at the tokenserver code, but I'm not sure).

Comment 5

[Ryan Kelly [:rfkelly]]

> Also, this patch was made under the assumption that metrics_uid is the hashed_fxa_uid
> we use elsewhere in the ping (it seems to be from looking at the tokenserver code, but I'm not sure).

Yep, that's the one I meant.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8789035 [details]
Bug 1299784 - Include a hashed version of the device ID with the sync ping

https://reviewboard.mozilla.org/r/77312/#review75680

This looks great to me. I think we should consider getting another bug on file so the UID and DeviceID isn't repeated in every sync (ie, appears only once in the entire ping) - I think that would also mean we need to observe a couple of other events relating to the device or the user changing and submit queued pings at that time, but that seems OK.

Can you please put a new version up with the improved comment, carry this r+ forward and request review from bsmedberg?

::: services/sync/modules/telemetry.js:231
(Diff revision 1)
>      if (error) {
>        this.failureReason = transformError(error);
>      }
>  
>      try {
>        this.uid = Weave.Service.identity.hashedUID();

let's comment here about why we are doing things this way (ie, paraphrasing Ryan's comment on this)

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8789035 [details]
Bug 1299784 - Include a hashed version of the device ID with the sync ping

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

Comment 8

[Benjamin Smedberg]

Comment on attachment 8789035 [details]
Bug 1299784 - Include a hashed version of the device ID with the sync ping

https://reviewboard.mozilla.org/r/77312/#review77062

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0123be9d2c87
Include a hashed version of the device ID with the sync ping r=bsmedberg,markh

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0123be9d2c87