Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions api/adc/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion internal/adc/cache/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
18 changes: 18 additions & 0 deletions internal/adc/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 16 additions & 0 deletions internal/adc/client/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
116 changes: 116 additions & 0 deletions internal/adc/client/redaction_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
9 changes: 7 additions & 2 deletions internal/adc/translator/consumer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
}
Expand Down
6 changes: 3 additions & 3 deletions internal/controller/apisixconsumer_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion internal/controller/apisixglobalrule_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions internal/controller/apisixroute_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
}

Expand Down
4 changes: 2 additions & 2 deletions internal/controller/consumer_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion internal/controller/httproute_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion internal/provider/apisix/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading