Add telemetry to WebAuthn / U2F

RESOLVED FIXED in Firefox 57

Status

()

Core
DOM: Device Interfaces
P1
enhancement
RESOLVED FIXED
a year ago
11 days ago

People

(Reporter: jcj, Assigned: jcj)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [webauthn])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
There should be telemetry available for WebAuth / FIDO U2F usage.
(Assignee)

Updated

a year ago
Blocks: 1065729
("fixlater" means "probably in the next few months" but maybe this should be "backlog"? "btpp" = "bug triage pilot program")
Whiteboard: btpp-fixlater

Updated

8 months ago
Summary: Add telemetry to WebAuth / U2F → Add telemetry to WebAuthn / U2F
(Assignee)

Updated

a month ago
Assignee: nobody → jjones
Blocks: 1294514
Severity: normal → enhancement
Status: NEW → ASSIGNED
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Whiteboard: btpp-fixlater → [webauthn]
Target Milestone: --- → mozilla57
(Assignee)

Comment 2

a month ago
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)
(Assignee)

Comment 4

28 days ago
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 hidden (mozreview-request)

Comment 6

28 days ago
mozreview-review
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+
(Assignee)

Comment 7

28 days ago
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 hidden (mozreview-request)

Comment 9

28 days ago
mozreview-review
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+
(Assignee)

Comment 10

27 days ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Comment 12

27 days ago
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

Comment 13

27 days ago
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)
seems also https://treeherder.mozilla.org/logviewer.html#?job_id=117866580&repo=autoland might be related

Comment 16

27 days ago
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
(Assignee)

Comment 17

27 days ago
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 hidden (mozreview-request)
(Assignee)

Comment 19

12 days ago
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+
(Assignee)

Comment 21

12 days ago
Let's try landing this again.
Keywords: checkin-needed

Comment 22

12 days ago
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)
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Comment 24

12 days ago
Once more, with feeling!
Keywords: checkin-needed

Comment 25

12 days ago
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
Last Resolved: 11 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.