Auth/PM-40520 - Registration - Open Org Invite Seal / Unseal Support - #1295
Conversation
The type had no cross-boundary API — its bytes were pub(crate) and the WASM ABI encoded them inline. Our first consumer of SecretProtectedKeyEnvelope outside bitwarden-crypto needs to embed the secret in a serde-serialized wire artifact (a struct crossing the WASM boundary alongside a paired sealed data blob), so surface the same base64 conversion the WASM ABI already uses as Rust API.
Two new production namespace variants, plus round-trip discriminant tests and a cross-namespace unseal test that mirrors test_namespace_cross_contamination_protection against the new DataEnvelopeNamespace::RegistrationOpenOrgInviteData variant. Discriminant values are load-bearing wire markers — once shipped they cannot be renumbered without invalidating every envelope in flight, hence the explicit discriminant assertions.
…ssing
Two new methods on the existing bitwarden-auth RegistrationClient carry an
open-organization-invite context across the anonymous verification-email tab
boundary that opens between registration-start and registration-finish.
Construction:
- Inner DataEnvelope seals a versioned payload ({organization_id,
invite_link_code, invite_key}) under a freshly generated AES-256-GCM CEK.
- Outer SecretProtectedKeyEnvelope seals that CEK under a per-registration
HighEntropySecret via HKDF-SHA-256.
- Wire format: CBOR{data_envelope, key_envelope} base64url-encoded, safe
for the verification-email URL fragment.
The seal method returns (sealed_data, high_entropy_secret) as strings — the
sealed data is server-visible and rides the email, the secret stays
client-side. Both halves are needed to unseal; substitution defense at both
layers is the AES-GCM auth-tag / wrong-key check.
The CEK never persists between calls: seal and unseal each create a
transient KeyStore<KeySlotIds> scoped to the operation.
Also mirrored the invite-link WASM integration test pattern to prove the
FFI boundary carries snake_case fields intact and per-registration
randomness across two independent seals.
There was a problem hiding this comment.
Pull request overview
Adds support for sealing/unsealing an “open organization invite” context across the registration → email-verification boundary using a two-envelope crypto construction, with Rust + WASM integration coverage.
Changes:
- Introduces
RegistrationClient::seal_open_org_invite_data/unseal_open_org_invite_dataand related wire framing (CBOR wrapped + base64url). - Adds new
DataEnvelopeNamespaceandSecretProtectedKeyEnvelopeNamespacediscriminants with regression tests to lock their wire values. - Extends
HighEntropySecretwith base64 encode/decode helpers and adds WASM integration tests for round-tripping.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/bitwarden-wasm-internal/integration-tests/tests/invite/open-org-invite-registration.test.ts | New WASM integration tests covering seal/unseal and base64url round-trip behavior. |
| crates/bitwarden-crypto/src/safe/secret_protected_key_envelope.rs | Adds a new envelope namespace discriminant plus mapping tests. |
| crates/bitwarden-crypto/src/safe/high_entropy_secret.rs | Adds base64 encode/decode helpers and tests; extends error enum. |
| crates/bitwarden-crypto/src/safe/data_envelope.rs | Adds a new data envelope namespace discriminant plus mapping/cross-namespace tests. |
| crates/bitwarden-auth/src/registration/unseal_open_org_invite_data.rs | New unseal implementation for the registration invite context. |
| crates/bitwarden-auth/src/registration/seal_open_org_invite_data.rs | New seal implementation + types for the registration invite context. |
| crates/bitwarden-auth/src/registration/open_org_invite_data.rs | New versioned, sealable payload for the invite context. |
| crates/bitwarden-auth/src/registration/mod.rs | Wires new registration modules and re-exports new public types. |
| crates/bitwarden-auth/Cargo.toml | Adds ciborium dependency and normalizes feature formatting. |
| Cargo.lock | Locks in the new ciborium dependency for bitwarden-auth. |
🔍 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. |
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-review at Code Review DetailsNo findings. All 11 unresolved threads are marked outdated and were addressed by later commits (camelCase serde,
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1295 +/- ##
==========================================
+ Coverage 85.81% 85.86% +0.04%
==========================================
Files 490 493 +3
Lines 70590 70929 +339
==========================================
+ Hits 60574 60900 +326
- Misses 10016 10029 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ration/ The sibling tests/invite/ subtree is admin-console-owned via .github/CODEOWNERS, so placing this file there auto-added admin-console as reviewers for an auth feature. Move to a new tests/registration/ subdirectory and add a matching CODEOWNERS entry routing it to team-auth-dev, mirroring how tests/unlock/** is KM-owned.
Companion to the preceding move of open-org-invite-registration.test.ts into tests/registration/. Adds an entry alongside the existing tests/invite/** (admin-console) and tests/unlock/** (KM) lines so review of auth-owned WASM integration tests routes to team-auth-dev.
Copilot review flagged that OpenOrgInviteData, SealedOpenOrgInvite, and the inner RegistrationOpenOrgInviteDataV1 payload were emitting snake_case fields across the WASM boundary, inconsistent with LoginRequest and every other recent bitwarden-auth model (e.g. login_via_password, send_access). The JitMasterPasswordRegistrationRequest siblings I originally mirrored are the outlier, not the pattern. Since none of these types have shipped, aligning them now avoids setting a new snake_case precedent on the WASM boundary and matches what the client- side TS caller (PM-39706) will actually want to consume. The inner V1 payload is the sealed CBOR wire format — no compatibility risk because no sealed blobs exist in the wild yet. Also simplifies the base64url round-trip check in the integration test to use Node's native "base64url" Buffer encoding (available since Node 16, which CI uses) instead of the manual `+`/`-`, `/`/`_`, and padding substitutions, and drops `await` on the now-synchronous WASM methods.
Copilot review noted the new base64 constructor bypassed the type's own 16-byte floor: any successfully-decoded input, however short, could produce a `HighEntropySecret` that lied about the length invariant the KDF assumes. Add the same MIN_SECRET_LENGTH check `make(...)` enforces so all constructors honor the same floor. The check is a floor, not an entropy check — a 32-byte ASCII string is still accepted (as documented) — but it closes the trivially- short-input gap. Also adds two guard tests (below-minimum lengths rejected as TooShort, and input at exactly MIN_SECRET_LENGTH accepted).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/bitwarden-crypto/src/safe/high_entropy_secret.rs:12
rand::Rngis imported but never used in this module, which will trigger an unused import warning (and may fail CI if warnings are denied).
use bitwarden_encoding::B64;
use bitwarden_sensitive_value::{ExposeSensitive, Sensitive, SensitiveSlice};
use rand::Rng;
use thiserror::Error;
Reviewer caught that the doc comment claimed `bstr` semantics but the code was relying on serde's default `Vec<u8>` handling, which ciborium serializes as a CBOR array of integers (one item per byte) rather than a byte string. Round- tripping still worked because both sides used the same shape, but the encoded sealed_data was roughly 2x larger than the comment implied — and the wire format is load-bearing per the namespace-discriminant tests once it ships. Fix: `#[serde(with = "serde_bytes")]` on both fields so serde honors the bstr intent; existing round-trip test proves the change stays correct. Also rewrote the doc comment to accurately describe why the attribute is needed (so future readers don't remove it thinking it's redundant).
Reviewer flagged that `open_org_invite_data.rs` was implementing `SealableVersionedData` manually, which the trait docs explicitly say not to do. The reason was structural: the macro emits `enum $enum_name` with no visibility keyword, so the generated enum is module-private and can't be shared across the sibling `seal_open_org_invite_data` / `unseal_open_org_invite_data` modules that both need it. Adding a single `$vis:vis` matcher in front of the enum name in the macro lets callers optionally opt into a specific visibility (`pub`, `pub(crate)`, `pub(super)`, etc.) while leaving every existing caller untouched — the empty match is still valid. Doc comment updated to describe the new prefix. With that in place, `open_org_invite_data.rs` now uses the macro properly, which drops ~15 lines of hand-written boilerplate that duplicated the macro's output. Renamed the variant match in the unseal path to match the macro's naming convention (variant name = struct name), matching how future new versions would be added.
Reorganize the open-organization-invite registration crossing from three flat sibling files (open_org_invite_data.rs, seal_open_org_invite_data.rs, unseal_open_org_invite_data.rs) into a single open_org_invite/ module: - mod.rs — versioned enum via generate_versioned_sealable!, re-exports - wire_v1.rs — RegistrationOpenOrgInviteDataV1 wire schema - seal.rs — public request/response types + seal path + shared helpers - unseal.rs — unseal path + framing split helper With seal/unseal as children of open_org_invite instead of top-level siblings, the versioned enum can stay module-private (visible to descendants) and the macro no longer needs a $vis:vis prefix. Reverts the macro visibility change from bd2af69 in bitwarden-crypto; that capability wasn't mine to add and the folder shape removes the need for it. Follow-up to the layout alternative Copilot suggested on #1295 (relocate vs. teach the macro). Renamed the seal-side public input `OpenOrgInviteData` to `OpenOrgInviteSealRequest` per the SDK's [Noun][Op]Request convention in CLAUDE.md. `SealedOpenOrgInvite` keeps its shape-neutral name and now carries a doc explaining it is both the seal response and the unseal request — either suffix would misrepresent one of the two roles.
Per PR feedback that some comments were noisy enough to distract rather
than inform. Drops redundant test-body preambles (test names already
describe intent), shortens defensive "a future refactor would break
this" commentary, condenses parallel KeyStore-scoping comments in seal
and unseal, and removes an external doc-reference ("AC 1.iii analogue")
from a test.
No behavior change; wire format unchanged.
…ation-open-org-invite-seal-unseal + merge conflict fixes
quexten
left a comment
quexten
left a comment
There was a problem hiding this comment.
Approving KM owned code. Please conisder the advice on structure. While it is optional, I do feel that the current implementation is an anti-pattern, as we are splitting the implementation of a struct's logic, and moving it into clients across multiple files, when not necessary.
This would be greatly simplified by moving the logic onto impl's on the struct, then making the client calls much lighter (and merging the files for the client functions, since now they are tiny).
Per PR review, RegistrationClient methods were orchestrating the crypto and constructing the sealed struct field-by-field, breaking encapsulation of the domain type. Crypto now lives on SealedOpenOrgInviteData::seal / ::unseal; the client methods are thin FFI wrappers. Also splits the module by concern: - open_org_invite.rs — domain types + crypto impls + round-trip tests - serialization.rs — wire encoding + WASM ABI + wire/parse tests - client.rs — FFI pair struct + thin RegistrationClient wrappers SealedOpenOrgInviteData's envelope fields tightened from pub to pub(super) since the client no longer needs field access. Folder renamed to open_org_invite_crypto to satisfy clippy's module_inception check. Wire format unchanged (pinned integration-test vector still passes). Public API surface unchanged.
…ation-open-org-invite-seal-unseal + codeowners merge conflict fix
…hrough Prettier Merge commit 8518461 accidentally reformatted CODEOWNERS as Markdown, which turned `*` into `-` (dropping the repo-wide default owner), backslash-escaped six glob patterns (breaking gitignore-style matching for API key connector, BW CLI vault, publish/build workflows, dockerignore, and uniffi.toml), and inserted blank lines after every comment header. Restore the file to origin/main and re-apply only the one intended line for the new tests/registration/** ownership.
| #[serde(rename = "d", with = "serde_bytes")] | ||
| data_envelope: Vec<u8>, | ||
| #[serde(rename = "k", with = "serde_bytes")] | ||
| key_envelope: Vec<u8>, |
There was a problem hiding this comment.
Nit: Naming this "sealed_data_envelope_cek" would be clearer. It is the CEK of the data envelope.
There was a problem hiding this comment.
Will address in fast follow.
There was a problem hiding this comment.
Actually, the related type is called SecretProtectedKeyEnvelope? I'm mirroring KM naming here. I've added an additional comment in the follow up work to clarify, but respectfully, I'm not seeing a benefit of the other name. Sealed is already in the struct name SealedOpenOrgInviteDataWire. It's an envelope that holds a CEK which can just be abbreviated as key therefore key_envelope is accurate?
See f3eceaf for new comments to try to make things more clear.
quexten
left a comment
quexten
left a comment
There was a problem hiding this comment.
Looks good. I have one nit on the naming in the serialization format. I would at least add a comment that the key envelope contains the CEK of the data envelope., along with a claude generated diagram, similar to what we have at the top of the invite crypto file, that describes which key protects what.
…egistration - Open Org Invite Seal / Unseal Support (bitwarden/sdk-internal#1295)
…- Fast Follow (#1309) ## 🎟️ Tracking <!-- Paste the link to the Jira or GitHub issue or otherwise describe / point to where this change is coming from. --> https://bitwarden.atlassian.net/browse/PM-40520 Follow up from #1295 ## 📔 Objective <!-- Describe what the purpose of this PR is, for example what bug you're fixing or new feature you're adding. --> To clean up some small remaining items on the new, simple API on the Auth registration client to seal and unseal the new open organization invite link data as requested by @quexten #1295 (review) --------- Co-authored-by: Dave <3836813+enmande@users.noreply.github.com>
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-40520
Tech breakdown: https://github.com/bitwarden/tech-breakdowns/tree/main/auth/PM-39706-open-invite-registration-crossing
📔 Objective
To provide a simple API on the Auth registration client to seal and unseal the new open organization invite link data so it can be safely carried through the registration with email verification process.