diff --git a/api/adc/types.go b/api/adc/types.go index 1c6d46e4..4149be1d 100644 --- a/api/adc/types.go +++ b/api/adc/types.go @@ -61,6 +61,23 @@ type Resources struct { SSLs []*SSL `json:"ssls,omitempty" yaml:"ssls,omitempty"` } +// 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 { + if r == nil { + return nil + } + return map[string]int{ + "consumerGroups": len(r.ConsumerGroups), + "consumers": len(r.Consumers), + "globalRules": len(r.GlobalRules), + "pluginMetadata": len(r.PluginMetadata), + "services": len(r.Services), + "ssls": len(r.SSLs), + } +} + type GlobalRule Plugins func (g *GlobalRule) DeepCopy() GlobalRule { diff --git a/internal/adc/cache/store.go b/internal/adc/cache/store.go index 6c5a1f23..b27ab24a 100644 --- a/internal/adc/cache/store.go +++ b/internal/adc/cache/store.go @@ -230,7 +230,7 @@ func (s *Store) GetResources(name string) (*adctypes.Resources, error) { } globalrule = adctypes.GlobalRule(merged) } - s.log.V(1).Info("GetResources fetched global rule items", "items", globalRuleItems, "gobalrule", globalrule) + s.log.V(1).Info("GetResources fetched global rule items", "itemCount", len(globalRuleItems), "pluginCount", len(globalrule)) if meta, ok := s.pluginMetadataMap[name]; ok { metadata = meta.DeepCopy() } diff --git a/internal/adc/client/client.go b/internal/adc/client/client.go index 8b75236b..b1ff2272 100644 --- a/internal/adc/client/client.go +++ b/internal/adc/client/client.go @@ -84,6 +84,24 @@ type Task struct { Resources *adctypes.Resources } +// MarshalLog implements logr.Marshaler so logging a Task never dumps the +// secret-bearing Resources bodies (SSL private keys, consumer credentials). +// Configs redact their own Token via Config.MarshalJSON. +func (t Task) MarshalLog() any { + configNames := make([]string, 0, len(t.Configs)) + for _, cfg := range t.Configs { + configNames = append(configNames, cfg.Name) + } + return map[string]any{ + "key": t.Key, + "name": t.Name, + "labels": t.Labels, + "resourceTypes": t.ResourceTypes, + "configs": configNames, + "resources": t.Resources.MarshalLog(), + } +} + type StoreDelta struct { Deleted map[types.NamespacedNameKind]adctypes.Config Applied map[types.NamespacedNameKind]adctypes.Config diff --git a/internal/adc/client/executor.go b/internal/adc/client/executor.go index 5664b4f2..7223b59b 100644 --- a/internal/adc/client/executor.go +++ b/internal/adc/client/executor.go @@ -82,6 +82,22 @@ type ADCServerOpts struct { CacheKey string `json:"cacheKey"` } +// MarshalLog implements logr.Marshaler so logging the request body redacts the +// AdminKey token and the secret-bearing config resources. It affects logging +// only, not the JSON actually sent to the ADC server. +func (r ADCServerRequest) MarshalLog() any { + return map[string]any{ + "backend": r.Task.Opts.Backend, + "server": r.Task.Opts.Server, + "token": "[REDACTED]", + "labelSelector": r.Task.Opts.LabelSelector, + "includeResourceType": r.Task.Opts.IncludeResourceType, + "tlsSkipVerify": r.Task.Opts.TlsSkipVerify, + "cacheKey": r.Task.Opts.CacheKey, + "config": r.Task.Config.MarshalLog(), + } +} + type ADCValidateResult struct { Success *bool `json:"success,omitempty"` ErrorMessage string `json:"message,omitempty"` diff --git a/internal/adc/client/redaction_test.go b/internal/adc/client/redaction_test.go new file mode 100644 index 00000000..0cbb1783 --- /dev/null +++ b/internal/adc/client/redaction_test.go @@ -0,0 +1,116 @@ +// 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 client + +import ( + "bytes" + "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" + + adctypes "github.com/apache/apisix-ingress-controller/api/adc" + "github.com/apache/apisix-ingress-controller/internal/types" +) + +// 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 ( + secretTLSKey = "-----BEGIN-PRIVATE-KEY-SUPER-SECRET-----" + secretCredKey = "SUPER-SECRET-KEYAUTH-KEY" + secretAdminKey = "SUPER-SECRET-ADMINKEY" +) + +func secretResources() *adctypes.Resources { + return &adctypes.Resources{ + SSLs: []*adctypes.SSL{{ + Metadata: adctypes.Metadata{Name: "ssl-1"}, + Certificates: []adctypes.Certificate{{Certificate: "cert", Key: secretTLSKey}}, + }}, + Consumers: []*adctypes.Consumer{{ + Username: "alice", + Plugins: adctypes.Plugins{"key-auth": map[string]any{"key": secretCredKey}}, + }}, + } +} + +// FINDING-016/037: logging a Task must not leak TLS private keys or consumer +// credentials, while still emitting identity for debugging. +func TestTaskMarshalLogRedactsSecrets(t *testing.T) { + var buf bytes.Buffer + log := bufferLogger(&buf) + + task := Task{ + Key: types.NamespacedNameKind{Namespace: "ns", Name: "route-1", Kind: "ApisixRoute"}, + Name: "ns/route-1", + Configs: map[types.NamespacedNameKind]adctypes.Config{ + {}: {Name: "gw", Token: secretAdminKey, ServerAddrs: []string{"http://x"}}, + }, + ResourceTypes: []string{"ssl", "consumer"}, + Resources: secretResources(), + } + log.Error(assert.AnError, "store insert failed", "args", task) + + out := buf.String() + assert.NotContains(t, out, secretTLSKey, "TLS private key leaked") + assert.NotContains(t, out, secretCredKey, "consumer credential leaked") + assert.NotContains(t, out, secretAdminKey, "admin key leaked") + assert.Contains(t, out, "route-1", "identity should still be logged") +} + +// FINDING-023: logging the ADC request body must redact the AdminKey token and +// the secret-bearing config, without altering what is sent to the server. +func TestADCServerRequestMarshalLogRedactsToken(t *testing.T) { + var buf bytes.Buffer + log := bufferLogger(&buf) + + reqBody := ADCServerRequest{ + Task: ADCServerTask{ + Opts: ADCServerOpts{ + Backend: "apisix", + Server: []string{"http://x"}, + Token: secretAdminKey, + CacheKey: "gw", + }, + Config: *secretResources(), + }, + } + log.V(1).Info("prepared request body", "body", reqBody) + + out := buf.String() + assert.NotContains(t, out, secretAdminKey, "admin key leaked") + assert.NotContains(t, out, secretTLSKey, "TLS private key leaked") + assert.NotContains(t, out, secretCredKey, "consumer credential leaked") + assert.Contains(t, out, "[REDACTED]", "token should be redacted") + + // The real wire payload must be untouched. + require.Equal(t, secretAdminKey, reqBody.Task.Opts.Token) +} diff --git a/internal/adc/translator/consumer.go b/internal/adc/translator/consumer.go index 93601fc3..0d73b483 100644 --- a/internal/adc/translator/consumer.go +++ b/internal/adc/translator/consumer.go @@ -63,7 +63,10 @@ func (t *Translator) TranslateConsumerV1alpha1(tctx *provider.TranslateContext, } else { authConfig := make(map[string]any) if err := json.Unmarshal(credentialSpec.Config.Raw, &authConfig); err != nil { - t.Log.Error(err, "failed to unmarshal credential config", "credential", credentialSpec) + t.Log.Error(err, "failed to unmarshal credential config", + "consumer", consumerV.Namespace+"/"+consumerV.Name, + "credential", credentialSpec.Name, + "type", credentialSpec.Type) continue } credential.Config = authConfig @@ -78,7 +81,9 @@ func (t *Translator) TranslateConsumerV1alpha1(tctx *provider.TranslateContext, pluginConfig := make(map[string]any) if len(plugin.Config.Raw) > 0 { if err := json.Unmarshal(plugin.Config.Raw, &pluginConfig); err != nil { - t.Log.Error(err, "failed to unmarshal plugin config", "plugin", plugin) + t.Log.Error(err, "failed to unmarshal plugin config", + "consumer", consumerV.Namespace+"/"+consumerV.Name, + "plugin", plugin.Name) continue } } diff --git a/internal/controller/apisixconsumer_controller.go b/internal/controller/apisixconsumer_controller.go index 5e471bd1..dbcfd024 100644 --- a/internal/controller/apisixconsumer_controller.go +++ b/internal/controller/apisixconsumer_controller.go @@ -75,7 +75,7 @@ func (r *ApisixConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Reque APIVersion: apiv2.GroupVersion.String(), } if err := r.Provider.Delete(ctx, ac); err != nil { - r.Log.Error(err, "failed to delete provider", "ApisixConsumer", ac) + r.Log.Error(err, "failed to delete provider", "ApisixConsumer", utils.NamespacedName(ac)) return ctrl.Result{}, err } return ctrl.Result{}, nil @@ -104,12 +104,12 @@ func (r *ApisixConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Reque } if err = r.processSpec(ctx, tctx, ac); err != nil { - r.Log.Error(err, "failed to process ApisixConsumer spec", "object", ac) + r.Log.Error(err, "failed to process ApisixConsumer spec", "object", utils.NamespacedName(ac)) return ctrl.Result{}, client.IgnoreNotFound(err) } if err = r.Provider.Update(ctx, tctx, ac); err != nil { - r.Log.Error(err, "failed to update provider", "ApisixConsumer", ac) + r.Log.Error(err, "failed to update provider", "ApisixConsumer", utils.NamespacedName(ac)) return ctrl.Result{}, err } return ctrl.Result{}, nil diff --git a/internal/controller/apisixglobalrule_controller.go b/internal/controller/apisixglobalrule_controller.go index fabc60c2..d50d3367 100644 --- a/internal/controller/apisixglobalrule_controller.go +++ b/internal/controller/apisixglobalrule_controller.go @@ -84,7 +84,7 @@ func (r *ApisixGlobalRuleReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, err } - r.Log.V(1).Info("reconciling ApisixGlobalRule", "object", globalRule) + r.Log.V(1).Info("reconciling ApisixGlobalRule", "object", utils.NamespacedName(&globalRule)) // create a translate context tctx := provider.NewDefaultTranslateContext(ctx) diff --git a/internal/controller/apisixroute_controller.go b/internal/controller/apisixroute_controller.go index dbb7c76a..db17a320 100644 --- a/internal/controller/apisixroute_controller.go +++ b/internal/controller/apisixroute_controller.go @@ -141,7 +141,7 @@ func (r *ApisixRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) } if err := r.Provider.Delete(ctx, &ar); err != nil { - r.Log.Error(err, "failed to delete apisixroute", "apisixroute", ar) + r.Log.Error(err, "failed to delete apisixroute", "apisixroute", utils.NamespacedName(&ar)) return ctrl.Result{}, err } return ctrl.Result{}, nil @@ -160,7 +160,7 @@ func (r *ApisixRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) "ingressClassName", ar.Spec.IngressClassName, "error", err.Error()) if err := r.Provider.Delete(ctx, &ar); err != nil { - r.Log.Error(err, "failed to delete apisixroute", "apisixroute", ar) + r.Log.Error(err, "failed to delete apisixroute", "apisixroute", utils.NamespacedName(&ar)) return ctrl.Result{}, err } return ctrl.Result{}, nil @@ -179,7 +179,7 @@ func (r *ApisixRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) Reason: string(apiv2.ConditionReasonSyncFailed), Message: err.Error(), } - r.Log.Error(err, "failed to process", "apisixroute", ar) + r.Log.Error(err, "failed to process", "apisixroute", utils.NamespacedName(&ar)) return ctrl.Result{}, err } diff --git a/internal/controller/consumer_controller.go b/internal/controller/consumer_controller.go index cfb1b296..5ef72bbf 100644 --- a/internal/controller/consumer_controller.go +++ b/internal/controller/consumer_controller.go @@ -230,12 +230,12 @@ func (r *ConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } if err := r.processSpec(ctx, tctx, consumer); err != nil { - r.Log.Error(err, "failed to process consumer spec", "consumer", consumer) + r.Log.Error(err, "failed to process consumer spec", "consumer", utils.NamespacedName(consumer)) statusErr = err } if err := r.Provider.Update(ctx, tctx, consumer); err != nil { - r.Log.Error(err, "failed to update consumer", "consumer", consumer) + r.Log.Error(err, "failed to update consumer", "consumer", utils.NamespacedName(consumer)) statusErr = err } diff --git a/internal/controller/httproute_controller.go b/internal/controller/httproute_controller.go index 216b50e1..c952b61c 100644 --- a/internal/controller/httproute_controller.go +++ b/internal/controller/httproute_controller.go @@ -268,7 +268,7 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if isRouteAccepted(gateways) && err == nil { routeToUpdate := hr if filteredHTTPRoute != nil { - r.Log.V(1).Info("filtered httproute", "httproute", filteredHTTPRoute) + r.Log.V(1).Info("filtered httproute", "httproute", utils.NamespacedName(filteredHTTPRoute)) routeToUpdate = filteredHTTPRoute } if err := r.Provider.Update(ctx, tctx, routeToUpdate); err != nil { diff --git a/internal/provider/apisix/provider.go b/internal/provider/apisix/provider.go index 791e1780..052cf04b 100644 --- a/internal/provider/apisix/provider.go +++ b/internal/provider/apisix/provider.go @@ -99,7 +99,7 @@ func (d *apisixProvider) Register(pathPrefix string, mux *http.ServeMux) { } func (d *apisixProvider) Update(ctx context.Context, tctx *provider.TranslateContext, obj client.Object) error { - d.log.V(1).Info("updating object", "object", obj) + d.log.V(1).Info("updating object", "object", utils.NamespacedNameKind(obj)) var ( result *translator.TranslateResult resourceTypes []string