[PM-26459] Implement data envelope

[quexten]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-26459
https://bitwarden.atlassian.net/wiki/spaces/EN/pages/2052653095/Draft+Tech+Breakdown+-+DataEnvelope

📔 Objective

Important

The confluence document has a more detailed breakdown. This is a shortened version.

Consumers want to protect complex structs such as:

• Vault exports
• Ciphers (vault items)
• Organization reports

Currently, they serialize these themselves (organization reports are serialized as json), and encrypt fields individually. This is slow to handle during encrypt/decrypt, and complex to maintain. The server now has to know the same representation as clients. At the same time, this can be abused by the server omitting certain fields, or swapping encrypted fields encrypted under the same key.

EncStrings are not the correct abstraction layer. A new abstraction / API that is safe and easier to use (an impossible to misuse) is needed.

Security Definition

Attacker model: The relevant attacker model here is a fully compromised server. That’s what E2E encryption / zero knowledge protect against.

Security Goals:

SG1: The structure should not be modifiable (malleable)

• Reason: This can be abused by the attacker by tampering with the encrypted item in (to the developer) unexpected ways

SG2: The attacker should not be able to infer anything about the contents of the structure aside from length

• Reason: We have a “Zero knowledge” product and customers expect their data to not be readable by anyone but them.

SG3: The structure should only be encryptable in the specific section of code that the developer intends

• I.e it should not be possible to swap an encrypted vault item into an encrypted “user settings” “slot”, since the behavior here is undefined.
• Reason: Same as SG1

This PR implements a "DataEnvelope". The DataEnvelope solves the problem of encrypting entire structs. The caller just provides a struct that is Serializable/Deserializable, and gets back an encrypted blob. The caller does not decide serialization.

Further, we force the creation of a "content encryption key" by the interface. This ensures that teams do not create a coupling to keys. When implementing key-rotation we do not want to re-upload data, but only re-upload re-encrypted keys, and the content-encryption-key enforces this key being present.

An example is provided to show usage.

⏰ 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 – 9eaf16b4-bb4c-4cb2-802c-16fcff9e3ae2

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

❌ Patch coverage is 81.12392% with 131 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.34%. Comparing base (d553d37) to head (8c8568b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/bitwarden-crypto/src/safe/data_envelope.rs	70.52%	102 Missing ⚠️
crates/bitwarden-crypto/src/content_format.rs	0.00%	6 Missing ⚠️
crates/bitwarden-crypto/src/store/context.rs	92.20%	6 Missing ⚠️
crates/bitwarden-crypto/src/uniffi_support.rs	0.00%	5 Missing ⚠️
crates/bitwarden-crypto/src/cose.rs	91.30%	4 Missing ⚠️
...twarden-crypto/src/safe/data_envelope_namespace.rs	80.00%	3 Missing ⚠️
...rates/bitwarden-crypto/src/enc_string/symmetric.rs	95.74%	2 Missing ⚠️
...bitwarden-vault/src/cipher/cipher_client/create.rs	0.00%	1 Missing ⚠️
...s/bitwarden-vault/src/cipher/cipher_client/edit.rs	0.00%	1 Missing ⚠️
...es/bitwarden-vault/src/cipher/cipher_client/mod.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #336      +/-   ##
==========================================
- Coverage   78.37%   78.34%   -0.03%     
==========================================
  Files         291      293       +2     
  Lines       29391    29892     +501     
==========================================
+ Hits        23034    23419     +385     
- Misses       6357     6473     +116     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Hinton]

Overall looks great, some general thoughts.

[Hinton]

I don't know if these macros provide much value, at best they save 2 lines of code but they also interup the control flow. I know we have precedence of this with require! ...

[quexten]

Imo, in this case it's not so much about saving 2 lines, versus readability. The ensure is used in a cluster of checks. Thus, matches or an equality check seem (slightly) harder to parse than something that states "ensure_{matches/equal}":

ensure_matches!(msg.protected.header.alg, Some(coset::Algorithm::PrivateUse(XCHACHA20_POLY1305)) => DataEnvelopeError::DecryptionError);
ensure_equal!(msg.protected.header.key_id, cek.key_id => DataEnvelopeError::WrongKey);
ensure_equal!(envelope_namespace, *namespace => DataEnvelopeError::InvalidNamespace);
ensure_equal!(content_format, CONTENT_TYPE_PADDED_CBOR => DataEnvelopeError::UnsupportedContentFormat);

vs

if !matches(msg.protected.header.alg, Some(coset::Algorithm::PrivateUse(XCHACHA20_POLY1305))) {
  return Err(DataEnvelopeError::DecryptionError);
}
if !msg.protected.header.key_id == cek.key_id {
  return Err(DataEnvelopeError::WrongKey);
}
if !envelope_namespace == *namespace {
  return Err(DataEnvelopeError::InvalidNamespace);
}
if !content_format == CONTENT_TYPE_PADDED_CBOR {
  return Err(DataEnvelopeError::UnsupportedContentFormat);
}

[Hinton]

I'm just saying that in the first case to understand what's actually going in I need to jump into the macros. The syntax is somewhat confusing with the last argument being an expression.

Unwrapped everything I need to know to follow the execution flow is there.

[quexten]

That makes sense. These were intended to make readability nicer, if they don't I'll undo them.

Looking for additional feedback @mzieniukbw and @Thomas-Avery. Do you have thoughts on readability here?

[quexten]

Actually, I'll make this an item for the next KM dev sync. I'll make a follow-up PR to undo the macros assuming the outcome is that they don't provide readability benefit.

[quexten]

Update: undoing this in a follow-up PR.

[Hinton]

question: This is generally called Cipher in other places. Could this cause confusion?

[quexten]

The vault team is free to rename this entry; However I'll argue that despite precedent of using the term for a long time, Cipher is extremely confusing, especially in the context of an end-to-end encrypted app. Thus I opted for simpler naming here.

I've added a comment clarifying that this is the same as "ciphers"

[Hinton]

CC @bitwarden/team-vault-dev

[Thomas-Avery]

Looking good, have a few minor things and some questions/confusion around versioning.

[quexten]

Claude, re: Critical issues:

1. The CEK keyid collision is just incoherent nonsense. Even if the key id collides, the key is different so nothing can be swapped between envelopes and SG1, SG3 are not broken.

2. This is not correct, we don't pass in any ephemeral AAD yes, but cose creates AAD from the protected headers.

3. No, it does not. The mac is validated before the padding. Only the correct padded message decrypts.

4. This is not a security issue but a usability issue, we just don't want developers to mis-use the key for unintended operations.

As the review is not useful, I'm muting it.