[PM-19801] Clear device keys on deactivate #5592
Conversation
quexten
changed the title
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5592 +/- ##
==========================================
+ Coverage 44.90% 44.99% +0.08%
==========================================
Files 1559 1563 +4
Lines 71160 71595 +435
Branches 6357 6407 +50
==========================================
+ Hits 31957 32213 +256
- Misses 37851 38015 +164
- Partials 1352 1367 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| EncryptedPublicKey IS NOT NULL OR | ||
| EncryptedPrivateKey IS NOT NULL | ||
| ); | ||
| GO; |
quexten
requested review from
a team and
Patrick-Pimentel-Bitwarden
and removed request for
a team
|
Added auth as reviewer since they own the device management feature (even if they don't own this service). |
There was a problem hiding this comment.
💭 : This script will need to be translated to the other database providers that we support PostgreSQL, SQLite, and MySQL (MariaDb) and ran using the entity framework migrator. For example
Should be easy enough to translate, you just have to create an empty Entity Framework migration (just run the migrator script IIRC) then edit the migration file to run the script, like the one I linked. You'll need to place the script in the HelperScripts directory of the migrator project.
A gotcha is postgreSQL uses the .psql file extension.
Since this is data deletion, I would be very careful to test it.
There was a problem hiding this comment.
Talked with the Auth Team and we aren't sure that there are any devices now that are in a state where device.active = false and device.<keys> != null. Because the de-active feature isn't live yet.
@rkac-bw do we have any devices today that meet this criteria?
# Psudo
SELECT COUNT(*)
FROM dbo.devices d
WHERE d.active = 0
AND d.EnryptedPrivateKey IS NOT NULL;If there are no devices that meet that criteria then there is no need for the script since we have modified the service to remove the keys when device.Active = false.
There was a problem hiding this comment.
Ok, removed the migration for now. If these users do happen to exist we can always add a migration later.
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-19801
📔 Objective
Encryption keys need to be removed from the device table once a device is deactivated, because the user expects to have revoked access, but e.g. key rotation and other operation will continuously re-share the new encryption keys to the devices.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes