Clean up crypto modules that are used/referenced from various platforms
Categories
(Firefox :: Migration, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox110 | --- | fixed |
People
(Reporter: Mardak, Assigned: brianpt1106, NeedInfo)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
Bug 1613337 added a ChromeWindowsLoginCrypto.jsm similar to ChromeMacOSLoginCrypto.jsm instead of directly importing OSCrypto.jsm from ChromeProfileMigrator.jsm.
MattN suggested as part of https://phabricator.services.mozilla.com/D64713 refactoring that it might be reasonable to get rid of OSCrypto/_win as it's not quite how things were expected of it.
Before the change, OSCrypto.jsm was actually only referenced from windows, but all_files_referenced let it pass as it was a false negative. But with the change, an entry was added to all_files_referenced whitelist for now.
Updated•8 months ago
|
Updated•6 months ago
|
Comment 1•6 months ago
|
||
Hey Mardak, am I understanding this right that what we want to do here is get rid of toolkit/components/passwordmgr/OSCrypto.jsm and toolkit/components/passwordmgr/OSCrypto_win.jsm altogether?
Comment 2•6 months ago
|
||
Cc'ing sgalich, since if I understand correctly, this will touch modules under passwordmgr.
Reporter | ||
Comment 3•6 months ago
|
||
I think it was more that they could somehow be combined as OSCrypto only supports windows and was only imported via ChromeWindowsLoginCrypto, which is only packaged for windows similar to OSCrypto_win conditionally imported and packaged only for windows:
conditional imports:
https://searchfox.org/mozilla-central/rev/9423bbd90c59ef7739c8453c265d04d8cf07ce73/toolkit/components/passwordmgr/OSCrypto.jsm#17,21-22
https://searchfox.org/mozilla-central/rev/9423bbd90c59ef7739c8453c265d04d8cf07ce73/browser/components/migration/ChromeProfileMigrator.sys.mjs#258-260
conditional package:
https://searchfox.org/mozilla-central/rev/9423bbd90c59ef7739c8453c265d04d8cf07ce73/browser/components/migration/moz.build#38,50,52
https://searchfox.org/mozilla-central/rev/9423bbd90c59ef7739c8453c265d04d8cf07ce73/toolkit/components/passwordmgr/moz.build#67-69
Basically this test exception means we're unnecessarily packaging OSCrypto on mac/linux for this bug to clean up:
https://searchfox.org/mozilla-central/rev/9423bbd90c59ef7739c8453c265d04d8cf07ce73/browser/base/content/test/static/browser_all_files_referenced.js#226-229
Comment 4•6 months ago
|
||
Ah, I see. Perhaps there was some wish or hope down the line that we'd have OSCrypto_mac and OSCrypto_linux implementations, and that OSCrypto.jsm would be the front-end for it, kind of like OSFile.jsm?
I think I understand though: until there's a need for a macOS or Linux implementation, we don't need this abstraction. Would this suffice as a plan?:
- Update ChromeWindowsLoginCrypto.sys.mjs to import OSCrypto_win directly, rather than importing OSCrypto.jsm.
- Get rid of OSCrypto.jsm
sgalich, just double-checking, there weren't plans on your roadmap to add Linux / macOS backends for OSCrypto.jsm, were there?
Comment 5•6 months ago
|
||
It feels that the hopes were to add other platforms, but it never happened and I can't tell if they will have same API shape anyway. Lets get rid of OSCrypto.jsm and just import OSCrypto_win directly as suggested.
Comment 6•6 months ago
|
||
Note there is browser/components/migration/ChromeMacOSLoginCrypto.jsm
that has this comment:
Instances of this class have a shape similar to OSCrypto so it can be dropped
into code which uses that. This isn't implemented as OSCrypto_mac.js since
it isn't calling into encryption functions provided by macOS but instead
relies on OS encryption key storage in Keychain. The algorithms here are
specific to what is needed for Chrome login storage on macOS.
Comment 7•6 months ago
|
||
It feels that the hopes were to add other platforms, but it never happened and I can't tell if they will have same API shape anyway. Lets get rid of OSCrypto.jsm and just import OSCrypto_win directly as suggested.
Sounds good, thanks!
So Brian, it sounds like we're going to want to follow the outline of comment 4 here.
Assignee | ||
Comment 8•6 months ago
|
||
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/38be9015acf7 Clean up crypto modules that are used/referenced from various platforms. r=mconley,credential-management-reviewers,dimi.
Comment 10•5 months ago
|
||
bugherder |
Description
•