ROB-997 Redact Secret data from outbound notifications

.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

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.

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

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)
+	}
+}

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