diff --git a/internal/secrets/candidate.go b/internal/secrets/candidate.go index dc3ac44..62b8eb2 100644 --- a/internal/secrets/candidate.go +++ b/internal/secrets/candidate.go @@ -8,8 +8,9 @@ import ( "github.com/alyraffauf/cattery/internal/failure" ) -// Candidate pairs the original plaintext with its candidate ciphertext for -// adoption validation. +// Candidate pairs caller-owned plaintext with candidate ciphertext for +// adoption validation. ValidateCandidate never clears or retains Plaintext; +// the caller remains responsible for clearing it after this call. type Candidate struct { Plaintext []byte Ciphertext []byte @@ -19,8 +20,9 @@ type Candidate struct { // ValidateCandidate round-trips candidate ciphertext through Decrypt and // requires byte-exact equality with the original plaintext before the // candidate may be adopted as repository content. The candidate must be -// nonempty valid JSON. Every plaintext buffer the round trip creates is -// zeroed on every path, and only the validated ciphertext returns. +// nonempty valid JSON. Every plaintext buffer created by the round trip is +// zeroed before return or error, and only the validated ciphertext returns. +// The caller-owned Candidate.Plaintext is not modified. func (client *Client) ValidateCandidate(ctx context.Context, candidate Candidate) ([]byte, error) { if len(candidate.Ciphertext) == 0 || !json.Valid(candidate.Ciphertext) { return nil, failure.New(failure.Operational, "sops encrypt "+candidate.SourcePath+" produced invalid candidate", nil) diff --git a/internal/secrets/candidate_test.go b/internal/secrets/candidate_test.go index c4ec564..18a91b5 100644 --- a/internal/secrets/candidate_test.go +++ b/internal/secrets/candidate_test.go @@ -42,12 +42,16 @@ func testCandidateEquality(t *testing.T) { if !bytes.Equal(adopted, candidate) { t.Fatalf("candidate not adopted exactly") } + if !bytes.Equal(plaintext, []byte("token=sekrit\n")) { + t.Fatal("caller-owned plaintext was modified") + } } func testCandidateMismatch(t *testing.T) { executable := sops.Build(t) repository := t.TempDir() client, _ := newTestClient(t, clientTarget{executable: executable, repository: repository, behavior: sops.Behavior{Stdout: []byte("different-plaintext")}}) - adopted, err := client.ValidateCandidate(context.Background(), Candidate{Plaintext: []byte("original-plaintext"), Ciphertext: []byte(`{"data":"x"}`), SourcePath: "app/token"}) + plaintext := []byte("original-plaintext") + adopted, err := client.ValidateCandidate(context.Background(), Candidate{Plaintext: plaintext, Ciphertext: []byte(`{"data":"x"}`), SourcePath: "app/token"}) expectKind(t, err, failure.Operational) if adopted != nil { t.Fatalf("mismatching candidate adopted") @@ -56,6 +60,9 @@ func testCandidateMismatch(t *testing.T) { if strings.Contains(message, "original-plaintext") || strings.Contains(message, "different-plaintext") { t.Fatalf("plaintext leaked into error: %q", message) } + if !bytes.Equal(plaintext, []byte("original-plaintext")) { + t.Fatal("caller-owned plaintext was modified on mismatch") + } } func testCandidateEmptyPlaintext(t *testing.T) { executable := sops.Build(t) diff --git a/internal/secrets/client.go b/internal/secrets/client.go index 843f759..07e43b8 100644 --- a/internal/secrets/client.go +++ b/internal/secrets/client.go @@ -10,6 +10,7 @@ import ( "context" "errors" "fmt" + "slices" "github.com/alyraffauf/cattery/internal/failure" "github.com/alyraffauf/cattery/internal/subprocess" @@ -32,7 +33,7 @@ type Client struct { // NewClient builds a client pinned to one executable, one repository working // directory, and one environment policy. func NewClient(executable string, directory string, environment []string) *Client { - return &Client{executable: executable, directory: directory, environment: environment} + return &Client{executable: executable, directory: directory, environment: slices.Clone(environment)} } // Request describes one SOPS invocation. Operation and SourcePath appear only diff --git a/internal/secrets/client_buffers_test.go b/internal/secrets/client_buffers_test.go new file mode 100644 index 0000000..57fa42b --- /dev/null +++ b/internal/secrets/client_buffers_test.go @@ -0,0 +1,103 @@ +package secrets + +import ( + "bytes" + "context" + "path/filepath" + "strings" + "testing" + + "github.com/alyraffauf/cattery/internal/failure" + "github.com/alyraffauf/cattery/internal/testfixture/sops" +) + +func testLargeStderr(t *testing.T) { + executable := sops.Build(t) + repository := t.TempDir() + marker := []byte("large-stderr-secret-") + stderr := bytes.Repeat(marker, 128*1024) + client, _ := newTestClient(t, clientTarget{executable: executable, repository: repository, behavior: sops.Behavior{Stderr: stderr, ExitCode: 7}}) + output, err := client.Run(context.Background(), basicRequest("decrypt", "app/token")) + expectKind(t, err, failure.Operational) + if output != nil { + t.Fatalf("output = %d bytes, want none", len(output)) + } + if strings.Contains(err.Error(), string(marker)) { + t.Fatal("large stderr leaked into error") + } +} + +func testEnvironmentCopy(t *testing.T) { + executable := sops.Build(t) + repository := t.TempDir() + command, err := executable.Command(sops.Behavior{Stdout: []byte("known-output")}) + if err != nil { + t.Fatal(err) + } + client := NewClient(executable.Path, repository, command.Env) + for index, entry := range command.Env { + if strings.HasPrefix(entry, "FAKE_SOPS_SPEC=") { + command.Env[index] = "FAKE_SOPS_SPEC=" + filepath.Join(repository, "missing-spec") + } + } + output, err := client.Run(context.Background(), basicRequest("encrypt", "app/token")) + if err != nil { + t.Fatalf("copied environment was changed: %v", err) + } + if !bytes.Equal(output, []byte("known-output")) { + t.Fatalf("output = %q, want known-output", output) + } +} + +func testBufferZeroing(t *testing.T) { + testZeroBytes(t) + testBoundedClear(t) +} + +func testZeroBytes(t *testing.T) { + data := []byte("sensitive-buffer") + zeroBytes(data) + if len(bytes.Trim(data, "\x00")) != 0 { + t.Fatal("zeroBytes left non-zero data") + } +} + +func testBoundedClear(t *testing.T) { + overflowed := false + capture := newBounded(4, func() { overflowed = true }) + capture.Write([]byte("abcd")) + if overflowed { + t.Fatal("overflow flagged before the limit") + } + capture.Write([]byte("xyz")) + if !overflowed { + t.Fatal("overflow not flagged past the limit") + } + if string(capture.buf) != "abcd" { + t.Fatalf("captured %q, want abcd", capture.buf) + } + owned := capture.buf + capture.clear() + if capture.buf != nil { + t.Fatal("clear left a buffer") + } + if !bytes.Equal(owned, make([]byte, len(owned))) { + t.Fatal("clear left captured bytes in its backing buffer") + } +} + +func testDrainCapture(t *testing.T) { + drain := newDrain(8) + drain.Write([]byte("abcdefghijklmnop")) + if len(drain.buf) != 8 { + t.Fatalf("drain kept %d bytes, want 8", len(drain.buf)) + } + owned := drain.buf + drain.clear() + if drain.buf != nil { + t.Fatal("drain clear left a buffer") + } + if !bytes.Equal(owned, make([]byte, len(owned))) { + t.Fatal("drain clear left captured bytes in its backing buffer") + } +} diff --git a/internal/secrets/client_test.go b/internal/secrets/client_test.go index 72a746b..2115143 100644 --- a/internal/secrets/client_test.go +++ b/internal/secrets/client_test.go @@ -22,9 +22,11 @@ func TestSOPSClient(t *testing.T) { }{ {"missing executable", testMissingExecutable}, {"nonzero exit", testNonzeroExit}, + {"large stderr", testLargeStderr}, {"stdout over limit", testStdoutOverLimit}, {"working directory", testWorkingDirectory}, {"environment", testEnvironment}, + {"environment copy", testEnvironmentCopy}, {"descendants", testDescendants}, {"buffer zeroing", testBufferZeroing}, {"drain capture", testDrainCapture}, @@ -136,37 +138,6 @@ func testDescendants(t *testing.T) { t.Fatal("descendant survived cancellation") } } -func testBufferZeroing(t *testing.T) { - data := []byte("sensitive-buffer") - zeroBytes(data) - if len(bytes.Trim(data, "\x00")) != 0 { - t.Fatal("zeroBytes left non-zero data") - } - overflowed := false - capture := newBounded(4, func() { overflowed = true }) - capture.Write([]byte("abcd")) - if overflowed { - t.Fatal("overflow flagged before the limit") - } - capture.Write([]byte("xyz")) - if !overflowed { - t.Fatal("overflow not flagged past the limit") - } - if string(capture.buf) != "abcd" { - t.Fatalf("captured %q, want abcd", capture.buf) - } - capture.clear() - if capture.buf != nil { - t.Fatal("clear left a buffer") - } -} -func testDrainCapture(t *testing.T) { - drain := newDrain(8) - drain.Write([]byte("abcdefghijklmnop")) - if len(drain.buf) != 8 { - t.Fatalf("drain kept %d bytes, want 8", len(drain.buf)) - } -} type clientTarget struct { executable *sops.Executable @@ -186,6 +157,7 @@ func newTestClient(t *testing.T, target clientTarget) (*Client, []string) { type fixtureRecord struct { Argv []string Cwd string + Stdin []byte ChildPid int } diff --git a/internal/secrets/decrypt_test.go b/internal/secrets/decrypt_test.go index 9a4a01f..4259382 100644 --- a/internal/secrets/decrypt_test.go +++ b/internal/secrets/decrypt_test.go @@ -21,6 +21,7 @@ func TestSOPSDecrypt(t *testing.T) { {"binary plaintext", testDecryptBinaryPlaintext}, {"empty plaintext", testDecryptEmptyPlaintext}, {"wrong json failure", testDecryptWrongJSON}, + {"launch failure", testDecryptLaunchFailure}, {"cancellation", testDecryptCancellation}, {"caller-owned plaintext", testDecryptOwnership}, } @@ -96,6 +97,15 @@ func testDecryptWrongJSON(t *testing.T) { t.Fatalf("diagnostic missing context: %q", message) } } +func testDecryptLaunchFailure(t *testing.T) { + repository := t.TempDir() + client := NewClient("sops-does-not-exist", repository, []string{}) + plaintext, err := client.Decrypt(context.Background(), []byte(`{"data":"x"}`), "app/token") + expectKind(t, err, failure.Dependency) + if plaintext != nil { + t.Fatalf("plaintext = %d bytes, want none", len(plaintext)) + } +} func testDecryptCancellation(t *testing.T) { executable := sops.Build(t) repository := t.TempDir() diff --git a/internal/secrets/encrypt_test.go b/internal/secrets/encrypt_test.go index 395d592..2e5230b 100644 --- a/internal/secrets/encrypt_test.go +++ b/internal/secrets/encrypt_test.go @@ -60,7 +60,7 @@ func testEncryptBinaryInput(t *testing.T) { repository := t.TempDir() plaintext := []byte{0x00, 0xff, 'a', 0x00, 0x01, 0xfe} output := []byte(`{"data":"aGVsbG8g","more":true}`) - client, _ := newTestClient(t, clientTarget{executable: executable, repository: repository, behavior: sops.Behavior{Stdout: output}}) + client, env := newTestClient(t, clientTarget{executable: executable, repository: repository, behavior: sops.Behavior{Stdout: output}}) got, err := client.Encrypt(context.Background(), plaintext, "app/token") if err != nil { t.Fatalf("err = %v", err) @@ -71,6 +71,9 @@ func testEncryptBinaryInput(t *testing.T) { if !json.Valid(got) { t.Fatalf("output is not valid json") } + if rec := readRecord(t, env); !bytes.Equal(rec.Stdin, plaintext) { + t.Fatalf("stdin = %v, want %v", rec.Stdin, plaintext) + } } func testEncryptMalformedOutput(t *testing.T) { executable := sops.Build(t) diff --git a/internal/testfixture/sops/executable.go b/internal/testfixture/sops/executable.go index 935c0ed..0ff180a 100644 --- a/internal/testfixture/sops/executable.go +++ b/internal/testfixture/sops/executable.go @@ -140,6 +140,7 @@ type spec struct { type record struct { Argv []string Cwd string + Stdin []byte Pid int ChildPid int } @@ -154,7 +155,8 @@ func main() { func run() { current := loadSpec() - rec := record{Argv: os.Args, Cwd: cwd(), Pid: os.Getpid()} + stdin, _ := io.ReadAll(os.Stdin) + rec := record{Argv: os.Args, Cwd: cwd(), Stdin: stdin, Pid: os.Getpid()} if current.Sleep > 0 { rec.ChildPid = spawnChild() } @@ -162,7 +164,7 @@ func run() { os.Stderr.Write(current.Stderr) os.Stdout.Write(current.Stdout) if current.EchoStdin { - io.Copy(os.Stdout, os.Stdin) + os.Stdout.Write(stdin) } if current.Sleep > 0 { time.Sleep(current.Sleep)