From 63590cbeee3ef23d66799ffcee5b39d5a005115f Mon Sep 17 00:00:00 2001 From: zhzhuang-zju Date: Wed, 27 Nov 2024 15:07:56 +0800 Subject: [PATCH] karmada-operator: add CRDs archive verification to enhance file system robustness Signed-off-by: zhzhuang-zju --- operator/pkg/tasks/init/crd.go | 32 +++++++++ pkg/util/validation/validation.go | 88 ++++++++++++++++++++++++ pkg/util/validation/validation_test.go | 95 ++++++++++++++++++++++++++ 3 files changed, 215 insertions(+) diff --git a/operator/pkg/tasks/init/crd.go b/operator/pkg/tasks/init/crd.go index 431ea840f19b..2879790ebf0d 100644 --- a/operator/pkg/tasks/init/crd.go +++ b/operator/pkg/tasks/init/crd.go @@ -23,6 +23,7 @@ import ( "fmt" "os" "path" + "path/filepath" "strings" "k8s.io/klog/v2" @@ -30,6 +31,7 @@ import ( operatorv1alpha1 "github.com/karmada-io/karmada/operator/pkg/apis/operator/v1alpha1" "github.com/karmada-io/karmada/operator/pkg/util" "github.com/karmada-io/karmada/operator/pkg/workflow" + "github.com/karmada-io/karmada/pkg/util/validation" ) var ( @@ -53,6 +55,10 @@ func NewPrepareCrdsTask() workflow.Task { Name: "Unpack", Run: runUnpack, }, + { + Name: "post-check", + Run: postCheck, + }, }, } } @@ -154,6 +160,9 @@ func runUnpack(r workflow.RunData) error { exist, _ := util.PathExists(crdsPath) if !exist { klog.V(2).InfoS("[runUnpack] CRD yaml files do not exist, unpacking tar file", "unpackDir", crdsDir) + if err = validation.ValidateTarball(crdsTarPath, validation.ValidateCrdsTarBall); err != nil { + return fmt.Errorf("[unpack] inValid crd tar, err: %w", err) + } if err := util.Unpack(crdsTarPath, crdsDir); err != nil { return fmt.Errorf("[unpack] failed to unpack crd tar, err: %w", err) } @@ -165,6 +174,29 @@ func runUnpack(r workflow.RunData) error { return nil } +func postCheck(r workflow.RunData) error { + data, ok := r.(InitData) + if !ok { + return errors.New("post-check task invoked with an invalid data struct") + } + + crdsDir, err := getCrdsDir(data) + if err != nil { + return fmt.Errorf("[post-check] failed to get CRD dir, err: %w", err) + } + + for _, archive := range validation.CrdsArchive { + expectedDir := filepath.Join(crdsDir, archive) + exist, _ := util.PathExists(expectedDir) + if !exist { + return fmt.Errorf("[post-check] Lacking the necessary file path: %s", expectedDir) + } + } + + klog.V(2).InfoS("[post-check] Successfully post-check the crd tar archive", "karmada", klog.KObj(data)) + return nil +} + func existCrdsTar(crdsDir string) bool { files := util.ListFiles(crdsDir) klog.V(2).InfoS("[existCrdsTar] Checking for CRD tar file in directory", "directory", crdsDir) diff --git a/pkg/util/validation/validation.go b/pkg/util/validation/validation.go index 0fdb9861d6a8..191c8754dc76 100644 --- a/pkg/util/validation/validation.go +++ b/pkg/util/validation/validation.go @@ -17,7 +17,13 @@ limitations under the License. package validation import ( + "archive/tar" + "compress/gzip" "fmt" + "io" + "os" + "path/filepath" + "strings" "github.com/go-openapi/jsonpointer" corev1 "k8s.io/api/core/v1" @@ -372,3 +378,85 @@ func validateOverrideOperator(operator policyv1alpha1.OverriderOperator, value a } return allErrs } + +// CrdsArchive defines the expected tar archive. +var CrdsArchive = []string{"crds", "crds/bases", "crds/patches"} + +// ValidateTarball opens a .tar.gz file, and validates each +// entry in the tar archive using a provided validate function. +func ValidateTarball(tarball string, validate func(*tar.Header) error) error { + r, err := os.Open(tarball) + if err != nil { + return err + } + defer r.Close() + + gr, err := gzip.NewReader(r) + if err != nil { + return fmt.Errorf("new reader failed. %v", err) + } + defer gr.Close() + + tr := tar.NewReader(gr) + for { + header, err := tr.Next() + if err != nil { + if err == io.EOF { + break + } + return err + } + + if err = validate(header); err != nil { + return err + } + } + + return nil +} + +// ValidateCrdsTarBall checks if the CRDs package complies with file specifications. +// It verifies the following: +// 1. Whether the path is clean. +// 2. Whether the file directory structure meets expectations. +func ValidateCrdsTarBall(header *tar.Header) error { + switch header.Typeflag { + case tar.TypeDir: + // in Unix-like systems, directory paths in tar archives end with a slash (/) to distinguish them from file paths. + if strings.HasSuffix(header.Name, "/") && len(header.Name) > 1 { + if !isCleanPath(header.Name[:len(header.Name)-1]) { + return fmt.Errorf("the given file contains unclean file dir: %s", header.Name) + } + } else { + if !isCleanPath(header.Name) { + return fmt.Errorf("the given file contains unclean file dir: %s", header.Name) + } + } + if !isExpectedPath(header.Name, CrdsArchive) { + return fmt.Errorf("the given file contains unexpected file dir: %s", header.Name) + } + case tar.TypeReg: + if !isCleanPath(header.Name) { + return fmt.Errorf("the given file contains unclean file path: %s", header.Name) + } + if !isExpectedPath(header.Name, CrdsArchive) { + return fmt.Errorf("the given file contains unexpected file path: %s", header.Name) + } + default: + return fmt.Errorf("unknown type: %v in %s", header.Typeflag, header.Name) + } + return nil +} + +func isExpectedPath(path string, expectedDirs []string) bool { + for _, dir := range expectedDirs { + if path == dir || strings.HasPrefix(path, dir+"/") { + return true + } + } + return false +} + +func isCleanPath(path string) bool { + return path == filepath.Clean(path) +} diff --git a/pkg/util/validation/validation_test.go b/pkg/util/validation/validation_test.go index b1046e4ac458..07243968b950 100644 --- a/pkg/util/validation/validation_test.go +++ b/pkg/util/validation/validation_test.go @@ -17,9 +17,12 @@ limitations under the License. package validation import ( + "archive/tar" + "fmt" "strings" "testing" + "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/utils/ptr" @@ -717,3 +720,95 @@ func TestValidateApplicationFailover(t *testing.T) { }) } } + +func TestCheckOperatorCrdsTar(t *testing.T) { + testItems := []struct { + name string + header *tar.Header + expectedErr error + }{ + { + name: "unclean file dir 'crds/../'", + header: &tar.Header{ + Name: "crds/../", + Typeflag: tar.TypeDir, + }, + expectedErr: fmt.Errorf("the given file contains unclean file dir: %s", "crds/../"), + }, + { + name: "unclean file dir 'crds/..'", + header: &tar.Header{ + Name: "crds/..", + Typeflag: tar.TypeDir, + }, + expectedErr: fmt.Errorf("the given file contains unclean file dir: %s", "crds/.."), + }, + { + name: "unexpected file dir '../crds'", + header: &tar.Header{ + Name: "../crds", + Typeflag: tar.TypeDir, + }, + expectedErr: fmt.Errorf("the given file contains unexpected file dir: %s", "../crds"), + }, + { + name: "unexpected file dir '..'", + header: &tar.Header{ + Name: "..", + Typeflag: tar.TypeDir, + }, + expectedErr: fmt.Errorf("the given file contains unexpected file dir: %s", ".."), + }, + { + name: "expected file dir 'crds/'", + header: &tar.Header{ + Name: "crds/", + Typeflag: tar.TypeDir, + }, + expectedErr: nil, + }, + { + name: "expected file dir 'crds'", + header: &tar.Header{ + Name: "crds", + Typeflag: tar.TypeDir, + }, + expectedErr: nil, + }, + { + name: "unclean file path 'crds/../a.yaml'", + header: &tar.Header{ + Name: "crds/../a.yaml", + Typeflag: tar.TypeReg, + }, + expectedErr: fmt.Errorf("the given file contains unclean file path: %s", "crds/../a.yaml"), + }, + { + name: "unexpected file path '../crds/a.yaml'", + header: &tar.Header{ + Name: "../crds/a.yaml", + Typeflag: tar.TypeReg, + }, + expectedErr: fmt.Errorf("the given file contains unexpected file path: %s", "../crds/a.yaml"), + }, + { + name: "unexpected file path '../a.yaml'", + header: &tar.Header{ + Name: "../a.yaml", + Typeflag: tar.TypeReg, + }, + expectedErr: fmt.Errorf("the given file contains unexpected file path: %s", "../a.yaml"), + }, + { + name: "expected file path 'crds/a.yaml'", + header: &tar.Header{ + Name: "crds/a.yaml", + Typeflag: tar.TypeReg, + }, + expectedErr: nil, + }, + } + for _, item := range testItems { + assert.Equal(t, item.expectedErr, ValidateCrdsTarBall(item.header)) + } +}