Refuse an expired signature whose key has expired too (closes #193)

verifySignature accepts a signature whose key has expired, since a
manifest outlives its key. The OpenPGP library stops at the expired key
without checking the signature's own expiry time, so a signature made
with one was refused while the key was live and accepted once the key
had expired as well. verifySignature now gets the signature packet from
openpgp.VerifyDetachedSignature and refuses it when it has expired.
docs/FORMAT.md Signature Scheme states the rule.

Model: opus-5-5

README.md

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

docs/FORMAT.md

@@ -75,15 +75,15 @@ After decompressing `innerMessage`, the result is a serialized `MFFile`
 
 Each file entry contains:
 
-| Field      | Number | Type                      | Description                         |
+| Field      | Number | Type                      | Description                                                   |
-| ---------- | ------ | ------------------------- | ----------------------------------- |
+| ---------- | ------ | ------------------------- | ------------------------------------------------------------- |
-| `path`     | 1      | string                    | Relative file path (see Path Rules) |
+| `path`     | 1      | string                    | Relative file path (see Path Rules)                           |
-| `size`     | 2      | int64                     | File size in bytes                  |
+| `size`     | 2      | int64                     | File size in bytes                                            |
-| `hashes`   | 3      | repeated `MFFileChecksum` | At least one hash required          |
+| `hashes`   | 3      | repeated `MFFileChecksum` | At least one hash required; readers reject an entry with none |
-| `mimeType` | 301    | string (optional)         | MIME type                           |
+| `mimeType` | 301    | string (optional)         | MIME type                                                     |
-| `mtime`    | 302    | Timestamp (optional)      | Modification time                   |
+| `mtime`    | 302    | Timestamp (optional)      | Modification time                                             |
-| `ctime`    | 303    | Timestamp (optional)      | Change time (inode metadata change) |
+| `ctime`    | 303    | Timestamp (optional)      | Change time (inode metadata change)                           |
-| `mode`     | 304    | uint32                    | Permission bits (see File Mode)     |
+| `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/freshen.go

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

internal/cli/signing_test.go

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

mfer/deserialize.go

@@ -27,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/manifest.go

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

mfer/openpgp.go

@@ -8,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
+// verifySignature checks that signature is one good, unexpired OpenPGP
-// over data, made by the one primary key in pubKey or one of its subkeys,
+// signature over data, made by the one primary key in pubKey or one of its
-// and returns that primary key's fingerprint. signature and pubKey may each
+// subkeys, and returns that primary key's fingerprint. signature and pubKey
-// be armored or binary.
+// 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"
@@ -175,25 +176,6 @@ func TestBuilderWithSigning(t *testing.T) {
 		"-----BEGIN PGP PUBLIC KEY BLOCK-----")
 }
 
-// TestManifestSigner checks that Signer gives the signing key's
-// fingerprint for a signed manifest, and nil once the signature is taken
-// out, although the signer field is still there.
-func TestManifestSigner(t *testing.T) {
-	t.Parallel()
-
-	key := newTestKey(t, nil)
-	data := signedTestManifest(t, &SigningOptions{SecretKey: armoredSecretKeys(t, key)})
-
-	signed, err := NewManifestFromReader(bytes.NewReader(data))
-	require.NoError(t, err)
-	assert.Equal(t, fingerprint(key), string(signed.Signer()))
-
-	unsigned, err := NewManifestFromReader(bytes.NewReader(
-		rewriteOuter(t, data, func(outer *MFFileOuter) { outer.Signature = nil })))
-	require.NoError(t, err)
-	assert.Nil(t, unsigned.Signer())
-}
-
 func TestScannerWithSigning(t *testing.T) {
 	t.Parallel()
 
@@ -435,8 +417,10 @@ func TestVerifySignature(t *testing.T) {
 	assert.Error(t, err)
 }
 
-// TestVerifySignatureKeyExpiredSince verifies a signature made in 2020 by
+// TestVerifySignatureKeyExpiredSince verifies two signatures made in 2020
-// a key that expired a day after it was made. The signature is still good.
+// 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()
 
@@ -448,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)
@@ -458,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) {