[PM-31052] User V2UpgradeToken for key rotation without logout#6995
[PM-31052] User V2UpgradeToken for key rotation without logout#6995mzieniukbw wants to merge 6 commits intomainfrom
Conversation
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6995 +/- ##
==========================================
+ Coverage 56.26% 60.33% +4.07%
==========================================
Files 1983 1989 +6
Lines 87651 87723 +72
Branches 7815 7820 +5
==========================================
+ Hits 49318 52932 +3614
+ Misses 36503 32873 -3630
- Partials 1830 1918 +88 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| SetV1ModelUser(model); | ||
|
|
||
| var originalSecurityStamp = user.SecurityStamp = Guid.NewGuid().ToString(); | ||
| var wrappedKey1 = "2.key1==|data1==|hmac1=="; |
There was a problem hiding this comment.
Nit: The V1 key that's wrapped would be "7.XXXXXXXXX" since the upgrade migration always is to a COSE key.
| { | ||
| var data = new V2UpgradeTokenData | ||
| { | ||
| WrappedUserKey1 = "2.key1==|data1==|hmac1==", |
|
|
||
| public class V2UpgradeTokenRequestModelTests | ||
| { | ||
| private const string _validWrappedKey1 = "2.AOs41Hd8OQiCPXjyJKCiDA==|O6OHgt2U2hJGBSNGnimJmg==|iD33s8B69C8JhYYhSa4V1tArjvLr8eEaGqOV7BRo5Jk="; |
| data.AccountKeys.SignatureKeyPair = null; | ||
| data.AccountUnlockData.V2UpgradeToken = new V2UpgradeTokenRequestModel | ||
| { | ||
| WrappedUserKey1 = "2.key1==|data1==|hmac1==", |
| userEntity.AccountRevisionDate = user.AccountRevisionDate; | ||
| userEntity.RevisionDate = user.RevisionDate; | ||
|
|
||
| userEntity.SignedPublicKey = user.SignedPublicKey; |
There was a problem hiding this comment.
Oof. Makes me wonder if we should QA V2 rotations on EF separately?
| } | ||
|
|
||
| [Theory, BitAutoData] | ||
| public async Task RotateUserAccountKeysAsync_WithExistingToken_WithNewToken_UpdatesToken( |
There was a problem hiding this comment.
Hmm, thinking through this scenario, I think we only ever expect one upgrade rotation, when going from V1 encryption to V2 encryption. I'm not sure when we would do a second rotation / if we would do this, then I do believe some devices would break.
If a device is on UK1, then goes to UK2, the token T12 allows devices with local method unlocking UK1 to get to UK2.
But if we now update from UK2 to UK3, then the token is T23. If a device still has unlock methods unlocking UK1, then the token would not function / break.
Should we instead only allow the token to be present if:
- The user is a v1 user
- AND The key-rotation contains a V2 cryptographic state
- AND There is no upgrade token present on the user table
OTHERWISE -> This is a regular logout key-rotation?
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31052
📔 Objective
The V2 upgrade token provides a way for upgrade rotations to retain access to old encrypted data, and both the old and new user-key for a temporary period.
Regular key rotations will still logout - v2 upgrade token expected to be null.
Exposes the V2 upgrade token in sync's user decryption options response, so other clients will be able to unlock with old and new user key.
Also fixes a bug in EF UserRepo, where User's V2 fields were not set (security state, version and signed public key)
📸 Screenshots