diff --git a/README.md b/README.md index 50e6515..dfbf86c 100644 --- a/README.md +++ b/README.md @@ -300,6 +300,9 @@ generates a new Age key. Decrypts data using an Age key stored as a secret. +With `--output`, both hold the result in memory and write it to `--output` only +once they have succeeded, so a failed command leaves `--output` untouched. + ## Storage Architecture ### Directory Structure diff --git a/internal/cli/crypto.go b/internal/cli/crypto.go index b2112ee..c6ef497 100644 --- a/internal/cli/crypto.go +++ b/internal/cli/crypto.go @@ -1,6 +1,7 @@ package cli import ( + "bytes" "errors" "fmt" "io" @@ -188,17 +189,15 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error { input = file } - // Set up output writer + // Set up output writer. With --output the result is held in memory + // and written only once encryption has succeeded, so a failure leaves + // --output untouched and --input can be the same file. output := cli.cmd.OutOrStdout() - if outputFile != "" { - file, err := cli.fs.Create(outputFile) - if err != nil { - return fmt.Errorf("failed to create output file: %w", err) - } - defer func() { _ = file.Close() }() + var result bytes.Buffer - output = file + if outputFile != "" { + output = &result } // Encrypt the data @@ -217,7 +216,11 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error { return fmt.Errorf("failed to finalize encryption: %w", err) } - return nil + if outputFile == "" { + return nil + } + + return cli.writeOutputFile(outputFile, &result) } // Decrypt decrypts data using an age secret key stored in a secret @@ -278,17 +281,15 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { input = file } - // Set up output writer + // Set up output writer. With --output the result is held in memory + // and written only once decryption has succeeded, so a failure leaves + // --output untouched and --input can be the same file. output := cli.cmd.OutOrStdout() - if outputFile != "" { - file, err := cli.fs.Create(outputFile) - if err != nil { - return fmt.Errorf("failed to create output file: %w", err) - } - defer func() { _ = file.Close() }() + var result bytes.Buffer - output = file + if outputFile != "" { + output = &result } // Decrypt the data @@ -302,6 +303,29 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { return fmt.Errorf("failed to decrypt data: %w", err) } + if outputFile == "" { + return nil + } + + return cli.writeOutputFile(outputFile, &result) +} + +// writeOutputFile creates outputFile, or empties it if it exists, and +// writes result into it. +func (cli *Instance) writeOutputFile( + outputFile string, result *bytes.Buffer, +) error { + file, err := cli.fs.Create(outputFile) + if err != nil { + return fmt.Errorf("failed to create output file: %w", err) + } + defer func() { _ = file.Close() }() + + _, err = result.WriteTo(file) + if err != nil { + return fmt.Errorf("failed to write output file: %w", err) + } + return nil } diff --git a/internal/cli/crypto_test.go b/internal/cli/crypto_test.go new file mode 100644 index 0000000..16f50d6 --- /dev/null +++ b/internal/cli/crypto_test.go @@ -0,0 +1,134 @@ +//nolint:testpackage // sets the unexported fields of Instance +package cli + +import ( + "fmt" + "io" + "os" + "path/filepath" + "testing" + + "github.com/spf13/afero" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" + "sneak.berlin/go/secret/internal/vault" +) + +// plainText is the content of plain.txt in the directory newCryptoTest makes. +const plainText = "document" + +// newCryptoTest returns an Instance whose current vault, in a new state +// directory on the real filesystem, holds the age key secret "key", and a +// new directory holding only plain.txt and plain.txt.age, its encryption +// with that key. +func newCryptoTest(t *testing.T) (*Instance, string) { + t.Helper() + + fs := afero.NewOsFs() + stateDir := t.TempDir() + + _, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil) + require.NoError(t, err) + + cli := NewCLIInstanceWithStateDir(fs, stateDir) + cli.Mnemonic = testMnemonicBuffer(t) + cli.cmd = &cobra.Command{} + + dir := t.TempDir() + plain := filepath.Join(dir, "plain.txt") + require.NoError(t, afero.WriteFile(fs, plain, []byte(plainText), 0o600)) + require.NoError(t, cli.Encrypt("key", plain, plain+".age")) + + return cli, dir +} + +// TestFailedCryptoLeavesOutputUnchanged is a regression test for +// https://git.eeqj.de/sneak/secret/issues/143, where secret encrypt and +// secret decrypt emptied the file named by --output before they failed. +// A failed command must leave an existing file as it was and create none, +// and give the same error as without --output. +func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + run func(cli *Instance, dir, output string) error + }{ + // Reading a directory fails once encryption has begun + {"encrypt", func(cli *Instance, dir, output string) error { + return cli.Encrypt("key", dir, output) + }}, + // plain.txt is not an age file + {"decrypt", func(cli *Instance, dir, output string) error { + return cli.Decrypt("key", filepath.Join(dir, "plain.txt"), output) + }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + existing := filepath.Join(dir, "existing.txt") + require.NoError(t, afero.WriteFile(cli.fs, existing, []byte("precious"), 0o600)) + + cli.cmd.SetOut(io.Discard) + toStdout := tt.run(cli, dir, "") + require.Error(t, toStdout) + + require.EqualError(t, tt.run(cli, dir, existing), toStdout.Error()) + require.EqualError(t, tt.run(cli, dir, filepath.Join(dir, "new.txt")), + toStdout.Error()) + + content, err := afero.ReadFile(cli.fs, existing) + require.NoError(t, err) + require.Equal(t, "precious", string(content)) + + exists, err := afero.Exists(cli.fs, filepath.Join(dir, "new.txt")) + require.NoError(t, err) + require.False(t, exists, "a failed %s created --output", tt.name) + }) + } +} + +// TestCryptoInPlace checks that secret encrypt and secret decrypt given +// one file as both --input and --output replace it with the right result. +// Before https://git.eeqj.de/sneak/secret/issues/143 they emptied the file +// before reading it. +func TestCryptoInPlace(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + file := filepath.Join(dir, "plain.txt") + + require.NoError(t, cli.Encrypt("key", file, file)) + require.NoError(t, cli.Decrypt("key", file, file)) + + content, err := afero.ReadFile(cli.fs, file) + require.NoError(t, err) + require.Equal(t, plainText, string(content)) +} + +// TestDecryptToDescriptorOfDeletedFile checks that secret decrypt given +// /dev/fd/N as --output writes through that descriptor. A program may give +// the command a temporary file it has already deleted as stdout, pass +// /dev/stdout as --output and read the result back from that file. +func TestDecryptToDescriptorOfDeletedFile(t *testing.T) { + t.Parallel() + + cli, dir := newCryptoTest(t) + + file, err := os.CreateTemp(t.TempDir(), "out") + require.NoError(t, err) + + defer func() { _ = file.Close() }() + + require.NoError(t, os.Remove(file.Name())) + + output := fmt.Sprintf("/dev/fd/%d", file.Fd()) + require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output)) + + content, err := io.ReadAll(file) + require.NoError(t, err) + require.Equal(t, plainText, string(content)) +}