From a6aa83b3984928feb077f2c613c1cc4c1d80a918 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 13 Oct 2026 15:06:46 +0000 Subject: [PATCH] Refuse an expired signature whose key has expired too (closes #193) verifySignature accepts a signature whose key has expired, since a manifest outlives its key. The OpenPGP library stops at the expired key without checking the signature's own expiry time, so a signature made with one was refused while the key was live and accepted once the key had expired as well. verifySignature now gets the signature packet from openpgp.VerifyDetachedSignature and refuses it when it has expired. docs/FORMAT.md Signature Scheme states the rule. Model: opus-5-5 --- docs/FORMAT.md | 4 ++++ mfer/openpgp.go | 19 +++++++++++++------ mfer/openpgp_test.go | 17 ++++++++++++++--- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/docs/FORMAT.md b/docs/FORMAT.md index d792527..7f73f99 100644 --- a/docs/FORMAT.md +++ b/docs/FORMAT.md @@ -174,6 +174,10 @@ also refuses a manifest whose `signingPubKey` holds a DSA key or subkey, or any secret key or subkey, since checking the self-signatures of a DSA key or the numbers of a secret key can take hours when those numbers are very large. +A signing key that has expired does not make the signature bad, because a +manifest outlives its key. A signature past its own expiry time is not good, +whether or not the key has expired too. + ## Deterministic Serialization By default, manifests are generated deterministically: diff --git a/mfer/openpgp.go b/mfer/openpgp.go index bff1eb5..cff74f1 100644 --- a/mfer/openpgp.go +++ b/mfer/openpgp.go @@ -8,6 +8,7 @@ import ( "io" "slices" "strings" + "time" "github.com/ProtonMail/go-crypto/openpgp" "github.com/ProtonMail/go-crypto/openpgp/armor" @@ -242,10 +243,10 @@ func fingerprint(key *openpgp.Entity) string { return strings.ToUpper(hex.EncodeToString(key.PrimaryKey.Fingerprint)) } -// verifySignature checks that signature is one good OpenPGP signature -// over data, made by the one primary key in pubKey or one of its subkeys, -// and returns that primary key's fingerprint. signature and pubKey may each -// be armored or binary. +// verifySignature checks that signature is one good, unexpired OpenPGP +// signature over data, made by the one primary key in pubKey or one of its +// subkeys, and returns that primary key's fingerprint. signature and pubKey +// may each be armored or binary. func verifySignature(data, signature, pubKey []byte) (string, error) { key, err := readOneKey(pubKey, "embedded public key block", true) if err != nil { @@ -266,10 +267,16 @@ func verifySignature(data, signature, pubKey []byte) (string, error) { return "", fmt.Errorf("%w, found %d", errNotOneSignature, sigs) } - _, err = openpgp.CheckDetachedSignature(openpgp.EntityList{key}, + sig, _, err := openpgp.VerifyDetachedSignature(openpgp.EntityList{key}, bytes.NewReader(data), bytes.NewReader(sigData), nil) // A manifest outlives its signing key, so a signature by a key that - // has expired since is still good. + // has expired since is still good, unless the signature has expired + // itself. The library does not check that once it finds the key + // expired. + if errors.Is(err, pgperrors.ErrKeyExpired) && sig.SigExpired(time.Now()) { + err = pgperrors.ErrSignatureExpired + } + if err != nil && !errors.Is(err, pgperrors.ErrKeyExpired) { return "", fmt.Errorf("verify signature: %w", err) } diff --git a/mfer/openpgp_test.go b/mfer/openpgp_test.go index 5b571bf..c6673f3 100644 --- a/mfer/openpgp_test.go +++ b/mfer/openpgp_test.go @@ -18,6 +18,7 @@ import ( "github.com/ProtonMail/go-crypto/openpgp" "github.com/ProtonMail/go-crypto/openpgp/armor" "github.com/ProtonMail/go-crypto/openpgp/elgamal" + pgperrors "github.com/ProtonMail/go-crypto/openpgp/errors" "github.com/ProtonMail/go-crypto/openpgp/packet" "github.com/spf13/afero" "github.com/stretchr/testify/assert" @@ -416,8 +417,10 @@ func TestVerifySignature(t *testing.T) { assert.Error(t, err) } -// TestVerifySignatureKeyExpiredSince verifies a signature made in 2020 by -// a key that expired a day after it was made. The signature is still good. +// TestVerifySignatureKeyExpiredSince verifies two signatures made in 2020 +// by a key that expired a day after it was made. A signature with no +// expiry time is still good. One that expired an hour after it was made is +// refused, although the key has expired too. func TestVerifySignatureKeyExpiredSince(t *testing.T) { t.Parallel() @@ -429,9 +432,14 @@ func TestVerifySignatureKeyExpiredSince(t *testing.T) { key := newTestKey(t, config) data := []byte("signed in 2020") - var sig bytes.Buffer + var sig, expiringSig bytes.Buffer require.NoError(t, openpgp.ArmoredDetachSign(&sig, key, bytes.NewReader(data), config)) + require.NoError(t, openpgp.ArmoredDetachSign(&expiringSig, key, + bytes.NewReader(data), &packet.Config{ + Time: config.Time, + SigLifetimeSecs: uint32(time.Hour.Seconds()), + })) pubKey, err := armoredPublicKey(key) require.NoError(t, err) @@ -439,6 +447,9 @@ func TestVerifySignatureKeyExpiredSince(t *testing.T) { signer, err := verifySignature(data, sig.Bytes(), pubKey) require.NoError(t, err) assert.Equal(t, fingerprint(key), signer) + + _, err = verifySignature(data, expiringSig.Bytes(), pubKey) + require.ErrorIs(t, err, pgperrors.ErrSignatureExpired) } func TestManifestSignatureVerification(t *testing.T) {