Read options after the path or URL and refuse a second argument (closes #196)

Every command read options only up to its first argument, and check,
fetch, freshen, list and export dropped everything after it, so
`check DIR --require-signature FPR` passed an unsigned manifest. Options
are now read wherever they appear, as urfave/cli v3 does by default, and
`--` still ends them. Those five commands refuse a second argument,
naming it.

TestFlagAfterArgumentIsArgument asserted the defect (`gen DIR -v` read
`-v` as a path); a test that `gen -- -x` reads `-x` as a path replaces it.

Model: opus-5-5

README.md

@@ -302,7 +302,10 @@ are now tracked only in the [issues](https://git.eeqj.de/sneak/mfer/issues).
       binary, as `gpg --export-secret-keys` writes it; `MFER_SIGN_KEY` names the
       file too. mfer signs it itself and does not need `gpg`. A file holding
       more than one key is refused, and a key held only on a smartcard cannot
-      sign
+      sign. The key must be a version 4 key, whose 40-character fingerprint is
+      what `--require-signature` takes, and not a DSA key. A key that cannot
+      sign, because it has expired or been revoked or its passphrase is wrong,
+      stops `gen` and `freshen` before they read any file
     - takes a protected key's passphrase from `MFER_SIGN_KEY_PASSPHRASE`, or
       else asks for it at the terminal
 - `mfer fetch https://example.com/stuff/`

docs/FORMAT.md

@@ -152,7 +152,10 @@ primary key, `signature` is one good signature over the canonical string made by
 that key (or one of its subkeys), and `signer` is that key's fingerprint. The
 reference implementation refuses to load a manifest that fails these checks;
 `check` and `fetch` given `--require-signature` then compare the required
-fingerprint with `signer`.
+fingerprint with `signer`. It 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.
 
 ## Deterministic Serialization
 

internal/cli/check.go

@@ -21,8 +21,8 @@ import (
 	"sneak.berlin/go/mfer/mfer"
 )
 
-// fingerprintHexLen is the length of a full OpenPGP key fingerprint in hex
+// fingerprintHexLen is the length in hex characters of the fingerprint of
-// characters.
+// an OpenPGP version 4 key, the only version mfer signs with.
 const fingerprintHexLen = 40
 
 var (

internal/cli/entry_test.go

@@ -1368,6 +1368,51 @@ func TestGenerateSeveralPathsDefaultOutput(t *testing.T) {
 	}
 }
 
+// testLink is the name of the symlink to a tree that the
+// DirectoryNamedThroughSymlink tests make in a temp dir.
+const testLink = "link"
+
+// TestGenerateDirectoryNamedThroughSymlink runs gen on a directory named
+// through a symlink, given as the argument or as the working directory: the
+// manifest lists the files in the directory the symlink points to, and
+// leaves out a symlink inside it, as gen does without --follow-symlinks.
+//
+//nolint:paralleltest // changes the process-global working directory
+func TestGenerateDirectoryNamedThroughSymlink(t *testing.T) {
+	// Paths are relative to a temp dir holding data and link, a symlink to
+	// data.
+	for name, tc := range map[string]struct {
+		workDir string
+		args    []string
+	}{
+		"argument":          {".", []string{testLink}},
+		"working directory": {testLink, nil},
+	} {
+		t.Run(name, func(t *testing.T) {
+			root := t.TempDir()
+			data := filepath.Join(root, "data")
+
+			fs := afero.NewOsFs()
+			writeTestTree(t, fs, data)
+			require.NoError(t,
+				os.Symlink(testFileTxt, filepath.Join(data, "alias.txt")))
+			require.NoError(t, os.Symlink(data, filepath.Join(root, testLink)))
+
+			// t.Chdir sets PWD to the path it is given, as a shell does, and
+			// os.Getwd returns PWD when it names the working directory.
+			t.Chdir(filepath.Join(root, tc.workDir))
+
+			opts := testOpts(slices.Concat(
+				[]string{testApp, cmdGenerate, "-q"}, tc.args,
+			), fs)
+			require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts))
+
+			assert.ElementsMatch(t, []string{testFileTxt, "sub/nested.txt"},
+				manifestPaths(t, fs, filepath.Join(data, defaultManifestName)))
+		})
+	}
+}
+
 // TestGenerateBytesDoNotDependOnOutput runs gen --seed on one tree, each
 // time writing to another file, over a file already there and beside an
 // earlier run's temp file: neither is listed, and what is listed depends
@@ -1619,20 +1664,119 @@ func TestGenerateValidatesInputPaths(t *testing.T) {
 	})
 }
 
