Bug 2007416 - wipe individual logins on DecryptionErrors #7343
Bug 2007416 - wipe individual logins on DecryptionErrors #7343bendk wants to merge 2 commits intomozilla:mainfrom
Conversation
bendk
force-pushed
the
push-myztqrztlsuu
branch
from
6c5a0c6 to
e8513bf
Compare
| named_params! { | ||
| ":now_millis": now_ms, | ||
| ":guid": id, | ||
| ":sync_status": SyncStatus::Changed as u8, |
There was a problem hiding this comment.
touch() now also updates the sync_status column. I needed this to make the tests pass, but it seems like the code should have always done that to me. Maybe we were just not using this method enough to notice?
| Ok(Some(ServerTimestamp(millis))) | ||
| Ok(db | ||
| .get_meta::<i64>(schema::LAST_SYNC_META_KEY)? | ||
| .map(ServerTimestamp)) |
There was a problem hiding this comment.
This also needed changes to make the tests pass, because it seemed like the old code was not correct. Before this would panic if there was no metadata for LAST_SYNC_META_KEY, now it returns None.
| self.execute("DELETE FROM loginsM WHERE guid=?", [guid])?; | ||
| // Delete the LAST_SYNC_META_KEY so that we resync the logins data next time | ||
| self.delete_meta(schema::LAST_SYNC_META_KEY)?; | ||
| tx.commit()?; |
There was a problem hiding this comment.
There's lots of incidental changes in this PR, but this is the main change.
Reworked how we handle `EncryptorDecryptor`: * Don't pass `EncryptorDecryptor` to database functions, this is redundant because the DB has it's own enc_dec arc. * Make `EncryptorDecryptor::decrypt_login` input a `LoginDb`. This enables a future change where we wipe the login on decryption failures. Most of this is just a pure refactor. The one real change is that we need to lock the store to get a `LoginDb` a bit more in the sync engine code.
bendk
force-pushed
the
push-myztqrztlsuu
branch
2 times, most recently
from
99137dd to
aac821a
Compare
| username: "cool_username".into(), | ||
| password: "hunter2".into(), | ||
| ..Default::default() | ||
| }) |
There was a problem hiding this comment.
I think I probably broke this test and I'm not sure exactly how to fix it or even run it (do I need to create a burner sync account for this?).
Is this still worthwhile to keep? It seems a bit redundant with the TPS tests. I can definitely fix this if it's providing value, but I want to make sure someone's using it before I do that.
There was a problem hiding this comment.
I don't think it's redundant with TPS because that's primarily just testing the existing desktop stuff, so the Rust components don't get much coverage - ie, this was intended to kinda be "TPS but for rust components" :) It uses the same auth as that fxa example, so should start an oauth flow and remember the account in .fxa-cli-credentials.json (although I don't recall how it manages 2 accounts!). It doesn't have much value as it's not run automatically etc, so in that regard it is even more like TPS (which also doesn't run in CI, so has questionable value, but we're also reluctant to just kill that because there is some value there)
I'd actually be surprised if it hasn't already gone stale, so I'd be fine with even just leaving a comment here explaining you think it might be broken and a reference to this PR, and we can have a longer discussion about it later.
There was a problem hiding this comment.
You're right it was already stale before this. I tested this on main and it's also failing with:
thread 'main' (3767285) panicked at testing/sync-test/src/logins.rs:278:5:
assertion failed: c1.logins_store.delete(&l0id).expect("Delete should work")
I'm thinking we should leave the code as-is, create a ticket for it, and update the readme.
There was a problem hiding this comment.
I'm still not entirely convinced wiping is the right thing to do here - I recall we started a discussion but don't remember it concluding - I think we agreed to rope in the credentials management team - did they say we should take this route? I'm fine with this if they did.
| username: "cool_username".into(), | ||
| password: "hunter2".into(), | ||
| ..Default::default() | ||
| }) |
There was a problem hiding this comment.
I don't think it's redundant with TPS because that's primarily just testing the existing desktop stuff, so the Rust components don't get much coverage - ie, this was intended to kinda be "TPS but for rust components" :) It uses the same auth as that fxa example, so should start an oauth flow and remember the account in .fxa-cli-credentials.json (although I don't recall how it manages 2 accounts!). It doesn't have much value as it's not run automatically etc, so in that regard it is even more like TPS (which also doesn't run in CI, so has questionable value, but we're also reluctant to just kill that because there is some value there)
I'd actually be surprised if it hasn't already gone stale, so I'd be fine with even just leaving a comment here explaining you think it might be broken and a reference to this PR, and we can have a longer discussion about it later.
I'm not sure either, but my memory is that we agreed this was an okay short-term step since it's the essentially the same behavior as when the canary check fails. Long-term, we should have the discussion with more teams and we may decide to go the other route: keep the DB row around and present it to the user as a login with missing data (or maybe some 3rd route). There actually is some urgency to this since the logins success rates are pretty low and most of the failures are decryption errors. That said, I do think it's a good idea to get the OK from credential management on this. WYDT @jo? BTW, here's the bugzilla bug for the longer-term decision. |
Also: * Set sync_status in `LoginsDb::touch()` * Handle missing row for LAST_SYNC_META_KEY in `get_last_sync()`
|
Oh, I think that's a tricky issue. I'd prefer to control that via a feature flag so we can enable it selectively. I'd be reluctant to have that enabled on the desktop for now. We often have problems getting the key from the OSKeyStore as well. Here, with form autofill data, we had actually implemented a similar mechanism and had just removed it when we discovered it. In this case, if no key was returned, a new one was created and stored immediately, which actively reset the credit card data. With OSKeyStore, there are many different reasons why the key might not be available at the moment. https://sql.telemetry.mozilla.org/queries/113887 lists 29 different errors only for Darwin. While this all pertains to OSKeyStore, I can imagine that similar issues exist on iOS or Android, and that the keys may simply be temporarily unavailable under certain circumstances. For the desktop, we have at least roughly considered various future backup strategies, partly for other reasons as well. One option is via FxA, with manual user interaction. Another possibility is backup codes (as with GH or 1Password) or the primary password. In this context, I could also imagine that we’d at least wait to wipe the data until we’re syncing, right? That way, there’s a high probability the data will be restored immediately. By the way, thanks a lot for the groundwork - I didn’t dare touch that back then :) It’s much cleaner this way. So, to cut a long story short: if we enable this with a feature flag, which we’d initially activate only for Android and/or iOS, I’d be okay with that as an interim solution. |
3 participants
First commit is prep work, without much changes.
Second commit is the actual change.
Pull Request checklist
[ci full]to the PR title.