Skip to content

Commit 2dd2d89

Browse files
fix: redact credentials and AdminKey from controller logs
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.
1 parent 5f84a00 commit 2dd2d89

12 files changed

Lines changed: 186 additions & 14 deletions

File tree

api/adc/types.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,23 @@ type Resources struct {
6161
SSLs []*SSL `json:"ssls,omitempty" yaml:"ssls,omitempty"`
6262
}
6363

64+
// MarshalLog implements logr.Marshaler so logging Resources emits only counts,
65+
// never the secret-bearing bodies (SSL private keys, consumer credentials).
66+
// It affects logging only, not the JSON sent to the data plane.
67+
func (r *Resources) MarshalLog() any {
68+
if r == nil {
69+
return nil
70+
}
71+
return map[string]int{
72+
"consumerGroups": len(r.ConsumerGroups),
73+
"consumers": len(r.Consumers),
74+
"globalRules": len(r.GlobalRules),
75+
"pluginMetadata": len(r.PluginMetadata),
76+
"services": len(r.Services),
77+
"ssls": len(r.SSLs),
78+
}
79+
}
80+
6481
type GlobalRule Plugins
6582

6683
func (g *GlobalRule) DeepCopy() GlobalRule {

internal/adc/cache/store.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ func (s *Store) GetResources(name string) (*adctypes.Resources, error) {
230230
}
231231
globalrule = adctypes.GlobalRule(merged)
232232
}
233-
s.log.V(1).Info("GetResources fetched global rule items", "items", globalRuleItems, "gobalrule", globalrule)
233+
s.log.V(1).Info("GetResources fetched global rule items", "itemCount", len(globalRuleItems), "pluginCount", len(globalrule))
234234
if meta, ok := s.pluginMetadataMap[name]; ok {
235235
metadata = meta.DeepCopy()
236236
}

internal/adc/client/client.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,24 @@ type Task struct {
8484
Resources *adctypes.Resources
8585
}
8686

