Add support for validating signatures from PGP subkeys #2874

drGrove · 2025-06-22T08:45:51Z

This change should update the gpgme and openpgp implmenetations to support validating signatures made by signing subkeys. It will close an open issue that was referenced specifically in podman, but should resolve this across a number of tools that use this library.

mtrmac

Thanks!

containers/podman#16406 contains a long discussion about expired (or revoked) keys, IIRC it’s fairly unclear what is the right thing to do for long-term artifacts like software, and with no trusted timestamps.

Adding what, 150? new lines of code to the security critical signing code path is also a bit unattractive (although that’s, by itself, not at all a reason not to do the right thing).

There’s also a bit of awkward timing in that #2569 will probably replace the default / recommended verification implementation.

OTOH, these design concerns are, right now, also a topic there: #2569 (comment) .

drGrove · 2025-06-23T16:15:24Z

There’s also a bit of awkward timing in that #2569 will probably replace the default / recommended verification implementation.

I noticed this as I was finishing out the implementation but based on my understanding of the comments. It appears the GPGPME/Go OpenPGP will still exist, it just won't be the hot path. As the sequoia OpenPGP implementation has also been something that's still being worked though, having any support for subkeys IMO is ideal.

containers/podman#16406 contains a long discussion about expired (or revoked) keys, IIRC it’s fairly unclear what is the right thing to do for long-term artifacts like software, and with no trusted timestamps.

To my understanding, once a key is revoked or expired the signature is no longer valid. Regardless of when it was signed. This default to close IMO is a feature. Revocations are packets within the key just like anything else and generally the systems/users doing validation of OpenPGP signatures understand the implications of this. If a key is revoked and pushed to a keyserver, the user/machine has to pull down the updated packets for it to take effect within the verification pipeline. If you are aware that a revocation has been published but you still need to use a previously signed image it would probably be more ideal for you to mirror the image and sign it yourself than the much less secure option of not importing the updated key. This same problem exists is most signing schemes (key rotation).

mtrmac · 2025-07-16T22:55:06Z

Note to self: Sequoia, by default, generates keys where the signing key is a subkey, so we almost certainly need some version of this.

drGrove · 2025-07-16T22:58:30Z

I've been meaning to rebase and fix this now that a whole new set of keyrings exist. Happy to do so

mtrmac · 2025-07-16T23:09:44Z

I think it would be fair for me to first actually read this PR at least once :)

Signed-off-by: Danny Grove <[email protected]>

