Refuse to freshen a signed manifest without --sign-key (closes #191)

freshen built the new manifest from its flags alone, so a signed
manifest freshened without --sign-key came out unsigned while the log
said its signature verified. freshen now stops after loading such a
manifest, before scanning, with an error naming the signer's
fingerprint and --sign-key, and leaves the manifest as it was. With
--sign-key it rewrites and signs as before.

The library gains Signer() on the loaded manifest: the signing key's
fingerprint, or nil when the manifest is unsigned, as Checker.Signer
gives it.

Model: opus-5-5

README.md

@@ -300,6 +300,9 @@ are now tracked only in the [issues](https://git.eeqj.de/sneak/mfer/issues).
     - leaves out hidden files unless given `--include-dotfiles`, and symlinks
       unless given `--follow-symlinks`, which lists each symlink to a regular
       file under its own name with the contents of the file it points to
+    - refuses a signed manifest, leaving it as it was and naming the fingerprint
+      of the key that signed it, unless `--sign-key` or `MFER_SIGN_KEY` names a
+      key to sign the new one with
 - `mfer gen --sign-key key.asc` / `mfer freshen --sign-key key.asc`
     - signs the manifest with the OpenPGP secret key in `key.asc`, armored or
       binary, as `gpg --export-secret-keys` writes it; `MFER_SIGN_KEY` names the

internal/cli/check.go

@@ -189,8 +189,8 @@ func reportCheckProgress(progress <-chan mfer.CheckStatus, wg *sync.WaitGroup) {
 	log.ProgressDone()
 }
 
-// countCheckFailures consumes check results, counting and logging
+// countCheckFailures consumes check results, counting failures and logging
-// failures, then closes done.
+// each at error level so that --quiet still shows them, then closes done.
 func countCheckFailures(
 	results <-chan mfer.Result, failures *int64, done chan<- struct{},
 ) {
@@ -198,7 +198,7 @@ func countCheckFailures(
 		if result.Status != mfer.StatusOK {
 			*failures++
 
-			log.Infof("%s: %s (%s)", result.Status, result.Path, result.Message)
+			log.Errorf("%s: %s (%s)", result.Status, result.Path, result.Message)
 		} else {
 			log.Verbosef("%s: %s", result.Status, result.Path)
 		}
@@ -208,8 +208,8 @@ func countCheckFailures(
 }
 
 // findExtraFiles reports files present on disk but absent from the
-// manifest, and anything the search cannot read: each is a failure under
+// manifest, and anything the search cannot read: each is a failure, logged
-// --no-extra-files, otherwise a warning.
+// at error level, under --no-extra-files, otherwise a warning.
 func findExtraFiles(
 	ctx context.Context, cmd *cli.Command, chk *mfer.Checker, failures *int64,
 ) error {
@@ -221,7 +221,7 @@ func findExtraFiles(
 			if cmd.Bool("no-extra-files") {
 				*failures++
 
-				log.Infof("%s: %s (%s)", result.Status, result.Path, result.Message)
+				log.Errorf("%s: %s (%s)", result.Status, result.Path, result.Message)
 			} else {
 				log.Warnf("%s: %s (%s)", result.Status, result.Path, result.Message)
 			}

internal/cli/entry_test.go

@@ -694,6 +694,33 @@ func TestCheckCommandWithSizeMismatch(t *testing.T) {
 		"check should have failed for size mismatch")
 }
 
+// TestCheckQuietShowsFailedFiles asserts that check -q prints nothing for a
+// tree that passes and still names each file that fails: --quiet hides
+// everything except errors, and a failed file is an error.
+func TestCheckQuietShowsFailedFiles(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	require.NoError(t, fs.MkdirAll(testDir, 0o755))
+	writeTestFile(t, fs, testFile1, "hello world")
+
+	opts := testOpts([]string{testApp, cmdGenerate, "-q", "-o", testManifest, testDir}, fs)
+	require.Equal(t, 0, runCLI(opts), "generate failed: %s", testStderr(t, opts))
+
+	args := []string{testApp, cmdCheck, "-q", testFlagBase, testDir, testManifest}
+
+	opts = testOpts(args, fs)
+	require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts))
+	assert.Empty(t, testStdout(t, opts))
+	assert.Empty(t, testStderr(t, opts))
+
+	writeTestFile(t, fs, testFile1, "HELLO WORLD")
+
+	opts = testOpts(args, fs)
+	assert.Equal(t, 1, runCLI(opts))
+	assert.Contains(t, testStderr(t, opts), "HASH_MISMATCH: file1.txt")
+}
+
 func TestBannerOutput(t *testing.T) {
 	t.Parallel()
 
@@ -926,8 +953,9 @@ func TestCheckWarnsAboutUnlistedFiles(t *testing.T) {
 			assert.Equal(t, 1, exitCode, "stderr: %s", stderr)
 			assert.Contains(t, stderr, unlisted)
 
-			exitCode, _ = check("-q", testFlagNoExtra)
+			exitCode, stderr = check("-q", testFlagNoExtra)
 			assert.Equal(t, 1, exitCode)
+			assert.Contains(t, stderr, unlisted)
 		})
 	}
 }

internal/cli/freshen.go

@@ -32,6 +32,10 @@ const (
 // modification time where one is required to carry it forward unchanged.
 var errEntryMissingMtime = errors.New("manifest entry has no mtime")
 
+// errSignKeyNeeded indicates a signed manifest given to freshen with no
+// key to sign the rewritten one.
+var errSignKeyNeeded = errors.New("give --sign-key to keep it signed")
+
 // FreshenStatus contains progress information for the freshen operation.
 type FreshenStatus struct {
 	Phase        string        // "scan" or "hash"
@@ -475,9 +479,10 @@ func runFreshenHash(
 }
 
 // loadExistingEntries loads the manifest and indexes its file entries
-// by path.
+// by path. It refuses a signed manifest when no --sign-key is given, as
+// the rewritten manifest would be unsigned.
 func (mfa *CLIApp) loadExistingEntries(
-	manifestPath string,
+	cmd *cli.Command, manifestPath string,
 ) (map[string]*mfer.MFFilePath, error) {
 	log.Infof("loading manifest from %s", manifestPath)
 
@@ -490,6 +495,10 @@ func (mfa *CLIApp) loadExistingEntries(
 		return nil, fmt.Errorf("load manifest: %w", err)
 	}
 
+	if signer := manifest.Signer(); signer != nil && cmd.String("sign-key") == "" {
+		return nil, fmt.Errorf("manifest is signed by %s: %w", signer, errSignKeyNeeded)
+	}
+
 	existingFiles := manifest.Files()
 	log.Infof("manifest contains %d files", len(existingFiles))
 
@@ -520,7 +529,7 @@ func (mfa *CLIApp) freshenManifestOperation(
 		return err
 	}
 
-	existingByPath, err := mfa.loadExistingEntries(manifestPath)
+	existingByPath, err := mfa.loadExistingEntries(cmd, manifestPath)
 	if err != nil {
 		return err
 	}

internal/cli/signing_test.go

@@ -73,6 +73,56 @@ func TestGenAndFreshenSignWithKeyFile(t *testing.T) {
 	}
 }
 
+// TestFreshenSignedManifestNeedsSignKey runs freshen on a signed manifest
+// after a file is added. Without --sign-key it must fail, naming the
+// signer and --sign-key, and leave the manifest as it was; with --sign-key
+// it must rewrite the manifest, signed.
+func TestFreshenSignedManifestNeedsSignKey(t *testing.T) {
+	t.Setenv(envSignKeyPassphrase, "")
+
+	secretKey, fingerprint := testSecretKey(t, nil, nil)
+
+	fs := afero.NewOsFs()
+	keyFile := filepath.Join(t.TempDir(), "key.asc")
+	root := t.TempDir()
+	manifestPath := filepath.Join(root, defaultManifestName)
+
+	require.NoError(t, afero.WriteFile(fs, keyFile, secretKey, 0o600))
+	writeTestFile(t, fs, filepath.Join(root, testFileTxt), "hello")
+
+	opts := testOpts([]string{
+		testApp, cmdGenerate, "-q", testFlagSignKey, keyFile,
+		"-o", manifestPath, root,
+	}, fs)
+	require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+
+	before, err := afero.ReadFile(fs, manifestPath)
+	require.NoError(t, err)
+
+	writeTestFile(t, fs, filepath.Join(root, "added.txt"), "added")
+
+	opts = testOpts([]string{testApp, cmdFreshen, "-q", manifestPath}, fs)
+	assert.Equal(t, 1, runCLI(opts))
+	assert.Contains(t, testStderr(t, opts), fingerprint)
+	assert.Contains(t, testStderr(t, opts), testFlagSignKey)
+
+	after, err := afero.ReadFile(fs, manifestPath)
+	require.NoError(t, err)
+	assert.Equal(t, before, after)
+
+	opts = testOpts([]string{
+		testApp, cmdFreshen, "-q", testFlagSignKey, keyFile, manifestPath,
+	}, fs)
+	require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+
+	opts = testOpts([]string{
+		testApp, cmdCheck, "-q",
+		"--" + flagRequireSignature, fingerprint, manifestPath,
+	}, fs)
+	require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+	assert.Len(t, manifestFiles(t, fs, manifestPath), 2)
+}
+
 // TestSignWithProtectedKeyNeedsPassphrase runs gen with a protected key,
 // with MFER_SIGN_KEY_PASSPHRASE empty and no terminal to ask on. gen must
 // fail, naming the variable, and write no manifest.

mfer/manifest.go

@@ -47,6 +47,17 @@ func (m *manifest) Files() []*MFFilePath {
 	return m.pbInner.GetFiles()
 }
 
+// Signer returns the fingerprint of the key that made a loaded manifest's
+// signature, which loading the manifest checked, or nil if the manifest is
+// not signed.
+func (m *manifest) Signer() []byte {
+	if len(m.pbOuter.GetSignature()) == 0 {
+		return nil
+	}
+
+	return m.pbOuter.GetSigner()
+}
+
 // signatureString generates the canonical string used for signing/verification.
 // Format: MAGIC-UUID-MULTIHASH where UUID and multihash are hex-encoded.
 // Requires pbOuter to be set with Uuid and Sha256 fields.

mfer/openpgp_test.go

@@ -175,6 +175,25 @@ func TestBuilderWithSigning(t *testing.T) {
 		"-----BEGIN PGP PUBLIC KEY BLOCK-----")
 }
 
+// TestManifestSigner checks that Signer gives the signing key's
+// fingerprint for a signed manifest, and nil once the signature is taken
+// out, although the signer field is still there.
+func TestManifestSigner(t *testing.T) {
+	t.Parallel()
+
+	key := newTestKey(t, nil)
+	data := signedTestManifest(t, &SigningOptions{SecretKey: armoredSecretKeys(t, key)})
+
+	signed, err := NewManifestFromReader(bytes.NewReader(data))
+	require.NoError(t, err)
+	assert.Equal(t, fingerprint(key), string(signed.Signer()))
+
+	unsigned, err := NewManifestFromReader(bytes.NewReader(
+		rewriteOuter(t, data, func(outer *MFFileOuter) { outer.Signature = nil })))
+	require.NoError(t, err)
+	assert.Nil(t, unsigned.Signer())
+}
+
 func TestScannerWithSigning(t *testing.T) {
 	t.Parallel()