From 6e56929d08c91a9fb6979f463901099d98cbdb4e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 08:16:20 +0000 Subject: [PATCH 1/2] ROB-997 Redact Secret data from outbound notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CloudEvent notifications carried whole runtime.Objects. With Secret watching enabled, every Secret create, update and delete JSON-serialized the Secret's data and stringData to the configured receiver — and updates sent the previous values too, via oldObj. Add pkg/redact and apply it in two layers: - Typed, in the controller as events are built, so every handler is covered rather than just the ones that serialize objects today. A Secret's data/stringData values become "[redacted by kubewatch]". Objects are deep-copied first: they come from the shared informer cache and redacting one in place would corrupt it for every other reader. - Defensive, on the marshalled bytes in the CloudEvent handler, right before the POST. This walks the document and redacts the data fields of any object whose kind is Secret, at any depth. It catches what the typed layer cannot see — notably the unstructured Secrets reachable through `customresources`, which is not gated by `resource.secret`. A payload that fails the round-trip is an error, not a fall-back to the unredacted bytes. Key names, labels, annotations, type and the rest of the metadata are kept, so a notification still says which Secret changed and which keys it has. Non-Secret resources are untouched and keep their full object body, which is what downstream consumers match on. Redacted `data` values stay valid base64 in both layers, so receivers that decode them still can. Tests drive the real path — Kubernetes API to informer to controller to handler to HTTP — with sentinel bytes, and assert neither the raw nor the base64 form reaches the receiver on create, update or delete. Verified by mutation: disabling either layer, or redacting in place, each fails a distinct test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GzZp8kPXKYjXuxY1oN1aU8 --- README.md | 15 ++ pkg/controller/controller.go | 9 +- pkg/controller/controller_test.go | 219 +++++++++++++++++ pkg/handlers/cloudevent/cloudevent.go | 16 +- pkg/handlers/cloudevent/cloudevent_test.go | 239 +++++++++++++++++++ pkg/redact/redact.go | 178 ++++++++++++++ pkg/redact/redact_test.go | 258 +++++++++++++++++++++ 7 files changed, 928 insertions(+), 6 deletions(-) create mode 100644 pkg/controller/controller_test.go create mode 100644 pkg/redact/redact.go create mode 100644 pkg/redact/redact_test.go diff --git a/README.md b/README.md index ca45caca..735b3387 100644 --- a/README.md +++ b/README.md @@ -177,6 +177,21 @@ customresources: resource: prometheusrules ``` +#### Watching Secrets + +Secret watching is off by default (`secret: false`). When you turn it on, kubewatch +notifies on Secret creates, updates and deletes, but the secret material itself never +leaves the process: the values under a Secret's `data` and `stringData` are replaced +with `[redacted by kubewatch]` before any notification is built. This applies to the +previous version of the object too, which update notifications also carry, and to +Secrets reached through `customresources` rather than `secret: true`. + +Key names, labels, annotations, the Secret's type and the rest of its metadata are +kept, so a notification still tells you which Secret changed and which of its keys +were added or removed — it just does not tell you their values. Handlers that +serialize whole objects, such as `cloudevent`, redact again on the bytes they are +about to send. + #### Working with RBAC Kubernetes Engine clusters running versions 1.6 or higher introduced Role-Based Access Control (RBAC). We can create `ServiceAccount` for it to work with RBAC. diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 5bb907a3..ca884271 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -31,6 +31,7 @@ import ( "github.com/bitnami-labs/kubewatch/config" "github.com/bitnami-labs/kubewatch/pkg/event" "github.com/bitnami-labs/kubewatch/pkg/handlers" + "github.com/bitnami-labs/kubewatch/pkg/redact" "github.com/bitnami-labs/kubewatch/pkg/utils" "github.com/sirupsen/logrus" @@ -783,7 +784,7 @@ func (c *Controller) processItem(newEvent Event) error { ApiVersion: newEvent.apiVersion, Status: status, Reason: "Created", - Obj: newEvent.obj, + Obj: redact.Object(newEvent.obj), } c.eventHandler.Handle(kbEvent) return nil @@ -805,8 +806,8 @@ func (c *Controller) processItem(newEvent Event) error { ApiVersion: newEvent.apiVersion, Status: status, Reason: "Updated", - Obj: newEvent.obj, - OldObj: newEvent.oldObj, + Obj: redact.Object(newEvent.obj), + OldObj: redact.Object(newEvent.oldObj), } c.eventHandler.Handle(kbEvent) return nil @@ -818,7 +819,7 @@ func (c *Controller) processItem(newEvent Event) error { ApiVersion: newEvent.apiVersion, Status: "Danger", Reason: "Deleted", - Obj: newEvent.obj, + Obj: redact.Object(newEvent.obj), } c.eventHandler.Handle(kbEvent) return nil diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go new file mode 100644 index 00000000..3250f914 --- /dev/null +++ b/pkg/controller/controller_test.go @@ -0,0 +1,219 @@ +/* +Copyright 2016 Skippbox, Ltd. + +Licensed 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 controller + +import ( + "context" + "encoding/base64" + "encoding/json" + "strings" + "sync" + "testing" + "time" + + "github.com/bitnami-labs/kubewatch/config" + "github.com/bitnami-labs/kubewatch/pkg/event" + "github.com/prometheus/client_golang/prometheus" + api_v1 "k8s.io/api/core/v1" + meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/watch" + "k8s.io/client-go/kubernetes" + k8sfake "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/tools/cache" +) + +// sentinel is the secret material the controller must never hand to a handler. +const sentinel = "s3nt1nel-D0-N0T-D1SCL0SE" + +// recordingHandler stands in for a notification handler and keeps the events it +// was given, so a test can inspect exactly what the controller emitted. +type recordingHandler struct { + mutex sync.Mutex + events []event.Event +} + +func (h *recordingHandler) Init(*config.Config) error { return nil } + +func (h *recordingHandler) Handle(e event.Event) { + h.mutex.Lock() + defer h.mutex.Unlock() + h.events = append(h.events, e) +} + +func (h *recordingHandler) recorded() []event.Event { + h.mutex.Lock() + defer h.mutex.Unlock() + return append([]event.Event(nil), h.events...) +} + +// waitForEvents waits for the controller's worker to drain want events. +func (h *recordingHandler) waitForEvents(t *testing.T, want int) []event.Event { + t.Helper() + + deadline := time.Now().Add(15 * time.Second) + for time.Now().Before(deadline) { + if recorded := h.recorded(); len(recorded) >= want { + return recorded + } + time.Sleep(10 * time.Millisecond) + } + + recorded := h.recorded() + t.Fatalf("controller emitted %d events, want %d", len(recorded), want) + return recorded +} + +func secretInformer(client kubernetes.Interface) cache.SharedIndexInformer { + return cache.NewSharedIndexInformer( + &cache.ListWatch{ + ListFunc: func(options meta_v1.ListOptions) (runtime.Object, error) { + return client.CoreV1().Secrets("default").List(context.Background(), options) + }, + WatchFunc: func(options meta_v1.ListOptions) (watch.Interface, error) { + return client.CoreV1().Secrets("default").Watch(context.Background(), options) + }, + }, + &api_v1.Secret{}, + 0, + cache.Indexers{}, + ) +} + +// TestSecretEventsReachHandlersRedacted drives the real trigger path — Kubernetes +// API → informer → controller → handler — and asserts the Secret's bytes are +// already gone by the time any handler sees the event, on create, update and +// delete alike. The update case also covers OldObj, which carries the previous +// secret value. +func TestSecretEventsReachHandlersRedacted(t *testing.T) { + client := k8sfake.NewSimpleClientset() + handler := &recordingHandler{} + metrics := prometheus.NewCounterVec(prometheus.CounterOpts{Name: "test_events_total"}, []string{"resource", "type"}) + + controller := newResourceController(client, handler, secretInformer(client), "secret", V1, metrics) + stop := make(chan struct{}) + defer close(stop) + go controller.Run(stop) + + if !cache.WaitForCacheSync(stop, controller.HasSynced) { + t.Fatal("informer cache never synced") + } + + secret := &api_v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "creds", + Namespace: "default", + // The create path only notifies on objects newer than the + // controller's start time. + CreationTimestamp: meta_v1.NewTime(time.Now().Add(time.Minute)), + }, + Data: map[string][]byte{"password": []byte(sentinel)}, + StringData: map[string]string{"token": sentinel}, + } + + // Each change waits for its event before the next one is made. processItem + // re-reads the object from the informer cache, so a burst of changes lets a + // later one race an earlier one's notification out of existence — a + // pre-existing kubewatch behaviour, and not what this test is about. + created, err := client.CoreV1().Secrets("default").Create(context.Background(), secret, meta_v1.CreateOptions{}) + if err != nil { + t.Fatalf("creating secret: %v", err) + } + handler.waitForEvents(t, 1) + + updated := created.DeepCopy() + updated.Data["password"] = []byte(sentinel + "-ROTATED") + if _, err := client.CoreV1().Secrets("default").Update(context.Background(), updated, meta_v1.UpdateOptions{}); err != nil { + t.Fatalf("updating secret: %v", err) + } + handler.waitForEvents(t, 2) + + if err := client.CoreV1().Secrets("default").Delete(context.Background(), "creds", meta_v1.DeleteOptions{}); err != nil { + t.Fatalf("deleting secret: %v", err) + } + events := handler.waitForEvents(t, 3) + + sawReason := map[string]bool{} + for _, e := range events { + sawReason[e.Reason] = true + + // Serialize the event the way a handler would and look for the bytes. + payload, err := json.Marshal(map[string]interface{}{"obj": e.Obj, "oldObj": e.OldObj}) + if err != nil { + t.Fatalf("marshalling %s event: %v", e.Reason, err) + } + for form, encoded := range map[string]string{ + "raw": sentinel, + "base64": base64.StdEncoding.EncodeToString([]byte(sentinel)), + } { + if strings.Contains(string(payload), encoded) { + t.Errorf("%s event: %s secret bytes reached the handler: %s", e.Reason, form, payload) + } + } + + if e.Name != "creds" { + t.Errorf("%s event: Name = %q, want %q", e.Reason, e.Name, "creds") + } + } + + for _, reason := range []string{"Created", "Updated", "Deleted"} { + if !sawReason[reason] { + t.Errorf("no %s event was emitted; got %v", reason, sawReason) + } + } +} + +// TestSecretRedactionLeavesInformerCacheIntact guards the shared informer cache. +// Redacting an object in place would corrupt the cache for every other reader in +// the process, so the controller has to copy before it redacts. +func TestSecretRedactionLeavesInformerCacheIntact(t *testing.T) { + client := k8sfake.NewSimpleClientset() + handler := &recordingHandler{} + metrics := prometheus.NewCounterVec(prometheus.CounterOpts{Name: "cache_test_events_total"}, []string{"resource", "type"}) + + informer := secretInformer(client) + controller := newResourceController(client, handler, informer, "secret", V1, metrics) + stop := make(chan struct{}) + defer close(stop) + go controller.Run(stop) + + if !cache.WaitForCacheSync(stop, controller.HasSynced) { + t.Fatal("informer cache never synced") + } + + _, err := client.CoreV1().Secrets("default").Create(context.Background(), &api_v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "creds", + Namespace: "default", + CreationTimestamp: meta_v1.NewTime(time.Now().Add(time.Minute)), + }, + Data: map[string][]byte{"password": []byte(sentinel)}, + }, meta_v1.CreateOptions{}) + if err != nil { + t.Fatalf("creating secret: %v", err) + } + + handler.waitForEvents(t, 1) + + cached, exists, err := informer.GetIndexer().GetByKey("default/creds") + if err != nil || !exists { + t.Fatalf("secret not in informer cache (exists=%v): %v", exists, err) + } + if got := string(cached.(*api_v1.Secret).Data["password"]); got != sentinel { + t.Errorf("informer cache was mutated: Data[password] = %q, want %q", got, sentinel) + } +} diff --git a/pkg/handlers/cloudevent/cloudevent.go b/pkg/handlers/cloudevent/cloudevent.go index 95ba1b4e..444dc29d 100644 --- a/pkg/handlers/cloudevent/cloudevent.go +++ b/pkg/handlers/cloudevent/cloudevent.go @@ -30,6 +30,7 @@ import ( "github.com/bitnami-labs/kubewatch/pkg/event" "github.com/bitnami-labs/kubewatch/pkg/filter" "github.com/bitnami-labs/kubewatch/pkg/metrics" + "github.com/bitnami-labs/kubewatch/pkg/redact" "k8s.io/apimachinery/pkg/runtime" ) @@ -142,8 +143,11 @@ func (m *CloudEvent) prepareMessage(e event.Event) *CloudEventMessage { ApiVersion: e.ApiVersion, ClusterUid: "TODO", Description: e.Message(), - Obj: e.Obj, - OldObj: e.OldObj, + // The controller already redacts these, but this handler serializes + // whole objects to an off-cluster receiver, so it redacts again + // rather than trusting its caller. + Obj: redact.Object(e.Obj), + OldObj: redact.Object(e.OldObj), }, } } @@ -167,6 +171,14 @@ func (m *CloudEvent) postMessage(webhookMessage *CloudEventMessage) error { return err } + // Defensive last pass over the actual wire bytes: catches Secrets the typed + // layer cannot recognise, notably the unstructured Secrets an operator can + // reach through `customresources` without enabling `resource.secret`. + message, err = redact.JSON(message) + if err != nil { + return fmt.Errorf("failed to redact outbound message, not sending it: %v", err) + } + req, err := http.NewRequest("POST", m.Url, bytes.NewBuffer(message)) if err != nil { return err diff --git a/pkg/handlers/cloudevent/cloudevent_test.go b/pkg/handlers/cloudevent/cloudevent_test.go index 790fd274..a3d54c25 100644 --- a/pkg/handlers/cloudevent/cloudevent_test.go +++ b/pkg/handlers/cloudevent/cloudevent_test.go @@ -17,11 +17,22 @@ limitations under the License. package cloudevent import ( + "encoding/base64" + "encoding/json" "fmt" + "io" + "net/http" + "net/http/httptest" "reflect" + "strings" "testing" "github.com/bitnami-labs/kubewatch/config" + "github.com/bitnami-labs/kubewatch/pkg/event" + "github.com/bitnami-labs/kubewatch/pkg/filter" + api_v1 "k8s.io/api/core/v1" + meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) func TestCloudEventInit(t *testing.T) { @@ -44,3 +55,231 @@ func TestCloudEventInit(t *testing.T) { } } } + +// sentinel is the secret material the disclosure tests look for on the wire. It +// must never appear in an outbound CloudEvent, in any encoding. +const sentinel = "s3nt1nel-D0-N0T-D1SCL0SE" + +// captureCloudEvents stands in for the CloudEvent receiver and returns the raw +// bodies it was POSTed, so assertions run against the actual wire bytes. +func captureCloudEvents(t *testing.T) (*CloudEvent, *[]string) { + t.Helper() + + var bodies []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("reading request body: %v", err) + return + } + bodies = append(bodies, string(body)) + })) + t.Cleanup(server.Close) + + return &CloudEvent{Url: server.URL, Filter: filter.NewFilter()}, &bodies +} + +func sentinelSecret(suffix string) *api_v1.Secret { + value := sentinel + suffix + return &api_v1.Secret{ + TypeMeta: meta_v1.TypeMeta{Kind: "Secret", APIVersion: "v1"}, + ObjectMeta: meta_v1.ObjectMeta{Name: "creds", Namespace: "default"}, + Data: map[string][]byte{ + "password": []byte(value), + "tls.key": []byte(value), + }, + StringData: map[string]string{"token": value}, + } +} + +// TestHandleNeverDisclosesSecretData drives the create, update and delete paths +// and asserts the Secret's bytes never reach the receiver. The update case also +// covers oldObj, which carries the *previous* secret value. +func TestHandleNeverDisclosesSecretData(t *testing.T) { + handler, bodies := captureCloudEvents(t) + + current := sentinelSecret("-CURRENT") + previous := sentinelSecret("-PREVIOUS") + + events := []event.Event{ + {Kind: "secret", Name: "creds", Namespace: "default", Reason: "Created", Obj: current}, + {Kind: "secret", Name: "creds", Namespace: "default", Reason: "Updated", Obj: current, OldObj: previous}, + {Kind: "secret", Name: "creds", Namespace: "default", Reason: "Deleted", Obj: current}, + } + for _, e := range events { + handler.Handle(e) + } + + if len(*bodies) != len(events) { + t.Fatalf("receiver got %d messages, want %d", len(*bodies), len(events)) + } + + for i, body := range *bodies { + for form, encoded := range map[string]string{ + "raw": sentinel, + "base64": base64.StdEncoding.EncodeToString([]byte(sentinel)), + } { + if strings.Contains(body, encoded) { + t.Errorf("message %d (%s): %s secret bytes disclosed: %s", i, events[i].Reason, form, body) + } + } + + // The notification itself must still be useful. + if !strings.Contains(body, `"creds"`) || !strings.Contains(body, `"default"`) { + t.Errorf("message %d (%s): lost the metadata it is a notification about: %s", i, events[i].Reason, body) + } + } +} + +// TestHandleRedactsUnstructuredSecret covers the `customresources` path, which +// yields unstructured objects and is not gated by the `resource.secret` flag. +func TestHandleRedactsUnstructuredSecret(t *testing.T) { + handler, bodies := captureCloudEvents(t) + + object := &unstructured.Unstructured{Object: map[string]interface{}{ + "kind": "Secret", + "apiVersion": "v1", + "metadata": map[string]interface{}{"name": "creds", "namespace": "default"}, + "data": map[string]interface{}{"password": base64.StdEncoding.EncodeToString([]byte(sentinel))}, + "stringData": map[string]interface{}{"token": sentinel}, + }} + + handler.Handle(event.Event{Kind: "secrets", Name: "creds", Namespace: "default", Reason: "Created", Obj: object}) + + if len(*bodies) != 1 { + t.Fatalf("receiver got %d messages, want 1", len(*bodies)) + } + body := (*bodies)[0] + if strings.Contains(body, sentinel) { + t.Errorf("raw secret bytes disclosed: %s", body) + } + if strings.Contains(body, base64.StdEncoding.EncodeToString([]byte(sentinel))) { + t.Errorf("base64 secret bytes disclosed: %s", body) + } +} + +// TestHandleKeepsNonSecretObjects guards the other half of the contract: +// redaction must not cost non-Secret resources their full object body, which is +// what downstream consumers match on. +func TestHandleKeepsNonSecretObjects(t *testing.T) { + handler, bodies := captureCloudEvents(t) + + pod := &api_v1.Pod{ + TypeMeta: meta_v1.TypeMeta{Kind: "Pod", APIVersion: "v1"}, + ObjectMeta: meta_v1.ObjectMeta{Name: "web", Namespace: "default", Labels: map[string]string{"app": "web"}}, + Spec: api_v1.PodSpec{ + NodeName: "node-1", + Containers: []api_v1.Container{{Name: "app", Image: "nginx:1.27"}}, + }, + Status: api_v1.PodStatus{Phase: api_v1.PodRunning}, + } + + handler.Handle(event.Event{Kind: "pod", Name: "web", Namespace: "default", ApiVersion: "v1", Reason: "Updated", Obj: pod, OldObj: pod}) + + if len(*bodies) != 1 { + t.Fatalf("receiver got %d messages, want 1", len(*bodies)) + } + + // CloudEventMessage cannot be unmarshalled back into (Obj is an interface), + // so assert on the wire shape a receiver actually sees. + var message struct { + Data struct { + Operation string `json:"operation"` + Kind string `json:"kind"` + Obj map[string]interface{} `json:"obj"` + OldObj map[string]interface{} `json:"oldObj"` + } `json:"data"` + } + if err := json.Unmarshal([]byte((*bodies)[0]), &message); err != nil { + t.Fatalf("unmarshalling message: %v", err) + } + if message.Data.Operation != "update" || message.Data.Kind != "pod" { + t.Errorf("event metadata wrong: %+v", message.Data) + } + for name, object := range map[string]map[string]interface{}{"obj": message.Data.Obj, "oldObj": message.Data.OldObj} { + if object["spec"] == nil || object["status"] == nil || object["metadata"] == nil { + t.Errorf("%s is not a full object: %+v", name, object) + } + } + + // Spot-check fields from across the object rather than only its metadata. + for _, want := range []string{`"nginx:1.27"`, `"node-1"`, `"Running"`, `"app":"web"`, `"oldObj"`} { + if !strings.Contains((*bodies)[0], want) { + t.Errorf("non-Secret object lost %s: %s", want, (*bodies)[0]) + } + } +} + +// TestHandleDoesNotMutateInformerCacheObject guards the shared informer cache: +// the objects handed to a handler are the cache's own, and redacting one in +// place would corrupt every other reader in the process. +func TestHandleDoesNotMutateInformerCacheObject(t *testing.T) { + handler, _ := captureCloudEvents(t) + + cached := sentinelSecret("-CACHED") + handler.Handle(event.Event{Kind: "secret", Name: "creds", Namespace: "default", Reason: "Updated", Obj: cached, OldObj: cached}) + + if got := string(cached.Data["password"]); got != sentinel+"-CACHED" { + t.Errorf("informer cache object was mutated: Data[password] = %q", got) + } + if got := cached.StringData["token"]; got != sentinel+"-CACHED" { + t.Errorf("informer cache object was mutated: StringData[token] = %q", got) + } +} + +// TestHandleKeepsEnvelopeIntactForSecrets pins down a sharp edge: objName() names +// the resource type "Secret", so the envelope's own data.kind is the literal +// string the defensive redaction layer looks for. The envelope must come out +// whole anyway — only a Secret object's data fields are ever redacted. +func TestHandleKeepsEnvelopeIntactForSecrets(t *testing.T) { + handler, bodies := captureCloudEvents(t) + + handler.Handle(event.Event{ + Kind: "Secret", Name: "creds", Namespace: "default", ApiVersion: "v1", + Reason: "Updated", Obj: sentinelSecret("-CURRENT"), OldObj: sentinelSecret("-PREVIOUS"), + }) + + if len(*bodies) != 1 { + t.Fatalf("receiver got %d messages, want 1", len(*bodies)) + } + + var message struct { + SpecVersion string `json:"specversion"` + Type string `json:"type"` + ID string `json:"id"` + Data struct { + Operation string `json:"operation"` + Kind string `json:"kind"` + ApiVersion string `json:"apiVersion"` + ClusterUid string `json:"clusterUid"` + Description string `json:"description"` + Obj map[string]interface{} `json:"obj"` + OldObj map[string]interface{} `json:"oldObj"` + } `json:"data"` + } + if err := json.Unmarshal([]byte((*bodies)[0]), &message); err != nil { + t.Fatalf("unmarshalling message: %v", err) + } + + if message.SpecVersion != "1.0" || message.Type != "KUBERNETES_TOPOLOGY_CHANGE" || message.ID == "" { + t.Errorf("envelope damaged: %+v", message) + } + if message.Data.Operation != "update" || message.Data.Kind != "Secret" || + message.Data.ApiVersion != "v1" || message.Data.ClusterUid == "" || + !strings.Contains(message.Data.Description, "creds") { + t.Errorf("event metadata damaged: %+v", message.Data) + } + + // Both objects are still there, still identifiable, with only data redacted. + for name, object := range map[string]map[string]interface{}{"obj": message.Data.Obj, "oldObj": message.Data.OldObj} { + metadata, ok := object["metadata"].(map[string]interface{}) + if !ok || metadata["name"] != "creds" || metadata["namespace"] != "default" { + t.Errorf("%s lost its metadata: %+v", name, object) + continue + } + data, ok := object["data"].(map[string]interface{}) + if !ok || len(data) != 2 { + t.Errorf("%s should still report its 2 data keys, got: %+v", name, object["data"]) + } + } +} diff --git a/pkg/redact/redact.go b/pkg/redact/redact.go new file mode 100644 index 00000000..07012d0f --- /dev/null +++ b/pkg/redact/redact.go @@ -0,0 +1,178 @@ +/* +Copyright 2016 Skippbox, Ltd. + +Licensed 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 redact strips secret material out of Kubernetes objects before they +// leave the process in a notification. +// +// Handlers such as the CloudEvent handler serialize whole runtime.Objects into +// their payload. When Secret watching is enabled that means every Secret create, +// update and delete would ship the Secret's `data` and `stringData` — cluster, +// cloud, registry, TLS and application credentials — to an off-cluster receiver. +// Redaction happens here, once, so no handler has to remember to do it. +// +// Two layers are provided, and both are used: +// +// - Object() is the typed layer. It runs on every event the controller emits, +// so it protects all handlers, not just the ones that serialize objects today. +// - JSON() is the defensive layer. It runs on the marshalled bytes immediately +// before they are written to the wire, and catches Secrets that the typed +// layer could not recognise — nested Secrets, and the unstructured Secrets +// produced by the `customresources` informer, which is not gated by the +// `resource.secret` flag. +package redact + +import ( + "bytes" + "encoding/base64" + "encoding/json" + + api_v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" +) + +// Placeholder replaces every redacted secret value. Key names are kept: they +// carry useful signal (which keys exist, which were added or removed) and are +// not themselves secret material, whereas the values always are. +const Placeholder = "[redacted by kubewatch]" + +// secretKind is the Kubernetes kind whose data fields are always redacted. +const secretKind = "Secret" + +// base64Placeholder is Placeholder as it appears on the wire under a Secret's +// `data`, whose values are []byte and so are base64-encoded by encoding/json. +// Redacting the unstructured form to the same bytes the typed form produces +// keeps a redacted `data` value decodable by receivers that expect base64. +var base64Placeholder = base64.StdEncoding.EncodeToString([]byte(Placeholder)) + +// dataFields are the fields holding secret material on a Secret object, mapped +// to the value each is redacted to on the wire. +var dataFields = map[string]string{ + "data": base64Placeholder, + "stringData": Placeholder, +} + +// Object returns a copy of obj with any secret material replaced by Placeholder. +// +// The input is never mutated: objects handed to us come from a shared informer +// cache, and redacting one in place would corrupt the cache for every other +// reader. Objects that hold no secret material are returned as-is, so the +// common path costs nothing but a type check. +func Object(obj runtime.Object) runtime.Object { + switch typed := obj.(type) { + case nil: + return nil + case *api_v1.Secret: + redacted := typed.DeepCopy() + for key := range redacted.Data { + redacted.Data[key] = []byte(Placeholder) + } + for key := range redacted.StringData { + redacted.StringData[key] = Placeholder + } + return redacted + case *unstructured.Unstructured: + if typed.GetKind() != secretKind { + return obj + } + redacted := typed.DeepCopy() + redactUnstructuredData(redacted.Object) + return redacted + default: + return obj + } +} + +// JSON redacts secret material in already-marshalled JSON, whatever shape it +// arrived in. Any object anywhere in the document whose "kind" is "Secret" has +// its "data" and "stringData" fields redacted. This is the backstop for Secrets +// the typed layer cannot see, so it deliberately keys off the wire +// representation rather than off a Go type. +// +// The document is returned unchanged if it holds no Secret, and — so that a +// redaction bug can never turn into a leak — an error is returned rather than +// the original bytes if anything about the round-trip fails. +func JSON(payload []byte) ([]byte, error) { + // UseNumber keeps numeric fields byte-identical across the round-trip + // instead of pushing them through float64. + decoder := json.NewDecoder(bytes.NewReader(payload)) + decoder.UseNumber() + + var document interface{} + if err := decoder.Decode(&document); err != nil { + return nil, err + } + + if !redactValue(document) { + return payload, nil + } + + return json.Marshal(document) +} + +// redactValue walks an unmarshalled JSON value and redacts the data fields of +// every Secret it finds. It reports whether anything was redacted. +func redactValue(value interface{}) bool { + switch typed := value.(type) { + case map[string]interface{}: + redacted := false + if kind, ok := typed["kind"].(string); ok && kind == secretKind { + redacted = redactUnstructuredData(typed) + } + for _, child := range typed { + if redactValue(child) { + redacted = true + } + } + return redacted + case []interface{}: + redacted := false + for _, child := range typed { + if redactValue(child) { + redacted = true + } + } + return redacted + default: + return false + } +} + +// redactUnstructuredData replaces the values of a Secret's data fields in an +// unstructured object. It reports whether anything was redacted. +// +// A data field that is present but not a map is replaced wholesale: we cannot +// tell what it holds, and anything we cannot account for is treated as secret. +func redactUnstructuredData(object map[string]interface{}) bool { + redacted := false + for field, placeholder := range dataFields { + value, present := object[field] + if !present || value == nil { + continue + } + entries, ok := value.(map[string]interface{}) + if !ok { + object[field] = placeholder + redacted = true + continue + } + for key := range entries { + entries[key] = placeholder + redacted = true + } + } + return redacted +} diff --git a/pkg/redact/redact_test.go b/pkg/redact/redact_test.go new file mode 100644 index 00000000..f939bcc8 --- /dev/null +++ b/pkg/redact/redact_test.go @@ -0,0 +1,258 @@ +/* +Copyright 2016 Skippbox, Ltd. + +Licensed 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 redact + +import ( + "encoding/base64" + "encoding/json" + "strings" + "testing" + + api_v1 "k8s.io/api/core/v1" + meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +// sentinel is the secret material every test looks for on the way out. It must +// never appear in a redacted object, in any encoding. +const sentinel = "s3nt1nel-D0-N0T-D1SCL0SE" + +// assertNoSentinel serializes value the way a handler would and fails if the +// sentinel survived, either as raw bytes or base64-encoded (the form +// encoding/json gives a Secret's []byte data). +func assertNoSentinel(t *testing.T, what string, value interface{}) { + t.Helper() + + payload, err := json.Marshal(value) + if err != nil { + t.Fatalf("%s: marshal: %v", what, err) + } + + for form, encoded := range map[string]string{ + "raw": sentinel, + "base64": base64.StdEncoding.EncodeToString([]byte(sentinel)), + } { + if strings.Contains(string(payload), encoded) { + t.Errorf("%s: %s sentinel disclosed in %s", what, form, payload) + } + } +} + +func secret(name string) *api_v1.Secret { + return &api_v1.Secret{ + TypeMeta: meta_v1.TypeMeta{Kind: "Secret", APIVersion: "v1"}, + ObjectMeta: meta_v1.ObjectMeta{Name: name, Namespace: "kube-system"}, + Type: api_v1.SecretTypeDockerConfigJson, + Data: map[string][]byte{ + "password": []byte(sentinel), + ".dockerconfigjson": []byte(sentinel), + "tls.key": []byte(sentinel), + }, + StringData: map[string]string{"token": sentinel}, + } +} + +func TestObjectRedactsTypedSecret(t *testing.T) { + original := secret("creds") + redacted, ok := Object(original).(*api_v1.Secret) + if !ok { + t.Fatalf("Object() returned %T, want *v1.Secret", Object(original)) + } + + assertNoSentinel(t, "typed secret", redacted) + + // Every key is still reported, and every value is the placeholder. + if len(redacted.Data) != len(original.Data) { + t.Errorf("Data has %d keys, want %d", len(redacted.Data), len(original.Data)) + } + for key, value := range redacted.Data { + if string(value) != Placeholder { + t.Errorf("Data[%q] = %q, want %q", key, value, Placeholder) + } + } + for key, value := range redacted.StringData { + if value != Placeholder { + t.Errorf("StringData[%q] = %q, want %q", key, value, Placeholder) + } + } + + // Metadata a notification is actually for must survive untouched. + if redacted.Name != "creds" || redacted.Namespace != "kube-system" { + t.Errorf("metadata lost: %+v", redacted.ObjectMeta) + } + if redacted.Type != api_v1.SecretTypeDockerConfigJson { + t.Errorf("Type = %q, want %q", redacted.Type, api_v1.SecretTypeDockerConfigJson) + } +} + +// The objects handed to Object() come from a shared informer cache. Redacting +// one in place would corrupt that cache for every other reader in the process. +func TestObjectDoesNotMutateInput(t *testing.T) { + original := secret("creds") + + Object(original) + + if got := string(original.Data["password"]); got != sentinel { + t.Errorf("input Data was mutated: got %q, want %q", got, sentinel) + } + if got := original.StringData["token"]; got != sentinel { + t.Errorf("input StringData was mutated: got %q, want %q", got, sentinel) + } +} + +func TestObjectPassesThroughNonSecrets(t *testing.T) { + pod := &api_v1.Pod{ + TypeMeta: meta_v1.TypeMeta{Kind: "Pod", APIVersion: "v1"}, + ObjectMeta: meta_v1.ObjectMeta{Name: "web", Namespace: "default"}, + Spec: api_v1.PodSpec{Containers: []api_v1.Container{{Name: "app", Image: "nginx"}}}, + } + + // Non-Secret objects must keep their full body: the notification payload is + // what downstream consumers match playbooks on. + if got := Object(pod); got != pod { + t.Errorf("Object() copied or altered a non-Secret object: %#v", got) + } +} + +func TestObjectHandlesNil(t *testing.T) { + // OldObj is nil on creates and deletes. + if got := Object(nil); got != nil { + t.Errorf("Object(nil) = %#v, want nil", got) + } +} + +func TestObjectRedactsUnstructuredSecret(t *testing.T) { + // The `customresources` informer produces unstructured objects and is not + // gated by the `resource.secret` flag, so a Secret can arrive this way. + object := &unstructured.Unstructured{Object: map[string]interface{}{ + "kind": "Secret", + "apiVersion": "v1", + "metadata": map[string]interface{}{"name": "creds", "namespace": "default"}, + "data": map[string]interface{}{"password": base64.StdEncoding.EncodeToString([]byte(sentinel))}, + "stringData": map[string]interface{}{"token": sentinel}, + }} + + redacted := Object(object) + assertNoSentinel(t, "unstructured secret", redacted) + + // A redacted `data` value stays valid base64, matching the typed path, so + // receivers that decode it do not choke. + value, _, err := unstructured.NestedString(redacted.(*unstructured.Unstructured).Object, "data", "password") + if err != nil { + t.Fatalf("reading redacted data: %v", err) + } + decoded, err := base64.StdEncoding.DecodeString(value) + if err != nil { + t.Fatalf("redacted data is not valid base64: %v", err) + } + if string(decoded) != Placeholder { + t.Errorf("decoded data = %q, want %q", decoded, Placeholder) + } + + if got, _, _ := unstructured.NestedString(object.Object, "stringData", "token"); got != sentinel { + t.Errorf("input was mutated: stringData.token = %q", got) + } +} + +func TestObjectPassesThroughUnstructuredNonSecret(t *testing.T) { + object := &unstructured.Unstructured{Object: map[string]interface{}{ + "kind": "Prometheus", + "apiVersion": "monitoring.coreos.com/v1", + "spec": map[string]interface{}{"replicas": int64(2)}, + }} + + if got := Object(object); got != object { + t.Errorf("Object() altered an unstructured non-Secret: %#v", got) + } +} + +func TestJSONRedactsNestedAndListedSecrets(t *testing.T) { + encoded := base64.StdEncoding.EncodeToString([]byte(sentinel)) + payload := []byte(`{ + "data": { + "obj": {"kind":"Secret","data":{"password":"` + encoded + `"},"stringData":{"token":"` + sentinel + `"}}, + "oldObj": {"kind":"Secret","data":{"password":"` + encoded + `"}}, + "items": [{"kind":"Secret","data":{"tls.key":"` + encoded + `"}}], + "deep": {"wrapper":{"kind":"Secret","stringData":{"token":"` + sentinel + `"}}} + } + }`) + + redacted, err := JSON(payload) + if err != nil { + t.Fatalf("JSON(): %v", err) + } + + if strings.Contains(string(redacted), sentinel) { + t.Errorf("raw sentinel disclosed in %s", redacted) + } + if strings.Contains(string(redacted), encoded) { + t.Errorf("base64 sentinel disclosed in %s", redacted) + } +} + +// A Secret's data field is not always a map — a hand-built or malformed payload +// can put anything there. Whatever we cannot account for is treated as secret. +func TestJSONRedactsNonMapDataField(t *testing.T) { + payload := []byte(`{"kind":"Secret","data":"` + sentinel + `","stringData":["` + sentinel + `"]}`) + + redacted, err := JSON(payload) + if err != nil { + t.Fatalf("JSON(): %v", err) + } + if strings.Contains(string(redacted), sentinel) { + t.Errorf("sentinel disclosed in %s", redacted) + } +} + +func TestJSONLeavesNonSecretPayloadsByteIdentical(t *testing.T) { + // Numbers must not be pushed through float64, and a payload with nothing to + // redact must come back exactly as it went in. + payload := []byte(`{"kind":"Pod","metadata":{"generation":9007199254740993},"data":{"note":"kept"}}`) + + redacted, err := JSON(payload) + if err != nil { + t.Fatalf("JSON(): %v", err) + } + if string(redacted) != string(payload) { + t.Errorf("JSON() rewrote a payload with no Secret in it:\n got %s\nwant %s", redacted, payload) + } +} + +// Redaction re-marshals the document, so numbers still have to survive it. +func TestJSONPreservesNumbersWhileRedacting(t *testing.T) { + payload := []byte(`{"kind":"Secret","metadata":{"generation":9007199254740993},"data":{"password":"` + + base64.StdEncoding.EncodeToString([]byte(sentinel)) + `"}}`) + + redacted, err := JSON(payload) + if err != nil { + t.Fatalf("JSON(): %v", err) + } + if !strings.Contains(string(redacted), `9007199254740993`) { + t.Errorf("large integer lost precision: %s", redacted) + } + if strings.Contains(string(redacted), sentinel) { + t.Errorf("sentinel disclosed in %s", redacted) + } +} + +func TestJSONRejectsInvalidPayload(t *testing.T) { + // Callers must not fall back to the unredacted bytes, so an unparseable + // payload is an error rather than a pass-through. + if _, err := JSON([]byte(`{"kind":`)); err == nil { + t.Error("JSON() accepted invalid JSON, want error") + } +} From 2ae8b56f174dd2a950e309883f3bcd8174ef83e0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 14:02:57 +0000 Subject: [PATCH 2/2] Pass the PR body to the description check as data, not as script MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check interpolated ${{ github.event.pull_request.body }} directly into the run: block, so the script text was built out of untrusted input. A description containing backticks had them run as commands on the runner, and one containing a double quote ended the string bash was parsing — failing the step with "conditional binary operator expected" on a valid description. Pass the body through the environment instead and quote it. The check keeps its teeth: a description with no "## Tests performed" section, or a section with no bullet list, still fails. Verified against the real body that broke it, four valid/invalid descriptions, and an injection attempt that now goes through as data with nothing executed. Also anchor the section-extracting sed to a line-initial h2 (/^## /), so a subheading inside the section no longer truncates it early. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GzZp8kPXKYjXuxY1oN1aU8 --- .github/workflows/pr-description-check.yaml | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pr-description-check.yaml b/.github/workflows/pr-description-check.yaml index 378e8ca0..63f722fb 100644 --- a/.github/workflows/pr-description-check.yaml +++ b/.github/workflows/pr-description-check.yaml @@ -14,14 +14,21 @@ jobs: - name: Validate PR description id: validate + # The body reaches bash through the environment, never through ${{ }} + # interpolation. Interpolating it built the script text out of untrusted + # input: backticks in a description ran as commands on the runner, and a + # double quote ended the string bash was parsing, failing the step with + # "conditional binary operator expected" on a perfectly good description. + env: + PR_BODY: ${{ github.event.pull_request.body }} run: | - if [[ ! "${{ github.event.pull_request.body }}" =~ "## Tests performed" ]]; then + if [[ ! "$PR_BODY" =~ "## Tests performed" ]]; then echo "PR description does not contain the section 'Tests performed'." exit 1 fi - # Extract the "Tests performed" section - tests_performed_section=$(sed -n '/## Tests performed/,/##/p' <<< "${{ github.event.pull_request.body }}") + # Extract the "Tests performed" section, up to the next h2 heading + tests_performed_section=$(sed -n '/^## Tests performed/,/^## /p' <<< "$PR_BODY") # Check if there is at least one test description in the "Tests performed" section if [[ ! "$tests_performed_section" =~ "- " ]]; then