[BEEEP|POC|Do not merge] Organization membership v2 crypto protocol & New Crypto Primitive Safe IdentitySealedKeyEnvelope

[quexten]

🎟️ Tracking

📔 Objective

Implements v2 organization membership protocol as an example. Further, implements a new SAFE primitive - the IdentitySealedKeyEnvelope.

🚨 Breaking Changes

⏰ 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 – 998e9cbb-36d5-4c68-a893-e01e2aac3c4a

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

[github-actions]

🔍 SDK Breaking Change Detection Results

SDK Version: km/poc-safe-identity-sealed (6a9b377)
Completed: 2025-11-30 22:38:23 UTC
Total Time: 214s

Client	Status	Details
typescript	✅ No breaking changes detected	TypeScript compilation passed with new SDK version - View Details

---

Breaking change detection completed. View SDK workflow

[codecov]

Codecov Report

❌ Patch coverage is 66.60584% with 183 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.23%. Comparing base (7080159) to head (6a9b377).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
...en-crypto/src/safe/identity_sealed_key_envelope.rs	68.65%	152 Missing ⚠️
...bitwarden-crypto/src/keys/asymmetric_crypto_key.rs	0.00%	28 Missing ⚠️
crates/bitwarden-crypto/src/signing/namespace.rs	33.33%	2 Missing ⚠️
...tes/bitwarden-crypto/src/traits/key_fingerprint.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
- Coverage   79.66%   79.23%   -0.44%     
==========================================
  Files         302      304       +2     
  Lines       32248    32873     +625     
==========================================
+ Hits        25691    26047     +356     
- Misses       6557     6826     +269     

☔ 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.

[claude]

Claude finished @quexten's task —— View job

---

Code Review Complete

PR Title and Description

💭 Title and description improvements needed

The PR title and description require more detail:

Suggested improvements

Title: Consider being more specific about what this POC demonstrates. Current title is acceptable but could be clearer.

Objective section: The objective is empty. Even for a POC, please add:

• What problem this solves
• Why a new primitive is needed vs. existing solutions
• Key security properties being demonstrated
• Whether this is exploring an approach or validating a specific design

Test Plan: Missing. Even for POCs, document:

• How the protocol was validated (the example runs successfully, test coverage, etc.)
• Any manual security analysis performed
• Known limitations or areas requiring further review

---

Summary of Changes

This PR introduces a new SAFE cryptographic primitive IdentitySealedKeyEnvelope for secure key transport between cryptographic identities with sender authentication. The implementation uses signcryption (COSE Sign1 containing COSE Encrypt) and is demonstrated through a v2 organization membership protocol example.

Key additions:

• New IdentitySealedKeyEnvelope primitive (706 lines)
• KeyFingerprint trait and implementation for identity binding
• Organization v2 protocol example (236 lines)
• Two new signing namespaces
• Fingerprint derivation for RSA and Ed25519 keys

---

Critical Issues

❌ Finding 1: Insufficient error handling with unwrap() in production code (identity_sealed_key_envelope.rs:260, 282, 296, 302, 331, 340, 366)

Multiple instances of unwrap() and expect() in the unsealing path create panic risks:

// Line 260, 282 - Could panic on malformed fingerprint bytes
.map(|bytes| KeyFingerprint(bytes.try_into().unwrap()))

// Line 296 - Expectation not guaranteed by protocol
.expect("CEK algorithm must be present in COSE Encrypt protected header");

// Line 302 - Assumption not cryptographically enforced
.expect("COSE Encrypt must have at least one recipient");

// Line 331 - Could panic on wrong nonce size
.expect("Nonce must be exactly NONCE_SIZE bytes");

// Line 340 - Should propagate error
let content_format = ContentFormat::try_from(&cose_encrypt.protected.header).unwrap();

These should return proper errors instead of panicking. An attacker-controlled envelope could trigger panics.

Recommendation: Convert all unwrap()/expect() to proper error handling with ? or map_err().

---

❌ Finding 2: Panic on unsupported algorithm instead of error (identity_sealed_key_envelope.rs:319)

