[PM-41576] PAM partial filtered data - #1359
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 #1359 +/- ##
==========================================
+ Coverage 86.17% 86.29% +0.12%
==========================================
Files 534 534
Lines 79747 80624 +877
==========================================
+ Hits 68719 69572 +853
- Misses 11028 11052 +24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…pivot) Pivots the PAM cipher-gating client work: instead of hiding gated rows from the SDK and hand-decrypting them, gated rows now flow through the SDK, which decrypts the reduced envelope into a `partial` view (bitwarden/sdk-internal#1359). - Revert the SdkRecordMapper `shouldInclude` filter and the `decryptPartialCiphers` path (the last caller of the deprecated `Cipher.decrypt`). Gated rows go through `decryptMany*` like any cipher. - CipherResponse passes `partialData` through verbatim (no client lift); `Cipher.toSdkCipher`/`fromSdkCipher` map it both ways so it round-trips losslessly (re-attach hack removed). The decrypted view's gating marker is the SDK's `partial` boolean. - Vault seams read `view.partial`; `CipherOpenVerdict` gains a `handled` case so the open gate can block the open and surface the "Privileged Controls license required" dialog (unlicensed-user design). - Move the "Privileged" badge into a dedicated "Controlled access" column, shown only when a PAM-enabled org (`Organization.usePam`) is in view and the badge seam is provided. Depends on sdk-internal#1359 (adds Cipher.partial_data + the `partial` view flag + the restricted decrypt path); the `@bitwarden/sdk-internal` bump lands once that publishes. Supersedes #22168, #22169, #22170.
…pivot) Pivots the PAM cipher-gating client work: instead of hiding gated rows from the SDK and hand-decrypting them, gated rows now flow through the SDK, which decrypts the reduced envelope into a `partial` view (bitwarden/sdk-internal#1359). - Revert the SdkRecordMapper `shouldInclude` filter and the `decryptPartialCiphers` path (the last caller of the deprecated `Cipher.decrypt`). Gated rows go through `decryptMany*` like any cipher. - CipherResponse passes `partialData` through verbatim (no client lift); `Cipher.toSdkCipher`/`fromSdkCipher` map it both ways so it round-trips losslessly (re-attach hack removed). The decrypted view's gating marker is the SDK's `partial` boolean. - Vault seams read `view.partial`; `CipherOpenVerdict` gains a `handled` case so the open gate can block the open and surface the "Privileged Controls license required" dialog (unlicensed-user design). - Move the "Privileged" badge into a dedicated "Controlled access" column, shown only when a PAM-enabled org (`Organization.usePam`) is in view and the badge seam is provided. Depends on sdk-internal#1359 (adds Cipher.partial_data + the `partial` view flag + the restricted decrypt path); the `@bitwarden/sdk-internal` bump lands once that publishes. Supersedes #22168, #22169, #22170.
Hinton
changed the title
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the PAM-gated partial cipher support: the new Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
harr1424
left a comment
harr1424
left a comment
There was a problem hiding this comment.
Tools-owned changes (bitwarden-exporters) look good
quexten
left a comment
quexten
left a comment
There was a problem hiding this comment.
Currently, my main two concerns are:
- The API here is brittle and will easily lead to bugs where a reduced cipher is updated in place of a full cipher
- If we cannot force callers to behave right (we can, but it's more effort), then at least we should make the API intuitive, and there should be a function or field "allow_re_encryption" that maps to the condition "partial data present"
- We should add test vectors to ensure permanent decryptability
| // Fail closed: a restricted (partial) view has all secret fields stripped; re-encrypting it | ||
| // would overwrite the item's secrets with empty values. See `decrypt_restricted_cipher_view`. | ||
| if view.partial { | ||
| return Err(BlobEncryptionError::Crypto( |
There was a problem hiding this comment.
@Hinton where does the partial view come from? Assuming that - as asked above - only org ciphers have partial views, then blob ciphers won't immediately be a problem, but become a problem as soon as we undertake organizations moving to blob ciphers.
Is the plan to:
- always keep the partial format with the name / uri separated out for partial ciphers?
- move to a (small) data blob
There was a problem hiding this comment.
I think this path is actually always correct. You wouldn't want to re-encrypt the partial data. Updates should be done on the full cipher.
Blobs will impact conversion to PAM and PAM updates, though, since the server can no longer separate our the partial data from the full data.
There was a problem hiding this comment.
Partials comes from the sync.
- Partials define their own DTO which has a separate type from the cipher blobs.
- Yes. We initially wanted blobs but it was deemed not viable at this stage.
| ctx: &mut KeyStoreContext<KeySlotIds>, | ||
| key: SymmetricKeySlotId, | ||
| ) -> Result<CipherView, CryptoError> { | ||
| if let Some(raw) = &self.0.partial_data { |
There was a problem hiding this comment.
I don't like that we make it easy to represent illegal states here. With f.e errors in rust the caller is forced to handle it correctly. They don't here.
| deleted_date: cipher.deleted_date, | ||
| revision_date: cipher.revision_date, | ||
| archived_date: cipher.archived_date, | ||
| partial: true, |
There was a problem hiding this comment.
Can we please add a new field on the cipher view "allow re-decryption". If a cipher is a partial cipher, this should be set to false. This makes it way clearer that this cipher should never be used for key rotation.
shane-melton
left a comment
shane-melton
left a comment
There was a problem hiding this comment.
I want to second Bernd's findings above and note a few of my own
| /// as the `CipherView` produced will not have all fields populated (e.g. `collection_ids`). | ||
| pub(crate) fn convert_request_to_cipher_view(r: CipherEditRequest) -> CipherView { | ||
| CipherView { | ||
| partial: false, |
There was a problem hiding this comment.
💭 : This seems dangerous. The CipherEditRequest doesn't carry a partial flag, so if a caller mistakenly attempts to edit a partial cipher, the edit.rs flow will happily strip the secrets (they aren't present to copy) and the new CipherView will have partial: false. Meaning the guard at the encryption level that throws CryptoError::EncryptRestrictedView never triggers.
There was a problem hiding this comment.
As for the encryption error, comment, editing a cipher should be blocked behind a complete data pull requirement. It WOULD be nice for the type system to enforce this, though.
There was a problem hiding this comment.
I don't quite agree, adding partial to CipherEditRequest makes it a valid type state to have a partial view which will always throw. You always have to make an explicit choice to convert something into the cipher edit request.
MGibson1
left a comment
MGibson1
left a comment
There was a problem hiding this comment.
Overall, the approach to implement by tweaking the Cipherview is having a lot of knock on effects where we need to assert whether or not something is partial. It feels like the better approach is to use a partial view for all lists, which would then convert all of the discussion around updates to blocked by the type system.
| }; | ||
|
|
||
| Self { | ||
| partial: false, |
There was a problem hiding this comment.
Is this a guarantee? Are we never allowing import/export of partials?
There was a problem hiding this comment.
Yes, partials is not import/exportable.
| // Fail closed: a restricted (partial) view has all secret fields stripped; re-encrypting it | ||
| // would overwrite the item's secrets with empty values. See `decrypt_restricted_cipher_view`. | ||
| if view.partial { | ||
| return Err(BlobEncryptionError::Crypto( |
There was a problem hiding this comment.
I think this path is actually always correct. You wouldn't want to re-encrypt the partial data. Updates should be done on the full cipher.
Blobs will impact conversion to PAM and PAM updates, though, since the server can no longer separate our the partial data from the full data.
| /// as the `CipherView` produced will not have all fields populated (e.g. `collection_ids`). | ||
| pub(crate) fn convert_request_to_cipher_view(r: CipherEditRequest) -> CipherView { | ||
| CipherView { | ||
| partial: false, |
There was a problem hiding this comment.
As for the encryption error, comment, editing a cipher should be blocked behind a complete data pull requirement. It WOULD be nice for the type system to enforce this, though.
| /// [`bitwarden_crypto::CryptoError::EncryptRestrictedView`] rather than silently stripping | ||
| /// secrets. | ||
| #[serde(default)] | ||
| pub partial: bool, |
There was a problem hiding this comment.
My expertise in the SDK is out of date here, but we have this CipherView and the CipherListView below. Is that not the definition of the difference between pam complete data and pam partial data? What is the need for inclusion on both of these data types?
Partials on the full CipherView object is the reason for most of the complexity and risk in this PR, can we limit it to the list view?
That won't work. First the web clients to not currently use
There is currently no timeline for org blobs, our original proposal leaned into the blob model but we had to pivot since it would not be available. Once organizations starts using blobs we will use a partial blob, it should be done in the same migration.
Agreed, not sure how to do that across the stack though.
No, per the first section of this comment. |
A PAM-gated cipher is only ever held partial in local state — the server withholds its secrets from every bulk read, and now from write-returns too. `edit_cipher` builds its original from that copy, so editing a gated cipher silently dropped the item's whole password history (a partial view has none to chain) and stamped a fresh `password_revision_date`, then PUT both. Refuse it there, and add `edit_gated`, which takes the full original the caller obtained from a lease-authorised read — mirroring the admin edit path, which takes its original as an argument for the same reason: no usable copy exists in local state. Both paths now share one body and differ only in where the original comes from. That shared body also declines to persist a write-return that would un-gate the stored copy. It should be unreachable now the server strips gated write-returns, but persisting one would put lease-scoped secrets into durable state, outliving the lease that justified them. It skips the write rather than erroring: the server already applied the change, so reporting failure would be the worse lie. Raised from #1359 review.
share_ciphers_bulk hand-built its repository entry from the mini response, hardcoding partial_data and data to None. A PAM-gated write-return — secrets withheld, partial_data set — was therefore persisted as an ungated husk: blank fields with nothing marking them as withheld, invisible to every partial_data gate downstream. Any blob data in the response was dropped the same way. The hand-rolled literal was a near-copy of CipherMiniResponseModel's merge_with_cipher, which already carries both fields and pulls the same local-only fields from the stored original. Use it, keeping the collection_ids the caller shared into. One deliberate delta: with no original in the repository, view_password now defaults to true, as it does in every other response conversion. Raised from #1359 review.
The admin edit takes its original view from the caller, and handing it a PAM-gated partial one only failed closed by accident: the merge step re-encrypts the original and trips EncryptRestrictedView before the PUT. That surfaces as an opaque crypto error, and the protection evaporates if the merge ever stops re-encrypting. Guard it explicitly at the top, mirroring the member path's PartialOriginal from 974a9ec, before password history is diffed against the stripped view. Raised from #1359 review.
The server applies a bulk share, then strips any now-gated cipher from the write-return when the calling client cannot render the partial shape — which today is every non-web client. The loop over the response never touched those entries, so the repository kept the stale pre-share copy: personal-owned, full secrets, wrong revision date, until the next full sync happened to reconcile it. The share stood server-side, so the stale copy is the worse artifact to keep. Track the requested ids against the returned ones and evict the difference; the next sync restores the cipher in its gated shape. Raised from #1359 review.
6efde93
be445f6 to
6efde93
Compare
harr1424
left a comment
harr1424
left a comment
There was a problem hiding this comment.
Tools-owned changes look good ✅
Add a `partial_data` field to `Cipher` and a `partial` flag to `CipherView`/`CipherListView`, plus a dedicated decrypt branch for restricted ciphers. When the server withholds a gated cipher's secret fields it sends a reduced `partial_data` envelope in their place: the encrypted name and, for logins, the encrypted URIs. The new branch parses that envelope (mirroring the server's `PartialCipherData.Strip` shape via `RestrictedCipherData`), decrypts only those allowlisted fields, and returns a view marked `partial = true` — it never reads the cipher's secret payloads, so an over-sharing server blob can't leak a password or TOTP onto a gated view. The allowlist lives once, in Rust. The branch runs ahead of both the lenient and strict decrypt paths and is independent of the `PM-34500-strict_cipher_decryption` flag, so a restricted Login/Card/BankAccount no longer fails with `MissingField`. Malformed envelopes fail closed: the row stays partial with an empty name rather than un-gating. The `CipherView` path still runs `remove_invalid_checksums`, mirroring the full paths' guard against a tampering server. This lets clients route gated ciphers through the SDK like any other cipher, instead of filtering them out and hand-decrypting them.
The server now emits the PAM `partial_data` envelope as a purpose-built camelCase shape carrying the same `LoginUri` fields the full decrypt path uses. Deserialize the URIs straight into `LoginUri` and drop the parallel PascalCase `RestrictedLoginUri` + its `From` impl. `RestrictedCipherData` stays the permissive top-level allowlist (no `deny_unknown_fields`) — the fail-closed boundary that drops over-shared secret fields.
…iews A CipherView produced from a server-restricted (PAM-gated) cipher has `partial = true` and all secret fields stripped to None. Nothing on the write path read `partial`, so feeding such a view back through any encrypt path (edit/share_cipher/move_to_organization via EncryptMode, or key rotation via encrypt_blob_cipher) silently overwrote the item's secrets with empty values server-side. Add a fail-closed guard at the two lowest-level encrypt functions (encrypt_legacy_field_encryption and encrypt_blob_cipher_with_wrapping_key), returning the new CryptoError::EncryptRestrictedView instead. This covers both client write APIs and rotation.
- Fail closed: partial_data is only decrypted for organization ciphers, across the lenient and strict paths for both CipherView and CipherListView (new CryptoError::RestrictedCipherRequiresOrganization). - Restricted views no longer carry the wrapped cipher key (they are never re-encrypted); the icon-checksum gate now reads the source cipher's key. - Preserve partial_data across merge_with_cipher so a partial_edit (favorite/folder toggle) no longer silently ungates a cipher. - Pin restricted partial_data envelopes as permanent const test vectors (org-keyed, decrypt-only) to guard against a format break.
decrypt_restricted_cipher_view/_list_view were unconditionally lenient, so StrictDecrypt<Cipher> silently stopped being strict for partials. A field that is present but fails to decrypt now propagates as an error under strict mode instead of degrading to empty. Absent fields (a partial legitimately lacks most) and malformed envelopes stay lenient in both modes, preserving the fail-closed-never-ungate contract.
The generated API models now carry `partialData`, so resolve the TODOs in the cipher merge paths: take `partial_data` from the server response (`self`) rather than preserving the stale local value. - `TryFrom<CipherDetailsResponseModel>` reads `partial_data` from the response (also fixes the sync conversion, which uses it directly). - The four `merge_with_cipher` impls now source `partial_data` from the response, so a full response un-gates a locally-restricted cipher and a restricted response gates it — the server is authoritative for gating. Add a test covering both directions across all four merge impls.
A PAM-gated cipher is only ever held partial in local state — the server withholds its secrets from every bulk read, and now from write-returns too. `edit_cipher` builds its original from that copy, so editing a gated cipher silently dropped the item's whole password history (a partial view has none to chain) and stamped a fresh `password_revision_date`, then PUT both. Refuse it there, and add `edit_gated`, which takes the full original the caller obtained from a lease-authorised read — mirroring the admin edit path, which takes its original as an argument for the same reason: no usable copy exists in local state. Both paths now share one body and differ only in where the original comes from. That shared body also declines to persist a write-return that would un-gate the stored copy. It should be unreachable now the server strips gated write-returns, but persisting one would put lease-scoped secrets into durable state, outliving the lease that justified them. It skips the write rather than erroring: the server already applied the change, so reporting failure would be the worse lie. Raised from #1359 review.
share_ciphers_bulk hand-built its repository entry from the mini response, hardcoding partial_data and data to None. A PAM-gated write-return — secrets withheld, partial_data set — was therefore persisted as an ungated husk: blank fields with nothing marking them as withheld, invisible to every partial_data gate downstream. Any blob data in the response was dropped the same way. The hand-rolled literal was a near-copy of CipherMiniResponseModel's merge_with_cipher, which already carries both fields and pulls the same local-only fields from the stored original. Use it, keeping the collection_ids the caller shared into. One deliberate delta: with no original in the repository, view_password now defaults to true, as it does in every other response conversion. Raised from #1359 review.
The admin edit takes its original view from the caller, and handing it a PAM-gated partial one only failed closed by accident: the merge step re-encrypts the original and trips EncryptRestrictedView before the PUT. That surfaces as an opaque crypto error, and the protection evaporates if the merge ever stops re-encrypting. Guard it explicitly at the top, mirroring the member path's PartialOriginal from 974a9ec, before password history is diffed against the stripped view. Raised from #1359 review.
The server applies a bulk share, then strips any now-gated cipher from the write-return when the calling client cannot render the partial shape — which today is every non-web client. The loop over the response never touched those entries, so the repository kept the stale pre-share copy: personal-owned, full secrets, wrong revision date, until the next full sync happened to reconcile it. The share stood server-side, so the stale copy is the worse artifact to keep. Track the requested ids against the returned ones and evict the difference; the next sync restores the cipher in its gated shape. Raised from #1359 review.
Start from every id we asked the server to share and remove each one the write-return acknowledges; the leftovers are the evictions. One set whittled down in place of three collections diffed after the fact, and ok_or replaces the require! that forced id collection into a loop.
`CipherResponseModel::name` and `CipherRequestModel::name` are both `Option<String>`, so `name: Some(body.name)` double-wrapped the value and failed to compile. Pre-existing on this branch (introduced with the test in "add a gated edit path for PAM ciphers under lease"), unrelated to the rebase.
7 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41576
📔 Objective
When a cipher is PAM-gated, the server withholds its secret fields and returns a reduced
partial_dataenvelope in their place — the encrypted name and, for logins, the encrypted URIs. This PR adds support to for this field in the SDK.Cipher.partial_data: Option<String>— the raw JSON envelope (defaulted + skip-if-none, so existing ciphers round-trip byte-identically).CipherView.partial/CipherListView.partial: bool— the flag clients read instead of inferring gating from a string.RestrictedCipherData— a private deserialize struct mirroring the server'sPartialCipherData.Stripoutput (a purpose-built camelCase envelope). This is the single authoritative allowlist for what a gated view may expose: name + login URIs, nothing else.decrypt_restricted_cipher_view/_list_view) — short-circuits ahead oftry_parse_blobin both the lenient andStrictDecryptimpls, decrypts only the allowlisted fields, marks the viewpartial = true, and never readslogin/card/… So a gatedLogin/Card/BankAccountno longer fails withMissingField, in either decrypt mode.Wire shape
partial_datais delivered in camelCase carrying the sameLoginUrifields (uri/uriChecksum/match) the SDK already consumes on a full login, so the restricted path reusesLoginUridirectly. Paired with the server change in bitwarden/server#8115, which emits a purpose-built camelCase envelope (rather than round-tripping the legacy PascalCase storage DTO). Clients passpartialDatathrough as an opaque string, so this is a two-party server↔SDK contract with no clients impact.In the future this can be replaced by a client encrypted blob of the partial fields.
Fail-closed
A malformed envelope or an undecryptable field degrades to empty rather than un-gating — the view is always returned
partial = true. TheCipherViewpath still runsremove_invalid_checksums, mirroring the full paths' guard against a tampering server changing URIs.⏰ Reminders before review
Most of the diff outside
cipher.rsis the mechanical one-linepartial_data: None/partial: falseadded to existingCipher/CipherView/CipherListViewconstruction sites (the two new fields). The substance is incrates/bitwarden-vault/src/cipher/cipher.rs.