87+
// MarshalLog implements logr.Marshaler so logging a Task never dumps the
88+
// secret-bearing Resources bodies (SSL private keys, consumer credentials).
89+
// Configs redact their own Token via Config.MarshalJSON.
90+
func (t Task) MarshalLog() any {
91+
configNames := make([]string, 0, len(t.Configs))
92+
for _, cfg := range t.Configs {
93+
configNames = append(configNames, cfg.Name)
94+
}
95+
return map[string]any{
96+
"key": t.Key,
97+
"name": t.Name,
98+
"labels": t.Labels,
99+
"resourceTypes": t.ResourceTypes,
100+
"configs": configNames,
101+
"resources": t.Resources.MarshalLog(),
102+
}
103+
}
104+
87105
type StoreDelta struct {
88106
Deleted map[types.NamespacedNameKind]adctypes.Config
89107
Applied map[types.NamespacedNameKind]adctypes.Config

internal/adc/client/executor.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,22 @@ type ADCServerOpts struct {
8282
CacheKey string `json:"cacheKey"`
8383
}
8484

85+
// MarshalLog implements logr.Marshaler so logging the request body redacts the
86+
// AdminKey token and the secret-bearing config resources. It affects logging
87+
// only, not the JSON actually sent to the ADC server.
88+
func (r ADCServerRequest) MarshalLog() any {
89+
return map[string]any{
90+
"backend": r.Task.Opts.Backend,
91+
"server": r.Task.Opts.Server,
92+
"token": "[REDACTED]",
93+
"labelSelector": r.Task.Opts.LabelSelector,
94+
"includeResourceType": r.Task.Opts.IncludeResourceType,
95+
"tlsSkipVerify": r.Task.Opts.TlsSkipVerify,
96+
"cacheKey": r.Task.Opts.CacheKey,
97+
"config": r.Task.Config.MarshalLog(),
98+
}
99+
}
100+
85101
type ADCValidateResult struct {
86102
Success *bool `json:"success,omitempty"`
87103
ErrorMessage string `json:"message,omitempty"`
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
package client
19+
20+
import (
21+
"bytes"
22+
"testing"
23+
24+
"github.com/go-logr/logr"
25+
"github.com/go-logr/zapr"
26+
"github.com/stretchr/testify/assert"
27+
"github.com/stretchr/testify/require"
28+
"go.uber.org/zap"
29+
"go.uber.org/zap/zapcore"
30+
31+
adctypes "github.com/apache/apisix-ingress-controller/api/adc"
32+
"github.com/apache/apisix-ingress-controller/internal/types"
33+
)
34+
35+
// bufferLogger builds a logger identical to the production one (zapr + zap
36+
// console encoder) but writing into buf, so we can assert on real log output.
37+
func bufferLogger(buf *bytes.Buffer) logr.Logger {
38+
core := zapcore.NewCore(
39+
zapcore.NewConsoleEncoder(zap.NewDevelopmentEncoderConfig()),
40+
zapcore.AddSync(buf),
41+
zapcore.DebugLevel,
42+
)
43+
return zapr.NewLogger(zap.New(core))
44+
}
45+
46+
const (
47+
secretTLSKey = "-----BEGIN-PRIVATE-KEY-SUPER-SECRET-----"
48+
secretCredKey = "SUPER-SECRET-KEYAUTH-KEY"
49+
secretAdminKey = "SUPER-SECRET-ADMINKEY"
50+
)
51+
52+
func secretResources() *adctypes.Resources {
53+
return &adctypes.Resources{
54+
SSLs: []*adctypes.SSL{{
55+
Metadata: adctypes.Metadata{Name: "ssl-1"},
56+
Certificates: []adctypes.Certificate{{Certificate: "cert", Key: secretTLSKey}},
57+
}},
58+
Consumers: []*adctypes.Consumer{{
59+
Username: "alice",
60+
Plugins: adctypes.Plugins{"key-auth": map[string]any{"key": secretCredKey}},
61+
}},
62+
}
63+
}
64+
65+
// FINDING-016/037: logging a Task must not leak TLS private keys or consumer
66+
// credentials, while still emitting identity for debugging.
67+
func TestTaskMarshalLogRedactsSecrets(t *testing.T) {
68+
var buf bytes.Buffer
69+
log := bufferLogger(&buf)
70+
71+
task := Task{
72+
Key: types.NamespacedNameKind{Namespace: "ns", Name: "route-1", Kind: "ApisixRoute"},
73+
Name: "ns/route-1",
74+
Configs: map[types.NamespacedNameKind]adctypes.Config{
75+
{}: {Name: "gw", Token: secretAdminKey, ServerAddrs: []string{"http://x"}},
76+
},
77+
ResourceTypes: []string{"ssl", "consumer"},
78+
Resources: secretResources(),
79+
}
80+
log.Error(assert.AnError, "store insert failed", "args", task)
81+
82+
out := buf.String()
83+
assert.NotContains(t, out, secretTLSKey, "TLS private key leaked")
84+
assert.NotContains(t, out, secretCredKey, "consumer credential leaked")
85+
assert.NotContains(t, out, secretAdminKey, "admin key leaked")
86+
assert.Contains(t, out, "route-1", "identity should still be logged")
87+
}
88+
89+
// FINDING-023: logging the ADC request body must redact the AdminKey token and
90+
// the secret-bearing config, without altering what is sent to the server.
91+
func TestADCServerRequestMarshalLogRedactsToken(t *testing.T) {
92+
var buf bytes.Buffer
93+
log := bufferLogger(&buf)
94+
95+
reqBody := ADCServerRequest{
96+
Task: ADCServerTask{
97+
Opts: ADCServerOpts{
98+
Backend: "apisix",
99+
Server: []string{"http://x"},
100+
Token: secretAdminKey,
101+
CacheKey: "gw",
102+
},
103+
Config: *secretResources(),
104+
},
105+
}
106+
log.V(1).Info("prepared request body", "body", reqBody)
107+
108+
out := buf.String()
109+
assert.NotContains(t, out, secretAdminKey, "admin key leaked")
110+
assert.NotContains(t, out, secretTLSKey, "TLS private key leaked")
111+
assert.NotContains(t, out, secretCredKey, "consumer credential leaked")
112+
assert.Contains(t, out, "[REDACTED]", "token should be redacted")
113+
114+
// The real wire payload must be untouched.
115+
require.Equal(t, secretAdminKey, reqBody.Task.Opts.Token)
116+
}

internal/adc/translator/consumer.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,10 @@ func (t *Translator) TranslateConsumerV1alpha1(tctx *provider.TranslateContext,
6363
} else {
6464
authConfig := make(map[string]any)
6565
if err := json.Unmarshal(credentialSpec.Config.Raw, &authConfig); err != nil {
66-
t.Log.Error(err, "failed to unmarshal credential config", "credential", credentialSpec)
66+
t.Log.Error(err, "failed to unmarshal credential config",
67+
"consumer", consumerV.Namespace+"/"+consumerV.Name,
68+
"credential", credentialSpec.Name,
69+
"type", credentialSpec.Type)
6770
continue
6871
}
6972
credential.Config = authConfig
@@ -78,7 +81,9 @@ func (t *Translator) TranslateConsumerV1alpha1(tctx *provider.TranslateContext,
7881
pluginConfig := make(map[string]any)
7982
if len(plugin.Config.Raw) > 0 {
8083
if err := json.Unmarshal(plugin.Config.Raw, &pluginConfig); err != nil {
81-
t.Log.Error(err, "failed to unmarshal plugin config", "plugin", plugin)
84+
t.Log.Error(err, "failed to unmarshal plugin config",
85+
"consumer", consumerV.Namespace+"/"+consumerV.Name,
86+
"plugin", plugin.Name)
8287
continue
8388
}
8489
}

internal/controller/apisixconsumer_controller.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func (r *ApisixConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Reque
7575
APIVersion: apiv2.GroupVersion.String(),
7676
}
7777
if err := r.Provider.Delete(ctx, ac); err != nil {
78-
r.Log.Error(err, "failed to delete provider", "ApisixConsumer", ac)
78+
r.Log.Error(err, "failed to delete provider", "ApisixConsumer", utils.NamespacedName(ac))
7979
return ctrl.Result{}, err
8080
}
8181
return ctrl.Result{}, nil
@@ -104,12 +104,12 @@ func (r *ApisixConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Reque
104104
}
105105

106106
if err = r.processSpec(ctx, tctx, ac); err != nil {
107-
r.Log.Error(err, "failed to process ApisixConsumer spec", "object", ac)
107+
r.Log.Error(err, "failed to process ApisixConsumer spec", "object", utils.NamespacedName(ac))
108108
return ctrl.Result{}, client.IgnoreNotFound(err)
109109
}
110110

111111
if err = r.Provider.Update(ctx, tctx, ac); err != nil {
112-
r.Log.Error(err, "failed to update provider", "ApisixConsumer", ac)
112+
r.Log.Error(err, "failed to update provider", "ApisixConsumer", utils.NamespacedName(ac))
113113
return ctrl.Result{}, err
114114
}
115115
return ctrl.Result{}, nil

internal/controller/apisixglobalrule_controller.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ func (r *ApisixGlobalRuleReconciler) Reconcile(ctx context.Context, req ctrl.Req
8484
return ctrl.Result{}, err
8585
}
8686

87-
r.Log.V(1).Info("reconciling ApisixGlobalRule", "object", globalRule)
87+
r.Log.V(1).Info("reconciling ApisixGlobalRule", "object", utils.NamespacedName(&globalRule))
8888

8989
// create a translate context
9090
tctx := provider.NewDefaultTranslateContext(ctx)

internal/controller/apisixroute_controller.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ func (r *ApisixRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request)
141141
}
142142

143143
if err := r.Provider.Delete(ctx, &ar); err != nil {
144-
r.Log.Error(err, "failed to delete apisixroute", "apisixroute", ar)
144+
r.Log.Error(err, "failed to delete apisixroute", "apisixroute", utils.NamespacedName(&ar))
145145
return ctrl.Result{}, err
146146
}
147147
return ctrl.Result{}, nil
@@ -160,7 +160,7 @@ func (r *ApisixRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request)
160160
"ingressClassName", ar.Spec.IngressClassName,
161161
"error", err.Error())
162162
if err := r.Provider.Delete(ctx, &ar); err != nil {
163-
r.Log.Error(err, "failed to delete apisixroute", "apisixroute", ar)
163+
r.Log.Error(err, "failed to delete apisixroute", "apisixroute", utils.NamespacedName(&ar))
164164
return ctrl.Result{}, err
165165
}
166166
return ctrl.Result{}, nil
@@ -179,7 +179,7 @@ func (r *ApisixRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request)
179179
Reason: string(apiv2.ConditionReasonSyncFailed),
180180
Message: err.Error(),
181181
}
182-
r.Log.Error(err, "failed to process", "apisixroute", ar)
182+
r.Log.Error(err, "failed to process", "apisixroute", utils.NamespacedName(&ar))
183183
return ctrl.Result{}, err
184184
}
185185

internal/controller/consumer_controller.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,12 +230,12 @@ func (r *ConsumerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
230230
}
231231

232232
if err := r.processSpec(ctx, tctx, consumer); err != nil {
233-
r.Log.Error(err, "failed to process consumer spec", "consumer", consumer)
233+
r.Log.Error(err, "failed to process consumer spec", "consumer", utils.NamespacedName(consumer))
234234
statusErr = err
235235
}
236236

237237
if err := r.Provider.Update(ctx, tctx, consumer); err != nil {
238-
r.Log.Error(err, "failed to update consumer", "consumer", consumer)
238+
r.Log.Error(err, "failed to update consumer", "consumer", utils.NamespacedName(consumer))
239239
statusErr = err
240240
}
241241

0 commit comments

Comments
 (0)