From 7e3019cdcea08feb14bf6e9307fd8f4791bbb296 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 22 Sep 2026 19:10:10 +0545 Subject: [PATCH] fix: stop logging plugin config and whole HTTPRoute/Gateway objects Plugin config is arbitrary user JSON and routinely carries credentials (kafka SASL passwords, logger tokens, OIDC client secrets), but several sites still serialized whole plugin maps and CRD objects into the log sink: - GatewayProxy spec.plugins[].config, logged whole at V(1) when filling global rules. - GatewayProxy spec.pluginMetadata, logged raw at ERROR (default verbosity) when it fails to unmarshal, and parsed at V(1). - The full HTTPRoute on delete failure at ERROR, including inline RequestHeaderModifier header values. - The full Gateway on GatewayProxy processing failure at ERROR. Plugins, GlobalRule and PluginMetadata now implement logr.Marshaler and emit only sorted plugin names, so every present and future site logging a plugin map is covered centrally. MarshalLog affects logging only, not the JSON sent to the data plane. The object sites log identity instead. --- api/adc/redaction_test.go | 78 +++++++++++++++++++++ api/adc/types.go | 28 ++++++++ internal/adc/translator/gateway.go | 4 +- internal/controller/consumer_controller.go | 2 +- internal/controller/httproute_controller.go | 4 +- 5 files changed, 111 insertions(+), 5 deletions(-) create mode 100644 api/adc/redaction_test.go diff --git a/api/adc/redaction_test.go b/api/adc/redaction_test.go new file mode 100644 index 00000000..283efc0b --- /dev/null +++ b/api/adc/redaction_test.go @@ -0,0 +1,78 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package adc + +import ( + "bytes" + "encoding/json" + "testing" + + "github.com/go-logr/logr" + "github.com/go-logr/zapr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" +) + +// bufferLogger builds a logger identical to the production one (zapr + zap +// console encoder) but writing into buf, so we can assert on real log output. +func bufferLogger(buf *bytes.Buffer) logr.Logger { + core := zapcore.NewCore( + zapcore.NewConsoleEncoder(zap.NewDevelopmentEncoderConfig()), + zapcore.AddSync(buf), + zapcore.DebugLevel, + ) + return zapr.NewLogger(zap.New(core)) +} + +const secretPluginValue = "SUPER-SECRET-KAFKA-PASSWORD" + +func secretPluginMap() map[string]any { + return map[string]any{ + "kafka-logger": map[string]any{"sasl_config": map[string]any{"password": secretPluginValue}}, + "http-logger": map[string]any{"uri": "http://logs.example"}, + } +} + +// Plugin config is arbitrary user JSON that routinely carries credentials, so +// no plugin map may reach the log sink whole. +func TestPluginMapsMarshalLogEmitNamesOnly(t *testing.T) { + for name, value := range map[string]any{ + "plugins": Plugins(secretPluginMap()), + "globalRules": GlobalRule(secretPluginMap()), + "pluginMetadata": PluginMetadata(secretPluginMap()), + } { + t.Run(name, func(t *testing.T) { + var buf bytes.Buffer + bufferLogger(&buf).V(1).Info("site", name, value) + out := buf.String() + + assert.NotContains(t, out, secretPluginValue, "plugin config leaked into logs") + assert.Contains(t, out, "kafka-logger", "plugin name should survive for debugging") + assert.Contains(t, out, "http-logger") + }) + } +} + +// MarshalLog must not change what goes on the wire to the data plane. +func TestPluginMapsMarshalJSONUnaffected(t *testing.T) { + b, err := json.Marshal(Plugins(secretPluginMap())) + require.NoError(t, err) + assert.Contains(t, string(b), secretPluginValue) +} diff --git a/api/adc/types.go b/api/adc/types.go index f7adae1c..c7c67019 100644 --- a/api/adc/types.go +++ b/api/adc/types.go @@ -22,6 +22,7 @@ import ( "encoding/json" "errors" "fmt" + "sort" "strconv" "strings" "time" @@ -86,6 +87,11 @@ func (g *GlobalRule) DeepCopy() GlobalRule { return GlobalRule(copied) } +// MarshalLog implements logr.Marshaler. See Plugins.MarshalLog. +func (g GlobalRule) MarshalLog() any { + return pluginNames(g) +} + // +k8s:deepcopy-gen=true type GlobalRuleItem struct { Metadata `json:",inline" yaml:",inline"` @@ -101,6 +107,11 @@ func (p *PluginMetadata) DeepCopy() PluginMetadata { return PluginMetadata(copied) } +// MarshalLog implements logr.Marshaler. See Plugins.MarshalLog. +func (p PluginMetadata) MarshalLog() any { + return pluginNames(p) +} + // +k8s:deepcopy-gen=true type ConsumerGroup struct { Metadata `json:",inline" yaml:",inline"` @@ -400,6 +411,23 @@ func (p Plugins) DeepCopy() Plugins { return out } +// MarshalLog implements logr.Marshaler so logging a plugin map emits only the +// plugin names. Plugin config is arbitrary user JSON and routinely carries +// credentials (kafka SASL passwords, logger tokens, OIDC client secrets). +// It affects logging only, not the JSON sent to the data plane. +func (p Plugins) MarshalLog() any { + return pluginNames(p) +} + +func pluginNames(p map[string]any) []string { + names := make([]string, 0, len(p)) + for name := range p { + names = append(names, name) + } + sort.Strings(names) + return names +} + // UpstreamNode is the node in upstream type UpstreamNode struct { Host string `json:"host" yaml:"host"` diff --git a/internal/adc/translator/gateway.go b/internal/adc/translator/gateway.go index fd4d4dff..03739329 100644 --- a/internal/adc/translator/gateway.go +++ b/internal/adc/translator/gateway.go @@ -395,10 +395,10 @@ func (t *Translator) fillPluginMetadataFromGatewayProxy(pluginMetadata adctypes. for pluginName, plugin := range gatewayProxy.Spec.PluginMetadata { var pluginConfig map[string]any if err := json.Unmarshal(plugin.Raw, &pluginConfig); err != nil { - t.Log.Error(err, "gateway proxy plugin_metadata unmarshal failed", "plugin", pluginName, "config", string(plugin.Raw)) + t.Log.Error(err, "gateway proxy plugin_metadata unmarshal failed", "plugin", pluginName) continue } - t.Log.V(1).Info("fill plugin_metadata for gateway proxy", "plugin", pluginName, "config", pluginConfig) + t.Log.V(1).Info("fill plugin_metadata for gateway proxy", "plugin", pluginName) pluginMetadata[pluginName] = pluginConfig } } diff --git a/internal/controller/consumer_controller.go b/internal/controller/consumer_controller.go index f3fb80a9..230255a2 100644 --- a/internal/controller/consumer_controller.go +++ b/internal/controller/consumer_controller.go @@ -232,7 +232,7 @@ func (r *ConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c rk := utils.NamespacedNameKind(consumer) if err := ProcessGatewayProxy(r.Client, r.Log, tctx, gateway, rk); err != nil { - r.Log.Error(err, "failed to process gateway proxy", "gateway", gateway) + r.Log.Error(err, "failed to process gateway proxy", "gateway", utils.NamespacedName(gateway)) statusErr = err } diff --git a/internal/controller/httproute_controller.go b/internal/controller/httproute_controller.go index 8e0153bc..e34c2c40 100644 --- a/internal/controller/httproute_controller.go +++ b/internal/controller/httproute_controller.go @@ -174,7 +174,7 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } if err := r.Provider.Delete(ctx, hr); err != nil { - r.Log.Error(err, "failed to delete httproute", "httproute", hr) + r.Log.Error(err, "failed to delete httproute", "httproute", utils.NamespacedName(hr)) return ctrl.Result{}, err } return ctrl.Result{}, nil @@ -217,7 +217,7 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( APIVersion: gatewayv1.GroupVersion.String(), } if err := r.Provider.Delete(ctx, hr); err != nil { - r.Log.Error(err, "failed to delete httproute", "httproute", hr) + r.Log.Error(err, "failed to delete httproute", "httproute", utils.NamespacedName(hr)) return ctrl.Result{}, err } return ctrl.Result{}, nil