From 7f0bcfb228c50baaf0e31a8fa976e8e7d865c3f8 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 7 Oct 2026 09:24:07 +0000 Subject: [PATCH] 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, or whose signer field is not the fingerprint gpg reports for the signing key on its VALIDSIG status line. --require-signature compares with the signer field, which loading has checked. Signing now signs with and exports the key by its fingerprint, so a key ID matching two keys still writes a manifest that loads. docs/FORMAT.md states what a verifier checks. Model: opus-5-5 --- docs/FORMAT.md | 13 ++- internal/cli/check.go | 25 ++--- internal/cli/entry_test.go | 27 ++++++ internal/cli/errmsg_test.go | 52 ++++++++--- internal/cli/fetch.go | 10 +- internal/cli/fetch_test.go | 19 +++- mfer/checker.go | 20 ++-- mfer/deserialize.go | 16 +++- mfer/gpg.go | 177 +++++++++++++++++++++--------------- mfer/gpg_test.go | 154 +++++++++++++++++++++++++------ mfer/serialize.go | 20 ++-- 11 files changed, 364 insertions(+), 169 deletions(-) diff --git a/docs/FORMAT.md b/docs/FORMAT.md index 94ec656..3d7e3d5 100644 --- a/docs/FORMAT.md +++ b/docs/FORMAT.md @@ -37,7 +37,7 @@ The outer message contains: | `uuid` | 105 | bytes | Random v4 UUID; must match the inner message UUID | | `innerMessage` | 199 | bytes | Zstd-compressed serialized `MFFile` message | | `signature` | 201 | bytes (optional) | GPG signature (ASCII-armored or binary) | -| `signer` | 202 | bytes (optional) | Full GPG key ID of the signer | +| `signer` | 202 | bytes (optional) | Fingerprint of the signing key | | `signingPubKey` | 203 | bytes (optional) | Full GPG signing public key | ### SHA-256 Hash @@ -137,7 +137,16 @@ Where: compressed data) Components are separated by hyphens. The signature is produced by GPG over this -canonical string and stored in the `signature` field of the outer message. +canonical string and stored in the `signature` field of the outer message. The +signing key's public key goes in `signingPubKey` and its fingerprint, in hex, in +`signer`. + +A verifier accepts a signed manifest only if `signingPubKey` holds exactly one +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`. ## Deterministic Serialization diff --git a/internal/cli/check.go b/internal/cli/check.go index 655b443..0d43bb7 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -126,10 +126,8 @@ func (mfa *CLIApp) fetchManifestToTemp( } // verifyRequiredSigner enforces the --require-signature fingerprint -// against the manifest's embedded signing key. -func verifyRequiredSigner( - ctx context.Context, chk *mfer.Checker, requiredSigner string, -) error { +// against the key that made the manifest's signature. +func verifyRequiredSigner(chk *mfer.Checker, requiredSigner string) error { // Validate fingerprint format: must be exactly 40 hex characters if len(requiredSigner) != fingerprintHexLen { return fmt.Errorf("%w, got %d", errInvalidFingerprint, len(requiredSigner)) @@ -145,22 +143,17 @@ func verifyRequiredSigner( errManifestNotSigned, requiredSigner) } - // Extract fingerprint from the embedded public key (not from the - // signer field). This validates the key is importable and gets its - // actual fingerprint. - embeddedFP, err := chk.ExtractEmbeddedSigningKeyFP(ctx) - if err != nil { - return fmt.Errorf( - "failed to extract fingerprint from embedded signing key: %w", err) - } + // Loading the manifest checked that the signer is the fingerprint of + // the key that made the signature. + signer := string(chk.Signer()) // Compare fingerprints - must be exact match (case-insensitive) - if !strings.EqualFold(embeddedFP, requiredSigner) { + if !strings.EqualFold(signer, requiredSigner) { return fmt.Errorf("embedded signing key fingerprint %s %w %s", - embeddedFP, errSignerMismatch, requiredSigner) + signer, errSignerMismatch, requiredSigner) } - log.Infof("manifest signature verified (signer: %s)", embeddedFP) + log.Infof("manifest signature verified (signer: %s)", signer) return nil } @@ -330,7 +323,7 @@ func (mfa *CLIApp) checkManifestOperation( // Check signature requirement requiredSigner := cmd.String(flagRequireSignature) if requiredSigner != "" { - err = verifyRequiredSigner(ctx, chk, requiredSigner) + err = verifyRequiredSigner(chk, requiredSigner) if err != nil { return err } diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 165d9e8..aa5623a 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -613,6 +613,33 @@ func runCheckAfterRewrite(t *testing.T, rewritten, msg string) { assert.Equal(t, 1, exitCode, msg) } +// TestCheckRequireSignatureRefusesOtherSigningKey runs check +// --require-signature on a manifest signed by another key whose embedded +// public key block also holds the required key. check must refuse it. It +// needs gpg and is skipped without it, as the other signing tests are. +// +//nolint:paralleltest // signedManifest calls t.Setenv, which bars t.Parallel +func TestCheckRequireSignatureRefusesOtherSigningKey(t *testing.T) { + content := []byte("signed file") + manifest, required := manifestSignedByAnotherKey(t, + map[string][]byte{testFileTxt: content}) + + fs := afero.NewMemMapFs() + require.NoError(t, fs.MkdirAll(testDir, 0o755)) + require.NoError(t, afero.WriteFile(fs, + filepath.Join(testDir, testFileTxt), content, 0o644)) + require.NoError(t, afero.WriteFile(fs, testManifest, manifest, 0o644)) + + opts := testOpts([]string{ + testApp, cmdCheck, "-q", testFlagBase, testDir, + "--" + flagRequireSignature, required, testManifest, + }, fs) + assert.Equal(t, 1, runCLI(opts)) + assert.Contains(t, testStderr(t, opts), + "failed to load manifest: signature verification failed: "+ + "embedded public key block must hold exactly one key, found 2") +} + func TestCheckCommandWithCorruptedFile(t *testing.T) { t.Parallel() diff --git a/internal/cli/errmsg_test.go b/internal/cli/errmsg_test.go index e7fc0e5..9b65ecf 100644 --- a/internal/cli/errmsg_test.go +++ b/internal/cli/errmsg_test.go @@ -9,12 +9,14 @@ import ( "os" "os/exec" "path/filepath" + "slices" "testing" "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" urfcli "github.com/urfave/cli/v3" + "google.golang.org/protobuf/proto" "sneak.berlin/go/mfer/mfer" ) @@ -86,8 +88,7 @@ func TestVerifyRequiredSignerMessages(t *testing.T) { t.Run("invalid fingerprint length", func(t *testing.T) { t.Parallel() - err := verifyRequiredSigner(context.Background(), - unsignedChecker(t), "12345678") + err := verifyRequiredSigner(unsignedChecker(t), "12345678") require.ErrorIs(t, err, errInvalidFingerprint) assert.EqualError(t, err, "invalid fingerprint: must be exactly 40 hex characters, got 8") @@ -96,8 +97,7 @@ func TestVerifyRequiredSignerMessages(t *testing.T) { t.Run("manifest not signed", func(t *testing.T) { t.Parallel() - err := verifyRequiredSigner(context.Background(), - unsignedChecker(t), msgFpA) + err := verifyRequiredSigner(unsignedChecker(t), msgFpA) require.ErrorIs(t, err, errManifestNotSigned) assert.EqualError(t, err, "manifest is not signed, but signature from "+msgFpA+" is required") @@ -105,23 +105,21 @@ func TestVerifyRequiredSignerMessages(t *testing.T) { } // TestSignerMismatchMessage drives verifyRequiredSigner against a real signed -// manifest. The embedded fingerprint is whatever the generated key produced, -// so it is read back from the checker and substituted into the expected -// string; the required signer is a fixed value that cannot match it. Requires -// gpg and is skipped where it is absent, as the other signing tests are. +// manifest. The signing key's fingerprint is whatever the generated key +// produced, so it is read back from the checker and substituted into the +// expected string; the required signer is a fixed value that cannot match +// it. Requires gpg and is skipped where it is absent, as the other signing +// tests are. // //nolint:paralleltest // signedManifest calls t.Setenv, which bars t.Parallel func TestSignerMismatchMessage(t *testing.T) { chk := signedChecker(t, signedManifest(t, map[string][]byte{"f.txt": []byte("signed file")})) - embeddedFP, err := chk.ExtractEmbeddedSigningKeyFP(context.Background()) - require.NoError(t, err) - - err = verifyRequiredSigner(context.Background(), chk, msgFpB) + err := verifyRequiredSigner(chk, msgFpB) require.ErrorIs(t, err, errSignerMismatch) assert.EqualError(t, err, - "embedded signing key fingerprint "+embeddedFP+ + "embedded signing key fingerprint "+string(chk.Signer())+ " does not match required "+msgFpB) } @@ -191,6 +189,34 @@ func signedChecker(t *testing.T, manifest []byte) *mfer.Checker { return chk } +// manifestSignedByAnotherKey returns a manifest of files and the +// fingerprint of a throwaway key, the required key, that did not sign it. +// The manifest is signed by a second throwaway key; its embedded public key +// block holds the required key followed by the second key, and its signer +// field names the required key. +func manifestSignedByAnotherKey( + t *testing.T, files map[string][]byte, +) ([]byte, string) { + t.Helper() + + required := new(mfer.MFFileOuter) + require.NoError(t, proto.Unmarshal( + signedManifest(t, files)[len(mfer.MAGIC):], required)) + + outer := new(mfer.MFFileOuter) + require.NoError(t, proto.Unmarshal( + signedManifest(t, files)[len(mfer.MAGIC):], outer)) + + outer.SigningPubKey = slices.Concat( + required.GetSigningPubKey(), outer.GetSigningPubKey()) + outer.Signer = required.GetSigner() + + data, err := proto.Marshal(outer) + require.NoError(t, err) + + return append([]byte(mfer.MAGIC), data...), string(required.GetSigner()) +} + func TestPathDoesNotExistMessage(t *testing.T) { t.Parallel() diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 3799e09..e30a4a0 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -456,7 +456,8 @@ func fetchManifest( requiredSigner := cmd.String(flagRequireSignature) if requiredSigner != "" { - err = verifyFetchedSigner(ctx, manifestData, requiredSigner) + //nolint:contextcheck // mfer loads a manifest without a context + err = verifyFetchedSigner(manifestData, requiredSigner) if err != nil { return nil, nil, err } @@ -538,9 +539,7 @@ func checkNoNameClash(files []*mfer.MFFilePath) error { // exactly as check does. verifyRequiredSigner takes a Checker, which loads // its manifest from a file, so the manifest is handed to it as a file in // memory. -func verifyFetchedSigner( - ctx context.Context, manifestData []byte, requiredSigner string, -) error { +func verifyFetchedSigner(manifestData []byte, requiredSigner string) error { memFs := afero.NewMemMapFs() manifestPath := "/" + defaultManifestName @@ -549,7 +548,6 @@ func verifyFetchedSigner( return err } - //nolint:contextcheck // mfer loads a manifest without a context chk, err := mfer.NewChecker(&mfer.CheckerOptions{ ManifestPath: manifestPath, BasePath: "/", @@ -559,7 +557,7 @@ func verifyFetchedSigner( return fmt.Errorf("failed to load manifest: %w", err) } - return verifyRequiredSigner(ctx, chk, requiredSigner) + return verifyRequiredSigner(chk, requiredSigner) } // saveManifest writes the fetched manifest into dest under the default diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index 8fae1f8..6f0038e 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -1117,8 +1117,10 @@ func TestFetchIntoDest(t *testing.T) { // TestFetchRequireSignature runs fetch with --require-signature. A // manifest that is unsigned, or signed by another key, must stop fetch // with check's message before it downloads or writes anything; the -// required key lets it through. The signed cases need gpg and are skipped -// without it, as the other signing tests are. +// required key lets it through. A manifest signed by another key whose +// embedded public key block also holds the required key must stop fetch +// too. The signed cases need gpg and are skipped without it, as the other +// signing tests are. // //nolint:paralleltest // signedManifest calls t.Setenv, which bars t.Parallel func TestFetchRequireSignature(t *testing.T) { @@ -1133,9 +1135,7 @@ func TestFetchRequireSignature(t *testing.T) { t.Run("signed", func(t *testing.T) { manifest := signedManifest(t, files) - signer, err := signedChecker(t, manifest). - ExtractEmbeddedSigningKeyFP(context.Background()) - require.NoError(t, err) + signer := string(signedChecker(t, manifest).Signer()) assertFetchRefused(t, manifest, files, "embedded signing key fingerprint "+signer+" does not match required "+msgFpB, @@ -1153,6 +1153,15 @@ func TestFetchRequireSignature(t *testing.T) { require.Equal(t, 0, runCLI(opts), testStderr(t, opts)) assert.Equal(t, files[testFileTxt], filesUnder(t, dest)[testFileTxt]) }) + + t.Run("signed by another key embedded after the required one", func(t *testing.T) { + manifest, required := manifestSignedByAnotherKey(t, files) + + assertFetchRefused(t, manifest, files, + "failed to parse manifest: signature verification failed: "+ + "embedded public key block must hold exactly one key, found 2", + "--"+flagRequireSignature, required) + }) } // TestFetchRefusesListedManifestName fetches manifests that list, at the diff --git a/mfer/checker.go b/mfer/checker.go index 2d121a9..3077303 100644 --- a/mfer/checker.go +++ b/mfer/checker.go @@ -15,7 +15,6 @@ import ( ) var ( - errNoSigningPubKey = errors.New("manifest has no signing public key") errManifestPathEmpty = errors.New("manifest path cannot be empty") errBasePathEmpty = errors.New("base path cannot be empty") ) @@ -173,8 +172,14 @@ func (c *Checker) IsSigned() bool { return len(c.signature) > 0 } -// Signer returns the signer fingerprint if the manifest is signed, nil otherwise. +// Signer returns the fingerprint of the key that made the manifest's +// signature, which loading the manifest checked, or nil if the manifest is +// not signed. func (c *Checker) Signer() []byte { + if !c.IsSigned() { + return nil + } + return c.signer } @@ -184,17 +189,6 @@ func (c *Checker) SigningPubKey() []byte { return c.signingPubKey } -// ExtractEmbeddedSigningKeyFP imports the manifest's embedded public key into a -// temporary keyring and extracts its fingerprint. This validates the key and -// returns its actual fingerprint from the key material itself. -func (c *Checker) ExtractEmbeddedSigningKeyFP(ctx context.Context) (string, error) { - if len(c.signingPubKey) == 0 { - return "", errNoSigningPubKey - } - - return gpgExtractPubKeyFingerprint(ctx, c.signingPubKey) -} - // Check verifies all files against the manifest. // Results are sent to the results channel as files are checked. // Progress updates are sent to the progress channel approximately once per second. diff --git a/mfer/deserialize.go b/mfer/deserialize.go index 7473126..85ec2b6 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "strings" "github.com/klauspost/compress/zstd" "github.com/spf13/afero" @@ -28,6 +29,8 @@ var ( errInvalidManifestPath = errors.New("manifest contains invalid path") errDecodedTooLarge = errors.New( "manifest would take too much memory to decode") + errSignerNotSigningKey = errors.New( + "signer is not the fingerprint of the key that made the signature") ) // validateUUID checks that the byte slice is the 16 bytes of a binary UUID. @@ -60,8 +63,10 @@ func (m *manifest) validateOuterHeader() error { return nil } -// verifyOuterIntegrity checks the hash of the compressed payload and, -// if a signature is present, verifies it against the embedded public key. +// verifyOuterIntegrity checks the hash of the compressed payload and, if a +// signature is present, verifies it against the embedded public key, which +// must be one key, and checks that the signer field is that key's +// fingerprint. func (m *manifest) verifyOuterIntegrity() error { h := sha256.New() @@ -91,7 +96,7 @@ func (m *manifest) verifyOuterIntegrity() error { } // Loading a manifest takes no context; gpgTimeout still bounds gpg. - err = gpgVerify( + signingKey, err := gpgVerify( context.Background(), []byte(sigString), m.pbOuter.GetSignature(), @@ -101,6 +106,11 @@ func (m *manifest) verifyOuterIntegrity() error { return fmt.Errorf("signature verification failed: %w", err) } + if !strings.EqualFold(string(m.pbOuter.GetSigner()), signingKey) { + return fmt.Errorf("%w: signer %q, signing key %s", + errSignerNotSigningKey, m.pbOuter.GetSigner(), signingKey) + } + log.Infof("signature verified successfully") return nil diff --git a/mfer/gpg.go b/mfer/gpg.go index 4aef420..b9e940a 100644 --- a/mfer/gpg.go +++ b/mfer/gpg.go @@ -41,6 +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 " + // gpg option names used from more than one call site. gpgOptArmor = "--armor" gpgOptHomedir = "--homedir" @@ -50,7 +59,10 @@ const ( var ( errGPGKeyNotFound = errors.New("gpg key not found") errFingerprintNotFound = errors.New("fingerprint not found for key") - errImportedFPRNotFound = errors.New("fingerprint not found in imported key") + errSigningKeyCount = errors.New( + "embedded public key block must hold exactly one key") + errNotOneGoodSignature = errors.New( + "gpg did not report exactly one good signature") ) // GPGKeyID represents a GPG key identifier (fingerprint or key ID). @@ -136,6 +148,40 @@ 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 + + for line := range strings.SplitSeq(statusOutput, "\n") { + if strings.HasPrefix(line, gpgValidSigStatus) { + fields := strings.Fields(line) + fingerprints = append(fingerprints, fields[len(fields)-1]) + } + } + + if len(fingerprints) != 1 { + return "", false + } + + return fingerprints[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) { @@ -187,12 +233,47 @@ func gpgGetKeyFingerprint(ctx context.Context, keyID GPGKeyID) ([]byte, error) { return []byte(fpr), nil } -// gpgExtractPubKeyFingerprint imports a public key into a temporary keyring -// and extracts its fingerprint. This verifies the key is valid and returns -// the actual fingerprint from the key material. -func gpgExtractPubKeyFingerprint(ctx context.Context, pubKey []byte) (string, error) { +// gpgImportOneKey imports the public key in pubKeyFile into the keyring in +// gpgHome, which must then 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)..., + ) + if err != nil { + return fmt.Errorf( + "failed to import public key: %w: %s", err, importStderr.String(), + ) + } + + // 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(), + ) + } + + keys := countPrimaryKeys(listStdout.String()) + if keys != 1 { + return fmt.Errorf("%w, found %d", errSigningKeyCount, keys) + } + + return nil +} + +// gpgVerify verifies a detached signature against data using the provided +// public key, imported into a temporary keyring, and returns the +// fingerprint of the primary key that made the signature. The public key +// must hold exactly one primary key, so that a good signature can come +// from no other key. +func gpgVerify(ctx context.Context, data, signature, pubKey []byte) (string, error) { // Create temporary directory for GPG operations - tmpDir, err := os.MkdirTemp("", "mfer-gpg-fingerprint-*") + tmpDir, err := os.MkdirTemp("", "mfer-gpg-verify-*") if err != nil { return "", fmt.Errorf("failed to create temp dir: %w", err) } @@ -213,67 +294,12 @@ func gpgExtractPubKeyFingerprint(ctx context.Context, pubKey []byte) (string, er return "", fmt.Errorf("failed to write public key: %w", err) } - // Import the public key into the temporary keyring - _, importStderr, err := runGPG(ctx, nil, - gpgArgs([]string{gpgOptHomedir, tmpDir, "--import"}, pubKeyFile)..., - ) - if err != nil { - return "", fmt.Errorf( - "failed to import public key: %w: %s", err, importStderr.String(), - ) - } - - // List keys to get fingerprint - listStdout, listStderr, err := runGPG(ctx, nil, - "--homedir", tmpDir, - "--with-colons", - "--fingerprint", - ) - if err != nil { - return "", fmt.Errorf( - "failed to list keys: %w: %s", err, listStderr.String(), - ) - } - - fpr, ok := parseFingerprint(listStdout.String()) - if !ok { - return "", errImportedFPRNotFound - } - - return fpr, nil -} - -// gpgVerify verifies a detached signature against data using the provided public key. -// It creates a temporary keyring to import the public key for verification. -func gpgVerify(ctx context.Context, data, signature, pubKey []byte) error { - // Create temporary directory for GPG operations - tmpDir, err := os.MkdirTemp("", "mfer-gpg-verify-*") - if err != nil { - return fmt.Errorf("failed to create temp dir: %w", err) - } - - defer func() { _ = os.RemoveAll(tmpDir) }() - - // Set restrictive permissions - err = os.Chmod(tmpDir, privateDirPerms) - if err != nil { - return fmt.Errorf("failed to set temp dir permissions: %w", err) - } - - // Write public key to temp file - pubKeyFile := filepath.Join(tmpDir, "pubkey.asc") - - err = os.WriteFile(pubKeyFile, pubKey, privateFilePerms) - if err != nil { - return fmt.Errorf("failed to write public key: %w", err) - } - // Write signature to temp file sigFile := filepath.Join(tmpDir, "signature.asc") err = os.WriteFile(sigFile, signature, privateFilePerms) if err != nil { - return fmt.Errorf("failed to write signature: %w", err) + return "", fmt.Errorf("failed to write signature: %w", err) } // Write data to temp file @@ -281,29 +307,30 @@ func gpgVerify(ctx context.Context, data, signature, pubKey []byte) error { err = os.WriteFile(dataFile, data, privateFilePerms) if err != nil { - return fmt.Errorf("failed to write data: %w", err) + return "", fmt.Errorf("failed to write data: %w", err) } - // Import the public key into the temporary keyring - _, importStderr, err := runGPG(ctx, nil, - gpgArgs([]string{gpgOptHomedir, tmpDir, "--import"}, pubKeyFile)..., - ) + err = gpgImportOneKey(ctx, tmpDir, pubKeyFile) if err != nil { - return fmt.Errorf( - "failed to import public key: %w: %s", err, importStderr.String(), - ) + return "", err } - // Verify the signature - _, verifyStderr, err := runGPG(ctx, nil, - gpgArgs([]string{gpgOptHomedir, tmpDir, gpgOptVerify}, + // --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}, sigFile, dataFile)..., ) if err != nil { - return fmt.Errorf( + return "", fmt.Errorf( "signature verification failed: %w: %s", err, verifyStderr.String(), ) } - return nil + fingerprint, ok := parseSigningKey(verifyStdout.String()) + if !ok { + return "", errNotOneGoodSignature + } + + return fingerprint, nil } diff --git a/mfer/gpg_test.go b/mfer/gpg_test.go index 0969f4f..690edd5 100644 --- a/mfer/gpg_test.go +++ b/mfer/gpg_test.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strconv" "strings" "syscall" @@ -17,6 +18,7 @@ import ( "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/proto" ) // testGPGEnv sets up a temporary GPG home directory with a test key. @@ -35,7 +37,47 @@ func testGPGEnv(t *testing.T) (GPGKeyID, string) { // Create temporary GPG home directory (0700 by default) gpgHome := t.TempDir() - // Generate a test key with no passphrase + genTestKey(t, gpgHome) + + ctx, cancel := context.WithTimeout(context.Background(), gpgTimeout) + defer cancel() + + // Get the key fingerprint + cmd := exec.CommandContext(ctx, "gpg", + "--list-keys", "--with-colons", "test@mfer.test") + + cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) + + output, err := cmd.Output() + if err != nil { + t.Fatalf("failed to list test key: %v", err) + } + + // Parse fingerprint from output + var keyID string + + for line := range strings.SplitSeq(string(output), "\n") { + fields := strings.Split(line, ":") + if len(fields) >= gpgFingerprintMinFields && + fields[0] == gpgFingerprintField { + keyID = fields[9] + + break + } + } + + if keyID == "" { + t.Fatal("failed to find test key fingerprint") + } + + return GPGKeyID(keyID), gpgHome +} + +// genTestKey generates a key with no passphrase for +// "MFER Test Key " in gpgHome, which may already hold one. +func genTestKey(t *testing.T, gpgHome string) { + t.Helper() + keyParams := `%no-protection Key-Type: RSA Key-Length: 2048 @@ -60,36 +102,41 @@ Expire-Date: 0 if err != nil { t.Skipf("failed to generate test GPG key: %v: %s", err, output) } +} - // Get the key fingerprint - cmd = exec.CommandContext(ctx, "gpg", - "--list-keys", "--with-colons", "test@mfer.test") +// signedTestManifest returns a manifest of one file signed with keyID. +func signedTestManifest(t *testing.T, keyID GPGKeyID) []byte { + t.Helper() - cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) + b := NewBuilder() + b.SetSigningOptions(&SigningOptions{KeyID: keyID}) - output, err = cmd.Output() - if err != nil { - t.Fatalf("failed to list test key: %v", err) - } + content := []byte("signed file content") + _, err := b.AddFile("test.txt", FileSize(len(content)), ModTime{}, 0, + bytes.NewReader(content), nil) + require.NoError(t, err) - // Parse fingerprint from output - var keyID string + var buf bytes.Buffer - for line := range strings.SplitSeq(string(output), "\n") { - fields := strings.Split(line, ":") - if len(fields) >= gpgFingerprintMinFields && - fields[0] == gpgFingerprintField { - keyID = fields[9] + require.NoError(t, b.Build(context.Background(), &buf)) - break - } - } + return buf.Bytes() +} - if keyID == "" { - t.Fatal("failed to find test key fingerprint") - } +// rewriteOuter returns manifest with its outer message changed by edit. +// A signature stays good as long as edit leaves the UUID and hash alone. +func rewriteOuter(t *testing.T, manifest []byte, edit func(*MFFileOuter)) []byte { + t.Helper() - return GPGKeyID(keyID), gpgHome + outer := new(MFFileOuter) + require.NoError(t, proto.Unmarshal(manifest[len(MAGIC):], outer)) + + edit(outer) + + data, err := proto.Marshal(outer) + require.NoError(t, err) + + return append([]byte(MAGIC), data...) } func TestGPGSign(t *testing.T) { @@ -265,9 +312,10 @@ func TestGPGVerify(t *testing.T) { pubKey, err := gpgExportPublicKey(context.Background(), keyID) require.NoError(t, err) - // Verify the signature - err = gpgVerify(context.Background(), data, sig, pubKey) + // Verify the signature; it names the key that made it + signingKey, err := gpgVerify(context.Background(), data, sig, pubKey) require.NoError(t, err) + assert.Equal(t, string(keyID), signingKey) } func TestGPGVerifyInvalidSignature(t *testing.T) { @@ -283,7 +331,7 @@ func TestGPGVerifyInvalidSignature(t *testing.T) { // Try to verify with different data - should fail wrongData := []byte("different data") - err = gpgVerify(context.Background(), wrongData, sig, pubKey) + _, err = gpgVerify(context.Background(), wrongData, sig, pubKey) assert.Error(t, err) } @@ -297,7 +345,7 @@ func TestGPGVerifyBadPublicKey(t *testing.T) { // Try to verify with invalid public key - should fail badPubKey := []byte("not a valid public key") - err = gpgVerify(context.Background(), data, sig, badPubKey) + _, err = gpgVerify(context.Background(), data, sig, badPubKey) assert.Error(t, err) } @@ -368,6 +416,58 @@ func TestManifestTamperedSignatureFails(t *testing.T) { assert.Error(t, err) } +// TestManifestRefusesSecondEmbeddedKey loads a manifest whose embedded +// public key block holds another key before the key that signed it. +// Loading must refuse it, although the signature is good and the signer +// field names the key that made it. +func TestManifestRefusesSecondEmbeddedKey(t *testing.T) { + otherKey, otherHome := testGPGEnv(t) + t.Setenv("GNUPGHOME", otherHome) + + otherPubKey, err := gpgExportPublicKey(context.Background(), 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, outer.GetSigningPubKey()) + }) + + _, err = NewManifestFromReader(bytes.NewReader(manifest)) + require.ErrorIs(t, err, errSigningKeyCount) +} + +// TestManifestRefusesSignerOtherThanSigningKey loads a manifest whose +// signer field names a key other than the one that made the signature. +func TestManifestRefusesSignerOtherThanSigningKey(t *testing.T) { + keyID, gpgHome := testGPGEnv(t) + t.Setenv("GNUPGHOME", gpgHome) + + manifest := rewriteOuter(t, signedTestManifest(t, keyID), + func(outer *MFFileOuter) { + outer.Signer = []byte(strings.Repeat("A", len(keyID))) + }) + + _, err := NewManifestFromReader(bytes.NewReader(manifest)) + require.ErrorIs(t, err, errSignerNotSigningKey) +} + +// TestBuilderSigningKeyIDMatchingTwoKeys signs with a key ID that two keys +// in the keyring match. The manifest must embed and name only the key that +// signed it, or loading refuses it. +func TestBuilderSigningKeyIDMatchingTwoKeys(t *testing.T) { + _, gpgHome := testGPGEnv(t) + genTestKey(t, gpgHome) + t.Setenv("GNUPGHOME", gpgHome) + + manifest := signedTestManifest(t, GPGKeyID("test@mfer.test")) + + _, err := NewManifestFromReader(bytes.NewReader(manifest)) + require.NoError(t, err) +} + func TestBuilderWithoutSigning(t *testing.T) { t.Parallel() diff --git a/mfer/serialize.go b/mfer/serialize.go index ded1f20..1c40835 100644 --- a/mfer/serialize.go +++ b/mfer/serialize.go @@ -143,20 +143,15 @@ 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. +// 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. 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) } - sig, err := gpgSign(ctx, []byte(sigString), m.signingOptions.KeyID) - if err != nil { - return fmt.Errorf("failed to sign manifest: %w", err) - } - - m.pbOuter.Signature = sig - fingerprint, err := gpgGetKeyFingerprint(ctx, m.signingOptions.KeyID) if err != nil { return fmt.Errorf("failed to get key fingerprint: %w", err) @@ -164,7 +159,14 @@ func (m *manifest) signOuter(ctx context.Context) error { m.pbOuter.Signer = fingerprint - pubKey, err := gpgExportPublicKey(ctx, m.signingOptions.KeyID) + sig, err := gpgSign(ctx, []byte(sigString), GPGKeyID(fingerprint)) + if err != nil { + return fmt.Errorf("failed to sign manifest: %w", err) + } + + m.pbOuter.Signature = sig + + pubKey, err := gpgExportPublicKey(ctx, GPGKeyID(fingerprint)) if err != nil { return fmt.Errorf("failed to export public key: %w", err) }