From 0e0daddd03b4769e722ec3501153f2140294a028 Mon Sep 17 00:00:00 2001 From: Connor Kuehl Date: Tue, 6 Feb 2024 15:54:48 -0600 Subject: [PATCH] Require CloudInit .spec.contents to be valid YAML The Elemental cloud-init executor will fail in disastrous ways instead of just discarding the document if the document's contents are not YAML. However, it does ignore instructions that it doesn't understand if the document is valid YAML. So, require that the CloudInit resource's .spec.contents are valid YAML. Ideally we could use the yip package to parse and perform semantic analysis on the document as a richer form of validation here, but that package does not expose a way of reading those diagnostics. In client code, it only logs what the errors are (in fact, the function signature doesn't return an error.) Some future work upstream in that package could be beneficial here to create an API such that the yip package can return errors to its caller instead of simply logging them. Signed-off-by: Connor Kuehl (cherry picked from commit 786b1b222bba52272a9afe70f0a16a3c61ae564f) --- pkg/admitter/cloudinit.go | 12 ++++++++++++ pkg/admitter/cloudinit_test.go | 2 ++ 2 files changed, 14 insertions(+) diff --git a/pkg/admitter/cloudinit.go b/pkg/admitter/cloudinit.go index 22a0c7b8..b0567d5e 100644 --- a/pkg/admitter/cloudinit.go +++ b/pkg/admitter/cloudinit.go @@ -7,6 +7,7 @@ import ( "path/filepath" "github.com/harvester/webhook/pkg/server/admission" + "gopkg.in/yaml.v3" admissionregv1 "k8s.io/api/admissionregistration/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -21,6 +22,7 @@ var ( errFilenameTaken = errors.New("filename already in use") errProtectedFilename = errors.New("filename conflicts with a critical system-owned file") errMissingExt = errors.New("filename does not end in .yaml or .yml") + errNotYAML = errors.New("could not parse document as yaml") ) var builtinFilenameDenyList = []string{ @@ -85,6 +87,16 @@ func (v *CloudInit) validate(cloudinit *v1beta1.CloudInit) error { return errFilenameTaken } + var obj map[string]any + var typeErr *yaml.TypeError + err = yaml.Unmarshal([]byte(cloudinit.Spec.Contents), &obj) + if errors.As(err, &typeErr) { + return errNotYAML + } + if err != nil { + return err + } + return nil } diff --git a/pkg/admitter/cloudinit_test.go b/pkg/admitter/cloudinit_test.go index b2453814..ab9a0964 100644 --- a/pkg/admitter/cloudinit_test.go +++ b/pkg/admitter/cloudinit_test.go @@ -56,6 +56,7 @@ func TestCreate(t *testing.T) { {"filename collision", v1beta1.CloudInitSpec{Filename: "99_ssh.yaml"}, errFilenameTaken}, {"conflicts with protected file", v1beta1.CloudInitSpec{Filename: "helloworld.yaml"}, errProtectedFilename}, {"not yaml or yml file ext", v1beta1.CloudInitSpec{Filename: "a"}, errMissingExt}, + {"not yaml contents", v1beta1.CloudInitSpec{Filename: "not.yaml", Contents: "hello, there"}, errNotYAML}, } for _, tt := range tests { @@ -96,6 +97,7 @@ func TestUpdate(t *testing.T) { {"filename collision", v1beta1.CloudInitSpec{Filename: "99_ssh.yaml"}, errFilenameTaken}, {"conflicts with protected file", v1beta1.CloudInitSpec{Filename: "helloworld.yaml"}, errProtectedFilename}, {"not yaml or yml file ext", v1beta1.CloudInitSpec{Filename: "a"}, errMissingExt}, + {"not yaml contents", v1beta1.CloudInitSpec{Filename: "not.yaml", Contents: "hello, there"}, errNotYAML}, } for _, tt := range tests {