1288445 - Sync telemetry ping should hash FxA UID

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

It turns out that sql.tmo only has these hashed, and so we only need them that way. Hashing them on the client before sending should be trivial and sounds like a win for privacy.

It doesn't sound worth it to bump the sync ping schema version for this, but I could be wrong.

We should keep the value of all "0".repeat(32) for unknown/invalid UID since that still sounds like useful information that has no bearing on privacy whatsoever.

Comment 1

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

:dcoates, do you know which hash function we'll need to use on the UID?

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Thom Chiovoloni [:tcsc] from comment #0)
> It doesn't sound worth it to bump the sync ping schema version for this, but
> I could be wrong.

Yeah, this shouldn't be a big problem while we're still on Nightly and developing things. Just update the schema without bumping the version number for now.

Comment 3

[Danny Coates [:dcoates]]

I think we'll want to do this on the server. Its actually a SHA256 HMAC of the fxa uid and a secret set in the server config.

These 2 bits of code are where it happens:

https://github.com/mozilla-services/tokenserver/blob/master/tokenserver/views.py#L113-L125
https://github.com/mozilla-services/tokenserver/blob/master/tokenserver/util.py#L57-L66

Or the ping could send up the fxa_uid from the token, but that seems like more work.

Comment 4

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

So I think we've come to the conclusion that: browserid_identity already has the full token from the tokenServer, which is a JSON blob. Currently we use only api_endpoint, but also in that payload is a field called |id|, which it a base64 encoded version of yet another JSON blob (but apparently with trailing garbage we can probably work around). Inside *that* blob is a field fxa_uid. This fxa_uid is the uid logged by the server. We should change the client code to send this as the uid rather than the "real" uid.

We should try and get this done ASAP. Sadly tests will probably be the most difficult part (ie, we'll probably need to ensure that all tests which validate telemetry now have a fake token that roughly corresponds to the above) - probably not particularly difficult, just tedious.

Comment 5

[Ryan Kelly [:rfkelly]]

> (but apparently with trailing garbage we can probably work around).

If by "garbage" you mean "HMAC signature so the storage node can check validity of the token", sure :-)

I have reservations about shipping client code that parses the "id" field, it's supposed to be an opaque token.  If you want a value out of it, I'd prefer that the server explicitly pass you that value in plain text as another field in the response.

Comment 6

[Ryan Kelly [:rfkelly]]

That is to say, have the server return fxa_uid as another field in this response:

  https://github.com/mozilla-services/tokenserver/blob/master/tokenserver/views.py#L302
  http://docs.services.mozilla.com/token/apis.html

Comment 7

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

