From 1ec0c24149dc104b0a75439fc8508a401fbba4d5 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 13 Oct 2026 19:08:53 +0000 Subject: [PATCH] Refuse an empty --require-signature or --sign-key (closes #205) check, fetch, gen and freshen took --require-signature or --sign-key given an empty value, on the command line or through MFER_REQUIRE_SIGNATURE or MFER_SIGN_KEY, as the flag not given, so an unset shell variable skipped the signature check or the signing. Each now asks whether the flag was given, as gen's --output does. An empty fingerprint is refused as one of the wrong length, by fetch before it requests anything; an empty --sign-key is refused, naming the flag, before any file is read. Model: opus-5-5 --- internal/cli/check.go | 8 +-- internal/cli/fetch.go | 9 ++- internal/cli/freshen.go | 9 ++- internal/cli/gen.go | 12 +++- internal/cli/signing_test.go | 106 +++++++++++++++++++++++++++++++++++ 5 files changed, 135 insertions(+), 9 deletions(-) diff --git a/internal/cli/check.go b/internal/cli/check.go index 8e6b34c..662a6d6 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -329,10 +329,10 @@ func (mfa *CLIApp) checkManifestOperation( return fmt.Errorf("load manifest: %w", err) } - // Check signature requirement - requiredSigner := cmd.String(flagRequireSignature) - if requiredSigner != "" { - err = verifyRequiredSigner(chk, requiredSigner) + // Check signature requirement. An empty fingerprint is checked too, + // and refused as one of the wrong length. + if cmd.IsSet(flagRequireSignature) { + err = verifyRequiredSigner(chk, cmd.String(flagRequireSignature)) if err != nil { return err } diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 6f2573f..32f1d75 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -429,6 +429,14 @@ func (mfa *CLIApp) fetchManifestOperation( func (mfa *CLIApp) fetchManifest( ctx context.Context, cmd *cli.Command, client retryingClient, manifestURL string, ) ([]byte, []*mfer.MFFilePath, error) { + // An empty fingerprint is refused before anything is requested, with + // the message check gives for it; any other is checked against the + // manifest once it is downloaded. + requiredSigner := cmd.String(flagRequireSignature) + if cmd.IsSet(flagRequireSignature) && requiredSigner == "" { + return nil, nil, fmt.Errorf("%w, got 0", errInvalidFingerprint) + } + log.Infof("fetching manifest from %s", manifestURL) // Read the whole manifest before parsing it, so that a connection @@ -460,7 +468,6 @@ func (mfa *CLIApp) fetchManifest( return nil, nil, fmt.Errorf("parse manifest: %w", err) } - requiredSigner := cmd.String(flagRequireSignature) if requiredSigner != "" { err = verifyFetchedSigner(manifestData, requiredSigner) if err != nil { diff --git a/internal/cli/freshen.go b/internal/cli/freshen.go index cb37312..4b5e614 100644 --- a/internal/cli/freshen.go +++ b/internal/cli/freshen.go @@ -351,7 +351,7 @@ func writeFreshenedManifest( } // newFreshenBuilder constructs the manifest builder configured from CLI -// flags. +// flags. A --sign-key given with an empty value is refused. func (mfa *CLIApp) newFreshenBuilder(cmd *cli.Command) (*mfer.Builder, error) { builder := mfer.NewBuilder() if cmd.Bool("include-timestamps") { @@ -359,7 +359,12 @@ func (mfa *CLIApp) newFreshenBuilder(cmd *cli.Command) (*mfer.Builder, error) { } // Set up signing options if sign-key is provided - if signKey := cmd.String("sign-key"); signKey != "" { + if cmd.IsSet("sign-key") { + signKey := cmd.String("sign-key") + if signKey == "" { + return nil, errEmptySignKey + } + signing, err := mfa.signingOptions(signKey) if err != nil { return nil, err diff --git a/internal/cli/gen.go b/internal/cli/gen.go index 4a24882..0dda37a 100644 --- a/internal/cli/gen.go +++ b/internal/cli/gen.go @@ -29,6 +29,8 @@ var ( "already exists (use --force to overwrite)") // errEmptyOutput indicates --output given with an empty value. errEmptyOutput = errors.New("--output must not be empty") + // errEmptySignKey indicates --sign-key given with an empty value. + errEmptySignKey = errors.New("--sign-key must not be empty") ) // reportEnumProgress renders enumeration progress until the channel @@ -121,7 +123,8 @@ func (mfa *CLIApp) outputPath(cmd *cli.Command) (string, error) { } // buildScannerOptions constructs scanner options from the CLI flags and -// the path the manifest is written to. +// the path the manifest is written to. A --sign-key given with an empty +// value is refused. func (mfa *CLIApp) buildScannerOptions( cmd *cli.Command, output string, ) (*mfer.ScannerOptions, error) { @@ -144,7 +147,12 @@ func (mfa *CLIApp) buildScannerOptions( } // Set up signing options if sign-key is provided - if signKey := cmd.String("sign-key"); signKey != "" { + if cmd.IsSet("sign-key") { + signKey := cmd.String("sign-key") + if signKey == "" { + return nil, errEmptySignKey + } + signing, err := mfa.signingOptions(signKey) if err != nil { return nil, err diff --git a/internal/cli/signing_test.go b/internal/cli/signing_test.go index 77062ba..10a2fd4 100644 --- a/internal/cli/signing_test.go +++ b/internal/cli/signing_test.go @@ -4,8 +4,12 @@ package cli import ( "bufio" "io" + "net/http" + "net/http/httptest" "path/filepath" + "slices" "strings" + "sync/atomic" "testing" "time" @@ -207,3 +211,105 @@ func TestGenAsksForPassphraseOnTerminal(t *testing.T) { }, fs) require.Equal(t, 0, runCLI(check), testStderr(t, check)) } + +// assertEmptySignKeyRefused runs gen and freshen, each with flags, on a +// directory holding a file and an unsigned manifest. Each must fail, +// naming --sign-key, and leave the manifest as it was. +func assertEmptySignKeyRefused(t *testing.T, flags ...string) { + t.Helper() + + manifest := manifestOf(t, map[string][]byte{testFileTxt: []byte("listed")}) + + for command, rest := range map[string][]string{ + cmdGenerate: {"-q", "-f", "-o", testMF, testDir}, + cmdFreshen: {"-q", testMF}, + } { + fs := afero.NewMemMapFs() + require.NoError(t, fs.MkdirAll(testDir, 0o755)) + writeTestFile(t, fs, testFile1, "hello") + require.NoError(t, afero.WriteFile(fs, testMF, manifest, 0o644)) + + opts := testOpts(slices.Concat([]string{testApp, command}, flags, rest), fs) + assert.Equal(t, 1, runCLI(opts), command) + assert.Contains(t, testStderr(t, opts), errEmptySignKey.Error(), command) + + content, err := afero.ReadFile(fs, testMF) + require.NoError(t, err) + assert.Equal(t, manifest, content, command) + } +} + +// TestGenAndFreshenRefuseEmptySignKey runs gen and freshen with --sign-key +// given an empty value, as an unset shell variable gives it. +func TestGenAndFreshenRefuseEmptySignKey(t *testing.T) { + t.Parallel() + + assertEmptySignKeyRefused(t, testFlagSignKey, "") +} + +// TestGenAndFreshenRefuseEmptySignKeyFromEnvironment runs gen and freshen +// with MFER_SIGN_KEY set to an empty string. +func TestGenAndFreshenRefuseEmptySignKeyFromEnvironment(t *testing.T) { + t.Setenv("MFER_SIGN_KEY", "") + + assertEmptySignKeyRefused(t) +} + +// assertEmptyFingerprintRefused runs check on an unsigned manifest, and +// fetch of that manifest from a server, each with flags. Each must refuse +// the empty fingerprint as malformed, and fetch must request nothing. +func assertEmptyFingerprintRefused(t *testing.T, flags ...string) { + t.Helper() + + const want = "invalid fingerprint: must be exactly 40 hex characters, got 0" + + files := map[string][]byte{testFileTxt: []byte("listed")} + manifest := manifestOf(t, files) + + fs := afero.NewMemMapFs() + require.NoError(t, fs.MkdirAll(testDir, 0o755)) + writeTestFile(t, fs, filepath.Join(testDir, testFileTxt), "listed") + require.NoError(t, afero.WriteFile(fs, testMF, manifest, 0o644)) + + opts := testOpts(slices.Concat( + []string{testApp, cmdCheck, "-q"}, flags, []string{testMF}, + ), fs) + assert.Equal(t, 1, runCLI(opts), cmdCheck) + assert.Contains(t, testStderr(t, opts), want, cmdCheck) + + var requests atomic.Int32 + + tree := fetchTestHandler(manifest, files) + + server := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + tree.ServeHTTP(w, r) + })) + defer server.Close() + + opts = testOpts(slices.Concat( + []string{testApp, cmdFetch, "-q", "--" + flagDest, t.TempDir()}, + flags, []string{server.URL}, + ), afero.NewOsFs()) + assert.Equal(t, 1, runCLI(opts), cmdFetch) + assert.Contains(t, testStderr(t, opts), want, cmdFetch) + assert.Zero(t, requests.Load(), "fetch made a request") +} + +// TestCheckAndFetchRefuseEmptyRequireSignature runs check and fetch with +// --require-signature given an empty value, as an unset shell variable +// gives it. +func TestCheckAndFetchRefuseEmptyRequireSignature(t *testing.T) { + t.Parallel() + + assertEmptyFingerprintRefused(t, "--"+flagRequireSignature, "") +} + +// TestCheckAndFetchRefuseEmptyRequireSignatureFromEnvironment runs check +// and fetch with MFER_REQUIRE_SIGNATURE set to an empty string. +func TestCheckAndFetchRefuseEmptyRequireSignatureFromEnvironment(t *testing.T) { + t.Setenv("MFER_REQUIRE_SIGNATURE", "") + + assertEmptyFingerprintRefused(t) +}