From 6a4bf74cf31cd1bf7898939d4fac3cba7164120f Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 16 Dec 2020 11:38:08 +0100 Subject: [PATCH 1/3] Add safe guards for relative paths This commit ensures that relative (user configurable) paths never traverse outside their working directory. It does _not_ provide protection against path traversal within `kustomization.yaml` files. Signed-off-by: Hidde Beydals --- controllers/kustomization_controller.go | 19 +++++++++++++++---- controllers/kustomization_decryptor.go | 7 +++++-- go.mod | 1 + go.sum | 2 ++ 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/controllers/kustomization_controller.go b/controllers/kustomization_controller.go index a2c4ad05..d5509321 100644 --- a/controllers/kustomization_controller.go +++ b/controllers/kustomization_controller.go @@ -24,11 +24,11 @@ import ( "net/http" "os" "os/exec" - "path" "path/filepath" "strings" "time" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -306,8 +306,16 @@ func (r *KustomizationReconciler) reconcile( ), err } - dirPath := path.Join(tmpDir, kustomization.Spec.Path) // check build path exists + dirPath, err := securejoin.SecureJoin(tmpDir, kustomization.Spec.Path) + if err != nil { + return kustomizev1.KustomizationNotReady( + kustomization, + source.GetArtifact().Revision, + kustomizev1.ArtifactFailedReason, + err.Error(), + ), err + } if _, err := os.Stat(dirPath); err != nil { err = fmt.Errorf("kustomization path not found: %w", err) return kustomizev1.KustomizationNotReady( @@ -606,12 +614,15 @@ func (r *KustomizationReconciler) writeKubeConfig(kustomization kustomizev1.Kust return "", err } - kubeConfigPath := path.Join(dirPath, secretName.Name) + kubeConfigPath, err := securejoin.SecureJoin(dirPath, secretName.Name) + if err != nil { + return "", err + } if err := ioutil.WriteFile(kubeConfigPath, kubeConfig, os.ModePerm); err != nil { return "", fmt.Errorf("unable to write KubeConfig secret '%s' to storage: %w", secretName.String(), err) } - return secretName.Name, nil + return kubeConfigPath, nil } func (r *KustomizationReconciler) getKubeConfig(kustomization kustomizev1.Kustomization) ([]byte, error) { diff --git a/controllers/kustomization_decryptor.go b/controllers/kustomization_decryptor.go index 4d64bc09..f027f947 100644 --- a/controllers/kustomization_decryptor.go +++ b/controllers/kustomization_decryptor.go @@ -23,8 +23,8 @@ import ( "io/ioutil" "os" "os/exec" - "path" + securejoin "github.com/cyphar/filepath-securejoin" "go.mozilla.org/sops/v3/aes" "go.mozilla.org/sops/v3/cmd/sops/common" "go.mozilla.org/sops/v3/cmd/sops/formats" @@ -133,7 +133,10 @@ func (kd *KustomizeDecryptor) ImportKeys(ctx context.Context) error { defer os.RemoveAll(tmpDir) for name, key := range secret.Data { - keyPath := path.Join(tmpDir, name) + keyPath, err := securejoin.SecureJoin(tmpDir, name) + if err != nil { + return err + } if err := ioutil.WriteFile(keyPath, key, os.ModePerm); err != nil { return fmt.Errorf("unable to write key to storage: %w", err) } diff --git a/go.mod b/go.mod index f4f6becf..842f7b42 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ go 1.15 replace github.com/fluxcd/kustomize-controller/api => ./api require ( + github.com/cyphar/filepath-securejoin v0.2.2 github.com/fluxcd/kustomize-controller/api v0.5.1 github.com/fluxcd/pkg/apis/meta v0.5.0 github.com/fluxcd/pkg/runtime v0.4.0 diff --git a/go.sum b/go.sum index cf0c06e1..303e94ea 100644 --- a/go.sum +++ b/go.sum @@ -120,6 +120,8 @@ github.com/coreos/pkg v0.0.0-20180928190104-399ea9e2e55f/go.mod h1:E3G3o1h8I7cfc github.com/cpuguy83/go-md2man v1.0.10/go.mod h1:SmD6nW6nTyfqj6ABTjUi3V3JVMnlJmwcJI5acqYI6dE= github.com/cpuguy83/go-md2man/v2 v2.0.0/go.mod h1:maD7wRr/U5Z6m/iR4s+kqSMx2CaBsrgA7czyZG/E6dU= github.com/creack/pty v1.1.7/go.mod h1:lj5s0c3V2DBrqTV7llrYr5NG6My20zk30Fl46Y7DoTY= +github.com/cyphar/filepath-securejoin v0.2.2 h1:jCwT2GTP+PY5nBz3c/YL5PAIbusElVrPujOBSCj8xRg= +github.com/cyphar/filepath-securejoin v0.2.2/go.mod h1:FpkQEhXnPnOthhzymB7CGsFk2G9VLXONKD9G7QGMM+4= github.com/davecgh/go-spew v0.0.0-20151105211317-5215b55f46b2/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= From 69a7e75a91c9b2a9c3264373b158d2fd2c0c24b0 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 16 Dec 2020 12:19:47 +0100 Subject: [PATCH 2/3] Make Path an optional field and remove validation As due to secure joins, the requirement on both providing a path and/or requiring it to be in a certain format offers little value over the UX experience of not having to provide it when you just want to reconcile whatever can be found in the root of the source reference. Signed-off-by: Hidde Beydals --- api/v1beta1/kustomization_types.go | 11 ++++++----- ...kustomize.toolkit.fluxcd.io_kustomizations.yaml | 9 +++++---- docs/api/kustomize.md | 14 ++++++++++---- docs/spec/v1alpha1/kustomization.md | 1 - docs/spec/v1beta1/kustomization.md | 11 ++++++----- 5 files changed, 27 insertions(+), 19 deletions(-) diff --git a/api/v1beta1/kustomization_types.go b/api/v1beta1/kustomization_types.go index 4d955ee7..f2e62b79 100644 --- a/api/v1beta1/kustomization_types.go +++ b/api/v1beta1/kustomization_types.go @@ -45,7 +45,7 @@ type KustomizationSpec struct { // +optional Decryption *Decryption `json:"decryption,omitempty"` - // The interval at which to reconcile the kustomization. + // The interval at which to reconcile the Kustomization. // +required Interval metav1.Duration `json:"interval"` @@ -54,10 +54,11 @@ type KustomizationSpec struct { // +optional KubeConfig *KubeConfig `json:"kubeConfig,omitempty"` - // Path to the directory containing the kustomization file. - // +kubebuilder:validation:Pattern="^\\./" - // +required - Path string `json:"path"` + // Path to the directory containing the kustomization.yaml file, or the + // set of plain YAMLs a kustomization.yaml should be generated for. + // Defaults to 'None', which translates to the root path of the SourceRef. + // +optional + Path string `json:"path,omitempty"` // Prune enables garbage collection. // +required diff --git a/config/crd/bases/kustomize.toolkit.fluxcd.io_kustomizations.yaml b/config/crd/bases/kustomize.toolkit.fluxcd.io_kustomizations.yaml index 08b9c9b0..4c6636a3 100644 --- a/config/crd/bases/kustomize.toolkit.fluxcd.io_kustomizations.yaml +++ b/config/crd/bases/kustomize.toolkit.fluxcd.io_kustomizations.yaml @@ -135,7 +135,7 @@ spec: type: object type: array interval: - description: The interval at which to reconcile the kustomization. + description: The interval at which to reconcile the Kustomization. type: string kubeConfig: description: The KubeConfig for reconciling the Kustomization on a @@ -159,8 +159,10 @@ spec: type: object type: object path: - description: Path to the directory containing the kustomization file. - pattern: ^\./ + description: Path to the directory containing the kustomization.yaml + file, or the set of plain YAMLs a kustomization.yaml should be generated + for. Defaults to 'None', which translates to the root path of the + SourceRef. type: string prune: description: Prune enables garbage collection. @@ -219,7 +221,6 @@ spec: type: string required: - interval - - path - prune - sourceRef type: object diff --git a/docs/api/kustomize.md b/docs/api/kustomize.md index 2c8ab523..f378a809 100644 --- a/docs/api/kustomize.md +++ b/docs/api/kustomize.md @@ -108,7 +108,7 @@ Kubernetes meta/v1.Duration -

