Closed Bug 1619090 Opened 3 years ago Closed 5 months ago

Clean up crypto modules that are used/referenced from various platforms

Categories

(Firefox :: Migration, task, P3)

Product:
Firefox
Component:
Migration
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
110 Branch
Tracking Flags:
Tracking Status
firefox110 --- fixed

People

(Reporter: Mardak, Assigned: brianpt1106, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1619090 - Clean up crypto modules that are used/referenced from various platforms. r?mconley.
48 bytes, text/x-phabricator-request
Details | Review

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.

Blocks: 1513718
Priority: -- → P3
Severity: normal → S3
Assignee: nobody → brianpt1106

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?

Flags: needinfo?(edilee)

Cc'ing sgalich, since if I understand correctly, this will touch modules under passwordmgr.

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?:

  1. Update ChromeWindowsLoginCrypto.sys.mjs to import OSCrypto_win directly, rather than importing OSCrypto.jsm.
  2. 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?

Flags: needinfo?(sgalich)
Flags: needinfo?(edilee)

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.

Flags: needinfo?(sgalich)

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.

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.

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.

https://hg.mozilla.org/mozilla-central/rev/38be9015acf7

Status: NEW → RESOLVED
Closed: 5 months ago
status-firefox110: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 110 Branch
You need to log in before you can comment on or make changes to this bug.