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 <chengxisheng777@gmail.com>

Author: anneheartrecord

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

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

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 {

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{}