diff --git a/crates/core/src/api/request.rs b/crates/core/src/api/request.rs index a6ce3f9..e1ae372 100644 --- a/crates/core/src/api/request.rs +++ b/crates/core/src/api/request.rs @@ -127,6 +127,19 @@ fn parse_copy_source(raw: &str) -> Result<(String, String, Option), Prox Ok((bucket.to_string(), key, version)) } +/// Extract a non-empty `versionId` query parameter. +/// +/// An empty `?versionId=` is treated as absent rather than as a version named +/// "": S3 has no such version, and reading it as one would turn a malformed +/// request into a versioned read that can never succeed. +fn version_id(query_params: &[(String, String)]) -> Option { + query_params + .iter() + .find(|(k, _)| k == "versionId") + .map(|(_, v)| v.clone()) + .filter(|v| !v.is_empty()) +} + /// Build an [`S3Operation`] from an already-extracted bucket, key, and query. /// /// This is used by both [`parse_s3_request`] and custom resolvers that parse @@ -160,18 +173,18 @@ pub fn build_s3_operation( raw_query: query.map(|q| q.to_string()), }) } else { - // `?versionId=` is deliberately not carried here: the read - // path does not address versions, so the backend returns the - // current object and authorizing a version would describe a - // read that never happens. See `S3Operation::GetObject::version`. Ok(S3Operation::GetObject { bucket, key, - version: None, + version: version_id(&query_params), }) } } - Method::HEAD => Ok(S3Operation::HeadObject { bucket, key }), + Method::HEAD => Ok(S3Operation::HeadObject { + bucket, + key, + version: version_id(&query_params), + }), Method::PUT => { if let Some(upload_id) = upload_id { let part_number = query_params diff --git a/crates/core/src/backend/multipart.rs b/crates/core/src/backend/multipart.rs index f40ba09..a5a70fa 100644 --- a/crates/core/src/backend/multipart.rs +++ b/crates/core/src/backend/multipart.rs @@ -87,6 +87,20 @@ pub fn build_backend_url( url.push('?'); url.push_str(&qs); } + S3Operation::GetObject { + version: Some(version), + .. + } + | S3Operation::HeadObject { + version: Some(version), + .. + } => { + let qs = url::form_urlencoded::Serializer::new(String::new()) + .append_pair("versionId", version) + .finish(); + url.push('?'); + url.push_str(&qs); + } S3Operation::CompleteMultipartUpload { upload_id, .. } | S3Operation::AbortMultipartUpload { upload_id, .. } => { let qs = url::form_urlencoded::Serializer::new(String::new()) diff --git a/crates/core/src/proxy.rs b/crates/core/src/proxy.rs index 0ffb529..0cffc4f 100644 --- a/crates/core/src/proxy.rs +++ b/crates/core/src/proxy.rs @@ -101,6 +101,18 @@ pub const DEFAULT_USER_AGENT: &str = concat!("multistore/", env!("CARGO_PKG_VERS /// `if-match`/`if-none-match` don't apply to `UploadPart`, but clients never /// send them there, so sharing this list with the part-streaming caller is a /// no-op for parts. +/// Client headers forwarded on an object read (`GET`/`HEAD`). Forwarded +/// unsigned on both read paths — the presigned one cannot sign them at all, and +/// the versioned one deliberately does not (see +/// [`build_versioned_read_forward`](ProxyGateway::build_versioned_read_forward)). +const READ_FORWARD_HEADERS: &[&str] = &[ + "range", + "if-match", + "if-none-match", + "if-modified-since", + "if-unmodified-since", +]; + const AWS_CHUNKED_FORWARD_HEADERS: &[&str] = &[ "content-type", "content-encoding", @@ -905,40 +917,48 @@ where }; match operation { - S3Operation::GetObject { key, .. } => { + S3Operation::GetObject { key, version, .. } => { + if version.is_some() { + return Ok(HandlerAction::Forward(self.build_versioned_read_forward( + Method::GET, + bucket_config, + operation, + original_headers, + READ_FORWARD_HEADERS, + request_id, + )?)); + } let fwd = self .build_forward( Method::GET, bucket_config, key, original_headers, - &[ - "range", - "if-match", - "if-none-match", - "if-modified-since", - "if-unmodified-since", - ], + READ_FORWARD_HEADERS, request_id, ) .await?; tracing::debug!(path = fwd.url.path(), "GET via presigned URL"); Ok(HandlerAction::Forward(fwd)) } - S3Operation::HeadObject { key, .. } => { + S3Operation::HeadObject { key, version, .. } => { + if version.is_some() { + return Ok(HandlerAction::Forward(self.build_versioned_read_forward( + Method::HEAD, + bucket_config, + operation, + original_headers, + READ_FORWARD_HEADERS, + request_id, + )?)); + } let fwd = self .build_forward( Method::HEAD, bucket_config, key, original_headers, - &[ - "range", - "if-match", - "if-none-match", - "if-modified-since", - "if-unmodified-since", - ], + READ_FORWARD_HEADERS, request_id, ) .await?; @@ -1140,6 +1160,74 @@ where }) } + /// Build a [`ForwardRequest`] for a **version-scoped** read (`GET`/`HEAD` + /// carrying `?versionId=`), signed with headers rather than presigned. + /// + /// A presigned URL cannot carry the parameter: the query string is part of + /// a presigned request's canonical form, so `versionId` must be present + /// when the signature is computed — and the [`Signer`] interface presigns a + /// bare path with no query. Appending it afterwards invalidates the + /// signature. Signing the request with an `Authorization` header instead + /// puts `versionId` in the URL before signing, and the runtime still + /// streams the response body exactly as it does for a presigned forward. + /// + /// The client's read headers are attached *after* signing, so they travel + /// unsigned — matching the presigned path. That is deliberate: SigV4 only + /// requires `host` and `x-amz-*` to be signed, and a runtime that + /// normalizes a forwarded `Range` or conditional header (Cloudflare may) + /// would otherwise break a signature that covered it. + fn build_versioned_read_forward( + &self, + method: Method, + config: &BucketConfig, + operation: &S3Operation, + original_headers: &HeaderMap, + forward_header_names: &[&'static str], + request_id: &str, + ) -> Result { + // Object versioning is an S3 concept; the other backends model it + // differently and would silently ignore the parameter. `NotImplemented` + // rather than the `require_s3_backend` 400: the request is valid S3, it + // is this backend that cannot serve it. + if !config.is_s3_backend() { + return Err(ProxyError::NotImplemented(format!( + "version-scoped reads are not supported for '{}' backends", + config.backend_type + ))); + } + + let url_str = build_backend_url(config, operation)?; + + // Sign an otherwise-empty header map: the signer contributes `host`, + // `x-amz-date`, `x-amz-content-sha256` and `authorization`, and nothing + // else ends up in `SignedHeaders`. + let mut headers = HeaderMap::new(); + sign_s3_request( + &method, + &url_str, + &mut headers, + config, + hash_payload(&[]).as_str(), + )?; + + for name in forward_header_names { + if let Some(v) = original_headers.get(*name) { + headers.insert(*name, v.clone()); + } + } + headers.insert(http::header::USER_AGENT, self.user_agent.parse().unwrap()); + + let url = url::Url::parse(&url_str) + .map_err(|e| ProxyError::Internal(format!("invalid backend URL: {e}")))?; + tracing::debug!(path = url.path(), "versioned read via backend re-sign"); + Ok(ForwardRequest { + method, + url, + headers, + request_id: request_id.to_string(), + }) + } + /// Handle a body-bearing PUT (PutObject/UploadPart) whose body is /// `aws-chunked`: stream an unsigned-payload upload through after re-signing /// the seed (returns the `Forward`), reject a signed-chunk one, or return @@ -3704,24 +3792,195 @@ mod tests { }); } - /// A plain `GET` carrying `?versionId=` stays unversioned: the proxy does - /// not address versions on the read path, so authorizing it as versioned - /// would describe a read that never happens. + /// A `GET` carrying `?versionId=` now *is* a versioned read, so it parses + /// as one — the read path honors the parameter rather than silently + /// serving the current object, and the version must reach authorization. + /// (This inverts the assertion made when the read path ignored versions.) #[test] - fn plain_get_with_version_id_authorizes_an_unversioned_read() { + fn plain_get_with_version_id_parses_as_a_versioned_read() { let headers = HeaderMap::new(); - let op = crate::api::request::parse_s3_request( - &Method::GET, - "/b/obj.txt", - Some("versionId=v42"), - &headers, - crate::api::request::HostStyle::Path, - None, - ) - .unwrap(); - assert!( - matches!(op, S3Operation::GetObject { version: None, .. }), - "expected an unversioned GetObject, got {op:?}" - ); + let parse = |method: Method, query| { + crate::api::request::parse_s3_request( + &method, + "/b/obj.txt", + query, + &headers, + crate::api::request::HostStyle::Path, + None, + ) + .unwrap() + }; + + match parse(Method::GET, Some("versionId=v42")) { + S3Operation::GetObject { version, .. } => { + assert_eq!(version.as_deref(), Some("v42")) + } + other => panic!("expected GetObject, got {other:?}"), + } + match parse(Method::HEAD, Some("versionId=v42")) { + S3Operation::HeadObject { version, .. } => { + assert_eq!(version.as_deref(), Some("v42")) + } + other => panic!("expected HeadObject, got {other:?}"), + } + // No parameter, and a malformed empty one, both mean "current". + for query in [None, Some("versionId=")] { + match parse(Method::GET, query) { + S3Operation::GetObject { version, .. } => assert_eq!(version, None), + other => panic!("expected GetObject, got {other:?}"), + } + } + } + + /// A versioned read is signed with an `Authorization` header and carries + /// `versionId` in the backend URL. A presigned URL could not express this: + /// the query is part of its canonical form, so appending the parameter + /// afterwards would invalidate the signature. + #[test] + fn versioned_get_forwards_a_header_signed_url_carrying_the_version() { + run(async { + let gw = gateway(); + let headers = HeaderMap::new(); + let action = gw + .resolve_request( + Method::GET, + "/test-bucket/obj.txt", + Some("versionId=v42"), + &headers, + None, + ) + .await; + let fwd = match action { + HandlerAction::Forward(fwd) => fwd, + other => panic!("expected Forward, got {:?}", std::mem::discriminant(&other)), + }; + assert_eq!(fwd.url.query(), Some("versionId=v42")); + let auth = fwd + .headers + .get("authorization") + .expect("versioned read must be header-signed") + .to_str() + .unwrap() + .to_string(); + assert!(auth.starts_with("AWS4-HMAC-SHA256 Credential=")); + // The signature must not be in the URL as well. + assert!(!fwd.url.as_str().contains("X-Amz-Signature")); + }); + } + + /// An unversioned read keeps the presigned path untouched. + #[test] + fn unversioned_get_still_uses_a_presigned_url() { + run(async { + let gw = gateway(); + let headers = HeaderMap::new(); + let action = gw + .resolve_request(Method::GET, "/test-bucket/obj.txt", None, &headers, None) + .await; + let fwd = match action { + HandlerAction::Forward(fwd) => fwd, + other => panic!("expected Forward, got {:?}", std::mem::discriminant(&other)), + }; + assert!(fwd.url.as_str().contains("X-Amz-Signature")); + assert!(fwd.headers.get("authorization").is_none()); + }); + } + + /// Read headers ride along on a versioned read, but stay *out* of + /// `SignedHeaders`: a runtime that normalizes a forwarded `Range` would + /// otherwise break a signature that covered it. + #[test] + fn versioned_read_forwards_range_unsigned() { + run(async { + let gw = gateway(); + let mut headers = HeaderMap::new(); + headers.insert("range", "bytes=0-99".parse().unwrap()); + headers.insert("if-none-match", "\"etag\"".parse().unwrap()); + let action = gw + .resolve_request( + Method::GET, + "/test-bucket/obj.txt", + Some("versionId=v42"), + &headers, + None, + ) + .await; + let fwd = match action { + HandlerAction::Forward(fwd) => fwd, + other => panic!("expected Forward, got {:?}", std::mem::discriminant(&other)), + }; + assert_eq!(fwd.headers.get("range").unwrap(), "bytes=0-99"); + assert_eq!(fwd.headers.get("if-none-match").unwrap(), "\"etag\""); + let auth = fwd.headers.get("authorization").unwrap().to_str().unwrap(); + assert!( + !auth.contains("range") && !auth.contains("if-none-match"), + "read headers must not be signed: {auth}" + ); + // A ranged read must still bypass the full-object cache. + assert!(fwd.should_bypass_cache()); + }); + } + + /// Versioning is an S3 concept. A versioned read of a non-S3 backend is + /// rejected rather than silently served as the current object. + #[test] + fn versioned_read_of_a_non_s3_backend_is_rejected() { + run(async { + let gw = gateway(); + let headers = HeaderMap::new(); + let action = gw + .resolve_request( + Method::GET, + "/azure-bucket/obj.txt", + Some("versionId=v42"), + &headers, + None, + ) + .await; + match action { + HandlerAction::Response(resp) => assert_eq!(resp.status, 501), + other => panic!( + "expected 501 Response, got {:?}", + std::mem::discriminant(&other) + ), + } + }); + } + + /// The version reaches the registry on a plain versioned `GET`, so a + /// version-aware policy can refuse it — the same guarantee a versioned + /// copy-source has. + #[test] + fn versioned_get_is_authorized_against_its_version() { + run(async { + let gw = ProxyGateway::new( + MockBackend, + RecordingRegistry { + seen: Arc::new(std::sync::Mutex::new(Vec::new())), + versions: Arc::new(std::sync::Mutex::new(Vec::new())), + deny: None, + deny_versioned_reads: true, + }, + MockCreds, + None, + ); + let headers = HeaderMap::new(); + let action = gw + .resolve_request( + Method::GET, + "/test-bucket/obj.txt", + Some("versionId=v42"), + &headers, + None, + ) + .await; + match action { + HandlerAction::Response(resp) => assert_eq!(resp.status, 403), + other => panic!( + "expected 403 Response, got {:?}", + std::mem::discriminant(&other) + ), + } + }); } } diff --git a/crates/core/src/types.rs b/crates/core/src/types.rs index 1c82624..f15967a 100644 --- a/crates/core/src/types.rs +++ b/crates/core/src/types.rs @@ -378,6 +378,9 @@ pub enum S3Operation { HeadObject { bucket: String, key: String, + /// The object version this read will return, or `None` for the current + /// version. Same contract as [`GetObject::version`](Self::GetObject). + version: Option, }, PutObject { bucket: String, diff --git a/docs/reference/operations.md b/docs/reference/operations.md index f1c6aef..0eebe3c 100644 --- a/docs/reference/operations.md +++ b/docs/reference/operations.md @@ -4,8 +4,8 @@ | Operation | HTTP Method | Dispatch | Description | |-----------|------------|----------|-------------| -| GetObject | `GET /{bucket}/{key}` | Forward | Download a file | -| HeadObject | `HEAD /{bucket}/{key}` | Forward | Get file metadata | +| GetObject | `GET /{bucket}/{key}[?versionId=ID]` | Forward | Download a file (optionally a specific version) | +| HeadObject | `HEAD /{bucket}/{key}[?versionId=ID]` | Forward | Get file metadata (optionally for a specific version) | | PutObject | `PUT /{bucket}/{key}` | Forward | Upload a file | | CopyObject | `PUT /{bucket}/{key}` + `x-amz-copy-source` | Response | Server-side copy within one S3 endpoint | | DeleteObject | `DELETE /{bucket}/{key}` | Forward | Delete a file | @@ -35,7 +35,7 @@ Credentials are *not* compared between the two ends. The copy is signed with the destination's credentials and the source contributes only its bucket name and prefix, so the source's own credentials are never used; whether the destination's credentials may read the source is an IAM question S3 answers with `AccessDenied`. (Comparing them would also be meaningless: a credential-injecting middleware such as `AwsBackendAuth` runs only against the destination config, so a source resolved from the same `auth_type=oidc` connection could never match.) The caller's own authorization is unaffected — the source is separately authorized as a read. -**Versioned sources are authorized as versioned reads.** An `x-amz-copy-source` may carry `?versionId=`, and that version is copied — bytes the read path cannot otherwise reach, since a plain `GET` ignores `?versionId=` and serves the current object. The version is therefore carried on the synthetic `GetObject` used to authorize the source (`S3Operation::GetObject::version`), which makes the authorization action `get_object_version` rather than `get_object` — mirroring S3's own split between `s3:GetObject` and `s3:GetObjectVersion`. This is not merely advice to registry implementors: the bundled policy (`auth::authorize`, which `multistore-static-config` calls) enforces it, because a version-scoped read resolves to its own [action](../configuration/roles.md). A caller granted `get_object` over a prefix therefore cannot read — or copy out — an older version of an object in it, and an anonymous caller never can; the scope must list `get_object_version` explicitly. Prefix-scoped policy cannot express "may read version X", so the default is to deny rather than silently permit. A registry with its own policy should draw the same distinction. A plain `GET` leaves the field `None` even when the client sends `?versionId=` — authorizing it as versioned would describe a read that never happens. +**Versioned sources are authorized as versioned reads.** An `x-amz-copy-source` may carry `?versionId=`, and that version is copied. The version is carried on the synthetic `GetObject` used to authorize the source (`S3Operation::GetObject::version`), which makes the authorization action `get_object_version` rather than `get_object` — mirroring S3's own split between `s3:GetObject` and `s3:GetObjectVersion`. This is not merely advice to registry implementors: the bundled policy (`auth::authorize`, which `multistore-static-config` calls) enforces it, because a version-scoped read resolves to its own [action](../configuration/roles.md). A caller granted `get_object` over a prefix therefore cannot read — or copy out — an older version of an object in it, and an anonymous caller never can; the scope must list `get_object_version` explicitly. Prefix-scoped policy cannot express "may read version X", so the default is to deny rather than silently permit. A registry with its own policy should draw the same distinction. The same field carries a plain read's version — see "Version-scoped reads" below. **Path-mapped deployments must map the copy-source.** `CopyObject` names its source in a header, so a proxy that rewrites client paths onto internal bucket names must rewrite the copy-source too — otherwise the source resolves as a client-facing name the registry has never heard of and every copy fails with `404 NoSuchBucket`. The client signed that header, so it must not be mutated; pass the mapped value alongside it via `RequestInfo::with_copy_source`, and signature verification keeps using the header as sent. `PathMapping::rewrite_copy_source` produces the mapped value: @@ -50,6 +50,16 @@ let req = RequestInfo::new(&method, &rewrite.path, query, &headers, None) .with_copy_source(mapped.as_deref()); ``` +### Version-scoped reads + +`GET` and `HEAD` honor `?versionId=`, returning that version rather than the current object. The version is carried on the operation (`version: Option`, `None` meaning current), so it reaches the registry and authorization sees the version that will actually be read. + +Such a read authorizes as **`get_object_version`**, not `get_object` — a caller granted ordinary read access on a prefix cannot reach previous versions of objects in it without that action, and anonymous callers never can. See "Server-side copy" above and the action table in [roles](../configuration/roles.md). + +Such a read is **signed with an `Authorization` header** rather than presigned. The query string is part of a presigned request's canonical form, so `versionId` has to be present when the signature is computed — and the `Signer` interface presigns a bare path with no query, so appending it afterwards would invalidate the signature. The runtime streams the response identically either way. Client read headers (`Range`, `If-*`) are attached after signing and travel unsigned, matching the presigned path: SigV4 only requires `host` and `x-amz-*` to be signed, and a runtime that normalizes a forwarded `Range` would otherwise break a signature covering it. + +An empty `?versionId=` is treated as absent. Object versioning is an S3 concept, so a version-scoped read of a non-S3 backend returns `501 NotImplemented` rather than silently serving the current object. + ### Writes and request headers `PutObject` forwards the request body plus standard HTTP entity headers (`Content-Type`, `Content-Disposition`, `Content-Encoding`, `Content-Language`, `Cache-Control`, `Expires`, `Content-MD5`) and the conditional-write preconditions (`If-Match`, `If-None-Match`) to a presigned URL. S3 applies all of these even though they are not part of the (host-only) presigned signature. `x-amz-*` headers (user metadata `x-amz-meta-*`, storage class, tagging, ACLs, SSE, checksum headers such as `x-amz-checksum-*`, and the `x-amz-copy-source-if-*` copy preconditions) are **not** forwarded: S3 rejects unsigned `x-amz-*` headers on presigned requests, and the proxy presigns over `host` only. Supporting those headers requires a header-signing forward path — see the design note in `.plans/`. @@ -87,7 +97,7 @@ Handled by OIDC discovery closures (registered on the `Router` via `OidcRouterEx > - **Server-side copy is same-endpoint only** — `CopyObject` works when source and destination resolve to the same S3 endpoint and region (see "Server-side copy" above). A copy across endpoints, or `UploadPartCopy`, is rejected with `501 NotImplemented`. > - **Path-mapped proxies must map the copy-source** — `CopyObject` names its source in a header, which no URL rewrite touches. Pass the mapped value via `RequestInfo::with_copy_source` or every copy fails with `404 NoSuchBucket` (see "Server-side copy" above). > - **`x-amz-*` write headers are dropped** — Object metadata, storage class, tagging, ACLs, SSE, and checksum headers on writes are not forwarded (see "Writes and request headers" above). -> - **Versioned/MFA delete is not handled** — A `?versionId=` on a delete is ignored; the current object version is deleted. +> - **Versioned/MFA delete is not handled** — A `?versionId=` on a delete is ignored; the current object version is deleted. Reads (`GET`/`HEAD`) do honor it — see "Version-scoped reads" above. > - **Degenerate object keys are rejected** — Keys containing empty, `.`, or `..` path segments (including leading/trailing slashes, e.g. `dir/` folder markers), or ASCII control characters, return `400 InvalidRequest` on every keyed operation. Real S3 accepts such keys; the proxy is deliberately stricter because they cannot be addressed consistently across its presigned and raw-signed backend paths. Batch-delete body keys are exempt, as the remediation route for legacy keys already stored under such names. > - **Keys are otherwise byte-faithful** — All other keys (including `*`, `%`, `~`, `#`, unicode) are stored on the backend exactly as sent. Objects written through versions **before 0.6.4** via single presigned PUT with characters in object_store's rewrite set (`*`, `%`, `~`, `#`, `?`, `[`, `]`, ...) were silently stored under percent-encoded names (`a*.bin` as `a%2A.bin`); they remain addressable only by that literal mangled name and need a one-time rename to recover their logical keys.