Report only symlinks to regular files as extra files in check (closes #192)

gen never lists a symlink to a directory or a broken one, and lists a
symlink to a regular file only with --follow-symlinks. check's search for
extra files reported every symlink, so on a tree holding a directory
symlink no manifest gen writes could pass check --no-extra-files. The
search now resolves a symlink and reports an entry only if it is a
regular file, which also covers the pipes, sockets and devices it already
left out. The README's check entry says what is reported.

Judgement call: one regular-file check replaces the separate test for
pipes, sockets and devices, so a file of unknown type is not reported
either; gen does not list one.

Model: opus-5-5

README.md

@@ -289,9 +289,12 @@ are now tracked only in the [issues](https://git.eeqj.de/sneak/mfer/issues).
       else the directory holding the manifest, or the current directory for a
       manifest given by URL. So `mfer check /media/drive` checks a drive against
       the `index.mf` at its root, from any directory
-    - warns about each file under the base directory that the manifest does not
-      list, hidden files included; with `--no-extra-files` each one is a failure
-      instead
+    - warns about each regular file under the base directory that the manifest
+      does not list, hidden files included, and each symlink there to a regular
+      file, which gen lists only with `--follow-symlinks`; with
+      `--no-extra-files` each one is a failure instead. A named pipe, socket or
+      device, a symlink to a directory and a broken symlink are never reported,
+      since gen cannot list them
 - `mfer freshen` / `mfer freshen .`
     - rewrites `index.mf` to list the files now under the directory holding it,
       or under the one `--base` names, hashing only the files that are new or

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          |
-| `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; 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)                               |
 
 ## File Mode
 
@@ -174,6 +174,10 @@ 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

@@ -960,6 +960,53 @@ func TestCheckWarnsAboutUnlistedFiles(t *testing.T) {
 	}
 }
 
+// TestCheckReportsUnlistedSymlinkToFileOnly makes a tree holding a symlink
+// to a directory, one to a file and a broken one. gen lists the symlink to
+// the file only with --follow-symlinks, and never lists the other two, so
+// check --no-extra-files passes on the manifest gen --follow-symlinks writes
+// and fails only on the symlink to the file on the one plain gen writes.
+func TestCheckReportsUnlistedSymlinkToFileOnly(t *testing.T) {
+	t.Parallel()
+
+	for name, tc := range map[string]struct {
+		genFlags []string
+		exitCode int
+		extras   []string // what check reports as not in the manifest
+	}{
+		"gen --follow-symlinks": {[]string{"--follow-symlinks"}, 0, nil},
+		"plain gen":             {nil, 1, []string{"filelink"}},
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			dir := t.TempDir()
+			fs := afero.NewOsFs()
+			writeTestTree(t, fs, dir)
+			require.NoError(t, os.Symlink(testSubdir, filepath.Join(dir, "dirlink")))
+			require.NoError(t, os.Symlink(testFileTxt, filepath.Join(dir, "filelink")))
+			require.NoError(t,
+				os.Symlink("nowhere", filepath.Join(dir, "brokenlink")))
+
+			opts := testOpts(slices.Concat(
+				[]string{testApp, cmdGenerate, "-q"}, tc.genFlags, []string{dir},
+			), fs)
+			require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts))
+
+			opts = testOpts([]string{testApp, cmdCheck, testFlagNoExtra, dir}, fs)
+			exitCode := runCLI(opts)
+			stderr := testStderr(t, opts)
+
+			assert.Equal(t, tc.exitCode, exitCode, "stderr: %s", stderr)
+			assert.Equal(t, len(tc.extras),
+				strings.Count(stderr, "not in manifest"), stderr)
+
+			for _, extra := range tc.extras {
+				assert.Contains(t, stderr, "EXTRA: "+extra)
+			}
+		})
+	}
+}
+
 // TestCheckNeverReportsManifest keeps the manifest inside the checked tree,
 // under several names and path spellings, and names the tree both directly
 // and through a symlink: the manifest is never reported, even under

internal/cli/freshen.go

@@ -28,9 +28,15 @@ const (
 	scanProgressInterval = 100
 )
 
-// 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")
+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")
+
+	// errEntryMissingHash indicates a manifest entry that carries no hash
+	// to carry forward.
+	errEntryMissingHash = errors.New("manifest entry has no hash")
+)
 
 // FreshenStatus contains progress information for the freshen operation.
 type FreshenStatus struct {
@@ -644,6 +650,8 @@ 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 {
@@ -651,7 +659,7 @@ func addExistingToBuilder(b *mfer.Builder, entry *mfer.MFFilePath) error {
 	}
 
 	if len(entry.GetHashes()) == 0 {
-		return nil
+		return fmt.Errorf("%w: %s", errEntryMissingHash, entry.GetPath())
 	}
 
 	err := b.AddFileWithHash(mfer.RelFilePath(entry.GetPath()),

internal/cli/freshen_test.go

@@ -471,6 +471,24 @@ 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.

mfer/checker.go

@@ -264,11 +264,12 @@ func (c *Checker) Check(
 	return nil
 }
 
-// FindExtraFiles walks the filesystem and reports files not in the manifest,
-// hidden files and directories included. The manifest file itself is not
-// reported. Anything the search cannot read, such as a directory that cannot
-// be listed, is reported with StatusError and the search goes on. Results are
-// sent to the results channel. The channel is closed when done.
+// FindExtraFiles walks the filesystem and reports the regular files, and
+// symlinks to them, that are not in the manifest, hidden files and
+// directories included. The manifest file itself is not reported. Anything
+// the search cannot read, such as a directory that cannot be listed, is
+// reported with StatusError and the search goes on. Results are sent to the
+// results channel. The channel is closed when done.
 func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) error {
 	if results != nil {
 		defer close(results)
@@ -318,11 +319,6 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err
 			return nil
 		}
 
-		// gen never lists a named pipe, a socket or a device.
-		if info.Mode()&(os.ModeNamedPipe|os.ModeSocket|os.ModeDevice) != 0 {
-			return nil
-		}
-
 		// A symlink is compared by what it points to, so a manifest reached
 		// through one is not reported either.
 		if info.Mode()&os.ModeSymlink != 0 {
@@ -332,6 +328,13 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err
 			}
 		}
 
+		// gen lists regular files and symlinks to them only, so a named
+		// pipe, a socket, a device, a symlink to a directory and a broken
+		// symlink are never reported.
+		if !info.Mode().IsRegular() {
+			return nil
+		}
+
 		// Skip the manifest file itself, however its path is spelled
 		if os.SameFile(info, c.manifestInfo) {
 			return nil

mfer/deserialize.go

@@ -27,6 +27,7 @@ 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")
@@ -293,7 +294,8 @@ 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.
+	// file against both entries. So is an entry with no hash: there is
+	// nothing to check the file against.
 	seen := make(map[string]bool, len(m.pbInner.GetFiles()))
 
 	for _, f := range m.pbInner.GetFiles() {
@@ -302,6 +304,10 @@ 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,6 +165,24 @@ 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
@@ -229,6 +247,8 @@ 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/openpgp.go

@@ -8,6 +8,7 @@ import (
 	"io"
 	"slices"
 	"strings"
+	"time"
 
 	"github.com/ProtonMail/go-crypto/openpgp"
 	"github.com/ProtonMail/go-crypto/openpgp/armor"
@@ -242,10 +243,10 @@ func fingerprint(key *openpgp.Entity) string {
 	return strings.ToUpper(hex.EncodeToString(key.PrimaryKey.Fingerprint))
 }
 
-// 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.
+// 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.
 func verifySignature(data, signature, pubKey []byte) (string, error) {
 	key, err := readOneKey(pubKey, "embedded public key block", true)
 	if err != nil {
@@ -266,10 +267,16 @@ func verifySignature(data, signature, pubKey []byte) (string, error) {
 		return "", fmt.Errorf("%w, found %d", errNotOneSignature, sigs)
 	}
 
-	_, err = openpgp.CheckDetachedSignature(openpgp.EntityList{key},
+	sig, _, err := openpgp.VerifyDetachedSignature(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.
+	// 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
+	}
+
 	if err != nil && !errors.Is(err, pgperrors.ErrKeyExpired) {
 		return "", fmt.Errorf("verify signature: %w", err)
 	}

mfer/openpgp_test.go

@@ -18,6 +18,7 @@ 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"
@@ -416,8 +417,10 @@ func TestVerifySignature(t *testing.T) {
 	assert.Error(t, err)
 }
 
-// TestVerifySignatureKeyExpiredSince verifies a signature made in 2020 by
-// a key that expired a day after it was made. The signature is still good.
+// 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.
 func TestVerifySignatureKeyExpiredSince(t *testing.T) {
 	t.Parallel()
 
@@ -429,9 +432,14 @@ func TestVerifySignatureKeyExpiredSince(t *testing.T) {
 	key := newTestKey(t, config)
 	data := []byte("signed in 2020")
 
-	var sig bytes.Buffer
+	var sig, expiringSig 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)
@@ -439,6 +447,9 @@ 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) {