1265472 - Add telemetry to WebAuthn / U2F

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, enhancement, P1)

Description

[J.C. Jones [:jcj] (he/they)]

There should be telemetry available for WebAuth / FIDO U2F usage.

Comment 1

[Andrew Overholt [:overholt]]

("fixlater" means "probably in the next few months" but maybe this should be "backlog"? "btpp" = "bug triage pilot program")

Comment 2

[J.C. Jones [:jcj] (he/they)]

I think we'll start with some (release) opt-in telemetry as just a keyed scalar indicating how often the WebAuthentication methods succeeded or failed within the subsession.

The keys would be: U2F-Register-Finish, U2F-Register-Abort, U2F-Sign-Finish, U2F-Sign-Abort.

The goal of this telemetry would be to obtain a rough sense of the percentage of our opt-in telemetry population using Web Authentication over time, and whether or not it worked. It counts Register (enrollment) and Sign (login) separately as we would anticipate there being far, far more Sign uses than Register.

Expected Scalars.yaml update:

>  webauthn_used:
>    bug_numbers:
>      - 1265472
>    description: >
>      Counts of how often Web Authentication was used in this subsession, keyed
>      by authenticator protocol, method and result. Currently: U2F-Register-Finish,
>      U2F-Register-Abort, U2F-Sign-Finish, U2F-Sign-Abort.
>    expires: "63"
>    kind: uint
>    keyed: true
>    notification_emails:
>      - [e-mail]
>    release_channel_collection: opt-in
>    record_in_processes:
>      - main

I'm not really sure if this is Category 3 (Web activity data) or Category 2 “Interaction data”, but I'm fine with it being opt-in for release.

I'd kind of like to set "expires" out to more like Firefox 70.... but we can bump it in the future. :)

I don't think I can set a feedback flag here until I have a patch, but I'm going to NI Francois anyway as I don't want to code this until it looks about right.

Thanks!

Comment 3

[François Marier [:francois]]

(In reply to J.C. Jones [:jcj] from comment #2)
> >  webauthn_used:
> >    bug_numbers:
> >      - 1265472
> >    description: >
> >      Counts of how often Web Authentication was used in this subsession, keyed

What do you mean by "subsession"?

> >      by authenticator protocol, method and result. Currently: U2F-Register-Finish,
> >      U2F-Register-Abort, U2F-Sign-Finish, U2F-Sign-Abort.

So the keys will always be fixed strings (i.e. something you can whitelist) and never include data that could come from the user or the site?

That's important because if you collect something that can identify a site (e.g. GitHubDraftU2F-Register-Finish) then it could end up as Category 3 data.

> >    expires: "63"
> >    kind: uint
> >    keyed: true
> >    notification_emails:
> >      - [e-mail]

We now ask for a real person's email too so if you could add your own email address to the list, that would be better. The plan is to use this in the future to flag probes as potentially obsolete.

> >    release_channel_collection: opt-in
> >    record_in_processes:
> >      - main
> 
> I'm not really sure if this is Category 3 (Web activity data) or Category 2
> “Interaction data”, but I'm fine with it being opt-in for release.

This would be Category 2 ("uses of specific Firefox feature") so opt-out is OK and will give you more representative data.

It would get into "Web activity data" territory if you were to keep track of the sites where the feature is being used (e.g. GitHub, Google Accounts).

> I'd kind of like to set "expires" out to more like Firefox 70.... but we can
> bump it in the future. :)

70 sounds fine given the adoption of that feature won't happen overnight.

Comment 4

[J.C. Jones [:jcj] (he/they)]

Thanks for the feedback, François! I think my work-in-progress patch is, then, actually ready-for-review!

(In reply to François Marier [:francois] from comment #3)
> What do you mean by "subsession"?

I was potentially misusing the term from one of the Telemetry design docs for Scalars, that they're aggregated by
'subsession' rather than 'session' (which may last a month).

I changed it to 'session' as that's probably more understandable.

> > >      by authenticator protocol, method and result. Currently: U2F-Register-Finish,
> > >      U2F-Register-Abort, U2F-Sign-Finish, U2F-Sign-Abort.
> 
> So the keys will always be fixed strings (i.e. something you can whitelist)
> and never include data that could come from the user or the site?

Exactly. The patch will show how I'm using it.

> That's important because if you collect something that can identify a site
> (e.g. GitHubDraftU2F-Register-Finish) then it could end up as Category 3
> data.

Understood, yeah, this is just to avoid defining 4 scalars.

Comment 5

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1265472 - Add Telemetry to Web Authentication datareview=francois

This patch intends to obtain a 1) rough sense of the percentage of telemetry
population using Web Authentication over time, and 2) whether or not the
Authentication request was successful or errored out as a scalar. It also tracks
3) how long it takes for requests to complete as a histogram.

It counts Register (enrollment) and Sign (login) separately as we would
anticipate there being far, far more Sign uses than Register.

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

Comment 6

[François Marier [:francois]]

Comment on attachment 8889694 [details]
Bug 1265472 - Add Telemetry to Web Authentication  datareview=francois

https://reviewboard.mozilla.org/r/160750/#review166010

datareview+

Comment 7

[J.C. Jones [:jcj] (he/they)]

Note to reviewers: There are some test issues on Try [1], so the browser_webauthn_telemetry.js is going to need some kind of adjustments.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=117255980&repo=try&lineNumber=4308

