Repository navigation
fix(cloudfront): decode query names strictly and preserve XML entities - #56
Merged
Merged
Conversation
Some spec-compliant S3 clients (e.g. Transmit 5) sign the normalized (decode + re-encode) canonical query string while the request line carries raw values, e.g. signing `prefix=periods%2F` for `?prefix=periods/`. The raw-preserving canonicalization rejected those signatures. Signature verification now tries the raw-preserving form first (keeping compatibility with clients that sign unencoded values, e.g. minio-java via OkHttp) and falls back to the SigV4-normalized form. Applies to both Authorization-header and presigned-URL verification.
Addresses review on PR #54: percent_decode_str(..).decode_utf8_lossy() mapped distinct wire encodings (e.g. %FF vs %EF%BF%BD) to the same normalized string, creating signature collisions. Normalization now uses strict UTF-8 decoding and returns AuthError::InvalidQueryString; verification callers skip the normalized fallback when it fails. Add a regression test for the collision scenario.
CI runs stable 'cargo fmt --all -- --check', whose chain-breaking differs from nightly on these two closures.
…city P1: normalization treated a raw '+' the same as '%2B', so a request signed for 'x=%2B' still verified after tampering the wire query to 'x=+', while downstream API Gateway / Lambda handlers decode the raw '+' as a space. Normalization now translates '+' to a space before percent-decoding (matching downstream form-decoding semantics), so '%2B' stays a plus and '+' becomes '%20' -- the two can no longer normalize to the same canonical string. P2: size the params buffer by separator count instead of 'query.len()/8' so a single long parameter cannot trigger memory amplification on unauthenticated requests before signature verification. Also fix a clippy redundant-pattern-matching lint in the UTF-8 test.
A raw '+' is the one byte whose meaning downstream decoders disagree on: S3 percent-decodes it to a literal plus while API Gateway / Lambda decode it as a space. The previous fix translated '+' to a space, which closed the '%2B'->'+' tampering but opened the inverse: a request signed for 'prefix=%20' still verified after tampering the wire query to 'prefix=+', while S3 delivers a literal plus. Instead of guessing (sanitization), reject the ambiguous representation: when the raw-preserving first attempt fails and the wire query contains a raw '+', the normalized fallback is skipped and verification falls through to a signature mismatch. This matches the repo's 'reject, don't sanitize' rule, and needs no per-service decoder map in the shared auth crate. Per the SigV4 spec '+' must be percent-encoded in the canonical query string anyway, so spec-compliant signers are unaffected; clients that sign wire bytes as-is are still covered by the raw first attempt.
P1: the normalized fallback asserts alternate wire encodings are equivalent, which only holds when downstream percent-decodes query params. CloudFront's DeleteRealtimeLogConfig reads `Name` from the raw query, so a `Name=a%2Fb` signature would verify a replayed `Name=a/b` that deletes a different config. Gate the fallback on service_supports_normalized_fallback() (allowlist after audit; default fail-closed). Currently only `s3`: all query params go through percent-decoding (rustack-s3-http parse_query_params). P2: cap the normalized query at 1024 parameters before allocating or parsing; each component becomes a pair of owned strings, so an unbounded count amplifies small inputs on every raw-signature miss. Fail closed via AuthError::InvalidQueryString, which callers already map to 'skip the fallback'. Tests: unaudited-service fallback skip (execute-api, 403 expected); param cap boundary (1024 ok / 1025 err).
…resigned auth-param aliases P1: fallback eligibility was keyed on the credential scope's service string, which is request-supplied. A request scoped 's3' served by a non-decoding endpoint (e.g. CloudFront reading Name raw) would pass the allowlist and verify a replayed encoding acting on a different value. verify_sigv4 is now raw-only; endpoints opt into the normalized fallback by calling the S3-specific verify_s3_sigv4 / verify_presigned entry points. The service allowlist is removed - the endpoint's choice of entry point is its trusted policy. P2: presigned X-Amz-* parameter names must be literal and unique. An encoded alias (%58-Amz-Expires) or duplicate would verify under the normalized canonical query while a different effective value (e.g. expiry) is extracted. parse_presigned_params now rejects both with AuthError::InvalidAuthParam.
Query parameter values are percent-decoded per AWS semantics, and query_resource() in the same file already decodes Resource. Reading Name raw meant ?Name=a%2Fb looked up the literal name "a%2Fb" instead of "a/b", so deleting a config whose name needs encoding failed with NoSuchResource. Decode Name with the existing percent_decode helper. Regression tests: encoded name deletes the right config, plain name still works.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4fc5d1e9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
tyrchen
changed the title
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
1 participant
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.
问题与修复
DeleteRealtimeLogConfig的 queryName=a%2Fb现在只解码一次并定位a/b。严格 UTF-8 解码避免%FF等非法字节被替换成�，进而误删另一个配置；共用的Resourcequery 解码遵循同一规则，非法值返回 HTTP 400InvalidArgument。XML body 的名称不进行 percent decoding。补回归时发现 quick-xml 的
GeneralRef事件被忽略，a%2Fb&c会错误地变为a%2Fbc。现在正确解析五种预定义实体和数字字符引用，只解码一次，保留 Text/CDATA/实体的顺序和内部空格。拒绝未知实体、DTD、非法 XML 数字字符引用、结束标签不匹配、未闭合及多个根元素，避免错误输入操作不同资源。移除不再需要的手写实体解码和 dead Method helper。Namequery 是现有 Rustack 兼容扩展。AWS 官方 DeleteRealtimeLogConfig API 使用 XML body；本次没有改变既有 HTTP 路由。分支依赖
此 PR 堆叠于 #54，已同步其最新签名重放修复。建议先合并 #54，再合并本 PR。分支也已合入最新 master 和共用的 Rust 1.99.0 pin（Docker 使用同一 toolchain）、rustls 安全补丁及新 Clippy lint 修复。
验证
cargo +nightly fmt --all -- --check与 Rust 1.99.0 stable format check 通过。git diff --check通过。cargo build --locked、cargo test --locked通过：108 passed / 0 failed（auth 74 单元测试、11 doctests，CloudFront HTTP 23 单元测试）。cargo clippy --locked --all-targets --all-features -- -D warnings通过；最终全 workspace 的 CI 格式、编译、lint 和测试全部通过。4cdabd601836ef9ef63ff25fafe9e6fbd6fa4ff0的 GitHub Actions 全部完成：30 checks 成功，4 个 MinIO/Alternator/SQS/Lambda 兼容性 checks 按既有 workflow 条件跳过，无失败。已同步 #54 的
27c7eaftest-only 修正：presigned auth-alias replay 回归的 credential 编码保持 SigV4 unreserved 字符，独立碰撞 oracle 与真实 normalized canonical query 一致，并断言原始/交换值后的歧义 query 均由生产 normalizer 拒绝。生产 auth 行为未改变。