_ => panic!("Unsupported recipient key encryption algorithm"),

This creates a DoS vector where a malformed message causes panic instead of returning an error.

Recommendation: Return IdentitySealedKeyEnvelopeError::RsaOperationFailed or a new error variant.

---

⚠️ Finding 3: Missing validation of fingerprint byte length (identity_sealed_key_envelope.rs:260, 282)

The fingerprint extraction uses unwrap() when converting variable-length bytes to fixed 32-byte arrays:

.map(|bytes| KeyFingerprint(bytes.try_into().unwrap()))

If an attacker provides a fingerprint of wrong length in the envelope, this panics instead of returning RecipientFingerprintMismatch or similar error.

Recommendation: Handle the conversion error explicitly:

.and_then(|bytes| {
    bytes.try_into()
        .map(KeyFingerprint)
        .map_err(|_| IdentitySealedKeyEnvelopeError::RecipientFingerprintMismatch)
})

---

Security Concerns

⚠️ Finding 4: RSA-OAEP with SHA-1 usage requires documentation (identity_sealed_key_envelope.rs:119-127)

Per CLAUDE.md guidelines, new crypto algorithms require detailed justification. While RSA-OAEP-SHA1 is mentioned as "currently the only supported algorithm," the security implications and migration path should be documented:

1. SHA-1 in OAEP is still considered secure for this use case (unlike SHA-1 for signatures), but this should be explicitly stated
2. Document why not using SHA-256 variant
3. Add TODO/issue link for migration to stronger hash if planned

Recommendation: Add inline documentation explaining the choice and any future migration plans.

---

⚠️ Finding 5: Potential timing side-channel in fingerprint comparison (identity_sealed_key_envelope.rs:264, 286)

Fingerprint comparisons use != which may not be constant-time:

if sender_fingerprint_in_envelope != expected_sender_fingerprint {
    return Err(IdentitySealedKeyEnvelopeError::SenderFingerprintMismatch);
}

While KeyFingerprint implements PartialEq using ConstantTimeEq (key_fingerprint.rs:35), the comparison here is still using != operator.

Recommendation: Verify the constant-time comparison is actually used, or explicitly use ct_eq() for clarity:

if !sender_fingerprint_in_envelope.0.ct_eq(&expected_sender_fingerprint.0).into() {
    return Err(...);
}

---

♻️ Finding 6: Changes to bitwarden-crypto core require justification (per CLAUDE.md)

This PR modifies core bitwarden-crypto functionality including:

• New trait DeriveFingerprint
• Fingerprint implementation for AsymmetricPublicCryptoKey and VerifyingKey
• Public API additions to AsymmetricCryptoKey

Per CLAUDE.md: "Generally speaking, this crate should not be modified. Changes need a comment explaining why."

Recommendation: Add module-level documentation or ADR explaining why these core changes are necessary and how they fit into the broader SAFE architecture.

---

Code Quality Issues

🎨 Finding 7: Missing documentation for new crypto primitive requirements (identity_sealed_key_envelope.rs:88-211)

The seal() and unseal() methods have good doc comments but should explicitly document:

1. Required preconditions (e.g., recipient_public_key MUST be signed by recipient_verifying_key)
2. Security properties guaranteed (confidentiality, authenticity, binding)
3. What happens if keys are reused/rotated
4. Replay attack considerations

Recommendation: Expand documentation with explicit security guarantees section.

---

🎨 Finding 8: Inconsistent error handling patterns (identity_sealed_key_envelope.rs)

Some errors use detailed context:

.map_err(|_| IdentitySealedKeyEnvelopeError::RecipientPublicKeyVerificationFailed)

Others discard potentially useful context:

.map_err(|_| IdentitySealedKeyEnvelopeError::DecryptionFailed)

Recommendation: Consider preserving error context for debugging, especially for crypto errors that could indicate protocol violations vs. implementation bugs.

---

🎨 Finding 9: Unreachable code should use compile-time checks (identity_sealed_key_envelope.rs:150, 179)

_ => unreachable!("Only BitwardenLegacyKey and CoseKey are supported content formats")
_ => unreachable!("CEK algorithm is always XChaCha20Poly1305")

