1 Commits
Author SHA1 Message Date
sneak bde833193c Write encrypt and decrypt output only once they succeed (closes #143)
check / check (push) Canceled after 0s
`secret encrypt` and `secret decrypt` emptied the file named by
`--output` before reading any input, so a failure left it empty, and a
file given as both `--input` and `--output` was emptied before it was
read. They now write into a temporary file of mode 0600 next to it, or
next to the file it is a symbolic link to, and rename that over it only
on success, with the new `secret.WriteFileAtomicFunc`. An output file
that cannot be written still fails. Ctrl-C, SIGTERM or a hangup removes
the temporary file. A device or pipe, such as `/dev/null`, is written to
directly.

Judgement calls: the device and pipe case; a command started with
`nohup` still ignores a hangup.

Model: opus-5-5
2026-10-14 22:32:52 +00:00
13 changed files with 532 additions and 234 deletions
+15 -13
View File
@@ -90,8 +90,7 @@ remove things pass `--force`.
#### `secret init` #### `secret init`
Initializes the secret manager with a default vault. Prompts for a BIP39 Initializes the secret manager with a default vault. Prompts for a BIP39
mnemonic phrase and creates the initial directory structure. When the vault mnemonic phrase and creates the initial directory structure.
`default` already exists, it fails before asking for the mnemonic or passphrase.
**Environment Variables:** **Environment Variables:**
@@ -106,8 +105,7 @@ Lists all available vaults. The current vault is marked.
#### `secret vault create <name>` #### `secret vault create <name>`
Creates a new vault with the specified name. When a vault of that name already Creates a new vault with the specified name.
exists, it fails before asking for the mnemonic or passphrase.
**Vault Name Format:** only lowercase ASCII letters, digits, `.`, `-` and `_` **Vault Name Format:** only lowercase ASCII letters, digits, `.`, `-` and `_`
are allowed, and a name must not be empty, `.` or `..`. are allowed, and a name must not be empty, `.` or `..`.
@@ -144,13 +142,7 @@ contain `//`, or have `..` as a path segment.
#### `secret get <secret-name> [--version <version>]` #### `secret get <secret-name> [--version <version>]`
Retrieves and outputs a secret value to stdout. A failed write of the value, Retrieves and outputs a secret value to stdout.
such as to a file on a full disk, prints the error and exits 1, with two
exceptions. If the program reading from a pipe has already exited, as in
`secret get s | true`, the broken-pipe signal (SIGPIPE) ends `secret` with no
message and the shell reports exit status 141. If the shell closed stdout before
`secret` started (`secret get s >&-`), the value is written to `/dev/null` and
`secret get` exits 0.
- `--version, -v`: Get a specific version (default: current) - `--version, -v`: Get a specific version (default: current)
@@ -300,8 +292,18 @@ generates a new Age key.
Decrypts data using an Age key stored as a secret. Decrypts data using an Age key stored as a secret.
With `--output`, both hold the result in memory and write it to `--output` only Both write the result to a new file next to `--output` and rename it over
once they have succeeded, so a failed command leaves `--output` untouched. `--output` only once they have succeeded, so a failed command leaves an existing
`--output` as it was and creates none, and `--input` and `--output` can be the
same file. When `--output` is a symbolic link, the file it points to is
replaced, or created if it does not exist yet, and the link kept; the new file
is then made next to that file. The directory the new file is made in must be
writable. The new file has mode 0600 and belongs to the user running the
command, whatever mode and owner the file it replaces had, so a hard link to the
old file keeps the old content. Ctrl-C, SIGTERM or a hangup removes the new
file; a command killed any other way, such as with SIGKILL, leaves it there,
named `.tmp-` and a number. A device or a pipe, such as `/dev/null`, is written
to directly.
## Storage Architecture ## Storage Architecture
+3 -42
View File
@@ -222,51 +222,12 @@ func TestMnemonicNotReadNamesOnlyMnemonic(t *testing.T) {
output, err := cmd.CombinedOutput() output, err := cmd.CombinedOutput()
require.Error(t, err) require.Error(t, err)
require.Equal(t, "Error: failed to read mnemonic: there is no terminal to "+ require.Equal(t, "Initialized secrets manager at: "+stateDir+"\n"+
"ask on. Please set the SB_SECRET_MNEMONIC environment variable or run "+ "Error: failed to read mnemonic: there is no terminal to ask on. "+
"Please set the SB_SECRET_MNEMONIC environment variable or run "+
"interactively\n", string(output)) "interactively\n", string(output))
} }
// TestCreateExistingVaultAsksForNothing is a regression test for
// https://git.eeqj.de/sneak/secret/issues/151: `secret init` and `secret
// vault create` asked for the mnemonic and the unlocker passphrase before
// refusing a vault that already exists, and `secret init` printed
// "Initialized secrets manager at" first. Each runs here as the built binary,
// in a new session, which has no terminal, and with neither
// SB_SECRET_MNEMONIC nor SB_UNLOCK_PASSPHRASE set, so asking for either
// fails. Each must fail saying only that the vault exists.
func TestCreateExistingVaultAsksForNothing(t *testing.T) {
t.Parallel()
stateDir := t.TempDir()
_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default", nil, nil)
require.NoError(t, err)
// run runs the built binary with args, which must fail, and returns what
// it printed
run := func(args ...string) string {
//nolint:gosec // G204: test executes the freshly built secret binary
cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
cmd.Env = []string{
secret.EnvStateDir + "=" + stateDir,
"PATH=" + os.Getenv("PATH"),
"HOME=" + os.Getenv("HOME"),
}
cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
output, err := cmd.CombinedOutput()
require.Error(t, err, "secret %v", args)
return string(output)
}
require.Equal(t,
"Error: failed to create default vault: vault default already exists\n",
run("init"))
require.Equal(t, "Error: vault default already exists\n",
run("vault", "create", "default"))
}
// TestStopDuringCreateLeavesWholeVaultOrNone is a regression test for // TestStopDuringCreateLeavesWholeVaultOrNone is a regression test for
// https://git.eeqj.de/sneak/secret/issues/105: `secret init` or `secret vault // https://git.eeqj.de/sneak/secret/issues/105: `secret init` or `secret vault
// create` killed after the passphrase prompt but before the unlocker was // create` killed after the passphrase prompt but before the unlocker was
+122 -63
View File
@@ -1,14 +1,16 @@
package cli package cli
import ( import (
"bytes"
"errors" "errors"
"fmt" "fmt"
"io" "io"
"os" "os"
"path/filepath"
"sync/atomic"
"filippo.io/age" "filippo.io/age"
"github.com/awnumar/memguard" "github.com/awnumar/memguard"
"github.com/spf13/afero"
"github.com/spf13/cobra" "github.com/spf13/cobra"
"sneak.berlin/go/secret/internal/secret" "sneak.berlin/go/secret/internal/secret"
"sneak.berlin/go/secret/internal/vault" "sneak.berlin/go/secret/internal/vault"
@@ -189,38 +191,24 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
input = file input = file
} }
// Set up output writer. With --output the result is held in memory return cli.writeOutput(outputFile, func(output io.Writer) error {
// and written only once encryption has succeeded, so a failure leaves encryptor, err := age.Encrypt(output, recipient)
// --output untouched and --input can be the same file. if err != nil {
output := cli.cmd.OutOrStdout() return fmt.Errorf("failed to create age encryptor: %w", err)
}
var result bytes.Buffer _, err = io.Copy(encryptor, input)
if err != nil {
return fmt.Errorf("failed to encrypt data: %w", err)
}
if outputFile != "" { err = encryptor.Close()
output = &result if err != nil {
} return fmt.Errorf("failed to finalize encryption: %w", err)
}
// Encrypt the data
encryptor, err := age.Encrypt(output, recipient)
if err != nil {
return fmt.Errorf("failed to create age encryptor: %w", err)
}
_, err = io.Copy(encryptor, input)
if err != nil {
return fmt.Errorf("failed to encrypt data: %w", err)
}
err = encryptor.Close()
if err != nil {
return fmt.Errorf("failed to finalize encryption: %w", err)
}
if outputFile == "" {
return nil return nil
} })
return cli.writeOutputFile(outputFile, &result)
} }
// Decrypt decrypts data using an age secret key stored in a secret // Decrypt decrypts data using an age secret key stored in a secret
@@ -281,52 +269,123 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
input = file input = file
} }
// Set up output writer. With --output the result is held in memory return cli.writeOutput(outputFile, func(output io.Writer) error {
// and written only once decryption has succeeded, so a failure leaves decryptor, err := age.Decrypt(input, identity)
// --output untouched and --input can be the same file. if err != nil {
output := cli.cmd.OutOrStdout() return fmt.Errorf("failed to create age decryptor: %w", err)
}
var result bytes.Buffer _, err = io.Copy(output, decryptor)
if err != nil {
return fmt.Errorf("failed to decrypt data: %w", err)
}
if outputFile != "" {
output = &result
}
// Decrypt the data
decryptor, err := age.Decrypt(input, identity)
if err != nil {
return fmt.Errorf("failed to create age decryptor: %w", err)
}
_, err = io.Copy(output, decryptor)
if err != nil {
return fmt.Errorf("failed to decrypt data: %w", err)
}
if outputFile == "" {
return nil return nil
} })
return cli.writeOutputFile(outputFile, &result)
} }
// writeOutputFile creates outputFile, or empties it if it exists, and // temporaryOutput is the name of the temporary file writeOutput is filling,
// writes result into it. // until it is renamed into place. The signal handler in Entry removes it,
func (cli *Instance) writeOutputFile( // since for decrypt it holds part of the secret.
outputFile string, result *bytes.Buffer, //
//nolint:gochecknoglobals // read by the signal handler, which has no Instance
var temporaryOutput atomic.Pointer[string]
// writeOutput calls write with stdout when outputFile is empty, and with
// outputFile itself when it is a device or a pipe, such as /dev/null.
// Otherwise write fills a new file of mode 0600 that replaces outputFile,
// or the file it is a symbolic link to, only once write has succeeded, so a
// failure leaves that file as it was, and it may be the input file too.
// An existing outputFile that cannot be written fails before write is
// called.
func (cli *Instance) writeOutput(
outputFile string, write func(output io.Writer) error,
) error { ) error {
file, err := cli.fs.Create(outputFile) if outputFile == "" {
return write(cli.cmd.OutOrStdout())
}
info, err := cli.fs.Stat(outputFile)
if err == nil && !info.Mode().IsRegular() {
file, err := cli.fs.OpenFile(outputFile, os.O_WRONLY, 0)
if err != nil {
return fmt.Errorf("failed to create output file: %w", err)
}
defer func() { _ = file.Close() }()
return write(file)
}
// An existing file that cannot be written fails here, as writing into it
// directly would. Opened without O_TRUNC, which would empty it.
file, err := cli.fs.OpenFile(outputFile, os.O_WRONLY, 0)
if err == nil {
err = file.Close()
}
if err != nil && !errors.Is(err, os.ErrNotExist) {
return fmt.Errorf("failed to create output file: %w", err)
}
path, err := outputPath(outputFile)
if err != nil { if err != nil {
return fmt.Errorf("failed to create output file: %w", err) return fmt.Errorf("failed to create output file: %w", err)
} }
defer func() { _ = file.Close() }()
_, err = result.WriteTo(file) err = secret.WriteFileAtomicFunc(cli.fs, path, func(file afero.File) error {
if err != nil { name := file.Name()
return fmt.Errorf("failed to write output file: %w", err) temporaryOutput.Store(&name)
return write(file)
})
temporaryOutput.Store(nil)
return err
}
// outputPath returns the path of the file that writing to outputFile
// replaces, with no symbolic link in it, so that filepath.Dir of it is the
// directory that file is in. When outputFile is a symbolic link, as
// /dev/stdout is when stdout goes to a file, that is the path the link
// points to, whether or not a file is there yet, so the link is kept, as
// writing through it would keep it. A link that cannot be followed, such as
// one that points to itself, is an error.
func outputPath(outputFile string) (string, error) {
path, err := filepath.EvalSymlinks(outputFile)
if !errors.Is(err, os.ErrNotExist) {
return path, err
} }
return nil // No file is at outputFile, or at the end of its links, yet
target, err := os.Readlink(outputFile)
if errors.Is(err, os.ErrNotExist) {
// Nothing is at outputFile itself. Its directory part is resolved
// with filepath.EvalSymlinks, since filepath.Dir would remove a link
// to a directory followed by ".." as text and give another directory.
dir, name := filepath.Split(outputFile)
dir, err = filepath.EvalSymlinks(dir)
if err != nil {
return "", err
}
return filepath.Join(dir, name), nil
}
if err != nil {
return "", err
}
if !filepath.IsAbs(target) {
// Not filepath.Join, which would remove a ".." in target together
// with the name before it, even when that name is a link to a
// directory
dir, _ := filepath.Split(outputFile)
target = dir + target
}
return outputPath(target)
} }
// isValidAgeSecretKey checks if a string is a valid age secret key by // isValidAgeSecretKey checks if a string is a valid age secret key by
+198 -2
View File
@@ -3,7 +3,10 @@ package cli
import ( import (
"io" "io"
"os"
"path/filepath" "path/filepath"
"strings"
"syscall"
"testing" "testing"
"github.com/spf13/afero" "github.com/spf13/afero"
@@ -82,13 +85,58 @@ func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) {
require.NoError(t, err) require.NoError(t, err)
require.Equal(t, "precious", string(content)) require.Equal(t, "precious", string(content))
exists, err := afero.Exists(cli.fs, filepath.Join(dir, "new.txt")) // No new.txt, and no temporary file left behind
entries, err := afero.ReadDir(cli.fs, dir)
require.NoError(t, err) require.NoError(t, err)
require.False(t, exists, "a failed %s created --output", tt.name)
names := make([]string, 0, len(entries))
for _, entry := range entries {
names = append(names, entry.Name())
}
require.Equal(t, []string{"existing.txt", "plain.txt", "plain.txt.age"}, names)
}) })
} }
} }
// writeFailFs fails every open of one path for writing, as a file of mode
// 0444 does for anyone but root, who runs these tests in Docker.
type writeFailFs struct {
afero.Fs
path string
}
//nolint:ireturn // afero.File is the interface required by afero.Fs
func (f *writeFailFs) OpenFile(
name string, flag int, perm os.FileMode,
) (afero.File, error) {
if name == f.path && flag&(os.O_WRONLY|os.O_RDWR) != 0 {
return nil, &os.PathError{Op: "open", Path: name, Err: syscall.EACCES}
}
return f.Fs.OpenFile(name, flag, perm)
}
// TestDecryptToUnwritableOutput checks that secret decrypt fails when
// --output is a file that cannot be written, as writing into it directly
// does, and leaves it as it was, although its directory can be written.
func TestDecryptToUnwritableOutput(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.fs = &writeFailFs{Fs: cli.fs, path: existing}
err := cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), existing)
require.ErrorIs(t, err, os.ErrPermission)
content, err := afero.ReadFile(cli.fs, existing)
require.NoError(t, err)
require.Equal(t, "precious", string(content))
}
// TestCryptoInPlace checks that secret encrypt and secret decrypt given // TestCryptoInPlace checks that secret encrypt and secret decrypt given
// one file as both --input and --output replace it with the right result. // 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 https://git.eeqj.de/sneak/secret/issues/143 they emptied the file
@@ -106,3 +154,151 @@ func TestCryptoInPlace(t *testing.T) {
require.NoError(t, err) require.NoError(t, err)
require.Equal(t, plainText, string(content)) require.Equal(t, plainText, string(content))
} }
// TestDecryptToPipe checks that secret decrypt writes into a named pipe
// given as --output, as it must into a device such as /dev/null, instead
// of replacing it with a file.
func TestDecryptToPipe(t *testing.T) {
t.Parallel()
cli, dir := newCryptoTest(t)
pipe := filepath.Join(dir, "pipe")
require.NoError(t, syscall.Mkfifo(pipe, 0o600))
received := make(chan []byte, 1)
go func() {
content, _ := afero.ReadFile(cli.fs, pipe)
received <- content
}()
require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), pipe))
info, err := os.Lstat(pipe)
require.NoError(t, err)
require.Equal(t, os.ModeNamedPipe, info.Mode().Type())
require.Equal(t, plainText, string(<-received))
}
// TestDecryptThroughLink checks that secret decrypt given a symbolic link
// as --output replaces the file the link points to, in another directory,
// and keeps the link, as writing through the link would.
func TestDecryptThroughLink(t *testing.T) {
t.Parallel()
cli, dir := newCryptoTest(t)
targetDir := t.TempDir()
target := filepath.Join(targetDir, "target.txt")
require.NoError(t, afero.WriteFile(cli.fs, target, []byte("old"), 0o600))
link := filepath.Join(dir, "link")
require.NoError(t, os.Symlink(target, link))
require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link))
info, err := os.Lstat(link)
require.NoError(t, err)
require.Equal(t, os.ModeSymlink, info.Mode().Type())
content, err := afero.ReadFile(cli.fs, target)
require.NoError(t, err)
require.Equal(t, plainText, string(content))
// No temporary file left beside the target
entries, err := afero.ReadDir(cli.fs, targetDir)
require.NoError(t, err)
require.Len(t, entries, 1)
}
// TestDecryptThroughLinkToMissingFile checks that secret decrypt given a
// symbolic link to a file that does not exist yet as --output creates that
// file and keeps the link, as writing through the link would. Here --output
// is a relative link to a second link, which points into another directory.
func TestDecryptThroughLinkToMissingFile(t *testing.T) {
t.Parallel()
cli, dir := newCryptoTest(t)
targetDir := t.TempDir()
target := filepath.Join(targetDir, "target.txt")
link := filepath.Join(dir, "link")
require.NoError(t, os.Symlink("second-link", link))
require.NoError(t, os.Symlink(target, filepath.Join(dir, "second-link")))
require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link))
for _, name := range []string{"link", "second-link"} {
info, err := os.Lstat(filepath.Join(dir, name))
require.NoError(t, err)
require.Equal(t, os.ModeSymlink, info.Mode().Type(), name)
}
content, err := afero.ReadFile(cli.fs, target)
require.NoError(t, err)
require.Equal(t, plainText, string(content))
// No temporary file left beside the target
entries, err := afero.ReadDir(cli.fs, targetDir)
require.NoError(t, err)
require.Len(t, entries, 1)
}
// TestDecryptThroughLinkAndParent checks that secret decrypt makes the file
// --output names in the directory its path leads to when the path holds a
// symbolic link to a directory followed by "..": dir/link/../sub/new.txt is
// targetDir/sub/new.txt, and dir/sub, where removing "link/.." as text
// leads, does not exist.
func TestDecryptThroughLinkAndParent(t *testing.T) {
t.Parallel()
cli, dir := newCryptoTest(t)
targetDir := t.TempDir()
require.NoError(t, os.Mkdir(filepath.Join(targetDir, "linked"), 0o700))
require.NoError(t, os.Mkdir(filepath.Join(targetDir, "sub"), 0o700))
require.NoError(t, os.Symlink(filepath.Join(targetDir, "linked"),
filepath.Join(dir, "link")))
// Not filepath.Join, which would remove "link/.." too
output := dir + "/link/../sub/new.txt"
require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output))
content, err := afero.ReadFile(cli.fs, filepath.Join(targetDir, "sub", "new.txt"))
require.NoError(t, err)
require.Equal(t, plainText, string(content))
}
// TestDecryptThroughLoopingLink checks that secret decrypt given a symbolic
// link that points to itself as --output fails, as writing through it
// would, and leaves the link as it was.
func TestDecryptThroughLoopingLink(t *testing.T) {
t.Parallel()
cli, dir := newCryptoTest(t)
link := filepath.Join(dir, "link")
require.NoError(t, os.Symlink("link", link))
require.Error(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), link))
target, err := os.Readlink(link)
require.NoError(t, err)
require.Equal(t, "link", target)
}
// TestDecryptToLongestName checks that secret decrypt writes an --output
// whose name is as long as a file name can be, 255 bytes on Linux and
// macOS: the temporary file's name must not be longer.
func TestDecryptToLongestName(t *testing.T) {
t.Parallel()
const longestName = 255
cli, dir := newCryptoTest(t)
output := filepath.Join(dir, strings.Repeat("a", longestName))
require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output))
content, err := afero.ReadFile(cli.fs, output)
require.NoError(t, err)
require.Equal(t, plainText, string(content))
}
+114
View File
@@ -7,7 +7,9 @@ import (
"os/exec" "os/exec"
"path/filepath" "path/filepath"
"strings" "strings"
"syscall"
"testing" "testing"
"time"
"github.com/awnumar/memguard" "github.com/awnumar/memguard"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
@@ -105,3 +107,115 @@ func TestInterruptExitsThroughMemguard(t *testing.T) {
require.ErrorAs(t, err, &exitErr) require.ErrorAs(t, err, &exitErr)
assert.Equal(t, 1, exitErr.ExitCode(), "add ended with %v", err) assert.Equal(t, 1, exitErr.ExitCode(), "add ended with %v", err)
} }
// Ctrl-C or a hangup while encrypt or decrypt waits for input must remove
// the temporary file it is writing next to --output, which for decrypt
// holds part of the secret.
func TestInterruptRemovesTemporaryOutput(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(t.Context(), commandWait)
defer cancel()
secretPath := secretBinaryPath(t)
env := []string{
secret.EnvStateDir + "=" + t.TempDir(),
secret.EnvMnemonic + "=" + testMnemonic,
secret.EnvUnlockPassphrase + "=test-passphrase",
"PATH=" + os.Getenv("PATH"),
}
//nolint:gosec // G204: test executes the freshly built secret binary
initCmd := exec.CommandContext(ctx, secretPath, "init")
initCmd.Env = env
output, err := initCmd.CombinedOutput()
require.NoError(t, err, "init should succeed: %s", output)
// encrypt stores the new key before it starts writing, so decrypt has it
for _, command := range []string{"encrypt", "decrypt"} {
for _, sig := range []os.Signal{os.Interrupt, syscall.SIGHUP} {
outputDir := t.TempDir()
//nolint:gosec // G204: test executes the freshly built secret binary
cmd := exec.CommandContext(ctx, secretPath, command, "key",
"--output", filepath.Join(outputDir, "out"))
cmd.Env = env
// Held open and never written, so the command keeps waiting for
// input. Wait closes it.
_, err := cmd.StdinPipe()
require.NoError(t, err)
require.NoError(t, cmd.Start())
require.Eventually(t, func() bool {
entries, err := os.ReadDir(outputDir)
return err == nil && len(entries) > 0
}, commandWait, 10*time.Millisecond, "%s made no temporary file", command)
require.NoError(t, cmd.Process.Signal(sig))
var exitErr *exec.ExitError
require.ErrorAs(t, cmd.Wait(), &exitErr)
entries, err := os.ReadDir(outputDir)
require.NoError(t, err)
assert.Empty(t, entries, "%s left its temporary file on %v", command, sig)
}
}
}
// encrypt started with nohup must go on ignoring a hangup, which it catches
// otherwise, and finish writing --output.
func TestNohupIgnoresHangup(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithTimeout(t.Context(), commandWait)
defer cancel()
secretPath := secretBinaryPath(t)
env := []string{
secret.EnvStateDir + "=" + t.TempDir(),
secret.EnvMnemonic + "=" + testMnemonic,
secret.EnvUnlockPassphrase + "=test-passphrase",
"PATH=" + os.Getenv("PATH"),
}
//nolint:gosec // G204: test executes the freshly built secret binary
initCmd := exec.CommandContext(ctx, secretPath, "init")
initCmd.Env = env
output, err := initCmd.CombinedOutput()
require.NoError(t, err, "init should succeed: %s", output)
outputDir := t.TempDir()
outputFile := filepath.Join(outputDir, "out")
//nolint:gosec // G204: test executes the freshly built secret binary
cmd := exec.CommandContext(ctx, "nohup", secretPath, "encrypt", "key",
"--output", outputFile)
cmd.Env = env
stdin, err := cmd.StdinPipe()
require.NoError(t, err)
require.NoError(t, cmd.Start())
require.Eventually(t, func() bool {
entries, err := os.ReadDir(outputDir)
return err == nil && len(entries) > 0
}, commandWait, 10*time.Millisecond, "encrypt made no temporary file")
require.NoError(t, cmd.Process.Signal(syscall.SIGHUP))
// Time for a hangup that is caught to end encrypt before it has its input
time.Sleep(500 * time.Millisecond)
_, err = stdin.Write([]byte("document"))
require.NoError(t, err)
require.NoError(t, stdin.Close())
require.NoError(t, cmd.Wait())
_, err = os.Stat(outputFile)
require.NoError(t, err)
}
+3 -9
View File
@@ -17,9 +17,6 @@ import (
// errPassphraseMismatch is returned when passphrase confirmation fails // errPassphraseMismatch is returned when passphrase confirmation fails
var errPassphraseMismatch = errors.New("passphrases do not match") var errPassphraseMismatch = errors.New("passphrases do not match")
// defaultVaultName is the name of the vault `secret init` creates
const defaultVaultName = "default"
// NewInitCmd creates the init command // NewInitCmd creates the init command
func NewInitCmd() *cobra.Command { func NewInitCmd() *cobra.Command {
return &cobra.Command{ return &cobra.Command{
@@ -95,10 +92,8 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
return fmt.Errorf("failed to create state directory: %w", err) return fmt.Errorf("failed to create state directory: %w", err)
} }
// Refuse an existing default vault before asking for anything if cmd != nil {
err = vault.CheckNewVaultName(cli.fs, cli.stateDir, defaultVaultName) cmd.Printf("Initialized secrets manager at: %s\n", stateDir)
if err != nil {
return fmt.Errorf("failed to create default vault: %w", err)
} }
// Prompt for mnemonic // Prompt for mnemonic
@@ -138,7 +133,7 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
// Create the default vault with its passphrase unlocker // Create the default vault with its passphrase unlocker
secret.Debug("Creating default vault") secret.Debug("Creating default vault")
vlt, err := vault.CreateVault(cli.fs, cli.stateDir, defaultVaultName, vlt, err := vault.CreateVault(cli.fs, cli.stateDir, "default",
mnemonic, passphraseBuffer) mnemonic, passphraseBuffer)
if err != nil { if err != nil {
secret.Debug("Failed to create default vault", "error", err) secret.Debug("Failed to create default vault", "error", err)
@@ -157,7 +152,6 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
} }
if cmd != nil { if cmd != nil {
cmd.Printf("Initialized secrets manager at: %s\n", stateDir)
cmd.Printf("\nDefault vault created and configured\n") cmd.Printf("\nDefault vault created and configured\n")
cmd.Printf("Long-term public key: %s\n", ltIdentity.Recipient().String()) cmd.Printf("Long-term public key: %s\n", ltIdentity.Recipient().String())
cmd.Printf("Unlocker ID: %s\n", unlocker.GetID()) cmd.Printf("Unlocker ID: %s\n", unlocker.GetID())
+4 -4
View File
@@ -52,10 +52,10 @@ func TestLeftoversRemovedByNextChangingCommand(t *testing.T) {
} }
for _, file := range []string{ for _, file := range []string{
testStateDir + "/.currentvault.tmp-4", testStateDir + "/.tmp-4",
vaultDir + "/.current-unlocker.tmp-5", vaultDir + "/.tmp-5",
secretDir + "/.current.tmp-6", secretDir + "/.tmp-6",
secretDir + "/versions/" + versions[0] + "/.metadata.age.tmp-7", secretDir + "/versions/" + versions[0] + "/.tmp-7",
} { } {
require.NoError(t, afero.WriteFile(fs, file, require.NoError(t, afero.WriteFile(fs, file,
[]byte("partial"), secret.FilePerms)) []byte("partial"), secret.FilePerms))
+22 -7
View File
@@ -2,6 +2,7 @@ package cli
import ( import (
"os" "os"
"os/signal"
"github.com/awnumar/memguard" "github.com/awnumar/memguard"
"github.com/spf13/cobra" "github.com/spf13/cobra"
@@ -14,12 +15,13 @@ import (
// every memguard buffer before it returns, so the caller must do nothing // every memguard buffer before it returns, so the caller must do nothing
// but exit with the code. // but exit with the code.
func Entry() int { func Entry() int {
// On SIGINT or SIGTERM memguard runs this function, wipes every buffer // On SIGINT, SIGTERM or SIGHUP memguard runs this function, wipes every
// and exits with status 1. The passphrase prompt turns echo off on the // buffer and exits with status 1. The function removes the temporary
// controlling terminal until the read finishes, so a signal there would // file that encrypt or decrypt is writing. The passphrase prompt turns
// leave echo off. Only a process in the terminal's foreground process // echo off on the controlling terminal until the read finishes, so a
// group may reset it: one in the background that tries is stopped // signal there would leave echo off. Only a process in the terminal's
// instead of exiting. // foreground process group may reset it: one in the background that
// tries is stopped instead of exiting.
terminal, terminalErr := os.Open(secret.TerminalPath) terminal, terminalErr := os.Open(secret.TerminalPath)
var terminalState *term.State var terminalState *term.State
@@ -30,7 +32,20 @@ func Entry() int {
terminalState, terminalErr = term.GetState(int(terminal.Fd())) terminalState, terminalErr = term.GetState(int(terminal.Fd()))
} }
signals := []os.Signal{os.Interrupt, unix.SIGTERM}
// A command started with nohup must go on ignoring SIGHUP, which
// catching it would undo
if !signal.Ignored(unix.SIGHUP) {
signals = append(signals, unix.SIGHUP)
}
memguard.CatchSignal(func(os.Signal) { memguard.CatchSignal(func(os.Signal) {
name := temporaryOutput.Load()
if name != nil {
_ = os.Remove(*name)
}
if terminalErr != nil { if terminalErr != nil {
return return
} }
@@ -39,7 +54,7 @@ func Entry() int {
if err == nil && foreground == unix.Getpgrp() { if err == nil && foreground == unix.Getpgrp() {
_ = term.Restore(int(terminal.Fd()), terminalState) _ = term.Restore(int(terminal.Fd()), terminalState)
} }
}, os.Interrupt, unix.SIGTERM) }, signals...)
defer memguard.Purge() defer memguard.Purge()
+2 -8
View File
@@ -443,10 +443,7 @@ func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error {
defer value.Destroy() defer value.Destroy()
// Write the value straight from locked memory, with no trailing newline // Write the value straight from locked memory, with no trailing newline
_, err = cmd.OutOrStdout().Write(value.Bytes()) _, _ = cmd.OutOrStdout().Write(value.Bytes())
if err != nil {
return fmt.Errorf("failed to write secret value: %w", err)
}
return nil return nil
} }
@@ -481,10 +478,7 @@ func (cli *Instance) GetSecretWithVersion(
secret.Debug("Got secret value", "valueLength", value.Size()) secret.Debug("Got secret value", "valueLength", value.Size())
// Write the value straight from locked memory, with no trailing newline // Write the value straight from locked memory, with no trailing newline
_, err = cmd.OutOrStdout().Write(value.Bytes()) _, _ = cmd.OutOrStdout().Write(value.Bytes())
if err != nil {
return fmt.Errorf("failed to write secret value: %w", err)
}
secret.Debug("Printed value to stdout") secret.Debug("Printed value to stdout")
-6
View File
@@ -267,12 +267,6 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
} }
defer release() defer release()
// Refuse an invalid name or an existing vault before asking for anything
err = vault.CheckNewVaultName(cli.fs, cli.stateDir, name)
if err != nil {
return err
}
mnemonic, cleanupMnemonic, err := cli.promptMnemonic() mnemonic, cleanupMnemonic, err := cli.promptMnemonic()
if err != nil { if err != nil {
return err return err
-41
View File
@@ -8,7 +8,6 @@
// - TestPromoteNonExistentVersion: Tests error handling for invalid promotion // - TestPromoteNonExistentVersion: Tests error handling for invalid promotion
// - TestGetSecretWithVersion: Tests `secret get --version` flag functionality // - TestGetSecretWithVersion: Tests `secret get --version` flag functionality
// - TestGetSecretWritesBinaryValue: Tests `secret get` output of binary values // - TestGetSecretWritesBinaryValue: Tests `secret get` output of binary values
// - TestGetSecretReturnsWriteError: Tests `secret get` when stdout fails
// - TestVersionCommandStructure: Tests command structure and help text // - TestVersionCommandStructure: Tests command structure and help text
// - TestListVersionsEmptyOutput: Tests edge case with no versions // - TestListVersionsEmptyOutput: Tests edge case with no versions
// //
@@ -21,7 +20,6 @@ package cli
import ( import (
"bytes" "bytes"
"os"
"path/filepath" "path/filepath"
"strings" "strings"
"testing" "testing"
@@ -362,45 +360,6 @@ func TestGetSecretWritesBinaryValue(t *testing.T) {
assert.Equal(t, value, buf.Bytes()) assert.Equal(t, value, buf.Bytes())
} }
// TestGetSecretReturnsWriteError checks that `secret get`, with and without
// a version, returns the error of a failed write of the value.
func TestGetSecretReturnsWriteError(t *testing.T) {
t.Parallel()
fs := afero.NewMemMapFs()
cli := NewCLIInstanceWithStateDir(fs, testStateDir)
cli.Mnemonic = testMnemonicBuffer(t)
setupTestVault(t, fs)
vlt, err := vault.GetCurrentVault(fs, testStateDir)
require.NoError(t, err)
addTestSecret(t, vlt, []byte("value"), false)
vaultDir, err := vlt.GetDirectory()
require.NoError(t, err)
versions, err := secret.ListVersions(fs,
filepath.Join(vaultDir, "secrets.d", "test%secret"))
require.NoError(t, err)
require.Len(t, versions, 1)
// Stdout is a file closed before the value is written to it
out, err := os.Create(filepath.Join(t.TempDir(), "stdout"))
require.NoError(t, err)
require.NoError(t, out.Close())
cmd := newRootCmd()
cmd.SetOut(out)
err = cli.GetSecret(cmd, "test/secret")
require.ErrorIs(t, err, os.ErrClosed)
err = cli.GetSecretWithVersion(cmd, "test/secret", versions[0])
require.ErrorIs(t, err, os.ErrClosed)
}
//nolint:paralleltest // reads process environment to determine the state dir //nolint:paralleltest // reads process environment to determine the state dir
func TestVersionCommandStructure(t *testing.T) { func TestVersionCommandStructure(t *testing.T) {
// Test that version commands are properly structured // Test that version commands are properly structured
+28 -7
View File
@@ -10,8 +10,8 @@ import (
"github.com/spf13/afero" "github.com/spf13/afero"
) )
// tempNamePart is in the name of every temporary file WriteFileAtomic makes, // tempNamePart starts the name of every temporary file WriteFileAtomicFunc
// ".NAME.tmp-123", and every temporary directory TempDirFor makes, ".tmp-123". // makes and every temporary directory TempDirFor makes, ".tmp-123".
const tempNamePart = ".tmp-" const tempNamePart = ".tmp-"
// WriteFileAtomic replaces the file at path with data so that a reader, or // WriteFileAtomic replaces the file at path with data so that a reader, or
@@ -21,17 +21,38 @@ const tempNamePart = ".tmp-"
// within one filesystem), is synced to disk, and is renamed over path. The // within one filesystem), is synced to disk, and is renamed over path. The
// temporary file is removed if any step fails. // temporary file is removed if any step fails.
func WriteFileAtomic(fs afero.Fs, path string, data []byte) error { func WriteFileAtomic(fs afero.Fs, path string, data []byte) error {
tmp, err := afero.TempFile(fs, filepath.Dir(path), return WriteFileAtomicFunc(fs, path, func(file afero.File) error {
"."+filepath.Base(path)+tempNamePart+"*") _, err := file.Write(data)
if err != nil {
return fmt.Errorf("failed to write %s: %w", path, err)
}
return nil
})
}
// WriteFileAtomicFunc is WriteFileAtomic for content that write streams
// into the temporary file, which it is given, instead of holding it in
// memory. An error from write is returned as it is. The temporary file's
// name leaves out path's, which may already be as long as a name can be.
func WriteFileAtomicFunc(
fs afero.Fs, path string, write func(file afero.File) error,
) error {
tmp, err := afero.TempFile(fs, filepath.Dir(path), tempNamePart)
if err != nil { if err != nil {
return fmt.Errorf("failed to create temporary file for %s: %w", path, err) return fmt.Errorf("failed to create temporary file for %s: %w", path, err)
} }
_, err = tmp.Write(data) err = write(tmp)
if err == nil { if err != nil {
err = tmp.Sync() _ = tmp.Close()
_ = fs.Remove(tmp.Name())
return err
} }
err = tmp.Sync()
closeErr := tmp.Close() closeErr := tmp.Close()
if err == nil { if err == nil {
err = closeErr err = closeErr
+21 -32
View File
@@ -212,40 +212,16 @@ func processMnemonicForVault(
return ltIdentity, derivationIndex, publicKeyHash, familyHash, nil return ltIdentity, derivationIndex, publicKeyHash, familyHash, nil
} }
// CheckNewVaultName returns an error when name cannot be the name of a new
// vault: one wrapping ErrInvalidVaultName when it is not a valid vault name,
// and one wrapping ErrVaultExists when that vault already exists. Creating an
// existing vault again would replace its keys, and its secrets could no
// longer be decrypted. CreateVault checks this before writing anything; the
// commands that create a vault also check it before they ask for the
// mnemonic or passphrase, holding the state directory lock.
func CheckNewVaultName(fs afero.Fs, stateDir string, name string) error {
err := ValidateVaultName(name)
if err != nil {
return err
}
exists, err := afero.DirExists(fs, filepath.Join(stateDir, "vaults.d", name))
if err != nil {
return fmt.Errorf("failed to check if vault exists: %w", err)
}
if exists {
return fmt.Errorf("vault %s %w", name, ErrVaultExists)
}
return nil
}
// CreateVault creates a new vault and selects it as the current vault. When // CreateVault creates a new vault and selects it as the current vault. When
// mnemonic is not nil, the vault's long-term key is derived from it, and the // mnemonic is not nil, the vault's long-term key is derived from it, and the
// returned vault has it as its Mnemonic; when it is nil, the vault has no // returned vault has it as its Mnemonic; when it is nil, the vault has no
// long-term key until one is imported. When passphrase is not nil, the vault // long-term key until one is imported. When passphrase is not nil, the vault
// gets a passphrase unlocker protected by it, as its current unlocker; that // gets a passphrase unlocker protected by it, as its current unlocker; that
// needs a mnemonic. It refuses an invalid name or a vault that already exists, // needs a mnemonic. It refuses a vault that already exists before writing
// as CheckNewVaultName does, before writing anything. The commands that call // anything: creating it again would replace its keys, and its secrets could
// it hold the state directory lock, so no other command can create the vault // no longer be decrypted. The commands that call it hold the state directory
// between the check and the writes. // lock, so no other command can create the vault between the check and the
// writes.
// //
// The vault is written whole into a temporary directory, which is renamed // The vault is written whole into a temporary directory, which is renamed
// into vaults.d only once complete, and only then selected: a crash at any // into vaults.d only once complete, and only then selected: a crash at any
@@ -257,17 +233,30 @@ func CreateVault(
) (*Vault, error) { ) (*Vault, error) {
secret.Debug("Creating new vault", "name", name, "state_dir", stateDir) secret.Debug("Creating new vault", "name", name, "state_dir", stateDir)
err := CheckNewVaultName(fs, stateDir, name) err := ValidateVaultName(name)
if err != nil { if err != nil {
secret.Debug("Invalid vault name provided", "vault_name", name)
return nil, err return nil, err
} }
secret.Debug("Vault name validation passed", "vault_name", name)
vaultDir := filepath.Join(stateDir, "vaults.d", name)
exists, err := afero.DirExists(fs, vaultDir)
if err != nil {
return nil, fmt.Errorf("failed to check if vault exists: %w", err)
}
if exists {
return nil, fmt.Errorf("vault %s %w", name, ErrVaultExists)
}
if passphrase != nil && mnemonic == nil { if passphrase != nil && mnemonic == nil {
return nil, fmt.Errorf("vault %s %w", name, ErrUnlockerWithoutMnemonic) return nil, fmt.Errorf("vault %s %w", name, ErrUnlockerWithoutMnemonic)
} }
vaultDir := filepath.Join(stateDir, "vaults.d", name)
secret.Debug("Creating vault directory structure", "vault_dir", vaultDir) secret.Debug("Creating vault directory structure", "vault_dir", vaultDir)
err = secret.WriteDir(fs, vaultDir, func(dir string) error { err = secret.WriteDir(fs, vaultDir, func(dir string) error {