-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: NSS managed logins key storage #6362
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6362 +/- ##
==========================================
- Coverage 22.00% 21.86% -0.14%
==========================================
Files 342 343 +1
Lines 30712 30907 +195
==========================================
Hits 6759 6759
- Misses 23953 24148 +195 ☔ View full report in Codecov by Sentry. |
cf850b8
to
efc9553
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great - maybe we could lean into it further :)
Note that only makes sense once you are actually moving ahead rather than exploring.
The existing sig for mobile is, eg:
pub fn update(&self, id: &str, entry: LoginEntry, enc_key: &str)
where you have removed that key. Which is much better for a public api.
Alternatively, we could have, say
trait KeyStore { ... }
- there would be a few options for what this should do, but it could start with just get_key()
.
The public API could use Arc<dyn KeyStore>
and maybe could be passed to the constructors. You could impl that trait like done here and mobile could impl that trait wrapping their string key (or however they like, we just don't want to hold it ourselves)
Or really, it seems like EncryptorDecryptor
could be a trait?
We could probably change many rust signatures here but avoid a breaking change for consumers by updating the wrappers.
There would be a few hiccups, but we might be able to end up with a great api that we can use everywhere. But as above, that's more for once you lean into this :)
* License, v. 2.0. If a copy of the MPL was not distributed with this | ||
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
||
namespace logins { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are very very close to being able to use procmacros, which would remove the need for all this duplication :)
+1 to this design. I really like the pattern of using traits for functionality that we can't always handle in Rust and giving consumers multiple options for constructing an implementation. |
55d752c
to
6387e2b
Compare
Run the test via ``` cd components/logins mkdir profile PROFILE_DIR=profile cargo test test_managed_store::test_general --features keydb ```
6387e2b
to
dc116a5
Compare
This pull request is a proof of concept, representing an initial approach to integrating key management for the logins component in Firefox’s desktop password manager. The implementation is still in its early stages and serves as a foundation for further discussion and refinement.
Objective
To integrate the logins component into the desktop environment and replace the existing logins backend with the Rust implementation, key management must be incorporated. Currently, NSS handles this, ensuring that the application code doesn't directly interact with key management. Instead, it simply uses the encrypt and decrypt endpoints provided by NSS, which internally access the corresponding keys stored in the slot.
Another approach would be to handle this integration on the JavaScript side, but it would present similar challenges due to the absence of the necessary bindings. Here, I have pursued the Rust path.
Contents of the Pull Request
This pull request includes the following changes:
keydb
:keydb
has been added, which provides the additional functionalities listed below.ManagedLoginStore
:ManagedLoginStore
, responsible for generating, persisting, and retrieving the key, providing the same API asLoginStore
, but with transparent key management.pub fn PK11_ListFixedKeysInSlot(...) -> *mut PK11SymKey;
nss::pk11::sym_key::retrieve_or_create_and_import_and_persist_aes256_key_data(name: &str) -> Result<Vec<u8>>
:ensure_nss_initialized
has been adjusted based on the new feature flag, allowing keys to be persisted.Open Questions/Problems/TODOs
ensure_nss_initialized
, this patch reads the path to the profile (where the NSS key databases are located) from thePROFILE_DIR
environment variable based on the feature flag. This approach might not be optimal. We need to explore better ways to pass the path.UDL
Integration:keydb
feature flag. I bet there is a better way?ManagedLoginStore
:Arc
handling):reset
register_with_sync_manager
create_logins_sync_engine
ManagedStore
:ManagedStore
struct.LoginStore
should be modified to accept aLoginManager
trait implementation, allowing key management to be abstractedPull Request checklist
[ci full]
to the PR title.Branch builds: add
[firefox-android: branch-name]
to the PR title.