Repository navigation
fix(sig): refuse SigV2 on the STS form and zip download floors - #1603
Merged
Merged
Conversation
Add OperationFloor::refuse_sigv2_under_every_policy, the narrower sibling of require_sigv4_under_every_policy: every SigV2 shape is refused AccessDenied at the floor, before its credential is parsed or resolved, while SigV4, anonymous, bearer and custom-scheme admissions are unchanged. SigV2 covers neither the form body that carries every STS parameter nor most of the query, and RustFS's router refuses it on these routes. Apply it to rustfs:StsFormPost and admin::zip_token_floor, record the decision in ADR-0050, and turn the goldens' SigV2 STS positives into refusals, with the legacy stand-in now modelling RustFS's SigV2 refusal. Fixes #1592
Collaborator
Author
Role findings (gateway-adversarial, single session, author-run)I ran these passes myself in the authoring session. They are advisory and do not replace the independent security review this PR needs before merge. simplicity-adversary: no break found. Attacked:
security-adversary: no break found. Attacked:
test-adversary: no break found. Attacked:
What I would attack first as an independent reviewer:
|
Takes #1600 (ADR-0050, the metadata listings). This branch's ADR becomes ADR-0051 (file, title, index row and every reference of this branch; #1600's own ADR-0050 references are untouched). Extends the SigV2 refusal to rustfs:ListObjectsV2WithMetadata and rustfs:ListObjectVersionsWithMetadata through admin::listing_floor and the generator's floor text. SigV2 does not sign `metadata`, so an appended metadata=true turned a SigV2 listing into a user-metadata disclosure. SigV4 and unsigned listings stay exactly as on the standard listings, which keep admitting SigV2. This is a deliberate difference from RustFS, which serves the SigV2 form (recorded in ADR-0051). Tests flipped deliberately: - metadata_listing_tests: the three tests that pinned SigV2 presigned, appended-presigned and appended-header listings as admitted are now refusals (403 AccessDenied, no authorizer question) on both floors, plus a correctly signed SigV2 metadata listing, also refused; - dialect metadata_listing: the floor equals the standard listing floor except refuses_sigv2, which the standard floors do not; - #1583's SigV2 census includes the zip rows (670-8), its control keeps only the OIDC row, and the dialect census record becomes the_zip_download_refuses_sigv2_without_requiring_sigv4. Also: the sig test-target guard lists sigv2_refused_floor. This fixes both CI failures on 88147f9 (Static checks; Guard self-test 5, "fails on the current tree"). The redundant zip assertions are dropped from extensions.rs to keep it within 800 lines; census.rs keeps them. Mutations, all run locally, each reverted, all red: M1 floor ignores the rule; M2 predicate loses the SigV4 implication; M3 setter also requires SigV4; M4 refusal after the credential parse; M5 STS floor without the rule; M6 zip floor without the rule (census included); M7 legacy stand-in admits SigV2; M8 deaf credential recorder; M9 no-op unknown-key rewrite; M10 and M11 both registry directions; M12 the rule refuses every family; M13 predicate always true; M14 metadata listing floor without the rule.
Address the independent security review of 828818f, which found no bypass: - ADR-0052: the mutation table is in #1592's Handoff comment, not in a pull request. "Always 403 AccessDenied" is corrected in the ADR and the rustdoc: the floor's H6 and H4 rules, and the RustFS profile's header-signature view policy (a missing or unreadable SigV2 date answers 400 InvalidRequest, a skewed one 403 RequestTimeTooSkewed), answer first, and none of them reads the key or the signature. - sec1_25_v2_header sends a Web Identity body, so a floor that read the filtered view would answer 200. sec1_10 asserts that no authorizer question was asked, and its doc and sec1_24_v2_query's now say what they pin since the floor refuses first. sec1_16/17 state their shared property, and ADR-0044's claimed-route SigV2 canonical resource is pinned on an OIDC row on a virtual host under RustFS addressing (sigv2_census_tests). - metadata_listing_tests: a standard-listing control shows the file's SigV2 signers produce requests served as the signer. The "versions added" and "forged" rows move to the standard listings, where SignatureDoesNotMatch can still be observed. - The review's odd spellings are pinned to "no principal reaches the handler": - %53ignature and every name encoded: through the service the query reader answers 400 first, and the sig floor test shows the floor refuses them as SigV2; - AWSAccessKeyId alone: anonymous with no principal under the profile, refused under the default floor; - a lowercase `aws` scheme: refused, no principal; - the zip SigV2 header with a token (verifier never asked) was already pinned. - A new test sends a bad SigV2 date through the STS assembly, which has the view policy on, and checks known and unknown keys get the same answer. Mutations, all local, each reverted: M15-M22 (see the Handoff comment on #1592 for the table).
Takes #1605, a one-line rustdoc link fix in crates/gateway/src/close.rs that touches none of this branch's files.
overtrue
marked this pull request as ready for review
November 11, 2026 11:04
overtrue
added a commit
that referenced
this pull request
Nov 11, 2026
main gained ADR-0052 with #1603, so the record of this change moves to the next free number: its file, title, index row and every reference to it. The sig test-target inventory keeps both sides' modules.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1592
What this changes
A new floor rule,
OperationFloor::refuse_sigv2_under_every_policy, refuses every SigV2 request — header, presigned URL or POST policy — under everySigV2Policy. The refusal is403 AccessDeniedat the floor's allow-list step (H3), before the SigV2 credential is parsed and before any credential lookup. It is the narrower sibling ofrequire_sigv4_under_every_policy(ADR-0049): SigV4 and requests without an AWS credential (anonymous, bearer token, custom scheme) are decided exactly as before.It is applied to:
rustfs:StsFormPost(generator templatextask/src/rustfs_admin_dialect/form.rs, regeneratedops/sts_form_post.rs). The floor staysbuiltin(Sts)with anonymous allowed, so unsigned Web Identity still reaches the handler, and SigV4AssumeRoleis unchanged.admin::zip_token_floor. The rule composes with the bearer claim, so the difference recorded on rustfs:StsFormPost admits SigV2 where RustFS refuses it #1592 (a SigV2 header without a token passed the floor and was refused later) is closed: the SigV2 header is refused at the floor, with or without a token or verifier.admin::listing_floor, which applies the SigV2 refusal to the two RustFS metadata listing operations.ADR-0052 records the decision and refines ADR-0042, ADR-0044, and ADR-0050.
Property, and parity: SigV2 does not cover the request body, and every STS parameter travels in the body. RustFS refuses SigV2 on this route once credentials are presented.
API added (
rustfs-gateway-sig)OperationFloor::refuse_sigv2_under_every_policy(self) -> Self(const)OperationFloor::refuses_sigv2_under_every_policy(&self) -> bool: true for the new rule or forrequire_sigv4_under_every_policy, which implies it.SecurityFloor::enforce_scheme_allowednow reads this predicate, so every SigV4-only floor behaves exactly as before.Composition (ADR-0052)
require_sigv4_under_every_policyWhy
sec1_14andsec1_15were wrongsec1_14asserted that a verified SigV2 headerAssumeRolereaches the handler, andsec1_15that a SigV2-presigned Web Identity form does. Both pinned the gateway's own behaviour, not RustFS's. The legacy stand-in insts_parity.rsmodelled an older RustFS access check that let every signed request through.Evidence: https://github.com/rustfs/rustfs/blob/443e8afee194fe38b32d62a9289a99c1f703715c/rustfs/src/admin/router.rs#L3391-L3421. At this commit, RustFS's router refuses a SigV2
Authorizationheader orSignaturequery key on its custom routes once credentials are present, and its own test (#L6226) lists the/?Action=AssumeRolequery among those routes.The SigV2 rows now refuse:
sec1_14keeps its SigV4 rows, and the SigV2 row moves to the new negativesec1_14_v2_header.sec1_15keeps its anonymous and SigV4-presigned rows, and the SigV2 row moves to the new negativesec1_15_v2_presigned.sec1_16changes fromSignatureDoesNotMatchtoAccessDenied: the floor refuses before any signature is computed.sec1_17(a correct SigV2 signature over the claimed route) andsec1_25_v2_headerchange from positives to refusals.The legacy stand-in's access check now models RustFS's router at that commit. I re-expressed the rule as a short predicate and did not copy RustFS code. With that change, the matrix tests compare SigV2 against RustFS again:
ordinary_assume_role_preserves_header_modes_…;the_sts_endpoint_is_taken_on_every_host_…;ordinary_assume_role_on_a_virtual_host_….Both stacks answer
403 AccessDeniedand run no handler.Known differences (both fail closed, recorded in ADR-0052)
SignatureDoesNotMatch, because it verifies before its SigV2 check. The gateway answersAccessDenied, because it refuses before any lookup.n_a_forged_credential_is_refused_and_runs_no_handlerpins both answers.token. Legacy's access check returns on a token-carrying zip path before its SigV2 refusal. The gateway refuses the SigV2 header at the floor. The minted token-only URL and the SigV4-signed download are unchanged.Merge with
mainafter #1583 (merge commit, no rebase)#1583 (
d66fb5c47) made every claimed admin floor require SigV4 and added a SigV2 census. Two of its tests were written to flip with #1592, and this merge flips them deliberately:zip_token_floorresolves toOperationFloor::custom(name, SigService::S3).refuse_sigv2_under_every_policy().with_query_bearer_claim(ZIP_DOWNLOAD_TOKEN). It keeps fix(dialect-rustfs-admin): refuse SigV2 on claimed admin routes #1583's reason for not requiring SigV4 and adds this PR's rule.crates/goldens/src/rustfs_admin_dialect/sigv2_census_tests.rs:670 - 8rows instead of670 - 8 - 2), because its floor refuses SigV2 before the authorizer;a_sigv2_header_signature_on_an_exempt_row_is_verified_and_reaches_its_handlerkeeps only its OIDC row. It used to send SigV2 to the zip download and expect200, which was the difference this PR closes.crates/dialect-rustfs-admin/tests/integration/census.rs: the recordthe_zip_download_sigv2_difference_is_recorded, whose own doc said a rustfs:StsFormPost admits SigV2 where RustFS refuses it #1592 fix was expected to change it, becomes the negativethe_zip_download_refuses_sigv2_without_requiring_sigv4.every_admin_operation_but_the_named_exemptions_requires_sigv4keeps the zip download as the one non-OIDC SigV4 exemption, and its doc now says why.extensions.rskeeps both sides: fix(dialect-rustfs-admin): refuse SigV2 on claimed admin routes #1583's mint assertion and this PR's download assertions.main's form; the only incoming changes to those files are fix(dialect-rustfs-admin): refuse SigV2 on claimed admin routes #1583's own.lambda_tests::sigv2_header_signedandtests::rowswere hunk-identical to fix(dialect-rustfs-admin): refuse SigV2 on claimed admin routes #1583's and merged cleanly.Guard list updated deliberately
scripts/check_sig_test_target_consolidation.shfreezes the module list of the sig crate's one integration target. This PR addstests/sigv2_refused_floor.rsto that target (tests/integration.rs), so the list gainssigv2_refused_floor, in the same order asintegration.rs.The first CI run on
88147f9efailed this guard inStatic checks, and failedGuard self-test 5's "fails on the current tree" case. Both now pass locally:GATEWAY_GUARD_SHARD_GROUP=4of 6: 340 ok, 0 FAIL;#1597 (STS handler receives the caller's secret) is unblocked by this change.
Tests
Tests were written first. Before the implementation, the sig tests did not compile against the missing API. With the floors still unchanged, 13 of 39 STS-parity and zip goldens failed, and every failure was a SigV2 row.
crates/sig/tests/sigv2_refused_floor.rscrates/sig/src/operation.rsunit testscrates/core/src/registry/reject_floor_tests.rscrates/goldens/src/operation_diff/sts_parity/sigv2.rscrates/goldens/src/rustfs_admin_dialect/zip_sigv2_tests.rssec1changes (sec1_14_v2_header,sec1_15_v2_presigned,sec1_16,sec1_17,sec1_25_v2_header)sts_form,extensionszip pair: added assertions;census::the_zip_download_refuses_sigv2_without_requiring_sigv4)Coverage against the issue's acceptance list:
HeaderOnly,HeaderAndPresignedand the RustFS-profile floor.AssumeRoleand Web Identity, on the default and the RustFS-profile floor.CredentialProvidersees no lookup for any SigV2 request. The control (HeadObject, no rule) shows the same recorder does see the SigV2 key, so it is not deaf. At the floor, an unreadable SigV2 signature is also refusedAccessDenied, where the control without the rule reaches the parser.AssumeRolereaches the handler as the caller, after exactly one lookup. Anonymous Web Identity reaches it with no caller and no lookup.HeadObject, and an OIDC admin row) still admits SigV2 under the RustFS profile.Mutations
Every mutation was applied to the working tree, run with the crate's own filtered tests, and reverted. None was committed. All ran locally. None needs the full matrix, so no probe PR was opened.
enforce_scheme_allowedignores the new rule (readsrequires_sigv4_…only)sigv2_refused_floornegatives +an_operation_refusing_sigv2_…. Goldens: all 13 SigV2 rows (sec1_14_v2_header,sec1_15_v2_presigned,sec1_16,sec1_17,sec1_25_v2_header, bothsigv2::n_*, bothzip_sigv2_tests::n_*, the 4 matrix tests)refuses_sigv2_…drops the SigV4 implicationn_the_answer_equals_the_sigv4_requirements_answer,requiring_sigv4_implies_refusing_sigv2_in_either_order,an_operation_requiring_sigv4_refuses_sigv2_under_every_policysigv4_requiredanonymous_bearer_and_custom_admissions_are_unchanged,an_operation_refusing_sigv2_…(!requires_sigv4); goldens: 39, because the registry then refuses the zip floor and every dialect assembly failsn_the_refusal_precedes_the_sigv2_credential_parse,n_every_sigv2_shape_…,n_the_answer_equals_…,an_operation_refusing_sigv2_…rustfs:StsFormPost's floor without the rulests_form::the_sts_operation_declares_…; goldens: 11 STS SigV2 rowszip_token_floorwithout the rule (re-run after the merge with #1583)extensions::the_zip_pair_declares_…,census::the_zip_download_refuses_sigv2_without_requiring_sigv4, bothzip_sigv2_tests::n_*, #1583'ssigv2_census_tests::n_a_sigv2_header_signature_on_every_claimed_admin_row_…(which now includes the zip rows)sigv4_assume_role_and_unsigned_web_identity_…,an_operation_without_the_rule_still_admits_sigv2_and_looks_its_key_upn_a_known_and_an_unknown_sigv2_key_get_one_answerrefusing_sigv2_registers_beside_a_bearer_claim_…n_a_sigv4_only_floor_with_a_bearer_claim_is_refused,n_refusing_sigv2_does_not_rescue_…sigv4_header_presigned_and_post_policy_are_still_admitted,anonymous_bearer_and_custom_…,requiring_sigv4_keeps_…; goldens:sigv2::sigv4_assume_role_…,sec1_14,sec1_15and 11 morerefuses_sigv2_…always truewithout_the_rule_the_same_operations_admit_sigv2,n_the_refusal_precedes_…(control half),n_a_sigv2_header_carrying_a_bearer_token_is_refused(control half),an_operation_refusing_sigv2_…The two one-directional risks are covered:
HeadObjectcontrols go red.Checklist
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace,cargo xtask verify --crateforrustfs-gateway-sig,rustfs-gateway-core,rustfs-gateway-dialect-rustfs-adminandrustfs-gateway-goldensdocs/adr/0050-*is a new ADR; no accepted ADR edited)unsafeintroducedinventory/linkmeintroduced#[non_exhaustive]added to a DTORole Verdicts
boolbesidesigv4_required(an enum would buy nothing: the only reader is the derivedrefuses_sigv2_under_every_policy, so the two fields cannot disagree), whether the new public predicate needs to be public (the dialect's floor-declaration tests read it across the crate boundary), the test-only visibility widening (tests::rows,lambda_tests::sigv2_header_signed,zip_token_tests::with_verifier,sec1's HeadObject fixtures; the first two are hunk-identical to fix(dialect-rustfs-admin): refuse SigV2 on claimed admin routes #1583), and thegateway_builder_onsplit, which has two callers — no break foundSigV2Policyand the legacy-recognition floor, a percent-encoded%53ignaturekey (count_query_paramdecodes keys, so it is recognized as SigV2 and refused), an unsignedAWSAccessKeyIdquery under legacy recognition (read as anonymous with no identity, so ordinaryAssumeRoleis still refused by the codec and nothing authenticates), custom-scheme laundering (only marker-less requests reach a scheme), a SigV2 header carrying a good zip token (refused at the floor, verifier never asked), and error bodies for key or signature echoes — no break foundsec1assertion fails on revert (M1, M5), the POST-policy shape (unreachable on the urlencoded STS claim, so it is covered at the floor in both directions), negative/positive balance (21/7), and determinism (the new tests useRequestNow::captureonly where the existing suite does, and sign and send in the same instant) — no break found