diff --git a/signature/docker.go b/signature/docker.go index b313231a8..ac7e8df21 100644 --- a/signature/docker.go +++ b/signature/docker.go @@ -51,6 +51,24 @@ func SignDockerManifest(m []byte, dockerReference string, mech SigningMechanism, // using mech. func VerifyDockerManifestSignature(unverifiedSignature, unverifiedManifest []byte, expectedDockerReference string, mech SigningMechanism, expectedKeyIdentity string) (*Signature, error) { + _, signerKeyIdentity, err := mech.Verify(unverifiedSignature) + if err != nil { + return nil, err + } + + // If signerKeyIdentity is different from expectedKeyIdentity, check if it's a valid subkey + if signerKeyIdentity != expectedKeyIdentity { + // Implementation depends on mechanism-specific functionality to verify subkey relationships + // This could be a new method on the SigningMechanism interface or mechanism-specific implementation + isValidSubkey, err := isKeyOrValidSubkey(mech, signerKeyIdentity, expectedKeyIdentity) + if err != nil { + return nil, err + } + if !isValidSubkey { + return nil, fmt.Errorf("signature by key %s does not match expected key %s", + signerKeyIdentity, expectedKeyIdentity) + } + } sig, _, err := VerifyImageManifestSignatureUsingKeyIdentityList(unverifiedSignature, unverifiedManifest, expectedDockerReference, mech, []string{expectedKeyIdentity}) return sig, err } diff --git a/signature/fixtures/.gitignore b/signature/fixtures/.gitignore index 2772b97f7..37fbda725 100644 --- a/signature/fixtures/.gitignore +++ b/signature/fixtures/.gitignore @@ -1,6 +1,8 @@ /*.gpg~ /.gpg-v21-migrated /private-keys-v1.d +/openpgp-revocs.d /random_seed /gnupg_spawn_agent_sentinel.lock +/tofu.db /.#* diff --git a/signature/fixtures/dir-img-rev-subkey/signature-1 b/signature/fixtures/dir-img-rev-subkey/signature-1 new file mode 100644 index 000000000..232941fb5 Binary files /dev/null and b/signature/fixtures/dir-img-rev-subkey/signature-1 differ diff --git a/signature/fixtures/dir-img-valid-3/manifest.json b/signature/fixtures/dir-img-valid-3/manifest.json new file mode 100644 index 000000000..198da23f9 --- /dev/null +++ b/signature/fixtures/dir-img-valid-3/manifest.json @@ -0,0 +1,26 @@ +{ + "schemaVersion": 2, + "mediaType": "application/vnd.docker.distribution.manifest.v2+json", + "config": { + "mediaType": "application/vnd.docker.container.image.v1+json", + "size": 7023, + "digest": "sha256:b5b2b2c507a0944348e0303114d8d93aaaa081732b86451d9bce1f432a537bc7" + }, + "layers": [ + { + "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip", + "size": 32654, + "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f" + }, + { + "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip", + "size": 16724, + "digest": "sha256:3c3a4604a545cdc127456d94e421cd355bca5b528f4a9c1905b15da2eb4a4c6b" + }, + { + "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip", + "size": 73109, + "digest": "sha256:ec4b8955958665577945c89419d1af06b5f7636b4ac3da7f12184802ad867736" + } + ] +} \ No newline at end of file diff --git a/signature/fixtures/dir-img-valid-3/signature-1 b/signature/fixtures/dir-img-valid-3/signature-1 new file mode 100644 index 000000000..5a05b8cdb Binary files /dev/null and b/signature/fixtures/dir-img-valid-3/signature-1 differ diff --git a/signature/fixtures/public-key-3.gpg b/signature/fixtures/public-key-3.gpg new file mode 100644 index 000000000..4b0010c5f Binary files /dev/null and b/signature/fixtures/public-key-3.gpg differ diff --git a/signature/fixtures/public-key-4.gpg b/signature/fixtures/public-key-4.gpg new file mode 100644 index 000000000..95a55c0d9 Binary files /dev/null and b/signature/fixtures/public-key-4.gpg differ diff --git a/signature/fixtures/pubring.gpg b/signature/fixtures/pubring.gpg index 8cad08aff..0ccff288e 100644 Binary files a/signature/fixtures/pubring.gpg and b/signature/fixtures/pubring.gpg differ diff --git a/signature/fixtures/trustdb.gpg b/signature/fixtures/trustdb.gpg index fe3c3d905..96680a97e 100644 Binary files a/signature/fixtures/trustdb.gpg and b/signature/fixtures/trustdb.gpg differ diff --git a/signature/fixtures_info_test.go b/signature/fixtures_info_test.go index 2ab4ed05a..1f411dbf4 100644 --- a/signature/fixtures_info_test.go +++ b/signature/fixtures_info_test.go @@ -19,6 +19,10 @@ const ( TestKeyFingerprintWithPassphrase = "F2B501009F78B0B340221A12A3CD242DA6028093" // TestPassphrase is the passphrase for TestKeyFingerprintWithPassphrase. TestPassphrase = "WithPassphrase123" + // TestKeyFingerprintWithSubkey is the fingerpring of the private key with a signing subkey in this directory. + TestKeyFingerprintWithSubkey = "92697D73924E7ADC91C14A001065C1A47B86FD50" + // TestKeyFingerprintWithRevokedSubkey is the fingerprint of the private key with a revoked signing subkey in this directory. + TestKeyFingerprintWithRevokedSubkey = "F501EDF1A5CD65421FCB51B57EB92DC581193558" ) var ( diff --git a/signature/mechanism.go b/signature/mechanism.go index 1d3fe0fdc..a78875370 100644 --- a/signature/mechanism.go +++ b/signature/mechanism.go @@ -27,7 +27,9 @@ type SigningMechanism interface { // Sign creates a (non-detached) signature of input using keyIdentity. // Fails with a SigningNotSupportedError if the mechanism does not support signing. Sign(input []byte, keyIdentity string) ([]byte, error) - // Verify parses unverifiedSignature and returns the content and the signer's identity + // Verify parses unverifiedSignature and returns the content and the signer's identity. + // For GPG signatures, the signer's identity is the fingerprint of the key or subkey + // that created the signature, not necessarily the primary key fingerprint. Verify(unverifiedSignature []byte) (contents []byte, keyIdentity string, err error) // UntrustedSignatureContents returns UNTRUSTED contents of the signature WITHOUT ANY VERIFICATION, // along with a short identifier of the key used for signing. diff --git a/signature/mechanism_gpgme.go b/signature/mechanism_gpgme.go index 8a8c5878a..fe145b936 100644 --- a/signature/mechanism_gpgme.go +++ b/signature/mechanism_gpgme.go @@ -204,3 +204,59 @@ func (m *gpgmeSigningMechanism) Verify(unverifiedSignature []byte) (contents []b func (m *gpgmeSigningMechanism) UntrustedSignatureContents(untrustedSignature []byte) (untrustedContents []byte, shortKeyIdentifier string, err error) { return gpgUntrustedSignatureContents(untrustedSignature) } + +// isSubkeyOf checks if signerKeyIdentity is a subkey of expectedKeyIdentity +func (m *gpgmeSigningMechanism) isSubkeyOf(signerKeyIdentity, expectedKeyIdentity string) (bool, error) { + // Load the key for expectedKeyIdentity + key, err := m.ctx.GetKey(expectedKeyIdentity, false) + if err != nil { + return false, err + } + + // Check if signerKeyIdentity is a subkey of the primary key + // Get the first subkey (which should be the primary key itself) + subkey := key.SubKeys() + + // Skip the primary key if needed and iterate through all subkeys + for subkey != nil { + // Check if the current subkey's fingerprint matches the signer's identity + if subkey.Fingerprint() == signerKeyIdentity { + return true, nil + } + + // Move to the next subkey + subkey = subkey.Next() + } + + // If we didn't find it as a subkey, check if it might be the primary key itself + // (in case the expectedKeyIdentity is actually a subkey) + key, err = m.ctx.GetKey(signerKeyIdentity, false) + if err != nil { + return false, err + } + + // Get the primary key fingerprint + primarySubkey := key.SubKeys() + if primarySubkey != nil && primarySubkey.Fingerprint() == expectedKeyIdentity { + return true, nil + } + + return false, nil +} + +// isKeyOrValidSubkey checks if the signerKeyIdentity is either the same as the expectedKeyIdentity, or is a valid +// subkey of the expectedKeyIdentity +func isKeyOrValidSubkey(mech SigningMechanism, signerKeyIdentity, expectedKeyIdentity string) (bool, error) { + // If they're the same, no need to check subkey relationship + if signerKeyIdentity == expectedKeyIdentity { + return true, nil + } + + // For gpgme mechanism, check subkey relationships + if gpgmeMech, ok := mech.(*gpgmeSigningMechanism); ok { + return gpgmeMech.isSubkeyOf(signerKeyIdentity, expectedKeyIdentity) + } + + // For other mechanisms, only accept exact matches + return false, nil +} diff --git a/signature/mechanism_openpgp.go b/signature/mechanism_openpgp.go index fea8590e1..5071ee552 100644 --- a/signature/mechanism_openpgp.go +++ b/signature/mechanism_openpgp.go @@ -166,7 +166,8 @@ func (m *openpgpSigningMechanism) Verify(unverifiedSignature []byte) (contents [ } // Uppercase the fingerprint to be compatible with gpgme - return content, strings.ToUpper(fmt.Sprintf("%x", md.SignedBy.PublicKey.Fingerprint)), nil + keyIdentity = strings.ToUpper(fmt.Sprintf("%x", md.SignedBy.PublicKey.Fingerprint)) + return content, keyIdentity, nil } // UntrustedSignatureContents returns UNTRUSTED contents of the signature WITHOUT ANY VERIFICATION, @@ -177,3 +178,101 @@ func (m *openpgpSigningMechanism) Verify(unverifiedSignature []byte) (contents [ func (m *openpgpSigningMechanism) UntrustedSignatureContents(untrustedSignature []byte) (untrustedContents []byte, shortKeyIdentifier string, err error) { return gpgUntrustedSignatureContents(untrustedSignature) } + +// isSubkeyOf checks if signerKeyIdentity is a subkey of expectedKeyIdentity +func (m *openpgpSigningMechanism) isSubkeyOf(signerKeyIdentity, expectedKeyIdentity string) (bool, error) { + // Convert fingerprints to lowercase for comparison + signerFingerprint := strings.ToLower(signerKeyIdentity) + expectedFingerprint := strings.ToLower(expectedKeyIdentity) + + // If they're the same, it's a match + if signerFingerprint == expectedFingerprint { + return true, nil + } + + // Find the entity with the expected fingerprint + var expectedEntity *openpgp.Entity + var signerEntity *openpgp.Entity + + 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)) + if primaryFingerprint == expectedFingerprint { + expectedEntity = entity + } + if primaryFingerprint == signerFingerprint { + signerEntity = entity + } + } + + // Also check subkeys + for _, subkey := range entity.Subkeys { + if subkey.PublicKey != nil { + subkeyFingerprint := strings.ToLower(fmt.Sprintf("%x", subkey.PublicKey.Fingerprint)) + if subkeyFingerprint == expectedFingerprint { + expectedEntity = entity + } + if subkeyFingerprint == signerFingerprint { + signerEntity = entity + } + } + } + } + + // If we couldn't find either key, return false + if expectedEntity == nil || signerEntity == nil { + return false, nil + } + + // Case 1: Both keys belong to the same entity - this is valid + if expectedEntity == signerEntity { + return true, nil + } + + // Case 2: The signer is a subkey of the expected entity + if expectedEntity.PrimaryKey != nil { + expectedPrimaryFingerprint := strings.ToLower(fmt.Sprintf("%x", expectedEntity.PrimaryKey.Fingerprint)) + if expectedPrimaryFingerprint == expectedFingerprint { + // The expected key is a primary key, check if signer is one of its subkeys + for _, subkey := range expectedEntity.Subkeys { + if subkey.PublicKey != nil { + subkeyFingerprint := strings.ToLower(fmt.Sprintf("%x", subkey.PublicKey.Fingerprint)) + if subkeyFingerprint == signerFingerprint { + return true, nil + } + } + } + } + } + + // Case 3: The expected key is a subkey, and the signer is the primary key of the same entity + for _, subkey := range signerEntity.Subkeys { + if subkey.PublicKey != nil { + subkeyFingerprint := strings.ToLower(fmt.Sprintf("%x", subkey.PublicKey.Fingerprint)) + if subkeyFingerprint == expectedFingerprint { + // The expected key is a subkey of the signer's entity + return true, nil + } + } + } + + return false, nil +} + +// isKeyOrValidSubkey checks if the signerKeyIdentity is either the same as the expectedKeyIdentity, or is a valid +// subkey of the expectedKeyIdentity +func isKeyOrValidSubkey(mech SigningMechanism, signerKeyIdentity, expectedKeyIdentity string) (bool, error) { + // If they're the same, no need to check subkey relationship + if signerKeyIdentity == expectedKeyIdentity { + return true, nil + } + + // For openpgp mechanism, check subkey relationships + if openpgpMech, ok := mech.(*openpgpSigningMechanism); ok { + return openpgpMech.isSubkeyOf(signerKeyIdentity, expectedKeyIdentity) + } + + // For other mechanisms, only accept exact matches + return false, nil +} diff --git a/signature/mechanism_test.go b/signature/mechanism_test.go index 566993e73..1d01b85fa 100644 --- a/signature/mechanism_test.go +++ b/signature/mechanism_test.go @@ -143,7 +143,16 @@ func TestNewEphemeralGPGSigningMechanism(t *testing.T) { mech, keyIdentities, err = NewEphemeralGPGSigningMechanism(bytes.Join([][]byte{keyBlob, keyBlob}, nil)) require.NoError(t, err) defer mech.Close() - assert.Equal(t, []string{TestKeyFingerprintWithPassphrase, TestKeyFingerprint, TestKeyFingerprintWithPassphrase, TestKeyFingerprint}, keyIdentities) + assert.Equal(t, []string{ + TestKeyFingerprintWithPassphrase, + TestKeyFingerprint, + TestKeyFingerprintWithSubkey, + TestKeyFingerprintWithRevokedSubkey, + TestKeyFingerprintWithPassphrase, + TestKeyFingerprint, + TestKeyFingerprintWithSubkey, + TestKeyFingerprintWithRevokedSubkey, + }, keyIdentities) // Two keys from two blobs: keyBlob1, err := os.ReadFile("./fixtures/public-key-1.gpg") diff --git a/signature/policy_eval_signedby.go b/signature/policy_eval_signedby.go index e5c932918..e398eff0f 100644 --- a/signature/policy_eval_signedby.go +++ b/signature/policy_eval_signedby.go @@ -6,7 +6,6 @@ import ( "context" "errors" "fmt" - "slices" "github.com/containers/image/v5/internal/multierr" "github.com/containers/image/v5/internal/private" @@ -52,8 +51,15 @@ func (pr *prSignedBy) isSignatureAuthorAccepted(ctx context.Context, image priva signature, err := verifyAndExtractSignature(mech, sig, signatureAcceptanceRules{ validateKeyIdentity: func(keyIdentity string) error { - if slices.Contains(trustedIdentities, keyIdentity) { - return nil + // Iterate over the keys, validate if the provided keyIdentity is either the key itself or a valid + // subkey + for _, allowedKeyIdentity := range trustedIdentities { + if isValid, err := isKeyOrValidSubkey(mech, keyIdentity, allowedKeyIdentity); err != nil { + // Throwing an error shouldn't exit the program, we should just move to the next key + continue + } else if isValid { + return nil + } } // Coverage: We use a private GPG home directory and only import trusted keys, so this should // not be reachable. diff --git a/signature/policy_eval_signedby_test.go b/signature/policy_eval_signedby_test.go index bca2e722c..bb29e04ea 100644 --- a/signature/policy_eval_signedby_test.go +++ b/signature/policy_eval_signedby_test.go @@ -207,6 +207,15 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) { require.NoError(t, err) sar, parsedSig, err = pr.isSignatureAuthorAccepted(context.Background(), image, sig) assertSARRejectedPolicyRequirement(t, sar, parsedSig, err) + + // An invalid signature from a revoked signing subkey + image = dirImageMock(t, "fixtures/dir-img-rev-subkey", "testing/manifest:latest") + sig, err = os.ReadFile("fixtures/dir-img-rev-subkey/signature-1") + require.NoError(t, err) + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key-4.gpg", prm) + require.NoError(t, err) + sar, parsedSig, err = pr.isSignatureAuthorAccepted(context.Background(), image, sig) + assertSARRejected(t, sar, parsedSig, err) } // createInvalidSigDir creates a directory suitable for dirImageMock, in which image.Signatures() @@ -277,4 +286,11 @@ func TestPRSignedByIsRunningImageAllowed(t *testing.T) { require.NoError(t, err) allowed, err = pr.isRunningImageAllowed(context.Background(), image) assertRunningRejectedPolicyRequirement(t, allowed, err) + + // 1 valid signature from a subkey + image = dirImageMock(t, "fixtures/dir-img-valid-3", "testing/manifest:latest") + pr, err = NewPRSignedByKeyPath(ktGPG, "fixtures/public-key-3.gpg", prm) + require.NoError(t, err) + allowed, err = pr.isRunningImageAllowed(context.Background(), image) + assertRunningAllowed(t, allowed, err) }