-// TestFlagAfterArgumentIsArgument asserts that flags are read only before a
+// generateTestDir writes testFile1 and runs gen on testDir, which writes
-// command's first argument: after it, -v is a path, not the verbose flag.
+// the manifest to testDir/index.mf.
-func TestFlagAfterArgumentIsArgument(t *testing.T) {
+func generateTestDir(t *testing.T, fs afero.Fs) {
-	t.Parallel()
+	t.Helper()
 
-	fs := afero.NewMemMapFs()
 	require.NoError(t, fs.MkdirAll(testDir, 0o755))
 	writeTestFile(t, fs, testFile1, "content")
 
-	opts := testOpts([]string{testApp, cmdGenerate, testDir, "-v"}, fs)
+	opts := testOpts([]string{testApp, cmdGenerate, "-q", testDir}, fs)
-	exitCode := runCLI(opts)
+	require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+}
 
-	assert.Equal(t, 1, exitCode)
+// TestCheckReadsOptionAfterPath runs check on an unsigned manifest with
-	assert.Contains(t, testStderr(t, opts), "path does not exist: -v")
+// --require-signature after the directory. check must refuse the manifest.
+func TestCheckReadsOptionAfterPath(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	generateTestDir(t, fs)
+
+	opts := testOpts([]string{
+		testApp, cmdCheck, "-q", testDir, "--" + flagRequireSignature, msgFpA,
+	}, fs)
+	assert.Equal(t, 1, runCLI(opts))
+	assert.Contains(t, testStderr(t, opts), "manifest is not signed")
+}
+
+// TestFreshenReadsOptionAfterPath runs freshen with --include-permissions
+// after the directory. freshen must record the file's mode.
+func TestFreshenReadsOptionAfterPath(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewOsFs()
+	root, manifestPath := setupFreshenDir(t, fs,
+		map[string]string{testFileTxt: "a file"})
+	require.NoError(t, fs.Chmod(filepath.Join(root, testFileTxt), 0o640))
+
+	opts := testOpts([]string{
+		testApp, cmdFreshen, "-q", root, "--" + flagIncludePermissions,
+	}, fs)
+	require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+
+	files := manifestFiles(t, fs, manifestPath)
+	require.Len(t, files, 1)
+	assert.Equal(t, uint32(0o640), files[0].GetMode())
+}
+
+// TestFetchReadsOptionAfterURL runs fetch with --dest after the URL. The
+// file and the manifest must land in that directory and nothing in the
+// current one.
+//
+//nolint:paralleltest // changes the process-global working directory
+func TestFetchReadsOptionAfterURL(t *testing.T) {
+	content := []byte("fetched")
+	manifest := manifestOf(t, map[string][]byte{testFileTxt: content})
+
+	server := httptest.NewServer(fetchTestHandler(manifest,
+		map[string][]byte{testFileTxt: content}))
+	defer server.Close()
+
+	cwd := chdirTemp(t)
+	dest := t.TempDir()
+
+	opts := testOpts([]string{
+		testApp, cmdFetch, "-q", server.URL, "--" + flagDest, dest,
+	}, afero.NewOsFs())
+	require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+
+	assert.Equal(t,
+		map[string][]byte{testFileTxt: content, defaultManifestName: manifest},
+		filesUnder(t, dest))
+	assert.Empty(t, filesUnder(t, cwd), "fetch wrote outside --dest")
+}
+
+// TestOneArgumentCommandsRefuseSecond runs each command that takes one
+// argument with two. It must fail naming the second.
+func TestOneArgumentCommandsRefuseSecond(t *testing.T) {
+	t.Parallel()
+
+	for _, command := range []string{
+		cmdCheck, cmdFreshen, cmdList, cmdExport, cmdFetch,
+	} {
+		t.Run(command, func(t *testing.T) {
+			t.Parallel()
+
+			fs := afero.NewMemMapFs()
+			generateTestDir(t, fs)
+
+			opts := testOpts([]string{testApp, command, testDir, "/second"}, fs)
+			assert.Equal(t, 1, runCLI(opts))
+			assert.Contains(t, testStderr(t, opts),
+				`unexpected argument "/second"`)
+		})
+	}
+}
+
+// TestGenerateReadsPathAfterDoubleDash runs gen -- -x where -x is a
+// directory. -x must be read as the path to list, not as an option.
+//
+//nolint:paralleltest // changes the process-global working directory
+func TestGenerateReadsPathAfterDoubleDash(t *testing.T) {
+	chdirTemp(t)
+
+	fs := afero.NewOsFs()
+	require.NoError(t, fs.Mkdir("-x", 0o755))
+	writeTestFile(t, fs, filepath.Join("-x", testFileTxt), "content")
+
+	opts := testOpts([]string{testApp, cmdGenerate, "-q", "--", "-x"}, fs)
+	require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+
+	files := manifestFiles(t, fs, filepath.Join("-x", defaultManifestName))
+	require.Len(t, files, 1)
+	assert.Equal(t, testFileTxt, files[0].GetPath())
 }
 
 func TestCheckDetectsManifestCorruption(t *testing.T) {

internal/cli/errmsg_test.go

@@ -128,13 +128,19 @@ func TestSignerMismatchMessage(t *testing.T) {
 
 // testSecretKey returns a new OpenPGP key with its secret key, armored, as
 // gpg --export-secret-keys --armor writes it, and the key's fingerprint.
-// The key is protected by passphrase unless that is nil. It is an Ed25519
+// The key is protected by passphrase unless that is nil. config sets how
-// key, which is quick to make.
+// the key is made; without one it is an Ed25519 key, which is quick to
-func testSecretKey(t *testing.T, passphrase []byte) ([]byte, string) {
+// make.
+func testSecretKey(
+	t *testing.T, passphrase []byte, config *packet.Config,
+) ([]byte, string) {
 	t.Helper()
 
-	key, err := openpgp.NewEntity("MFER Test Key", "", "test@mfer.test",
+	if config == nil {
-		&packet.Config{Algorithm: packet.PubKeyAlgoEdDSA})
+		config = &packet.Config{Algorithm: packet.PubKeyAlgoEdDSA}
+	}
+
+	key, err := openpgp.NewEntity("MFER Test Key", "", "test@mfer.test", config)
 	require.NoError(t, err)
 
 	if passphrase != nil {
@@ -155,7 +161,7 @@ func testSecretKey(t *testing.T, passphrase []byte) ([]byte, string) {
 func signedManifest(t *testing.T, files map[string][]byte) []byte {
 	t.Helper()
 
-	secretKey, _ := testSecretKey(t, nil)
+	secretKey, _ := testSecretKey(t, nil, nil)
 
 	b := mfer.NewBuilder()
 	b.SetSigningOptions(&mfer.SigningOptions{SecretKey: secretKey})

internal/cli/freshen.go

@@ -381,6 +381,14 @@ func (mfa *CLIApp) freshenScan(
 		}
 	}
 
+	// The walk does not follow a symlink at its top, so a base directory
+	// named through one is resolved first. If that fails, the base is
+	// walked as named and the walk reports the problem.
+	resolved, err := filepath.EvalSymlinks(absBase)
+	if err == nil {
+		absBase = resolved
+	}
+
 	scanner := &freshenScanner{
 		fs:                 mfa.Fs,
 		absBase:            absBase,
@@ -392,7 +400,7 @@ func (mfa *CLIApp) freshenScan(
 		existingByPath:     existingByPath,
 	}
 
-	err := afero.Walk(mfa.Fs, absBase, scanner.walk)
+	err = afero.Walk(mfa.Fs, absBase, scanner.walk)
 
 	if showProgress {
 		log.ProgressDone()

internal/cli/freshen_test.go

@@ -336,6 +336,49 @@ func TestFreshenResolvesEntriesAgainstManifestDirectory(t *testing.T) {
 	}
 }
 
+// TestFreshenDirectoryNamedThroughSymlink adds a file to a tree after gen
+// made its manifest, then freshens it with the tree named through a symlink,
+// given as the argument or as the working directory: the manifest lists the
+// files in the tree, and leaves out a symlink inside it, as freshen does
+// without --follow-symlinks.
+//
+//nolint:paralleltest // changes the process-global working directory
+func TestFreshenDirectoryNamedThroughSymlink(t *testing.T) {
+	// Paths are relative to a temp dir holding link, a symlink to the tree.
+	for name, tc := range map[string]struct {
+		workDir string
+		args    []string
+	}{
+		"argument":          {".", []string{testLink}},
+		"working directory": {testLink, nil},
+	} {
+		t.Run(name, func(t *testing.T) {
+			fs := afero.NewOsFs()
+			tree, manifestPath := setupFreshenDir(t, fs,
+				map[string]string{testFileTxt: "in the tree"})
+			writeTestFile(t, fs, filepath.Join(tree, "later.txt"), "added later")
+			require.NoError(t,
+				os.Symlink(testFileTxt, filepath.Join(tree, "alias.txt")))
+
+			root := t.TempDir()
+			require.NoError(t, os.Symlink(tree, filepath.Join(root, testLink)))
+
+			// t.Chdir sets PWD to the path it is given, as a shell does, and
+			// os.Getwd returns PWD when it names the working directory.
+			t.Chdir(filepath.Join(root, tc.workDir))
+
+			opts := testOpts(slices.Concat(
+				[]string{testApp, cmdFreshen, "-q"}, tc.args,
+			), fs)
+			require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts))
+
+			assertManifestLists(t, fs, tree, manifestPath, map[string]string{
+				testFileTxt: "in the tree", "later.txt": "added later",
+			})
+		})
+	}
+}
+
 // TestFreshenRecordEntryMtimePresence pins the behavior of recordEntry
 // with respect to MFFilePath.Mtime, which is a message pointer with
 // proto3 field presence and may legitimately be absent.

internal/cli/mfer.go

@@ -48,6 +48,10 @@ func manifestTempPath(out string) string {
 // errUnknownCommand indicates an unrecognized command argument.
 var errUnknownCommand = errors.New("unknown command")
 
+// errUnexpectedArgument indicates a second argument to a command that takes
+// one.
+var errUnexpectedArgument = errors.New("unexpected argument")
+
 // CLIApp is the main CLI application container. It holds configuration,
 // I/O streams, and filesystem abstraction to enable testing and flexibility.
 //
@@ -154,13 +158,14 @@ func commonFlags() []cli.Flag {
 	}
 }
 
-// stopOnFirstArg returns the StopOnNthArg setting every command uses: flags
+// refuseExtraArgs is the ArgValidator of the commands that take one
-// are read only before the command's first argument, and everything after it
+// argument: it refuses a second one, naming it.
-// is an argument, as with urfave/cli v2. So `mfer gen d -v` names a path "-v".
+func refuseExtraArgs(_ context.Context, cmd *cli.Command) error {
-func stopOnFirstArg() *int {
+	if cmd.Args().Len() > 1 {
-	n := 1
+		return fmt.Errorf("%w %q", errUnexpectedArgument, cmd.Args().Get(1))
+	}
 
-	return &n
+	return nil
 }
 
 // requireSignatureFlag returns the --require-signature flag taken by the
@@ -186,11 +191,10 @@ func includePermissionsFlag() *cli.BoolFlag {
 
 func (mfa *CLIApp) generateCommand() *cli.Command {
 	return &cli.Command{
-		Name:         cmdGenerate,
+		Name:      cmdGenerate,
-		Aliases:      []string{"gen"},
+		Aliases:   []string{"gen"},
-		Usage:        "Generate manifest file",
+		Usage:     "Generate manifest file",
-		ArgsUsage:    "[path ...]",
+		ArgsUsage: "[path ...]",
-		StopOnNthArg: stopOnFirstArg(),
 		Action: func(ctx context.Context, cmd *cli.Command) error {
 			mfa.setVerbosity(cmd)
 			mfa.printBanner()
@@ -252,7 +256,7 @@ func (mfa *CLIApp) checkCommand() *cli.Command {
 		Name:         cmdCheck,
 		Usage:        "Validate files using manifest file",
 		ArgsUsage:    manifestArgsUsage,
-		StopOnNthArg: stopOnFirstArg(),
+		ArgValidator: refuseExtraArgs,
 		Action: func(ctx context.Context, cmd *cli.Command) error {
 			mfa.setVerbosity(cmd)
 			mfa.printBanner()
@@ -286,7 +290,7 @@ func (mfa *CLIApp) freshenCommand() *cli.Command {
 		Name:         cmdFreshen,
 		Usage:        "Update manifest with changed, new, and removed files",
 		ArgsUsage:    manifestArgsUsage,
-		StopOnNthArg: stopOnFirstArg(),
+		ArgValidator: refuseExtraArgs,
 		Action: func(ctx context.Context, cmd *cli.Command) error {
 			mfa.setVerbosity(cmd)
 			mfa.printBanner()
@@ -337,7 +341,7 @@ func (mfa *CLIApp) exportCommand() *cli.Command {
 		Name:         cmdExport,
 		Usage:        "Export manifest contents as JSON",
 		ArgsUsage:    "[manifest file or URL]",
-		StopOnNthArg: stopOnFirstArg(),
+		ArgValidator: refuseExtraArgs,
 		Action: func(ctx context.Context, cmd *cli.Command) error {
 			mfa.setVerbosity(cmd)
 
@@ -348,9 +352,8 @@ func (mfa *CLIApp) exportCommand() *cli.Command {
 
 func (mfa *CLIApp) versionCommand() *cli.Command {
 	return &cli.Command{
-		Name:         cmdVersion,
+		Name:  cmdVersion,
-		Usage:        "Show version",
+		Usage: "Show version",
-		StopOnNthArg: stopOnFirstArg(),
 		Action: func(context.Context, *cli.Command) error {
 			mfa.printVersion()
 
@@ -365,7 +368,7 @@ func (mfa *CLIApp) listCommand() *cli.Command {
 		Aliases:      []string{"ls"},
 		Usage:        "List files in manifest",
 		ArgsUsage:    manifestArgsUsage,
-		StopOnNthArg: stopOnFirstArg(),
+		ArgValidator: refuseExtraArgs,
 		Action:       mfa.listManifestOperation,
 		Flags: []cli.Flag{
 			&cli.BoolFlag{
@@ -386,7 +389,7 @@ func (mfa *CLIApp) fetchCommand() *cli.Command {
 		Name:         cmdFetch,
 		Usage:        "fetch manifest and referenced files",
 		ArgsUsage:    "URL",
-		StopOnNthArg: stopOnFirstArg(),
+		ArgValidator: refuseExtraArgs,
 		Action: func(ctx context.Context, cmd *cli.Command) error {
 			mfa.setVerbosity(cmd)
 			mfa.printBanner()
@@ -445,9 +448,8 @@ func (mfa *CLIApp) run(args []string) {
 		Writer:                mfa.Stdout,
 		// v3 writes its "Incorrect Usage" line to ErrWriter; v2 wrote it to
 		// stdout, before the help, so it stays on stdout.
-		ErrWriter:    mfa.Stdout,
+		ErrWriter: mfa.Stdout,
-		Flags:        commonFlags(),
+		Flags:     commonFlags(),
-		StopOnNthArg: stopOnFirstArg(),
 		Action: func(_ context.Context, cmd *cli.Command) error {
 			if cmd.Args().Len() > 0 {
 				return fmt.Errorf("%w %q", errUnknownCommand, cmd.Args().First())

internal/cli/signing.go

@@ -23,9 +23,9 @@ var errNoPassphrase = errors.New(
 	"signing key is protected: set " + envSignKeyPassphrase + " to its passphrase")
 
 // signingOptions returns the signing options for the OpenPGP secret key in
-// the file path. The passphrase of a protected key comes from
+// the file path, which must be able to sign. The passphrase of a protected
-// MFER_SIGN_KEY_PASSPHRASE, or else from the terminal on stdin, and must
+// key comes from MFER_SIGN_KEY_PASSPHRASE, or else from the terminal on
-// unlock the key.
+// stdin, and must unlock the key.
 func (mfa *CLIApp) signingOptions(path string) (*mfer.SigningOptions, error) {
 	secretKey, err := afero.ReadFile(mfa.Fs, path)
 	if err != nil {
@@ -40,17 +40,16 @@ func (mfa *CLIApp) signingOptions(path string) (*mfer.SigningOptions, error) {
 	log.Infof("signing manifest with the OpenPGP key in %s", path)
 
 	opts := &mfer.SigningOptions{SecretKey: secretKey}
-	if !protected {
+	if protected {
-		return opts, nil
+		opts.Passphrase, err = mfa.readPassphrase(path)
+		if err != nil {
+			return nil, err
+		}
 	}
 
-	opts.Passphrase, err = mfa.readPassphrase(path)
+	// gen and freshen read the signing options before any file, so a key
-	if err != nil {
+	// that cannot sign, or a wrong passphrase, stops them before they hash
-		return nil, err
+	// anything.
-	}
-
-	// gen and freshen read the signing options before any file, so a
-	// wrong passphrase stops them before they hash anything.
 	err = mfer.CheckSigningKey(opts)
 	if err != nil {
 		return nil, fmt.Errorf("%s: %w", path, err)

internal/cli/signing_test.go

@@ -7,7 +7,9 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 
+	"github.com/ProtonMail/go-crypto/openpgp/packet"
 	"github.com/creack/pty"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
@@ -33,7 +35,7 @@ func TestGenAndFreshenSignWithKeyFile(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			t.Setenv(envSignKeyPassphrase, string(passphrase))
 
-			secretKey, fingerprint := testSecretKey(t, passphrase)
+			secretKey, fingerprint := testSecretKey(t, passphrase, nil)
 
 			fs := afero.NewOsFs()
 			keyFile := filepath.Join(t.TempDir(), "key.asc")
@@ -77,7 +79,7 @@ func TestGenAndFreshenSignWithKeyFile(t *testing.T) {
 func TestSignWithProtectedKeyNeedsPassphrase(t *testing.T) {
 	t.Setenv(envSignKeyPassphrase, "")
 
-	secretKey, _ := testSecretKey(t, []byte("secret"))
+	secretKey, _ := testSecretKey(t, []byte("secret"), nil)
 
 	fs := afero.NewMemMapFs()
 	require.NoError(t, afero.WriteFile(fs, testKeyFile, secretKey, 0o600))
@@ -97,29 +99,48 @@ func TestSignWithProtectedKeyNeedsPassphrase(t *testing.T) {
 	assert.False(t, exists)
 }
 
-// TestSignWithWrongPassphraseFailsFirst runs gen on a directory and
+// TestSignWithKeyThatCannotSignFailsFirst runs gen on a directory and
-// freshen on a manifest, neither of which exists, with a protected key and
+// freshen on a manifest, neither of which exists, with keys that cannot
-// a wrong MFER_SIGN_KEY_PASSPHRASE. Each must fail to unlock the key: it
+// sign: a protected key with a wrong MFER_SIGN_KEY_PASSPHRASE, a key that
-// does that before it reads any file, so a missing file goes unnoticed.
+// expired in 2020, and a version 6 key. Each run must fail on the key: it
-func TestSignWithWrongPassphraseFailsFirst(t *testing.T) {
+// checks the key before it reads any file, so a missing file goes
+// unnoticed.
+func TestSignWithKeyThatCannotSignFailsFirst(t *testing.T) {
 	t.Setenv(envSignKeyPassphrase, "wrong")
 
-	secretKey, _ := testSecretKey(t, []byte("right"))
+	wrongPassphrase, _ := testSecretKey(t, []byte("right"), nil)
 
-	fs := afero.NewMemMapFs()
+	made := time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC)
-	require.NoError(t, afero.WriteFile(fs, testKeyFile, secretKey, 0o600))
+	expired, _ := testSecretKey(t, nil, &packet.Config{
+		Algorithm:       packet.PubKeyAlgoEdDSA,
+		Time:            func() time.Time { return made },
+		KeyLifetimeSecs: uint32((24 * time.Hour).Seconds()),
+	})
 
-	for _, args := range [][]string{
+	version6, _ := testSecretKey(t, nil, &packet.Config{
-		{
+		Algorithm: packet.PubKeyAlgoEd25519,
-			testApp, cmdGenerate, "-q", testFlagSignKey, testKeyFile,
+		V6Keys:    true,
-			"-o", testMF, "/missing",
+	})
-		},
+
-		{testApp, cmdFreshen, "-q", testFlagSignKey, testKeyFile, "/missing.mf"},
+	for want, secretKey := range map[string][]byte{
+		"unlock signing key":                           wrongPassphrase,
+		"signing key cannot sign":                      expired,
+		"signing key must be an OpenPGP version 4 key": version6,
 	} {
-		opts := testOpts(args, fs)
+		fs := afero.NewMemMapFs()
-		assert.Equal(t, 1, runCLI(opts), args[1])
+		require.NoError(t, afero.WriteFile(fs, testKeyFile, secretKey, 0o600))
-		assert.Contains(t, testStderr(t, opts),
+
-			testKeyFile+": unlock signing key", args[1])
+		for _, args := range [][]string{
+			{
+				testApp, cmdGenerate, "-q", testFlagSignKey, testKeyFile,
+				"-o", testMF, "/missing",
+			},
+			{testApp, cmdFreshen, "-q", testFlagSignKey, testKeyFile, "/missing.mf"},
+		} {
+			opts := testOpts(args, fs)
+			assert.Equal(t, 1, runCLI(opts), args[1], want)
+			assert.Contains(t, testStderr(t, opts), testKeyFile+": "+want, args[1])
+		}
 	}
 }
 
@@ -130,7 +151,7 @@ func TestSignWithWrongPassphraseFailsFirst(t *testing.T) {
 func TestGenAsksForPassphraseOnTerminal(t *testing.T) {
 	t.Setenv(envSignKeyPassphrase, "")
 
-	secretKey, fingerprint := testSecretKey(t, []byte("passphrase"))
+	secretKey, fingerprint := testSecretKey(t, []byte("passphrase"), nil)
 
 	fs := afero.NewOsFs()
 	keyFile := filepath.Join(t.TempDir(), "key.asc")

mfer/openpgp.go

@@ -16,12 +16,26 @@ import (
 )
 
 const (
-	// The tags of OpenPGP signature, secret key and public key packets
+	// The tags of OpenPGP signature, key and subkey packets (RFC 9580,
-	// (RFC 9580, section 5). Subkeys have tags of their own, so each
+	// section 5). Subkeys have tags of their own, so each secret or public
-	// secret or public key packet is one primary key.
+	// key packet is one primary key.
-	signaturePacketTag = 2
+	signaturePacketTag    = 2
-	secretKeyPacketTag = 5
+	secretKeyPacketTag    = 5
-	publicKeyPacketTag = 6
+	publicKeyPacketTag    = 6
+	secretSubkeyPacketTag = 7
+	publicSubkeyPacketTag = 14
+
+	// In the body of a key or subkey packet the algorithm octet follows
+	// the version octet and the four-octet creation time, and from version
+	// 5 on a four-octet length as well (RFC 9580, section 5.5.2).
+	keyAlgorithmOffset            = 5
+	firstKeyVersionWithLength     = 5
+	keyAlgorithmOffsetAfterLength = 9
+
+	// signingKeyVersion is the only OpenPGP key version mfer signs with.
+	// Its fingerprints are 40 hex characters, the length
+	// --require-signature takes.
+	signingKeyVersion = 4
 
 	// armorBegin and armorEnd start the lines that begin and end an
 	// armored block.
@@ -30,14 +44,19 @@ const (
 )
 
 var (
-	errKeyCount     = errors.New("must hold exactly one key")
+	errKeyCount    = errors.New("must hold exactly one key")
-	errNoSecretKey  = errors.New("signing key file holds no secret key")
+	errDSAKey      = errors.New("must not hold a DSA key")
+	errSecretKey   = errors.New("must not hold a secret key")
+	errNoSecretKey = errors.New("signing key file holds no secret key")
+	errNotV4Key    = errors.New("signing key must be an OpenPGP version 4 key, " +
+		"the only kind whose fingerprint --require-signature takes")
 	errNoPassphrase = errors.New(
 		"signing key is protected and no passphrase was given")
 	errNotOneSignature = errors.New(
 		"signature must hold exactly one signature")
 	errNotOneArmoredBlock = errors.New(
 		"must be exactly one armored block and nothing else")
+	errMalformedArmor = errors.New("armor is malformed")
 )
 
 // SigningOptions holds the key a manifest is signed with.
@@ -51,7 +70,7 @@ type SigningOptions struct {
 
 // SecretKeyIsProtected reports whether the OpenPGP secret key secretKey,
 // armored or binary, needs a passphrase to sign. It fails unless
-// secretKey holds one primary key with its secret key.
+// secretKey holds one version 4 primary key with its secret key.
 func SecretKeyIsProtected(secretKey []byte) (bool, error) {
 	key, err := readSecretKey(secretKey)
 	if err != nil {
@@ -61,14 +80,24 @@ func SecretKeyIsProtected(secretKey []byte) (bool, error) {
 	return isProtected(key), nil
 }
 
-// CheckSigningKey fails unless opts can sign: opts.SecretKey must hold one
+// CheckSigningKey fails unless opts can sign now: opts.SecretKey must hold
-// primary key with its secret key, and opts.Passphrase must unlock it when
+// one version 4 primary key with a secret key that may sign and has not
-// it is protected. It lets a caller find a wrong passphrase before it
+// expired or been revoked, and opts.Passphrase must unlock it when it is
-// builds a manifest.
+// protected. It lets a caller find a key that cannot sign before it builds
+// a manifest.
 func CheckSigningKey(opts *SigningOptions) error {
-	_, err := readSigningKey(opts)
+	key, err := readSigningKey(opts)
+	if err != nil {
+		return err
+	}
 
-	return err
+	// Signing nothing fails wherever signing the manifest would.
+	err = openpgp.DetachSign(io.Discard, key, bytes.NewReader(nil), nil)
+	if err != nil {
+		return fmt.Errorf("signing key cannot sign: %w", err)
+	}
+
+	return nil
 }
 
 // readSigningKey returns the key in opts.SecretKey, unlocked with
@@ -96,13 +125,18 @@ func readSigningKey(opts *SigningOptions) (*openpgp.Entity, error) {
 }
 
 // readSecretKey returns the one key in secretKey, armored or binary,
-// which must include its secret key.
+// which must be a version 4 key and include its secret key.
 func readSecretKey(secretKey []byte) (*openpgp.Entity, error) {
-	key, err := readOneKey(secretKey, "signing key file")
+	key, err := readOneKey(secretKey, "signing key file", false)
 	if err != nil {
 		return nil, err
 	}
 
+	if key.PrimaryKey.Version != signingKeyVersion {
+		return nil, fmt.Errorf("%w; this key is version %d",
+			errNotV4Key, key.PrimaryKey.Version)
+	}
+
 	if key.PrivateKey == nil {
 		return nil, errNoSecretKey
 	}
@@ -111,8 +145,9 @@ func readSecretKey(secretKey []byte) (*openpgp.Entity, error) {
 }
 
 // readOneKey returns the key in data, armored or binary, which must hold
-// exactly one primary key. what names data in errors.
+// exactly one primary key and no DSA key or subkey, and when publicOnly no
-func readOneKey(data []byte, what string) (*openpgp.Entity, error) {
+// secret key or subkey either. what names data in errors.
+func readOneKey(data []byte, what string, publicOnly bool) (*openpgp.Entity, error) {
 	packets, err := dearmor(data)
 	if err != nil {
 		return nil, fmt.Errorf("read %s: %w", what, err)
@@ -127,6 +162,29 @@ func readOneKey(data []byte, what string) (*openpgp.Entity, error) {
 		return nil, fmt.Errorf("%s %w, found %d", what, errKeyCount, keys)
 	}
 
+	// openpgp.ReadKeyRing checks the numbers of every secret key it reads,
+	// and an ElGamal secret subkey with a very large prime makes that take
+	// minutes.
+	secretKeys, err := countPackets(packets, secretKeyPacketTag, secretSubkeyPacketTag)
+	if err != nil {
+		return nil, fmt.Errorf("read %s: %w", what, err)
+	}
+
+	if publicOnly && secretKeys != 0 {
+		return nil, fmt.Errorf("%s %w", what, errSecretKey)
+	}
+
+	// openpgp.ReadKeyRing also checks every self-signature, and a DSA key
+	// with very large numbers makes each check take seconds to minutes.
+	dsa, err := holdsDSAKey(packets)
+	if err != nil {
+		return nil, fmt.Errorf("read %s: %w", what, err)
+	}
+
+	if dsa {
+		return nil, fmt.Errorf("%s %w", what, errDSAKey)
+	}
+
 	keyring, err := openpgp.ReadKeyRing(bytes.NewReader(packets))
 	if err != nil {
 		return nil, fmt.Errorf("read %s: %w", what, err)
@@ -189,7 +247,7 @@ func fingerprint(key *openpgp.Entity) string {
 // 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")
+	key, err := readOneKey(pubKey, "embedded public key block", true)
 	if err != nil {
 		return "", err
 	}
@@ -240,12 +298,20 @@ func dearmor(data []byte) ([]byte, error) {
 		return nil, errNotOneArmoredBlock
 	}
 
+	// armor.Decode passes over a block it cannot read, such as one with a
+	// header line that has no colon, and returns io.EOF on finding no
+	// other.
 	block, err := armor.Decode(bytes.NewReader(text))
 	if err != nil {
-		return nil, err
+		return nil, errMalformedArmor
 	}
 
-	return io.ReadAll(block.Body)
+	body, err := io.ReadAll(block.Body)
+	if err != nil {
+		return nil, fmt.Errorf("%w: %w", errMalformedArmor, err)
+	}
+
+	return body, nil
 }
 
 // countPackets returns how many packets in the binary OpenPGP data have
@@ -271,3 +337,40 @@ func countPackets(data []byte, tags ...uint8) (int, error) {
 		}
 	}
 }
+
+// holdsDSAKey reports whether any key or subkey packet in the binary
+// OpenPGP data holds a DSA key. It reads each packet's algorithm octet
+// rather than parsing the packet, since parsing a secret key packet checks
+// its numbers, which for a DSA key with very large numbers is as slow as
+// checking a self-signature.
+func holdsDSAKey(data []byte) (bool, error) {
+	packets := packet.NewOpaqueReader(bytes.NewReader(data))
+
+	for {
+		p, err := packets.Next()
+		if errors.Is(err, io.EOF) {
+			return false, nil
+		}
+
+		if err != nil {
+			return false, err
+		}
+
+		if !slices.Contains([]uint8{
+			secretKeyPacketTag, publicKeyPacketTag,
+			secretSubkeyPacketTag, publicSubkeyPacketTag,
+		}, p.Tag) {
+			continue
+		}
+
+		offset := keyAlgorithmOffset
+		if len(p.Contents) > 0 && p.Contents[0] >= firstKeyVersionWithLength {
+			offset = keyAlgorithmOffsetAfterLength
+		}
+
+		if len(p.Contents) > offset &&
+			packet.PublicKeyAlgorithm(p.Contents[offset]) == packet.PubKeyAlgoDSA {
+			return true, nil
+		}
+	}
+}

mfer/openpgp_test.go

@@ -4,7 +4,9 @@ package mfer
 import (
 	"bytes"
 	"context"
+	"crypto/dsa" //nolint:staticcheck // SA1019: tests need a DSA key to refuse
 	"io"
+	"math/big"
 	"os"
 	"path/filepath"
 	"slices"
@@ -15,6 +17,7 @@ import (
 
 	"github.com/ProtonMail/go-crypto/openpgp"
 	"github.com/ProtonMail/go-crypto/openpgp/armor"
+	"github.com/ProtonMail/go-crypto/openpgp/elgamal"
 	"github.com/ProtonMail/go-crypto/openpgp/packet"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
@@ -284,6 +287,97 @@ func TestSigningKeyFileWithoutSecretKey(t *testing.T) {
 	require.ErrorIs(t, b.Build(context.Background(), io.Discard), errNoSecretKey)
 }
 
+// TestCheckSigningKeyRefusesKeyThatCannotSign checks a key that can sign,
+// and keys that read and need no passphrase but cannot sign now: one that
+// expired in 2020 and one that has been revoked. CheckSigningKey must
+// refuse each of the two, as signing a manifest with it would fail.
+func TestCheckSigningKeyRefusesKeyThatCannotSign(t *testing.T) {
+	t.Parallel()
+
+	require.NoError(t, CheckSigningKey(testSigningOptions(t)))
+
+	made := time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC)
+	expired := newTestKey(t, &packet.Config{
+		Time:            func() time.Time { return made },
+		KeyLifetimeSecs: uint32((24 * time.Hour).Seconds()),
+	})
+
+	revoked := newTestKey(t, nil)
+	require.NoError(t, revoked.RevokeKey(packet.KeyRetired, "", nil))
+
+	for name, key := range map[string]*openpgp.Entity{
+		"expired": expired,
+		"revoked": revoked,
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			assert.ErrorContains(t, CheckSigningKey(
+				&SigningOptions{SecretKey: armoredSecretKeys(t, key)}),
+				"signing key cannot sign")
+		})
+	}
+}
+
+// TestSigningRefusesVersion6Key signs with an OpenPGP version 6 key, whose
+// fingerprint is 64 hex characters. mfer signs only with version 4 keys.
+func TestSigningRefusesVersion6Key(t *testing.T) {
+	t.Parallel()
+
+	key, err := openpgp.NewEntity("MFER Test Key", "", "test@mfer.test",
+		&packet.Config{V6Keys: true, Algorithm: packet.PubKeyAlgoEd25519})
+	require.NoError(t, err)
+
+	secretKey := armoredSecretKeys(t, key)
+
+	_, err = SecretKeyIsProtected(secretKey)
+	require.ErrorIs(t, err, errNotV4Key)
+
+	b := NewBuilder()
+	b.SetSigningOptions(&SigningOptions{SecretKey: secretKey})
+	require.ErrorIs(t, b.Build(context.Background(), io.Discard), errNotV4Key)
+}
+
+// TestMalformedArmorIsNamed reads a signing key file, a signature and an
+// embedded public key block whose armor is malformed: a header line with
+// no colon, or no blank line after the BEGIN line. Each must fail saying
+// the armor is malformed.
+func TestMalformedArmorIsNamed(t *testing.T) {
+	t.Parallel()
+
+	opts := testSigningOptions(t)
+	manifest := signedTestManifest(t, opts)
+
+	for name, change := range map[string]func([]byte) []byte{
+		"header line with no colon": func(block []byte) []byte {
+			return bytes.Replace(block,
+				[]byte("-----\n"), []byte("-----\nno colon\n"), 1)
+		},
+		"no blank line after the BEGIN line": func(block []byte) []byte {
+			return bytes.Replace(block, []byte("-----\n\n"), []byte("-----\n"), 1)
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			_, err := SecretKeyIsProtected(change(opts.SecretKey))
+			require.ErrorIs(t, err, errMalformedArmor)
+
+			for _, changed := range [][]byte{
+				rewriteOuter(t, manifest, func(outer *MFFileOuter) {
+					outer.Signature = change(outer.GetSignature())
+				}),
+				rewriteOuter(t, manifest, func(outer *MFFileOuter) {
+					outer.SigningPubKey = change(outer.GetSigningPubKey())
+				}),
+			} {
+				_, err = NewManifestFromReader(bytes.NewReader(changed))
+				require.ErrorIs(t, err, errMalformedArmor)
+			}
+		})
+	}
+}
+
 func TestVerifySignature(t *testing.T) {
 	t.Parallel()
 
@@ -463,6 +557,105 @@ func TestManifestRefusesSecondEmbeddedKeyWithoutUserID(t *testing.T) {
 	require.ErrorIs(t, err, errKeyCount)
 }
 
+// dsaKeyPacket returns a public key packet holding a DSA key, or a public
+// subkey packet when isSubkey. Its numbers are not a working key: loading
+// must refuse the packet before it uses them.
+func dsaKeyPacket(t *testing.T, isSubkey bool) []byte {
+	t.Helper()
+
+	key := packet.NewDSAPublicKey(time.Now(), &dsa.PublicKey{
+		P: big.NewInt(23), Q: big.NewInt(11), G: big.NewInt(4), Y: big.NewInt(8),
+	})
+	key.IsSubkey = isSubkey
+
+	var buf bytes.Buffer
+
+	require.NoError(t, key.Serialize(&buf))
+
+	return buf.Bytes()
+}
+
+// TestManifestRefusesDSAKey loads manifests whose embedded public key
+// block holds a DSA key: alone, or as a subkey after the key that signed
+// the manifest. openpgp.ReadKeyRing checks a key's self-signatures, which
+// for a DSA key with very large numbers takes minutes each. Loading must
+// refuse each block before that.
+func TestManifestRefusesDSAKey(t *testing.T) {
+	t.Parallel()
+
+	signer := newTestKey(t, nil)
+	manifest := signedTestManifest(t,
+		&SigningOptions{SecretKey: armoredSecretKeys(t, signer)})
+
+	var signerKey bytes.Buffer
+
+	require.NoError(t, signer.Serialize(&signerKey))
+
+	for name, block := range map[string][]byte{
+		"DSA key":    dsaKeyPacket(t, false),
+		"DSA subkey": slices.Concat(signerKey.Bytes(), dsaKeyPacket(t, true)),
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			embedded := rewriteOuter(t, manifest, func(outer *MFFileOuter) {
+				outer.SigningPubKey = block
+			})
+
+			_, err := NewManifestFromReader(bytes.NewReader(embedded))
+			require.ErrorIs(t, err, errDSAKey)
+		})
+	}
+}
+
+// elGamalSecretSubkeyPacket returns a secret subkey packet holding an
+// ElGamal key. Its numbers are not a working key: loading must refuse the
+// packet before it uses them.
+func elGamalSecretSubkeyPacket(t *testing.T) []byte {
+	t.Helper()
+
+	key := packet.NewElGamalPrivateKey(time.Now(), &elgamal.PrivateKey{
+		P: big.NewInt(23), G: big.NewInt(4), Y: big.NewInt(8), X: big.NewInt(3),
+	})
+	key.IsSubkey = true
+
+	var buf bytes.Buffer
+
+	require.NoError(t, key.Serialize(&buf))
+
+	return buf.Bytes()
+}
+
+// TestManifestRefusesSecretKey loads manifests whose embedded public key
+// block holds a secret key: the key that signed the manifest with its
+// secret key, or its public key followed by an ElGamal secret subkey.
+// openpgp.ReadKeyRing checks the numbers of every secret key it reads,
+// which for an ElGamal key with a very large prime takes minutes. Loading
+// must refuse each block before that.
+func TestManifestRefusesSecretKey(t *testing.T) {
+	t.Parallel()
+
+	signer := newTestKey(t, nil)
+	manifest := signedTestManifest(t,
+		&SigningOptions{SecretKey: armoredSecretKeys(t, signer)})
+
+	var signerKey bytes.Buffer
+
+	require.NoError(t, signer.Serialize(&signerKey))
+
+	for _, block := range [][]byte{
+		armoredSecretKeys(t, signer),
+		slices.Concat(signerKey.Bytes(), elGamalSecretSubkeyPacket(t)),
+	} {
+		embedded := rewriteOuter(t, manifest, func(outer *MFFileOuter) {
+			outer.SigningPubKey = block
+		})
+
+		_, err := NewManifestFromReader(bytes.NewReader(embedded))
+		require.ErrorIs(t, err, errSecretKey)
+	}
+}
+
 // TestManifestRefusesTwoSignatures loads a manifest whose signature field
 // holds its good signature twice, not armored. Loading must refuse it.
 func TestManifestRefusesTwoSignatures(t *testing.T) {

mfer/scanner.go

@@ -139,25 +139,15 @@ func (s *Scanner) EnumerateFile(filePath string) error {
 	return s.enumerateFileWithInfo(filepath.Base(abs), basePath, info, nil)
 }
 
-// EnumeratePath walks a directory path and adds all files to the scanner.
+// EnumeratePath adds inputPath, a directory or a file, to the scanner as
+// EnumeratePaths adds each of its paths.
 // If progress is non-nil, status updates are sent as files are discovered.
 // The progress channel is closed when the method returns.
 func (s *Scanner) EnumeratePath(
 	inputPath string,
 	progress chan<- EnumerateStatus,
 ) error {
-	if progress != nil {
+	return s.EnumeratePaths(progress, inputPath)
-		defer close(progress)
-	}
-
-	abs, err := filepath.Abs(inputPath)
-	if err != nil {
-		return err
-	}
-
-	afs := afero.NewReadOnlyFs(afero.NewBasePathFs(s.fs, abs))
-
-	return s.enumerateFS(afs, abs, progress)
 }
 
 // EnumeratePaths adds to the scanner the files under each directory path,
@@ -185,6 +175,14 @@ func (s *Scanner) EnumeratePaths(
 		}
 
 		if info.IsDir() {
+			// The walk does not follow a symlink at its top, so a directory
+			// named through one is resolved first. If that fails, the
+			// directory is walked as named and the walk reports the problem.
+			resolved, evalErr := filepath.EvalSymlinks(abs)
+			if evalErr == nil {
+				abs = resolved
+			}
+
 			afs := afero.NewReadOnlyFs(afero.NewBasePathFs(s.fs, abs))
 			err = s.enumerateFS(afs, abs, progress)
 		} else {

mfer/scanner_test.go

@@ -118,6 +118,27 @@ func TestScannerEnumeratePathWithProgress(t *testing.T) {
 	assert.Equal(t, FileSize(6), final.BytesFound)
 }
 
+// TestScannerEnumeratePathFile gives EnumeratePath a file: it is listed
+// by its name, as EnumerateFile lists it, and the manifest can be built.
+func TestScannerEnumeratePathFile(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	require.NoError(t, fs.MkdirAll("/dir", 0o755))
+	require.NoError(t, afero.WriteFile(fs, "/dir/one.txt", []byte("1"), 0o644))
+
+	s := NewScannerWithOptions(&ScannerOptions{Fs: fs})
+	require.NoError(t, s.EnumeratePath("/dir/one.txt", nil))
+
+	var buf bytes.Buffer
+	require.NoError(t, s.ToManifest(context.Background(), &buf, nil))
+
+	m, err := NewManifestFromReader(&buf)
+	require.NoError(t, err)
+	require.Len(t, m.Files(), 1)
+	assert.Equal(t, "one.txt", m.Files()[0].GetPath())
+}
+
 func TestScannerEnumeratePaths(t *testing.T) {
 	t.Parallel()