fix(executor)!: give input artifacts without mode a default of 0644. Fixes #14792

docs/upgrading.md

@@ -29,6 +29,32 @@ 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 `0644`
+
+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.
+
+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 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:
+  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: `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`.
 
 ```yaml

workflow/executor/executor.go

@@ -344,19 +344,25 @@ 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)
 		if err != nil {
 			logger.WithError(err).Error(ctx, "Failed to chmod plugin artifact")
 			return err
 		}
+	default:
+		err = chmodDefault(artPath)
+		if err != nil {
+			return err
+		}
 	}
 	return nil
 }
@@ -1331,6 +1337,51 @@ 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`. 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 = 0o755
+)
+
+// 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-r--r--", 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: "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)
+			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{}