Use a friendlier name for the OSKeyStore Keychain item
Categories
(Core :: Security: PSM, enhancement, P1)
Tracking
()
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.
Reporter | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
Matt -- do you have an STR and/or screenshot that demonstrates the circumstances under which this ends up being displayed to users?
Reporter | ||
Comment 2•4 years ago
|
||
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:
- Enable CC autofill and
extensions.formautofill.reauth.enabled
in two different channels of Firefox (two different profiles and binaries) - Save a credit card in one
- Try to edit the card in the other .app
Reporter | ||
Comment 3•4 years ago
|
||
Reporter | ||
Comment 4•4 years ago
|
||
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.
Reporter | ||
Comment 5•4 years ago
|
||
Reporter | ||
Comment 6•4 years ago
|
||
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.
Assignee | ||
Comment 8•4 years ago
|
||
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?)
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.
Reporter | ||
Comment 10•4 years ago
|
||
(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.
Assignee | ||
Comment 11•4 years ago
|
||
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.
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).
Assignee | ||
Comment 13•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 14•4 years ago
|
||
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.
Assignee | ||
Comment 15•4 years ago
|
||
Here's an example of how the string looks in the "Keychain Access" application.
Assignee | ||
Comment 16•4 years ago
|
||
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.
Assignee | ||
Comment 17•4 years ago
|
||
Depends on D78610
Comment 18•4 years ago
|
||
Comment 19•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2d8e1ff06427
https://hg.mozilla.org/mozilla-central/rev/69cc1de6f754
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 20•4 years ago
|
||
Verified with 79.0b4 on macOS 10.15.5.
Description
•