Closed Bug 1639795 Opened 6 months ago Closed 6 months ago

Use a friendlier name for the OSKeyStore Keychain item

Categories

(Core :: Security: PSM, enhancement, P1)

Desktop
macOS
enhancement

Tracking

()

VERIFIED FIXED
mozilla79
Tracking Status
firefox79 --- verified

People

(Reporter: MattN, Assigned: abr)

References

Details

(Whiteboard: [cc-autofill-mvp])

Attachments

(7 files)

Chrome uses "Chrome Safe Storage" whereas we are restricted to org.mozilla.nss.keystore.* but this can be shown to users in some cases and would probably confuse them since "Firefox" would be at the end e.g. org.mozilla.nss.keystore.firefox. I think it would be good to make this friendlier from the get-go to avoid having to do an error-prone migration later (where a user denying access at that point would be a problem).

Filing this to track for bug 1527746, it's not a priority yet.

OS: Unspecified → macOS
Hardware: Unspecified → Desktop
Assignee: nobody → adam

Matt -- do you have an STR and/or screenshot that demonstrates the circumstances under which this ends up being displayed to users?

Flags: needinfo?(MattN+bmo)

The easiest way is to delete Firefox*.app from the Keychain entry in Keychain Access.app and then trigger the dialog to appear again with extensions.formautofill.reauth.enabled = true and then editing a saved card.

The more common user STR would be:

  1. Enable CC autofill and extensions.formautofill.reauth.enabled in two different channels of Firefox (two different profiles and binaries)
  2. Save a credit card in one
  3. Try to edit the card in the other .app
Flags: needinfo?(MattN+bmo)

One option could be to not enforce a prefix and allow each app to specify its own string. We may also want to use two separate strings for Account Name vs. Service Name like Chrome does.

Then because of bug 1499241 and how macOS works you will see multiple of these prompts in a row even if you grant access with Allow. If the user uses Always Allow then both Firefox can access the same key.

Duplicate of this bug: 1624646

Digging into this, it looks like the bulk of the changes required here are in the PSM module, which is where the "org.mozilla.nss.keystore.*" prefix is enforced. I'm tagging Dana Keeler in as the module owner (and I believe main implementor) for that module.

Dana: my specific question is: given the UX papercuts demonstrated in the screenshots above, does it make sense to keep the policy that requires key identifiers to be prefixed in this way? (Alternately: do you know of a way to display a more friendly name to the user in the dialogs shown above while maintaining the current policy?)

Flags: needinfo?(dkeeler)

I think it's fine to change how this works. If I recall correctly, the original motivation behind that prefix was to use something that nothing else would be using, but my understanding is that really only applies in the case where NSS is the backend storing the secrets (and in any case there's nothing actually stopping something else from using that label).
My only concern here is that if anything has actually used this secret storage API yet, we'll need to migrate the keys.

Flags: needinfo?(dkeeler)

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #9)

My only concern here is that if anything has actually used this secret storage API yet, we'll need to migrate the keys.

Or we make that consumer add the prefix to their string. I don't know of any consumers in m-c that have shipped but maybe TB or some fork has used it… I don't think we really need to worry about them since they can easily add the old prefix back.

I did a quick run through the codebase, and agree with MattN's assessment: The six methods in OSKeyStore that use this prefix string are only currently called via IDL, and the only user of nsIOSKeyStore is OSKeyStore.jsm. That class -- or, at least its relevant methods -- are only called from FormAutofillStorage.jsm and its tests (which only use it for credit card numbers).

So, Dana, to be clear: would you be okay if I just pulled mLabelPrefix out of OSKeyStore completely and passed the caller-provided label as-is to the underlying OS-specific keystore? We know that this will break any existing stored credit cards, but since we haven't deployed the feature yet, we're willing to deal with that break.

Flags: needinfo?(dkeeler)

That sounds good to me. Please also update the documentation to make it clear that it's the caller's responsibility to not use duplicate labels (which actually could always happen, but anyway).

Flags: needinfo?(dkeeler)
Attachment #9154789 - Attachment description: Bug 1639795: Update keystore name to be user-friendly → Bug 1639795: Update keystore name to be user-friendly r?keeler,zbraniecki,MattN

Holly -- I'd like your feedback on the exact string we'll use here. My current patch calls it {productname} Encrypted Storage (so "Firefox Encrypted Storage" for release builds, "Firefox Nightly Encrypted Storage" for official Nightly builds, and "Nightly Encrypted Storage" for unbranded local builds). This appears in a couple of places in the macOS key management interface. Neither of these are part of the normal flow of adding, editing, or using a stored credit card. Users will only encounter them if they go looking in the Mac "Keychain Access" application, or if they have gone into that application and deauthorized Firefox from using the "Firefox Encrypted Storage" key.

Flags: needinfo?(hcollier)

Here's an example of how the string looks in the "Keychain Access" application.

Okay, there's a complicated discussion going on over on the Phabricator patch that may well change how we need to handle this, so I'm removing the ni? for now.

Flags: needinfo?(hcollier)
Pushed by adam@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/2d8e1ff06427
Update keystore name to be user-friendly r=MattN,keeler
https://hg.mozilla.org/integration/autoland/rev/69cc1de6f754
Part II - "Migrate" older cards by deleting them. r=MattN
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
Whiteboard: [cc-autofill-mvp]
Priority: P3 → P1
Flags: qe-verify+

Verified with 79.0b4 on macOS 10.15.5.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.