Bind both halves of the caller on the content ticket path - #41
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: LucaCappelletti94/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @LucaCappelletti94, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 1 day and 23 hours by commenting @sourcery-ai review. Upgrade to get a review now.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer's GuideThe PR closes the content-ticket caller-binding gap by carrying identity and capability subjects together, binding whichever halves are present under deployment-configured settings throughout minting and file-server authorization checks, and using a consistent non-empty attribution key for uploads, commits, and metering; extensive integration tests cover all four caller states and renamed settings. Sequence diagram for content ticket caller bindingsequenceDiagram
participant Session as Session
participant Signer as ContentTicketSigner
participant Ticket as ContentTicket
participant FileServer as FileServer
participant DB as ReaderDatabase
Session->>Signer: mint(ContentCaller, file_id, verb)
Signer->>Ticket: Store identity and subjects
Session->>FileServer: Request content with ticket
FileServer->>DB: bind_caller(settings, ticket.caller)
DB-->>FileServer: Bind present halves, leave absent halves unbound
FileServer->>DB: connetto_visible_files(file_id)
DB-->>FileServer: Visibility result
Flow diagram for caller attribution and upload ownershipflowchart TD
A[Ticket carries ContentCaller] --> B{Caller has identity?}
B -->|Yes| C[Use identity as manifest key]
B -->|No| D{Caller has subjects?}
D -->|Yes| E[Use packed subjects as manifest key]
D -->|No| F[Refuse upload: no manifest key]
C --> G[Bind present caller halves]
E --> G
G --> H[Run visibility and dedup checks]
H --> I[Commit with the same key as attribution]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9aeb328b15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut rendered: Vec<String> = keys.iter().map(|key| key.key().to_string()).collect(); | ||
| rendered.sort_unstable(); |
There was a problem hiding this comment.
When a client presents the same valid capability grant multiple times, Principal::accept retains every copy and this code only sorts them, so the same logical caller can become key:k1, key:k1,key:k1, and so on. Because the packed value now keys both manifest ownership and allow_content_bytes, a capability holder can bypass the per-caller upload window by varying the duplicate count, while equivalent later handshakes may also fail to resume the same manifest; deduplicate the rendered subjects after sorting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and it bit harder than the sort suggested: the packed value keys the manifest and the upload window, so a repeated grant moved both. Packing now sorts and drops repeats, so one holder packs one value however many copies of a grant it presented.
| pub fn attribution(&self) -> Option<&str> { | ||
| self.identity().or_else(|| self.subjects()) |
There was a problem hiding this comment.
For deployments whose string user IDs can overlap capability renderings, an identity such as key:k1 and a capability-only caller holding key:k1 produce the same attribution string. The file server consequently treats these distinct callers as the same uploader for manifest lookup, chunk PUT, and commit, allowing one to resume or commit the other's in-progress manifest when both can obtain a write ticket for that file; encode the caller kind into the storage key rather than returning the raw value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Taken, with the two roles split rather than one value namespaced. A manifest is now owned under a key that names the half it came from, so an identity and a subject that render alike can never share a row, while the deployment keeps receiving the plain value for attribution, because that is what its own policies compare against. A test pins that an identity spelled like the key owns nothing the key holder declared.
| match (caller.identity(), caller.subjects()) { | ||
| (None, None) => Ok(()), |
There was a problem hiding this comment.
After a custom GUC has been set with transaction-local set_config, PostgreSQL keeps the placeholder on that session and restores it to the empty string when the transaction ends; it is no longer missing. Therefore, when a pooled connection previously served an identified caller, this no-op branch makes a later caller with neither half observe current_setting(..., true) = '' rather than NULL, so a policy that legitimately matches an empty identity can authorize the anonymous caller—the exact case this change intends to prevent. Absence needs an explicit representation that survives connection reuse instead of relying on the setting never having existed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it reached further than this change: the same reliance on absence sits behind the snapshot, the write, the visibility check and the per-viewer read on main today. Measured on postgres 16, once a custom setting has a placeholder on a session it reads as the empty string for the rest of that session, and neither set_config with NULL, nor RESET, nor DISCARD ALL, nor a rolled back transaction takes it away. An unheld half now binds an unguessable per-process marker, so absence means the same thing on a fresh and a reused connection and no row can carry the value. The regression test opens a connection as an identified caller first and then asks as a caller holding nothing, and it fails without the marker.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
==========================================
+ Coverage 83.53% 83.64% +0.11%
==========================================
Files 114 115 +1
Lines 26322 26451 +129
Branches 26322 26451 +129
==========================================
+ Hits 21987 22124 +137
+ Misses 3096 3088 -8
Partials 1239 1239
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 584b979595
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let user = user.unwrap_or_else(|| absent_marker().to_owned()); | ||
| let subjects = subjects.unwrap_or_else(|| absent_marker().to_owned()); |
There was a problem hiding this comment.
When a deployment uses current_setting(..., true) IS NOT NULL to recognize an authenticated/capability-bearing caller, these sentinel values make an anonymous caller satisfy that predicate; conversely, policies using IS NULL for anonymous access now deny it. This changes the documented absent-half semantics even on fresh connections, and the file server's bind_caller mirrors the same behavior. Use an absence representation that policies normalize back to SQL NULL, or otherwise keep absent callers on connections where the setting remains unset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The consequence is real and deliberate, and the alternative offered is the one shape that cannot work. Keeping absent callers on connections where the setting was never set is not available under pooling: once any transaction on a session binds a custom setting the placeholder outlives it, and current_setting then returns the empty string forever, which neither set_config with NULL nor RESET nor DISCARD ALL nor a rolled back transaction undoes. That was measured on postgres 16 before the marker was chosen. Normalizing back to NULL in the policy means every deployment rewrites every policy to say NULLIF, and the failure mode of forgetting is silent. So absence is a value no row can carry rather than NULL, and a policy that wants to recognize an anonymous caller tests its own rows rather than testing the setting. Chapter 08 now says this where it used to promise NULL, and chapter 12 follows it.
| pub fn attribution(&self) -> Option<&str> { | ||
| self.identity().or_else(|| self.subjects()) |
There was a problem hiding this comment.
For a capability-only caller holding two grants, subjects() is a packed value such as key:k1,key:k2, so this returns the entire list and post_commit passes it to connetto_set_content_state as one uploader. A deployment setter that stores p_caller as uploaded_by—as the repository fixture does—then creates an owner that cannot match the documented policy uploaded_by = ANY(string_to_array(current_setting('app.subjects', true), ',')), because neither held subject equals the packed list; the newly attributed committed file is therefore invisible to the same caller. Attribution needs to preserve individual subjects or select an explicit subject rather than treating the packed set as one principal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This one is right and the fix is in 48d2da0. The caller now carries its subjects as the list they are and joins them only at the binding, under the separator the deployment's key type chose, so nothing downstream can mistake the joined value for a name. The commit calls the setter once per subject, which gives a two key caller one owner row per key and lets it see its own upload under either. A deployment setter is consequently called more than once for one commit and must be idempotent, which chapter 12 now states. The regression test uploads and commits as a caller holding two keys and then reads the file back as each key alone; attributing only the first key leaves the second at 404.
| /// Caller identity carried for attribution. | ||
| pub caller: String, | ||
| /// The caller this ticket binds to, both halves of it. | ||
| pub caller: ContentCaller, |
There was a problem hiding this comment.
When a deployment reuses CONNETTO_CONTENT_KEY during a rolling upgrade, an unexpired ticket minted by the previous binary still contains a postcard String in this field, while the new verifier expects the two-option ContentCaller structure and returns TicketError::Postcard. Thus downloads and in-progress uploads intermittently fail depending on which old/new node handles the request despite sharing the signing key; version the payload or retain legacy decoding for at least one ticket TTL.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taken. The ticket payload is internal to one deployment's own mint and verify pair, the workspace is at 0.0.0 with no released version to be compatible with, and a ticket lives minutes rather than across an upgrade window. Versioning it now would add a decoding path that no deployment can exercise and that nothing would ever remove. A rolling upgrade that must not drop in-flight tickets is a real concern for the first released version, and the place to solve it is a version byte introduced deliberately at that point, not a legacy arm carried from a pre-release shape.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48d2da0043
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.packed_subjects(',') | ||
| .map(|subjects| format!("keys:{subjects}")) |
There was a problem hiding this comment.
When a deployment chooses a non-comma CapabilityKey::SEPARATOR, commas remain valid inside rendered keys because minting rejects only the configured separator. Consequently, distinct subject sets such as {"a", "b,c"} and {"a,b", "c"} both produce keys:a,b,c here. Since this value keys both manifests and the upload-byte window, those capability callers can share or interfere with the same upload state and quota when both are authorized for a file; use the deployment separator or another collision-free encoding for the storage key.
Useful? React with 👍 / 👎.
…ller-binding # Conflicts: # crates/connetto-core/src/auth.rs # crates/connetto-server/src/bin/connetto-server.rs # crates/connetto-server/src/capability.rs # crates/connetto-test-harness/src/fanout.rs
…ller-binding # Conflicts: # crates/connetto-server/src/bin/connetto-server.rs # crates/connetto-server/src/openfga.rs
|
1 participant
Chapter 08 promises four arrival cases for a caller, and says every row is load-bearing: an identity, a share key, both, or neither. Every path that runs SQL as the caller honoured that through one shared binding, except the content ticket path, which rendered the caller as the identity or an empty string. A caller whose rights come from a share key could therefore never obtain a ticket, and an unidentified one was bound to a blank identity the chapter explicitly forbids, since an empty string is a real identity a policy can match where an unbound setting reads as NULL and fails closed.
The caller now crosses the mint seam as a value with two optional halves, and the ticket carries both, so the file server binds what the mint bound. One binding function replaces the three inline bindings behind the download, the needed-hashes answer and the commit dedup check, and the file server takes the setting names from its own configuration rather than a literal, so a deployment that renamed either is answered under its own names. A caller holding neither half owns no manifest key and is refused when it declares an upload, rather than keyed on the empty string alongside every other nameless caller, and that same key is what the deployment receives as the commit's attribution. The per-viewer re-execution read follows the same rule: a share key is somebody to read as, so only a caller that binds nothing is refused.
Every case was watched failing first, for the reason stated, and the ones that could go green by accident were re-confirmed by removing the subject half and the configured name again afterwards. Two things are deliberately not here. The demo's browser arm needs a policy shape that admits a share key, which no translator pair accepts today, and a caller holding only a key still cannot take a subscription whose policy compiles a term, because the membership mirror renders the identity and has no spelling for a subject.
Summary by Sourcery
Propagate the complete caller through content ticket minting and file-server authorization so share-key access works consistently while absent caller halves fail closed.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: