[Vault] Cipher Versioning POC #384
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #384 +/- ##
==========================================
+ Coverage 74.09% 74.47% +0.37%
==========================================
Files 253 256 +3
Lines 21781 22160 +379
==========================================
+ Hits 16138 16503 +365
- Misses 5643 5657 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
| .find(|m| m.source_version() == current_version) | ||
| .ok_or(CipherError::UnsupportedCipherVersion(current_version))?; | ||
|
|
||
| migration.migrate(cipher_data, ctx.as_deref_mut(), cipher_key)?; |
There was a problem hiding this comment.
This is hard for me to conceptualize when or why this would happen, but I'll throw it out there.
What would be the experience if a migration were to fail? I would imagine that there would be some ciphers that have migrated and some that have not. It's hard to say why this would happen and maybe we don't know that it ever would.
There was a problem hiding this comment.
Yeah, good question. I thought about this, but didn't capture it in this POC. We might have migration failures in cases where we need to decrypt or encrypt and then encounter a failure. The way I'll address this is to create a back up before migration and then if we encounter an error we restore the backup on failure
There was a problem hiding this comment.
👍 Good idea on the backup! That probably is the most straight forward approach. Being in a partial state could spin up any number of edge cases.
| let enc_string: EncString = credential_id_str.parse()?; | ||
| let dec_credential_id: String = enc_string.decrypt(ctx, ciphers_key)?; | ||
| let b64_credential_id = BASE64_STANDARD.encode(&dec_credential_id); | ||
| let enc_credential_id: EncString = | ||
| b64_credential_id.encrypt(ctx, ciphers_key)?; |
There was a problem hiding this comment.
issue: I think this migration does a good job as an example of how one would modify a field, but the what it actually does (since this is a real example) is wrong. Probably don't need to fix it in the PoC, but this should not be merged to production as-is.
What it does:
It takes the UUID as a string directly and encodes the Unicode bytes as B64 i.e. UUID -> B64.
E.g. the UUID standard format 0000 ... will be converted to the unicode bytes 0x30 0x30 0x30 0x30 ... and saved as B64
What it should do:
Take the UUID standard format and convert it into raw format e.g. 0000 ... means 0x00 0x00 and then save that as B64. There are already existing examples and functions for this so you don't need to write your own :)
There was a problem hiding this comment.
Oh nice, Thanks for pointing that out
| /// The version of the cipher data. Default is 1 for backward compatibility. | ||
| #[serde(rename = "version", skip_serializing_if = "Option::is_none")] | ||
| pub version: Option<i32>, |
There was a problem hiding this comment.
🤔 Is there any important distinction between version: Some(1) and version: None? If not then you could use serde(default = )to default it to 1 and ditch the Option.
This would mean writing version: 1 on save - would that matter for older clients, or do they already safely ignore unknown fields?
| fn migrate( | ||
| &self, | ||
| cipher_data: &mut serde_json::Value, | ||
| _ctx: Option<&mut KeyStoreContext<KeyIds>>, | ||
| _cipher_key: Option<SymmetricKeyId>, | ||
| ) -> Result<(), crate::CipherError> { | ||
| if let Some(obj) = cipher_data.as_object_mut() { | ||
| if !obj.contains_key("SecurityQuestions") { | ||
| obj.insert( | ||
| "SecurityQuestions".to_string(), | ||
| serde_json::Value::Array(vec![]), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
💭 I'm still digging through the feature docs elsewhere, but here are some brainstorming / initial thoughts:
Where is the spec defined for each version - should we maintain a spec for each version inside the SDK code?
From this code it looks like each version will have a structured schema that we want to be able to enforce / verify - Rust's typing system is really good for this type of migration. I would suggest it defining each version as its own structured data type.
A few benefits of this would be:
- make the code a little bit safer & error-resistant for invalid or mispelled keys.
- Give us a place in the SDK code where the version schemas are strictly defined, making interfacing with the data elsewhere in the SDK easier and more maintainable.
- We could potentially create an
enum CipherData { .... }, and consolidate some split-versioning logic to that struct.- Perhaps adding getters for consistent data access between versions.
- We could skip the need for these isolated
Migrationstructs, in favor of something likeimpl From<CipherDataV1> for CipherDataV2 {...}(orTryFrom, if we anticipate the possibility of errors - ref: @nick-livefront's comment above)
If we have a need for unstructured elements in the version as well, we could use #[serde(flatten)] to create a catch-all for unmatched values, as shown in this example
| #[test] | ||
| fn test_v1_to_v2_adds_security_questions() { | ||
| let mut data = serde_json::json!({ | ||
| "Username": "2.PE7g9afvjh9N57ORdUlCDQ==|d8C4kLo0CYAKfa9Gjp4mqg==|YmgGDxGWXtIzW+TJsjDW3CoS0k+U4NZSAwygzq6zV/0=", | ||
| "Password": "2.sGpXvg4a6BPFOPN3ePxZaQ==|ChseXEroqhbB11sBk+hH4Q==|SVz2WMGDvZSJwTivSnCFCCfQmmnuiHHPEgw4gzr09pQ=", | ||
| "Uris": [], | ||
| "Totp": null | ||
| }); | ||
|
|
||
| V1ToV2Migration.migrate(&mut data, None, None).unwrap(); | ||
|
|
||
| assert!(data.get("SecurityQuestions").is_some()); | ||
| assert_eq!(data["SecurityQuestions"], serde_json::json!([])); | ||
| } | ||
| } |
There was a problem hiding this comment.
⛏️ Would it be benficial to store some .json files with some actual test-data to verify the before/after for various cipher types?
🎟️ Tracking
📔 Objective
⏰ Reminders before review
team
🦮 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 confirmedissue 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