(In reply to Ryan Kelly [:rfkelly] from comment #6)
> That is to say, have the server return fxa_uid as another field in this
> response:

That sounds perfect (although it might make sense to use a different name, as it's not the "fxa uid" as the rest of the code thinks of it (ie, maybe hashed_uid would be better?)

Comment 8

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

I opened https://github.com/mozilla-services/tokenserver/issues/92

I propose that once that gets traction, we change the client code to look for the new entry in the token and if it exists, use that as the UID. If it doesn't exist, we don't record any UID. We could land the client change before the server change hits prod.

Comment 9

[Ryan Kelly [:rfkelly]]

Attachment: tokenserver PR to send the hashed_fxa_uid field

Comment 10

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

Attachment: Bug 1288445 - Use new hashed_fxa_uid field instead of the real FxA UID in sync telemetry ping.

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

Comment 11

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

Comment on attachment 8779398 [details]
Bug 1288445 - Use new hashed_fxa_uid field instead of the real FxA UID in sync telemetry ping.

https://reviewboard.mozilla.org/r/70384/#review67846

Looks great!

::: toolkit/components/telemetry/docs/sync-ping.rst:89
(Diff revision 1)
>  These values should be monotonic.  If we can't get a monotonic timestamp, -1 will be reported on the payload, and the values will be omitted from the engines. Additionally, the value will be omitted from an engine if it would be 0 (either due to timer inaccuracy or finishing instantaneously).
>  
>  uid
>  ~~~
>  
> -This property containing the FxA account identifier, which is provided by the FxA auth server APIs: `<https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md>`_. It may be an empty string in the case that we are unable to authenticate with FxA, and have never authenticated in the past.  If present, it should be a 32 character hexidecimal string.
> +This property containing a hash of the FxA account identifier. It may be an empty string in the case that we are unable to authenticate with FxA, and have never authenticated in the past.  If present, it should be a 32 character hexidecimal string.

It seems these docs are wrong about the empty string, and that it is "0" * 32 - if that's correct we might as well update these docs at the same time.

Comment 12

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

Comment on attachment 8779398 [details]
Bug 1288445 - Use new hashed_fxa_uid field instead of the real FxA UID in sync telemetry ping.

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

Comment 13

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

Comment on attachment 8779398 [details]
Bug 1288445 - Use new hashed_fxa_uid field instead of the real FxA UID in sync telemetry ping.

This is a very small change to the data we're sending (replacing a UID with a hash of the same UID sent down from the server).

Comment 14

[Benjamin Smedberg]

Comment on attachment 8779398 [details]
Bug 1288445 - Use new hashed_fxa_uid field instead of the real FxA UID in sync telemetry ping.

https://reviewboard.mozilla.org/r/70384/#review68284

::: toolkit/components/telemetry/docs/sync-ping.rst:89
(Diff revision 2)
>  These values should be monotonic.  If we can't get a monotonic timestamp, -1 will be reported on the payload, and the values will be omitted from the engines. Additionally, the value will be omitted from an engine if it would be 0 (either due to timer inaccuracy or finishing instantaneously).
>  
>  uid
>  ~~~
>  
> -This property containing the FxA account identifier, which is provided by the FxA auth server APIs: `<https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md>`_. It may be an empty string in the case that we are unable to authenticate with FxA, and have never authenticated in the past.  If present, it should be a 32 character hexidecimal string.
> +This property containing a hash of the FxA account identifier, which is a 32 character hexidecimal string.  In the case that we are unable to authenticate with FxA and have never authenticated in the past, it will be a placeholder string consisting of 32 repeated ``0`` characters.

This is a little weird to me: I'd normally expect a value like this to just be `null`. But that's just an implementation suggestion, not a requirement.

Comment 15

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

Comment on attachment 8779398 [details]
Bug 1288445 - Use new hashed_fxa_uid field instead of the real FxA UID in sync telemetry ping.

https://reviewboard.mozilla.org/r/70384/#review68316

::: toolkit/components/telemetry/docs/sync-ping.rst:89
(Diff revision 2)
>  These values should be monotonic.  If we can't get a monotonic timestamp, -1 will be reported on the payload, and the values will be omitted from the engines. Additionally, the value will be omitted from an engine if it would be 0 (either due to timer inaccuracy or finishing instantaneously).
>  
>  uid
>  ~~~
>  
> -This property containing the FxA account identifier, which is provided by the FxA auth server APIs: `<https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md>`_. It may be an empty string in the case that we are unable to authenticate with FxA, and have never authenticated in the past.  If present, it should be a 32 character hexidecimal string.
> +This property containing a hash of the FxA account identifier, which is a 32 character hexidecimal string.  In the case that we are unable to authenticate with FxA and have never authenticated in the past, it will be a placeholder string consisting of 32 repeated ``0`` characters.

Unless we were to bump the version, I don't see the benefit of doing this at this point -- This is already the current behavior, it was just misdocumented. So all code will likely have to handle "0" * 32 anyway, and if we added null, it would need to handle that as well.

Comment 16

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

(Whoops, may have entered that in the wrong spot on mozreview. That was a reply to :bsmedberg's comment directly above).

Anyway...

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4437c4da937
Use new hashed_fxa_uid field instead of the real FxA UID in sync telemetry ping. r=bsmedberg,markh

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1848147&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/913802fd98ec

Comment 19

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

Comment on attachment 8779398 [details]
Bug 1288445 - Use new hashed_fxa_uid field instead of the real FxA UID in sync telemetry ping.

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

Comment 20

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

Ack, apologies. Should be fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ad8b983b588. 

It looks like one of the tests is failing due to having the code for the patch from bug 1295138, but I can rebase if you need me to.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bdad03af959
Use new hashed_fxa_uid field instead of the real FxA UID in sync telemetry ping. r=bsmedberg,markh

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1bdad03af959

Comment 23

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

Comment on attachment 8779398 [details]
Bug 1288445 - Use new hashed_fxa_uid field instead of the real FxA UID in sync telemetry ping.

Approval Request Comment
[Feature/regressing bug #]: Sync telemetry ping
[User impact if declined]: Inaccurate sync metrics
[Describe test coverage new/current, TreeHerder]: Tests pass
[Risks and why]: Low
[String/UUID change made/needed]: None

We now hash the userid that we report to telemetry with the sync ping. This is both for user privacy and consistency with server metrics. The Sync ping landed in 50, so we should get this there too.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8779398 [details]
Bug 1288445 - Use new hashed_fxa_uid field instead of the real FxA UID in sync telemetry ping.

Makes sense, a test was added for this, Aurora50+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3b7c8b6aa43a