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..6de5fd6 100644 --- a/mfer/gpg.go +++ b/mfer/gpg.go @@ -41,16 +41,26 @@ const ( // fields in a gpg fingerprint record (the fingerprint is field 10). gpgFingerprintMinFields = 10 + // 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 ( 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") + errSigningKeyNotReported = errors.New( + "gpg did not report the key that made the signature") ) // GPGKeyID represents a GPG key identifier (fingerprint or key ID). @@ -136,19 +146,51 @@ func parseFingerprint(colonOutput string) (string, bool) { return "", false } -// 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) { +// 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") { + fields := strings.Fields(line) + if len(fields) > 2 && fields[0] == gpgStatusPrefix && fields[1] == keyword { + found = append(found, fields[2:]) + } + } + + if len(found) != 1 { + return nil, false + } + + return found[0], true +} + +// 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. @@ -187,12 +229,44 @@ 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 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 { + // --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( + "failed to import public key: %w: %s", err, importStderr.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) + } + + if result[0] != "1" { + return fmt.Errorf("%w, found %s", errSigningKeyCount, result[0]) + } + + 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 +287,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 +300,33 @@ 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, gpgOptStatusFD, "1", gpgOptVerify}, sigFile, dataFile)..., ) if err != nil { - return fmt.Errorf( + return "", fmt.Errorf( "signature verification failed: %w: %s", err, verifyStderr.String(), ) } - return nil + // 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 valid[len(valid)-1], nil } diff --git a/mfer/gpg_test.go b/mfer/gpg_test.go index 0969f4f..1185971 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,39 +37,18 @@ 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 - keyParams := `%no-protection -Key-Type: RSA -Key-Length: 2048 -Name-Real: MFER Test Key -Name-Email: test@mfer.test -Expire-Date: 0 -%commit -` - paramsFile := filepath.Join(gpgHome, "key-params") - require.NoError(t, os.WriteFile(paramsFile, []byte(keyParams), 0o600)) + genTestKey(t, gpgHome, testKeyParams) ctx, cancel := context.WithTimeout(context.Background(), gpgTimeout) defer cancel() - //nolint:gosec // paramsFile is a test-controlled path inside t.TempDir() - cmd := exec.CommandContext(ctx, "gpg", - "--batch", "--gen-key", paramsFile) - - cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) - - output, err := cmd.CombinedOutput() - if err != nil { - t.Skipf("failed to generate test GPG key: %v: %s", err, output) - } - // Get the key fingerprint - cmd = exec.CommandContext(ctx, "gpg", + cmd := exec.CommandContext(ctx, "gpg", "--list-keys", "--with-colons", "test@mfer.test") cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) - output, err = cmd.Output() + output, err := cmd.Output() if err != nil { t.Fatalf("failed to list test key: %v", err) } @@ -92,13 +73,79 @@ Expire-Date: 0 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 " from the gpg key generation parameters +// keyParams in gpgHome, which may already hold one. +func genTestKey(t *testing.T, gpgHome, keyParams string) { + t.Helper() + + 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(params), 0o600)) + + ctx, cancel := context.WithTimeout(context.Background(), gpgTimeout) + defer cancel() + + //nolint:gosec // paramsFile is a test-controlled path inside t.TempDir() + cmd := exec.CommandContext(ctx, "gpg", + "--batch", "--gen-key", paramsFile) + + cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) + + output, err := cmd.CombinedOutput() + if err != nil { + t.Skipf("failed to generate test GPG key: %v: %s", err, output) + } +} + +// signedTestManifest returns a manifest of one file signed with keyID. +func signedTestManifest(t *testing.T, keyID GPGKeyID) []byte { + t.Helper() + + b := NewBuilder() + b.SetSigningOptions(&SigningOptions{KeyID: keyID}) + + content := []byte("signed file content") + _, err := b.AddFile("test.txt", FileSize(len(content)), ModTime{}, 0, + bytes.NewReader(content), nil) + require.NoError(t, err) + + var buf bytes.Buffer + + require.NoError(t, b.Build(context.Background(), &buf)) + + return buf.Bytes() +} + +// 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() + + 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) { keyID, gpgHome := testGPGEnv(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-----") @@ -169,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) } @@ -259,15 +306,16 @@ 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) 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) { @@ -275,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) @@ -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) } @@ -292,12 +340,12 @@ 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 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,144 @@ 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) +} + +// 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) { + 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, testKeyParams) + t.Setenv("GNUPGHOME", gpgHome) + + manifest := signedTestManifest(t, GPGKeyID("test@mfer.test")) + + _, err := NewManifestFromReader(bytes.NewReader(manifest)) + 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() @@ -421,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") } @@ -446,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 }() diff --git a/mfer/serialize.go b/mfer/serialize.go index ded1f20..f0945a7 100644 --- a/mfer/serialize.go +++ b/mfer/serialize.go @@ -143,28 +143,32 @@ 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. 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) } - sig, err := gpgSign(ctx, []byte(sigString), m.signingOptions.KeyID) + 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 - fingerprint, err := gpgGetKeyFingerprint(ctx, m.signingOptions.KeyID) + // 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, m.signingOptions.KeyID) + pubKey, err := gpgExportPublicKey(ctx, GPGKeyID(fingerprint)) if err != nil { return fmt.Errorf("failed to export public key: %w", err) }