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

docs/FORMAT.md

@@ -75,15 +75,15 @@ After decompressing `innerMessage`, the result is a serialized `MFFile`
 
 Each file entry contains:
 
-| Field      | Number | Type                      | Description                                                   |
-| ---------- | ------ | ------------------------- | ------------------------------------------------------------- |
-| `path`     | 1      | string                    | Relative file path (see Path Rules)                           |
-| `size`     | 2      | int64                     | File size in bytes                                            |
-| `hashes`   | 3      | repeated `MFFileChecksum` | At least one hash required; readers reject an entry with none |
-| `mimeType` | 301    | string (optional)         | MIME type                                                     |
-| `mtime`    | 302    | Timestamp (optional)      | Modification time                                             |
-| `ctime`    | 303    | Timestamp (optional)      | Change time (inode metadata change)                           |
-| `mode`     | 304    | uint32                    | Permission bits (see File Mode)                               |
+| Field      | Number | Type                      | Description                         |
+| ---------- | ------ | ------------------------- | ----------------------------------- |
+| `path`     | 1      | string                    | Relative file path (see Path Rules) |
+| `size`     | 2      | int64                     | File size in bytes                  |
+| `hashes`   | 3      | repeated `MFFileChecksum` | At least one hash required          |
+| `mimeType` | 301    | string (optional)         | MIME type                           |
+| `mtime`    | 302    | Timestamp (optional)      | Modification time                   |
+| `ctime`    | 303    | Timestamp (optional)      | Change time (inode metadata change) |
+| `mode`     | 304    | uint32                    | Permission bits (see File Mode)     |
 
 ## File Mode
 
@@ -174,10 +174,6 @@ also refuses a manifest whose `signingPubKey` holds a DSA key or subkey, or any
 secret key or subkey, since checking the self-signatures of a DSA key or the
 numbers of a secret key can take hours when those numbers are very large.
 
-A signing key that has expired does not make the signature bad, because a
-manifest outlives its key. A signature past its own expiry time is not good,
-whether or not the key has expired too.
-
 ## Deterministic Serialization
 
 By default, manifests are generated deterministically:

internal/cli/entry_test.go

@@ -36,8 +36,6 @@ const (
 	testFlagNoExtra = "--no-extra-files"
 	testFlagVersion = "--version"
 	testFlagVerbose = "--verbose"
-	testFlagHelp    = "--help"
-	testCmdHelp     = "help"
 )
 
 var errSimulatedWrite = errors.New("simulated write failure")
