From c94327ab7fa57b8393f1b7b8f46c98fb52885a30 Mon Sep 17 00:00:00 2001 From: sneak Date: Sat, 3 Oct 2026 15:37:23 +0000 Subject: [PATCH] Enforce real timeouts on gpg subprocess calls (closes #62) Every gpg run now has a one-minute deadline (gpgTimeout) on top of its caller's context and is killed when either ends. A timeout is reported as "gpg timed out" under the failing operation instead of "signal: killed". Only gpg itself is killed; WaitDelay (one second) stops the run from waiting on a process gpg left behind that still holds its output, such as a wrapper script that does not exec the real gpg. Builder.Build and Checker.ExtractEmbeddedSigningKeyFP take a context, so ToManifest's context now reaches signing and the contextcheck suppression calling signing non-cancellable is gone. Manifest loading takes no context, so its signature check is bounded by the timeout alone. Model: opus-5-5 --- TODO.md | 7 +- internal/cli/check.go | 9 ++- internal/cli/errmsg_test.go | 12 ++-- internal/cli/freshen.go | 7 +- mfer/builder.go | 10 +-- mfer/builder_test.go | 17 ++--- mfer/checker.go | 4 +- mfer/checker_test.go | 2 +- mfer/deserialize.go | 3 + mfer/deserialize_path_test.go | 3 +- mfer/errmsg_test.go | 6 +- mfer/gpg.go | 63 +++++++++++++----- mfer/gpg_test.go | 120 ++++++++++++++++++++++++++++------ mfer/scanner.go | 3 +- mfer/serialize.go | 17 ++--- 15 files changed, 207 insertions(+), 76 deletions(-) diff --git a/TODO.md b/TODO.md index c4e97e2..df188c2 100644 --- a/TODO.md +++ b/TODO.md @@ -24,6 +24,10 @@ only thing left of the `chore/align-repo-policies` branch is the list below. # Completed Steps +- 2026-10-03: every gpg run is killed after one minute or when its caller's + context ends, and a timeout reads as "gpg timed out" under the failing + operation; `Builder.Build` and `Checker.ExtractEmbeddedSigningKeyFP` take a + context, which reaches gpg (#62) - 2026-10-03: pinned the CLI error messages by driving the functions that emit them in `internal/cli/errmsg_test.go`, and made the freshen mtime-presence test distinguish an absent mtime from the epoch (#87) @@ -118,8 +122,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..4375d32 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) @@ -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/errmsg_test.go b/internal/cli/errmsg_test.go index 72b4b48..5683120 100644 --- a/internal/cli/errmsg_test.go +++ b/internal/cli/errmsg_test.go @@ -83,7 +83,8 @@ func TestVerifyRequiredSignerMessages(t *testing.T) { t.Run("invalid fingerprint length", func(t *testing.T) { t.Parallel() - err := verifyRequiredSigner(unsignedChecker(t), "12345678") + err := verifyRequiredSigner(context.Background(), + unsignedChecker(t), "12345678") require.ErrorIs(t, err, errInvalidFingerprint) assert.EqualError(t, err, "invalid fingerprint: must be exactly 40 hex characters, got 8") @@ -92,7 +93,8 @@ func TestVerifyRequiredSignerMessages(t *testing.T) { t.Run("manifest not signed", func(t *testing.T) { t.Parallel() - err := verifyRequiredSigner(unsignedChecker(t), msgFpA) + err := verifyRequiredSigner(context.Background(), + unsignedChecker(t), msgFpA) require.ErrorIs(t, err, errManifestNotSigned) assert.EqualError(t, err, "manifest is not signed, but signature from "+msgFpA+" is required") @@ -109,10 +111,10 @@ func TestVerifyRequiredSignerMessages(t *testing.T) { func TestSignerMismatchMessage(t *testing.T) { chk := signedChecker(t) - embeddedFP, err := chk.ExtractEmbeddedSigningKeyFP() + embeddedFP, err := chk.ExtractEmbeddedSigningKeyFP(context.Background()) require.NoError(t, err) - err = verifyRequiredSigner(chk, msgFpB) + err = verifyRequiredSigner(context.Background(), chk, msgFpB) require.ErrorIs(t, err, errSignerMismatch) assert.EqualError(t, err, "embedded signing key fingerprint "+embeddedFP+ @@ -160,7 +162,7 @@ func signedChecker(t *testing.T) *mfer.Checker { var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) + require.NoError(t, b.Build(context.Background(), &buf)) fs := afero.NewMemMapFs() require.NoError(t, afero.WriteFile(fs, "/index.mf", buf.Bytes(), 0o644)) diff --git a/internal/cli/freshen.go b/internal/cli/freshen.go index bc0ee20..0640b15 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,7 @@ 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 +314,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 { @@ -530,7 +531,7 @@ 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/mfer/builder.go b/mfer/builder.go index b93f79e..c2d7856 100644 --- a/mfer/builder.go +++ b/mfer/builder.go @@ -3,6 +3,7 @@ package mfer import ( + "context" "crypto/sha256" "errors" "fmt" @@ -281,8 +282,9 @@ func (b *Builder) SetSigningOptions(opts *SigningOptions) { b.signingOptions = opts } -// Build finalizes the manifest and writes it to the writer. -func (b *Builder) Build(w io.Writer) error { +// Build finalizes the manifest and writes it to the writer. ctx bounds the +// gpg runs that sign the manifest when signing options are set. +func (b *Builder) Build(ctx context.Context, w io.Writer) error { b.mu.Lock() defer b.mu.Unlock() @@ -308,13 +310,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..9408a93 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,7 +326,7 @@ 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) require.NoError(t, err) @@ -383,7 +384,7 @@ 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) require.NoError(t, err) @@ -397,7 +398,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,7 +417,7 @@ 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) require.NoError(t, err) @@ -438,7 +439,7 @@ 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) require.NoError(t, err) @@ -464,7 +465,7 @@ func TestBuilderDeterministicFileOrder(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) require.NoError(t, err) diff --git a/mfer/checker.go b/mfer/checker.go index a47d4d1..1a97028 100644 --- a/mfer/checker.go +++ b/mfer/checker.go @@ -164,12 +164,12 @@ 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) { +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..febc428 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)) } diff --git a/mfer/deserialize.go b/mfer/deserialize.go index b4c2b94..b204311 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -2,6 +2,7 @@ package mfer import ( "bytes" + "context" "crypto/sha256" "errors" "fmt" @@ -92,7 +93,9 @@ func (m *manifest) verifyOuterIntegrity() error { ) } + // Loading a manifest takes no context; gpgTimeout still bounds gpg. err = gpgVerify( + context.Background(), []byte(sigString), m.pbOuter.GetSignature(), m.pbOuter.GetSigningPubKey(), diff --git a/mfer/deserialize_path_test.go b/mfer/deserialize_path_test.go index b83f9fb..f2abacc 100644 --- a/mfer/deserialize_path_test.go +++ b/mfer/deserialize_path_test.go @@ -3,6 +3,7 @@ package mfer import ( "bytes" + "context" "crypto/sha256" "fmt" "testing" @@ -122,7 +123,7 @@ func TestDeserializeValidManifestRoundTrips(t *testing.T) { require.NoError(t, b.AddFileWithHash("dir/file.txt", 123, ModTime{}, hash)) var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) + require.NoError(t, b.Build(context.Background(), &buf)) m, err := NewManifestFromReader(bytes.NewReader(buf.Bytes())) require.NoError(t, err) 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..b3bb24d 100644 --- a/mfer/gpg.go +++ b/mfer/gpg.go @@ -10,9 +10,21 @@ import ( "os/exec" "path/filepath" "strings" + "time" ) const ( + // gpgTimeout bounds every gpg run, which can otherwise wait forever on + // a passphrase prompt or a stalled gpg-agent. A minute leaves a person + // time to type a passphrase or touch a smartcard. + gpgTimeout = time.Minute + + // gpgWaitDelay is how long a gpg run keeps waiting for gpg's stdout + // and stderr to close once gpg has been killed or has exited. Reading + // what gpg itself wrote takes far less; only a process gpg left behind + // holds them open longer. + gpgWaitDelay = time.Second + // privateDirPerms is the permission mode for temporary GPG home // directories. privateDirPerms os.FileMode = 0o700 @@ -66,8 +78,20 @@ 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) { +// optional stdin, returning captured stdout and stderr. gpg is killed when +// ctx ends or gpgTimeout passes, whichever comes first. +func runGPG( + ctx context.Context, stdin io.Reader, args ...string, +) (*bytes.Buffer, *bytes.Buffer, error) { + // exec.CommandContext kills only gpg itself. A gpg-agent that gpg + // starts runs detached and holds none of gpg's output, but another + // process gpg leaves behind (a wrapper script that runs the real gpg + // without exec, for example) can keep gpg's stdout or stderr open, and + // Run would wait for it to exit. WaitDelay stops that wait + // gpgWaitDelay after the kill; that process is left running. + ctx, cancel := context.WithTimeout(ctx, gpgTimeout) + defer cancel() + fullArgs := append([]string{"--batch", "--no-tty"}, args...) // G204: the executable name is a compile-time constant. The arguments @@ -76,7 +100,8 @@ func runGPG(stdin io.Reader, args ...string) (*bytes.Buffer, *bytes.Buffer, erro // 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...) + ctx, "gpg", fullArgs...) + cmd.WaitDelay = gpgWaitDelay cmd.Stdin = stdin var stdout, stderr bytes.Buffer @@ -85,6 +110,14 @@ func runGPG(stdin io.Reader, args ...string) (*bytes.Buffer, *bytes.Buffer, erro cmd.Stderr = &stderr err := cmd.Run() + if err != nil && ctx.Err() != nil { + // gpg was killed because ctx ended, which Run reports only as + // "signal: killed"; return the reason instead. + err = ctx.Err() + if errors.Is(err, context.DeadlineExceeded) { + err = fmt.Errorf("gpg timed out: %w", err) + } + } return &stdout, &stderr, err } @@ -105,8 +138,8 @@ 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, bytes.NewReader(data), "--detach-sign", gpgOptArmor, "--local-user", string(keyID), @@ -120,8 +153,8 @@ 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, nil, gpgArgs([]string{"--export", gpgOptArmor}, string(keyID))..., ) if err != nil { @@ -136,8 +169,8 @@ 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, nil, gpgArgs([]string{"--with-colons", "--fingerprint"}, string(keyID))..., ) if err != nil { @@ -157,7 +190,7 @@ 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 +214,7 @@ func gpgExtractPubKeyFingerprint(pubKey []byte) (string, error) { } // Import the public key into the temporary keyring - _, importStderr, err := runGPG(nil, + _, importStderr, err := runGPG(ctx, nil, gpgArgs([]string{gpgOptHomedir, tmpDir, "--import"}, pubKeyFile)..., ) if err != nil { @@ -191,7 +224,7 @@ func gpgExtractPubKeyFingerprint(pubKey []byte) (string, error) { } // List keys to get fingerprint - listStdout, listStderr, err := runGPG(nil, + listStdout, listStderr, err := runGPG(ctx, nil, "--homedir", tmpDir, "--with-colons", "--fingerprint", @@ -212,7 +245,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 +285,7 @@ func gpgVerify(data, signature, pubKey []byte) error { } // Import the public key into the temporary keyring - _, importStderr, err := runGPG(nil, + _, importStderr, err := runGPG(ctx, nil, gpgArgs([]string{gpgOptHomedir, tmpDir, "--import"}, pubKeyFile)..., ) if err != nil { @@ -262,7 +295,7 @@ func gpgVerify(data, signature, pubKey []byte) error { } // Verify the signature - _, verifyStderr, err := runGPG(nil, + _, verifyStderr, err := runGPG(ctx, nil, gpgArgs([]string{gpgOptHomedir, tmpDir, gpgOptVerify}, sigFile, dataFile)..., ) diff --git a/mfer/gpg_test.go b/mfer/gpg_test.go index 43aa9d6..12bc342 100644 --- a/mfer/gpg_test.go +++ b/mfer/gpg_test.go @@ -4,11 +4,14 @@ package mfer import ( "bytes" "context" + "io" "os" "os/exec" "path/filepath" + "strconv" "strings" "testing" + "time" "github.com/spf13/afero" "github.com/stretchr/testify/assert" @@ -43,8 +46,11 @@ Expire-Date: 0 paramsFile := filepath.Join(gpgHome, "key-params") require.NoError(t, os.WriteFile(paramsFile, []byte(keyParams), 0o600)) + ctx, cancel := context.WithTimeout(context.Background(), gpgTimeout) + defer cancel() + //nolint:gosec // paramsFile is a test-controlled path inside t.TempDir() - cmd := exec.CommandContext(context.Background(), "gpg", + cmd := exec.CommandContext(ctx, "gpg", "--batch", "--gen-key", paramsFile) cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) @@ -55,7 +61,7 @@ Expire-Date: 0 } // Get the key fingerprint - cmd = exec.CommandContext(context.Background(), "gpg", + cmd = exec.CommandContext(ctx, "gpg", "--list-keys", "--with-colons", "test@mfer.test") cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) @@ -90,7 +96,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 +107,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 +118,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 +152,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 +168,8 @@ 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,7 +192,7 @@ 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 @@ -251,14 +258,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 +274,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 +291,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,7 +319,7 @@ 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 @@ -341,7 +348,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 @@ -375,7 +382,7 @@ 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 @@ -390,3 +397,76 @@ func TestBuilderWithoutSigning(t *testing.T) { assert.Empty(t, manifest.pbOuter.GetSigningPubKey(), "signing public key should be empty when not signing") } + +// fakeGPGPath writes script as an executable named gpg into a temporary +// directory and returns a PATH value with that directory first. +func fakeGPGPath(t *testing.T, script string) string { + t.Helper() + + binDir := t.TempDir() + //nolint:gosec // G306: the fake gpg has to be executable + require.NoError(t, os.WriteFile(filepath.Join(binDir, "gpg"), + []byte(script), 0o700)) + + return binDir + string(os.PathListSeparator) + os.Getenv("PATH") +} + +// TestGPGTimeoutKillsGPG puts a fake gpg that never finishes first on +// PATH and checks that a run past its deadline is killed and reported as +// a timeout of the named operation, instead of hanging. +func TestGPGTimeoutKillsGPG(t *testing.T) { + t.Setenv("PATH", fakeGPGPath(t, "#!/bin/sh\nexec sleep 10\n")) + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + _, err := gpgSign(ctx, []byte("data"), GPGKeyID("any")) + require.ErrorIs(t, err, context.DeadlineExceeded) + assert.Contains(t, err.Error(), "gpg sign failed: gpg timed out") +} + +// TestGPGTimeoutWhenChildHoldsOutput uses a fake gpg that runs sleep as a +// child instead of exec-ing it, the way a wrapper script around the real +// gpg might. Killing the fake gpg leaves sleep holding its stdout and +// stderr open; the call must still return shortly after the deadline +// instead of waiting for sleep to exit. The fake gpg writes the process ID +// of sleep to a file so that the test can kill it before returning. +func TestGPGTimeoutWhenChildHoldsOutput(t *testing.T) { + pidFile := filepath.Join(t.TempDir(), "sleep.pid") + t.Setenv("PATH", fakeGPGPath(t, + "#!/bin/sh\nsleep 3 &\necho $! >'"+pidFile+"'\nwait\n")) + t.Cleanup(func() { + pid, err := os.ReadFile(pidFile) //nolint:gosec // G304: path inside t.TempDir() + require.NoError(t, err) + + n, err := strconv.Atoi(strings.TrimSpace(string(pid))) + require.NoError(t, err) + + sleep, err := os.FindProcess(n) + require.NoError(t, err) + require.NoError(t, sleep.Kill()) + }) + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + start := time.Now() + _, err := gpgSign(ctx, []byte("data"), GPGKeyID("any")) + require.ErrorIs(t, err, context.DeadlineExceeded) + assert.Less(t, time.Since(start), 3*time.Second, + "the call waited for the child holding gpg's output to exit") +} + +// TestBuildPassesContextToSigning checks that a caller can cancel the gpg +// runs that sign a manifest through the context given to Build. +func TestBuildPassesContextToSigning(t *testing.T) { + t.Parallel() + + b := NewBuilder() + b.SetSigningOptions(&SigningOptions{KeyID: "any"}) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + require.ErrorIs(t, b.Build(ctx, io.Discard), context.Canceled) +} diff --git a/mfer/scanner.go b/mfer/scanner.go index 8d83051..01b2770 100644 --- a/mfer/scanner.go +++ b/mfer/scanner.go @@ -283,8 +283,7 @@ func (s *Scanner) ToManifest( } // Build and write manifest - //nolint:contextcheck // Build's GPG signing exec is not cancellable by design - return builder.Build(w) + 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) }