From cb5cffe714ad417615439f9ea08b1c81fcdea905 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 27 Oct 2026 11:11:43 -0400 Subject: [PATCH 1/3] fix(render): reject non-Secret documents in function credentials LoadCredentials decoded every YAML document straight into a corev1.Secret, so a ConfigMap with plain-text data failed with a confusing base64 error and documents without data were silently loaded as empty Secrets. Check each document's apiVersion and kind first and return an error naming it. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- pkg/render/load.go | 14 +++++ pkg/render/load_test.go | 63 +++++++++++++++++++ .../testdata/credentials-configmap.yaml | 14 +++++ .../testdata/credentials-deployment.yaml | 5 ++ pkg/render/testdata/credentials.yaml | 14 +++++ 5 files changed, 110 insertions(+) create mode 100644 pkg/render/testdata/credentials-configmap.yaml create mode 100644 pkg/render/testdata/credentials-deployment.yaml create mode 100644 pkg/render/testdata/credentials.yaml diff --git a/pkg/render/load.go b/pkg/render/load.go index fa5971b0..536cfcd4 100644 --- a/pkg/render/load.go +++ b/pkg/render/load.go @@ -23,6 +23,7 @@ import ( "github.com/spf13/afero" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/yaml" @@ -121,6 +122,19 @@ func LoadCredentials(fs afero.Fs, file string) ([]corev1.Secret, error) { secrets := make([]corev1.Secret, 0, len(stream)) for _, y := range stream { + // Check the kind before decoding into a Secret, so that a non-Secret + // document (e.g. a ConfigMap with plain-text data) gets a clear error + // rather than a confusing base64 decoding failure, or being silently + // treated as an empty Secret. + m := &metav1.PartialObjectMetadata{} + if err := yaml.Unmarshal(y, m); err != nil { + return nil, errors.Wrap(err, "cannot parse YAML secret manifest") + } + + if gvk := m.GroupVersionKind(); gvk != corev1.SchemeGroupVersion.WithKind("Secret") { + return nil, errors.Errorf("%s/%s is not a v1 Secret", gvk.Kind, m.GetName()) + } + s := &corev1.Secret{} if err := yaml.Unmarshal(y, s); err != nil { return nil, errors.Wrap(err, "cannot parse YAML secret manifest") diff --git a/pkg/render/load_test.go b/pkg/render/load_test.go index 9c226de0..962f0dda 100644 --- a/pkg/render/load_test.go +++ b/pkg/render/load_test.go @@ -25,6 +25,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/spf13/afero" + corev1 "k8s.io/api/core/v1" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -329,6 +330,68 @@ func TestLoadFunctions(t *testing.T) { } } +func TestLoadCredentials(t *testing.T) { + fs := afero.FromIOFS{FS: testdatafs} + + type want struct { + secrets []corev1.Secret + err string + } + + cases := map[string]struct { + file string + want want + }{ + "Success": { + file: "testdata/credentials.yaml", + want: want{ + secrets: []corev1.Secret{ + { + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Secret"}, + ObjectMeta: metav1.ObjectMeta{Name: "creds-a"}, + Data: map[string][]byte{"token": []byte("secret")}, + }, + { + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Secret"}, + ObjectMeta: metav1.ObjectMeta{Name: "creds-b"}, + StringData: map[string]string{"password": "hunter2"}, + }, + }, + }, + }, + "ConfigMapWithPlainTextData": { + file: "testdata/credentials-configmap.yaml", + want: want{ + err: "ConfigMap/not-a-secret is not a v1 Secret", + }, + }, + "DeploymentWithNoData": { + file: "testdata/credentials-deployment.yaml", + want: want{ + err: "Deployment/not-a-secret is not a v1 Secret", + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + secrets, err := LoadCredentials(fs, tc.file) + + if diff := cmp.Diff(tc.want.secrets, secrets); diff != "" { + t.Errorf("LoadCredentials(..), -want, +got:\n%s", diff) + } + + gotErr := "" + if err != nil { + gotErr = err.Error() + } + if diff := cmp.Diff(tc.want.err, gotErr); diff != "" { + t.Errorf("LoadCredentials(..), -want error, +got error:\n%s", diff) + } + }) + } +} + func TestLoadObservedResources(t *testing.T) { fs := afero.FromIOFS{FS: testdatafs} diff --git a/pkg/render/testdata/credentials-configmap.yaml b/pkg/render/testdata/credentials-configmap.yaml new file mode 100644 index 00000000..71fd8bde --- /dev/null +++ b/pkg/render/testdata/credentials-configmap.yaml @@ -0,0 +1,14 @@ +--- +apiVersion: v1 +kind: Secret +metadata: + name: creds-a +data: + token: c2VjcmV0 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: not-a-secret +data: + setting: plain text, not base64! diff --git a/pkg/render/testdata/credentials-deployment.yaml b/pkg/render/testdata/credentials-deployment.yaml new file mode 100644 index 00000000..54cd81f5 --- /dev/null +++ b/pkg/render/testdata/credentials-deployment.yaml @@ -0,0 +1,5 @@ +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: not-a-secret diff --git a/pkg/render/testdata/credentials.yaml b/pkg/render/testdata/credentials.yaml new file mode 100644 index 00000000..abb55688 --- /dev/null +++ b/pkg/render/testdata/credentials.yaml @@ -0,0 +1,14 @@ +--- +apiVersion: v1 +kind: Secret +metadata: + name: creds-a +data: + token: c2VjcmV0 +--- +apiVersion: v1 +kind: Secret +metadata: + name: creds-b +stringData: + password: hunter2 From 894343f9f814adcb85a05e9fcd3b107c76a907c0 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Thu, 29 Oct 2026 16:03:34 -0400 Subject: [PATCH 2/3] perf(render): decode each credentials document once Decode into a Secret first and check its kind, falling back to a metadata-only decode only when the Secret decode fails, so a non-Secret document still gets a clear error. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- pkg/render/load.go | 28 ++++++++++--------- pkg/render/load_test.go | 6 ++++ .../testdata/credentials-invalid-base64.yaml | 7 +++++ 3 files changed, 28 insertions(+), 13 deletions(-) create mode 100644 pkg/render/testdata/credentials-invalid-base64.yaml diff --git a/pkg/render/load.go b/pkg/render/load.go index 536cfcd4..f3938ddf 100644 --- a/pkg/render/load.go +++ b/pkg/render/load.go @@ -122,24 +122,26 @@ func LoadCredentials(fs afero.Fs, file string) ([]corev1.Secret, error) { secrets := make([]corev1.Secret, 0, len(stream)) for _, y := range stream { - // Check the kind before decoding into a Secret, so that a non-Secret - // document (e.g. a ConfigMap with plain-text data) gets a clear error - // rather than a confusing base64 decoding failure, or being silently - // treated as an empty Secret. - m := &metav1.PartialObjectMetadata{} - if err := yaml.Unmarshal(y, m); err != nil { - return nil, errors.Wrap(err, "cannot parse YAML secret manifest") - } - - if gvk := m.GroupVersionKind(); gvk != corev1.SchemeGroupVersion.WithKind("Secret") { - return nil, errors.Errorf("%s/%s is not a v1 Secret", gvk.Kind, m.GetName()) - } - s := &corev1.Secret{} if err := yaml.Unmarshal(y, s); err != nil { + // Decoding a non-Secret document (e.g. a ConfigMap with + // plain-text data) into a Secret can fail with a confusing + // base64 error before we ever see its kind. Only on failure, + // decode just the metadata to report the wrong kind clearly. + m := &metav1.PartialObjectMetadata{} + if merr := yaml.Unmarshal(y, m); merr == nil && m.GroupVersionKind() != corev1.SchemeGroupVersion.WithKind("Secret") { + return nil, errors.Errorf("%s/%s is not a v1 Secret", m.GroupVersionKind().Kind, m.GetName()) + } return nil, errors.Wrap(err, "cannot parse YAML secret manifest") } + // A non-Secret document without Secret-incompatible fields decodes + // cleanly, so check the kind too rather than silently treating it + // as an empty Secret. + if gvk := s.GroupVersionKind(); gvk != corev1.SchemeGroupVersion.WithKind("Secret") { + return nil, errors.Errorf("%s/%s is not a v1 Secret", gvk.Kind, s.GetName()) + } + secrets = append(secrets, *s) } diff --git a/pkg/render/load_test.go b/pkg/render/load_test.go index 962f0dda..081af145 100644 --- a/pkg/render/load_test.go +++ b/pkg/render/load_test.go @@ -371,6 +371,12 @@ func TestLoadCredentials(t *testing.T) { err: "Deployment/not-a-secret is not a v1 Secret", }, }, + "SecretWithInvalidBase64Data": { + file: "testdata/credentials-invalid-base64.yaml", + want: want{ + err: "cannot parse YAML secret manifest: error unmarshaling JSON: while decoding JSON: illegal base64 data at input byte 3", + }, + }, } for name, tc := range cases { diff --git a/pkg/render/testdata/credentials-invalid-base64.yaml b/pkg/render/testdata/credentials-invalid-base64.yaml new file mode 100644 index 00000000..98152313 --- /dev/null +++ b/pkg/render/testdata/credentials-invalid-base64.yaml @@ -0,0 +1,7 @@ +--- +apiVersion: v1 +kind: Secret +metadata: + name: bad-data +data: + token: not-base64! From 3eb246b78bb20f9c855df59d0b3352c58fc86b7b Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Fri, 30 Oct 2026 13:12:57 -0400 Subject: [PATCH 3/3] fix(render): say which credentials type field is wrong Report the apiVersion and kind a non-Secret credentials document actually has, quoted so empty values are visible, and say how to fix it. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Jonathan Ogilvie --- pkg/render/load.go | 15 ++++++++-- pkg/render/load_test.go | 28 +++++++++++++++++-- .../credentials-missing-apiversion.yaml | 3 ++ .../testdata/credentials-missing-kind.yaml | 3 ++ pkg/render/testdata/credentials-unnamed.yaml | 2 ++ .../credentials-wrong-apiversion.yaml | 4 +++ 6 files changed, 51 insertions(+), 4 deletions(-) create mode 100644 pkg/render/testdata/credentials-missing-apiversion.yaml create mode 100644 pkg/render/testdata/credentials-missing-kind.yaml create mode 100644 pkg/render/testdata/credentials-unnamed.yaml create mode 100644 pkg/render/testdata/credentials-wrong-apiversion.yaml diff --git a/pkg/render/load.go b/pkg/render/load.go index f3938ddf..4efd5d39 100644 --- a/pkg/render/load.go +++ b/pkg/render/load.go @@ -18,6 +18,7 @@ package render import ( "bufio" + "fmt" "io" "path/filepath" @@ -25,6 +26,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/yaml" "github.com/crossplane/crossplane-runtime/v2/pkg/errors" @@ -130,7 +132,7 @@ func LoadCredentials(fs afero.Fs, file string) ([]corev1.Secret, error) { // decode just the metadata to report the wrong kind clearly. m := &metav1.PartialObjectMetadata{} if merr := yaml.Unmarshal(y, m); merr == nil && m.GroupVersionKind() != corev1.SchemeGroupVersion.WithKind("Secret") { - return nil, errors.Errorf("%s/%s is not a v1 Secret", m.GroupVersionKind().Kind, m.GetName()) + return nil, errNotSecret(m.GroupVersionKind(), m.GetName()) } return nil, errors.Wrap(err, "cannot parse YAML secret manifest") } @@ -139,7 +141,7 @@ func LoadCredentials(fs afero.Fs, file string) ([]corev1.Secret, error) { // cleanly, so check the kind too rather than silently treating it // as an empty Secret. if gvk := s.GroupVersionKind(); gvk != corev1.SchemeGroupVersion.WithKind("Secret") { - return nil, errors.Errorf("%s/%s is not a v1 Secret", gvk.Kind, s.GetName()) + return nil, errNotSecret(gvk, s.GetName()) } secrets = append(secrets, *s) @@ -148,6 +150,15 @@ func LoadCredentials(fs afero.Fs, file string) ([]corev1.Secret, error) { return secrets, nil } +// errNotSecret reports a credentials document whose type is not a v1 Secret. +func errNotSecret(gvk schema.GroupVersionKind, name string) error { + doc := "unnamed credentials document" + if name != "" { + doc = fmt.Sprintf("credentials document %q", name) + } + return errors.Errorf("%s has apiVersion %q and kind %q; set apiVersion: v1 and kind: Secret", doc, gvk.GroupVersion().String(), gvk.Kind) +} + // LoadRequiredResources from a stream of YAML manifests. func LoadRequiredResources(fs afero.Fs, file string) ([]unstructured.Unstructured, error) { stream, err := LoadYAMLStream(fs, file) diff --git a/pkg/render/load_test.go b/pkg/render/load_test.go index 081af145..1481ce00 100644 --- a/pkg/render/load_test.go +++ b/pkg/render/load_test.go @@ -362,13 +362,37 @@ func TestLoadCredentials(t *testing.T) { "ConfigMapWithPlainTextData": { file: "testdata/credentials-configmap.yaml", want: want{ - err: "ConfigMap/not-a-secret is not a v1 Secret", + err: `credentials document "not-a-secret" has apiVersion "v1" and kind "ConfigMap"; set apiVersion: v1 and kind: Secret`, }, }, "DeploymentWithNoData": { file: "testdata/credentials-deployment.yaml", want: want{ - err: "Deployment/not-a-secret is not a v1 Secret", + err: `credentials document "not-a-secret" has apiVersion "apps/v1" and kind "Deployment"; set apiVersion: v1 and kind: Secret`, + }, + }, + "MissingKind": { + file: "testdata/credentials-missing-kind.yaml", + want: want{ + err: `credentials document "creds" has apiVersion "v1" and kind ""; set apiVersion: v1 and kind: Secret`, + }, + }, + "MissingAPIVersion": { + file: "testdata/credentials-missing-apiversion.yaml", + want: want{ + err: `credentials document "creds" has apiVersion "" and kind "Secret"; set apiVersion: v1 and kind: Secret`, + }, + }, + "WrongAPIVersion": { + file: "testdata/credentials-wrong-apiversion.yaml", + want: want{ + err: `credentials document "creds" has apiVersion "v2" and kind "Secret"; set apiVersion: v1 and kind: Secret`, + }, + }, + "Unnamed": { + file: "testdata/credentials-unnamed.yaml", + want: want{ + err: `unnamed credentials document has apiVersion "v1" and kind "ConfigMap"; set apiVersion: v1 and kind: Secret`, }, }, "SecretWithInvalidBase64Data": { diff --git a/pkg/render/testdata/credentials-missing-apiversion.yaml b/pkg/render/testdata/credentials-missing-apiversion.yaml new file mode 100644 index 00000000..24b4d630 --- /dev/null +++ b/pkg/render/testdata/credentials-missing-apiversion.yaml @@ -0,0 +1,3 @@ +kind: Secret +metadata: + name: creds diff --git a/pkg/render/testdata/credentials-missing-kind.yaml b/pkg/render/testdata/credentials-missing-kind.yaml new file mode 100644 index 00000000..b3d0b707 --- /dev/null +++ b/pkg/render/testdata/credentials-missing-kind.yaml @@ -0,0 +1,3 @@ +apiVersion: v1 +metadata: + name: creds diff --git a/pkg/render/testdata/credentials-unnamed.yaml b/pkg/render/testdata/credentials-unnamed.yaml new file mode 100644 index 00000000..c66c31b7 --- /dev/null +++ b/pkg/render/testdata/credentials-unnamed.yaml @@ -0,0 +1,2 @@ +apiVersion: v1 +kind: ConfigMap diff --git a/pkg/render/testdata/credentials-wrong-apiversion.yaml b/pkg/render/testdata/credentials-wrong-apiversion.yaml new file mode 100644 index 00000000..4643ea81 --- /dev/null +++ b/pkg/render/testdata/credentials-wrong-apiversion.yaml @@ -0,0 +1,4 @@ +apiVersion: v2 +kind: Secret +metadata: + name: creds