Compare --require-signature with the key that signed (closes #167)

check and fetch --require-signature compared the required fingerprint
with the first key in the manifest's embedded public key block, while
gpg accepted a good signature by any key in that block.

Loading a signed manifest now refuses one whose embedded block holds
more than one primary key, counted as gpg reads the block, or whose
signer field is not the primary key fingerprint gpg reports for the
signature. --require-signature compares with the signer field, which
loading has checked. Signing names and embeds the key gpg reports it
signed with, so a key ID matching several keys still writes a manifest
that loads. docs/FORMAT.md states what a verifier checks.

Model: opus-5-5

mfer/gpg.go

@@ -41,19 +41,15 @@ const (
 	// fields in a gpg fingerprint record (the fingerprint is field 10).
 	gpgFingerprintMinFields = 10
 
-	// gpgPrimaryKeyRecord starts the record of each primary public key in
-	// gpg --with-colons output.
-	gpgPrimaryKeyRecord = "pub:"
-
-	// gpgValidSigStatus starts the status line gpg --verify writes for a
-	// good signature. Its last field is the fingerprint of the primary key
-	// that made the signature.
-	gpgValidSigStatus = "[GNUPG:] VALIDSIG "
+	// gpgStatusPrefix starts each status line gpg writes to the file
+	// descriptor named by --status-fd.
+	gpgStatusPrefix = "[GNUPG:]"
 
 	// gpg option names used from more than one call site.
-	gpgOptArmor   = "--armor"
-	gpgOptHomedir = "--homedir"
-	gpgOptVerify  = "--verify"
+	gpgOptArmor    = "--armor"
+	gpgOptHomedir  = "--homedir"
+	gpgOptStatusFD = "--status-fd"
+	gpgOptVerify   = "--verify"
 )
 
 var (
@@ -63,6 +59,8 @@ var (
 		"embedded public key block must hold exactly one key")
 	errNotOneGoodSignature = errors.New(
 		"gpg did not report exactly one good signature")
+	errSigningKeyNotReported = errors.New(
+		"gpg did not report the key that made the signature")
 )
 
 // GPGKeyID represents a GPG key identifier (fingerprint or key ID).
@@ -148,53 +146,51 @@ func parseFingerprint(colonOutput string) (string, bool) {
 	return "", false
 }
 
-// countPrimaryKeys returns the number of primary keys in gpg --with-colons
-// key listing output.
-func countPrimaryKeys(colonOutput string) int {
-	count := 0
-
-	for line := range strings.SplitSeq(colonOutput, "\n") {
-		if strings.HasPrefix(line, gpgPrimaryKeyRecord) {
-			count++
-		}
-	}
-
-	return count
-}
-
-// parseSigningKey returns the fingerprint of the primary key that made a
-// signature, the last field of the VALIDSIG line in gpg --verify status
-// output, or ok=false unless there is exactly one such line.
-func parseSigningKey(statusOutput string) (string, bool) {
-	var fingerprints []string
+// parseStatusLine returns the arguments of the status line for keyword in
+// gpg --status-fd output, or ok=false unless there is exactly one such line
+// and it has arguments.
+func parseStatusLine(statusOutput, keyword string) ([]string, bool) {
+	var found [][]string
 
 	for line := range strings.SplitSeq(statusOutput, "\n") {
-		if strings.HasPrefix(line, gpgValidSigStatus) {
-			fields := strings.Fields(line)
-			fingerprints = append(fingerprints, fields[len(fields)-1])
+		fields := strings.Fields(line)
+		if len(fields) > 2 && fields[0] == gpgStatusPrefix && fields[1] == keyword {
+			found = append(found, fields[2:])
 		}
 	}
 
-	if len(fingerprints) != 1 {
-		return "", false
+	if len(found) != 1 {
+		return nil, false
 	}
 
-	return fingerprints[0], true
+	return found[0], true
 }
 
-// gpgSign creates a detached signature of the data using the specified key.
-// Returns the armored detached signature.
-func gpgSign(ctx context.Context, data []byte, keyID GPGKeyID) ([]byte, error) {
+// gpgSign creates an armored detached signature of data with the key gpg
+// picks for keyID, and returns it with the fingerprint of the key that made
+// it, which is a subkey's when gpg signed with a subkey.
+func gpgSign(
+	ctx context.Context, data []byte, keyID GPGKeyID,
+) ([]byte, string, error) {
+	// The signature goes to stdout, so the status lines go to stderr.
 	stdout, stderr, err := runGPG(ctx, bytes.NewReader(data),
 		"--detach-sign",
 		gpgOptArmor,
+		gpgOptStatusFD, "2",
 		"--local-user", string(keyID),
 	)
 	if err != nil {
-		return nil, fmt.Errorf("gpg sign failed: %w: %s", err, stderr.String())
+		return nil, "", fmt.Errorf("gpg sign failed: %w: %s", err, stderr.String())
 	}
 
-	return stdout.Bytes(), nil
+	// The last argument of SIG_CREATED is the fingerprint of the key that
+	// made the signature.
+	created, ok := parseStatusLine(stderr.String(), "SIG_CREATED")
+	if !ok {
+		return nil, "", fmt.Errorf("%w: %s", errSigningKeyNotReported, stderr.String())
+	}
+
+	return stdout.Bytes(), created[len(created)-1], nil
 }
 
 // gpgExportPublicKey exports the public key for the specified key ID.
@@ -233,12 +229,14 @@ func gpgGetKeyFingerprint(ctx context.Context, keyID GPGKeyID) ([]byte, error) {
 	return []byte(fpr), nil
 }
 
-// gpgImportOneKey imports the public key in pubKeyFile into the keyring in
-// gpgHome, which must then hold exactly one primary key.
+// gpgImportOneKey imports the public key block in pubKeyFile into the
+// keyring in gpgHome. The block must hold exactly one primary key.
 func gpgImportOneKey(ctx context.Context, gpgHome, pubKeyFile string) error {
-	// Import the public key into the keyring
-	_, importStderr, err := runGPG(ctx, nil,
-		gpgArgs([]string{gpgOptHomedir, gpgHome, "--import"}, pubKeyFile)...,
+	// --status-fd 1 sends gpg's status lines to stdout, which importing
+	// otherwise leaves empty; its messages go to stderr.
+	importStdout, importStderr, err := runGPG(ctx, nil,
+		gpgArgs([]string{gpgOptHomedir, gpgHome, gpgOptStatusFD, "1", "--import"},
+			pubKeyFile)...,
 	)
 	if err != nil {
 		return fmt.Errorf(
@@ -246,21 +244,16 @@ func gpgImportOneKey(ctx context.Context, gpgHome, pubKeyFile string) error {
 		)
 	}
 
-	// List keys to count them
-	listStdout, listStderr, err := runGPG(ctx, nil,
-		gpgOptHomedir, gpgHome,
-		"--with-colons",
-		"--list-keys",
-	)
-	if err != nil {
-		return fmt.Errorf(
-			"failed to list keys: %w: %s", err, listStderr.String(),
-		)
+	// The first argument of IMPORT_RES counts the primary keys gpg read
+	// from the block, those it then skipped (one with no user ID, for
+	// example) included.
+	result, ok := parseStatusLine(importStdout.String(), "IMPORT_RES")
+	if !ok {
+		return fmt.Errorf("%w, gpg reported no count", errSigningKeyCount)
 	}
 
-	keys := countPrimaryKeys(listStdout.String())
-	if keys != 1 {
-		return fmt.Errorf("%w, found %d", errSigningKeyCount, keys)
+	if result[0] != "1" {
+		return fmt.Errorf("%w, found %s", errSigningKeyCount, result[0])
 	}
 
 	return nil
@@ -318,7 +311,7 @@ func gpgVerify(ctx context.Context, data, signature, pubKey []byte) (string, err
 	// --status-fd 1 sends gpg's status lines to stdout, which verifying a
 	// detached signature otherwise leaves empty; its messages go to stderr.
 	verifyStdout, verifyStderr, err := runGPG(ctx, nil,
-		gpgArgs([]string{gpgOptHomedir, tmpDir, "--status-fd", "1", gpgOptVerify},
+		gpgArgs([]string{gpgOptHomedir, tmpDir, gpgOptStatusFD, "1", gpgOptVerify},
 			sigFile, dataFile)...,
 	)
 	if err != nil {
@@ -327,10 +320,13 @@ func gpgVerify(ctx context.Context, data, signature, pubKey []byte) (string, err
 		)
 	}
 
-	fingerprint, ok := parseSigningKey(verifyStdout.String())
+	// gpg writes a VALIDSIG line for each good signature. Its first
+	// argument is the fingerprint of the key that made the signature,
+	// which may be a subkey; its last is that of the primary key.
+	valid, ok := parseStatusLine(verifyStdout.String(), "VALIDSIG")
 	if !ok {
 		return "", errNotOneGoodSignature
 	}
 
-	return fingerprint, nil
+	return valid[len(valid)-1], nil
 }

mfer/gpg_test.go

@@ -37,7 +37,7 @@ func testGPGEnv(t *testing.T) (GPGKeyID, string) {
 	// Create temporary GPG home directory (0700 by default)
 	gpgHome := t.TempDir()
 
-	genTestKey(t, gpgHome)
+	genTestKey(t, gpgHome, testKeyParams)
 
 	ctx, cancel := context.WithTimeout(context.Background(), gpgTimeout)
 	defer cancel()
@@ -73,21 +73,20 @@ func testGPGEnv(t *testing.T) (GPGKeyID, string) {
 	return GPGKeyID(keyID), gpgHome
 }
 
+// testKeyParams are the gpg key generation parameters of an RSA key that
+// signs and does not expire.
+const testKeyParams = "Key-Type: RSA\nKey-Length: 2048\nExpire-Date: 0\n"
+
 // genTestKey generates a key with no passphrase for
-// "MFER Test Key <test@mfer.test>" in gpgHome, which may already hold one.
-func genTestKey(t *testing.T, gpgHome string) {
+// "MFER Test Key <test@mfer.test>" from the gpg key generation parameters
+// keyParams in gpgHome, which may already hold one.
+func genTestKey(t *testing.T, gpgHome, keyParams string) {
 	t.Helper()
 
-	keyParams := `%no-protection
-Key-Type: RSA
-Key-Length: 2048
-Name-Real: MFER Test Key
-Name-Email: test@mfer.test
-Expire-Date: 0
-%commit
-`
+	params := "%no-protection\n" + keyParams +
+		"Name-Real: MFER Test Key\nName-Email: test@mfer.test\n%commit\n"
 	paramsFile := filepath.Join(gpgHome, "key-params")
-	require.NoError(t, os.WriteFile(paramsFile, []byte(keyParams), 0o600))
+	require.NoError(t, os.WriteFile(paramsFile, []byte(params), 0o600))
 
 	ctx, cancel := context.WithTimeout(context.Background(), gpgTimeout)
 	defer cancel()
@@ -144,8 +143,9 @@ func TestGPGSign(t *testing.T) {
 	t.Setenv("GNUPGHOME", gpgHome)
 
 	data := []byte("test data to sign")
-	sig, err := gpgSign(context.Background(), data, keyID)
+	sig, signingKey, err := gpgSign(context.Background(), data, keyID)
 	require.NoError(t, err)
+	assert.Equal(t, string(keyID), signingKey)
 	assert.NotEmpty(t, sig)
 	assert.Contains(t, string(sig), "-----BEGIN PGP SIGNATURE-----")
 	assert.Contains(t, string(sig), "-----END PGP SIGNATURE-----")
@@ -216,7 +216,7 @@ func TestGPGSignInvalidKey(t *testing.T) {
 	t.Setenv("GNUPGHOME", gpgHome)
 
 	data := []byte("test data")
-	_, err := gpgSign(context.Background(), data,
+	_, _, err := gpgSign(context.Background(), data,
 		GPGKeyID("NONEXISTENT_KEY_ID_12345"))
 	assert.Error(t, err)
 }
@@ -306,7 +306,7 @@ func TestGPGVerify(t *testing.T) {
 	t.Setenv("GNUPGHOME", gpgHome)
 
 	data := []byte("test data to sign and verify")
-	sig, err := gpgSign(context.Background(), data, keyID)
+	sig, _, err := gpgSign(context.Background(), data, keyID)
 	require.NoError(t, err)
 
 	pubKey, err := gpgExportPublicKey(context.Background(), keyID)
@@ -323,7 +323,7 @@ func TestGPGVerifyInvalidSignature(t *testing.T) {
 	t.Setenv("GNUPGHOME", gpgHome)
 
 	data := []byte("test data to sign")
-	sig, err := gpgSign(context.Background(), data, keyID)
+	sig, _, err := gpgSign(context.Background(), data, keyID)
 	require.NoError(t, err)
 
 	pubKey, err := gpgExportPublicKey(context.Background(), keyID)
@@ -340,7 +340,7 @@ func TestGPGVerifyBadPublicKey(t *testing.T) {
 	t.Setenv("GNUPGHOME", gpgHome)
 
 	data := []byte("test data")
-	sig, err := gpgSign(context.Background(), data, keyID)
+	sig, _, err := gpgSign(context.Background(), data, keyID)
 	require.NoError(t, err)
 
 	// Try to verify with invalid public key - should fail
@@ -439,6 +439,70 @@ func TestManifestRefusesSecondEmbeddedKey(t *testing.T) {
 	require.ErrorIs(t, err, errSigningKeyCount)
 }
 
+// TestManifestRefusesSecondEmbeddedKeyWithoutUserID loads a manifest whose
+// embedded public key block holds, before the key that signed it, another
+// key with its user ID removed, which gpg skips on import. Loading must
+// refuse it: the block holds two keys.
+func TestManifestRefusesSecondEmbeddedKeyWithoutUserID(t *testing.T) {
+	otherKey, otherHome := testGPGEnv(t)
+	t.Setenv("GNUPGHOME", otherHome)
+
+	// Keeping only the user IDs that match "nobody" exports none.
+	otherPubKey, _, err := runGPG(context.Background(), nil,
+		gpgArgs([]string{
+			"--export", gpgOptArmor, "--export-filter", "keep-uid=uid = nobody",
+		}, string(otherKey))...)
+	require.NoError(t, err)
+
+	keyID, gpgHome := testGPGEnv(t)
+	t.Setenv("GNUPGHOME", gpgHome)
+
+	manifest := rewriteOuter(t, signedTestManifest(t, keyID),
+		func(outer *MFFileOuter) {
+			outer.SigningPubKey = slices.Concat(
+				otherPubKey.Bytes(), outer.GetSigningPubKey())
+		})
+
+	_, err = NewManifestFromReader(bytes.NewReader(manifest))
+	require.ErrorIs(t, err, errSigningKeyCount)
+}
+
+// TestManifestRefusesTwoSignatures loads a manifest whose signature field
+// holds its good signature twice. Loading must refuse it.
+func TestManifestRefusesTwoSignatures(t *testing.T) {
+	keyID, gpgHome := testGPGEnv(t)
+	t.Setenv("GNUPGHOME", gpgHome)
+
+	manifest := rewriteOuter(t, signedTestManifest(t, keyID),
+		func(outer *MFFileOuter) {
+			outer.Signature = slices.Concat(
+				outer.GetSignature(), outer.GetSignature())
+		})
+
+	_, err := NewManifestFromReader(bytes.NewReader(manifest))
+	require.ErrorIs(t, err, errNotOneGoodSignature)
+}
+
+// TestManifestSignedWithSubkey signs with a key whose primary key can only
+// certify, so gpg signs with its signing subkey. The manifest must load,
+// with the primary key's fingerprint as signer.
+func TestManifestSignedWithSubkey(t *testing.T) {
+	gpgHome := t.TempDir()
+	t.Setenv("GNUPGHOME", gpgHome)
+
+	genTestKey(t, gpgHome, "Key-Type: RSA\nKey-Length: 2048\nKey-Usage: cert\n"+
+		"Subkey-Type: RSA\nSubkey-Length: 2048\nSubkey-Usage: sign\n"+
+		"Expire-Date: 0\n")
+
+	primary, err := gpgGetKeyFingerprint(context.Background(), "test@mfer.test")
+	require.NoError(t, err)
+
+	m, err := NewManifestFromReader(bytes.NewReader(
+		signedTestManifest(t, GPGKeyID("test@mfer.test"))))
+	require.NoError(t, err)
+	assert.Equal(t, primary, m.pbOuter.GetSigner())
+}
+
 // TestManifestRefusesSignerOtherThanSigningKey loads a manifest whose
 // signer field names a key other than the one that made the signature.
 func TestManifestRefusesSignerOtherThanSigningKey(t *testing.T) {
@@ -459,7 +523,7 @@ func TestManifestRefusesSignerOtherThanSigningKey(t *testing.T) {
 // signed it, or loading refuses it.
 func TestBuilderSigningKeyIDMatchingTwoKeys(t *testing.T) {
 	_, gpgHome := testGPGEnv(t)
-	genTestKey(t, gpgHome)
+	genTestKey(t, gpgHome, testKeyParams)
 	t.Setenv("GNUPGHOME", gpgHome)
 
 	manifest := signedTestManifest(t, GPGKeyID("test@mfer.test"))
@@ -468,6 +532,28 @@ func TestBuilderSigningKeyIDMatchingTwoKeys(t *testing.T) {
 	require.NoError(t, err)
 }
 
+// TestBuilderSigningUserIDWithExpiredFirstKey signs with a user ID whose
+// first key in the keyring has expired. gpg signs with the other key for
+// that user ID, and the manifest must name and embed that key.
+func TestBuilderSigningUserIDWithExpiredFirstKey(t *testing.T) {
+	gpgHome := t.TempDir()
+	t.Setenv("GNUPGHOME", gpgHome)
+
+	// Made in 2020 and valid for one day.
+	genTestKey(t, gpgHome, "Key-Type: RSA\nKey-Length: 2048\n"+
+		"Creation-Date: 20200101T000000\nExpire-Date: 1d\n")
+
+	expired, err := gpgGetKeyFingerprint(context.Background(), "test@mfer.test")
+	require.NoError(t, err)
+
+	genTestKey(t, gpgHome, testKeyParams)
+
+	m, err := NewManifestFromReader(bytes.NewReader(
+		signedTestManifest(t, GPGKeyID("test@mfer.test"))))
+	require.NoError(t, err)
+	assert.NotEqual(t, expired, m.pbOuter.GetSigner())
+}
+
 func TestBuilderWithoutSigning(t *testing.T) {
 	t.Parallel()
 
@@ -521,7 +607,7 @@ func TestGPGTimeoutKillsGPG(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
 	defer cancel()
 
-	_, err := gpgSign(ctx, []byte("data"), GPGKeyID("any"))
+	_, _, err := gpgSign(ctx, []byte("data"), GPGKeyID("any"))
 	require.ErrorIs(t, err, context.DeadlineExceeded)
 	assert.Contains(t, err.Error(), "gpg sign failed: gpg timed out")
 }
@@ -546,7 +632,7 @@ func TestGPGCancelWhenChildHoldsOutput(t *testing.T) {
 	signErr := make(chan error, 1)
 
 	go func() {
-		_, err := gpgSign(ctx, []byte("data"), GPGKeyID("any"))
+		_, _, err := gpgSign(ctx, []byte("data"), GPGKeyID("any"))
 		signErr <- err
 	}()
 

mfer/serialize.go

@@ -143,29 +143,31 @@ func (m *manifest) generateOuter(ctx context.Context) error {
 }
 
 // signOuter signs the outer message with the configured GPG key and
-// embeds the signature, signer fingerprint, and public key. It signs with
-// and exports the key by its fingerprint, so that a key ID matching more
-// than one key cannot embed a key other than the one that signed.
+// embeds the signature, signer fingerprint, and public key. The signer
+// and public key are those of the key gpg reports it signed with, so that
+// a key ID matching more than one key cannot name or embed another key.
 func (m *manifest) signOuter(ctx context.Context) error {
 	sigString, err := m.signatureString()
 	if err != nil {
 		return fmt.Errorf("failed to generate signature string: %w", err)
 	}
 
-	fingerprint, err := gpgGetKeyFingerprint(ctx, m.signingOptions.KeyID)
-	if err != nil {
-		return fmt.Errorf("failed to get key fingerprint: %w", err)
-	}
-
-	m.pbOuter.Signer = fingerprint
-
-	sig, err := gpgSign(ctx, []byte(sigString), GPGKeyID(fingerprint))
+	sig, signingKey, err := gpgSign(ctx, []byte(sigString), m.signingOptions.KeyID)
 	if err != nil {
 		return fmt.Errorf("failed to sign manifest: %w", err)
 	}
 
 	m.pbOuter.Signature = sig
 
+	// Listing the signing key, a subkey's included, puts its primary key's
+	// fingerprint first.
+	fingerprint, err := gpgGetKeyFingerprint(ctx, GPGKeyID(signingKey))
+	if err != nil {
+		return fmt.Errorf("failed to get key fingerprint: %w", err)
+	}
+
+	m.pbOuter.Signer = fingerprint
+
 	pubKey, err := gpgExportPublicKey(ctx, GPGKeyID(fingerprint))
 	if err != nil {
 		return fmt.Errorf("failed to export public key: %w", err)