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

Author: anneheartrecord

docs/upgrading.md

@@ -29,19 +29,20 @@ 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.
 
 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:

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`.

workflow/executor/executor.go

@@ -344,20 +344,21 @@ 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
 		}
-	} 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

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)