From 5f561e19d8cf280a9ab068c431a5681404ab435a Mon Sep 17 00:00:00 2001 From: Charles Cheng Date: Mon, 10 Aug 2026 15:59:40 +0800 Subject: [PATCH 1/2] fix(executor)!: give input artifacts without `mode` a default of 0600 Artifact repositories generally cannot store file permissions, so an artifact only kept the permissions it was created with if it was archived: a tarball restores the modes recorded in its headers, while an artifact stored with `archive: none` arrives with whatever mode its driver created the files with. The same artifact therefore landed with different permissions depending on how the template that produced it chose to store it. Give an artifact that does not set `mode` a well known set of permissions instead: 0600 for its files and 0700 for its directories, which have to stay traversable. This covers the whole artifact, as an artifact stored as-is can be a directory too. Symlinks are skipped, since os.Chmod follows them and untar preserves the symlinks within an artifact, so following one could change the permissions of a file outside it. Signed-off-by: Charles Cheng --- docs/upgrading.md | 25 ++++++++++++ docs/walk-through/artifacts.md | 3 ++ workflow/executor/executor.go | 48 ++++++++++++++++++++++ workflow/executor/executor_test.go | 64 ++++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+) diff --git a/docs/upgrading.md b/docs/upgrading.md index 1104e3db4440..7aece5d087f1 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -29,6 +29,31 @@ This variable controlled whether to write workflow updates back to the informer Alternative mechanisms now prevent reprocessing, making both behaviors unnecessary. If you have this variable set, it can be safely removed from your configuration. +### Input artifacts without `mode` now default to `0600` + +An input artifact that does not set `mode` is now given a fixed set of permissions once it has been loaded: `0600` for its files and `0700` for its directories ([#14792](https://github.com/argoproj/argo-workflows/issues/14792)). +This covers the whole artifact, not only its top level, since an artifact can be a directory. +Symlinks within an artifact are left alone, and artifacts loaded by an artifact plugin keep their existing `0666` default. +Setting `mode` still overrides all of this, and `recurseMode` continues to control how an explicit `mode` is applied. + +Previously no default was applied, so the permissions an artifact ended up with depended on how it had been stored rather than on the template consuming it. +A tarball (the default `archive` strategy) restores the modes recorded in its headers, so a file saved as `0644` was loaded as `0644`; the same file saved with `archive: none: {}` is stored without any permission metadata and was loaded as `0600`. +Object stores generally cannot preserve filesystem permissions, and some artifact repositories (`http`, for example) have nowhere to record them at all, so the producing template's permissions were never something a consuming template could rely on. + +Set `mode` (and `recurseMode` for a directory) explicitly on any input artifact that needs more than owner access. +The two cases most likely to need it are a file that has to stay executable, and a `main` container that runs as a different user than the one that loaded the artifact — `0600` and `0700` are owner-only, so a differing `runAsUser` can no longer read the artifact: + +```yaml +inputs: + artifacts: + - name: my-script + path: /tmp/my-script.sh + mode: 0755 +``` + +Note that when an artifact's `path` falls inside a volume you mounted yourself, it is loaded into that volume rather than into the `input-artifacts` volume. +For a `persistentVolumeClaim` the new permissions therefore outlive the pod, so a later step reading the file directly from the claim sees them too. + ## Upgrading to v4.0.7 and v3.7.16 ### Outputs of skipped and omitted steps and tasks now resolve diff --git a/docs/walk-through/artifacts.md b/docs/walk-through/artifacts.md index 8c0ab8caee83..2cb1e7a8d073 100644 --- a/docs/walk-through/artifacts.md +++ b/docs/walk-through/artifacts.md @@ -132,6 +132,9 @@ For example: It is good practice to specify the `mode` of the input file, to ensure your container code can always interact with it (reading/writing/executing) as expected. [This article](https://www.redhat.com/en/blog/linux-file-permissions-explained) explains file permissions and octal values. +An input artifact that does not specify `mode` is loaded with a fixed set of permissions: `0600` for its files and `0700` for its directories. +Do not rely on the permissions the template that produced the artifact gave it: an artifact repository generally cannot store file permissions, so they only survive if the artifact was archived (see [Archive Strategy](#archive-strategy) above). + For example, to allow the user to execute `/bin/kubectl`, we set `mode: 0755`. ```yaml diff --git a/workflow/executor/executor.go b/workflow/executor/executor.go index 4adf454158c0..c4a318a3b8a2 100644 --- a/workflow/executor/executor.go +++ b/workflow/executor/executor.go @@ -357,6 +357,11 @@ func (we *WorkflowExecutor) loadArtifact(ctx context.Context, pluginName wfv1.Ar logger.WithError(err).Error(ctx, "Failed to chmod plugin artifact") return err } + } else { + err = chmodDefault(artPath) + if err != nil { + return err + } } return nil } @@ -1331,6 +1336,49 @@ func unpack(srcPath string, destPath string, decompressor func(string, string) e return nil } +const ( + // defaultArtifactFileMode is applied to the files of an input artifact that does not + // set `mode`. + defaultArtifactFileMode = 0o600 + // defaultArtifactDirMode is defaultArtifactFileMode with the execute bit added, as a + // directory has to be traversable to be of any use. + defaultArtifactDirMode = 0o700 +) + +// chmodDefault gives an input artifact that does not set `mode` a well known set of +// permissions: defaultArtifactFileMode for its files and defaultArtifactDirMode for its +// directories. Without it an artifact keeps whatever permissions its archive strategy +// happened to carry - a tarball restores the modes recorded in its headers, while an +// artifact stored as-is only has the modes its driver created the files with, as object +// stores do not store permissions - so the same artifact arrives with different +// permissions depending on how the template that produced it chose to store it. +// +// This covers the whole artifact rather than just its root, because an artifact stored +// as-is can be a directory too. `recurseMode` is deliberately not consulted: it selects +// how an explicit `mode` is applied, and these are the defaults for its absence. +// +// Symlinks are skipped, as os.Chmod follows them and untar preserves the symlinks within +// an artifact, so following one could change the permissions of a file outside it. +func chmodDefault(artPath string) error { + err := filepath.WalkDir(artPath, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + switch { + case d.IsDir(): + return os.Chmod(path, defaultArtifactDirMode) + case d.Type().IsRegular(): + return os.Chmod(path, defaultArtifactFileMode) + default: + return nil + } + }) + if err != nil { + return argoerrs.InternalWrapError(err) + } + return nil +} + func chmod(artPath string, mode int32, recurse bool) error { err := os.Chmod(artPath, os.FileMode(mode)) if err != nil { diff --git a/workflow/executor/executor_test.go b/workflow/executor/executor_test.go index 4c5f5dd91799..19eb0eaa1710 100644 --- a/workflow/executor/executor_test.go +++ b/workflow/executor/executor_test.go @@ -483,6 +483,70 @@ func TestChmod(t *testing.T) { } } +func TestChmodDefault(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("chmod does not work in windows") + } + + // A single file artifact, which is what `archive: none` gives you for a file. Its + // mode is whatever the driver created it with. + t.Run("single file", func(t *testing.T) { + artPath := filepath.Join(t.TempDir(), "myfile.txt") + require.NoError(t, os.WriteFile(artPath, []byte("test content\n"), 0o644)) + + require.NoError(t, chmodDefault(artPath)) + + info, err := os.Stat(artPath) + require.NoError(t, err) + assert.Equal(t, "-rw-------", info.Mode().String()) + }) + + // A directory artifact, as `untar` or a directory download leaves it. The whole + // tree is covered, and directories keep the execute bit so they can be entered. + t.Run("directory", func(t *testing.T) { + root := t.TempDir() + nested := filepath.Join(root, "nested") + require.NoError(t, os.Mkdir(nested, 0o755)) + file := filepath.Join(nested, "script.sh") + require.NoError(t, os.WriteFile(file, []byte("#!/bin/sh\n"), 0o755)) + require.NoError(t, os.Chmod(root, 0o755)) + + require.NoError(t, chmodDefault(root)) + + want := map[string]string{root: "drwx------", nested: "drwx------", file: "-rw-------"} + for path, mode := range want { + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, mode, info.Mode().String(), path) + } + }) + + // os.Chmod follows symlinks, and untar preserves the symlinks in an artifact, so + // following one would chmod a file outside the artifact. + t.Run("does not follow symlinks out of the artifact", func(t *testing.T) { + outside := filepath.Join(t.TempDir(), "outside.txt") + require.NoError(t, os.WriteFile(outside, []byte("not part of the artifact\n"), 0o644)) + + root := t.TempDir() + require.NoError(t, os.Symlink(outside, filepath.Join(root, "link"))) + + require.NoError(t, chmodDefault(root)) + + info, err := os.Stat(outside) + require.NoError(t, err) + assert.Equal(t, "-rw-r--r--", info.Mode().String()) + }) + + // A dangling symlink must not fail the artifact load, which did not chmod at all + // before this default existed. + t.Run("dangling symlink artifact", func(t *testing.T) { + artPath := filepath.Join(t.TempDir(), "link") + require.NoError(t, os.Symlink("/does/not/exist", artPath)) + + require.NoError(t, chmodDefault(artPath)) + }) +} + func TestSaveArtifacts(t *testing.T) { fakeClientset := fake.NewClientset() mockRuntimeExecutor := mocks.ContainerRuntimeExecutor{} From f58ca22ea0b647a2a12daefc831e926ad355475b Mon Sep 17 00:00:00 2001 From: Charles Cheng Date: Tue, 11 Aug 2026 12:49:54 +0800 Subject: [PATCH 2/2] fix(executor): use 0644/0755 for the default artifact mode, not 0600/0700 An artifact-plugin sidecar reads a saved output through its own mount of the main container's filesystem, and is not guaranteed to run as the same user as the container that loaded the input artifact. A 0600 default made that read fail with permission denied, caught by TestArtifactsSuite/TestOutputOnInputPlugin. 0644/0755 stays readable across containers while still restricting writes to the owner. Also rewrites the mode-selection if/else-if/else as a switch to satisfy golangci-lint's gocritic ifElseChain check. Signed-off-by: Charles Cheng --- docs/upgrading.md | 9 +++++---- docs/walk-through/artifacts.md | 2 +- workflow/executor/executor.go | 15 +++++++++------ workflow/executor/executor_test.go | 4 ++-- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/docs/upgrading.md b/docs/upgrading.md index 7aece5d087f1..321eb9c93e0b 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -29,9 +29,9 @@ This variable controlled whether to write workflow updates back to the informer Alternative mechanisms now prevent reprocessing, making both behaviors unnecessary. If you have this variable set, it can be safely removed from your configuration. -### Input artifacts without `mode` now default to `0600` +### Input artifacts without `mode` now default to `0644` -An input artifact that does not set `mode` is now given a fixed set of permissions once it has been loaded: `0600` for its files and `0700` for its directories ([#14792](https://github.com/argoproj/argo-workflows/issues/14792)). +An input artifact that does not set `mode` is now given a fixed set of permissions once it has been loaded: `0644` for its files and `0755` for its directories ([#14792](https://github.com/argoproj/argo-workflows/issues/14792)). This covers the whole artifact, not only its top level, since an artifact can be a directory. Symlinks within an artifact are left alone, and artifacts loaded by an artifact plugin keep their existing `0666` default. Setting `mode` still overrides all of this, and `recurseMode` continues to control how an explicit `mode` is applied. @@ -39,9 +39,10 @@ Setting `mode` still overrides all of this, and `recurseMode` continues to contr Previously no default was applied, so the permissions an artifact ended up with depended on how it had been stored rather than on the template consuming it. A tarball (the default `archive` strategy) restores the modes recorded in its headers, so a file saved as `0644` was loaded as `0644`; the same file saved with `archive: none: {}` is stored without any permission metadata and was loaded as `0600`. Object stores generally cannot preserve filesystem permissions, and some artifact repositories (`http`, for example) have nowhere to record them at all, so the producing template's permissions were never something a consuming template could rely on. +The new default is world-readable rather than owner-only, since a step that saves the artifact through an artifact plugin reads it from a separate sidecar container that is not guaranteed to run as the same user as the container that loaded it. -Set `mode` (and `recurseMode` for a directory) explicitly on any input artifact that needs more than owner access. -The two cases most likely to need it are a file that has to stay executable, and a `main` container that runs as a different user than the one that loaded the artifact — `0600` and `0700` are owner-only, so a differing `runAsUser` can no longer read the artifact: +Set `mode` (and `recurseMode` for a directory) explicitly on any input artifact that needs more than this. +The main case that still needs it is a file that has to stay executable, or one that a container running as a different user than the one that loaded it needs to write to — `0644` gives the owner no execute bit and gives no one but the owner write access: ```yaml inputs: diff --git a/docs/walk-through/artifacts.md b/docs/walk-through/artifacts.md index 2cb1e7a8d073..c1cb5be9961d 100644 --- a/docs/walk-through/artifacts.md +++ b/docs/walk-through/artifacts.md @@ -132,7 +132,7 @@ For example: It is good practice to specify the `mode` of the input file, to ensure your container code can always interact with it (reading/writing/executing) as expected. [This article](https://www.redhat.com/en/blog/linux-file-permissions-explained) explains file permissions and octal values. -An input artifact that does not specify `mode` is loaded with a fixed set of permissions: `0600` for its files and `0700` for its directories. +An input artifact that does not specify `mode` is loaded with a fixed set of permissions: `0644` for its files and `0755` for its directories. Do not rely on the permissions the template that produced the artifact gave it: an artifact repository generally cannot store file permissions, so they only survive if the artifact was archived (see [Archive Strategy](#archive-strategy) above). For example, to allow the user to execute `/bin/kubectl`, we set `mode: 0755`. diff --git a/workflow/executor/executor.go b/workflow/executor/executor.go index c4a318a3b8a2..a97d11b877d2 100644 --- a/workflow/executor/executor.go +++ b/workflow/executor/executor.go @@ -344,12 +344,13 @@ func (we *WorkflowExecutor) loadArtifact(ctx context.Context, pluginName wfv1.Ar } logger.WithField("path", artPath).Info(ctx, "Successfully download file") - if art.Mode != nil { + switch { + case art.Mode != nil: err = chmod(artPath, *art.Mode, art.RecurseMode) if err != nil { return err } - } else if driverArt.Plugin != nil { + case driverArt.Plugin != nil: // For plugin artifacts without explicit mode, ensure the file is writable // by setting mode to 0666 so the main container can read/write it err = chmod(artPath, 0666, art.RecurseMode) @@ -357,7 +358,7 @@ func (we *WorkflowExecutor) loadArtifact(ctx context.Context, pluginName wfv1.Ar logger.WithError(err).Error(ctx, "Failed to chmod plugin artifact") return err } - } else { + default: err = chmodDefault(artPath) if err != nil { return err @@ -1338,11 +1339,13 @@ func unpack(srcPath string, destPath string, decompressor func(string, string) e const ( // defaultArtifactFileMode is applied to the files of an input artifact that does not - // set `mode`. - defaultArtifactFileMode = 0o600 + // set `mode`. It is world-readable rather than owner-only because an artifact-plugin + // output step reads the file from a separate sidecar container, which is not + // guaranteed to run as the same user as the container that loaded the artifact. + defaultArtifactFileMode = 0o644 // defaultArtifactDirMode is defaultArtifactFileMode with the execute bit added, as a // directory has to be traversable to be of any use. - defaultArtifactDirMode = 0o700 + defaultArtifactDirMode = 0o755 ) // chmodDefault gives an input artifact that does not set `mode` a well known set of diff --git a/workflow/executor/executor_test.go b/workflow/executor/executor_test.go index 19eb0eaa1710..5f18e3d00b51 100644 --- a/workflow/executor/executor_test.go +++ b/workflow/executor/executor_test.go @@ -498,7 +498,7 @@ func TestChmodDefault(t *testing.T) { info, err := os.Stat(artPath) require.NoError(t, err) - assert.Equal(t, "-rw-------", info.Mode().String()) + assert.Equal(t, "-rw-r--r--", info.Mode().String()) }) // A directory artifact, as `untar` or a directory download leaves it. The whole @@ -513,7 +513,7 @@ func TestChmodDefault(t *testing.T) { require.NoError(t, chmodDefault(root)) - want := map[string]string{root: "drwx------", nested: "drwx------", file: "-rw-------"} + want := map[string]string{root: "drwxr-xr-x", nested: "drwxr-xr-x", file: "-rw-r--r--"} for path, mode := range want { info, err := os.Stat(path) require.NoError(t, err)