Sign and verify manifests in Go with OpenPGP instead of running gpg (closes #181)

mfer ran the gpg binary to sign, export keys and verify, so it failed
wherever gpg is missing. It now uses github.com/ProtonMail/go-crypto/openpgp.
--sign-key and MFER_SIGN_KEY name a file holding one version 4 OpenPGP
secret key; a protected key's passphrase comes from
MFER_SIGN_KEY_PASSPHRASE or a terminal prompt. gen and freshen check that
the key can sign before they read any file. Verification keeps the rules
of the --require-signature fix: one primary key in the embedded block,
counted from its packets, exactly one signature, made by that key or a
subkey, and signer equal to its fingerprint. The embedded block may hold
no DSA key and no secret key, and an armored field must be one
well-formed block.

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
 

go.mod

@@ -4,6 +4,7 @@ go 1.27.1
 
 require (
 	github.com/ProtonMail/go-crypto v1.5.2
+	github.com/creack/pty v1.1.25-0.20260601142114-9246436fffe8
 	github.com/davecgh/go-spew v1.1.1
 	github.com/dustin/go-humanize v1.1.0
 	github.com/klauspost/compress v1.20.1

go.sum

@@ -2,6 +2,8 @@ github.com/ProtonMail/go-crypto v1.5.2 h1:cucYnvqcY7UOXVD//mSyjeaPY0SSN3v5cDkYPx
 github.com/ProtonMail/go-crypto v1.5.2/go.mod h1:/RaSu30DaKO4RY+XdV/ACcCcZkGr7AhUIduq5sjzzCo=
 github.com/cloudflare/circl v1.6.3 h1:9GPOhQGF9MCYUeXyMYlqTR6a5gTrgR/fBLXvUgtVcg8=
 github.com/cloudflare/circl v1.6.3/go.mod h1:2eXP6Qfat4O/Yhh8BznvKnJ+uzEoTQ6jVKJRn81BiS4=
+github.com/creack/pty v1.1.25-0.20260601142114-9246436fffe8 h1:CY3gjC7naqYGLMiywvj3suPfa1i0p/QEr7o8ujxL/2M=
+github.com/creack/pty v1.1.25-0.20260601142114-9246436fffe8/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/dustin/go-humanize v1.1.0 h1:dbKTrvD0klcbBV/h4AWJdMuZogJACoMlvWIWZ5b2xWg=

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
-// characters.
+// fingerprintHexLen is the length in hex characters of the fingerprint of
+// 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

internal/cli/errmsg_test.go

@@ -5,11 +5,11 @@ import (
 	"bytes"
 	"context"
 	"encoding/hex"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"os"
 	"path/filepath"
-	"slices"
 	"strings"
 	"testing"
 
@@ -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
-// key, which is quick to make.
-func testSecretKey(t *testing.T, passphrase []byte) ([]byte, string) {
+// The key is protected by passphrase unless that is nil. config sets how
+// the key is made; without one it is an Ed25519 key, which is quick to
+// 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",
-		&packet.Config{Algorithm: packet.PubKeyAlgoEdDSA})
+	if config == nil {
+		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})
@@ -209,10 +215,26 @@ func manifestSignedByAnotherKey(
 	require.NoError(t, proto.Unmarshal(
 		signedManifest(t, files)[len(mfer.MAGIC):], outer))
 
-	// Armored blocks start on a line of their own; mfer, unlike gpg, ends
-	// one without a newline.
-	outer.SigningPubKey = slices.Concat(
-		required.GetSigningPubKey(), []byte("\n"), outer.GetSigningPubKey())
+	// One armored block holding both keys, as gpg --export --armor writes
+	// two keys.
+	var block bytes.Buffer
+
+	w, err := armor.Encode(&block, openpgp.PublicKeyType, nil)
+	require.NoError(t, err)
+
+	for _, key := range [][]byte{
+		required.GetSigningPubKey(), outer.GetSigningPubKey(),
+	} {
+		decoded, err := armor.Decode(bytes.NewReader(key))
+		require.NoError(t, err)
+
+		_, err = io.Copy(w, decoded.Body)
+		require.NoError(t, err)
+	}
+
+	require.NoError(t, w.Close())
+
+	outer.SigningPubKey = block.Bytes()
 	outer.Signer = required.GetSigner()
 
 	data, err := proto.Marshal(outer)

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

@@ -23,8 +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
-// MFER_SIGN_KEY_PASSPHRASE, or else from the terminal on stdin.
+// the file path, which must be able to sign. The passphrase of a protected
+// key comes from MFER_SIGN_KEY_PASSPHRASE, or else from the terminal on
+// 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 {
@@ -39,13 +40,19 @@ 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 {
-		return opts, nil
+	if protected {
+		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
+	// that cannot sign, or a wrong passphrase, stops them before they hash
+	// anything.
+	err = mfer.CheckSigningKey(opts)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("%s: %w", path, err)
 	}
 
 	return opts, nil

internal/cli/signing_test.go

@@ -2,15 +2,24 @@
 package cli
 
 import (
+	"bufio"
+	"io"
 	"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"
 	"github.com/stretchr/testify/require"
 )
 
-const testFlagSignKey = "--sign-key"
+const (
+	testFlagSignKey = "--sign-key"
+	testKeyFile     = "/key.asc"
+)
 
 // TestGenAndFreshenSignWithKeyFile runs gen, then freshen after a file is
 // added, with --sign-key naming a key file: one key with no passphrase and
@@ -26,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")
@@ -70,15 +79,15 @@ 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, "/key.asc", secretKey, 0o600))
+	require.NoError(t, afero.WriteFile(fs, testKeyFile, secretKey, 0o600))
 	require.NoError(t, fs.MkdirAll(testDir, 0o755))
 	writeTestFile(t, fs, testFile1, "hello")
 
 	opts := testOpts([]string{
-		testApp, cmdGenerate, "-q", testFlagSignKey, "/key.asc",
+		testApp, cmdGenerate, "-q", testFlagSignKey, testKeyFile,
 		"-o", testMF, testDir,
 	}, fs)
 	assert.Equal(t, 1, runCLI(opts))
@@ -89,3 +98,112 @@ func TestSignWithProtectedKeyNeedsPassphrase(t *testing.T) {
 	require.NoError(t, err)
 	assert.False(t, exists)
 }
+
+// TestSignWithKeyThatCannotSignFailsFirst runs gen on a directory and
+// freshen on a manifest, neither of which exists, with keys that cannot
+// sign: a protected key with a wrong MFER_SIGN_KEY_PASSPHRASE, a key that
+// expired in 2020, and a version 6 key. Each run must fail on the key: it
+// checks the key before it reads any file, so a missing file goes
+// unnoticed.
+func TestSignWithKeyThatCannotSignFailsFirst(t *testing.T) {
+	t.Setenv(envSignKeyPassphrase, "wrong")
+
+	wrongPassphrase, _ := testSecretKey(t, []byte("right"), nil)
+
+	made := time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC)
+	expired, _ := testSecretKey(t, nil, &packet.Config{
+		Algorithm:       packet.PubKeyAlgoEdDSA,
+		Time:            func() time.Time { return made },
+		KeyLifetimeSecs: uint32((24 * time.Hour).Seconds()),
+	})
+
+	version6, _ := testSecretKey(t, nil, &packet.Config{
+		Algorithm: packet.PubKeyAlgoEd25519,
+		V6Keys:    true,
+	})
+
+	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,
+	} {
+		fs := afero.NewMemMapFs()
+		require.NoError(t, afero.WriteFile(fs, testKeyFile, secretKey, 0o600))
+
+		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])
+		}
+	}
+}
+
+// TestGenAsksForPassphraseOnTerminal runs gen with a protected key, no
+// MFER_SIGN_KEY_PASSPHRASE, and a terminal as stdin and stderr. gen must
+// ask for the passphrase on stderr, and sign with what is typed after the
+// prompt.
+func TestGenAsksForPassphraseOnTerminal(t *testing.T) {
+	t.Setenv(envSignKeyPassphrase, "")
+
+	secretKey, fingerprint := testSecretKey(t, []byte("passphrase"), 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")
+
+	terminal, tty, err := pty.Open()
+	require.NoError(t, err)
+
+	t.Cleanup(func() { _ = terminal.Close() })
+
+	opts := testOpts([]string{
+		testApp, cmdGenerate, "-q", testFlagSignKey, keyFile,
+		"-o", manifestPath, root,
+	}, fs)
+	opts.Stdin = tty
+	opts.Stderr = tty
+
+	exitCode := make(chan int, 1)
+
+	go func() {
+		exitCode <- runCLI(opts)
+
+		// Once gen has ended, reading the terminal fails instead of
+		// waiting for a prompt that will not come.
+		_ = tty.Close()
+	}()
+
+	prompt := "Passphrase for " + keyFile + ": "
+	output := bufio.NewReader(terminal)
+	written := ""
+
+	for !strings.HasSuffix(written, prompt) {
+		b, err := output.ReadByte()
+		require.NoError(t, err, "gen wrote %q and no prompt", written)
+
+		written += string(b)
+	}
+
+	_, err = terminal.WriteString("passphrase\n")
+	require.NoError(t, err)
+
+	code := <-exitCode
+	rest, _ := io.ReadAll(output)
+	require.Equal(t, 0, code, "gen wrote %q", rest)
+
+	check := testOpts([]string{
+		testApp, cmdCheck, "-q",
+		"--" + flagRequireSignature, fingerprint, manifestPath,
+	}, fs)
+	require.Equal(t, 0, runCLI(check), testStderr(t, check))
+}

mfer/openpgp.go

@@ -16,24 +16,47 @@ import (
 )
 
 const (
-	// The tags of OpenPGP signature, secret key and public key packets
-	// (RFC 9580, section 5). Subkeys have tags of their own, so each
-	// secret or public key packet is one primary key.
-	signaturePacketTag = 2
-	secretKeyPacketTag = 5
-	publicKeyPacketTag = 6
+	// The tags of OpenPGP signature, key and subkey packets (RFC 9580,
+	// section 5). Subkeys have tags of their own, so each secret or public
+	// key packet is one primary key.
+	signaturePacketTag    = 2
+	secretKeyPacketTag    = 5
+	publicKeyPacketTag    = 6
+	secretSubkeyPacketTag = 7
+	publicSubkeyPacketTag = 14
 
-	// armorEnd starts the line that ends an armored block.
-	armorEnd = "-----END "
+	// 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.
+	armorBegin = "-----BEGIN "
+	armorEnd   = "-----END "
 )
 
 var (
-	errKeyCount     = errors.New("must hold exactly one key")
-	errNoSecretKey  = errors.New("signing key file holds no secret key")
+	errKeyCount    = errors.New("must hold exactly one 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.
@@ -47,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 {
@@ -57,6 +80,26 @@ func SecretKeyIsProtected(secretKey []byte) (bool, error) {
 	return isProtected(key), nil
 }
 
+// CheckSigningKey fails unless opts can sign now: opts.SecretKey must hold
+// one version 4 primary key with a secret key that may sign and has not
+// expired or been revoked, and opts.Passphrase must unlock it when it is
+// protected. It lets a caller find a key that cannot sign before it builds
+// a manifest.
+func CheckSigningKey(opts *SigningOptions) error {
+	key, err := readSigningKey(opts)
+	if err != nil {
+		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
 // opts.Passphrase if it is protected.
 func readSigningKey(opts *SigningOptions) (*openpgp.Entity, error) {
@@ -82,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
 	}
@@ -97,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.
-func readOneKey(data []byte, what string) (*openpgp.Entity, error) {
+// exactly one primary key and no DSA key or subkey, and when publicOnly no
+// 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)
@@ -113,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)
@@ -175,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
 	}
@@ -206,39 +278,40 @@ func verifySignature(data, signature, pubKey []byte) (string, error) {
 }
 
 // dearmor returns the binary OpenPGP data in data: data itself when it is
-// not armored, or else the bodies of all its armored blocks, one after
-// another. armor.Decode reads only the first block it finds, so dearmor
-// decodes the text after each block's END line in turn, and a second key
-// or signature after the first is read too.
+// not armored, or else the body of its armored block. Armored data must be
+// one block and nothing else: its first line is the only BEGIN line and
+// its last line the only END line, white space around them aside.
+// armor.Decode skips any text before a BEGIN line and reads only the first
+// block, so without this a second key or signature would go unseen.
 func dearmor(data []byte) ([]byte, error) {
-	var binary []byte
-
-	rest := data
-
-	for {
-		block, err := armor.Decode(bytes.NewReader(rest))
-		if errors.Is(err, io.EOF) {
-			break
-		}
-
-		if err != nil {
-			return nil, err
-		}
-
-		body, err := io.ReadAll(block.Body)
-		if err != nil {
-			return nil, err
-		}
-
-		binary = append(binary, body...)
-		_, rest, _ = bytes.Cut(rest, []byte(armorEnd))
-	}
-
-	if binary == nil {
+	if !bytes.Contains(data, []byte(armorBegin)) {
 		return data, nil
 	}
 
-	return binary, nil
+	text := bytes.TrimSpace(data)
+	lastLine := text[bytes.LastIndexByte(text, '\n')+1:]
+
+	if !bytes.HasPrefix(text, []byte(armorBegin)) ||
+		bytes.Count(text, []byte(armorBegin)) != 1 ||
+		!bytes.HasPrefix(lastLine, []byte(armorEnd)) ||
+		bytes.Count(text, []byte(armorEnd)) != 1 {
+		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, errMalformedArmor
+	}
+
+	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
@@ -264,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()
 
@@ -403,12 +497,12 @@ func TestManifestSignedByGPGLoads(t *testing.T) {
 }
 
 // TestManifestRefusesSecondEmbeddedKey loads manifests whose embedded
-// public key block holds another key besides the key that signed it: in an
-// armored block of its own, in the same armored block, as a secret key
-// with no user ID, which openpgp.ReadKeyRing skips, and written as a
-// subkey packet at the start of the block, which openpgp.ReadKeyRing reads
-// as a primary key. Loading must refuse each, although the signature is
-// good and the signer field names the key that made it.
+// public key block holds another key besides the key that signed it: as a
+// public key, as a secret key with no user ID, which openpgp.ReadKeyRing
+// skips, and written as a subkey packet at the start of the block, which
+// openpgp.ReadKeyRing reads as a primary key. Loading must refuse each,
+// although the signature is good and the signer field names the key that
+// made it.
 func TestManifestRefusesSecondEmbeddedKey(t *testing.T) {
 	t.Parallel()
 
@@ -424,12 +518,9 @@ func TestManifestRefusesSecondEmbeddedKey(t *testing.T) {
 	otherAsSubkey.PrimaryKey.IsSubkey = true
 
 	for name, block := range map[string][]byte{
-		"armored block of its own": joinArmored(
-			armoredPublicKeys(t, other), armoredPublicKeys(t, signer)),
-		"same armored block": armoredPublicKeys(t, other, signer),
-		"secret key without user ID": joinArmored(
-			armoredPublicKeys(t, signer), armoredSecretKeys(t, otherWithoutUserID)),
-		"subkey packet first": armoredPublicKeys(t, otherAsSubkey, signer),
+		"public key":                 armoredPublicKeys(t, other, signer),
+		"secret key without user ID": armoredSecretKeys(t, signer, otherWithoutUserID),
+		"subkey packet first":        armoredPublicKeys(t, otherAsSubkey, signer),
 	} {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
@@ -454,31 +545,175 @@ func TestManifestRefusesSecondEmbeddedKeyWithoutUserID(t *testing.T) {
 	other := newTestKey(t, nil)
 	other.Identities = map[string]*openpgp.Identity{}
 
-	manifest := rewriteOuter(t, signedTestManifest(t, testSigningOptions(t)),
+	signer := newTestKey(t, nil)
+
+	manifest := rewriteOuter(t, signedTestManifest(t,
+		&SigningOptions{SecretKey: armoredSecretKeys(t, signer)}),
 		func(outer *MFFileOuter) {
-			outer.SigningPubKey = joinArmored(
-				armoredPublicKeys(t, other), outer.GetSigningPubKey())
+			outer.SigningPubKey = armoredPublicKeys(t, other, signer)
 		})
 
 	_, err := NewManifestFromReader(bytes.NewReader(manifest))
 	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. Loading must refuse it.
+// holds its good signature twice, not armored. Loading must refuse it.
 func TestManifestRefusesTwoSignatures(t *testing.T) {
 	t.Parallel()
 
 	manifest := rewriteOuter(t, signedTestManifest(t, testSigningOptions(t)),
 		func(outer *MFFileOuter) {
-			outer.Signature = joinArmored(
-				outer.GetSignature(), outer.GetSignature())
+			sig, err := dearmor(outer.GetSignature())
+			require.NoError(t, err)
+
+			outer.Signature = slices.Concat(sig, sig)
 		})
 
 	_, err := NewManifestFromReader(bytes.NewReader(manifest))
 	require.ErrorIs(t, err, errNotOneSignature)
 }
 
+// TestManifestRefusesFieldNotOneArmoredBlock loads manifests whose
+// signature or embedded public key block holds its good armored block with
+// something else: a second armored block, text after the END line, or many
+// END lines before the block. Decoding the block once for each END line
+// before it would take time and memory that grow with the square of the
+// field's size. Loading must refuse each.
+func TestManifestRefusesFieldNotOneArmoredBlock(t *testing.T) {
+	t.Parallel()
+
+	manifest := signedTestManifest(t, testSigningOptions(t))
+
+	for name, change := range map[string]func([]byte) []byte{
+		"second armored block": func(block []byte) []byte {
+			return joinArmored(block, block)
+		},
+		"text after the END line": func(block []byte) []byte {
+			return slices.Concat(block, []byte("\nmore text\n"))
+		},
+		"END lines before the block": func(block []byte) []byte {
+			return slices.Concat(
+				[]byte(strings.Repeat(armorEnd+"\n", 1000)), block)
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			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, errNotOneArmoredBlock)
+			}
+		})
+	}
+}
+
 // TestManifestSignedWithSubkey signs with a key that has a signing subkey,
 // which signs in place of the primary key. The manifest must load, with
 // the primary key's fingerprint as signer.

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 {
-		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)
+	return s.EnumeratePaths(progress, inputPath)
 }
 
 // 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()