@@ -323,7 +321,7 @@ func TestHelpCommand(t *testing.T) {
 	t.Parallel()
 
 	fs := afero.NewMemMapFs()
-	opts := testOpts([]string{testApp, testFlagHelp}, fs)
+	opts := testOpts([]string{testApp, "--help"}, fs)
 
 	exitCode := runCLI(opts)
 
@@ -1809,82 +1807,6 @@ func TestGenerateReadsPathAfterDoubleDash(t *testing.T) {
 	assert.Equal(t, testFileTxt, files[0].GetPath())
 }
 
-// TestPathNamedHelp runs gen, then check with --require-signature, on a
-// directory named help and on one named h, the two names of urfave/cli's
-// help command. gen must write the manifest into the directory, and check
-// must refuse it as unsigned.
-//
-//nolint:paralleltest // changes the process-global working directory
-func TestPathNamedHelp(t *testing.T) {
-	for _, name := range []string{testCmdHelp, "h"} {
-		t.Run(name, func(t *testing.T) {
-			chdirTemp(t)
-
-			fs := afero.NewOsFs()
-			require.NoError(t, fs.Mkdir(name, 0o755))
-			writeTestFile(t, fs, filepath.Join(name, testFileTxt), "content")
-
-			opts := testOpts([]string{testApp, cmdGenerate, "-q", name}, fs)
-			require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
-
-			files := manifestFiles(t, fs, filepath.Join(name, defaultManifestName))
-			require.Len(t, files, 1)
-
-			opts = testOpts([]string{
-				testApp, cmdCheck, "-q", name, "--" + flagRequireSignature, msgFpA,
-			}, fs)
-			assert.Equal(t, 1, runCLI(opts))
-			assert.Contains(t, testStderr(t, opts), "manifest is not signed")
-		})
-	}
-}
-
-// TestCommandsReadHelpAsArgument runs each command that takes a path or URL
-// on help and on h where nothing by that name exists. Each must fail on the
-// missing path or bad URL instead of printing its help.
-func TestCommandsReadHelpAsArgument(t *testing.T) {
-	t.Parallel()
-
-	for _, command := range []string{
-		cmdGenerate, cmdCheck, cmdFreshen, cmdList, cmdExport, cmdFetch,
-	} {
-		for _, name := range []string{testCmdHelp, "h"} {
-			t.Run(command+" "+name, func(t *testing.T) {
-				t.Parallel()
-
-				opts := testOpts([]string{testApp, command, name}, afero.NewMemMapFs())
-				assert.Equal(t, 1, runCLI(opts))
-				assert.NotContains(t, testStdout(t, opts), "USAGE:")
-			})
-		}
-	}
-}
-
-// TestCheckHelpBesideArgument runs check with --help or -h before and after
-// a directory, and runs help check. Each must print what check --help prints.
-func TestCheckHelpBesideArgument(t *testing.T) {
-	t.Parallel()
-
-	want := testOpts([]string{testApp, cmdCheck, testFlagHelp}, afero.NewMemMapFs())
-	require.Equal(t, 0, runCLI(want))
-	require.Contains(t, testStdout(t, want), "Validate files using manifest file")
-
-	for _, args := range [][]string{
-		{cmdCheck, testDir, testFlagHelp},
-		{cmdCheck, testFlagHelp, testDir},
-		{cmdCheck, testDir, "-h"},
-		{testCmdHelp, cmdCheck},
-	} {
-		t.Run(strings.Join(args, " "), func(t *testing.T) {
-			t.Parallel()
-
-			opts := testOpts(slices.Concat([]string{testApp}, args), afero.NewMemMapFs())
-			assert.Equal(t, 0, runCLI(opts), testStderr(t, opts))
-			assert.Equal(t, testStdout(t, want), testStdout(t, opts))
-		})
-	}
-}
-
 func TestCheckDetectsManifestCorruption(t *testing.T) {
 	t.Parallel()
 

internal/cli/freshen.go

@@ -28,15 +28,13 @@ const (
 	scanProgressInterval = 100
 )
 
-var (
-	// errEntryMissingMtime indicates a manifest entry that carries no
-	// modification time where one is required to carry it forward unchanged.
-	errEntryMissingMtime = errors.New("manifest entry has no mtime")
+// errEntryMissingMtime indicates a manifest entry that carries no
+// modification time where one is required to carry it forward unchanged.
+var errEntryMissingMtime = errors.New("manifest entry has no mtime")
 
-	// errEntryMissingHash indicates a manifest entry that carries no hash
-	// to carry forward.
-	errEntryMissingHash = errors.New("manifest entry has no hash")
-)
+// 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 {
@@ -481,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)
 
@@ -496,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))
 
@@ -526,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
 	}
