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