From 5b238e740b3198ce4395b3418e5cfcaf46cbb343 Mon Sep 17 00:00:00 2001 From: user Date: Thu, 3 Sep 2026 14:50:59 +0000 Subject: [PATCH] Enforce real timeouts on gpg subprocess calls (closes #62) Every gpg invocation went through runGPG, which built its command with exec.CommandContext(context.Background(), ...). That is the right call with the wrong context: context.Background() never expires, so no deadline was ever enforced on any of the five gpg call sites. runGPG now takes a context and derives a gpgTimeout deadline from it, honouring an earlier caller deadline when there is one. The gpg-touching library entry points take a ctx as their first argument so cancellation propagates from above: Builder.Build, NewManifestFromReader, NewManifestFromFile, NewChecker, Checker.ExtractEmbeddedSigningKeyFP. Scanner.ToManifest already had a ctx and now passes it down, which withdraws the //nolint:contextcheck claiming signing was "not cancellable by design" -- it is, and now it is. A deadline alone is not enough, and the added test proves it. gpg delegates to helpers (gpg-agent, pinentry) that inherit the captured stdout and stderr pipes. Go's default cancellation kills only the direct child, so the helper keeps the pipes open and Cmd.Wait blocks on the output-copying goroutines forever -- a dead process and a call that still never returns. Two additions fix that: the child runs in its own process group and cancellation kills the group, and Cmd.WaitDelay caps how long Wait will hold on for the pipes if something escapes the group anyway. Measured with the stand-in gpg from the new test: neither mechanism, hangs until `go test` gives up; WaitDelay only, returns in 2.2s; both, returns in 0.20s. Timeout errors now name the operation and how long gpg ran instead of surfacing a bare "signal: killed" or "context deadline exceeded", and a cancellation from above is reported as a cancellation rather than a timeout, so an abort is distinguishable from a stall. The test helper's own keygen invocations had the same unbounded context.Background() and the same pipe-inheriting agent problem, which makes them the actual mechanism behind the intermittent suite timeout noted in the issue: keygen starts gpg-agent, and a stalled agent hung the suite rather than failing it. They now run under a deadline with the same hardening, so a broken gpg environment skips instead of hanging. Verified with a cold `docker buildx build --no-cache`: prettier, gofmt, `make lint` (0 issues) and `make test` all executed and passed. The golang:1.23 image ships gpg, so the real signing, export, fingerprint, import and verify tests run against real gpg there, not skipped. The process-group kill is unix-only and lives in a build-tagged file; on other platforms the deadline is still enforced via cancellation plus WaitDelay, only the group kill of helpers is unavailable. --- README.md | 9 ++- TODO.md | 8 ++- internal/cli/check.go | 11 ++-- internal/cli/entry_test.go | 5 +- internal/cli/export.go | 2 +- internal/cli/fetch.go | 2 +- internal/cli/fetch_test.go | 12 ++-- internal/cli/freshen.go | 15 +++-- internal/cli/freshen_test.go | 6 +- internal/cli/list.go | 2 +- mfer/builder.go | 10 +++- mfer/builder_test.go | 34 ++++++----- mfer/checker.go | 17 ++++-- mfer/checker_test.go | 44 +++++++------- mfer/deserialize.go | 23 ++++--- mfer/errmsg_test.go | 6 +- mfer/gpg.go | 104 +++++++++++++++++++++++++++----- mfer/gpg_procgroup_other.go | 11 ++++ mfer/gpg_procgroup_unix.go | 36 +++++++++++ mfer/gpg_test.go | 107 ++++++++++++++++++++++----------- mfer/gpg_timeout_unix_test.go | 109 ++++++++++++++++++++++++++++++++++ mfer/scanner.go | 6 +- mfer/serialize.go | 17 +++--- 23 files changed, 457 insertions(+), 139 deletions(-) create mode 100644 mfer/gpg_procgroup_other.go create mode 100644 mfer/gpg_procgroup_unix.go create mode 100644 mfer/gpg_timeout_unix_test.go diff --git a/README.md b/README.md index ec05d47..6cd74c3 100644 --- a/README.md +++ b/README.md @@ -404,7 +404,14 @@ proto `go_package` option. Which is canonical? - [ ] Replace GPG subprocess calls with pure-Go crypto (pending design question answer) — current implementation shells out to `gpg` which may not be installed -- [ ] Add timeout to any remaining subprocess calls +- [x] Add timeout to any remaining subprocess calls — every `gpg` invocation now + runs under a real deadline (`gpgTimeout`, or an earlier caller deadline), + with a process-group kill and a `Cmd.WaitDelay` backstop so a helper + process holding the output pipes cannot outlive it. The library entry + points that can reach `gpg` (`Builder.Build`, `NewManifestFromReader`, + `NewManifestFromFile`, `NewChecker`, + `Checker.ExtractEmbeddedSigningKeyFP`) take a `context.Context` as their + first argument so cancellation propagates from above. ### CLI diff --git a/TODO.md b/TODO.md index 06f9dba..b63b5ac 100644 --- a/TODO.md +++ b/TODO.md @@ -24,6 +24,11 @@ only thing left of the `chore/align-repo-policies` branch is the list below. # Completed Steps +- 2026-09-03: enforced real deadlines on every gpg subprocess call + (`gpgTimeout`, process-group kill, `Cmd.WaitDelay`) and threaded + `context.Context` through the library entry points that can reach gpg, + withdrawing the "signing exec is not cancellable" `//nolint` in + `mfer/scanner.go` (#62) - 2026-08-09: added `.prettierrc`/`.prettierignore`, gave `script/fmt` and `script/fmt-check` one shared prettier file set via `script/prettier`, dropped the `|| true` that hid prettier failures, and added a node-based Dockerfile @@ -92,8 +97,7 @@ only thing left of the `chore/align-repo-policies` branch is the list below. - Add decompression size limit via io.LimitReader in deserializeInner() - Fix errors.Is dead code in checker; make AddFile verify totalRead == size - Export manifest type or define a public interface (pending) - - Replace GPG subprocess with pure-Go crypto (pending); add timeouts to - remaining subprocess calls + - Replace GPG subprocess with pure-Go crypto (pending) - CLI: - Kebab-case primary flag names; fix fetch URL construction with url.JoinPath; add http.Client timeout and retry with backoff to fetch; diff --git a/internal/cli/check.go b/internal/cli/check.go index 459465e..ab3b900 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -2,6 +2,7 @@ package cli import ( + "context" "encoding/hex" "errors" "fmt" @@ -126,7 +127,9 @@ func (mfa *CLIApp) fetchManifestToTemp(url string) (string, error) { // verifyRequiredSigner enforces the --require-signature fingerprint // against the manifest's embedded signing key. -func verifyRequiredSigner(chk *mfer.Checker, requiredSigner string) error { +func verifyRequiredSigner( + ctx context.Context, chk *mfer.Checker, requiredSigner string, +) error { // Validate fingerprint format: must be exactly 40 hex characters if len(requiredSigner) != fingerprintHexLen { return fmt.Errorf("%w, got %d", errInvalidFingerprint, len(requiredSigner)) @@ -145,7 +148,7 @@ func verifyRequiredSigner(chk *mfer.Checker, requiredSigner string) error { // Extract fingerprint from the embedded public key (not from the // signer field). This validates the key is importable and gets its // actual fingerprint. - embeddedFP, err := chk.ExtractEmbeddedSigningKeyFP() + embeddedFP, err := chk.ExtractEmbeddedSigningKeyFP(ctx) if err != nil { return fmt.Errorf( "failed to extract fingerprint from embedded signing key: %w", err) @@ -295,7 +298,7 @@ func (mfa *CLIApp) checkManifestOperation(ctx *cli.Context) error { log.Infof("checking manifest %s with base %s", manifestPath, basePath) // Create checker - chk, err := mfer.NewChecker(manifestPath, basePath, mfa.Fs) + chk, err := mfer.NewChecker(ctx.Context, manifestPath, basePath, mfa.Fs) if err != nil { return fmt.Errorf("failed to load manifest: %w", err) } @@ -303,7 +306,7 @@ func (mfa *CLIApp) checkManifestOperation(ctx *cli.Context) error { // Check signature requirement requiredSigner := ctx.String("require-signature") if requiredSigner != "" { - err = verifyRequiredSigner(chk, requiredSigner) + err = verifyRequiredSigner(ctx.Context, chk, requiredSigner) if err != nil { return err } diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 61868ac..8b3e389 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -3,6 +3,7 @@ package cli import ( "bytes" + "context" "errors" "fmt" "math/rand" @@ -283,7 +284,7 @@ func TestGenerateExcludesDotfilesByDefault(t *testing.T) { assert.True(t, exists) // Verify manifest only has 1 file (the non-dotfile) - manifest, err := mfer.NewManifestFromFile(fs, testMF) + manifest, err := mfer.NewManifestFromFile(context.Background(), fs, testMF) require.NoError(t, err) assert.Len(t, manifest.Files(), 1) assert.Equal(t, "file1.txt", manifest.Files()[0].GetPath()) @@ -307,7 +308,7 @@ func TestGenerateWithIncludeDotfiles(t *testing.T) { require.Equal(t, 0, exitCode) // Verify manifest has 2 files (including dotfile) - manifest, err := mfer.NewManifestFromFile(fs, testMF) + manifest, err := mfer.NewManifestFromFile(context.Background(), fs, testMF) require.NoError(t, err) assert.Len(t, manifest.Files(), 2) } diff --git a/internal/cli/export.go b/internal/cli/export.go index a557137..1889e63 100644 --- a/internal/cli/export.go +++ b/internal/cli/export.go @@ -32,7 +32,7 @@ func (mfa *CLIApp) exportManifestOperation(ctx *cli.Context) error { defer func() { _ = rc.Close() }() - manifest, err := mfer.NewManifestFromReader(rc) + manifest, err := mfer.NewManifestFromReader(ctx.Context, rc) if err != nil { return fmt.Errorf("export: failed to parse manifest: %w", err) } diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 8254a4c..6811118 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -183,7 +183,7 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error { } // Parse manifest - manifest, err := mfer.NewManifestFromReader(resp.Body) + manifest, err := mfer.NewManifestFromReader(ctx.Context, resp.Body) if err != nil { return fmt.Errorf("failed to parse manifest: %w", err) } diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index dd7a3e8..f18169d 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -244,7 +244,8 @@ func TestFetchFromHTTP(t *testing.T) { destDir := chdirTemp(t) // Parse the manifest to get file entries - manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + manifest, err := mfer.NewManifestFromReader( + context.Background(), bytes.NewReader(manifestData)) require.NoError(t, err) files := manifest.Files() @@ -293,7 +294,8 @@ func TestFetchHashMismatch(t *testing.T) { // Generate and parse manifest manifestData := scanToManifest(t, sourceFs) - manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + manifest, err := mfer.NewManifestFromReader( + context.Background(), bytes.NewReader(manifestData)) require.NoError(t, err) files := manifest.Files() @@ -339,7 +341,8 @@ func TestFetchSizeMismatch(t *testing.T) { // Generate and parse manifest manifestData := scanToManifest(t, sourceFs) - manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + manifest, err := mfer.NewManifestFromReader( + context.Background(), bytes.NewReader(manifestData)) require.NoError(t, err) files := manifest.Files() @@ -381,7 +384,8 @@ func TestFetchProgress(t *testing.T) { // Generate and parse manifest manifestData := scanToManifest(t, sourceFs) - manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + manifest, err := mfer.NewManifestFromReader( + context.Background(), bytes.NewReader(manifestData)) require.NoError(t, err) files := manifest.Files() diff --git a/internal/cli/freshen.go b/internal/cli/freshen.go index bc0ee20..179110a 100644 --- a/internal/cli/freshen.go +++ b/internal/cli/freshen.go @@ -1,6 +1,7 @@ package cli import ( + "context" "crypto/sha256" "errors" "fmt" @@ -304,7 +305,8 @@ func (h *freshenHasher) processEntry(e *freshenEntry) error { // writeFreshenedManifest writes the manifest atomically (write to a // temp file, then rename over the target). func writeFreshenedManifest( - afs afero.Fs, builder *mfer.Builder, manifestPath string, + ctx context.Context, afs afero.Fs, builder *mfer.Builder, + manifestPath string, ) error { tmpPath := manifestPath + ".tmp" @@ -313,7 +315,7 @@ func writeFreshenedManifest( return fmt.Errorf("failed to create temp file: %w", err) } - err = builder.Build(outFile) + err = builder.Build(ctx, outFile) _ = outFile.Close() if err != nil { @@ -439,12 +441,12 @@ func runFreshenHash( // loadExistingEntries loads the manifest and indexes its file entries // by path. func (mfa *CLIApp) loadExistingEntries( - manifestPath string, + ctx context.Context, manifestPath string, ) (map[string]*mfer.MFFilePath, error) { log.Infof("loading manifest from %s", manifestPath) // Load existing manifest - manifest, err := mfer.NewManifestFromFile(mfa.Fs, manifestPath) + manifest, err := mfer.NewManifestFromFile(ctx, mfa.Fs, manifestPath) if err != nil { return nil, fmt.Errorf("failed to load manifest: %w", err) } @@ -473,7 +475,7 @@ func (mfa *CLIApp) freshenManifestOperation(ctx *cli.Context) error { return fmt.Errorf("freshen: %w", err) } - existingByPath, err := mfa.loadExistingEntries(manifestPath) + existingByPath, err := mfa.loadExistingEntries(ctx.Context, manifestPath) if err != nil { return err } @@ -530,7 +532,8 @@ func (mfa *CLIApp) freshenManifestOperation(ctx *cli.Context) error { } // Write updated manifest atomically (write to temp, then rename) - err = writeFreshenedManifest(mfa.Fs, hasher.builder, manifestPath) + err = writeFreshenedManifest( + ctx.Context, mfa.Fs, hasher.builder, manifestPath) if err != nil { return err } diff --git a/internal/cli/freshen_test.go b/internal/cli/freshen_test.go index e5d844d..9ee72fc 100644 --- a/internal/cli/freshen_test.go +++ b/internal/cli/freshen_test.go @@ -58,7 +58,8 @@ func TestFreshenUnchanged(t *testing.T) { setupFreshenDir(t, fs) // Parse manifest to verify - manifest, err := mfer.NewManifestFromFile(fs, "/testdir/.index.mf") + manifest, err := mfer.NewManifestFromFile( + context.Background(), fs, "/testdir/.index.mf") require.NoError(t, err) assert.Len(t, manifest.Files(), 2) } @@ -70,7 +71,8 @@ func TestFreshenWithChanges(t *testing.T) { setupFreshenDir(t, fs) // Verify initial manifest has 2 files - manifest, err := mfer.NewManifestFromFile(fs, "/testdir/.index.mf") + manifest, err := mfer.NewManifestFromFile( + context.Background(), fs, "/testdir/.index.mf") require.NoError(t, err) assert.Len(t, manifest.Files(), 2) diff --git a/internal/cli/list.go b/internal/cli/list.go index dbf2096..f883283 100644 --- a/internal/cli/list.go +++ b/internal/cli/list.go @@ -28,7 +28,7 @@ func (mfa *CLIApp) listManifestOperation(ctx *cli.Context) error { defer func() { _ = rc.Close() }() - manifest, err := mfer.NewManifestFromReader(rc) + manifest, err := mfer.NewManifestFromReader(ctx.Context, rc) if err != nil { return fmt.Errorf("list: failed to parse manifest: %w", err) } diff --git a/mfer/builder.go b/mfer/builder.go index b93f79e..5e1524e 100644 --- a/mfer/builder.go +++ b/mfer/builder.go @@ -3,6 +3,7 @@ package mfer import ( + "context" "crypto/sha256" "errors" "fmt" @@ -282,7 +283,10 @@ func (b *Builder) SetSigningOptions(opts *SigningOptions) { } // Build finalizes the manifest and writes it to the writer. -func (b *Builder) Build(w io.Writer) error { +// +// When signing options are set, Build shells out to gpg; ctx bounds +// those invocations and cancels them if it is cancelled. +func (b *Builder) Build(ctx context.Context, w io.Writer) error { b.mu.Lock() defer b.mu.Unlock() @@ -308,13 +312,13 @@ func (b *Builder) Build(w io.Writer) error { } // Generate outer wrapper - err := m.generateOuter() + err := m.generateOuter(ctx) if err != nil { return fmt.Errorf("build: generate outer: %w", err) } // Generate final output - err = m.generate() + err = m.generate(ctx) if err != nil { return fmt.Errorf("build: generate: %w", err) } diff --git a/mfer/builder_test.go b/mfer/builder_test.go index 7c20e9c..d7d9aca 100644 --- a/mfer/builder_test.go +++ b/mfer/builder_test.go @@ -3,6 +3,7 @@ package mfer import ( "bytes" + "context" "strings" "testing" "time" @@ -113,7 +114,7 @@ func TestBuilderBuild(t *testing.T) { var buf bytes.Buffer - err = b.Build(&buf) + err = b.Build(context.Background(), &buf) require.NoError(t, err) // Should have magic bytes @@ -177,7 +178,7 @@ func TestBuilderDeterministicOutput(t *testing.T) { var buf bytes.Buffer - err := b.Build(&buf) + err := b.Build(context.Background(), &buf) require.NoError(t, err) return buf.Bytes() @@ -325,9 +326,9 @@ func TestBuilderBuildRoundTrip(t *testing.T) { } var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) + require.NoError(t, b.Build(context.Background(), &buf)) - m, err := NewManifestFromReader(&buf) + m, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) mfiles := m.Files() @@ -351,7 +352,8 @@ func TestBuilderBuildRoundTrip(t *testing.T) { func TestNewManifestFromReaderInvalidMagic(t *testing.T) { t.Parallel() - _, err := NewManifestFromReader(bytes.NewReader([]byte("NOT_VALID"))) + _, err := NewManifestFromReader( + context.Background(), bytes.NewReader([]byte("NOT_VALID"))) require.Error(t, err) assert.Contains(t, err.Error(), "invalid file format") } @@ -359,7 +361,7 @@ func TestNewManifestFromReaderInvalidMagic(t *testing.T) { func TestNewManifestFromReaderEmpty(t *testing.T) { t.Parallel() - _, err := NewManifestFromReader(bytes.NewReader([]byte{})) + _, err := NewManifestFromReader(context.Background(), bytes.NewReader([]byte{})) assert.Error(t, err) } @@ -367,7 +369,7 @@ func TestNewManifestFromReaderTruncated(t *testing.T) { t.Parallel() // Just the magic with nothing after - _, err := NewManifestFromReader(bytes.NewReader([]byte(MAGIC))) + _, err := NewManifestFromReader(context.Background(), bytes.NewReader([]byte(MAGIC))) assert.Error(t, err) } @@ -383,9 +385,9 @@ func TestManifestString(t *testing.T) { require.NoError(t, err) var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) + require.NoError(t, b.Build(context.Background(), &buf)) - m, err := NewManifestFromReader(&buf) + m, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) assert.Contains(t, m.String(), "count=1") } @@ -397,7 +399,7 @@ func TestBuilderBuildEmpty(t *testing.T) { var buf bytes.Buffer - err := b.Build(&buf) + err := b.Build(context.Background(), &buf) require.NoError(t, err) // Should still produce valid manifest with 0 files @@ -416,9 +418,9 @@ func TestBuilderOmitsCreatedAtByDefault(t *testing.T) { require.NoError(t, err) var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) + require.NoError(t, b.Build(context.Background(), &buf)) - m, err := NewManifestFromReader(&buf) + m, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) assert.Nil(t, m.pbInner.GetCreatedAt(), "createdAt should be nil by default for deterministic output") @@ -438,9 +440,9 @@ func TestBuilderIncludesCreatedAtWhenRequested(t *testing.T) { require.NoError(t, err) var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) + require.NoError(t, b.Build(context.Background(), &buf)) - m, err := NewManifestFromReader(&buf) + m, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) assert.NotNil(t, m.pbInner.GetCreatedAt(), "createdAt should be set when IncludeTimestamps is true") @@ -464,8 +466,8 @@ func TestBuilderDeterministicFileOrder(t *testing.T) { } var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) - m, err := NewManifestFromReader(&buf) + require.NoError(t, b.Build(context.Background(), &buf)) + m, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) return m.Files() diff --git a/mfer/checker.go b/mfer/checker.go index fe5969e..db928e4 100644 --- a/mfer/checker.go +++ b/mfer/checker.go @@ -83,14 +83,19 @@ type Checker struct { } // NewChecker creates a new Checker for the given manifest, base path, and filesystem. +// +// Loading a signed manifest verifies its signature, which shells out to +// gpg; ctx bounds that invocation. // The basePath is the directory relative to which manifest paths are resolved. // If fs is nil, the real filesystem (OsFs) is used. -func NewChecker(manifestPath string, basePath string, fs afero.Fs) (*Checker, error) { +func NewChecker( + ctx context.Context, manifestPath string, basePath string, fs afero.Fs, +) (*Checker, error) { if fs == nil { fs = afero.NewOsFs() } - m, err := NewManifestFromFile(fs, manifestPath) + m, err := NewManifestFromFile(ctx, fs, manifestPath) if err != nil { return nil, err } @@ -164,12 +169,16 @@ func (c *Checker) SigningPubKey() []byte { // ExtractEmbeddedSigningKeyFP imports the manifest's embedded public key into a // temporary keyring and extracts its fingerprint. This validates the key and // returns its actual fingerprint from the key material itself. -func (c *Checker) ExtractEmbeddedSigningKeyFP() (string, error) { +// +// The import shells out to gpg; ctx bounds that invocation. +func (c *Checker) ExtractEmbeddedSigningKeyFP( + ctx context.Context, +) (string, error) { if len(c.signingPubKey) == 0 { return "", errNoSigningPubKey } - return gpgExtractPubKeyFingerprint(c.signingPubKey) + return gpgExtractPubKeyFingerprint(ctx, c.signingPubKey) } // Check verifies all files against the manifest. diff --git a/mfer/checker_test.go b/mfer/checker_test.go index 1e79218..35b79f3 100644 --- a/mfer/checker_test.go +++ b/mfer/checker_test.go @@ -61,7 +61,7 @@ func createTestManifest( } var buf bytes.Buffer - require.NoError(t, builder.Build(&buf)) + require.NoError(t, builder.Build(context.Background(), &buf)) require.NoError(t, afero.WriteFile(fs, manifestPath, buf.Bytes(), 0o644)) } @@ -92,7 +92,7 @@ func TestNewChecker(t *testing.T) { } createTestManifest(t, fs, "/manifest.mf", files) - chk, err := NewChecker("/manifest.mf", "/", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/", fs) require.NoError(t, err) assert.NotNil(t, chk) assert.Equal(t, FileCount(2), chk.FileCount()) @@ -102,7 +102,7 @@ func TestNewChecker(t *testing.T) { t.Parallel() fs := afero.NewMemMapFs() - _, err := NewChecker("/nonexistent.mf", "/", fs) + _, err := NewChecker(context.Background(), "/nonexistent.mf", "/", fs) assert.Error(t, err) }) @@ -111,7 +111,7 @@ func TestNewChecker(t *testing.T) { fs := afero.NewMemMapFs() require.NoError(t, afero.WriteFile(fs, "/bad.mf", []byte("not a manifest"), 0o644)) - _, err := NewChecker("/bad.mf", "/", fs) + _, err := NewChecker(context.Background(), "/bad.mf", "/", fs) assert.Error(t, err) }) } @@ -127,7 +127,7 @@ func TestCheckerFileCountAndTotalBytes(t *testing.T) { } createTestManifest(t, fs, "/manifest.mf", files) - chk, err := NewChecker("/manifest.mf", "/", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/", fs) require.NoError(t, err) assert.Equal(t, FileCount(3), chk.FileCount()) @@ -145,7 +145,7 @@ func TestCheckAllFilesOK(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -178,7 +178,7 @@ func TestCheckMissingFile(t *testing.T) { testExistsFile: []byte("I exist"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -217,7 +217,7 @@ func TestCheckSizeMismatch(t *testing.T) { testFileName: []byte("short"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -245,7 +245,7 @@ func TestCheckHashMismatch(t *testing.T) { testFileName: differentContent, }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -268,7 +268,7 @@ func TestCheckWithProgress(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -308,7 +308,7 @@ func TestCheckContextCancellation(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) ctx, cancel := context.WithCancel(context.Background()) @@ -335,7 +335,7 @@ func TestFindExtraFiles(t *testing.T) { testFile2: []byte("extra file"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -371,7 +371,7 @@ func TestFindExtraFilesSkipsManifestAndDotfiles(t *testing.T) { require.NoError(t, fs.MkdirAll("/data", 0o755)) require.NoError(t, afero.WriteFile(fs, "/data/extra.txt", []byte("extra"), 0o644)) - chk, err := NewChecker("/data/.index.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/data/.index.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -403,7 +403,7 @@ func TestFindExtraFilesContextCancellation(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) ctx, cancel := context.WithCancel(context.Background()) @@ -422,7 +422,7 @@ func TestCheckNilChannels(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) // Should not panic with nil channels @@ -438,7 +438,7 @@ func TestFindExtraFilesNilChannel(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) // Should not panic with nil channel @@ -465,7 +465,7 @@ func TestCheckSubdirectories(t *testing.T) { require.NoError(t, afero.WriteFile(fs, fullPath, content, 0o644)) } - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -499,7 +499,7 @@ func TestCheckMissingFileDetectedWithoutFallback(t *testing.T) { testExistsFile: []byte("here"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -537,7 +537,7 @@ func TestFindExtraFilesSkipsDotfiles(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/data/.git/config", []byte("git config"), 0o644)) - chk, err := NewChecker("/data/.index.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/data/.index.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -566,7 +566,7 @@ func TestFindExtraFilesSkipsManifestFile(t *testing.T) { createTestManifest(t, fs, "/data/index.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/data/index.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/data/index.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -589,7 +589,7 @@ func TestCheckEmptyManifest(t *testing.T) { // Create manifest with no files createTestManifest(t, fs, "/manifest.mf", map[string][]byte{}) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) assert.Equal(t, FileCount(0), chk.FileCount()) @@ -624,7 +624,7 @@ func TestCheckProgressRateLimited(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 200) diff --git a/mfer/deserialize.go b/mfer/deserialize.go index a099d4a..383d5c0 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -2,6 +2,7 @@ package mfer import ( "bytes" + "context" "crypto/sha256" "errors" "fmt" @@ -63,7 +64,7 @@ func (m *manifest) validateOuterHeader() error { // verifyOuterIntegrity checks the hash of the compressed payload and, // if a signature is present, verifies it against the embedded public key. -func (m *manifest) verifyOuterIntegrity() error { +func (m *manifest) verifyOuterIntegrity(ctx context.Context) error { h := sha256.New() _, err := h.Write(m.pbOuter.GetInnerMessage()) @@ -92,6 +93,7 @@ func (m *manifest) verifyOuterIntegrity() error { } err = gpgVerify( + ctx, []byte(sigString), m.pbOuter.GetSignature(), m.pbOuter.GetSigningPubKey(), @@ -139,13 +141,13 @@ func (m *manifest) decompressInner() ([]byte, error) { return dat, nil } -func (m *manifest) deserializeInner() error { +func (m *manifest) deserializeInner(ctx context.Context) error { err := m.validateOuterHeader() if err != nil { return err } - err = m.verifyOuterIntegrity() + err = m.verifyOuterIntegrity(ctx) if err != nil { return err } @@ -200,8 +202,13 @@ func validateMagic(dat []byte) bool { // NewManifestFromReader reads a manifest from an io.Reader. // +// A signed manifest is verified as it is loaded, which shells out to +// gpg; ctx bounds that invocation and cancels it if it is cancelled. +// //nolint:revive // unexported-return: exporting manifest is owner question 13 -func NewManifestFromReader(input io.Reader) (*manifest, error) { +func NewManifestFromReader( + ctx context.Context, input io.Reader, +) (*manifest, error) { m := &manifest{} dat, err := io.ReadAll(input) @@ -227,7 +234,7 @@ func NewManifestFromReader(input io.Reader) (*manifest, error) { } // deserialize inner: - err = m.deserializeInner() + err = m.deserializeInner(ctx) if err != nil { return nil, err } @@ -239,7 +246,9 @@ func NewManifestFromReader(input io.Reader) (*manifest, error) { // If fs is nil, the real filesystem (OsFs) is used. // //nolint:revive // unexported-return: exporting manifest is owner question 13 -func NewManifestFromFile(fs afero.Fs, path string) (*manifest, error) { +func NewManifestFromFile( + ctx context.Context, fs afero.Fs, path string, +) (*manifest, error) { if fs == nil { fs = afero.NewOsFs() } @@ -251,5 +260,5 @@ func NewManifestFromFile(fs afero.Fs, path string) (*manifest, error) { defer func() { _ = f.Close() }() - return NewManifestFromReader(f) + return NewManifestFromReader(ctx, f) } diff --git a/mfer/errmsg_test.go b/mfer/errmsg_test.go index f07e06c..d2dc32c 100644 --- a/mfer/errmsg_test.go +++ b/mfer/errmsg_test.go @@ -2,6 +2,7 @@ package mfer import ( + "context" "testing" "github.com/stretchr/testify/assert" @@ -80,6 +81,7 @@ func TestSerializeInternalErrorMessagesVerbatim(t *testing.T) { t.Parallel() m := &manifest{} - require.EqualError(t, m.generate(), "internal error: pbInner not set") - require.EqualError(t, m.generateOuter(), "internal error") + require.EqualError(t, m.generate(context.Background()), + "internal error: pbInner not set") + require.EqualError(t, m.generateOuter(context.Background()), "internal error") } diff --git a/mfer/gpg.go b/mfer/gpg.go index 568d672..70b3ebe 100644 --- a/mfer/gpg.go +++ b/mfer/gpg.go @@ -10,6 +10,7 @@ import ( "os/exec" "path/filepath" "strings" + "time" ) const ( @@ -33,12 +34,51 @@ const ( gpgOptArmor = "--armor" gpgOptHomedir = "--homedir" gpgOptVerify = "--verify" + + // gpgTimeout bounds a single gpg invocation. + // + // Every invocation runs with --batch --no-tty, so gpg never waits on + // human input: a missing passphrase or an unusable pinentry makes it + // fail immediately instead of blocking. The legitimately slow paths + // that remain are a cold gpg-agent start, hardware-token crypto, and + // entropy starvation on a freshly booted VM -- all low single-digit + // seconds. 30s leaves an order of magnitude of headroom over that + // while still bounding a wedged agent to something that fits inside a + // normal CI step instead of hanging forever. + // + // Callers needing a tighter bound pass a ctx with an earlier + // deadline; runGPG honours whichever of the two comes first. + gpgTimeout = 30 * time.Second + + // gpgWaitDelay bounds how long Cmd.Wait keeps waiting after the + // invocation has been cancelled. + // + // Cancellation kills gpg, but any descendant that inherited the write + // end of our stdout or stderr pipe keeps it open, and Wait blocks on + // the output-copying goroutines until every writer is gone. Without a + // WaitDelay that turns the deadline into no deadline at all: gpg is + // dead and Run still never returns. Once this delay elapses Cmd + // force-closes the pipes and Wait returns. gpg itself dies to SIGKILL + // at once, so this only ever elapses when something downstream is + // still holding a pipe. + gpgWaitDelay = 2 * time.Second + + // gpg operation labels. These name the failing invocation in timeout + // and cancellation errors, which the os/exec layer would otherwise + // report as a bare "signal: killed". + gpgOpSign = "sign" + gpgOpExport = "export" + gpgOpFingerprint = "fingerprint" + gpgOpImport = "import" + gpgOpListKeys = "list-keys" + gpgOpVerify = "verify" ) var ( errGPGKeyNotFound = errors.New("gpg key not found") errFingerprintNotFound = errors.New("fingerprint not found for key") errImportedFPRNotFound = errors.New("fingerprint not found in imported key") + errGPGTimeout = errors.New("gpg invocation timed out") ) // GPGKeyID represents a GPG key identifier (fingerprint or key ID). @@ -67,25 +107,53 @@ func gpgArgs(opts []string, positional ...string) []string { // runGPG runs the gpg binary in batch mode with the given arguments and // optional stdin, returning captured stdout and stderr. -func runGPG(stdin io.Reader, args ...string) (*bytes.Buffer, *bytes.Buffer, error) { +// +// The invocation is bounded by gpgTimeout, or by ctx's own deadline when +// that is sooner. op names the operation for diagnostics; see the gpgOp* +// constants. +func runGPG( + ctx context.Context, op string, stdin io.Reader, args ...string, +) (*bytes.Buffer, *bytes.Buffer, error) { fullArgs := append([]string{"--batch", "--no-tty"}, args...) + runCtx, cancel := context.WithTimeout(ctx, gpgTimeout) + defer cancel() + // G204: the executable name is a compile-time constant. The arguments // are not, so the guarantee that matters is placement: every // caller-supplied value is passed either as the value of a named // option or after the "--" end-of-options marker inserted by gpgArgs, // and therefore cannot be reinterpreted by gpg as an option. cmd := exec.CommandContext( //nolint:gosec // G204: see comment above - context.Background(), "gpg", fullArgs...) + runCtx, "gpg", fullArgs...) cmd.Stdin = stdin + cmd.WaitDelay = gpgWaitDelay + + setGPGProcessGroup(cmd) var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr + started := time.Now() err := cmd.Run() + // A killed subprocess reports only "signal: killed", which names + // neither the operation nor the reason, and the bare context error is + // no better. Say which invocation died and why. Whether the deadline + // that fired was gpgTimeout or an earlier one from the caller does not + // change the diagnosis, so report how long gpg actually ran rather + // than guessing which limit applied. + switch { + case err == nil: + case errors.Is(runCtx.Err(), context.DeadlineExceeded): + err = fmt.Errorf("%w: operation %q killed after %s", + errGPGTimeout, op, time.Since(started).Round(time.Millisecond)) + case errors.Is(runCtx.Err(), context.Canceled): + err = fmt.Errorf("gpg %s cancelled: %w", op, runCtx.Err()) + } + return &stdout, &stderr, err } @@ -105,8 +173,10 @@ func parseFingerprint(colonOutput string) (string, bool) { // gpgSign creates a detached signature of the data using the specified key. // Returns the armored detached signature. -func gpgSign(data []byte, keyID GPGKeyID) ([]byte, error) { - stdout, stderr, err := runGPG(bytes.NewReader(data), +func gpgSign( + ctx context.Context, data []byte, keyID GPGKeyID, +) ([]byte, error) { + stdout, stderr, err := runGPG(ctx, gpgOpSign, bytes.NewReader(data), "--detach-sign", gpgOptArmor, "--local-user", string(keyID), @@ -120,8 +190,10 @@ func gpgSign(data []byte, keyID GPGKeyID) ([]byte, error) { // gpgExportPublicKey exports the public key for the specified key ID. // Returns the armored public key. -func gpgExportPublicKey(keyID GPGKeyID) ([]byte, error) { - stdout, stderr, err := runGPG(nil, +func gpgExportPublicKey( + ctx context.Context, keyID GPGKeyID, +) ([]byte, error) { + stdout, stderr, err := runGPG(ctx, gpgOpExport, nil, gpgArgs([]string{"--export", gpgOptArmor}, string(keyID))..., ) if err != nil { @@ -136,8 +208,10 @@ func gpgExportPublicKey(keyID GPGKeyID) ([]byte, error) { } // gpgGetKeyFingerprint gets the full fingerprint for a key ID. -func gpgGetKeyFingerprint(keyID GPGKeyID) ([]byte, error) { - stdout, stderr, err := runGPG(nil, +func gpgGetKeyFingerprint( + ctx context.Context, keyID GPGKeyID, +) ([]byte, error) { + stdout, stderr, err := runGPG(ctx, gpgOpFingerprint, nil, gpgArgs([]string{"--with-colons", "--fingerprint"}, string(keyID))..., ) if err != nil { @@ -157,7 +231,9 @@ func gpgGetKeyFingerprint(keyID GPGKeyID) ([]byte, error) { // gpgExtractPubKeyFingerprint imports a public key into a temporary keyring // and extracts its fingerprint. This verifies the key is valid and returns // the actual fingerprint from the key material. -func gpgExtractPubKeyFingerprint(pubKey []byte) (string, error) { +func gpgExtractPubKeyFingerprint( + ctx context.Context, pubKey []byte, +) (string, error) { // Create temporary directory for GPG operations tmpDir, err := os.MkdirTemp("", "mfer-gpg-fingerprint-*") if err != nil { @@ -181,7 +257,7 @@ func gpgExtractPubKeyFingerprint(pubKey []byte) (string, error) { } // Import the public key into the temporary keyring - _, importStderr, err := runGPG(nil, + _, importStderr, err := runGPG(ctx, gpgOpImport, nil, gpgArgs([]string{gpgOptHomedir, tmpDir, "--import"}, pubKeyFile)..., ) if err != nil { @@ -191,7 +267,7 @@ func gpgExtractPubKeyFingerprint(pubKey []byte) (string, error) { } // List keys to get fingerprint - listStdout, listStderr, err := runGPG(nil, + listStdout, listStderr, err := runGPG(ctx, gpgOpListKeys, nil, "--homedir", tmpDir, "--with-colons", "--fingerprint", @@ -212,7 +288,7 @@ func gpgExtractPubKeyFingerprint(pubKey []byte) (string, error) { // gpgVerify verifies a detached signature against data using the provided public key. // It creates a temporary keyring to import the public key for verification. -func gpgVerify(data, signature, pubKey []byte) error { +func gpgVerify(ctx context.Context, data, signature, pubKey []byte) error { // Create temporary directory for GPG operations tmpDir, err := os.MkdirTemp("", "mfer-gpg-verify-*") if err != nil { @@ -252,7 +328,7 @@ func gpgVerify(data, signature, pubKey []byte) error { } // Import the public key into the temporary keyring - _, importStderr, err := runGPG(nil, + _, importStderr, err := runGPG(ctx, gpgOpImport, nil, gpgArgs([]string{gpgOptHomedir, tmpDir, "--import"}, pubKeyFile)..., ) if err != nil { @@ -262,7 +338,7 @@ func gpgVerify(data, signature, pubKey []byte) error { } // Verify the signature - _, verifyStderr, err := runGPG(nil, + _, verifyStderr, err := runGPG(ctx, gpgOpVerify, nil, gpgArgs([]string{gpgOptHomedir, tmpDir, gpgOptVerify}, sigFile, dataFile)..., ) diff --git a/mfer/gpg_procgroup_other.go b/mfer/gpg_procgroup_other.go new file mode 100644 index 0000000..bb3842e --- /dev/null +++ b/mfer/gpg_procgroup_other.go @@ -0,0 +1,11 @@ +//go:build !unix + +package mfer + +import "os/exec" + +// setGPGProcessGroup is a no-op on platforms without POSIX process +// groups. The deadline is still enforced there: cancellation kills the +// gpg process itself and Cmd.WaitDelay still bounds Cmd.Wait. Only the +// group kill of helper processes gpg may have started is unavailable. +func setGPGProcessGroup(_ *exec.Cmd) {} diff --git a/mfer/gpg_procgroup_unix.go b/mfer/gpg_procgroup_unix.go new file mode 100644 index 0000000..8f10bfd --- /dev/null +++ b/mfer/gpg_procgroup_unix.go @@ -0,0 +1,36 @@ +//go:build unix + +package mfer + +import ( + "os/exec" + "syscall" +) + +// setGPGProcessGroup puts gpg in its own process group and makes +// cancellation kill that whole group rather than gpg alone. +// +// Go's default cancel function signals only the direct child. gpg +// routinely delegates to helpers (gpg-agent, pinentry, scdaemon) and +// starts them itself on first use; killing gpg alone leaves those +// running, still holding the write end of our stdout and stderr pipes, +// which is precisely what would keep Cmd.Wait blocked past the deadline. +// Killing the group kills everything gpg started that has not +// deliberately left it. +// +// A gpg-agent that was already running is unaffected: it calls setsid at +// startup and so is never a member of our group. Nor can we guarantee +// every future helper stays in the group. That is why gpgWaitDelay +// remains necessary as a backstop rather than being made redundant by +// this. +func setGPGProcessGroup(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + + cmd.Cancel = func() error { + // A negative pid targets the process group. SIGKILL rather than + // SIGINT: gpg blocked on an agent socket does not reliably act + // on SIGINT, and by the time Cancel runs the deadline has + // already expired, so there is nothing left to be polite about. + return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + } +} diff --git a/mfer/gpg_test.go b/mfer/gpg_test.go index 43aa9d6..8a069f5 100644 --- a/mfer/gpg_test.go +++ b/mfer/gpg_test.go @@ -9,12 +9,55 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// gpgTestKeygenTimeout bounds the gpg invocations that build the test +// keyring. Generating an RSA-2048 key also starts gpg-agent, which is +// far slower than any operation under test (measured at well under a +// second with a working agent). If it has not finished by now the +// environment is not going to produce a key at all, and the test should +// skip rather than consume the whole `go test` budget. +const gpgTestKeygenTimeout = 5 * time.Second + +// runTestGPG runs gpg for test setup under a real deadline, returning +// captured stdout and stderr. +// +// This mirrors runGPG's hardening rather than calling it: setup needs its +// own GNUPGHOME, and it runs commands (--gen-key) that no production call +// site issues. The deadline matters as much here as in production -- +// keygen starts gpg-agent, which inherits the output pipes, so without +// WaitDelay and the process-group kill a stalled agent hangs the suite +// instead of failing it. +func runTestGPG( + t *testing.T, gpgHome string, args ...string, +) (*bytes.Buffer, *bytes.Buffer, error) { + t.Helper() + + ctx, cancel := context.WithTimeout( + context.Background(), gpgTestKeygenTimeout) + defer cancel() + + //nolint:gosec // G204: every argument here is a test-controlled literal + cmd := exec.CommandContext(ctx, "gpg", args...) + + cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) + cmd.WaitDelay = gpgWaitDelay + + setGPGProcessGroup(cmd) + + var stdout, stderr bytes.Buffer + + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + return &stdout, &stderr, cmd.Run() +} + // testGPGEnv sets up a temporary GPG home directory with a test key. // Returns the key ID and the GPG home directory; callers must point // GNUPGHOME at the returned directory (via t.Setenv) before using the @@ -43,24 +86,16 @@ Expire-Date: 0 paramsFile := filepath.Join(gpgHome, "key-params") require.NoError(t, os.WriteFile(paramsFile, []byte(keyParams), 0o600)) - //nolint:gosec // paramsFile is a test-controlled path inside t.TempDir() - cmd := exec.CommandContext(context.Background(), "gpg", + _, genStderr, err := runTestGPG(t, gpgHome, "--batch", "--gen-key", paramsFile) - - cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) - - output, err := cmd.CombinedOutput() if err != nil { - t.Skipf("failed to generate test GPG key: %v: %s", err, output) + t.Skipf("failed to generate test GPG key: %v: %s", + err, genStderr.String()) } // Get the key fingerprint - cmd = exec.CommandContext(context.Background(), "gpg", + listStdout, _, err := runTestGPG(t, gpgHome, "--list-keys", "--with-colons", "test@mfer.test") - - cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) - - output, err = cmd.Output() if err != nil { t.Fatalf("failed to list test key: %v", err) } @@ -68,7 +103,7 @@ Expire-Date: 0 // Parse fingerprint from output var keyID string - for _, line := range strings.Split(string(output), "\n") { + for _, line := range strings.Split(listStdout.String(), "\n") { fields := strings.Split(line, ":") if len(fields) >= gpgFingerprintMinFields && fields[0] == gpgFingerprintField { @@ -90,7 +125,7 @@ func TestGPGSign(t *testing.T) { t.Setenv("GNUPGHOME", gpgHome) data := []byte("test data to sign") - sig, err := gpgSign(data, keyID) + sig, err := gpgSign(context.Background(), data, keyID) require.NoError(t, err) assert.NotEmpty(t, sig) assert.Contains(t, string(sig), "-----BEGIN PGP SIGNATURE-----") @@ -101,7 +136,7 @@ func TestGPGExportPublicKey(t *testing.T) { keyID, gpgHome := testGPGEnv(t) t.Setenv("GNUPGHOME", gpgHome) - pubKey, err := gpgExportPublicKey(keyID) + pubKey, err := gpgExportPublicKey(context.Background(), keyID) require.NoError(t, err) assert.NotEmpty(t, pubKey) assert.Contains(t, string(pubKey), "-----BEGIN PGP PUBLIC KEY BLOCK-----") @@ -112,7 +147,7 @@ func TestGPGGetKeyFingerprint(t *testing.T) { keyID, gpgHome := testGPGEnv(t) t.Setenv("GNUPGHOME", gpgHome) - fingerprint, err := gpgGetKeyFingerprint(keyID) + fingerprint, err := gpgGetKeyFingerprint(context.Background(), keyID) require.NoError(t, err) assert.NotEmpty(t, fingerprint) // The fingerprint should be 40 hex chars @@ -146,12 +181,12 @@ func TestGPGOptionLikeKeyIDIsNotAnOption(t *testing.T) { _, gpgHome := testGPGEnv(t) t.Setenv("GNUPGHOME", gpgHome) - pubKey, err := gpgExportPublicKey(GPGKeyID("--version")) + pubKey, err := gpgExportPublicKey(context.Background(), GPGKeyID("--version")) require.Error(t, err) require.ErrorIs(t, err, errGPGKeyNotFound) assert.NotContains(t, string(pubKey), "gpg (GnuPG)") - fpr, err := gpgGetKeyFingerprint(GPGKeyID("--version")) + fpr, err := gpgGetKeyFingerprint(context.Background(), GPGKeyID("--version")) require.Error(t, err) assert.NotContains(t, string(fpr), "gpg (GnuPG)") } @@ -162,7 +197,7 @@ func TestGPGSignInvalidKey(t *testing.T) { t.Setenv("GNUPGHOME", gpgHome) data := []byte("test data") - _, err := gpgSign(data, GPGKeyID("NONEXISTENT_KEY_ID_12345")) + _, err := gpgSign(context.Background(), data, GPGKeyID("NONEXISTENT_KEY_ID_12345")) assert.Error(t, err) } @@ -185,11 +220,11 @@ func TestBuilderWithSigning(t *testing.T) { // Build the manifest var buf bytes.Buffer - err = b.Build(&buf) + err = b.Build(context.Background(), &buf) require.NoError(t, err) // Parse the manifest and verify signature fields are populated - manifest, err := NewManifestFromReader(&buf) + manifest, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) require.NotNil(t, manifest.pbOuter) @@ -238,7 +273,7 @@ func TestScannerWithSigning(t *testing.T) { require.NoError(t, s.ToManifest(context.Background(), &buf, nil)) // Parse and verify - manifest, err := NewManifestFromReader(&buf) + manifest, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) assert.NotEmpty(t, manifest.pbOuter.GetSignature()) @@ -251,14 +286,14 @@ func TestGPGVerify(t *testing.T) { t.Setenv("GNUPGHOME", gpgHome) data := []byte("test data to sign and verify") - sig, err := gpgSign(data, keyID) + sig, err := gpgSign(context.Background(), data, keyID) require.NoError(t, err) - pubKey, err := gpgExportPublicKey(keyID) + pubKey, err := gpgExportPublicKey(context.Background(), keyID) require.NoError(t, err) // Verify the signature - err = gpgVerify(data, sig, pubKey) + err = gpgVerify(context.Background(), data, sig, pubKey) require.NoError(t, err) } @@ -267,15 +302,15 @@ func TestGPGVerifyInvalidSignature(t *testing.T) { t.Setenv("GNUPGHOME", gpgHome) data := []byte("test data to sign") - sig, err := gpgSign(data, keyID) + sig, err := gpgSign(context.Background(), data, keyID) require.NoError(t, err) - pubKey, err := gpgExportPublicKey(keyID) + pubKey, err := gpgExportPublicKey(context.Background(), keyID) require.NoError(t, err) // Try to verify with different data - should fail wrongData := []byte("different data") - err = gpgVerify(wrongData, sig, pubKey) + err = gpgVerify(context.Background(), wrongData, sig, pubKey) assert.Error(t, err) } @@ -284,12 +319,12 @@ func TestGPGVerifyBadPublicKey(t *testing.T) { t.Setenv("GNUPGHOME", gpgHome) data := []byte("test data") - sig, err := gpgSign(data, keyID) + sig, err := gpgSign(context.Background(), data, keyID) require.NoError(t, err) // Try to verify with invalid public key - should fail badPubKey := []byte("not a valid public key") - err = gpgVerify(data, sig, badPubKey) + err = gpgVerify(context.Background(), data, sig, badPubKey) assert.Error(t, err) } @@ -312,11 +347,11 @@ func TestManifestSignatureVerification(t *testing.T) { // Build the manifest var buf bytes.Buffer - err = b.Build(&buf) + err = b.Build(context.Background(), &buf) require.NoError(t, err) // Parse the manifest - signature should be verified during load - manifest, err := NewManifestFromReader(&buf) + manifest, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) require.NotNil(t, manifest) @@ -341,7 +376,7 @@ func TestManifestTamperedSignatureFails(t *testing.T) { var buf bytes.Buffer - err = b.Build(&buf) + err = b.Build(context.Background(), &buf) require.NoError(t, err) // Tamper with the signature by replacing some bytes @@ -356,7 +391,7 @@ func TestManifestTamperedSignatureFails(t *testing.T) { } // Try to load the tampered manifest - should fail - _, err = NewManifestFromReader(bytes.NewReader(data)) + _, err = NewManifestFromReader(context.Background(), bytes.NewReader(data)) assert.Error(t, err) } @@ -375,11 +410,11 @@ func TestBuilderWithoutSigning(t *testing.T) { // Build the manifest var buf bytes.Buffer - err = b.Build(&buf) + err = b.Build(context.Background(), &buf) require.NoError(t, err) // Parse the manifest and verify signature fields are empty - manifest, err := NewManifestFromReader(&buf) + manifest, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) require.NotNil(t, manifest.pbOuter) diff --git a/mfer/gpg_timeout_unix_test.go b/mfer/gpg_timeout_unix_test.go new file mode 100644 index 0000000..d79e6ed --- /dev/null +++ b/mfer/gpg_timeout_unix_test.go @@ -0,0 +1,109 @@ +//go:build unix + +// The deadline tests need an executable stand-in for gpg first on PATH, +// which is written as a POSIX shell script, and they exercise the +// process-group kill that only exists on unix. There is no equivalent +// coverage on other platforms. + +//nolint:testpackage // white-box tests exercise unexported internals +package mfer + +import ( + "context" + "os" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + // gpgTestDeadline is the deadline the deadline tests give gpg. It is + // short so the suite stays fast; gpgTimeout itself is far too long to + // wait out in a test, and passing an earlier deadline is exactly the + // caller-propagation path under test. + gpgTestDeadline = 200 * time.Millisecond + + // fakeGPGSleepSeconds is how long the stand-in gpg blocks. It must be + // long enough that only cancellation can end it -- if the deadline is + // not enforced, the test hangs until `go test` times out, which is the + // loud failure we want rather than a silently passing race. + fakeGPGSleepSeconds = 30 + + // fakeGPGPerms makes the stand-in script executable. + fakeGPGPerms os.FileMode = 0o700 +) + +// fakeSlowGPGOnPath puts a stand-in for gpg first on PATH that blocks +// until it is killed, so a deadline is the only thing that can end the +// invocation. +// +// The stand-in is a shell script that spawns a child, which means the +// child inherits the write end of the captured stdout and stderr pipes. +// That mirrors a real gpg delegating to a helper process, and it is the +// case that hangs Cmd.Wait if only the direct child is killed. +func fakeSlowGPGOnPath(t *testing.T) { + t.Helper() + + dir := t.TempDir() + script := "#!/bin/sh\nsleep " + strconv.Itoa(fakeGPGSleepSeconds) + "\n" + + // fakeGPGPerms is 0700 rather than 0600 because the stand-in has to + // be executable to be run at all; it lives in a test-controlled + // t.TempDir(). + require.NoError(t, + os.WriteFile(filepath.Join(dir, "gpg"), []byte(script), fakeGPGPerms)) + + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +// TestGPGDeadlineIsEnforced pins that a gpg invocation which outlives its +// deadline returns a timeout error naming the operation, rather than +// hanging or surfacing a bare context error. +// +//nolint:paralleltest // fakeSlowGPGOnPath mutates the process-global PATH +func TestGPGDeadlineIsEnforced(t *testing.T) { + fakeSlowGPGOnPath(t) + + ctx, cancel := context.WithTimeout(context.Background(), gpgTestDeadline) + defer cancel() + + started := time.Now() + sig, err := gpgSign(ctx, []byte("data to sign"), GPGKeyID("KEYID")) + elapsed := time.Since(started) + + require.Error(t, err) + require.ErrorIs(t, err, errGPGTimeout) + assert.Empty(t, sig) + + // The error must say which invocation stalled. "context deadline + // exceeded" alone tells an operator nothing. + assert.Contains(t, err.Error(), gpgOpSign) + + // Returning at all is the headline, but it has to return promptly: + // if the stand-in's child kept the output pipes open, Cmd.Wait would + // sit there until gpgWaitDelay elapsed. + assert.Less(t, elapsed, gpgWaitDelay, + "Run should return on cancellation, not wait out gpgWaitDelay") +} + +// TestGPGCancellationPropagates pins that a caller's cancellation reaches +// gpg and is reported as a cancellation rather than as a timeout. +// +//nolint:paralleltest // fakeSlowGPGOnPath mutates the process-global PATH +func TestGPGCancellationPropagates(t *testing.T) { + fakeSlowGPGOnPath(t) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := gpgSign(ctx, []byte("data to sign"), GPGKeyID("KEYID")) + + require.Error(t, err) + require.ErrorIs(t, err, context.Canceled) + require.NotErrorIs(t, err, errGPGTimeout) + assert.Contains(t, err.Error(), gpgOpSign) +} diff --git a/mfer/scanner.go b/mfer/scanner.go index 8d83051..ba131c3 100644 --- a/mfer/scanner.go +++ b/mfer/scanner.go @@ -282,9 +282,9 @@ func (s *Scanner) ToManifest( }) } - // Build and write manifest - //nolint:contextcheck // Build's GPG signing exec is not cancellable by design - return builder.Build(w) + // Build and write manifest. Build takes ctx because signing shells + // out to gpg, and those invocations are cancellable. + return builder.Build(ctx, w) } // configureBuilder constructs a manifest builder configured from the diff --git a/mfer/serialize.go b/mfer/serialize.go index 487d49b..69fbe95 100644 --- a/mfer/serialize.go +++ b/mfer/serialize.go @@ -2,6 +2,7 @@ package mfer import ( "bytes" + "context" "crypto/sha256" "errors" "fmt" @@ -50,13 +51,13 @@ func newTimestampFromTime(t time.Time) *Timestamp { } } -func (m *manifest) generate() error { +func (m *manifest) generate(ctx context.Context) error { if m.pbInner == nil { return errInnerNotSet } if m.pbOuter == nil { - e := m.generateOuter() + e := m.generateOuter(ctx) if e != nil { return e } @@ -77,7 +78,7 @@ func (m *manifest) generate() error { return nil } -func (m *manifest) generateOuter() error { +func (m *manifest) generateOuter(ctx context.Context) error { if m.pbInner == nil { return errInternal } @@ -135,7 +136,7 @@ func (m *manifest) generateOuter() error { // Sign the manifest if signing options are provided if m.signingOptions != nil && m.signingOptions.KeyID != "" { - return m.signOuter() + return m.signOuter(ctx) } return nil @@ -143,27 +144,27 @@ func (m *manifest) generateOuter() error { // signOuter signs the outer message with the configured GPG key and // embeds the signature, signer fingerprint, and public key. -func (m *manifest) signOuter() error { +func (m *manifest) signOuter(ctx context.Context) error { sigString, err := m.signatureString() if err != nil { return fmt.Errorf("failed to generate signature string: %w", err) } - sig, err := gpgSign([]byte(sigString), m.signingOptions.KeyID) + sig, err := gpgSign(ctx, []byte(sigString), m.signingOptions.KeyID) if err != nil { return fmt.Errorf("failed to sign manifest: %w", err) } m.pbOuter.Signature = sig - fingerprint, err := gpgGetKeyFingerprint(m.signingOptions.KeyID) + fingerprint, err := gpgGetKeyFingerprint(ctx, m.signingOptions.KeyID) if err != nil { return fmt.Errorf("failed to get key fingerprint: %w", err) } m.pbOuter.Signer = fingerprint - pubKey, err := gpgExportPublicKey(m.signingOptions.KeyID) + pubKey, err := gpgExportPublicKey(ctx, m.signingOptions.KeyID) if err != nil { return fmt.Errorf("failed to export public key: %w", err) }