Closed Bug 1265472 Opened 4 years ago Closed 3 years ago

Add telemetry to WebAuthn / U2F

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webauthn])

Attachments

(1 file)

There should be telemetry available for WebAuth / FIDO U2F usage.
("fixlater" means "probably in the next few months" but maybe this should be "backlog"? "btpp" = "bug triage pilot program")
Whiteboard: btpp-fixlater
Summary: Add telemetry to WebAuth / U2F → Add telemetry to WebAuthn / U2F
Assignee: nobody → jjones
Blocks: webauthn
Severity: normal → enhancement
Status: NEW → ASSIGNED
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Whiteboard: btpp-fixlater → [webauthn]
Target Milestone: --- → mozilla57
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:
>      - seceng-telemetry@mozilla.com
>    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!
Flags: needinfo?(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:
> >      - seceng-telemetry@mozilla.com

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.
Flags: needinfo?(francois)
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 on attachment 8889694 [details]
Bug 1265472 - Add Telemetry to Web Authentication  datareview=francois

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

datareview+
Attachment #8889694 - Flags: review?(francois) → review+
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 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.
Attachment #8889694 - Flags: review?(dkeeler) → review+
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.
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
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/674770e65a4f
Add Telemetry to Web Authentication r=francois,keeler datareview=francois
Keywords: checkin-needed
backed out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=117791270&repo=autoland
Flags: needinfo?(jjones)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f821dab4306
Backed out changeset 674770e65a4f for memory leaks in browser_webauthn_telemetry.js
Thanks, :tomcat. I've never worked with browser-chrome mochitests, so I'm going to have to look for help on this.
Flags: needinfo?(jjones)
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?
Attachment #8889694 - Flags: review+ → review?(dkeeler)
Comment on attachment 8889694 [details]
Bug 1265472 - Add Telemetry to Web Authentication  datareview=francois

LGTM.
Attachment #8889694 - Flags: review?(dkeeler) → review+
Let's try landing this again.
Keywords: checkin-needed
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)
Once more, with feeling!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3492692822dc
Add Telemetry to Web Authentication r=francois,keeler datareview=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3492692822dc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Regressions: 1553175
You need to log in before you can comment on or make changes to this bug.