Using unreachable!() for exhaustiveness checking is fine, but consider using typed enums or const generics to make these compile-time guarantees where possible.

---

💭 Finding 10: Key commitment properties unclear (identity_sealed_key_envelope.rs:12-16)

The module uses XChaCha20Poly1305 which lacks key commitment (as noted in xchacha20.rs:10-15). For identity-sealed envelopes where multiple parties are involved, this could theoretically enable invisible salamander attacks.

Question: Has this been analyzed for this use case? The binding to sender/recipient identities may provide sufficient protection, but this should be documented.

---

Test Coverage

⚠️ Finding 11: Insufficient test coverage (66.60% patch coverage, 183 lines uncovered)

Critical paths missing coverage based on Codecov report:

• Error handling paths (especially the unwrap/expect cases)
• Invalid fingerprint scenarios
• Malformed COSE structure handling
• Content format edge cases

Recommendation: Add negative test cases for:

• Malformed envelopes (wrong fingerprint length, missing fields, etc.)
• Replay attacks (if applicable)
• Cross-protocol attacks (wrong namespace)
• Key rotation scenarios

---

💭 Finding 12: Test vector generation is ignored (identity_sealed_key_envelope.rs:618-705)

The #[ignore] test for generating test vectors is useful but won't run in CI. Consider:

• Running it once and committing the test vectors as constants
• Adding a verification test that uses committed test vectors
• This enables cross-implementation validation and prevents regressions

---

Example Code

🎨 Finding 13: Example has placeholder security functions (organization_v2_protocol.rs:76-88)

fn prompt_user_to_verify_fingerprint(_org: &OrganizationIdentity) -> bool {
    return true;  // Always returns true!
}

While this is POC code, it's dangerous to have security-critical placeholders that always succeed.

Recommendation: At minimum, add prominent comments and todo!() or unimplemented!() to prevent accidental use:

fn prompt_user_to_verify_fingerprint(_org: &OrganizationIdentity) -> bool {
    // SECURITY: This MUST perform out-of-band fingerprint verification in production!
    // The user must manually verify the organization's fingerprint matches what they
    // expect through a separate secure channel.
    todo!("Implement actual fingerprint verification UI")
}

---

🎨 Finding 14: Example could demonstrate replay protection (organization_v2_protocol.rs)

The example doesn't show how to prevent replay attacks or handle key rotation. Consider adding comments about:

• How membership agreements are tracked/stored
• Timestamp or nonce usage for freshness
• How revocation would work

---

Infrastructure Changes

📝 Finding 15: New COSE constants lack documentation (cose.rs:32-33)

pub(crate) const IDENTITY_SEALED_ENVELOPE_RECIPIENT_FINGERPRINT: i64 = -71005;
pub(crate) const IDENTITY_SEALED_ENVELOPE_SENDER_FINGERPRINT: i64 = -71006;

These private-use COSE labels should document:

• Why these specific values were chosen
• Range allocation strategy (to prevent conflicts)
• Whether they'll be registered with IANA later

---

Good Practices Observed

👍 Well-structured signcryption using standard COSE primitives
👍 Proper use of domain separation with signing namespaces
👍 Constant-time comparison in KeyFingerprint PartialEq implementation
👍 ZeroizeOnDrop for sensitive key material
👍 Comprehensive module documentation explaining the protocol

---

Action Items for Author

1. Critical: Replace all production unwrap()/expect() with proper error handling
2. Critical: Fix panic on unsupported algorithm (Finding 2)
3. Important: Add justification for bitwarden-crypto core changes (Finding 6)
4. Important: Increase test coverage, especially error paths (Finding 11)
5. Important: Document RSA-OAEP-SHA1 usage and migration path (Finding 4)
6. Nice to have: Add security properties documentation to API docs (Finding 7)
7. Nice to have: Improve example security placeholders (Finding 13)
8. Consider: Document key commitment analysis (Finding 10)

---

Note: This is a POC and marked "Do not merge," but the findings above should be addressed before any production use. The cryptographic approach appears sound, but the implementation needs hardening.