@@ -650,8 +653,6 @@ func addFileToBuilder(
 // Entries reach this path only when recordEntry classified them as
 // unchanged, which requires a recorded mtime, so an absent mtime here is
 // an error rather than something to paper over with the Unix epoch.
-// Loading refuses an entry with no hash; one that got here anyway is an
-// error too, not a file to leave out of the manifest.
 func addExistingToBuilder(b *mfer.Builder, entry *mfer.MFFilePath) error {
 	mtime, ok := entryMtime(entry)
 	if !ok {
@@ -659,7 +660,7 @@ func addExistingToBuilder(b *mfer.Builder, entry *mfer.MFFilePath) error {
 	}
 
 	if len(entry.GetHashes()) == 0 {
-		return fmt.Errorf("%w: %s", errEntryMissingHash, entry.GetPath())
+		return nil
 	}
 
 	err := b.AddFileWithHash(mfer.RelFilePath(entry.GetPath()),

internal/cli/freshen_test.go

@@ -471,24 +471,6 @@ func TestFreshenAddExistingRejectsMissingMtime(t *testing.T) {
 	assert.Contains(t, err.Error(), "file1.txt")
 }
 
-// TestFreshenAddExistingRejectsMissingHash pins that an entry with no hash
-// is an error rather than a file left out of the rebuilt manifest.
-func TestFreshenAddExistingRejectsMissingHash(t *testing.T) {
-	t.Parallel()
-
-	b := mfer.NewBuilder()
-	entry := &mfer.MFFilePath{
-		Path:  "data.bin",
-		Size:  8,
-		Mtime: &mfer.Timestamp{Seconds: 1_700_000_000},
-	}
-
-	err := addExistingToBuilder(b, entry)
-	require.ErrorIs(t, err, errEntryMissingHash)
-	assert.Contains(t, err.Error(), "data.bin")
-	assert.Zero(t, b.FileCount())
-}
-
 // TestFreshenAddExistingRejectsShortHash pins that an existing manifest
 // entry whose hash the builder refuses is reported as a problem with the
 // manifest, with the command that fixes it.

internal/cli/mfer.go

@@ -168,17 +168,6 @@ func refuseExtraArgs(_ context.Context, cmd *cli.Command) error {
 	return nil
 }
 
-// showCommandHelp is the CommandNotFound of the commands that take a path or
-// URL. Those commands set HideHelpCommand, so that a path or URL named help or
-// h is an argument rather than urfave/cli's help command; each sets it itself,
-// because on the root it would also remove `mfer help`. With --help,
-// urfave/cli looks the command's first argument up as a help topic, so
-// `check DIR --help` asks for a topic DIR and finds none; this prints the
-// command's own help instead, as `check --help` does.
-func showCommandHelp(ctx context.Context, cmd *cli.Command, _ string) {
-	_ = cli.ShowCommandHelp(ctx, cmd.Root(), cmd.Name)
-}
-
 // requireSignatureFlag returns the --require-signature flag taken by the
 // check and fetch subcommands.
 func requireSignatureFlag() *cli.StringFlag {
@@ -202,12 +191,10 @@ func includePermissionsFlag() *cli.BoolFlag {
 
 func (mfa *CLIApp) generateCommand() *cli.Command {
 	return &cli.Command{
-		Name:            cmdGenerate,
-		Aliases:         []string{"gen"},
-		Usage:           "Generate manifest file",
-		ArgsUsage:       "[path ...]",
-		HideHelpCommand: true,
-		CommandNotFound: showCommandHelp,
+		Name:      cmdGenerate,
+		Aliases:   []string{"gen"},
+		Usage:     "Generate manifest file",
+		ArgsUsage: "[path ...]",
 		Action: func(ctx context.Context, cmd *cli.Command) error {
 			mfa.setVerbosity(cmd)
 			mfa.printBanner()
@@ -266,12 +253,10 @@ func (mfa *CLIApp) generateCommand() *cli.Command {
 
 func (mfa *CLIApp) checkCommand() *cli.Command {
 	return &cli.Command{
-		Name:            cmdCheck,
-		Usage:           "Validate files using manifest file",
-		ArgsUsage:       manifestArgsUsage,
-		ArgValidator:    refuseExtraArgs,
-		HideHelpCommand: true,
-		CommandNotFound: showCommandHelp,
+		Name:         cmdCheck,
+		Usage:        "Validate files using manifest file",
+		ArgsUsage:    manifestArgsUsage,
+		ArgValidator: refuseExtraArgs,
 		Action: func(ctx context.Context, cmd *cli.Command) error {
 			mfa.setVerbosity(cmd)
 			mfa.printBanner()
@@ -302,12 +287,10 @@ func (mfa *CLIApp) checkCommand() *cli.Command {
 
 func (mfa *CLIApp) freshenCommand() *cli.Command {
 	return &cli.Command{
-		Name:            cmdFreshen,
-		Usage:           "Update manifest with changed, new, and removed files",
-		ArgsUsage:       manifestArgsUsage,
-		ArgValidator:    refuseExtraArgs,
-		HideHelpCommand: true,
-		CommandNotFound: showCommandHelp,
+		Name:         cmdFreshen,
+		Usage:        "Update manifest with changed, new, and removed files",
+		ArgsUsage:    manifestArgsUsage,
+		ArgValidator: refuseExtraArgs,
 		Action: func(ctx context.Context, cmd *cli.Command) error {
 			mfa.setVerbosity(cmd)
 			mfa.printBanner()
@@ -355,12 +338,10 @@ func (mfa *CLIApp) freshenCommand() *cli.Command {
 
 func (mfa *CLIApp) exportCommand() *cli.Command {
 	return &cli.Command{
-		Name:            cmdExport,
-		Usage:           "Export manifest contents as JSON",
-		ArgsUsage:       "[manifest file or URL]",
-		ArgValidator:    refuseExtraArgs,
-		HideHelpCommand: true,
-		CommandNotFound: showCommandHelp,
+		Name:         cmdExport,
+		Usage:        "Export manifest contents as JSON",
+		ArgsUsage:    "[manifest file or URL]",
+		ArgValidator: refuseExtraArgs,
 		Action: func(ctx context.Context, cmd *cli.Command) error {
 			mfa.setVerbosity(cmd)
 
@@ -383,14 +364,12 @@ func (mfa *CLIApp) versionCommand() *cli.Command {
 
 func (mfa *CLIApp) listCommand() *cli.Command {
 	return &cli.Command{
-		Name:            cmdList,
-		Aliases:         []string{"ls"},
-		Usage:           "List files in manifest",
-		ArgsUsage:       manifestArgsUsage,
-		ArgValidator:    refuseExtraArgs,
-		HideHelpCommand: true,
-		CommandNotFound: showCommandHelp,
-		Action:          mfa.listManifestOperation,
+		Name:         cmdList,
+		Aliases:      []string{"ls"},
+		Usage:        "List files in manifest",
+		ArgsUsage:    manifestArgsUsage,
+		ArgValidator: refuseExtraArgs,
+		Action:       mfa.listManifestOperation,
 		Flags: []cli.Flag{
 			&cli.BoolFlag{
 				Name:    "long",
@@ -407,12 +386,10 @@ func (mfa *CLIApp) listCommand() *cli.Command {
 
 func (mfa *CLIApp) fetchCommand() *cli.Command {
 	return &cli.Command{
-		Name:            cmdFetch,
-		Usage:           "fetch manifest and referenced files",
-		ArgsUsage:       "URL",
-		ArgValidator:    refuseExtraArgs,
-		HideHelpCommand: true,
-		CommandNotFound: showCommandHelp,
+		Name:         cmdFetch,
+		Usage:        "fetch manifest and referenced files",
+		ArgsUsage:    "URL",
+		ArgValidator: refuseExtraArgs,
 		Action: func(ctx context.Context, cmd *cli.Command) error {
 			mfa.setVerbosity(cmd)
 			mfa.printBanner()

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/deserialize.go

@@ -27,7 +27,6 @@ var (
 	errUUIDMismatch         = errors.New("outer and inner UUID mismatch")
 	errInvalidFileFormat    = errors.New("invalid file format")
 	errInvalidManifestPath  = errors.New("invalid file entry")
-	errEntryMissingHash     = errors.New("file entry has no hash")
 	errDecodedTooLarge      = errors.New("too much memory needed")
 	errSignerNotSigningKey  = errors.New(
 		"signer is not the fingerprint of the key that made the signature")
@@ -294,8 +293,7 @@ func (m *manifest) deserializeInner() error {
 	// untrusted .mf. Reject loudly on the first offender rather than
 	// dropping entries, which would let a hostile manifest hide files from a
 	// check. A path listed twice is refused too: check would check the one
-	// file against both entries. So is an entry with no hash: there is
-	// nothing to check the file against.
+	// file against both entries.
 	seen := make(map[string]bool, len(m.pbInner.GetFiles()))
 
 	for _, f := range m.pbInner.GetFiles() {
@@ -304,10 +302,6 @@ func (m *manifest) deserializeInner() error {
 			return fmt.Errorf("%w: %w", errInvalidManifestPath, err)
 		}
 
-		if len(f.GetHashes()) == 0 {
-			return fmt.Errorf("%w: %q", errEntryMissingHash, f.GetPath())
-		}
-
 		if seen[f.GetPath()] {
 			return fmt.Errorf("%w %q", errDuplicatePath, f.GetPath())
 		}

mfer/deserialize_path_test.go

@@ -165,24 +165,6 @@ func TestDeserializeRefusesPathListedTwice(t *testing.T) {
 	}
 }
 
-// A manifest holding a file entry with no hash is refused as it is loaded,
-// naming the path: there is nothing to check the file against.
-func TestDeserializeRefusesEntryWithNoHash(t *testing.T) {
-	t.Parallel()
-
-	id := uuid.NewV4()
-	inner, err := proto.Marshal(&MFFile{
-		Version: MFFile_VERSION_ONE,
-		Files:   []*MFFilePath{{Path: "x", Size: 1}},
-		Uuid:    id[:],
-	})
-	require.NoError(t, err)
-
-	_, err = NewManifestFromReader(bytes.NewReader(wrapInner(t, id, inner)))
-	require.ErrorIs(t, err, errEntryMissingHash)
-	require.EqualError(t, err, `file entry has no hash: "x"`)
-}
-
 // Entries of a path, an empty hash, an empty MIME type and empty modification
 // and change times are counted at 432 bytes each (176 + 112 + 16 + 64 + 64)
 // and take 16 bytes plus the path to encode. A 37-character path makes that
@@ -247,8 +229,6 @@ func TestDeserializeDropsUnknownFields(t *testing.T) {
 
 	entry := protowire.AppendTag(nil, 1, protowire.BytesType) // MFFilePath.path
 	entry = protowire.AppendString(entry, "a")
-	entry = protowire.AppendTag(entry, 3, protowire.BytesType) // MFFilePath.hashes
-	entry = protowire.AppendBytes(entry, nil)
 	entry = append(entry, unknown...)
 
 	id := uuid.NewV4()

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.go

@@ -8,7 +8,6 @@ import (
 	"io"
 	"slices"
 	"strings"
-	"time"
 
 	"github.com/ProtonMail/go-crypto/openpgp"
 	"github.com/ProtonMail/go-crypto/openpgp/armor"
@@ -243,10 +242,10 @@ func fingerprint(key *openpgp.Entity) string {
 	return strings.ToUpper(hex.EncodeToString(key.PrimaryKey.Fingerprint))
 }
 
-// verifySignature checks that signature is one good, unexpired OpenPGP
-// signature over data, made by the one primary key in pubKey or one of its
-// subkeys, and returns that primary key's fingerprint. signature and pubKey
-// may each be armored or binary.
+// verifySignature checks that signature is one good OpenPGP signature
+// over data, made by the one primary key in pubKey or one of its subkeys,
+// and returns that primary key's fingerprint. signature and pubKey may each
+// be armored or binary.
 func verifySignature(data, signature, pubKey []byte) (string, error) {
 	key, err := readOneKey(pubKey, "embedded public key block", true)
 	if err != nil {
@@ -267,16 +266,10 @@ func verifySignature(data, signature, pubKey []byte) (string, error) {
 		return "", fmt.Errorf("%w, found %d", errNotOneSignature, sigs)
 	}
 
-	sig, _, err := openpgp.VerifyDetachedSignature(openpgp.EntityList{key},
+	_, err = openpgp.CheckDetachedSignature(openpgp.EntityList{key},
 		bytes.NewReader(data), bytes.NewReader(sigData), nil)
 	// A manifest outlives its signing key, so a signature by a key that
-	// has expired since is still good, unless the signature has expired
-	// itself. The library does not check that once it finds the key
-	// expired.
-	if errors.Is(err, pgperrors.ErrKeyExpired) && sig.SigExpired(time.Now()) {
-		err = pgperrors.ErrSignatureExpired
-	}
-
+	// has expired since is still good.
 	if err != nil && !errors.Is(err, pgperrors.ErrKeyExpired) {
 		return "", fmt.Errorf("verify signature: %w", err)
 	}

mfer/openpgp_test.go

@@ -18,7 +18,6 @@ import (
 	"github.com/ProtonMail/go-crypto/openpgp"
 	"github.com/ProtonMail/go-crypto/openpgp/armor"
 	"github.com/ProtonMail/go-crypto/openpgp/elgamal"
-	pgperrors "github.com/ProtonMail/go-crypto/openpgp/errors"
 	"github.com/ProtonMail/go-crypto/openpgp/packet"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
@@ -176,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()
 
@@ -417,10 +435,8 @@ func TestVerifySignature(t *testing.T) {
 	assert.Error(t, err)
 }
 
-// TestVerifySignatureKeyExpiredSince verifies two signatures made in 2020
-// by a key that expired a day after it was made. A signature with no
-// expiry time is still good. One that expired an hour after it was made is
-// refused, although the key has expired too.
+// TestVerifySignatureKeyExpiredSince verifies a signature made in 2020 by
+// a key that expired a day after it was made. The signature is still good.
 func TestVerifySignatureKeyExpiredSince(t *testing.T) {
 	t.Parallel()
 
@@ -432,14 +448,9 @@ func TestVerifySignatureKeyExpiredSince(t *testing.T) {
 	key := newTestKey(t, config)
 	data := []byte("signed in 2020")
 
-	var sig, expiringSig bytes.Buffer
+	var sig bytes.Buffer
 
 	require.NoError(t, openpgp.ArmoredDetachSign(&sig, key, bytes.NewReader(data), config))
-	require.NoError(t, openpgp.ArmoredDetachSign(&expiringSig, key,
-		bytes.NewReader(data), &packet.Config{
-			Time:            config.Time,
-			SigLifetimeSecs: uint32(time.Hour.Seconds()),
-		}))
 
 	pubKey, err := armoredPublicKey(key)
 	require.NoError(t, err)
@@ -447,9 +458,6 @@ func TestVerifySignatureKeyExpiredSince(t *testing.T) {
 	signer, err := verifySignature(data, sig.Bytes(), pubKey)
 	require.NoError(t, err)
 	assert.Equal(t, fingerprint(key), signer)
-
-	_, err = verifySignature(data, expiringSig.Bytes(), pubKey)
-	require.ErrorIs(t, err, pgperrors.ErrSignatureExpired)
 }
 
 func TestManifestSignatureVerification(t *testing.T) {