Repository navigation
fix(security): AWS SigV4 headers are credential slots; full-version image tags only; count unscannable-body bypasses - #1126
Conversation
A release tag published `:X.Y.Z`, `:X.Y`, `:X` and `:latest`. The two abbreviations are removed; `:latest`, `:dev`, `:sha-<short>` and PR refs are unchanged. An abbreviated tag reads like a line but is a snapshot: `:1` published at 1.0.0 stops moving the day 1.1.0 ships, so anyone who pinned it silently keeps an old image under a name that suggests otherwise. The full version and `:latest` are the two honest answers, and the project is about to cut its first non-0.x version, which is when a `:1` would first be created. Also removes the `:X.Y` sentence the release-draft workflow stamps into every stable release body, and the tag list in RELEASING.md — both would otherwise advertise tags that are no longer published. Existing published abbreviations are unaffected; this only stops new ones being created.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe release configuration removes Docker ChangesDocker tag strategy
Guardrail bypass metrics
AWS SigV4 header forwarding
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: ⚪ Minimal · up to This change protects AWS SigV4 credential headers from wildcard forwarding while retaining explicit forwarding, records unscannable-body bypasses in metrics, and removes abbreviated release image tags. No current merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
Full details: E2e Test Quality ReviewExplanation PASS — The changed runtime behavior has end-to-end coverage. The guardrail test drives a real gateway with etcd, a real local upstream, and an SLS exporter, then checks successful responses, usage-event reasons, and the Full details: Security CheckExplanation PASS — the diff introduces no finding under the seven stated security categories. 1. Sensitive data exposure: No new logging, response serialization, audit serialization, or secret-bearing object formatting was added. AWS SigV4 headers are blocked for wildcard forwarding and exact forwarding requires explicit operator configuration. Forwarded values are marked sensitive. 2. Unencrypted database secrets: No database model, migration, or persistence code changed. 3. Authorization bypass: No endpoint, permission check, or error-response control flow changed. 4. Cross-resource access: No resource lookup, ownership validation, or parent-scope query changed. 5. TLS/cryptographic errors: No TLS configuration or certificate handling changed. 6. Shared-resource safety: No shared-resource binding or cascade operation changed. 7. Secret reference resolution: No configuration resolution path changed. The guardrail metric addition passes bounded reasons to audit and metrics. The Docker and release workflow changes alter image-tag prose and tag patterns only; permissions and secret handling are unchanged. The changed-file diff confirms that the AWS header list and guardrail telemetry paths are the only security-relevant implementation changes, and both changes reduce uncontrolled credential forwarding or add bounded observability rather than introduce a listed vulnerability. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are limited to CI/workflow/docs, and the updated workflows no longer reference or depend on the removed abbreviated image tags.
Pull request overview
This PR updates the container image publishing strategy so releases only publish “honest” moving/stable tags: the full version (X.Y.Z) plus latest (stable only) and sha-<short>, removing the misleading abbreviated semver tags (:X.Y, :X) from the release workflow.
Changes:
- Remove
docker/metadata-actionsemver tag patterns for{{major}}.{{minor}}and{{major}}, leaving only{{version}}(plus existinglatest,dev,sha-, and PR/ref tags). - Update workflow comments and release text generation to stop advertising
:X.Y/:Xtags. - Update release process documentation to reflect the new published tag set.
File summaries
| File | Description |
|---|---|
RELEASING.md |
Updates release documentation to reflect that only full version + latest + sha-<short> are published. |
.github/workflows/release-draft.yml |
Adjusts the generated rolling-tags sentence so stable releases advertise latest only (no X.Y). |
.github/workflows/docker-image.yml |
Removes major/minor semver tag publishing and updates tag-strategy documentation/comments accordingly. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…s_total `aisix_guardrail_bypasses_total` only ever counted bypasses a guardrail member reported: the increment hangs off the per-execution record, and an unscannable body has no execution by construction — the body could not be decoded, so no member ran. The chain recorded that pass-through to the audit log (and so to `UsageEvent.guardrail_bypassed_reason`) and to nothing else. The sibling counter is the other way round: `aisix_guardrail_blocks_total` deliberately includes the fail-closed paths that happen before a member executes. So one situation — the body could not be scanned — was counted when the chain refused and counted nowhere when it let the request through, which is the direction an operator reads `bypasses_total` to find. #1115 widened exactly that fail-open half, so the traffic newly passing through was the traffic the counter could not see. `GuardrailMetricsSink` gains `record_guardrail_bypass(reason)` and `GuardrailChain::record_bypass` now feeds both receivers. Deliberately a second method rather than a synthetic execution: there is no member, kind or duration to report, and inventing them would put a phantom row in the per-execution latency histogram. It is required rather than defaulted so a sink cannot silently drop these. Which pass-throughs count is unchanged and is still decided in one place. `record_unevaluable_{input,output}_bypass` already distinguishes a chain whose members all fail open on that side (a bypass) from one where no member reads that side (never offered to screen it, so not a bypass), off the same member set the refusal gate uses. The counter reads that decision rather than deriving its own. Tests: the chain unit test that pins the distinction now asserts both sinks, so the usage field and the counter cannot disagree about which pass-through was a bypass. All three assertions were verified to go red under mutation — dropping the sink forwarding, counting every pass-through, and letting the counter derive its own predicate. The bypass-reason e2e additionally asserts the `reason="unscannable_body"` delta in `GET /metrics` after driving a real unscannable request.
jarvis9443
changed the title
|
@coderabbitai the E2e Test Quality Review warning is looking at the wrong base. There is nothing to remove or revert — the suggested resolution would drop a commit off main. The two changes actually in the PR are batched deliberately: this repo's contributing rules ask for one PR per repository per effort rather than splitting related work across several bot-reviewed PRs. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- merge base ---'
git merge-base origin/main HEAD
printf '%s\n' '--- origin/main HEAD ---'
git rev-parse origin/main
git log -1 --format='%H%n%s' origin/main
printf '%s\n' '--- files in origin/main...HEAD ---'
git diff --name-only origin/main...HEAD
printf '%s\n' '--- CLAUDE.md status in the merge-base diff ---'
git diff --name-status origin/main...HEAD -- CLAUDE.md
printf '%s\n' '--- commits introduced by HEAD relative to origin/main ---'
git log --oneline origin/main..HEADLength of output: 1491
The E2E Test Quality Review warning used a change from 🐇🔍 ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive with the stated behavior changes, the new required trait method is implemented at all in-repo call sites, and unit/e2e coverage was extended to prevent audit-vs-metrics drift.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Review findings on the doc comments added by the previous commit. All three were prose that overstated what the counter says; no behaviour changes. - `blocks_total` and `bypasses_total` are not the same unit, so calling them counterparts invited summing or comparing them. The first fires once per REQUEST off the terminal usage event; the second fires once per bypass EVENT, so one outage on a chain with two fail-open members on both hooks adds 4 for a single request. - "a body the gateway could not give the guardrails at all" claimed the counter covers every unscannable pass-through. It does not: `jobs::scan_output_blob` and `passthrough_route` scan a lossy copy with no failure policy involved and record nothing by design (crates/aisix-proxy/AGENTS.md), so a zero here is not proof that nothing went unread. - The pre-execution producer reaches this counter only. With no execution there is no `aisix_guardrail_latency_seconds` row, so that family's `result="bypassed"` slice stays execution-only and the two no longer agree on a total — worth saying where both are documented together. Also corrects the claim in `GuardrailChain::record_bypass` that the two producers cannot both fire on one request. No EVENT is counted twice, but a request can legitimately produce both: `audio.rs` records an undecodable transcript tail and then scans the decodable remainder, whose members may themselves fail open.
Three headers became reachable by an ordinary glob when the forwarding rules moved out of `config.rs`. `RESERVED_UPSTREAM_HEADERS` used to block `x-amz-security-token`, `x-amz-date` and `x-amz-content-sha256` outright. The split into `forwarded_headers` kept six of that constant's names as `CREDENTIAL_SLOT_HEADERS`, which a glob cannot reach, and the three `x-amz-*` names landed in no list at all — not the credential slots, not `NON_FORWARDABLE_HEADERS`, not `NEVER_FORWARD_FROM_CLIENT`. So a provider key configured `forward_client_headers: ["x-*"]` relayed the caller's AWS session token to the upstream. That contradicts the module's own rule, that a glob is a statement about the operator's own headers and not consent to hand a third party the caller's credential. Bedrock upstreams were never exposed — the SigV4 signer owns all three names and drops any supplied value before signing — so the reach was every OTHER upstream, which is where an AWS credential has no business arriving. The fix restores the credential-SLOT rule rather than the old absolute block: a glob does not reach the three, and an operator who names one exactly still forwards it, exactly as the other six behave. That keeps the capability available to an upstream that genuinely reads the caller's SigV4 headers, which the pre-split behaviour did not allow. `provider_key` and `passthrough_route` enumerate the credential slots in the field descriptions the control plane and the public API reference read, so both gain the three names and the schemas are regenerated. `mcp_server` says "a credential slot" without enumerating and needed no change. Tests name the three headers explicitly rather than iterating the constant — a loop over `CREDENTIAL_SLOT_HEADERS` can only confirm that whatever is in the list behaves, and can never notice a name that dropped out of it, which is exactly how this went unseen. Both go red without the fix, the gateway-level one by showing all three arriving at the upstream under `["x-*"]`.
jarvis9443
changed the title
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, fully covered by targeted unit/e2e tests for the security and metrics behavior, and the workflow/tag policy updates are consistent across all repo publish/documentation touchpoints.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
Review findings on the previous commit. The SigV4 fix was pinned only at the unit layer, while the bug's observable form is an upstream actually receiving the caller's AWS session token. `forward-client-headers-e2e` already drives a ProviderKey configured `X-*` against a real upstream, so the three names join the request it already makes and the assertion says they must not arrive. Verified to fail without the fix, reporting the token's value at the upstream. That fixture had no coverage of a credential slot being refused by a glob at all — it asserted only the positive direction, an exactly-named `x-api-key` forwarding — so this closes a gap wider than the one commit. Also corrects two counts the previous commit left stale: the module header still enumerated six credential slots, and `exact_match_only_with` said "the two" shared names a passthrough route's own slots may take, which is five once the trio is on the shared list. The claim around it is unaffected — those names are all on the shared list, so the rule stays complete — only the number was wrong.
There was a problem hiding this comment.
🔵 Needs a closer look
It combines security-sensitive header-forwarding behavior changes with release publishing workflow changes and core observability plumbing, warranting final human review despite strong test coverage.
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
Three independent changes on the 1.0.0 critical path, batched into one PR per this repo's one-PR-per-repository rule.
1. Security: the AWS SigV4 headers are credential slots again
This is the important one.
x-amz-security-token,x-amz-dateandx-amz-content-sha256were blocked outright byRESERVED_UPSTREAM_HEADERSinconfig.rs. When the forwarding rules moved intocrates/aisix-core/src/forwarded_headers.rs, six of that constant's names becameCREDENTIAL_SLOT_HEADERS— the set a glob cannot reach — and the threex-amz-*names landed in no list at all: not the credential slots, notNON_FORWARDABLE_HEADERS, notNEVER_FORWARD_FROM_CLIENT.A provider key configured
forward_client_headers: ["x-*"]therefore relayed the caller's AWS session token to the upstream. That contradicts the module's own stated rule — a glob is a statement about the operator's own headers, not consent to hand a third party the caller's credential.Bedrock upstreams were never exposed: the SigV4 signer owns all three names and drops any supplied value before signing. The reach was every other upstream, which is exactly where an AWS credential has no business arriving.
The fix restores the credential-slot rule, not the old absolute block: a glob does not reach the three, and an operator who names one exactly still forwards it — identical to how the other six behave, and strictly more capable than the pre-split behaviour, which allowed neither.
provider_keyandpassthrough_routeenumerate the credential slots in the field descriptions the control plane and the published API reference read, so both gain the three names and the schemas are regenerated.mcp_serverrefers to "a credential slot" without enumerating and needed no change.Scope note. No released build ever shipped the glob-reachable state — it existed only between the refactor and this commit, both after
v0.13.0. This is a defect fixed before release, not an advisory: no user could have observed it, and the release notes should not describe it as a behaviour change.Testing. The tests name the three headers explicitly rather than iterating the constant. A loop over
CREDENTIAL_SLOT_HEADERScan only confirm that whatever is in the list behaves and can never notice a name that dropped out of it — which is precisely how this went unseen. Both new tests go red without the fix; the gateway-level one fails by showing all three headers arriving at the upstream under["x-*"], and also pins the positive direction (an exactly-namedx-amz-security-tokenstill forwards).2. Image tags: publish the full version only, no
:X.Yor:XA release tag push published
:X.Y.Z,:X.Y,:Xand:latest. The two abbreviations are snapshots dressed as lines::1published at 1.0.0 stops moving the day 1.1.0 ships, so anyone who pinned it silently keeps an old image under a name that suggests it tracks the major. The full version and:latestare the two honest answers.The project is about to cut its first non-
0.xversion, which is the first time a:1would be created — and the workflow that runs on a tag push is the one at the tagged commit, so this has to be onmainbefore that tag point is chosen..github/workflows/docker-image.yml— the metadata-actiontags:block drops:type=semver,pattern={{version}},type=raw,value=latest,type=raw,value=dev,type=sha,prefix=sha-andtype=ref,event=prare unchanged. The tag-strategy comment at the top of the file and the comment above the block are updated to match.Two places that described or advertised the removed tags:
.github/workflows/release-draft.ymlstamps a rolling-tags sentence into every stable release body. It named the:X.Ytag, which would otherwise be advertised on the release page while never being pushed. It now nameslatestonly; the-rcbranch is unchanged.RELEASING.mdlisted the published tag set.Publish paths swept.
docker/metadata-actionis invoked in exactly one place (docker-image.yml); it is the only workflow that builds or pushes an image, and it covers both the GHCR publish and the Docker Hub mirror.release-draft.ymlmentions the action only in prose. No other tag-publishing path exists in the repo, and nothing in the repo pins an abbreviated tag.Behavior change. A release tag now publishes
:X.Y.Z,:latestand:sha-<short>to GHCR and the Docker Hub mirror — no:X.Y, no:X. Already-published abbreviations are untouched; this only stops new ones being created. Anything pinning an abbreviated tag must move to the full version or:latest.Testing. CI-only with no runtime behavior, so there is nothing for a fail-before/pass-after test to bite on, and no test or linter in the repo asserts the published tag set. Verified by inspection instead: both workflows still parse, the edited
release-draft.ymlshell block still passesbash -n, and the downstream steps consumingsteps.meta.outputs.tagsare unaffected by there being fewer tags — the release-path steps address the image by digest, the PR-path steps takehead -n1, and the cosign step dedups per repository and signs the digest.3.
aisix_guardrail_bypasses_totalcounts an unscannable-body bypassaisix_guardrail_bypasses_totalonly ever counted bypasses a guardrail member reported: the increment hangs off the per-execution record, and an unscannable body has no execution by construction — the body could not be decoded, so no member ran. The chain recorded that pass-through to the audit log (and so toUsageEvent.guardrail_bypassed_reason) and to nothing else.The sibling counter is the other way round:
aisix_guardrail_blocks_totaldeliberately includes fail-closed paths that happen before a member executes. So one situation — the body could not be scanned — was counted when the chain refused and counted nowhere when it let the request through, which is the direction an operator readsbypasses_totalto find. #1115 widened exactly that fail-open half in this release, so the traffic newly passing through was the traffic the counter could not see.GuardrailMetricsSinkgainsrecord_guardrail_bypass(reason), andGuardrailChain::record_bypassnow feeds both receivers. A second trait method rather than a synthetic execution, deliberately: there is no member, kind or duration to report, and inventing them would put a phantom row in the per-execution latency histogram. Required rather than defaulted, so a sink cannot silently drop these.Which pass-throughs count is unchanged and still decided in one place.
record_unevaluable_{input,output}_bypassalready distinguishes a chain whose members all fail open on that side (a bypass) from one where no member reads that side (never offered to screen it, so not a bypass), read off the same member set the refusal gate uses. The counter reads that decision rather than deriving its own.Behavior change
aisix_guardrail_bypasses_totalnow also counts bypasses that happen before any guardrail member executes — today that isreason="unscannable_body", a body the gateway could not give the guardrails at all, which a fail-open chain lets through. Thereasonlabel gains that value; it is a bounded DP-internal tag and is clamped on the way in, so cardinality is unchanged in kind.Operators are affected. The
reason="unscannable_body"series was always zero for this cause and starts being non-zero, and the unlabelled total rises accordingly on any deployment that receives bodies the scanner cannot read. An alert threshold onaisix_guardrail_bypasses_totalmay fire on traffic that was always passing unscreened and simply was not counted. Nothing about which traffic is allowed or refused changes — this is a counting fix only.Two limits are now stated where the counter is declared, because the change makes both easier to get wrong:
blocks_totalandbypasses_totalare not the same unit and must not be summed or compared. The first fires once per request, off the terminal usage event; the second fires once per bypass event, so one outage on a chain with two fail-open members on both hooks adds 4 for a single request.aisix_guardrail_latency_secondsrow, so that family'sresult="bypassed"slice stays execution-only and the two no longer agree on a total.jobs::scan_output_blobandpassthrough_routescan a lossy copy with no failure policy involved and record nothing by design, so a zero is not proof that nothing went unread.Follow-up owed in this effort
For the credential-slot fix (change 1). The credential-slot names are enumerated verbatim in six places in
AISIX-Cloud: openapi/cp-admin.yaml(provider_keydefault_headers+forward_client_headers, mcp_server create/update, passthrough_route ×2) and in four pages in each docs repo (models/upstream-request-headers.md,mcp-gateway/upstream-authentication.md,reference/resources-file.md×2). All still list six names. This is description-only drift — the CP'sinternal/cpapi/resources/forward_client_headers.gomirrors the transport block lists and deliberately keeps no credential names, so the CP acceptsx-amz-security-tokeninforward_client_headers, projects it, and the DP forwards it on an exact match. Nothing is functionally broken; the published contract just under-describes which names a glob no longer reaches. Tracked outside this PR.The published metric reference and the guardrail behaviour guide contradict this change and need the paired EN + ZH docs PRs (
api7/docsandapi7/docs.apiseven.com):docs/ai-gateway/traffic-controls/guardrails/behavior.mdstates that a proxy-raisedunscannable_bodypass "reaches the usage record only", andsrc/components/MetricReference/data.tsdescribes the counter as timed fail-open executions and enumerates a closedreasonlist that omitsunscannable_body. Tracked outside this PR.Testing
The chain unit test that pins which pass-throughs are bypasses now asserts both sinks, so the usage field and the counter cannot disagree. All three assertions were verified to go red under mutation: dropping the sink forwarding, counting every pass-through, and letting the counter derive its own predicate off a separate member set.
The bypass-reason e2e additionally asserts the
reason="unscannable_body"delta inGET /metricsafter driving a real unscannable request through a real DP — the positive direction only; the discrimination between a bypass and a never-offered pass-through is pinned in the unit test, where an output-only chain can be built without seeding a second route.Summary by CodeRabbit
latestand SHA-based tags.