diff --git a/.github/workflows/staging.yml b/.github/workflows/staging.yml index cd1e7c0..835fbc6 100644 --- a/.github/workflows/staging.yml +++ b/.github/workflows/staging.yml @@ -47,9 +47,37 @@ jobs: if: vars.FEDERATION_TEST_ACCOUNT != '' && vars.FEDERATION_TEST_PRODUCT != '' && vars.FEDERATION_TEST_KEY != '' runs-on: ubuntu-latest timeout-minutes: 10 + permissions: + contents: read + # Mints the caller identity for the copy-source authz test; see below. + id-token: write steps: - uses: actions/checkout@v4 - uses: astral-sh/setup-uv@v5 + - name: Mint caller identity token (GitHub OIDC) + # A GitHub Actions OIDC token, minted per run — short-lived by design + # and never stored, unlike a token parked in a repo secret. + # + # Dormant until Source registers GitHub as a valid IdP, so that products + # can accept writes from GitHub Actions. Two things must land first: + # the deployment's AUTH_ISSUER must accept GitHub's issuer (today it is + # a single Ory URL — src/config.rs reads AUTH_ISSUER as one String, + # unlike the comma-separated AUTH_AUDIENCE, so this needs a code change + # too), and the audience Source expects must be set as + # FEDERATION_TEST_AUDIENCE. Until then no token is minted and the + # copy-source authz test skips; the rest of the suite is unaffected. + if: vars.FEDERATION_TEST_AUDIENCE != '' + run: | + set -euo pipefail + token=$(curl -sSf -H "Authorization: bearer $ACTIONS_ID_TOKEN_REQUEST_TOKEN" \ + "$ACTIONS_ID_TOKEN_REQUEST_URL&audience=${{ vars.FEDERATION_TEST_AUDIENCE }}" \ + | jq -r '.value') + if [ -z "$token" ] || [ "$token" = "null" ]; then + echo "::error::FEDERATION_TEST_AUDIENCE is set but minting returned no token" + exit 1 + fi + echo "::add-mask::$token" + echo "CI_WRITE_ID_TOKEN=$token" >> "$GITHUB_ENV" - name: Run federation smoke tests env: PROXY_URL: ${{ needs.deploy.outputs.deploy_url }} @@ -57,4 +85,9 @@ jobs: FEDERATION_TEST_PRODUCT: ${{ vars.FEDERATION_TEST_PRODUCT }} FEDERATION_TEST_KEY: ${{ vars.FEDERATION_TEST_KEY }} FEDERATION_RESTRICTED_PRODUCT: ${{ vars.FEDERATION_RESTRICTED_PRODUCT }} - run: uvx --with requests pytest tests/test_federation.py -v + FEDERATION_WRITE_PRODUCT: ${{ vars.FEDERATION_WRITE_PRODUCT }} + # Set by the mint step above, and only when it runs. + CI_WRITE_ID_TOKEN: ${{ env.CI_WRITE_ID_TOKEN }} + # boto3: the copy-source authz test signs SigV4 through the AWS SDK + # rather than hand-rolling requests. + run: uvx --with requests --with boto3 pytest tests/test_federation.py -v diff --git a/Cargo.lock b/Cargo.lock index c6cd587..a056ce4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1034,9 +1034,9 @@ dependencies = [ [[package]] name = "multistore" -version = "0.7.0" +version = "0.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "10553bb6d41a7caf40663151d2ebb73f0032bed3474031bb69c81ce8a0a46b39" +checksum = "f2ea59297a4aac2cbea49e6e55320f6cc5d974cd167337afc9dfcc1b386bd3f4" dependencies = [ "async-trait", "base64", @@ -1063,9 +1063,9 @@ dependencies = [ [[package]] name = "multistore-cf-workers" -version = "0.7.0" +version = "0.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "89cb288f2d08ba9fa1b1970e3323266da7cee51dea786d3be6c73cbb85ebeac6" +checksum = "b23afaa6c9ecc498314858a3a2c2600b4880ffa8c4f0f3339cb2a86c8b70e857" dependencies = [ "async-trait", "bytes", @@ -1090,9 +1090,9 @@ dependencies = [ [[package]] name = "multistore-oidc-provider" -version = "0.7.0" +version = "0.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b743bc10e0fdfb5c70f9f302e0bf756714b7e09cfd41d837918425db3c6b36eb" +checksum = "00a3ee043aa16fbbca9b324e001865d9d7e3c1fb55041106cf8d8c52b452f95d" dependencies = [ "base64", "chrono", @@ -1110,9 +1110,9 @@ dependencies = [ [[package]] name = "multistore-path-mapping" -version = "0.7.0" +version = "0.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "17ce2241c2cdd6bbbf239498ec7caa9a4488b28c159f58288261435b4f090975" +checksum = "b7fd87532e2f48fa4496597012735c05dd4cdfcb3e9517d8c5cfa89e5cd20e4e" dependencies = [ "multistore", "percent-encoding", @@ -1121,9 +1121,9 @@ dependencies = [ [[package]] name = "multistore-sts" -version = "0.7.0" +version = "0.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ee8407b2bc78eeadd420fc6c952cc9c6ac5f51c7e52643417d3ea048859cdad2" +checksum = "3e89dd8f5e7755359eaf9d9ba94a30ed69cc2a5b067ccc53c61a8a28b10fd044" dependencies = [ "aes-gcm", "base64", diff --git a/Cargo.toml b/Cargo.toml index cd16c3c..9f29b5a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,10 +36,10 @@ path = "tests/fixtures.rs" [dependencies] # Multistore -multistore = { version = "0.7.0", features = ["azure", "gcp"] } -multistore-oidc-provider = "0.7.0" -multistore-path-mapping = "0.7.0" -multistore-sts = "0.7.0" +multistore = { version = "0.7.1", features = ["azure", "gcp"] } +multistore-oidc-provider = "0.7.1" +multistore-path-mapping = "0.7.1" +multistore-sts = "0.7.1" # Serialization serde = { version = "1", features = ["derive"] } @@ -67,7 +67,7 @@ getrandom = { version = "0.4", features = ["wasm_js"] } # ring is transitive (via object_store); this direct entry only turns the # feature on for the wasm target. ring = { version = "0.17", features = ["wasm32_unknown_unknown_js"] } -multistore-cf-workers = { version = "0.7.0", features = ["azure", "gcp"] } +multistore-cf-workers = { version = "0.7.1", features = ["azure", "gcp"] } # On wasm32-unknown-unknown reqwest selects its browser `fetch` backend by # target arch (not a cargo feature), so the default TLS/backend features are # unnecessary here. The `form` feature gates `.form()`, which the STS @@ -86,3 +86,4 @@ web-sys = { version = "0.3", features = [ ] } worker = { version = "=0.7.5", features = ["http"] } worker-macros = { version = "=0.7.5", features = ["http"] } + diff --git a/src/lib.rs b/src/lib.rs index 05eb846..5ba2f4b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -35,7 +35,7 @@ use multistore_oidc_provider::{HttpExchange, OidcCredentialProvider, OidcProvide use multistore_path_mapping::{MappedRegistry, PathMapping}; use multistore_sts::jwks::JwksCache; use multistore_sts::route_handler::StsRouterExt; -use object_path::{extract_path_segments, is_keyless_write}; +use object_path::{extract_path_segments, is_keyless_write, mapped_copy_source}; use std::sync::OnceLock; use sts::StsCredentialRegistry; use worker::{event, Context, Env, Result}; @@ -316,6 +316,11 @@ async fn fetch(req: web_sys::Request, env: Env, ctx: Context) -> Result Result (Option<&str>, Option<&str>, (account, product, key) } +/// Map an inbound `x-amz-copy-source` header into the registry's namespace. +/// +/// `CopyObject` names its source in a header rather than the URL, in client +/// coordinates (`/{account}/{product}/{key}`). The registry only knows mapped +/// bucket names (`account:product`), so an unmapped source resolves to a bucket +/// it has never heard of and the copy fails with 404 NoSuchBucket. The client +/// signed the header, so it must not be mutated — the mapped value produced +/// here is passed alongside it via `RequestInfo::with_copy_source`, and +/// signature verification keeps using the header as sent. +/// +/// Returns `None` when there is no copy-source header (the request isn't a +/// copy) or when the value can't be mapped — too few segments to name an +/// object, or not valid UTF-8. `None` leaves multistore reading the header +/// as-is, which is the correct fallback: a non-copy request has nothing to map, +/// and an unmappable source should fail on its own merits rather than on a +/// silently-invented bucket name. +pub(crate) fn mapped_copy_source( + headers: &http::HeaderMap, + mapping: &multistore_path_mapping::PathMapping, +) -> Option { + headers + .get("x-amz-copy-source") + .and_then(|v| v.to_str().ok()) + .and_then(|v| mapping.rewrite_copy_source(v)) +} + /// Whether `method` writes to a single object but `path` carries no object key. /// /// `PUT`/`DELETE` (PutObject/DeleteObject) address one object, so they need a diff --git a/tests/object_path.rs b/tests/object_path.rs index 58f32b6..59ab34f 100644 --- a/tests/object_path.rs +++ b/tests/object_path.rs @@ -6,7 +6,81 @@ mod object_path; use http::Method; -use object_path::{extract_path_segments, is_keyless_write}; +use object_path::{extract_path_segments, is_keyless_write, mapped_copy_source}; + +/// The deployment's real mapping: `/{account}/{product}/{key}` folds the first +/// two segments into the internal bucket `account:product`. +fn mapping() -> multistore_path_mapping::PathMapping { + multistore_path_mapping::PathMapping { + bucket_segments: 2, + bucket_separator: ":".to_string(), + display_bucket_segments: 1, + } +} + +fn headers(copy_source: Option<&str>) -> http::HeaderMap { + let mut h = http::HeaderMap::new(); + if let Some(v) = copy_source { + h.insert("x-amz-copy-source", v.parse().unwrap()); + } + h +} + +/// The whole point: a client-coordinate copy source (`/account/product/key`) +/// becomes a registry-coordinate one (`account:product/key`). Without this the +/// source names a bucket the registry has never heard of and the copy 404s. +#[test] +fn copy_source_is_folded_into_the_internal_bucket_name() { + assert_eq!( + mapped_copy_source(&headers(Some("/acct/prod/README.md")), &mapping()), + Some("/acct:prod/README.md".to_string()) + ); + // Leading slash is optional in `x-amz-copy-source`; both forms map alike. + assert_eq!( + mapped_copy_source(&headers(Some("acct/prod/README.md")), &mapping()), + Some("/acct:prod/README.md".to_string()) + ); + // Nested keys keep every segment past the bucket. + assert_eq!( + mapped_copy_source(&headers(Some("/acct/prod/a/b/c.txt")), &mapping()), + Some("/acct:prod/a/b/c.txt".to_string()) + ); +} + +/// `versionId` rides along untouched — multistore#129 authorizes the source +/// against that version, so losing it here would silently copy the wrong one. +/// Percent-encoding is likewise preserved rather than decoded. +#[test] +fn copy_source_preserves_version_and_encoding() { + assert_eq!( + mapped_copy_source( + &headers(Some("/acct/prod/a%20b.txt?versionId=v42")), + &mapping() + ), + Some("/acct:prod/a%20b.txt?versionId=v42".to_string()) + ); +} + +/// No header means the request isn't a copy: nothing to map, and multistore +/// must be left reading the (absent) header itself rather than handed a value. +#[test] +fn absent_copy_source_maps_to_none() { + assert_eq!(mapped_copy_source(&headers(None), &mapping()), None); +} + +/// Too few segments to name an object inside a product. Mapping these would +/// invent a bucket name; `None` lets them fail on their own merits instead. +#[test] +fn unmappable_copy_source_maps_to_none() { + assert_eq!( + mapped_copy_source(&headers(Some("/acct/prod")), &mapping()), + None + ); + assert_eq!( + mapped_copy_source(&headers(Some("/acct")), &mapping()), + None + ); +} #[test] fn extract_splits_account_product_key() { diff --git a/tests/test_federation.py b/tests/test_federation.py index ae97c21..fff5ad2 100644 --- a/tests/test_federation.py +++ b/tests/test_federation.py @@ -24,6 +24,25 @@ same account, used by the authz/confused-deputy test (an anonymous caller must be denied before federation reads its private backend) + FEDERATION_WRITE_PRODUCT product id, in the same account, that the caller + identified by CI_WRITE_ID_TOKEN may *write*. Used + only by the copy-source authorization test + CI_WRITE_ID_TOKEN that caller's identity token — a GitHub Actions + OIDC token, minted per run by staging.yml (short- + lived by design, never stored) once + FEDERATION_TEST_AUDIENCE is set. + + Dormant until Source registers GitHub as a valid + IdP so products can accept writes from GitHub + Actions. That needs the deployment's AUTH_ISSUER to + accept GitHub's issuer — today it is a single Ory + URL, and ``src/config.rs`` reads AUTH_ISSUER as one + String (unlike the comma-separated AUTH_AUDIENCE), + so it is a code change as well as config — and the + audience Source expects to be set as + FEDERATION_TEST_AUDIENCE. Until then this test + skips. The caller must also hold write on + FEDERATION_WRITE_PRODUCT """ import os @@ -36,6 +55,8 @@ PRODUCT = os.environ.get("FEDERATION_TEST_PRODUCT") KEY = os.environ.get("FEDERATION_TEST_KEY") RESTRICTED_PRODUCT = os.environ.get("FEDERATION_RESTRICTED_PRODUCT") +WRITE_PRODUCT = os.environ.get("FEDERATION_WRITE_PRODUCT") +ID_TOKEN = os.environ.get("CI_WRITE_ID_TOKEN") @pytest.mark.skipif( @@ -86,3 +107,49 @@ def test_restricted_product_denied_to_anonymous(): "anonymous caller was not denied a restricted federated product " f"(status {resp.status_code}); federation may have served private data" ) + + +@pytest.mark.skipif( + not (ACCOUNT and RESTRICTED_PRODUCT and WRITE_PRODUCT and ID_TOKEN), + reason=( + "copy-source authz target not configured (set FEDERATION_TEST_ACCOUNT/" + "FEDERATION_RESTRICTED_PRODUCT/FEDERATION_WRITE_PRODUCT and an " + "issuer-appropriate CI_WRITE_ID_TOKEN against a deployed proxy)" + ), +) +def test_copy_source_authorization_is_enforced(): + """The confused-deputy guard for `CopyObject`: holding write on the + destination must not let a caller read a product they aren't entitled to. + + A copy is authorized in two halves — the destination as a write, the source + as a synthetic `GetObject`. This names a source the caller cannot read and a + destination they can write, so only the source half can deny it. A success + would mean CopyObject is a hole around product authorization: read any + restricted product by copying it somewhere readable. + + Deliberately here and not in the CI integration tier, where it could not + fail: federation is attempted against the destination before the source is + resolved, so on CI's throwaway signing key every copy dies at federation + first — and `AccessDenied` is itself one of the federation error codes, so + the assertion below would pass without proving anything. It needs a + deployment where the destination write genuinely succeeds, which is what + this suite provides. + """ + import botocore.exceptions + + from test_writes import s3_client # /.sts exchange + SigV4, see its module docstring + + client = s3_client(retries={"max_attempts": 1}) + # A success raises nothing and fails here as DID NOT RAISE — the outcome + # this test exists to catch. + with pytest.raises(botocore.exceptions.ClientError) as exc: + client.copy_object( + Bucket=ACCOUNT, + Key=f"{WRITE_PRODUCT}/copy-authz-probe.txt", + CopySource=f"{ACCOUNT}/{RESTRICTED_PRODUCT}/{KEY or 'any-key'}", + ) + status = exc.value.response["ResponseMetadata"]["HTTPStatusCode"] + assert status in (401, 403, 404), ( + f"copy from a restricted source was not denied (status {status}); " + "CopyObject may be bypassing source authorization" + ) diff --git a/tests/test_writes.py b/tests/test_writes.py index 9dd8c16..0122ec3 100644 --- a/tests/test_writes.py +++ b/tests/test_writes.py @@ -342,6 +342,50 @@ def test_federated_write_fails_closed(op): ) +def test_anonymous_copy_is_denied(): + """A copy is a write, so it dies at the authz gate without credentials — + before any source resolution or backend call.""" + resp = requests.put( + f"{PROXY_URL}/{WRITE_ACCOUNT}/{WRITE_PRODUCT}/copy-dest.txt", + headers={ + "x-amz-copy-source": f"/{WRITE_ACCOUNT}/{WRITE_PRODUCT}/{HIVE_KEY_DIR}/src.pmtiles" + }, + ) + assert resp.status_code == 403 + + +@needs_token +def test_copy_reaches_the_federation_seam(): + """A credentialed CopyObject is parsed and dispatched as a copy, and fails + closed at federation like every other write — a bounded, parseable S3 + error rather than a hang or a silent success. + + Scope, measured rather than assumed: this does NOT pin the copy-source + path mapping. Federation is attempted against the destination before the + source is resolved, so a copy fails identically here whether or not + `RequestInfo::with_copy_source` is wired up — verified by deleting the + wiring and watching this test still pass. The mapping's real regression + test is native: `copy_source_is_folded_into_the_internal_bucket_name` in + tests/object_path.rs, which does fail without it.""" + import botocore.exceptions + + client = s3_client(retries={"max_attempts": 1}) + with pytest.raises(botocore.exceptions.ClientError) as exc: + client.copy_object( + Bucket=WRITE_ACCOUNT, + Key=f"{WRITE_PRODUCT}/{HIVE_KEY_DIR}/copy-dest.pmtiles", + CopySource=f"{WRITE_ACCOUNT}/{WRITE_PRODUCT}/{HIVE_KEY_DIR}/copy-src.pmtiles", + ) + code = exc.value.response["Error"].get("Code") + assert code, "unparseable error response" + # NotImplemented would mean the copy was rejected as a cross-store copy; + # SignatureDoesNotMatch would mean the client-signed x-amz-copy-source + # header got mutated instead of the mapped value being passed alongside it. + assert code not in {"NotImplemented", "SignatureDoesNotMatch", "InvalidRequest"}, ( + f"{code}: rejected before the federation seam this test pins" + ) + + @needs_token def test_corrupted_session_token_rejected(): """A SigV4 request whose sealed SessionToken has been tampered with must