From 812ea2fa6698e92bd4a18be95dcc47acc705b1b7 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 13 Oct 2026 18:35:09 +0000 Subject: [PATCH] Refuse a signature made when its key was not valid (closes #209) verifySignature lets a key that has expired since verify a signature, because a manifest outlives its key, but it checked the key only as of now. It accepted a signature dated after its key expired, after the key's self-signature or the signing subkey's binding signature expired, or before the key was made. It now verifies a second time with the library's clock set to the signature's creation time and refuses the signature if the key or subkey was not valid then. docs/FORMAT.md states the rule. Judgement call: the tests make the refused signatures with the library's packet API, because openpgp.DetachSign will not sign with a key that is not valid at its clock's time. Model: opus-5-5 --- docs/FORMAT.md | 15 ++++- mfer/openpgp.go | 29 +++++++++- mfer/openpgp_test.go | 128 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 164 insertions(+), 8 deletions(-) diff --git a/docs/FORMAT.md b/docs/FORMAT.md index 6758f88..ed401f9 100644 --- a/docs/FORMAT.md +++ b/docs/FORMAT.md @@ -174,9 +174,18 @@ 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. +A signature is good only if the primary key, and the subkey that made the +signature, were valid at the signature's creation time under the key validity +rules of RFC 9580. The key is judged by its current self-signatures and binding +signatures: for a version 4 key the newest self-signature on its primary user +ID, for a version 6 key its newest direct-key signature, and for a subkey its +newest binding signature with the signature embedded in it. So a signature made +before the newest of those is not good, even if an older one made the key valid +at that time. The reference implementation checks this by verifying the +signature a second time with its clock set to the signature's creation time. A +key that has expired since 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 diff --git a/mfer/openpgp.go b/mfer/openpgp.go index cff74f1..37ef5aa 100644 --- a/mfer/openpgp.go +++ b/mfer/openpgp.go @@ -57,7 +57,9 @@ var ( "signature must hold exactly one signature") errNotOneArmoredBlock = errors.New( "must be exactly one armored block and nothing else") - errMalformedArmor = errors.New("armor is malformed") + errMalformedArmor = errors.New("armor is malformed") + errKeyNotValidWhenSigned = errors.New( + "signature was made when its key was not valid") ) // SigningOptions holds the key a manifest is signed with. @@ -245,8 +247,9 @@ func fingerprint(key *openpgp.Entity) string { // 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. +// subkeys while that key was valid, and returns that primary key's +// fingerprint. The key may have expired since. 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 { @@ -281,6 +284,26 @@ func verifySignature(data, signature, pubKey []byte) (string, error) { return "", fmt.Errorf("verify signature: %w", err) } + // The check above judges the key as of now. Checking the signature a + // second time with the library's clock set to its creation time + // requires the primary key, and the subkey that made the signature, to + // have been valid then under the key validity rules of RFC 9580. The + // key is judged by its current self-signatures and binding signatures: + // for a version 4 key the newest self-signature on its primary user + // ID, for a version 6 key its newest direct-key signature, and for a + // subkey its newest binding signature with the signature embedded in + // it. So a signature made before the newest of those is refused. + signedAt := &packet.Config{ + Time: func() time.Time { return sig.CreationTime }, + } + + _, _, err = openpgp.VerifyDetachedSignature(openpgp.EntityList{key}, + bytes.NewReader(data), bytes.NewReader(sigData), signedAt) + if err != nil { + return "", fmt.Errorf("verify signature: %w: %w", + errKeyNotValidWhenSigned, err) + } + return fingerprint(key), nil } diff --git a/mfer/openpgp_test.go b/mfer/openpgp_test.go index c6673f3..d7314ed 100644 --- a/mfer/openpgp_test.go +++ b/mfer/openpgp_test.go @@ -4,6 +4,7 @@ package mfer import ( "bytes" "context" + "crypto" "crypto/dsa" //nolint:staticcheck // SA1019: tests need a DSA key to refuse "io" "math/big" @@ -419,8 +420,9 @@ func TestVerifySignature(t *testing.T) { // 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. +// expiry time is still good, as is one made then by a signing subkey that +// expired a day after it was made. One that expired an hour after it was +// made is refused, although the key has expired too. func TestVerifySignatureKeyExpiredSince(t *testing.T) { t.Parallel() @@ -448,10 +450,132 @@ func TestVerifySignatureKeyExpiredSince(t *testing.T) { require.NoError(t, err) assert.Equal(t, fingerprint(key), signer) + withSubkey := newTestKey(t, &packet.Config{Time: config.Time}) + require.NoError(t, withSubkey.AddSigningSubkey(&packet.Config{ + Algorithm: packet.PubKeyAlgoEdDSA, Time: config.Time, + KeyLifetimeSecs: config.KeyLifetimeSecs, + })) + + var subkeySig bytes.Buffer + + require.NoError(t, openpgp.ArmoredDetachSign(&subkeySig, withSubkey, + bytes.NewReader(data), config)) + + subkeyPubKey, err := armoredPublicKey(withSubkey) + require.NoError(t, err) + + _, err = verifySignature(data, subkeySig.Bytes(), subkeyPubKey) + require.NoError(t, err) + _, err = verifySignature(data, expiringSig.Bytes(), pubKey) require.ErrorIs(t, err, pgperrors.ErrSignatureExpired) } +// signDated returns a detached signature over data by signer, dated when. +// openpgp.DetachSign signs only with a key that is valid at the time its +// config gives, so this makes the signature packet without that check. +func signDated(t *testing.T, signer *packet.PrivateKey, data []byte, + when time.Time, +) []byte { + t.Helper() + + sig := &packet.Signature{ + SigType: packet.SigTypeBinary, + PubKeyAlgo: signer.PubKeyAlgo, + Hash: crypto.SHA256, + CreationTime: when, + IssuerKeyId: &signer.KeyId, + } + + h, err := sig.PrepareSign(nil) + require.NoError(t, err) + + _, err = h.Write(data) + require.NoError(t, err) + + require.NoError(t, sig.Sign(h, signer, nil)) + + var buf bytes.Buffer + + require.NoError(t, sig.Serialize(&buf)) + + return buf.Bytes() +} + +// TestVerifySignatureRefusesKeyNotValidWhenSigned verifies signatures +// dated when the key or subkey that made them was not valid: after the key +// expired; after the key's self-signature, or the signing subkey's binding +// signature, expired, the key or subkey having expired since as well; and +// before the key was made, also by a key made in 2100. Each must be +// refused. +func TestVerifySignatureRefusesKeyNotValidWhenSigned(t *testing.T) { + t.Parallel() + + made := time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC) + twoDaysLater := made.Add(48 * time.Hour) + day := uint32((24 * time.Hour).Seconds()) + clock := func(when time.Time) func() time.Time { + return func() time.Time { return when } + } + + expiredAfterADay := newTestKey(t, &packet.Config{ + Time: clock(made), KeyLifetimeSecs: day, + }) + selfSignatureExpiredAfterADay := newTestKey(t, &packet.Config{ + Time: clock(made), KeyLifetimeSecs: 30 * day, SigLifetimeSecs: day, + }) + + withSubkey := newTestKey(t, &packet.Config{Time: clock(made)}) + require.NoError(t, withSubkey.AddSigningSubkey(&packet.Config{ + Algorithm: packet.PubKeyAlgoEdDSA, Time: clock(made), + KeyLifetimeSecs: 30 * day, SigLifetimeSecs: day, + })) + subkey := withSubkey.Subkeys[len(withSubkey.Subkeys)-1].PrivateKey + + madeADayLater := newTestKey(t, &packet.Config{ + Time: clock(made.Add(24 * time.Hour)), + }) + madeIn2100 := newTestKey(t, &packet.Config{ + Time: clock(time.Date(2100, 1, 1, 0, 0, 0, 0, time.UTC)), + }) + + for name, test := range map[string]struct { + key *openpgp.Entity + signer *packet.PrivateKey + when time.Time + }{ + "after the key expired": { + expiredAfterADay, expiredAfterADay.PrivateKey, twoDaysLater, + }, + "after the self-signature expired": { + selfSignatureExpiredAfterADay, + selfSignatureExpiredAfterADay.PrivateKey, twoDaysLater, + }, + "after the binding signature expired": { + withSubkey, subkey, twoDaysLater, + }, + "before the key was made": { + madeADayLater, madeADayLater.PrivateKey, made, + }, + "before a key made in 2100": { + madeIn2100, madeIn2100.PrivateKey, made, + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + data := []byte("signed in 2020") + + pubKey, err := armoredPublicKey(test.key) + require.NoError(t, err) + + _, err = verifySignature(data, + signDated(t, test.signer, data, test.when), pubKey) + require.ErrorIs(t, err, errKeyNotValidWhenSigned) + }) + } +} + func TestManifestSignatureVerification(t *testing.T) { t.Parallel() -- 2.54.0