1436078 - Web Authentication - Support already-enrolled U2F devices with Google Accounts

Status: RESOLVED FIXED

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

Description

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

To paraphrase the Intent-to-Ship [1]:

Web Authentication is on-track to ship in Firefox 60, and contains
within it support for already-deployed USB-connected FIDO U2F devices, and
we intend to ship with a spec extension feature implemented to support
devices that were already-enrolled using the older U2F Javascript API .

This bug will hard-code support in Gecko to permit Google Accounts’ cross-origin U2F behavior, the same way as Chrome has. This is proposed for a period of 5 years, until 2023 The code should be a small search similar to Chrome [2].

[1] https://groups.google.com/d/msg/mozilla.dev.platform/Uiu3fwnA2xw/201ynAiPAQAJ
[2] https://chromium.googlesource.com/chromium/src.git/+/master/chrome/browser/extensions/api/cryptotoken_private/cryptotoken_private_api.cc#161

Comment 1

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

Attachment: Bug 1436078 - Hard-code U2F permissions for Google Accounts

This patch support already-enrolled U2F devices at Google Accounts by adding a
hard-coded "OK" into the U2F EvaluateAppID method, per the intent-to-ship [1].

This adds no tests, as this is not testable in our infrastructure. It will
require cooporation with Google Accounts to validate.

[1] https://groups.google.com/d/msg/mozilla.dev.platform/Uiu3fwnA2xw/201ynAiPAQAJ

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

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8948852 [details]
Bug 1436078 - Hard-code U2F permissions for Google Accounts

https://reviewboard.mozilla.org/r/218234/#review224096

Let's do this.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8948852 [details]
Bug 1436078 - Hard-code U2F permissions for Google Accounts

https://reviewboard.mozilla.org/r/218234/#review224158

::: dom/u2f/U2F.cpp:217
(Diff revision 1)
> +  // Bug #1436078 - Permit Google Accounts. Remove in Bug #1436085 in Jan 2023.
> +  if (lowestFacetHost.EqualsLiteral("google.com") &&
> +      (aAppId.Equals(kGoogleAccountsAppId1) ||
> +       aAppId.Equals(kGoogleAccountsAppId2))) {
> +    MOZ_LOG(gU2FLog, LogLevel::Debug,
> +            ("U2F permitted for Google Accounts via Bug #1436085"));
> +    return ErrorCode::OK;
> +  }

On second thought... we should only allow this for sign operations, not for registration, right? The function currently isn't aware of that... maybe we need another parameter?

Comment 4

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

Comment on attachment 8948852 [details]
Bug 1436078 - Hard-code U2F permissions for Google Accounts

https://reviewboard.mozilla.org/r/218234/#review224158

> On second thought... we should only allow this for sign operations, not for registration, right? The function currently isn't aware of that... maybe we need another parameter?

Good idea!

Comment 5

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

Comment on attachment 8948852 [details]
Bug 1436078 - Hard-code U2F permissions for Google Accounts

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

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8948852 [details]
Bug 1436078 - Hard-code U2F permissions for Google Accounts

https://reviewboard.mozilla.org/r/218234/#review224210

Awesome, thank you.

Comment 7

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev ff1f162dd874 needs "Bug N" or "No bug" in the commit message.
remote: J.C. Jones <[e-mail]>
remote: Bug #1436078 - Hard-code U2F permissions for Google Accounts r=ttaubert
remote: 
remote: This patch support already-enrolled U2F devices at Google Accounts by adding a
remote: hard-coded "OK" into the U2F EvaluateAppID method, per the intent-to-ship [1].
remote: 
remote: This adds no tests, as this is not testable in our infrastructure. It will
remote: require cooporation with Google Accounts to validate.
remote: 
remote: [1] https://groups.google.com/d/msg/mozilla.dev.platform/Uiu3fwnA2xw/201ynAiPAQAJ
remote: 
remote: MozReview-Commit-ID: 1YLd5sfeTKv
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote

Comment 8

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

Comment on attachment 8948852 [details]
Bug 1436078 - Hard-code U2F permissions for Google Accounts

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89ac5a28c228
Hard-code U2F permissions for Google Accounts r=ttaubert

Comment 10

[Dorel Luca [:dluca]]

Backed out changeset 89ac5a28c228 (bug 1436078) for build bustage on multiple platforms on a CLOSED TREE

https://treeherder.mozilla.org/logviewer.html#?job_id=160862677&repo=autoland&lineNumber=18802

https://hg.mozilla.org/integration/autoland/rev/66e626da016edca4f8d1f1f544b6981bed0d2906

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=89ac5a28c228649e436cd8dbcec0d395c231e4e1

Comment 11

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

Comment on attachment 8948852 [details]
Bug 1436078 - Hard-code U2F permissions for Google Accounts

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

Comment 12

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

Blast! No static enums for me. I've a new try build running without that modifier: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca9f726ea9d1

Comment 13

[Tom S. (please needinfo tschuster)]

Did you mean "enum class" instead of "static enum" ?

Comment 14

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

(In reply to Tom Schuster [:evilpie] from comment #13)
> Did you mean "enum class" instead of "static enum" ?

This is a lesson in C++ to me. :) Yeah, that's what I wanted, to avoid polluting the name scope. Thanks, Tom.

Comment 15

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

Comment on attachment 8948852 [details]
Bug 1436078 - Hard-code U2F permissions for Google Accounts

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62646c1718b2
Hard-code U2F permissions for Google Accounts r=ttaubert

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/62646c1718b2