The interval at which to reconcile the kustomization.

+

The interval at which to reconcile the Kustomization.

@@ -134,7 +134,10 @@ string -

Path to the directory containing the kustomization file.

+(Optional) +

Path to the directory containing the kustomization.yaml file, or the +set of plain YAMLs a kustomization.yaml should be generated for. +Defaults to ‘None’, which translates to the root path of the SourceRef.

@@ -609,7 +612,7 @@ Kubernetes meta/v1.Duration -

The interval at which to reconcile the kustomization.

+

The interval at which to reconcile the Kustomization.

@@ -635,7 +638,10 @@ string -

Path to the directory containing the kustomization file.

+(Optional) +

Path to the directory containing the kustomization.yaml file, or the +set of plain YAMLs a kustomization.yaml should be generated for. +Defaults to ‘None’, which translates to the root path of the SourceRef.

diff --git a/docs/spec/v1alpha1/kustomization.md b/docs/spec/v1alpha1/kustomization.md index 858a5f5c..883dd064 100644 --- a/docs/spec/v1alpha1/kustomization.md +++ b/docs/spec/v1alpha1/kustomization.md @@ -30,7 +30,6 @@ type KustomizationSpec struct { KubeConfig *KubeConfig `json:"kubeConfig,omitempty"` // Path to the directory containing the kustomization file. - // +kubebuilder:validation:Pattern="^\\./" // +required Path string `json:"path"` diff --git a/docs/spec/v1beta1/kustomization.md b/docs/spec/v1beta1/kustomization.md index 77655916..5fe6d03a 100644 --- a/docs/spec/v1beta1/kustomization.md +++ b/docs/spec/v1beta1/kustomization.md @@ -21,7 +21,7 @@ type KustomizationSpec struct { // +optional Decryption *Decryption `json:"decryption,omitempty"` - // The interval at which to apply the kustomization. + // The interval at which to reconcile the Kustomization. // +required Interval metav1.Duration `json:"interval"` @@ -30,10 +30,11 @@ type KustomizationSpec struct { // +optional KubeConfig *KubeConfig `json:"kubeConfig,omitempty"` - // Path to the directory containing the kustomization.yaml file. - // +kubebuilder:validation:Pattern="^\\./" - // +required - Path string `json:"path"` + // Path to the directory containing the kustomization.yaml file, or the + // set of plain YAMLs a kustomization.yaml should be generated for. + // Defaults to 'None', which translates to the root path of the SourceRef. + // +optional + Path string `json:"path,omitempty"` // Enables garbage collection. // +required From d7a0deac97cf6e13e8081f8aaedbeb9c461ad0bf Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 16 Dec 2020 12:59:19 +0100 Subject: [PATCH 3/3] Write KubeConfig to tmp file in working dir Instead of using the name of the secret, as this can cause unexpected collisions in edge case scenarios. Signed-off-by: Hidde Beydals --- controllers/kustomization_controller.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/controllers/kustomization_controller.go b/controllers/kustomization_controller.go index d5509321..e34e32b5 100644 --- a/controllers/kustomization_controller.go +++ b/controllers/kustomization_controller.go @@ -614,15 +614,15 @@ func (r *KustomizationReconciler) writeKubeConfig(kustomization kustomizev1.Kust return "", err } - kubeConfigPath, err := securejoin.SecureJoin(dirPath, secretName.Name) + f, err := ioutil.TempFile(dirPath, "kubeconfig") + defer f.Close() if err != nil { - return "", err + return "", fmt.Errorf("unable to write KubeConfig secret '%s' to storage: %w", secretName.String(), err) } - if err := ioutil.WriteFile(kubeConfigPath, kubeConfig, os.ModePerm); err != nil { + if _, err := f.Write(kubeConfig); err != nil { return "", fmt.Errorf("unable to write KubeConfig secret '%s' to storage: %w", secretName.String(), err) } - - return kubeConfigPath, nil + return f.Name(), nil } func (r *KustomizationReconciler) getKubeConfig(kustomization kustomizev1.Kustomization) ([]byte, error) {