1545232 - Add ContentProvider for sharing authentication state

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Firefox Accounts, task, P1)

Description

[:Grisha Kruglov]

Our mobile application ecosystem is growing, and we need a mechanism to share our authentication state with other trusted (read: ours) applications on the system. This greatly helps in reducing on-boarding friction, and overall makes for a smoother user experience.

For example, with the upcoming Fenix launch, it would be nice to be able to ask the user if they want to re-use the FxA account they currently have configured in Fennec.

After some discussions, proposed approach to this is an exported ContentProvider which checks its callers against a signature whitelist. This is somewhat similar to a signature-level ContentProvider readPermission, but with quite a bit more operational flexibility since we're performing all of the necessary checks ourselves. Whitelist will be a map from applicationId to a known certificate signature. We will explicitly not support multiple signers or key rotation for the first iteration of this (refusing access if those are detected).

Users of this ContentProvider are expected to do their due diligence and check that the authority they're relying on is signed with the known Fennec's keys.

Comment 1

[:Grisha Kruglov]

Attachment: Bug 1545232 - Add AuthStateProvider r=nalexander,sebastian

See comments in the patch for details.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc1cbac5213d
Add AuthStateProvider r=nalexander,sebastian

Comment 3

[Andrei Ciure[:aciure]]

Backed out changeset dc1cbac5213d (bug 1545232) for causing lint build bustages

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=dc1cbac5213d6027127892e460a852bbf6e683b3

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=b03a3303ad6e4be3f61f597dce9b1d875c0ebd04&selectedJob=241367792&searchStr=android%2C4.0%2Capi16%2B%2Copt%2Candroid%2Cgradle%2Ctests%2Cbuild-android-lint%2Fopt%2Ca%28lint%29

backout: https://hg.mozilla.org/integration/autoland/rev/9bf44f82eea61a6f22a12e20ce941df0602993ba

Comment 4

[:Grisha Kruglov]

Ugh. https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=dc1cbac5213d6027127892e460a852bbf6e683b3&selectedJob=241370169 has the details. This is the case where lint isn't being very helpful. It's flagging our use of newer APIs (which is gated on version checks) and exporting a content provider without permissions (which is protected by manual signature checks)... Need to figure out how to appease lint.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c712980e970
Add AuthStateProvider r=nalexander,sebastian

Comment 6

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9c712980e970

Comment 7

[:Grisha Kruglov]

Attachment: auth_state_provider.patch

Beta/Release Uplift Approval Request

• User impact if declined: This work is done in preparation for launching Fenix. This patch allows whitelisted application (currently, release channels of Fenix and Lockwise) to securely access Fennec's authentication information, and thus seamlessly migrate their Firefox Account from Fennec to Fenix (or Lockwise).

If uplift is declined, Fenix release in June will not support any account migration from Fennec. That's the biggest impact, and will negatively impact our FxA users as they're trying out the new browser. This may also negatively impact Fenix launch overall.

• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: Due to signature whitelist, testing this requires Fenix support. We don't have support for this in Fenix yet. Tested with non-released applications locally, by adjusting hard-coded whitelist. Once Fenix nightly comes with a basic migration support, we will be able to test two nightlies against each other, and QA will be able to verify.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This patch exposes authentication information (enough to synchronize everything that user has stored in Firefox Sync) to a set of whitelisted applications. This is obviously sec-sensitive.

We - myself, reviewers - are confident that the signature-whitelist approach is good enough to share this information securely among our own applications.

Alternatives to this have been deemed to be riskier (complexity wise), more time consuming (non-feasible given the tight timelines), potentially less secure, and less flexible when considering authentication sharing strategies within our app ecosystem.

• String changes made/needed: none

Comment 8

[:Grisha Kruglov]

Adding to the above, functionality is quite isolated from everything else in Fennec, and should have no impact on the application. It's also written in a defensive style, to help make sure we won't see unexpected problems in the wild. Consuming side (Fenix, Lockwise) will require a similar whitelist as well, to verify they're interacting with a real Fennec.

Comment 9

[Pascal Chevrel:pascalc (PTO until April 26)]

Grisha, was there a security review done for this patch?

Comment 10

[:Grisha Kruglov]

We haven't done a formal security review, because it was deemed unnecessary.

We're relying on basic security guarantees of the android application signing mechanism. The system APIs we're using are partially intended for this specific purpose (different packages verifying trust in each other against their internal whitelists). Through some cursory research (externally and internally), this kind of verification seems like a fairly standard practice in the industry.

Additionally, our specific version of signature verification is as conservative as we can make it.

Comment 11

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9060178 [details] [diff] [review]
auth_state_provider.patch

The patch is low risk at this is prep work for Fenix past the 67 release and is rather isolated from Fennec code, we have added a Trello card for this in our Firefox board so as that other stakeholders have this change on their radar, specifically QA. Uplift approved for 67 beta 15, thanks.

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ded211603671

Comment 13

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I filed a PI Request for a security review. That team can assess what kind of attention this patch needs. I understand it is low risk but I think it will be good to get another pair of security-minded eyes on it.