From 584b5278138aac455d47e25eb9930b85a29db387 Mon Sep 17 00:00:00 2001 From: Scott Date: Wed, 12 May 2021 10:55:44 -0400 Subject: [PATCH] Encode scripts as base 64 to avoid k8s mangling "$$" Kubernetes replaces instances of "$$" in container args fields with "$". This can muck up the contents of script fields because scripts are passed into a TaskRun Pod as an arg to an init container. Prior to this commit we [tried to prevent the replacement](https://github.com/tektoncd/pipeline/pull/3888) from happening by: 1. putting scripts into annotations on a pod and projecting them using downward API - con: the max size of the annotations map is capped to ~250kB. The aggregate size of all scripts in a single Task therefore becomes constrained by this. Any other systems using annotations will reduce the available headroom. Backwards incompatible. 2. replacing instances of "$$" in scripts with "$$$$" for k8s to then process back to "$$" - con: k8s doesn't actually process _all_ instances of "$$". For example, if you write an arg with format "echo $(eval \$$foo)" then k8s will see the first "$(", assume it's a variable reference, and pass it through verbatim. So user's scripts with bash variable become broken by tekton's new replacement. Backwards incompatible. This commit takes a third approach, proposed by @MartinKanters, encoding scripts as base64 in the controller and then having them decoded in the init container. This bypasses Kubernetes' args processing completely because dollar signs aren't used in base64 encodings. It also doesn't introduce a backwards-incompatible limit to the aggregate script size. And it doesn't mangle existing bash scripts with variable replacements. The most noticeable trade-offs we now make are: 1. Tiny scripts can be up to 300% bigger, but as scripts get longer the max increase gets closer to 133%. 2. Also the TaskRun's `initContainer` YAML is a bit less human readable: ``` initContainers: - args: - -c - | tmpfile="/tekton/scripts/script-0-f8fmf" touch ${tmpfile} && chmod +x ${tmpfile} cat > ${tmpfile} << '_EOF_' IyEvYmluL3NoCnNldCAteGUKZWNobyAibm8gc2hlYmFuZyI= _EOF_ /tekton/tools/entrypoint decode-script "${tmpfile}" ``` The entrypoint is extended to decode base64 files so that the `shellImage` (which is used to write scripts to disk for Step containers) is not required to package a `base64` binary. --- cmd/entrypoint/main.go | 38 ++---- cmd/entrypoint/subcommands/cp.go | 46 ++++++++ cmd/entrypoint/subcommands/cp_test.go | 69 +++++++++++ cmd/entrypoint/subcommands/decode_script.go | 71 +++++++++++ .../subcommands/decode_script_test.go | 111 ++++++++++++++++++ cmd/entrypoint/subcommands/subcommands.go | 76 ++++++++++++ .../subcommands/subcommands_test.go | 70 +++++++++++ examples/v1alpha1/taskruns/step-script.yaml | 34 ++++++ examples/v1beta1/taskruns/step-script.yaml | 34 ++++++ pkg/pod/pod.go | 4 +- pkg/pod/pod_test.go | 99 ++++++++++++---- pkg/pod/script.go | 21 ++-- pkg/pod/script_test.go | 70 ++++++----- 13 files changed, 642 insertions(+), 101 deletions(-) create mode 100644 cmd/entrypoint/subcommands/cp.go create mode 100644 cmd/entrypoint/subcommands/cp_test.go create mode 100644 cmd/entrypoint/subcommands/decode_script.go create mode 100644 cmd/entrypoint/subcommands/decode_script_test.go create mode 100644 cmd/entrypoint/subcommands/subcommands.go create mode 100644 cmd/entrypoint/subcommands/subcommands_test.go diff --git a/cmd/entrypoint/main.go b/cmd/entrypoint/main.go index 9e2ecaaf234..be95770e567 100644 --- a/cmd/entrypoint/main.go +++ b/cmd/entrypoint/main.go @@ -18,7 +18,6 @@ package main import ( "flag" - "io" "log" "os" "os/exec" @@ -26,6 +25,7 @@ import ( "syscall" "time" + "github.com/tektoncd/pipeline/cmd/entrypoint/subcommands" "github.com/tektoncd/pipeline/pkg/credentials" "github.com/tektoncd/pipeline/pkg/credentials/dockercreds" "github.com/tektoncd/pipeline/pkg/credentials/gitcreds" @@ -45,25 +45,6 @@ var ( const defaultWaitPollingInterval = time.Second -func cp(src, dst string) error { - s, err := os.Open(src) - if err != nil { - return err - } - defer s.Close() - - // Owner has permission to write and execute, and anybody has - // permission to execute. - d, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE, 0311) - if err != nil { - return err - } - defer d.Close() - - _, err = io.Copy(d, s) - return err -} - func main() { // Add credential flags originally introduced with our legacy credentials helper // image (creds-init). @@ -72,17 +53,14 @@ func main() { flag.Parse() - // If invoked in "cp mode" (`entrypoint cp `), simply copy - // the src path to the dst path. This is used to place the entrypoint - // binary in the tools directory, without requiring the cp command to - // exist in the base image. - if len(flag.Args()) == 3 && flag.Args()[0] == "cp" { - src, dst := flag.Args()[1], flag.Args()[2] - if err := cp(src, dst); err != nil { - log.Fatal(err) + if err := subcommands.Process(flag.Args()); err != nil { + log.Println(err.Error()) + switch err.(type) { + case subcommands.SubcommandSuccessful: + return + default: + os.Exit(1) } - log.Println("Copied", src, "to", dst) - return } // Copy credentials we're expecting from the legacy credentials helper (creds-init) diff --git a/cmd/entrypoint/subcommands/cp.go b/cmd/entrypoint/subcommands/cp.go new file mode 100644 index 00000000000..974e08b436c --- /dev/null +++ b/cmd/entrypoint/subcommands/cp.go @@ -0,0 +1,46 @@ +/* +Copyright 2020 The Tekton Authors + +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 subcommands + +import ( + "io" + "os" +) + +const CopyCommand = "cp" + +// Owner has permission to write and execute, and anybody has +// permission to execute. +const dstPermissions = 0311 + +// cp copies a files from src to dst. +func cp(src, dst string) error { + s, err := os.Open(src) + if err != nil { + return err + } + defer s.Close() + + d, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE, dstPermissions) + if err != nil { + return err + } + defer d.Close() + + _, err = io.Copy(d, s) + return err +} diff --git a/cmd/entrypoint/subcommands/cp_test.go b/cmd/entrypoint/subcommands/cp_test.go new file mode 100644 index 00000000000..534425cbf07 --- /dev/null +++ b/cmd/entrypoint/subcommands/cp_test.go @@ -0,0 +1,69 @@ +/* +Copyright 2020 The Tekton Authors + +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 subcommands + +import ( + "errors" + "io/ioutil" + "os" + "path/filepath" + "testing" +) + +func TestCp(t *testing.T) { + tmp, err := ioutil.TempDir("", "cp-test-*") + if err != nil { + t.Fatalf("error creating temp directory: %v", err) + } + defer os.RemoveAll(tmp) + src := filepath.Join(tmp, "foo.txt") + dst := filepath.Join(tmp, "bar.txt") + + if err = ioutil.WriteFile(src, []byte("hello world"), 0700); err != nil { + t.Fatalf("error writing source file: %v", err) + } + + if err := cp(src, dst); err != nil { + t.Errorf("error copying: %v", err) + } + + info, err := os.Lstat(dst) + if err != nil { + t.Fatalf("error statting destination file: %v", err) + } + + if info.Mode().Perm() != dstPermissions { + t.Errorf("expected permissions %#o for destination file but found %#o", dstPermissions, info.Mode().Perm()) + } +} + +func TestCpMissingFile(t *testing.T) { + tmp, err := ioutil.TempDir("", "cp-test-*") + if err != nil { + t.Fatalf("error creating temp directory: %v", err) + } + defer os.RemoveAll(tmp) + src := filepath.Join(tmp, "doesnt-exist.txt") + dst := filepath.Join(tmp, "bar.txt") + err = cp(src, dst) + if err == nil { + t.Errorf("unexpected success copying missing file") + } + if !errors.Is(err, os.ErrNotExist) { + t.Errorf(`expected "file does not exist" error but received %v`, err) + } +} diff --git a/cmd/entrypoint/subcommands/decode_script.go b/cmd/entrypoint/subcommands/decode_script.go new file mode 100644 index 00000000000..c6a4d7929e8 --- /dev/null +++ b/cmd/entrypoint/subcommands/decode_script.go @@ -0,0 +1,71 @@ +/* +Copyright 2020 The Tekton Authors + +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 subcommands + +import ( + "bytes" + "encoding/base64" + "fmt" + "io" + "io/ioutil" + "os" +) + +const DecodeScriptCommand = "decode-script" + +// decodeScript rewrites a script file from base64 back into its original content from +// the Step definition. +func decodeScript(scriptPath string) error { + decodedBytes, permissions, err := decodeScriptFromFile(scriptPath) + if err != nil { + return fmt.Errorf("error decoding script file %q: %w", scriptPath, err) + } + err = ioutil.WriteFile(scriptPath, decodedBytes, permissions) + if err != nil { + return fmt.Errorf("error writing decoded script file %q: %w", scriptPath, err) + } + return nil +} + +// decodeScriptFromFile reads the script at scriptPath, decodes it from +// base64, and returns the decoded bytes w/ the permissions to use when re-writing +// or an error. +func decodeScriptFromFile(scriptPath string) ([]byte, os.FileMode, error) { + scriptFile, err := os.Open(scriptPath) + if err != nil { + return nil, 0, fmt.Errorf("error reading from script file %q: %w", scriptPath, err) + } + defer scriptFile.Close() + + encoded := bytes.NewBuffer(nil) + if _, err = io.Copy(encoded, scriptFile); err != nil { + return nil, 0, fmt.Errorf("error reading from script file %q: %w", scriptPath, err) + } + + fileInfo, err := scriptFile.Stat() + if err != nil { + return nil, 0, fmt.Errorf("error statting script file %q: %w", scriptPath, err) + } + perms := fileInfo.Mode().Perm() + + decoded := make([]byte, base64.StdEncoding.DecodedLen(encoded.Len())) + n, err := base64.StdEncoding.Decode(decoded, encoded.Bytes()) + if err != nil { + return nil, 0, fmt.Errorf("error decoding script file %q: %w", scriptPath, err) + } + return decoded[0:n], perms, nil +} diff --git a/cmd/entrypoint/subcommands/decode_script_test.go b/cmd/entrypoint/subcommands/decode_script_test.go new file mode 100644 index 00000000000..eb701d48494 --- /dev/null +++ b/cmd/entrypoint/subcommands/decode_script_test.go @@ -0,0 +1,111 @@ +/* +Copyright 2020 The Tekton Authors + +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 subcommands + +import ( + "encoding/base64" + "errors" + "io/ioutil" + "os" + "path/filepath" + "testing" +) + +func TestDecodeScript(t *testing.T) { + encoded := "IyEvdXNyL2Jpbi9lbnYgc2gKZWNobyAiSGVsbG8gV29ybGQhIgo=" + decoded := `#!/usr/bin/env sh +echo "Hello World!" +` + mode := os.FileMode(0600) + expectedPermissions := os.FileMode(0600) + + tmp, err := ioutil.TempDir("", "decode-script-test-*") + if err != nil { + t.Fatalf("error creating temp file: %v", err) + } + src := filepath.Join(tmp, "script.txt") + defer func() { + if err := os.Remove(src); err != nil { + t.Errorf("temporary script file %q was not cleaned up: %v", src, err) + } + }() + if err = ioutil.WriteFile(src, []byte(encoded), mode); err != nil { + t.Fatalf("error writing encoded script: %v", err) + } + + if err = decodeScript(src); err != nil { + t.Errorf("unexpected error decoding script: %v", err) + } + + file, err := os.Open(src) + if err != nil { + t.Fatalf("unexpected error opening decoded script: %v", err) + } + defer file.Close() + info, err := file.Stat() + if err != nil { + t.Fatalf("unexpected error statting decoded script: %v", err) + } + mod := info.Mode() + b, err := ioutil.ReadAll(file) + if err != nil { + t.Fatalf("unexpected error reading content of decoded script: %v", err) + } + if string(b) != decoded { + t.Errorf("expected decoded value %q received %q", decoded, string(b)) + } + if mod != expectedPermissions { + t.Errorf("expected mode %#o received %#o", expectedPermissions, mod) + } +} + +func TestDecodeScriptMissingFileError(t *testing.T) { + b, mod, err := decodeScriptFromFile("/path/to/non-existent/file") + if !errors.Is(err, os.ErrNotExist) { + t.Errorf("expected error %q received %q", os.ErrNotExist, err) + } + if b != nil || mod != 0 { + t.Errorf("unexpected non-zero bytes or file mode returned") + } +} + +func TestDecodeScriptInvalidBase64(t *testing.T) { + invalidData := []byte("!") + expectedError := base64.CorruptInputError(0) + + src, err := ioutil.TempFile("", "decode-script-test-*") + if err != nil { + t.Fatalf("error creating temp file: %v", err) + } + defer func() { + if err := os.Remove(src.Name()); err != nil { + t.Errorf("temporary file %q was not cleaned up: %v", src.Name(), err) + } + }() + if _, err := src.Write(invalidData); err != nil { + t.Fatalf("error writing invalid base64 data: %v", err) + } + src.Close() + + b, mod, err := decodeScriptFromFile(src.Name()) + if b != nil || mod != 0 { + t.Errorf("unexpected non-zero bytes or file mode returned") + } + if !errors.Is(err, expectedError) { + t.Errorf("expected error %q received %q", expectedError, err) + } +} diff --git a/cmd/entrypoint/subcommands/subcommands.go b/cmd/entrypoint/subcommands/subcommands.go new file mode 100644 index 00000000000..6d3cd8b482f --- /dev/null +++ b/cmd/entrypoint/subcommands/subcommands.go @@ -0,0 +1,76 @@ +/* +Copyright 2020 The Tekton Authors + +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 subcommands + +import ( + "fmt" +) + +type SubcommandSuccessful struct { + message string +} + +func (err SubcommandSuccessful) Error() string { + return err.message +} + +type SubcommandError struct { + subcommand string + message string +} + +func (err SubcommandError) Error() string { + return fmt.Sprintf("%s error: %s", err.subcommand, err.message) +} + +// Process takes the set of arguments passed to entrypoint and executes any +// subcommand that the args call for. An error is returned to the caller to +// indicate that a subcommand was matched and to pass back its success/fail +// state. The returned error will be nil if no subcommand was matched to the +// passed args, SubcommandSuccessful if args matched and the subcommand +// succeeded, or any other error if the args matched but the subcommand failed. +func Process(args []string) error { + if len(args) == 0 { + return nil + } + switch args[0] { + case CopyCommand: + // If invoked in "cp mode" (`entrypoint cp `), simply copy + // the src path to the dst path. This is used to place the entrypoint + // binary in the tools directory, without requiring the cp command to + // exist in the base image. + if len(args) == 3 { + src, dst := args[1], args[2] + if err := cp(src, dst); err != nil { + return SubcommandError{subcommand: CopyCommand, message: err.Error()} + } + return SubcommandSuccessful{message: fmt.Sprintf("Copied %s to %s", src, dst)} + } + case DecodeScriptCommand: + // If invoked in "decode-script" mode (`entrypoint decode-script `), + // read the script at and overwrite it with its decoded content. + if len(args) == 2 { + src := args[1] + if err := decodeScript(src); err != nil { + return SubcommandError{subcommand: DecodeScriptCommand, message: err.Error()} + } + return SubcommandSuccessful{message: fmt.Sprintf("Decoded script %s", src)} + } + default: + } + return nil +} diff --git a/cmd/entrypoint/subcommands/subcommands_test.go b/cmd/entrypoint/subcommands/subcommands_test.go new file mode 100644 index 00000000000..666bcd8e495 --- /dev/null +++ b/cmd/entrypoint/subcommands/subcommands_test.go @@ -0,0 +1,70 @@ +package subcommands + +import ( + "io/ioutil" + "os" + "path/filepath" + "testing" +) + +const helloWorldBase64 = "aGVsbG8gd29ybGQK" + +// TestProcessSuccessfulSubcommands checks that input args matching the format +// expected by subcommands results in successfully running those subcommands. +func TestProcessSuccessfulSubcommands(t *testing.T) { + tmp, err := ioutil.TempDir("", "cp-test-*") + if err != nil { + t.Fatalf("error creating temp directory: %v", err) + } + defer os.RemoveAll(tmp) + src := filepath.Join(tmp, "foo.txt") + dst := filepath.Join(tmp, "bar.txt") + + srcFile, err := os.OpenFile(src, os.O_WRONLY|os.O_CREATE, 0666) + if err != nil { + t.Fatalf("error opening temp file for writing: %v", err) + } + defer srcFile.Close() + if _, err := srcFile.Write([]byte(helloWorldBase64)); err != nil { + t.Fatalf("error writing source file: %v", err) + } + + returnValue := Process([]string{CopyCommand, src, dst}) + if _, ok := returnValue.(SubcommandSuccessful); !ok { + t.Errorf("unexpected return value from cp command: %v", returnValue) + } + + returnValue = Process([]string{DecodeScriptCommand, src}) + if _, ok := returnValue.(SubcommandSuccessful); !ok { + t.Errorf("unexpected return value from decode-script command: %v", returnValue) + } +} + +// TestProcessIgnoresNonSubcommands checks that any input to Process which +// does not exactly match the requirements of a configured subcommand +// correctly passes back a nil result. +func TestProcessIgnoresNonSubcommands(t *testing.T) { + if err := Process([]string{"not", "a", "matching", "subcommand"}); err != nil { + t.Errorf("unexpected error processing unmatched subcommand: %v", err) + } + + if err := Process([]string{}); err != nil { + t.Errorf("unexpected error processing 0 args: %v", err) + } + + if err := Process([]string{CopyCommand}); err != nil { + t.Errorf("unexpected error processing decode-script command with 0 additional args: %v", err) + } + + if err := Process([]string{CopyCommand, "foo.txt", "bar.txt", "baz.txt"}); err != nil { + t.Errorf("unexpected error processing cp command with invalid number of args: %v", err) + } + + if err := Process([]string{DecodeScriptCommand}); err != nil { + t.Errorf("unexpected error processing decode-script command with 0 additional args: %v", err) + } + + if err := Process([]string{DecodeScriptCommand, "foo.txt", "bar.txt"}); err != nil { + t.Errorf("unexpected error processing decode-script command with invalid number of args: %v", err) + } +} diff --git a/examples/v1alpha1/taskruns/step-script.yaml b/examples/v1alpha1/taskruns/step-script.yaml index 6eaf5fcac64..bc8b2fe6bc4 100644 --- a/examples/v1alpha1/taskruns/step-script.yaml +++ b/examples/v1alpha1/taskruns/step-script.yaml @@ -97,3 +97,37 @@ spec: [[ $# == 2 ]] [[ $1 == "hello" ]] [[ $2 == "world" ]] + + # Test that multiple dollar signs next to each other are not replaced by Kubernetes + - name: dollar-signs-allowed + image: python + script: | + #!/usr/bin/env python3 + if '$' != '\u0024': + print('single dollar signs ($) are not passed through as expected :(') + exit(1) + if '$$' != '\u0024\u0024': + print('double dollar signs ($$) are not passed through as expected :(') + exit(2) + if '$$$' != '\u0024\u0024\u0024': + print('three dollar signs ($$$) are not passed through as expected :(') + exit(3) + if '$$$$' != '\u0024\u0024\u0024\u0024': + print('four dollar signs ($$$$) are not passed through as expected :(') + exit(4) + print('dollar signs appear to be handled correctly! :)') + + # Test that bash scripts with variable evaluations work as expected + - name: bash-variable-evaluations + image: bash:5.1.8 + script: | + #!/usr/bin/env bash + set -xe + var1=var1_value + var2=var1 + echo $(eval echo \$$var2) > tmpfile + eval_result=$(cat tmpfile) + if [ "$eval_result" != "var1_value" ] ; then + echo "unexpected eval result: $eval_result" + exit 1 + fi diff --git a/examples/v1beta1/taskruns/step-script.yaml b/examples/v1beta1/taskruns/step-script.yaml index 19cb06c7dac..0e0315fbde1 100644 --- a/examples/v1beta1/taskruns/step-script.yaml +++ b/examples/v1beta1/taskruns/step-script.yaml @@ -96,3 +96,37 @@ spec: [[ $# == 2 ]] [[ $1 == "hello" ]] [[ $2 == "world" ]] + + # Test that multiple dollar signs next to each other are not replaced by Kubernetes + - name: dollar-signs-allowed + image: python + script: | + #!/usr/bin/env python3 + if '$' != '\u0024': + print('single dollar signs ($) are not passed through as expected :(') + exit(1) + if '$$' != '\u0024\u0024': + print('double dollar signs ($$) are not passed through as expected :(') + exit(2) + if '$$$' != '\u0024\u0024\u0024': + print('three dollar signs ($$$) are not passed through as expected :(') + exit(3) + if '$$$$' != '\u0024\u0024\u0024\u0024': + print('four dollar signs ($$$$) are not passed through as expected :(') + exit(4) + print('dollar signs appear to be handled correctly! :)') + + # Test that bash scripts with variable evaluations work as expected + - name: bash-variable-evaluations + image: bash:5.1.8 + script: | + #!/usr/bin/env bash + set -xe + var1=var1_value + var2=var1 + echo $(eval echo \$$var2) > tmpfile + eval_result=$(cat tmpfile) + if [ "$eval_result" != "var1_value" ] ; then + echo "unexpected eval result: $eval_result" + exit 1 + fi diff --git a/pkg/pod/pod.go b/pkg/pod/pod.go index 4698600879d..0f245b82fae 100644 --- a/pkg/pod/pod.go +++ b/pkg/pod/pod.go @@ -154,7 +154,9 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1beta1.TaskRun, taskSpec if err != nil { return nil, err } - initContainers = append(initContainers, entrypointInit) + // place the entrypoint first in case other init containers rely on its + // features (e.g. decode-script). + initContainers = append([]corev1.Container{entrypointInit}, initContainers...) volumes = append(volumes, toolsVolume, downwardVolume) limitRangeMin, err := getLimitRangeMinimum(ctx, taskRun.Namespace, b.KubeClient) diff --git a/pkg/pod/pod_test.go b/pkg/pod/pod_test.go index d227be7b070..6e6b57b1ec9 100644 --- a/pkg/pod/pod_test.go +++ b/pkg/pod/pod_test.go @@ -438,6 +438,7 @@ func TestPodBuild(t *testing.T) { want: &corev1.PodSpec{ RestartPolicy: corev1.RestartPolicyNever, InitContainers: []corev1.Container{ + placeToolsInit, { Name: "working-dir-initializer", Image: images.ShellImage, @@ -446,7 +447,6 @@ func TestPodBuild(t *testing.T) { WorkingDir: pipeline.WorkspaceDir, VolumeMounts: implicitVolumeMounts, }, - placeToolsInit, }, Containers: []corev1.Container{{ Name: "step-name", @@ -550,20 +550,20 @@ func TestPodBuild(t *testing.T) { want: &corev1.PodSpec{ RestartPolicy: corev1.RestartPolicyNever, InitContainers: []corev1.Container{ + placeToolsInit, { Name: "place-scripts", Image: "busybox", Command: []string{"sh"}, - VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount}, + VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount, toolsMount}, Args: []string{"-c", `tmpfile="/tekton/scripts/sidecar-script-0-9l9zj" touch ${tmpfile} && chmod +x ${tmpfile} -cat > ${tmpfile} << 'sidecar-script-heredoc-randomly-generated-mz4c7' -#!/bin/sh -echo hello from sidecar -sidecar-script-heredoc-randomly-generated-mz4c7 +cat > ${tmpfile} << '_EOF_' +IyEvYmluL3NoCmVjaG8gaGVsbG8gZnJvbSBzaWRlY2Fy +_EOF_ +/tekton/tools/entrypoint decode-script "${tmpfile}" `}, }, - placeToolsInit, }, Containers: []corev1.Container{{ Name: "step-primary-name", @@ -779,32 +779,27 @@ print("Hello from Python")`, want: &corev1.PodSpec{ RestartPolicy: corev1.RestartPolicyNever, InitContainers: []corev1.Container{ + placeToolsInit, { Name: "place-scripts", Image: images.ShellImage, Command: []string{"sh"}, Args: []string{"-c", `tmpfile="/tekton/scripts/script-0-9l9zj" touch ${tmpfile} && chmod +x ${tmpfile} -cat > ${tmpfile} << 'script-heredoc-randomly-generated-mz4c7' -#!/bin/sh -echo hello from step one -script-heredoc-randomly-generated-mz4c7 -tmpfile="/tekton/scripts/script-1-mssqb" +cat > ${tmpfile} << '_EOF_' +IyEvYmluL3NoCmVjaG8gaGVsbG8gZnJvbSBzdGVwIG9uZQ== +_EOF_ +/tekton/tools/entrypoint decode-script "${tmpfile}" +tmpfile="/tekton/scripts/script-1-mz4c7" touch ${tmpfile} && chmod +x ${tmpfile} -cat > ${tmpfile} << 'script-heredoc-randomly-generated-78c5n' -#!/usr/bin/env python -print("Hello from Python") -script-heredoc-randomly-generated-78c5n +cat > ${tmpfile} << '_EOF_' +IyEvdXNyL2Jpbi9lbnYgcHl0aG9uCnByaW50KCJIZWxsbyBmcm9tIFB5dGhvbiIp +_EOF_ +/tekton/tools/entrypoint decode-script "${tmpfile}" `}, - VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount}, + VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount, toolsMount}, }, - { - Name: "place-tools", - WorkingDir: "/", - Image: images.EntrypointImage, - Command: []string{"/ko-app/entrypoint", "cp", "/ko-app/entrypoint", "/tekton/tools/entrypoint"}, - VolumeMounts: []corev1.VolumeMount{toolsMount}, - }}, + }, Containers: []corev1.Container{{ Name: "step-one", Image: "image", @@ -842,7 +837,7 @@ script-heredoc-randomly-generated-78c5n "-termination_path", "/tekton/termination", "-entrypoint", - "/tekton/scripts/script-1-mssqb", + "/tekton/scripts/script-1-mz4c7", "--", "template", "args", @@ -891,6 +886,60 @@ script-heredoc-randomly-generated-78c5n VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory}}, }), }, + }, { + desc: "step with script that uses two dollar signs", + ts: v1beta1.TaskSpec{ + Steps: []v1beta1.Step{{ + Container: corev1.Container{ + Name: "one", + Image: "image", + }, + Script: "#!/bin/sh\n$$", + }}, + }, + want: &corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyNever, + InitContainers: []corev1.Container{placeToolsInit, { + Name: "place-scripts", + Image: images.ShellImage, + Command: []string{"sh"}, + Args: []string{"-c", `tmpfile="/tekton/scripts/script-0-9l9zj" +touch ${tmpfile} && chmod +x ${tmpfile} +cat > ${tmpfile} << '_EOF_' +IyEvYmluL3NoCiQk +_EOF_ +/tekton/tools/entrypoint decode-script "${tmpfile}" +`}, + VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount, toolsMount}, + }}, + Containers: []corev1.Container{{ + Name: "step-one", + Image: "image", + Command: []string{"/tekton/tools/entrypoint"}, + Args: []string{ + "-wait_file", + "/tekton/downward/ready", + "-wait_file_content", + "-post_file", + "/tekton/tools/0", + "-termination_path", + "/tekton/termination", + "-entrypoint", + "/tekton/scripts/script-0-9l9zj", + "--", + }, + VolumeMounts: append([]corev1.VolumeMount{scriptsVolumeMount, toolsMount, downwardMount, { + Name: "tekton-creds-init-home-0", + MountPath: "/tekton/creds", + }}, implicitVolumeMounts...), + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, + TerminationMessagePath: "/tekton/termination", + }}, + Volumes: append(implicitVolumes, scriptsVolume, toolsVolume, downwardVolume, corev1.Volume{ + Name: "tekton-creds-init-home-0", + VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory}}, + }), + }, }, { desc: "using another scheduler", ts: v1beta1.TaskSpec{ diff --git a/pkg/pod/script.go b/pkg/pod/script.go index 288c6875cc1..cbcce2d6957 100644 --- a/pkg/pod/script.go +++ b/pkg/pod/script.go @@ -17,6 +17,7 @@ limitations under the License. package pod import ( + "encoding/base64" "fmt" "path/filepath" "strings" @@ -58,7 +59,7 @@ func convertScripts(shellImage string, steps []v1beta1.Step, sidecars []v1beta1. Image: shellImage, Command: []string{"sh"}, Args: []string{"-c", ""}, - VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount}, + VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount, toolsMount}, } convertedStepContainers := convertListOfSteps(steps, &placeScriptsInit, &placeScripts, "script") @@ -107,22 +108,18 @@ func convertListOfSteps(steps []v1beta1.Step, initContainer *corev1.Container, p // non-nil init container. *placeScripts = true + script = encodeScript(script) + // Append to the place-scripts script to place the // script file in a known location in the scripts volume. tmpFile := filepath.Join(scriptsDir, names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("%s-%d", namePrefix, i))) - // heredoc is the "here document" placeholder string - // used to cat script contents into the file. Typically - // this is the string "EOF" but if this value were - // "EOF" it would prevent users from including the - // string "EOF" in their own scripts. Instead we - // randomly generate a string to (hopefully) prevent - // collisions. - heredoc := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("%s-heredoc-randomly-generated", namePrefix)) + heredoc := "_EOF_" // underscores because base64 doesnt include them in its alphabet initContainer.Args[1] += fmt.Sprintf(`tmpfile="%s" touch ${tmpfile} && chmod +x ${tmpfile} cat > ${tmpfile} << '%s' %s %s +/tekton/tools/entrypoint decode-script "${tmpfile}" `, tmpFile, heredoc, script, heredoc) // Set the command to execute the correct script in the mounted @@ -136,3 +133,9 @@ cat > ${tmpfile} << '%s' } return containers } + +// encodeScript encodes a script field into a format that avoids kubernetes' built-in processing of container args, +// which can mangle dollar signs and unexpectedly replace variable references in the user's script. +func encodeScript(script string) string { + return base64.StdEncoding.EncodeToString([]byte(script)) +} diff --git a/pkg/pod/script_test.go b/pkg/pod/script_test.go index 940b7ceeedd..d78538fb05e 100644 --- a/pkg/pod/script_test.go +++ b/pkg/pod/script_test.go @@ -160,26 +160,24 @@ script-3`, Command: []string{"sh"}, Args: []string{"-c", `tmpfile="/tekton/scripts/script-0-9l9zj" touch ${tmpfile} && chmod +x ${tmpfile} -cat > ${tmpfile} << 'script-heredoc-randomly-generated-mz4c7' -#!/bin/sh -script-1 -script-heredoc-randomly-generated-mz4c7 -tmpfile="/tekton/scripts/script-2-mssqb" +cat > ${tmpfile} << '_EOF_' +IyEvYmluL3NoCnNjcmlwdC0x +_EOF_ +/tekton/tools/entrypoint decode-script "${tmpfile}" +tmpfile="/tekton/scripts/script-2-mz4c7" touch ${tmpfile} && chmod +x ${tmpfile} -cat > ${tmpfile} << 'script-heredoc-randomly-generated-78c5n' - -#!/bin/sh -script-3 -script-heredoc-randomly-generated-78c5n -tmpfile="/tekton/scripts/script-3-6nl7g" +cat > ${tmpfile} << '_EOF_' +CiMhL2Jpbi9zaApzY3JpcHQtMw== +_EOF_ +/tekton/tools/entrypoint decode-script "${tmpfile}" +tmpfile="/tekton/scripts/script-3-mssqb" touch ${tmpfile} && chmod +x ${tmpfile} -cat > ${tmpfile} << 'script-heredoc-randomly-generated-j2tds' -#!/bin/sh -set -xe -no-shebang -script-heredoc-randomly-generated-j2tds +cat > ${tmpfile} << '_EOF_' +IyEvYmluL3NoCnNldCAteGUKbm8tc2hlYmFuZw== +_EOF_ +/tekton/tools/entrypoint decode-script "${tmpfile}" `}, - VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount}, + VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount, toolsMount}, } want := []corev1.Container{{ Image: "step-1", @@ -189,12 +187,12 @@ script-heredoc-randomly-generated-j2tds Image: "step-2", }, { Image: "step-3", - Command: []string{"/tekton/scripts/script-2-mssqb"}, + Command: []string{"/tekton/scripts/script-2-mz4c7"}, Args: []string{"my", "args"}, VolumeMounts: append(preExistingVolumeMounts, scriptsVolumeMount), }, { Image: "step-3", - Command: []string{"/tekton/scripts/script-3-6nl7g"}, + Command: []string{"/tekton/scripts/script-3-mssqb"}, Args: []string{"my", "args"}, VolumeMounts: []corev1.VolumeMount{ {Name: "pre-existing-volume-mount", MountPath: "/mount/path"}, @@ -251,24 +249,24 @@ sidecar-1`, Command: []string{"sh"}, Args: []string{"-c", `tmpfile="/tekton/scripts/script-0-9l9zj" touch ${tmpfile} && chmod +x ${tmpfile} -cat > ${tmpfile} << 'script-heredoc-randomly-generated-mz4c7' -#!/bin/sh -script-1 -script-heredoc-randomly-generated-mz4c7 -tmpfile="/tekton/scripts/script-2-mssqb" +cat > ${tmpfile} << '_EOF_' +IyEvYmluL3NoCnNjcmlwdC0x +_EOF_ +/tekton/tools/entrypoint decode-script "${tmpfile}" +tmpfile="/tekton/scripts/script-2-mz4c7" touch ${tmpfile} && chmod +x ${tmpfile} -cat > ${tmpfile} << 'script-heredoc-randomly-generated-78c5n' -#!/bin/sh -script-3 -script-heredoc-randomly-generated-78c5n -tmpfile="/tekton/scripts/sidecar-script-0-6nl7g" +cat > ${tmpfile} << '_EOF_' +IyEvYmluL3NoCnNjcmlwdC0z +_EOF_ +/tekton/tools/entrypoint decode-script "${tmpfile}" +tmpfile="/tekton/scripts/sidecar-script-0-mssqb" touch ${tmpfile} && chmod +x ${tmpfile} -cat > ${tmpfile} << 'sidecar-script-heredoc-randomly-generated-j2tds' -#!/bin/sh -sidecar-1 -sidecar-script-heredoc-randomly-generated-j2tds +cat > ${tmpfile} << '_EOF_' +IyEvYmluL3NoCnNpZGVjYXItMQ== +_EOF_ +/tekton/tools/entrypoint decode-script "${tmpfile}" `}, - VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount}, + VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount, toolsMount}, } want := []corev1.Container{{ Image: "step-1", @@ -278,7 +276,7 @@ sidecar-script-heredoc-randomly-generated-j2tds Image: "step-2", }, { Image: "step-3", - Command: []string{"/tekton/scripts/script-2-mssqb"}, + Command: []string{"/tekton/scripts/script-2-mz4c7"}, Args: []string{"my", "args"}, VolumeMounts: []corev1.VolumeMount{ {Name: "pre-existing-volume-mount", MountPath: "/mount/path"}, @@ -289,7 +287,7 @@ sidecar-script-heredoc-randomly-generated-j2tds wantSidecars := []corev1.Container{{ Image: "sidecar-1", - Command: []string{"/tekton/scripts/sidecar-script-0-6nl7g"}, + Command: []string{"/tekton/scripts/sidecar-script-0-mssqb"}, VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount}, }} if d := cmp.Diff(wantInit, gotInit); d != "" {