fix: redact credentials and AdminKey from controller logs#435
Open
shreemaan-abhishek wants to merge 1 commit into
Open
fix: redact credentials and AdminKey from controller logs#435shreemaan-abhishek wants to merge 1 commit into
shreemaan-abhishek wants to merge 1 commit into
Conversation
Whole-object logging serialized secret-bearing material into controller stdout on routine paths: - The ADC request body was logged at V(1) with the plaintext AdminKey (ADCServerOpts.Token) and full config resources. - Task/args logging at ERROR and V(1) dumped TLS private keys and consumer credentials (adctypes.Resources). - Controllers logged full ApisixConsumer / Consumer / ApisixRoute / ApisixGlobalRule / HTTPRoute objects, whose specs can carry inline plugin credentials. - The translator logged raw credential/plugin blobs on unmarshal errors. - The store logged full global-rule plugin config. Fixes: - Add MarshalLog (logr.Marshaler) to adctypes.Resources, client.Task and ADCServerRequest so logging emits identity/counts and [REDACTED] for the token, never secret bodies. Wire JSON is unchanged. - Log only NamespacedName / identity at the controller, provider, translator and store sites. - Add tests asserting secrets never reach the real console-encoder log output while identity still does.
|
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 (12)
📝 WalkthroughWalkthroughLogging output now redacts sensitive ADC data, summarizes resource collections, adds structured translation context, and uses namespaced identifiers for Kubernetes objects. Request payloads and reconciliation behavior remain unchanged. ChangesLogging hygiene
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
conformance test report - apisix-standalone modeapiVersion: gateway.networking.k8s.io/v1
date: "2026-07-17T11:36: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:
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-17T11:36: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:
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-17T11:55:26Z"
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:
failedTests:
- HTTPRouteBackendProtocolWebSocket
result: failure
skippedTests:
- HTTPRouteRedirectPortAndScheme
statistics:
Failed: 1
Passed: 10
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 failed with 1 test
failures.
- core:
failedTests:
- GatewayModifyListeners
- TLSRouteSimpleSameNamespace
result: failure
statistics:
Failed: 2
Passed: 9
Skipped: 0
name: GATEWAY-TLS
summary: Core tests failed with 2 test failures. |
nic-6443
approved these changes
Jul 23, 2026
| // MarshalLog implements logr.Marshaler so logging Resources emits only counts, | ||
| // never the secret-bearing bodies (SSL private keys, consumer credentials). | ||
| // It affects logging only, not the JSON sent to the data plane. | ||
| func (r *Resources) MarshalLog() any { |
Collaborator
There was a problem hiding this comment.
Diffed this against the OSS half (apache/apisix-ingress-controller#2808) — identical apart from hunk offsets, so the sync side is fine.
Mechanism verified: I built the production logger (zapr + zap console encoder) into a buffer and confirmed MarshalLog is honoured at all three sites, the token comes out [REDACTED], and the wire payload is untouched. The remaining "config", config / "configs", configs sites in client.go are already safe too — Config.MarshalJSON gets picked up by zap's reflect encoder, so the AdminKey doesn't survive there.
One sharp edge on this receiver: it's on *Resources, so a Resources value doesn't satisfy logr.Marshaler and silently falls back to reflection, which dumps everything:
log.V(1).Info("resources", "resources", *someResources)
// {"resources": {"consumers":[{"plugins":{"key-auth":{"key":"SUPER-SECRET"}}...
No current call site does that (Task.Resources is a pointer, ADCServerRequest.MarshalLog calls it explicitly), so nothing broken — but a value receiver would keep it safe under refactoring, since the pointer method set includes it either way.
Out of scope for this PR, but same class of leak: translator/httproute.go still logs the decoded plugin map from a PluginConfig extensionRef at V(1), and translator/gateway.go does the same for GatewayProxy plugins plus the raw plugin_metadata blob on unmarshal error. Worth a follow-up?
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
Several controller log sites serialize whole, secret-bearing objects, so
credentials end up in controller stdout / log aggregation on routine paths
(a weaker boundary than
secrets/getRBAC). This redacts them.Leaks fixed
ADCServerOpts.Token): the ADC request body was logged verbatimat
V(1)inbuildHTTPRequest, printing the plaintext admin key.adctypes.Resources):Tasklogging at ERROR (
store insert/delete failed) andV(1)(updating config)dumped
SSL.Certificates[].Keyand consumer credential material.ApisixConsumer,Consumer,ApisixRoute,ApisixGlobalRuleand filteredHTTPRouteobjects,whose specs can carry inline plugin config (key-auth key, hmac/jwt secrets).
credential/plugin JSON on unmarshal errors.
(logger-plugin tokens).
Approach
MarshalLog(logr.Marshaler) toadctypes.Resources,client.TaskandADCServerRequest. These emit identity + counts, and[REDACTED]for thetoken, never secret bodies.
MarshalLogaffects logging only - the JSONactually sent to the data plane is untouched (a
MarshalJSON-based redactionwould have corrupted the wire payload, which is why it is not used here).
NamespacedName/ identity at the controller, provider, translatorand store sites.
Tests
internal/adc/client/redaction_test.gobuilds the production logger(
zapr+ zap console encoder) into a buffer, logs aTaskand anADCServerRequestpopulated with a TLS key, a consumer credential and an adminkey, and asserts none of the secrets appear in the output while identity still
does. It also asserts the request's real
Tokenfield is unchanged afterlogging.
Sync
This is the enterprise-side half of a shared fix. The upstream OSS PR is
apache/apisix-ingress-controller#2808; both must merge to complete the sync.
The two sides carry identical hunks.
Summary by CodeRabbit