[PM-35796] Invite link Crypto Bundle Generation and Unsealing#1157
[PM-35796] Invite link Crypto Bundle Generation and Unsealing#1157BTreston wants to merge 4 commits into
Conversation
🔍 SDK Breaking Change DetectionSDK Version:
Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1157 +/- ##
==========================================
+ Coverage 83.89% 84.15% +0.26%
==========================================
Files 441 447 +6
Lines 57844 58890 +1046
==========================================
+ Hits 48528 49560 +1032
- Misses 9316 9330 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE with minor follow-ups This PR introduces the Code Review Details
|
|
@claude review |
|
Claude finished @BTreston's task in 3m 11s —— View job Claude Code ReviewScope reviewed: New Overall this is a clean, well-tested PR. Tests cover the round-trip, non-determinism, EncString-format validation, invalid org key, and wrong-key unsealing — nice coverage. A few items below worth considering. 🟠 Sensitive material exposed via
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| .expect("`to_buffer` never fails for `EncString`"), | ||
| ) | ||
| .to_string() | ||
| key_data.0.to_string() |
There was a problem hiding this comment.
♻️ DEBT: Sibling docs/examples still describe the old base64 format for InviteKeyEnvelope.
Details and fix
This PR changed From<&InviteKeyEnvelope> for String and FromStr from the B64-of-buffer encoding to the EncString text format ("2.iv|data|mac"), but two adjacent locations in the same crate still describe the prior behavior:
-
crates/bitwarden-organization-crypto/src/wasm.rs:1-6— module-level doc says: "base64url for theInviteKeyDatatype, and base64 for the InviteKey type". The "base64 for the InviteKey" half no longer applies — theIntoWasmAbi/FromWasmAbiimpls now serialize via the new EncString-text format. -
crates/bitwarden-organization-crypto/examples/make_invite_key_bundle.rs:32-36— comment saysInviteKeyEnvelope"automatically serializes tobase64when using serde,String::from(&inviteKeyEnvelope), or wasm abi serialization." This is now misleading.
Updating these to reference the EncString text format will keep the crate's documentation consistent with its new serialization contract (and with the new doc comment you added on From<&InviteKeyEnvelope> for String).
coroiu
left a comment
coroiu
left a comment
There was a problem hiding this comment.
bitwarden-organization-invite-link seems very specific 🤷
| #[serde(rename_all = "camelCase")] | ||
| pub struct OrganizationInviteCryptoBundle { | ||
| /// Raw invite key as base64Url. CRITICAL: MUST NOT be sent to the server. | ||
| pub invite_key: String, |
There was a problem hiding this comment.
| pub invite_key: String, | |
| pub invite_key: SymmetricKey, |
| /// Raw invite key as base64Url. CRITICAL: MUST NOT be sent to the server. | ||
| pub invite_key: String, | ||
| /// Invite key sealed with the organization key, as a Bitwarden EncString (`"2.iv|data|mac"`). | ||
| pub sealed_invite_key_envelope: String, |
There was a problem hiding this comment.
| pub sealed_invite_key_envelope: String, | |
| pub sealed_invite_key_envelope: EncString, |
Ideally the newtype should actually be wasm exposed. I'm not sure why we didn't in the first place. But we should always use as specific of a type as possible, even if they are newtypes around string.
On the WASM side you can still directly pass the generated type into functions expecting string, though using a newtype there is also preferred.
| use tsify::Tsify; | ||
|
|
||
| /// Errors from generating an organization invite crypto bundle. | ||
| #[allow(missing_docs)] |
There was a problem hiding this comment.
| #[allow(missing_docs)] |
Please add docs. The missing docs allow is only meant for cases where we did not yet provide docs.
| impl std::fmt::Debug for OrganizationInviteCryptoBundle { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| f.debug_struct("OrganizationInviteCryptoBundle") | ||
| .field("invite_key", &"<REDACTED>") |
There was a problem hiding this comment.
If invite key were using the proper symmetrickey type, this would not be needed because symmetrickey already implemnets debug and prevents you from logging it. That's why using string here is dangerous / relies on you not logging it.
|
|
||
| /// Generates a new [`OrganizationInviteCryptoBundle`] sealed with the provided organization key. | ||
| /// | ||
| /// Each call produces a unique, non-deterministic invite key. |
There was a problem hiding this comment.
| /// Each call produces a unique, non-deterministic invite key. | |
| /// Each call produces a unique invite key sampled from a secure cryptographic source. |
| /// The `invite_key` field MUST NOT be sent to the server. | ||
| #[cfg_attr(feature = "wasm", wasm_bindgen::prelude::wasm_bindgen)] | ||
| pub fn generate_organization_invite_crypto_bundle( | ||
| org_key: Vec<u8>, |
There was a problem hiding this comment.
| org_key: Vec<u8>, | |
| org_key: &SymmetricKey, |
Please do not use Vec<u8> or String to represent keys. We want to use the SymmetricKey type for this.
| org_key: Vec<u8>, | ||
| sealed_invite_key_envelope: String, |
There was a problem hiding this comment.
| org_key: Vec<u8>, | |
| sealed_invite_key_envelope: String, | |
| org_key: SymmetricKey, | |
| sealed_invite_key_envelope: &EncString, // OR &InviteKeyEnvelope?? |
|
@BTreston Looks mostly good, but we have a few times where we are using generic types (String, Vec) when we can use the newtypes (SymmetricKey, EncString). These are preferred because the provide better type safety, but also prevent dangerous logging of cryptographic material. |
| #[error("Invalid sealed invite key envelope: {0}")] | ||
| /// Invalid Envelope | ||
| InvalidSealedEnvelope(InviteKeyBundleError), |
There was a problem hiding this comment.
♻️ DEBT: InvalidSealedEnvelope variant is never constructed and is unreachable dead code.
Details and fix
This variant is declared on OrganizationInviteCryptoBundleError but no code path constructs it. The two real failure paths use BundleGenerationFailed (via #[from]) and UnsealingFailed (mapped explicitly in unseal_organization_invite_key). Envelope parsing happens in the wasm from_abi, which calls unwrap_throw() rather than surfacing a typed error.
| #[error("Invalid sealed invite key envelope: {0}")] | |
| /// Invalid Envelope | |
| InvalidSealedEnvelope(InviteKeyBundleError), | |
| #[error("Failed to unseal invite key: {0}")] | |
| /// Unsealing Envelope failed | |
| UnsealingFailed(InviteKeyBundleError), | |
| } |
Since this is exposed in the #[bitwarden_error(flat)] enum, an unused variant also leaks into the generated TypeScript surface as a value that JS callers can never actually receive. Either drop the variant or wire it into a real parse path.
| pub fn unseal_organization_invite_key( | ||
| org_key: SymmetricCryptoKey, | ||
| sealed_invite_key_envelope: InviteKeyEnvelope, | ||
| ) -> Result<String, OrganizationInviteCryptoBundleError> { |
There was a problem hiding this comment.
🎨 SUGGESTED: Return InviteKeyData instead of bare String to preserve the redacted Debug impl.
Details and fix
The function currently unseals the envelope and immediately calls String::from(&invite_key_data), returning the raw invite key as an unprotected String. This is the same class of concern that motivated using SymmetricCryptoKey / InviteKeyEnvelope newtypes for the inputs in the latest commit: InviteKeyData has a redacted Debug impl (delegates to SymmetricCryptoKey::fmt), but a String does not — any future Rust caller (mobile/desktop) that logs the result would leak the raw key material.
The wasm ABI for InviteKeyData already serializes to a base64Url string, so the TypeScript-side surface stays identical:
pub fn unseal_organization_invite_key(
org_key: SymmetricCryptoKey,
sealed_invite_key_envelope: InviteKeyEnvelope,
) -> Result<InviteKeyData, OrganizationInviteCryptoBundleError> {
// ...
sealed_invite_key_envelope
.unseal(org_key_slot, &mut context)
.map_err(OrganizationInviteCryptoBundleError::UnsealingFailed)
}The TS signature in TS_FUNCTIONS would then change its return type from string to InviteKeyData (still a tagged string), matching how the bundle's invite_key field is already typed.
|
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-35796
📔 Objective
Create the admin console owned crate in the SDK + WASM bindings that call the new KM interface
InviteKeyBundle. Also editsInviteKeyBundleto return anEncStringinstead ofB64.Wired up in the clients in bitwarden/clients#21004
🚨 Breaking Changes