Repository: getsops/sops (language: python)

Issue:
`sops execfile` filename should not have a random suffix appended in --no-fifo mode
Hello,

Some programs (in this case, Chef `knife`) expect the file extension to reflect the data type of the file.

When performing `sops exec-file --filename tmp.json --no-fifo my-role.json 'knife role from file {}'`, `knife` breaks because the file name is not tmp.json, as requested, but something along the lines of `/private/var/folders/vy/_x8ql9xb6ehd634h9q00000gn/T/.sops2839518645/tmp.json3758396011`.

In my opinion, there is no real benefit for sops to essentially randomize the extension, as the whole path is essentially security by obscurity anyhow and adding one more unnecessary "layer" to it won't prevent shell globbing from catching the file name anyway.

When using a FIFO (default), the temporary file name is as expected, with no random suffix appended to the file name.

Thanks.


Discussion / hints:
Interestingly this behavior was kept when #761 got implemented, whose aim was `Some tools (e.g. Terraform) require the right file extension to recognize the file type.`.

My guess is that it it's OK to keep the random extension if `--filename` isn't specified, but it definitely shouldn't use a random extension in case `--filename` is specified.

Fix patch:
diff --git a/cmd/sops/main.go b/cmd/sops/main.go
index bef0182d1..d50db90eb 100644
--- a/cmd/sops/main.go
+++ b/cmd/sops/main.go
@@ -237,7 +237,7 @@ func main() {
 				},
 				cli.StringFlag{
 					Name:  "filename",
-					Usage: "filename for the temporarily file (default: tmp-file)",
+					Usage: fmt.Sprintf("filename for the temporarily file (default: %s)", exec.FallbackFilename),
 				},
 			}, keyserviceFlags...),
 			Action: func(c *cli.Context) error {
@@ -272,11 +272,6 @@ func main() {
 					return toExitError(err)
 				}
 
-				filename := c.String("filename")
-				if filename == "" {
-					filename = "tmp-file"
-				}
-
 				if c.Bool("background") {
 					log.Warn("exec-file's --background option is deprecated and will be removed in a future version of sops")
 				}
@@ -287,7 +282,7 @@ func main() {
 					Background: c.Bool("background"),
 					Fifo:       !c.Bool("no-fifo"),
 					User:       c.String("user"),
-					Filename:   filename,
+					Filename:   c.String("filename"),
 				}); err != nil {
 					return toExitError(err)
 				}
diff --git a/cmd/sops/subcommand/exec/exec.go b/cmd/sops/subcommand/exec/exec.go
index e7201107f..550790986 100644
--- a/cmd/sops/subcommand/exec/exec.go
+++ b/cmd/sops/subcommand/exec/exec.go
@@ -3,6 +3,7 @@ package exec
 import (
 	"bytes"
 	"os"
+	"path/filepath"
 	"runtime"
 	"strings"
 
@@ -11,6 +12,10 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
+const (
+	FallbackFilename = "tmp-file"
+)
+
 var log *logrus.Logger
 
 func init() {
@@ -28,10 +33,23 @@ type ExecOpts struct {
 }
 
 func GetFile(dir, filename string) *os.File {
-	handle, err := os.CreateTemp(dir, filename)
+	// If no filename is provided, create a random one based on FallbackFilename
+	if filename == "" {
+		handle, err := os.CreateTemp(dir, FallbackFilename)
+		if err != nil {
+			log.Fatal(err)
+		}
+		return handle
+	}
+	// If a filename is provided, use that one
+	handle, err := os.Create(filepath.Join(dir, filename))
 	if err != nil {
 		log.Fatal(err)
 	}
+	// read+write for owner only
+	if err = handle.Chmod(0600); err != nil {
+		log.Fatal(err)
+	}
 	return handle
 }
 
@@ -55,9 +73,15 @@ func ExecWithFile(opts ExecOpts) error {
 	if opts.Fifo {
 		// fifo handling needs to be async, even opening to write
 		// will block if there is no reader present
-		filename = GetPipe(dir, opts.Filename)
+		filename = opts.Filename
+		if filename == "" {
+			filename = FallbackFilename
+		}
+		filename = GetPipe(dir, filename)
 		go WritePipe(filename, opts.Plaintext)
 	} else {
+		// GetFile handles opts.Filename == "" specially, that's why we have
+		// to pass in opts.Filename without handling the fallback here
 		handle := GetFile(dir, opts.Filename)
 		handle.Write(opts.Plaintext)
 		handle.Close()


Test patch:
diff --git a/functional-tests/src/lib.rs b/functional-tests/src/lib.rs
index 46703a459..c2cf72666 100644
--- a/functional-tests/src/lib.rs
+++ b/functional-tests/src/lib.rs
@@ -949,4 +949,47 @@ bar: |-
 }"#
         );
     }
+
+    #[test]
+    fn exec_file_filename() {
+        let file_path = prepare_temp_file(
+            "test_exec_file_filename.yaml",
+            r#"foo: bar
+bar: |-
+  baz
+  bam
+"#
+            .as_bytes(),
+        );
+        assert!(
+            Command::new(SOPS_BINARY_PATH)
+                .arg("-e")
+                .arg("-i")
+                .arg(file_path.clone())
+                .output()
+                .expect("Error running sops")
+                .status
+                .success(),
+            "sops didn't exit successfully"
+        );
+        let output = Command::new(SOPS_BINARY_PATH)
+            .arg("exec-file")
+            .arg("--no-fifo")
+            .arg("--filename")
+            .arg("foobar")
+            .arg(file_path.clone())
+            .arg("echo {}")
+            .output()
+            .expect("Error running sops");
+        assert!(output.status.success(), "sops didn't exit successfully");
+        println!(
+            "stdout: {}, stderr: {}",
+            String::from_utf8_lossy(&output.stdout),
+            String::from_utf8_lossy(&output.stderr)
+        );
+        assert!(
+            String::from_utf8_lossy(&output.stdout).ends_with("foobar\n"),
+            "filename did not end with 'foobar'"
+        );
+    }
 }