fix: parse hmac-auth signed_headers from Secret as a header list#436
Open
shreemaan-abhishek wants to merge 1 commit into
Open
fix: parse hmac-auth signed_headers from Secret as a header list#436shreemaan-abhishek wants to merge 1 commit into
shreemaan-abhishek wants to merge 1 commit into
Conversation
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.
📝 WalkthroughWalkthroughHMAC secret translation now rejects invalid numeric values, applies defaults for negative values, and parses comma-separated signed headers with trimming and empty-entry removal. Tests cover signed-header translation and invalid ChangesHMAC secret translation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
Comment on lines
+68
to
+82
| func TestTranslateApisixConsumer_HMACAuthRejectsInvalidClockSkew(t *testing.T) { | ||
| translator := NewTranslator(logr.Discard()) | ||
| tctx := provider.NewDefaultTranslateContext(context.Background()) | ||
| tctx.Secrets[k8stypes.NamespacedName{Namespace: "default", Name: "hmac"}] = &corev1.Secret{ | ||
| Data: map[string][]byte{ | ||
| "key_id": []byte("my-key"), | ||
| "secret_key": []byte("my-secret"), | ||
| "clock_skew": []byte("3O0"), // typo: letter O | ||
| }, | ||
| } | ||
|
|
||
| _, err := translator.TranslateApisixConsumer(tctx, hmacConsumerWithSecret("hmac")) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "clock_skew") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the remaining changed parsing paths.
Add cases for invalid max_req_body and empty signed_headers entries; the current inputs do not verify either new behavior.
Proposed test adjustments
- "signed_headers": []byte("X-Date, Host"),
+ "signed_headers": []byte(" X-Date, , Host, "),+func TestTranslateApisixConsumer_HMACAuthRejectsInvalidMaxReqBody(t *testing.T) {
+ translator := NewTranslator(logr.Discard())
+ tctx := provider.NewDefaultTranslateContext(context.Background())
+ tctx.Secrets[k8stypes.NamespacedName{Namespace: "default", Name: "hmac"}] = &corev1.Secret{
+ Data: map[string][]byte{
+ "key_id": []byte("my-key"),
+ "secret_key": []byte("my-secret"),
+ "max_req_body": []byte("invalid"),
+ },
+ }
+
+ _, err := translator.TranslateApisixConsumer(tctx, hmacConsumerWithSecret("hmac"))
+ require.Error(t, err)
+ require.Contains(t, err.Error(), "max_req_body")
+}🤖 Prompt for 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.
In `@internal/adc/translator/apisixconsumer_test.go` around lines 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.
Contributor
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-19T05:11:06Z"
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-19T05:11:43Z"
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
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:
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-19T05:28:57Z"
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. |
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.
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
Bug Fixes
Tests