docs(file-storage): split into control plane + signed-URL sidecar (ADR-0003)#4097
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates FileStorage documentation and migration notes to describe a control-plane/sidecar split, opaque signed URL transport, immutable versioned content storage, revised conditional and range semantics, and schema changes that move content state from ChangesFileStorage sidecar architecture transition
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
| * Good, because it is the **canonical S3 SigV4 scheme** — ~20 years in production, universally understood, well-tooled | ||
| * Good, because discrete fields are **debuggable** (visible/greppable in logs) and verified without an extra decode step | ||
| * Bad, because re-presigning changes the URL → CDN cache-key churn unless normalized | ||
| * Bad, because the signature appears in URLs (logs/Referer/history) — accepted, same as S3 |
There was a problem hiding this comment.
Bad, because browser/frontend/proxy may log everything (maybe it's OK for some cases, but not OK for others), imagine some of the fields need to be hiddent (e.g. signature expiration)
Bad, because if I have a batch request I'll need to pass and parse parameters for every file
There was a problem hiding this comment.
Addressed in the reworked ADR-0004. Fields like exp aren't secrets — the only secret is the signature, which is in the URL regardless, so hiding the other fields buys no security. If the credential must stay out of URLs/logs, that's the new header envelope (for programmatic/SDK/batch), which also removes the per-file query-param parsing for batch. The query envelope stays only for bare, embeddable URLs (browser/<img>).
There was a problem hiding this comment.
Resolved by the token model. The credential is now an opaque token; to keep it out of URLs/logs entirely, programmatic/batch callers use the X-FS-Token header (the fs-token query form is only for bare embeddable URLs). One opaque token per file also removes the per-file param parsing in batch. (Note: PASETO v4.public is signed-not-encrypted, so exp is technically decodable — the opacity is an interface/evolvability boundary, not secrecy; exp isn't a secret anyway.)
| ### B. Single opaque query token | ||
|
|
||
| * Good, because it is compact and hides internal structure | ||
| * Bad, because it must be base64-decoded and parsed before verification — extra ceremony, easy to get wrong |
There was a problem hiding this comment.
Does it mean 'query' parameters will not be in base64 and will not parse it?
I think we need to have base64-decode, parsing and verification regardless of the URL vs header vs other options.
There was a problem hiding this comment.
Correct — parsing + Ed25519 verification always happen, in any envelope. I dropped the misleading 'verify without a decode step' pro. The point is only that with discrete fields there is no opaque token to base64-decode (named params + one composite signature); decode/parse isn't an argument for either side. We kept discrete fields.
There was a problem hiding this comment.
Agreed, and that is exactly the model now: the sidecar base64-decodes + PASETO-verifies the token in whichever carrier it arrives (query or header). The earlier 'verify without a decode step' framing is gone.
| ### D. Single opaque HTTP-header token | ||
|
|
||
| * Good, because URL is stable and the secret is out of the URL | ||
| * Bad, because it combines the worst of B and C: opaque blob (no debuggability, decode ceremony, no benefit) **and** |
There was a problem hiding this comment.
I see contradiction of what is Bad and what is Good:
Option A says:
Bad, because the signature appears in URLs (logs/Referer/history) <<< I think is correct 'bad'
Option D says:
Bad, because ... no debuggability
What is our debuggability pattern?
I think it should be like - whatever logging is intentional:
- no information leak on rpoxy / routers / browser / etc
- whatever logging is intentional and sanitized (e.g. log tenant ID, but do not log token expiration)
There was a problem hiding this comment.
Fixed — the contradictory 'debuggability via visible URL' pro is removed. Debuggability is now defined as intentional, sanitized server-side structured logging (log tenant/file ids + outcome, never the signature or raw exp), independent of the envelope. URL visibility is treated only as the leak it is, not as a feature.
There was a problem hiding this comment.
Fixed. Debuggability is defined as sanitized server-side structured logging by control/sidecar (tenant/file ids + outcome; never the token or exp), independent of carrier. With the opaque token the edge intentionally cannot read fields — observability is server-side only. The contradictory 'debuggability via visible URL' pro is removed.
c9df4f4 to
7498453
Compare
|
Quota enforcement: the data plane should enforce the cap directly at upload, with the allowance carried in the signed URL Context. DESIGN §4.5 ("No policy or quota in the data plane", lines 1427–1434) and PRD cpt-cf-file-storage-fr-storage-quota currently place all quota logic in the control plane, evaluated only at presign: if an We discussed this and believe presign-only enforcement leaves two gaps that the control/data-plane split (ADR-0003) introduces and that the monolith (ADR-0001) did not have, because in the monolith the quota check
Proposed direction (what we want the P2 design to adopt). Make the data plane enforce the quota cap directly during the upload stream, and carry the maximum allowance inside the signed URL / token — not just a Concretely:
Reconciling with the "policy-free data plane" principle. This does not ask the sidecar to read tenant/owner quota state or make a policy decision — the decision stays in the control plane at presign; the sidecar What remains a control-plane responsibility is the aggregate owner quota across many outstanding URLs (gap 2). The clean complement to the above is a reserve-at-presign / commit-at-bind model: the control plane Asks for the P2 FEATURE (cpt-cf-file-storage-fr-storage-quota):
|
7498453 to
60bed98
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
gears/file-storage/docs/ADR/0003-cpt-cf-file-storage-adr-sidecar-data-plane.md (2)
158-158: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor style: remove redundant "of".
Line 158 reads "all of the prior proxy-all design's properties" — the "of" is redundant here. Suggested fix: "all the prior proxy-all design's properties".
✏️ Proposed fix
-* Good, because all of the prior proxy-all design's properties hold trivially (everything is in one place) +* Good, because all the prior proxy-all design's properties hold trivially (everything is in one place)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gears/file-storage/docs/ADR/0003-cpt-cf-file-storage-adr-sidecar-data-plane.md` at line 158, The phrase "all of the prior proxy-all design's properties" contains a redundant "of" that should be removed for better style and readability. Locate this text in the bullet point describing why the design is good (around line 158) and change it to "all the prior proxy-all design's properties" by removing the word "of" between "all" and "the".
99-107: 🧹 Nitpick | 🔵 TrivialClarify owner-level quota enforcement responsibility.
Per the PR objectives and substantive feedback, FileStorage must address two quota gaps: (1) presign cannot know real size unless the client declares
ExactSize, and (2) concurrent presigns create a TOCTOU race. The proposed P2 solution bakes a per-URL quota allowance (computed from the owner's remaining quota at presign time) into the signed URL and has the sidecar enforce it mid-stream.ADR-0003 mentions per-upload size constraints (
max_size/exact_sizein lines 102–103) but does not address the owner-level reserve/commit/release lifecycle that synchronizes presign → bind → expiry. DESIGN §4.5 clarifies that owner-level quota (storage quota, per-owner caps) is a control-plane concern evaluated at presign (P2); the sidecar enforces only per-URL constraints and holds no tenant/user/backend policy state.Add a brief note to §4 explaining that per-upload size constraints (
max_size/exact_size) are sidecar-enforced, while owner-level quota reserve/commit/release is a control-plane responsibility (documented in DESIGN §4.5).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gears/file-storage/docs/ADR/0003-cpt-cf-file-storage-adr-sidecar-data-plane.md` around lines 99 - 107, The ADR currently mentions per-upload size constraints (max_size and exact_size) in the constraints section but does not clearly delineate responsibility boundaries between the sidecar and control plane for quota enforcement. Add a clarifying note to section §4 explaining that max_size and exact_size are sidecar-enforced per-URL constraints applied during upload, while the owner-level quota reserve/commit/release lifecycle (managing storage quota and per-owner caps) remains a control-plane responsibility evaluated at presign time and synchronized across presign, bind, and expiry events, noting that DESIGN section 4.5 provides further details on this control-plane quota mechanism.gears/file-storage/docs/ADR/0004-cpt-cf-file-storage-adr-signed-url-transport.md (1)
114-132: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winToken Opacity Contract is correctly framed as an encapsulation/evolvability boundary.
The contract explicitly distinguishes between secrecy (not provided; PASETO v4.public is signed-not-encrypted, so payload is base64-decodable) and opacity (enforced by convention: only control+sidecar parse; everyone else treats it as opaque bytes and format can evolve freely). This is the correct mental model and resolves the prior design tension: opacity is not about hiding field values — it is about decoupling the external world from internal format changes.
The rationale (lines 129–132) is sound:
expwas never a secret; the only secret is the signing key (held solely by control). Field values likeexpare acceptable-to-expose; the real value of opacity is format encapsulation — control and sidecar deploy together, so they can change claims and crypto at will without coordinating with browsers, CDNs, proxies, or consuming apps that merely forward the token.This closes the earlier design debate (past comments from Artifizer / ffedoroff about what is debuggable, what leaks, what the security model is) and provides a clean, logically consistent model: encapsulation, not secrecy.
Recommend: ensure observability guidance in DESIGN/ops docs emphasizes that logging/debugging must be done at the control/sidecar level (they know the format), never by decoding the token at the edge. This prevents intermediaries from mistakenly trying to parse the token and breaking on format changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gears/file-storage/docs/ADR/0004-cpt-cf-file-storage-adr-signed-url-transport.md` around lines 114 - 132, Update the observability and operations documentation to ensure it clearly emphasizes that all logging and debugging related to token validation and format must be performed exclusively at the control plane and sidecar level where the token format is known, and that intermediaries (browsers, CDNs, proxies, API gateways, logging systems) must never attempt to parse, decode, or inspect tokens as this practice would break when the internal format evolves. Make explicit that any needed debugging insights should come from sanitized structured logs emitted by the control/sidecar components, not from token introspection at the edge.gears/file-storage/docs/api.md (3)
122-144: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor: Clarify which signed URL claims are mandatory in P1 vs optional.
The claims table (lines 122–133) is well-formatted and shows the phase and violation for each claim. However, the "Req." column uses "yes" for
expandopand "no" for all others, which implies that in P1 onlyexpandopare mandatory. The narrative below (lines 134–144) reinforces this.For absolute clarity to implementers, consider adding a brief note: "In P1, only
expandopare required on every signed URL. All other claims are optional; if not present, they impose no constraint." This makes the read path explicit: ifmax_sizeis absent, the sidecar does not enforce a size limit for that URL.Alternatively, the table header could be expanded to clarify: "Req. (in Phase X)" to remove ambiguity about what "yes" means across phases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gears/file-storage/docs/api.md` around lines 122 - 144, The documentation about signed URL claims needs clearer explanation of mandatory vs optional claims in P1. Add an explicit note after the claims table stating that in P1, only the `exp` and `op` claims are required on every signed URL, while all other claims are optional and impose no constraint if absent (e.g., if `max_size` is not present, the sidecar will not enforce a size limit). Alternatively, modify the table header's "Req." column to read "Req. (in Phase X)" to make it unambiguous that the yes/no values apply specifically to the P1 phase. This will clarify the read path for implementers so they understand the default behavior when optional claims are omitted.
173-193: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffVerify that response headers X-FS- header set is complete and matches implementation expectations.*
The response headers section (lines 173–193) lists a comprehensive set of headers, including
X-FS-File-Id,X-FS-Version-Id,X-FS-GTS-File-Type,X-FS-Hash-Algorithm,X-FS-Hash-Value,X-FS-Metadata-Revision,X-FS-Owner-Kind,X-FS-Owner-Id,X-FS-Created-At, baked response headers, and per-metadata-key headers (X-FS-Meta-<key>).The list aligns with the PRD's description of versioning, metadata, and hash tracking. However, a few details should be verified:
Are all X-FS- headers always present, or optional?*
- For instance, if a file has no custom metadata, should
X-FS-Meta-*headers be absent?- Should all headers be present on every response?
Custom metadata headers (X-FS-Meta-):
- Custom metadata is updated via PATCH and optionally passed on download presign.
- Should these headers be included on sidecar GET/HEAD responses, or only on control GET/HEAD?
- If included on sidecar responses, how does the sidecar know which custom metadata keys to return (the signed URL could carry them, but this adds overhead)?
Baked response headers:
- The section notes "baked response headers echoed verbatim from the token's response-header claims".
- These are useful for setting
Content-Disposition,Cache-Control, etc. without a control-plane round-trip.- Should a list of allowed/safe baked headers be documented to prevent header injection?
For clarity, the response headers section should note which headers are always present, which are conditional, and how custom metadata headers are conveyed to the sidecar (if at all).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gears/file-storage/docs/api.md` around lines 173 - 193, The response headers section (lines 173–193) needs clarification on header presence and handling. Add documentation notes to specify which X-FS-* headers are always present versus optional/conditional (for example, clarify whether X-FS-Meta-<key> headers appear only when custom metadata exists). Additionally, clarify whether custom metadata headers are included on sidecar GET/HEAD responses and explain how the sidecar determines which metadata keys to return. Finally, document any restrictions or an allowed list for baked response headers to prevent header injection vulnerabilities, ensuring the section clearly distinguishes between headers always present, conditionally present, and those controlled by token claims.
74-90: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffClarify multipart behavior for backends without native multipart support and the BLAKE3 subtree-hash mechanics.
The multipart section (lines 74–90) states:
"For a
multipart_nativebackend the sidecar drives the backend multipart API; otherwise it offset-writes each part into the single new-version object. Per-part BLAKE3 subtree hashes are persisted inmultipart_upload_parts.part_hashand combined into the root atcomplete."Several details need clarification:
How does the sidecar know whether a backend is
multipart_native?
- Is this advertised via the backend capabilities discovery?
- Or inferred at presign/initiate time?
When a backend is NOT multipart-native, does the sidecar offset-write parts sequentially or in parallel?
- If sequential, the order is deterministic.
- If parallel, how are offsets coordinated to avoid overwrites?
BLAKE3 subtree-hash combination:
- The PRD mentions multipart validation (line 326–329: "first part validation for content-type checking").
- The api.md references BLAKE3 subtree hashes but does not explain the hash-combination algorithm.
- Is the root hash =
BLAKE3_combine(part_hash_0, part_hash_1, ...)?- Or is it computed incrementally as parts arrive?
Is P2 multipart meant to be server-authoritative only, or can clients also request specific part boundaries?
- Line 76 says "server-authoritative" (client suggests params, control plane returns exact plan).
- This is good for avoiding off-by-one errors, but limits client flexibility (e.g., resuming a partial upload with different part boundaries).
For P1 clarity and P2 design, the multipart specification should address these details or explicitly defer them to the P2 FEATURE. If they are in DESIGN.md, this api.md section should cross-reference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gears/file-storage/docs/api.md` around lines 74 - 90, The P2 multipart upload section lacks clarity on four critical implementation details that must be addressed: (1) how the sidecar discovers whether a backend is multipart_native (via capabilities discovery or runtime inference), (2) whether non-native backends perform offset-writes sequentially or in parallel and how offset coordination prevents overwrites, (3) the exact BLAKE3 subtree-hash combination algorithm (simple combine vs. incremental computation), and (4) whether server-authoritative behavior is absolute or if clients can negotiate specific part boundaries. Expand the documentation after the endpoint definitions (P2-1 through P2-5) to explicitly answer each point, or add cross-references to DESIGN.md if these details are defined elsewhere. Include information about how the multipart_native capability is advertised and how the sidecar behaves differently for native versus non-native backends.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gears/file-storage/docs/api.md`:
- Around line 91-107: The "Upload, bind, and the conflict retry" section
describes the happy path but does not address the edge case behavior when a
client receives a 412 conflict with a version_id but then decides to re-presign
(call POST /files/{id}/versions again) instead of rebinding the returned
version_id. Add a clarification note to this section explaining whether
re-presigning after a 412 is idempotent (returns the same version_id) or creates
a new sibling version_id, and recommend that clients should prefer rebinding the
returned version_id without re-presigning to avoid orphan accumulation. You can
document this as either: "On 412, the client should rebind the returned
version_id; if re-presigning occurs, the control plane returns the same
version_id (idempotent)" or "Presigning for a file with a pending version
returns the existing pending version_id and its upload URL, making re-presign a
no-op."
In `@gears/file-storage/docs/PRD.md`:
- Around line 214-224: In the section describing the bind conflict behavior
(where 412 is returned), clarify the signed URL retry behavior by explicitly
documenting whether the original signed URL remains valid for the retry or if
the client must re-presign with the control plane. Add guidance that if the
original signed URL has expired by the time the 412 is received, the client
should request a fresh signed URL before retrying the bind, and that the control
plane provides a fresh If-Match value for the retry attempt. This clarification
should address the operational concern about using potentially stale URLs across
multiple retries while maintaining the core requirement that bytes do not need
to be re-uploaded.
---
Nitpick comments:
In
`@gears/file-storage/docs/ADR/0003-cpt-cf-file-storage-adr-sidecar-data-plane.md`:
- Line 158: The phrase "all of the prior proxy-all design's properties" contains
a redundant "of" that should be removed for better style and readability. Locate
this text in the bullet point describing why the design is good (around line
158) and change it to "all the prior proxy-all design's properties" by removing
the word "of" between "all" and "the".
- Around line 99-107: The ADR currently mentions per-upload size constraints
(max_size and exact_size) in the constraints section but does not clearly
delineate responsibility boundaries between the sidecar and control plane for
quota enforcement. Add a clarifying note to section §4 explaining that max_size
and exact_size are sidecar-enforced per-URL constraints applied during upload,
while the owner-level quota reserve/commit/release lifecycle (managing storage
quota and per-owner caps) remains a control-plane responsibility evaluated at
presign time and synchronized across presign, bind, and expiry events, noting
that DESIGN section 4.5 provides further details on this control-plane quota
mechanism.
In
`@gears/file-storage/docs/ADR/0004-cpt-cf-file-storage-adr-signed-url-transport.md`:
- Around line 114-132: Update the observability and operations documentation to
ensure it clearly emphasizes that all logging and debugging related to token
validation and format must be performed exclusively at the control plane and
sidecar level where the token format is known, and that intermediaries
(browsers, CDNs, proxies, API gateways, logging systems) must never attempt to
parse, decode, or inspect tokens as this practice would break when the internal
format evolves. Make explicit that any needed debugging insights should come
from sanitized structured logs emitted by the control/sidecar components, not
from token introspection at the edge.
In `@gears/file-storage/docs/api.md`:
- Around line 122-144: The documentation about signed URL claims needs clearer
explanation of mandatory vs optional claims in P1. Add an explicit note after
the claims table stating that in P1, only the `exp` and `op` claims are required
on every signed URL, while all other claims are optional and impose no
constraint if absent (e.g., if `max_size` is not present, the sidecar will not
enforce a size limit). Alternatively, modify the table header's "Req." column to
read "Req. (in Phase X)" to make it unambiguous that the yes/no values apply
specifically to the P1 phase. This will clarify the read path for implementers
so they understand the default behavior when optional claims are omitted.
- Around line 173-193: The response headers section (lines 173–193) needs
clarification on header presence and handling. Add documentation notes to
specify which X-FS-* headers are always present versus optional/conditional (for
example, clarify whether X-FS-Meta-<key> headers appear only when custom
metadata exists). Additionally, clarify whether custom metadata headers are
included on sidecar GET/HEAD responses and explain how the sidecar determines
which metadata keys to return. Finally, document any restrictions or an allowed
list for baked response headers to prevent header injection vulnerabilities,
ensuring the section clearly distinguishes between headers always present,
conditionally present, and those controlled by token claims.
- Around line 74-90: The P2 multipart upload section lacks clarity on four
critical implementation details that must be addressed: (1) how the sidecar
discovers whether a backend is multipart_native (via capabilities discovery or
runtime inference), (2) whether non-native backends perform offset-writes
sequentially or in parallel and how offset coordination prevents overwrites, (3)
the exact BLAKE3 subtree-hash combination algorithm (simple combine vs.
incremental computation), and (4) whether server-authoritative behavior is
absolute or if clients can negotiate specific part boundaries. Expand the
documentation after the endpoint definitions (P2-1 through P2-5) to explicitly
answer each point, or add cross-references to DESIGN.md if these details are
defined elsewhere. Include information about how the multipart_native capability
is advertised and how the sidecar behaves differently for native versus
non-native backends.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61acd372-4f6f-4a4f-a49f-8713f038b660
📒 Files selected for processing (8)
gears/file-storage/docs/ADR/0001-cpt-cf-file-storage-adr-proxy-content-traffic.mdgears/file-storage/docs/ADR/0002-cpt-cf-file-storage-adr-content-hash-selection.mdgears/file-storage/docs/ADR/0003-cpt-cf-file-storage-adr-sidecar-data-plane.mdgears/file-storage/docs/ADR/0004-cpt-cf-file-storage-adr-signed-url-transport.mdgears/file-storage/docs/DESIGN.mdgears/file-storage/docs/PRD.mdgears/file-storage/docs/api.mdgears/file-storage/docs/migration.sql
💤 Files with no reviewable changes (1)
- gears/file-storage/docs/ADR/0001-cpt-cf-file-storage-adr-proxy-content-traffic.md
|
@pr0tey good idea — and it already fits the model: the sidecar enforces a per-URL size cap carried in the token (the One contract decision, though: On the aggregate-owner-quota / TOCTOU gap: the reserve-at-presign / commit-at-bind / release-on-expiry lifecycle is the right direction and we've recorded it as a P2 control-plane concern in the Captured in DESIGN §4.5 (the size claim is optional; when present the sidecar MUST enforce it; the per-URL |
|
Pushed
(Inline replies left on each thread.) |
33c3649 to
aebb006
Compare
MikeFalcon77
left a comment
MikeFalcon77
left a comment
There was a problem hiding this comment.
Inline architecture notes.
| discrete `X-FS-*` parameters are replaced by a **single token** carried as `?fs-token=<token>` (query) or an | ||
| `X-FS-Token` header. SigV4-style canonical-string signing is replaced by **PASETO mint (control) / verify | ||
| (sidecar)**; all claims live inside the token. | ||
| * **New dependency:** a PASETO v4 library — control plane signs (`v4.public`), sidecar verifies. Ed25519 keys as before |
There was a problem hiding this comment.
Before we lock in PASETO, can we confirm the FIPS story? Ed25519/PASETO may be fine cryptographically, but if this must run in FIPS mode we need a compliant dependency/provider plan, or a fallback, before implementation.
There was a problem hiding this comment.
Confirmed in ADR-0004: documented the FIPS posture — Ed25519/PASETO is the choice, with a noted fallback plan if a FIPS-mode provider is mandated (tracked for P3).
There was a problem hiding this comment.
What I mean that we shouldn't bring new rust crates which are using non-FIPS algorithms and we will unable to replace thse algs
There was a problem hiding this comment.
Understood — the constraint is on dependency selection, not a deferred fallback. Reworked ADR-0004 "FIPS posture" (cc63f02f) accordingly:
- MUST NOT add any crate that hard-wires a non-FIPS algorithm or a self-contained crypto impl we cannot swap.
- Sign/verify sits behind a thin in-house
SignatureProviderabstraction (sign/verify); that abstraction — not the crate — binds to the concrete algorithm + module. - FIPS deployments back the provider with a validated module (
rustls-corecrypto-provider); otherwise swap to a FIPS-approved alternative (ECDSA P-256) with no codec/claim-set change (opaque token). - The candidate PASETO crate is evaluated for this replaceability before adoption; one that statically links a non-replaceable non-FIPS impl is rejected.
DESIGN.md and api.md carry the same rule. The replaceability requirement gates dependency selection and is no longer deferrable.
b99cda9 to
4a86012
Compare
MikeFalcon77
left a comment
MikeFalcon77
left a comment
There was a problem hiding this comment.
Approved with note: please be sure that we don't use non-FIPS compatible crates
…R-0003)
Supersede ADR-0001 (proxy-all monolith) with ADR-0003: a control plane that
issues short-lived Ed25519 signed URLs and a data-plane sidecar that moves the
bytes. The signed URL points at the sidecar (never the backend), so backend
opacity, per-byte metering, and uniform audit/policy coverage are preserved
while the byte-moving data plane scales independently.
- Content is an immutable blob per version at /{file_id}/{version_id}; a file's
live content is the content_id pointer, swapped under optimistic CAS
(If-Match). A new version is a new object + a pointer swap; backend content is
never mutated. A bind 412 retries the rebind without re-uploading bytes.
- Three identifiers: content_id (current version pointer; content-only ETag),
meta_version (metadata CAS), derived snapshot (file_id, content_id, meta_version).
- Versioning is now P1 and FileStorage-level (distinct objects + pointer),
backend-agnostic; versioning_native capability removed. Cleanup engine
(version retention + orphan reconciliation) stays P2.
- Multipart (P2): server-authoritative parts plan with per-part BLAKE3 subtree
hashes persisted in the shared DB and combined at complete.
- Signed URLs: Ed25519, stateless, AND-combined constraints (exp, ip, token-claim
predicates) + baked response headers; single static keypair in P1.
- DB: files slimmed to identity + content_id + meta_version; new P1 file_versions
table (immutable versions); P2 hash widening + P3 encryption move to file_versions.
PRD/DESIGN/api.md/migration.sql updated; ADR-0002 references repointed to the
sidecar. cpt validate: 0 errors, code coverage 104/104.
Signed-off-by: Roland From <rfedorov@linkentools.com>
…d examples Builds on the sidecar redesign: - ADR-0004 (signed-URL field & signature transport): discrete X-FS-* query params chosen (bare shareable URL, S3 SigV4 query form); headers = deferred opt-in; token blobs rejected. Includes an option-comparison matrix. - Signed-URL constraint model: exp mandatory + capped by max_url_ttl (~7d, enforced at signing); optional ip, token-claim predicates, and upload-only MaxSize/ExactSize (mutually exclusive) + ExpectedHash; MaxRate/MaxConns declared P2 (per-(file_id,op), cross-instance coordination deferred). op is signed and method-checked. - Trimmed redundant URL params: file_id is the path; backend_id resolved from the version row; no iat (cap enforced at signing); no key id in P1 (single static key; X-FS-KeyId returns in P2 with rotation). - Data plane holds no backend/tenant/user quota or policy — all such limits live in the control plane (P2) and are applied at presign; the sidecar only enforces per-URL signed constraints (+ per-URL conn/rate in P2). - Worked examples (DESIGN 4.6/4.7): LMS image upload+display end-to-end with example URLs and a per-field breakdown; multipart upload + resume after logout (non-contiguous missing parts; session expires_at vs per-part URL exp; reaped-session branch). Sidecar domain placement (cookies vs short-lived URLs) and signing locality/cost notes added. - Fix: removed ';' from mermaid sequence message/note text (mermaid treats it as a statement separator). cpt validate: 0 errors, code coverage 104/104. Signed-off-by: Roland From <rfedorov@linkentools.com>
…(query + header) Address @Artifizer review on signed-URL transport: - Keep discrete SigV4-style X-FS-* fields with one composite Ed25519 signature; reject the opaque token (PASETO). Opacity gives no security (fields aren't secrets; the signature is in the URL regardless), loses edge observability (CDN/WAF/logs can't read exp/scope without decoding), is non-standard, and the 'single signature' it advertises is already provided by composite signing. - Adopt a dual envelope chosen by access intent: query params (bare, embeddable URL) + the same fields/signature carried as request headers (programmatic / SDK / batch: credential out of the URL, stable cacheable URL). Supersedes the earlier 'query-only, header deferred'. - Remove the contradictory 'debuggability via visible URL' pro; debuggability is sanitized server-side structured logging (never signature/exp). - S3 SigV4 cited only as evidence the shape (discrete fields, one signature, dual envelope) is sound; NOT S3 wire-compatible. - Reworked options (A-D), pros/cons, and the option-comparison table. Branch rebased onto latest main. Signed-off-by: Roland From <rfedorov@linkentools.com>
Team decision (reverses the discrete-fields ADR-0004): the signed-URL credential is a single opaque PASETO v4.public token (Ed25519, asymmetric, not JWT), carried in the query param 'fs-token' (?fs-token=<token>, embeddable) or the 'X-FS-Token' header (programmatic/batch). The token is NEVER carried in Authorization, which always holds the platform JWT. Rationale: we are not S3-wire-compatible, so discrete-field readability buys nothing externally and only couples intermediaries to our layout; an atomic, opaque token is simpler and lets the format evolve freely. Token Opacity Contract: the token's claim-set and crypto are private to control plane + sidecar; every other participant (browser, CDN, proxy, app, logs, SDK) MUST treat it as opaque bytes and never parse it — the format can and will change. kid lives in the PASETO footer (P2 rotation). - ADR-0004 fully reworked (opaque token; dual envelope; option matrix). - DESIGN: §4.5 token spec + claims, §4.6/§4.7 examples (?fs-token=<token>), domain SignedUrl, components, principle, driver table, mermaid. - PRD: fr-signed-urls, glossary, sidecar interface, acceptance criteria. - api.md: Signed URLs section, status codes. - migration.sql unchanged (signing is stateless; nothing in the DB). Rebased onto latest main (origin/main). Signed-off-by: Roland From <rfedorov@linkentools.com>
Per PR review (@Artifizer): superseded ADRs add confusion for humans/LLMs and the history is available via git log. ADR-0001 (proxy-all monolith) is superseded by ADR-0003; deleted the file and scrubbed all references — ADR-0003/0002/DESIGN no longer link it, and the dangling cpt-id is removed. ADR-0003 now reads self-contained (the prior proxy-all design is described in-place). Signed-off-by: Roland From <rfedorov@linkentools.com>
Address PR review:
- CodeRabbit (api.md, PRD): on a 412 bind conflict the client re-binds the
returned version_id via the control endpoint POST /files/{id}/bind, which is
independent of the signed upload URL (its exp is irrelevant; no re-presign, no
re-upload). The 412 carries the current content ETag for the fresh If-Match.
Re-presigning is not idempotent (creates a new sibling version, swept by the
P2 cleanup engine).
- @pr0tey (quota): record that the token's max_size is OPTIONAL — if present the
data plane MUST enforce it (mid-stream 413), if absent only the backend's own
default limits apply. The per-URL max_size baked from a quota decision is the
one quota-derived value the data plane enforces (still a signed number, not a
policy lookup). Reserve-at-presign/commit-at-bind/release-on-expiry deferred to
the P2 fr-storage-quota FEATURE.
Signed-off-by: Roland From <rfedorov@linkentools.com>
Bring the review-resolved documentation onto the PR branch: - DESIGN.md: auth model (delegated authz), sidecar contract, short-default + ceiling URL TTL, P1 implementation notes - api.md: sidecar platform exception, Ed25519 codec + FIPS note, default/max URL TTL, stale-permission behavior - migration.sql: flat column names, version_id PK, atomic-bind invariant - ADR-0003/0004, PRD.md: aligned with the above - PLAN-P1.md: P1 coding plan Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Roland From <rfedorov@linkentools.com>
4a86012 to
8dc91ff
Compare
…ncy constraint Address review: the concern is not a deferred fallback but dependency selection — do not pull in any crate that hard-wires a non-FIPS algorithm we cannot swap. - ADR-0004 'FIPS posture': reworked into a binding rule — sign/verify sits behind an in-house SignatureProvider abstraction; MUST NOT add a crate that statically links a non-replaceable non-FIPS implementation; FIPS deployments back the provider with a validated module (rustls-corecrypto-provider) or swap to ECDSA P-256 without codec changes; candidate PASETO crate evaluated for replaceability before adoption. - DESIGN.md + api.md: synchronized short form of the same rule. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Roland From <rfedorov@linkentools.com>
4 participants
Overview
Reworks the FileStorage design to a two-plane architecture (new ADR-0003, supersedes ADR-0001).
The presigned URL always points at our sidecar, never at the raw backend — so backend opacity, centralized per-byte metering, and uniform audit/policy coverage are all preserved, while the bandwidth-bound data plane scales independently of the control plane. Every content operation is therefore at least two requests: a control request to mint a signed URL, then one or more data requests against the sidecar.
Identity & version types
A file's identity is separated from its content. Content is an immutable blob per version; the file is a small mutable record holding a pointer to the current version.
file_iduuidfiles(PK)version_iduuidfile_versions/{file_id}/{version_id}and is written once, never mutated.content_iduuid(nullable)filesversion_idcurrently bound as the file's live content.NULLuntil the first bind. Changing content is a pointer swap (content_id := new version_id), not an in-place mutation.meta_versionbigintfilesDerived, not stored:
(file_id, content_id, meta_version)— uniquely pins the complete observable state (content axis × metadata axis, orthogonal).(file_id, content_id):content_id, so the ETag returns to that version's value (content-addressed).Write semantics. A content write is: upload a new immutable version → bind it. Bind is an optimistic CAS on
content_idguarded byIf-Match. On conflict the bind returns412and theversion_id; the client re-reads the current ETag and replays the bind with the freshIf-Match— no byte re-upload, because the version already exists durably.meta_versiongives metadata its own validator via the optionalIf-Match-Metadata: <u64>header (absent → last-write-wins).content_idreplaces the oldcontent_revisioncounter: it is the pointer (no separate counter to maintain), at the cost of monotonicity (usecreated_atfor ordering).Signed URLs & token validation
Signed URLs are how the control plane delegates a single content operation to the sidecar. They are stateless (à-la S3 presigned: verifiable with no DB lookup).
Signing (Ed25519, control-minted)
X-FS-*):X-FS-Algorithm=Ed25519,X-FS-KeyId,X-FS-Expires=<unix>,X-FS-Op=GET|PUT|part, resource (X-FS-FileId,X-FS-ContentId/X-FS-VersionId,X-FS-BackendId), constraints (X-FS-Ip,X-FS-Tok-<claim>), baked response headers (X-FS-Rh-<name>), and finallyX-FS-Signature.method+host+path+ allX-FS-*params exceptX-FS-Signature, sorted by key and percent-encoded (RFC 3986), joinedk=v&….hostis signed → a URL cannot be replayed against a different sidecar.X-FS-*(minus the signature) means any added, removed, or altered parameter breaks verification — so there is no separateSignedParamslist, and a client cannot strip or weaken a constraint.Rangeheader (so one signed download URL serves many ranges — random access), conditional headers, and thePUTbody (byte integrity is verified by the hash at bind, not the URL). Consequence: a signedPUTURL can be replayed with different bytes untilexp→ that only creates an orphan version/blob (swept by the P2 cleanup engine).Constraints (AND-combined)
A signed URL carries any number of constraints, all of which must hold:
exp— expiry (required).ip/ CIDR — optional client-address restriction.X-FS-Tok-<claim>=<value>(e.g.typ=user,sub=<id>,tenant_id=<id>, any claim)."Available to everyone for 5 minutes" = only
exp(no token needed). "Only ausertoken withsub=123…" =exp+X-FS-Tok-typ=user+X-FS-Tok-sub=123….Verification (at the sidecar)
On each request the sidecar:
X-FS-KeyId; rejects (403) on mismatch.exp; checksip/CIDR if present.X-FS-Tok-<claim>predicates, the request must present a valid platform JWT. The sidecar validates that token the standard platform way and then checks each predicate (claim == value), all AND-combined. Any missing/invalid token or unmatched claim →403. A URL with no token predicate is served on signature +exp/ipalone.X-FS-Rh-*response headers verbatim (e.g.Content-Disposition,Content-Typeoverride,Cache-Control) — no control round-trip.This keeps authorization decisions on the control plane (made at presign, encoded into the URL) and the sidecar a pure enforcer of what the URL + token assert.
Flows
Upload (single-shot, P1):
POST /files(authz) → signedPUTurl → clientPUTs to the sidecar withIf-Match→ sidecar pre-registers apendingversion via SDK (checksIf-Matchas a free early-fail, before any bytes) → streams to the backend computing size + SHA-256 → auto-binds under app-token + on-behalf-of user.412→ sidecar returnsversion_id→ client rebinds, no re-upload.Download (P1):
GET /files/{id}/download-url(authz) → signedGETurl pinning the currentcontent_id→ client fetches from the sidecar (Range/conditional handled there).Multipart (P2): server-authoritative parts plan (control returns exact part sizes/offsets + a signed URL per part); native backend multipart or offset-writes into the new-version object; per-part BLAKE3 subtree hashes persisted in
multipart_upload_parts.part_hashand combined into the root atcomplete.Versioning & cleanup
content_idpointer) — works on any backend; theversioning_nativecapability is removed.≤ X/ ageT+ orphan reconciliation) is P2, a single internal control-plane background process.DB schema
filesslimmed to identity + pointer:file_id, tenant_id, owner_kind, owner_id, name, gts_file_type, content_id, meta_version, timestamps(no bytes, no per-content fields).file_versions:(file_id, version_id, mime_type, size, hash_algorithm, hash_value, status pending|available, is_current, backend_id, backend_path, created_at).file_versions.Files & validation
ADR/0003-…sidecar-data-plane.md(new);ADR/0001→ superseded;ADR/0002references repointed to the sidecar.PRD.md,DESIGN.md,api.md,migration.sqlreworked.cpt validate: 0 errors, code coverage 104/104.Summary by CodeRabbit
Rangebehavior.v4.public) signed URL tokens delivered via query (fs-token) orX-FS-Token, with strict expiry and claim validation.