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) }