Comment 8

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8889694 [details]
Bug 1265472 - Add Telemetry to Web Authentication  datareview=francois

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

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8889694 [details]
Bug 1265472 - Add Telemetry to Web Authentication  datareview=francois

https://reviewboard.mozilla.org/r/160750/#review166400

Looks good to me - just some minor comments.

::: dom/webauthn/tests/browser/browser_webauthn_telemetry.js:15
(Diff revision 2)
> +    Services.telemetry.snapshotKeyedScalars(aChannel, aClear)["parent"] :
> +    Services.telemetry.snapshotScalars(aChannel, aClear)["parent"];
> +  return scalars || {};
> +}
> +
> +function getTelemetryForScalar(aProcess, aName) {

aProcess seems unused here?

::: dom/webauthn/tests/browser/browser_webauthn_telemetry.js:27
(Diff revision 2)
> +  Services.telemetry.clearEvents();
> +  Services.telemetry.getHistogramById("WEBAUTHN_CREATE_CREDENTIAL_MS").clear();
> +  Services.telemetry.getHistogramById("WEBAUTHN_GET_ASSERTION_MS").clear();
> +}
> +
> +function validateHistogramEntryCount(aHistogramName, aCount) {

nit: maybe aExpectedCount?

::: dom/webauthn/tests/browser/browser_webauthn_telemetry.js:100
(Diff revision 2)
> +    let webauthn_used = getTelemetryForScalar("content", "security.webauthn_used");
> +    ok(webauthn_used, "Scalar keys are set: " + Object.keys(webauthn_used).join(", "));
> +    is(webauthn_used["U2FRegisterFinish"], undefined, "webauthn_used U2FRegisterFinish must be unset");
> +    is(webauthn_used["U2FSignFinish"], undefined, "webauthn_used U2FSignFinish scalar must be unset");
> +    is(webauthn_used["U2FRegisterAbort"], 1, "webauthn_used U2FRegisterAbort scalar should be a 1");
> +    is(webauthn_used["U2FSignAbort"], undefined, "webauthn_used U2FSignAbort scalar must be unset");

hmmm - so I guess we don't have a testcase for register succeeding and sign failing? That's probably ok, but maybe add a comment?

::: toolkit/components/telemetry/Scalars.yaml:368
(Diff revision 2)
> +    bug_numbers:
> +      - 1265472
> +    description: >
> +      Counts of how often Web Authentication was used in this session, keyed
> +      by authenticator protocol, method and result. Currently: U2F-Register-Finish,
> +      U2F-Register-Abort, U2F-Sign-Finish, U2F-Sign-Abort.

The strings in the implementation don't have hyphens, so I imagine they shouldn't here either.

Comment 10

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8889694 [details]
Bug 1265472 - Add Telemetry to Web Authentication  datareview=francois

https://reviewboard.mozilla.org/r/160750/#review166400

Thanks a bunch, David!

> aProcess seems unused here?

Good catch :)

> nit: maybe aExpectedCount?

True.

> hmmm - so I guess we don't have a testcase for register succeeding and sign failing? That's probably ok, but maybe add a comment?

Done.

> The strings in the implementation don't have hyphens, so I imagine they shouldn't here either.

Fixed.

Comment 11

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8889694 [details]
Bug 1265472 - Add Telemetry to Web Authentication  datareview=francois

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

Comment 12

[J.C. Jones [:jcj] (he/they)]

Try run [1] only has known intermittents on the ASAN build, otherwise looks fine. Marking checkin-needed.


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=168b84da3a4ceeb4e8d9b81a7bee13e22754493b&selectedJob=117644438

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/674770e65a4f
Add Telemetry to Web Authentication r=francois,keeler datareview=francois

Comment 14

[Carsten Book [:Tomcat]]

backed out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=117791270&repo=autoland

Comment 15

[Carsten Book [:Tomcat]]

seems also https://treeherder.mozilla.org/logviewer.html#?job_id=117866580&repo=autoland might be related

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f821dab4306
Backed out changeset 674770e65a4f for memory leaks in browser_webauthn_telemetry.js

Comment 17

[J.C. Jones [:jcj] (he/they)]

Thanks, :tomcat. I've never worked with browser-chrome mochitests, so I'm going to have to look for help on this.

Comment 18

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8889694 [details]
Bug 1265472 - Add Telemetry to Web Authentication  datareview=francois

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

Comment 19

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8889694 [details]
Bug 1265472 - Add Telemetry to Web Authentication  datareview=francois

With some browser-chrome help from Tim Taubert, I've adjusted the test (and rebased). This try run was clean:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe3a171181f1d8ad92aca08872ca43462b949842

Keeler, can I get a second look before we try re-landing it?

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8889694 [details]
Bug 1265472 - Add Telemetry to Web Authentication  datareview=francois

LGTM.

Comment 21

[J.C. Jones [:jcj] (he/they)]

Let's try landing this again.

Comment 22

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 104611d24bd7 -d c95150c0fe9d: rebasing 412512:104611d24bd7 "Bug 1265472 - Add Telemetry to Web Authentication r=francois,keeler datareview=francois" (tip)
merging toolkit/components/telemetry/Histograms.json
warning: conflicts while merging toolkit/components/telemetry/Histograms.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 23

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8889694 [details]
Bug 1265472 - Add Telemetry to Web Authentication  datareview=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160750/diff/4-5/

Comment 24

[J.C. Jones [:jcj] (he/they)]

Once more, with feeling!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3492692822dc
Add Telemetry to Web Authentication r=francois,keeler datareview=francois

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3492692822dc