[PM-6271] Propose cipher versioning scheme dynamic data

[coroiu]

🎟️ Tracking

📔 Objective

This is a version of the proposal that uses a "dynamic data object" i.e. the cipher data object is not statically typed. Migrators instead rely on runtime checks to perform their work.

Of course, they don't have to use dynamic objects. A migrator is just a function and it can be implemented however you like

📸 Screenshots

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
  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 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

[github-actions]

Checkmarx One – Scan Summary & Details – 14643cbe-68c3-4c53-afdd-20d8df1eb857

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Privacy_Violation	/languages/java/src/main/java/com/bitwarden/sdk/SecretsClient.java: 40	Attack Vector
	Privacy_Violation	/languages/java/Example.java: 13	Attack Vector
	Privacy_Violation	/languages/java/Example.java: 12	Attack Vector
	Unpinned Actions Full Length Commit SHA	/publish-rust-crates.yml: 200	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...

Fixed Issues

Severity	Issue	Source File / Package
	Privacy_Violation	/languages/java/example/Example.java: 16
	Privacy_Violation	/languages/java/example/Example.java: 17
	Privacy_Violation	/languages/java/src/main/java/com/bitwarden/sdk/SecretsClient.java: 40
	Unpinned Actions Full Length Commit SHA	/publish-rust-crates.yml: 56

[Hinton]

I've unfortunately only taken a quick look at this, and it deserves significantly more time to wrap my mind around everything.

There are quite a few things to like here, but also some concerns.

I noticed password_history is not in data currently. And data contains items that are currently not always encrypted. I'm worried we will get into scenarios where we need to migrate fields outside the data blob.

It also feels like we're doing much of the work for the cipher refactor in the client without doing the slightly additional work of updating the server too.

I'm curious what items if any we access in the Cipher in a decrypted state. Would it be viable to do this:

struct Cipher {
  id: UUID,
  meta_data: UnreadableBlob,
  full_data: UnreadableBlob,
  version: 1
}

Migrating requires the whole context of Cipher, and would affect content in the data blobs, and the version field.

meta_data and full_data in this struct can't be parsed. Migration is implicitly done while decrypting, or through an explicit migrate step.

struct CipherViewFull {
  id: UUID,
  data: {
    login: {
      ..
    },
    edit: true
  }
}

[Hinton]

thought: A versioning crate feels a bit odd. I think it's fine to put this in the core crate.

[coroiu]

Yeah I agree, I thought this was gonna be much bigger but alas :)

[coroiu]

@Hinton

I noticed password_history is not in data currently.

I forgot to add password_history to the data object and then I just never got around to it because of how fast I was trying to get this out. In other words, it can definitely move into Data

And data contains items that are currently not always encrypted.

What I tried to do was to think of Data as a structure defined entirely by the client, while everything outside of Data is defined by the server. We can definitely move those fields into another object called Metadata if we want to make that distinction clearer.

I'm worried we will get into scenarios where we need to migrate fields outside the data blob.

Given this distinction, migrating fields outside of the data blob should never be a thing. At least not on the client. Those fields are owned by the server and should be handled appropriately.

It also feels like we're doing much of the work for the cipher refactor in the client without doing the slightly additional work of updating the server too.

Not sure I follow, but I have skipped the server at this stage because I think those changes are much more straight forward. In other words I didn't think we needed to prove the concept on the server side.