Conversation
cbfbb17 to
516027a
Compare
503e4c4 to
c37bde5
Compare
c37bde5 to
57f41f7
Compare
57f41f7 to
5574c93
Compare
5574c93 to
b6d4ceb
Compare
jo
left a comment
There was a problem hiding this comment.
Everything looks really clean and well done. A thousand thanks, Tessa - a really great first patch for Application Services! 🎉
Next, we’ll need to work through the Pull Request Checklist, in particular, the corresponding consumer patches should ideally be linked. I think that will be https://phabricator.services.mozilla.com/D325372; I still need to take a look at it. It’s possible that this one is also based on #7576. In that case, we’d need to make sure we merge them one after the other, or better yet, base #7576 against this one.
| anyhow = "1.0" | ||
| error-support = { path = "../support/error" } | ||
| interrupt-support = { path = "../support/interrupt" } | ||
| db-crypto = { path = "../support/db-crypto" } |
There was a problem hiding this comment.
IMO we should set default-features = false here
There was a problem hiding this comment.
agreed, and changed.
| // | ||
| // Note this is only temporarily needed until a bug with UniFFI and JavaScript is fixed, which | ||
| // prevents passing around traits in JS | ||
| pub fn create_autofill_store_with_static_key_manager(path: String, key: String) -> Arc<Store> { |
There was a problem hiding this comment.
I know this is what logins does, but ApiResult<Arc<Store>> might be better to not panic on initialization.
There was a problem hiding this comment.
Done, both store factories return ApiResult<Arc> now ([Throws=AutofillApiError] in the UDL).
1405cb1 to
835f79e
Compare
6cc54e8 to
cec147e
Compare
cec147e to
e893ad6
Compare
jo
left a comment
There was a problem hiding this comment.
I would remove the require_number sanitation, because it changes the API, and I also think the scrubbed cc number is not used as a marker. Also, we are discussion if we can align scrubbing between logins and autofill, and make the latter also delete the entire record.
| // | ||
| // Note this is only temporarily needed until a bug with UniFFI and JavaScript is fixed, which | ||
| // prevents passing around traits in JS | ||
| pub fn create_autofill_store_with_static_key_manager( |
There was a problem hiding this comment.
Would be nice if this would validate the key, eg via canary:
let canary = encdec.encrypt(b"tschilp".to_vec())?;
encdec.decrypt(canary)?;
There was a problem hiding this comment.
Good point, I added the validation.
6156f4a to
637e469
Compare
The pull request has been modified, dismissing previous reviews.
637e469 to
5071768
Compare
Move autofill's credit-card encryption onto the shared db-crypto crate and let AutofillDb own the encryptor, as logins' LoginDb does. The consumer supplies it when building the store, so no key is passed into individual calls or down through the sync layers.
The consumer passes cc_number in cleartext and gets it back decrypted; encryption is internal to the store, which also derives the last-4 digits. A scrubbed card - or one the key cannot read, which the scrub-and-resync flow replaces - comes back with an empty number. Values written before this change decrypt unchanged, so there is nothing to migrate.
Open a Firefox profile's autofill.db with the NSS-managed key, prompting for the primary password in the terminal - like sync-pass does for logins.
With transparent card numbers no consumer handles ciphertext, so the namespace functions that took a key have no callers left. create_autofill_key() stays for consumers that manage a static key.
5071768 to
54fc561
Compare
What
https://bugzilla.mozilla.org/show_bug.cgi?id=2075563
Four commits:
1. The autofill store owns its encryption context.
Store::new()takes anEncryptorDecryptor(from the shareddb-cryptocrate) and hands it to the database; every access to the encrypted column uses it.autofill/src/encryption.rsis gone, and factory functions matching the logins API cover the UniFFI/JS trait gap:create_static_key_manager,create_managed_encdec,create_autofill_store_with_static_key_manager, and - behind the newkeydbfeature -create_autofill_store_with_nss_keymanager(key nameas-autofill-key). Both store factories returnApiResult, so a broken database surfaces as a catchable error rather than a panic.2. Credit card numbers are transparent in the API - like logins, the client does not notice these fields are encrypted.
UpdatableCreditCardFieldstakescc_numberin cleartext (the store encrypts it and derivescc_number_last_4itself, also for the bulk-import APIs), andCreditCard.cc_numbercomes back decrypted - empty for a scrubbed card, while a value the key cannot read fails the read (per review: errors propagate instead of being swallowed). An emptycc_numberis stored as an emptycc_number_encrather than as a ciphertext of "" (per review: a card may be saved without a number, and the empty ciphertext is what marks a scrubbed card for Sync). Values written before this change decrypt unchanged (test_reads_pre_transparency_ciphertextpins that), so there is nothing to migrate.3. A
--profileoption for the autofill-utils example (as requested in review). Opens a Firefox profile'sautofill.dbwith the NSS-managed key, prompting for the primary password in the terminal - likesync-passdoes for logins. First consumer ofcreate_autofill_store_with_nss_keymanageroutside desktop.4. Bug 2075560: The key-based
encrypt_string/decrypt_stringnamespace functions are removed - with transparent card numbers no consumer handles ciphertext, so nothing needs them.create_autofill_key()stays; for canaries usedb_crypto.create_canary/check_canary(format-compatible).Breaking changes
Store::new()takes the encryptor; use the factories. Store construction: DesktopRustAutofillStore.sys.mjs, AndroidAutofillCreditCardsAddressesStorage.kt, iOSRustAutofill.swift.scrub_undecryptable_credit_card_data_for_remote_replacement()no longer takes a key (iOS is its only caller).encrypt_string(key, ...)/decrypt_string(key, ...)removed (AndroidAutofillCrypto.kt& iOSRustAutofill.swift/RustKeychain.swift; Desktop never used these).cc_number(cleartext) replacescc_number_encin both directions, andUpdatableCreditCardFieldsno longer takescc_number_last_4. Consumers stop encrypting/decrypting values entirely - this removes code on all three platforms. Consumer patches are being reworked to this model and will be linked here before landing.Not breaking: the key passed to the sync manager via
local_encryption_keysis accepted but ignored (set_local_encryption_keylogs a warning); removal is tracked in Bug 2075562.Pull Request checklist
This PR follows the breaking change policy: