fix: parse hmac-auth signed_headers from Secret as a header list#436
Open
shreemaan-abhishek wants to merge 3 commits into
Open
fix: parse hmac-auth signed_headers from Secret as a header list#436shreemaan-abhishek wants to merge 3 commits into
shreemaan-abhishek wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughHMAC Secret translation now validates numeric fields, preserves negative-value defaults, and tokenizes ChangesHMAC secret translation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/adc/translator/apisixconsumer_test.go`:
- Around line 68-82: Extend
TestTranslateApisixConsumer_HMACAuthRejectsInvalidClockSkew with cases covering
an invalid max_req_body value and an empty entry in signed_headers. For each
case, configure the corresponding secret data, call TranslateApisixConsumer, and
assert an error mentioning the affected field, while preserving the existing
clock_skew coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0edda70f-e478-4d19-8882-e40479af60b9
📒 Files selected for processing (2)
internal/adc/translator/apisixconsumer.gointernal/adc/translator/apisixconsumer_test.go
Contributor
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-24T10:09:59Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
result: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded.
- core:
result: partial
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 0
Passed: 32
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests partially succeeded with 1 test skips. Extended tests partially
succeeded with 1 test skips.
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips. |
Contributor
conformance test report - apisix modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-24T10:11:07Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
result: success
statistics:
Failed: 0
Passed: 12
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests succeeded.
- core:
failedTests:
- HTTPRouteInvalidBackendRefUnknownKind
- HTTPRouteMatchingAcrossRoutes
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 2
Passed: 30
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 2 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
result: partial
skippedTests:
- TLSRouteSimpleSameNamespace
statistics:
Failed: 0
Passed: 10
Skipped: 1
name: GATEWAY-TLS
summary: Core tests partially succeeded with 1 test skips. |
Contributor
conformance test reportapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-24T10:27:16Z"
gatewayAPIChannel: experimental
gatewayAPIVersion: v1.3.0
implementation:
contact: null
organization: APISIX
project: apisix-ingress-controller
url: https://github.com/apache/apisix-ingress-controller.git
version: v2.0.0
kind: ConformanceReport
mode: default
profiles:
- core:
failedTests:
- GatewayModifyListeners
result: failure
statistics:
Failed: 1
Passed: 11
Skipped: 0
name: GATEWAY-GRPC
summary: Core tests failed with 1 test failures.
- core:
failedTests:
- GatewayModifyListeners
result: failure
skippedTests:
- HTTPRouteHTTPSListener
statistics:
Failed: 1
Passed: 31
Skipped: 1
extended:
result: partial
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 0
Passed: 11
Skipped: 1
supportedFeatures:
- GatewayAddressEmpty
- GatewayPort8080
- HTTPRouteBackendProtocolWebSocket
- HTTPRouteDestinationPortMatching
- HTTPRouteHostRewrite
- HTTPRouteMethodMatching
- HTTPRoutePathRewrite
- HTTPRoutePortRedirect
- HTTPRouteQueryParamMatching
- HTTPRouteRequestMirror
- HTTPRouteResponseHeaderModification
- HTTPRouteSchemeRedirect
unsupportedFeatures:
- GatewayHTTPListenerIsolation
- GatewayInfrastructurePropagation
- GatewayStaticAddresses
- HTTPRouteBackendProtocolH2C
- HTTPRouteBackendRequestHeaderModification
- HTTPRouteBackendTimeout
- HTTPRouteParentRefPort
- HTTPRoutePathRedirect
- HTTPRouteRequestMultipleMirrors
- HTTPRouteRequestPercentageMirror
- HTTPRouteRequestTimeout
name: GATEWAY-HTTP
summary: Core tests failed with 1 test failures. Extended tests partially succeeded
with 1 test skips.
- core:
failedTests:
- GatewayModifyListeners
- TLSRouteSimpleSameNamespace
result: failure
statistics:
Failed: 2
Passed: 9
Skipped: 0
name: GATEWAY-TLS
summary: Core tests failed with 2 test failures. |
jarvis9443
reviewed
Jul 24, 2026
| clockSkew, err = strconv.ParseInt(string(clockSkewRaw), 10, 64) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("hmac-auth: invalid clock_skew %q in secret: %w", string(clockSkewRaw), err) | ||
| } |
Contributor
There was a problem hiding this comment.
This pair has already drifted from apache/apisix-ingress-controller#2809: upstream commit 8b165835 now includes the Secret namespace/name in both parse errors, while this branch still emits context-free messages. Please forward-port that follow-up to both the clock_skew and max_req_body paths so the shared translator behavior stays aligned and operators can identify the offending Secret.
Contributor
Author
There was a problem hiding this comment.
Forward-ported in 6e300ed — both the clock_skew and max_req_body paths now include the Secret namespace/name, matching upstream 8b165835.
This branch also picks up the wider follow-up from apache/apisix-ingress-controller#2824 (whitespace-tolerant signed_headers split, CRD field docs, test-helper rename), so internal/adc/translator/apisixconsumer.go is now byte-identical to the upstream version.
translateConsumerHMACAuthPlugin ranged over the raw bytes of the signed_headers Secret value, emitting one single-character string per byte instead of the configured header names. The data-plane signature policy then bound nonsensical headers, silently voiding the operator's intended integrity control. Only the secretRef path was affected; the inline Value path already passed a []string. Split the value on commas and trim entries. Also surface strconv.ParseInt failures for clock_skew and max_req_body instead of discarding them, so a typo no longer coerces silently to a default.
Forward-ports apache/apisix-ingress-controller#2809 follow-up 8b165835 so both parse errors name the offending Secret, and brings over the follow-up in apache/apisix-ingress-controller#2824: - accept whitespace as well as commas when splitting signed_headers, so a newline- or space-separated Secret value no longer collapses into one bogus header name - document the Secret-vs-Value format difference on the CRD field - rename the ambiguous hmacConsumerWithSecret parameter - cover the delimiter variants, the empty case and the previously untested max_req_body parse failure
shreemaan-abhishek
force-pushed
the
fix/hmac-signed-headers-secret-parsing
branch
from
July 24, 2026 08:51
1cde587 to
6e300ed
Compare
crd-ref-docs joins consecutive godoc lines with spaces and only emits <br /> for blank-line paragraph breaks, so the hand-written version did not match the generator.
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
2 participants
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.
What this PR does
translateConsumerHMACAuthPluginreadsigned_headersfrom a Secret by ranging over its raw bytes:Ranging over a
[]byteyields(index, byte), so"X-Date,Host"became["X","-","D","a","t","e",",","H","o","s","t"]. The data-plane hmac-auth policy then bound those single-character names into the signature, which never match real headers, so the operator's intended headers were silently not enforced as part of the HMAC signature. Only thesecretRefpath was affected; the inlineValuepath already passes a[]string.Fix
signed_headersvalue on commas and trim entries.strconv.ParseIntfailures forclock_skewandmax_req_bodyinstead of discarding them, so a typo no longer coerces silently to a default.Tests
signed_headersfrom a Secret yields the correct header list; an unparseableclock_skewis rejected.Paired open-source PR: apache/apisix-ingress-controller#2809. Fixes FINDING-050 (rfcs#184).
Summary by CodeRabbit
signed_headersfrom Secrets by splitting on commas or whitespace, trimming entries, and ignoring empty values.signed_headersstorage format for HMAC secret references in API docs and CRD schema.