This change should update the gpgme and openpgp implmenetations to support validating signatures made by signing subkeys. It will close an open issue that was referenced specifically in [podman](containers/podman#16406), but should resolve this across a number of tools that use this library. Signed-off-by: Danny Grove <[email protected]>

Signed-off-by: Danny Grove <[email protected]>

drGrove · 2025-07-18T17:20:55Z

@mtrmac everything has been rebased and 2 keyrings have been added. One for the subkey validation and 1 for policy checking around revoked subkeys after a signature is made.

As a sidenote, it might be easier longer term to export the secret keys to file and rebuild the keyring on the fly. Then adding/removing or replacing keys becomes a more atomic process

mtrmac

Thanks!

Getting back to this now:

The design of verifyAndExtractSignature / signatureAcceptanceRules is that it centralizes all of the “is a signature valid“ policy for simple signatures. There should be no policy anywhere outside that function, other than what is defined as explicit parameters to that function. In particular, no two code paths should differ of what is permitted / rejected (other than explicit variable parameters like “which public key is trusted”).

So, with this PR, we end up with three code paths:

VerifyImageManifestSignatureUsingKeyIdentityList just calling verifyAndExtractSignature (with primary-key-only matching)
VerifyDockerManifestSignature imposing a subkey restriction in advance, and then calling verifyAndExtractSignature (with the old-style primary-key-only matching still happening)
prSignedBy.isSignatureAuthorAccepted using verifyAndExtractSignature, and taking subkeys into account in signatureAcceptanceRules

That’s not acceptable.

But, also, verifyAndExtractSignature is private and we have full freedom to redesign it as necessary.

Eventually, there should be unit test code coverage of ~every reasonably reachable line in ./signature. But we can deal with that after the structure settles.

mtrmac · 2025-07-21T16:53:43Z

signature/docker.go

 // using mech.
 func VerifyDockerManifestSignature(unverifiedSignature, unverifiedManifest []byte,
 	expectedDockerReference string, mech SigningMechanism, expectedKeyIdentity string) (*Signature, error) {
+	_, signerKeyIdentity, err := mech.Verify(unverifiedSignature)


I don’t understand the location (and to an extent, purpose) of this.

VerifyImageManifestSignatureUsingKeyIdentityList is a public API. It must also handle the situation correctly.

This does an extra cryptographic verification, an unwanted overhead

… and AFAICS it doesn’t affect expectedKeyIdentity, so a subkey signature would still fail in this function.

mtrmac · 2025-07-21T17:02:31Z

signature/mechanism_gpgme.go

+
+	// Get the primary key fingerprint
+	primarySubkey := key.SubKeys()
+	if primarySubkey != nil && primarySubkey.Fingerprint() == expectedKeyIdentity {


I don’t know whether we should support setting expectedKeyIdentity to a subkey at all; but if we would accept that, then it doesn’t make sense to me to accept any other key than that particular subkey.

Also, this functionality directly contradicts the function name. Such a confusion in security-critical code is a risk in itself.

Generally you'd use !<subkey fingerprint> though I don't believe the current container-polices.d spec makes any specific note of that. But since that is a common to gpg signing implementation not supporting it may be labeled as a bug.

For me, the primary thing that matters is the prSignedBy usage pattern, where newEphemeralGPGSigningMechanism is used with raw public keys, returns $identifiers, and then, while verifying signatures, we sanity-check that the signatures not only are valid in general, but match $identifiers.

(One could argue that the sanity-check is entirely unnecessary, I suppose.)

The way the various implementations of newEphemeralGPGSigningMechanism import keys, $identifiers have always been fingerprints of the primary keys; so that’s the situation I most care about.

Non-prSignedBy callers of verification should get consistent results with prSignedBy, but I don’t worry much about cases where they might try to do something impossible to do via prSignedBy.

current container-polices.d spec

? Google finds nothing.

Apologies it's container-policy.json(5)

policy.json never had a feature to specify fingerprints of any kinds — users would point at whole keyrings.

Perfect then we at least don't have to worry about that piece. If an end-user only wanted to support signatures from a specific subkey they can generate the keyring they specifically care about.

mtrmac · 2025-07-21T17:05:14Z

signature/mechanism_gpgme.go

+}
+
+// isKeyOrValidSubkey checks if the signerKeyIdentity is either the same as the expectedKeyIdentity, or is a valid
+// subkey of the expectedKeyIdentity


I can’t see anything dealing with revocation / expiration, so what does “valid” mean here? (We very likely don’t need to explicitly handle revocation / expiration, but comments need to be clear about what the functions do.)

mtrmac · 2025-07-21T17:07:07Z

signature/mechanism_gpgme.go

+	}
+
+	// For gpgme mechanism, check subkey relationships
+	if gpgmeMech, ok := mech.(*gpgmeSigningMechanism); ok {


Having such a function copy&pasted for every transport is fragile and hard to follow. Centralizing the conditional support via an optional interface (perhaps something like signingMechanismWithPassphrase, though even the method could be private), and handling the condition in a single caller, would make this more explicit.

I wasn't sure of the impact of centralizing that. I see at the top of the file there are flags for gpgme vs. openpgp. This is a part of go that I am not familiar with so I erred on the side of separation based on the mechanism used since it seemed like the most straight-forward and easy to reason about approach

mtrmac · 2025-07-21T17:08:42Z

signature/mechanism_openpgp.go

+	for _, entity := range m.keyring {
+		// Check if this entity's primary key matches the expected fingerprint
+		if entity.PrimaryKey != nil {
+			primaryFingerprint := strings.ToLower(fmt.Sprintf("%x", entity.PrimaryKey.Fingerprint))


%x is already lower-case.

mtrmac · 2025-07-21T17:14:12Z

signature/mechanism_openpgp.go

+		}
+	}
+
+	// Case 3: The expected key is a subkey, and the signer is the primary key of the same entity


Similarly to the GPG case, with expectedFingerprint matching a subkey, ignoring that and accepting any other subkey is surprising.

mtrmac

Looking at the possible implementation approaches:

… Sequoia in #2876 is already implemented to return the primary key’s fingerprint as an outcome of verification.

mtrmac · 2025-07-21T17:48:59Z

signature/mechanism_gpgme.go

 }
+
+// isSubkeyOf checks if signerKeyIdentity is a subkey of expectedKeyIdentity
+func (m *gpgmeSigningMechanism) isSubkeyOf(signerKeyIdentity, expectedKeyIdentity string) (bool, error) {


Reading the GPGME implementation, yes, we’ll probably need to issue a GetKey of some kind to be able to check these things.

mtrmac · 2025-07-21T17:49:58Z

signature/mechanism_openpgp.go

 }
+
+// isSubkeyOf checks if signerKeyIdentity is a subkey of expectedKeyIdentity
+func (m *openpgpSigningMechanism) isSubkeyOf(signerKeyIdentity, expectedKeyIdentity string) (bool, error) {


It seems that we can immediately read md.SignedBy.Entity.PrimaryKey.Fingerprint to get the primary key. I didn’t yet verify that it is always available.

mtrmac · 2025-07-21T19:07:20Z

@drGrove I think the structure in #2905 might be an alternative approach:

Centralize more of the logic into verifyAndExtractSignature
For backends that can return the primary key directly, just let them do that
For GPGME, special-case and do the lookup. (That part doesn’t work there now, I just wanted the sketch to do ASAP.)

mtrmac · 2025-07-21T19:17:09Z

As a sidenote, it might be easier longer term to export the secret keys to file and rebuild the keyring on the fly. Then adding/removing or replacing keys becomes a more atomic process

I think at least for the primary success smoke cases, having a test work with “the expected use case” is valuable: If users have a long-term key stored in a GNUPGHOME, some test should work against an existing key in GNUPGHOME file structure, to not risk that an import might affect things. (Also, the unit tests tend to run during interactive development in IDEs, so there’s a bit of a preference to avoid expensive operations and external tools.)

Admittedly, using a GNUPGHOME fixture with a pre-GnuPG 2.1 layout is increasingly less representative.

There’s also the history of #2875 — it would have been nice, at that point, to have a script that regenerates everything, not just the keyrings, but also the public keys and signatures. In that sense, rebuilding keyrings but still using manually-built keys would be a halfway step. But then again, hopefully we won’t need to regenerate keys again, for a long time.

drGrove · 2025-07-21T19:18:52Z

Looking at the possible implementation approaches:

… Sequoia in #2876 is already implemented to return the primary key’s fingerprint as an outcome of verification.

That doesn't seem to align with supporting pinned signing subkeys then

mtrmac · 2025-07-21T19:22:10Z

Hum, not just accepting signatures by subkeys, but specifically restricting the policy to only accept a specified subkey (if that’s what you mean) would be yet another separate feature — notably it would need a new config option, I think.

@drGrove

… a subkey signature This matches Sequoia, and allows accepting subkey signatures via signatureAcceptanceRules. FIXME: Needs tests - for keyIdentityForVerificationKeyIdentity - replace keys, generate v3 signatures. Currently the new keys+sigs come from containers#2874, credit to @drGrove Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac · 2025-08-20T17:33:12Z

#2905 was merged. Thanks for this work!

drGrove force-pushed the drgrove/fix-pgp-subkey-verification branch from 77893fc to f79e1b8 Compare June 22, 2025 22:38

mtrmac reviewed Jun 23, 2025

View reviewed changes

mtrmac mentioned this pull request Jun 23, 2025

signature: add OpenPGP signing mechanism based on Sequoia #2569

Merged

drGrove force-pushed the drgrove/fix-pgp-subkey-verification branch from f79e1b8 to 02743dc Compare July 17, 2025 07:56

drGrove added 2 commits July 17, 2025 11:18

Generate new key with signing subkey

1f7ff52

Signed-off-by: Danny Grove <[email protected]>

drGrove force-pushed the drgrove/fix-pgp-subkey-verification branch 3 times, most recently from 7ef722b to 7bedb9b Compare July 18, 2025 07:16

drGrove added 2 commits July 18, 2025 00:16

Add a new key with a revoked signing subkey after the signature time

4161631

Signed-off-by: Danny Grove <[email protected]>

Add test for revoked signing subkeys

bd6e15e

Signed-off-by: Danny Grove <[email protected]>

drGrove force-pushed the drgrove/fix-pgp-subkey-verification branch from 7bedb9b to bd6e15e Compare July 18, 2025 07:17

mtrmac requested changes Jul 21, 2025

View reviewed changes

mtrmac reviewed Jul 21, 2025

View reviewed changes

mtrmac mentioned this pull request Jul 21, 2025

Accept subkey signatures #2905

Merged

mtrmac closed this Aug 20, 2025

Add support for validating signatures from PGP subkeys #2874

Add support for validating signatures from PGP subkeys #2874

Uh oh!

Conversation

drGrove commented Jun 22, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

drGrove commented Jun 23, 2025

Uh oh!

mtrmac commented Jul 16, 2025

Uh oh!

drGrove commented Jul 16, 2025

Uh oh!

mtrmac commented Jul 16, 2025

Uh oh!

drGrove commented Jul 18, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drGrove Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Jul 21, 2025

Uh oh!

mtrmac commented Jul 21, 2025

Uh oh!

drGrove commented Jul 21, 2025

Uh oh!

mtrmac commented Jul 21, 2025

Uh oh!

mtrmac commented Aug 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

drGrove Jul 21, 2025 •

edited

Loading