From b0bab23d7d05cc8c6117e7b2bfad8dc2f5190331 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 13 Jul 2022 16:32:18 +0200 Subject: [PATCH 1/8] Fix a typo in error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- signature/policy_config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/signature/policy_config.go b/signature/policy_config.go index d3c07d03b..61eb6e99f 100644 --- a/signature/policy_config.go +++ b/signature/policy_config.go @@ -408,7 +408,7 @@ func (pr *prSignedBy) UnmarshalJSON(data []byte) error { case !gotKeyPath && gotKeyData: res, err = newPRSignedByKeyData(tmp.KeyType, tmp.KeyData, tmp.SignedIdentity) case !gotKeyPath && !gotKeyData: - return InvalidPolicyFormatError("At least one of keyPath and keyData mus be specified") + return InvalidPolicyFormatError("At least one of keyPath and keyData must be specified") default: // Coverage: This should never happen return fmt.Errorf("Impossible keyPath/keyData presence combination!?") } @@ -582,7 +582,7 @@ func (pr *prSigstoreSigned) UnmarshalJSON(data []byte) error { case !gotKeyPath && gotKeyData: res, err = newPRSigstoreSignedKeyData(tmp.KeyData, tmp.SignedIdentity) case !gotKeyPath && !gotKeyData: - return InvalidPolicyFormatError("At least one of keyPath and keyData mus be specified") + return InvalidPolicyFormatError("At least one of keyPath and keyData must be specified") default: // Coverage: This should never happen return fmt.Errorf("Impossible keyPath/keyData presence combination!?") } From db58334da68ab06baf2dbc97716e878dee017244 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 13 Jul 2022 07:34:58 +0200 Subject: [PATCH 2/8] Remove a copy&pasted test entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- signature/policy_config_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/signature/policy_config_test.go b/signature/policy_config_test.go index 5572869bb..da0dff890 100644 --- a/signature/policy_config_test.go +++ b/signature/policy_config_test.go @@ -802,7 +802,6 @@ func TestPRSignedByUnmarshalJSON(t *testing.T) { func(v mSI) { v["keyPath"] = "/foo/bar" }, // Invalid "keyPath" field func(v mSI) { delete(v, "keyData"); v["keyPath"] = 1 }, - func(v mSI) { v["type"] = "this is invalid" }, // Invalid "keyData" field func(v mSI) { v["keyData"] = 1 }, func(v mSI) { v["keyData"] = "this is invalid base64" }, From f17fc55b819d409419d8b0c0fc6c58ae0ab8ced7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 13 Jul 2022 07:33:18 +0200 Subject: [PATCH 3/8] Add context to some test failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to make it easier to figure out which of breakFns failed. Signed-off-by: Miloslav Trmač --- signature/policy_config_test.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/signature/policy_config_test.go b/signature/policy_config_test.go index da0dff890..24c8f20cd 100644 --- a/signature/policy_config_test.go +++ b/signature/policy_config_test.go @@ -3,6 +3,7 @@ package signature import ( "bytes" "encoding/json" + "fmt" "os" "path/filepath" "testing" @@ -342,15 +343,17 @@ func (d policyJSONUmarshallerTests) run(t *testing.T) { assertJSONUnmarshalFromObjectFails(t, invalid, dest) } // Various ways to corrupt the JSON - for _, fn := range d.breakFns { - var tmp mSI - err := json.Unmarshal(validJSON, &tmp) - require.NoError(t, err) + for index, fn := range d.breakFns { + t.Run(fmt.Sprintf("breakFns[%d]", index), func(t *testing.T) { + var tmp mSI + err := json.Unmarshal(validJSON, &tmp) + require.NoError(t, err) - fn(tmp) + fn(tmp) - dest := d.newDest() - assertJSONUnmarshalFromObjectFails(t, tmp, dest) + dest := d.newDest() + assertJSONUnmarshalFromObjectFails(t, tmp, dest) + }) } // Duplicated fields From e37f4dbf3ea1b3c2c7f0ed74e5064ceb62054a26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 13 Jul 2022 08:13:34 +0200 Subject: [PATCH 4/8] Use more valid data in TestPRSignedByIsSignatureAuthorAccepted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to make sure we are exercising exactly the failure pathss we are trying to test. Signed-off-by: Miloslav Trmač --- signature/policy_eval_signedby_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/signature/policy_eval_signedby_test.go b/signature/policy_eval_signedby_test.go index e234617b7..cde078eba 100644 --- a/signature/policy_eval_signedby_test.go +++ b/signature/policy_eval_signedby_test.go @@ -55,6 +55,8 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) { testImage := dirImageMock(t, "fixtures/dir-img-valid", "testing/manifest:latest") testImageSig, err := os.ReadFile("fixtures/dir-img-valid/signature-1") require.NoError(t, err) + keyData, err := os.ReadFile("fixtures/public-key.gpg") + require.NoError(t, err) // Successful validation, with KeyData and KeyPath pr, err := NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) @@ -65,8 +67,6 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) { DockerReference: "testing/manifest:latest", }) - keyData, err := os.ReadFile("fixtures/public-key.gpg") - require.NoError(t, err) pr, err = NewPRSignedByKeyData(ktGPG, keyData, prm) require.NoError(t, err) sar, parsedSig, err = pr.isSignatureAuthorAccepted(context.Background(), testImage, testImageSig) @@ -84,7 +84,7 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) { // Do not use NewPRSignedByKeyData, because it would reject invalid values. pr := &prSignedBy{ KeyType: keyType, - KeyData: []byte("abc"), + KeyData: keyData, SignedIdentity: prm, } // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. @@ -95,8 +95,8 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) { // Both KeyPath and KeyData set. Do not use NewPRSignedBy*, because it would reject this. prSB := &prSignedBy{ KeyType: ktGPG, - KeyPath: "/foo/bar", - KeyData: []byte("abc"), + KeyPath: "fixtures/public-key.gpg", + KeyData: keyData, SignedIdentity: prm, } // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. From a7ef900a5625b18e7d2b38bcf5a4442c331c0e88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 13 Jul 2022 07:31:29 +0200 Subject: [PATCH 5/8] Generalize keyPath/keyData exclusivity checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to make it a bit easier to add one more variant. Should not change behavior (apart from error messages). Signed-off-by: Miloslav Trmač --- signature/policy_config.go | 17 +++++++++++------ signature/policy_eval_signedby.go | 16 ++++++++++------ 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/signature/policy_config.go b/signature/policy_config.go index 61eb6e99f..9c36dca98 100644 --- a/signature/policy_config.go +++ b/signature/policy_config.go @@ -320,8 +320,15 @@ func newPRSignedBy(keyType sbKeyType, keyPath string, keyData []byte, signedIden if !keyType.IsValid() { return nil, InvalidPolicyFormatError(fmt.Sprintf("invalid keyType \"%s\"", keyType)) } - if len(keyPath) > 0 && len(keyData) > 0 { - return nil, InvalidPolicyFormatError("keyType and keyData cannot be used simultaneously") + keySources := 0 + if keyPath != "" { + keySources++ + } + if keyData != nil { + keySources++ + } + if keySources != 1 { + return nil, InvalidPolicyFormatError("exactly one of keyPath and keyData must be specified") } if signedIdentity == nil { return nil, InvalidPolicyFormatError("signedIdentity not specified") @@ -401,16 +408,14 @@ func (pr *prSignedBy) UnmarshalJSON(data []byte) error { var res *prSignedBy var err error switch { - case gotKeyPath && gotKeyData: - return InvalidPolicyFormatError("keyPath and keyData cannot be used simultaneously") case gotKeyPath && !gotKeyData: res, err = newPRSignedByKeyPath(tmp.KeyType, tmp.KeyPath, tmp.SignedIdentity) case !gotKeyPath && gotKeyData: res, err = newPRSignedByKeyData(tmp.KeyType, tmp.KeyData, tmp.SignedIdentity) case !gotKeyPath && !gotKeyData: return InvalidPolicyFormatError("At least one of keyPath and keyData must be specified") - default: // Coverage: This should never happen - return fmt.Errorf("Impossible keyPath/keyData presence combination!?") + default: + return fmt.Errorf("Exactly one of keyPath and keyData must be specified, more than one present") } if err != nil { return err diff --git a/signature/policy_eval_signedby.go b/signature/policy_eval_signedby.go index b04e57382..38ca2750b 100644 --- a/signature/policy_eval_signedby.go +++ b/signature/policy_eval_signedby.go @@ -25,20 +25,24 @@ func (pr *prSignedBy) isSignatureAuthorAccepted(ctx context.Context, image priva return sarRejected, nil, fmt.Errorf(`Unknown "keyType" value "%s"`, string(pr.KeyType)) } - if pr.KeyPath != "" && pr.KeyData != nil { - return sarRejected, nil, errors.New(`Internal inconsistency: both "keyPath" and "keyData" specified`) - } // FIXME: move this to per-context initialization var data []byte - if pr.KeyData != nil { - data = pr.KeyData - } else { + keySources := 0 + if pr.KeyPath != "" { + keySources++ d, err := os.ReadFile(pr.KeyPath) if err != nil { return sarRejected, nil, err } data = d } + if pr.KeyData != nil { + keySources++ + data = pr.KeyData + } + if keySources != 1 { + return sarRejected, nil, errors.New(`Internal inconsistency: not exactly one of "keyPath" and "keyData" specified`) + } // FIXME: move this to per-context initialization mech, trustedIdentities, err := NewEphemeralGPGSigningMechanism(data) From 03c8d799d086aad5042d55b1589d8882a2160b5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 13 Jul 2022 07:42:50 +0200 Subject: [PATCH 6/8] Remove repetition in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to make it easier to add more cases. Should not change (test) behavior. Signed-off-by: Miloslav Trmač --- signature/policy_eval_signedby_test.go | 70 ++++++++++++++------------ 1 file changed, 37 insertions(+), 33 deletions(-) diff --git a/signature/policy_eval_signedby_test.go b/signature/policy_eval_signedby_test.go index cde078eba..c12f47505 100644 --- a/signature/policy_eval_signedby_test.go +++ b/signature/policy_eval_signedby_test.go @@ -59,21 +59,22 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) { require.NoError(t, err) // Successful validation, with KeyData and KeyPath - pr, err := NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) - require.NoError(t, err) - sar, parsedSig, err := pr.isSignatureAuthorAccepted(context.Background(), testImage, testImageSig) - assertSARAccepted(t, sar, parsedSig, err, Signature{ - DockerManifestDigest: TestImageManifestDigest, - DockerReference: "testing/manifest:latest", - }) - - pr, err = NewPRSignedByKeyData(ktGPG, keyData, prm) - require.NoError(t, err) - sar, parsedSig, err = pr.isSignatureAuthorAccepted(context.Background(), testImage, testImageSig) - assertSARAccepted(t, sar, parsedSig, err, Signature{ - DockerManifestDigest: TestImageManifestDigest, - DockerReference: "testing/manifest:latest", - }) + for _, fn := range []func() (PolicyRequirement, error){ + func() (PolicyRequirement, error) { + return NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) + }, + func() (PolicyRequirement, error) { + return NewPRSignedByKeyData(ktGPG, keyData, prm) + }, + } { + pr, err := fn() + require.NoError(t, err) + sar, parsedSig, err := pr.isSignatureAuthorAccepted(context.Background(), testImage, testImageSig) + assertSARAccepted(t, sar, parsedSig, err, Signature{ + DockerManifestDigest: TestImageManifestDigest, + DockerReference: "testing/manifest:latest", + }) + } // Unimplemented and invalid KeyType values for _, keyType := range []sbKeyType{SBKeyTypeSignedByGPGKeys, @@ -92,31 +93,34 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) { assertSARRejected(t, sar, parsedSig, err) } - // Both KeyPath and KeyData set. Do not use NewPRSignedBy*, because it would reject this. - prSB := &prSignedBy{ - KeyType: ktGPG, - KeyPath: "fixtures/public-key.gpg", - KeyData: keyData, - SignedIdentity: prm, + // Invalid KeyPath/KeyPaths/KeyData combinations. + for _, fn := range []func() (PolicyRequirement, error){ + // Both KeyPath and KeyData set. Do not use NewPRSignedBy*, because it would reject this. + func() (PolicyRequirement, error) { + return &prSignedBy{KeyType: ktGPG, KeyPath: "fixtures/public-key.gpg", KeyData: []byte("abc"), SignedIdentity: prm}, nil + }, + // Neither KeyPath nor KeyData set. Do not use NewPRSignedBy*, because it would reject this. + func() (PolicyRequirement, error) { + return &prSignedBy{KeyType: ktGPG, SignedIdentity: prm}, nil + }, + func() (PolicyRequirement, error) { // Invalid KeyPath + return NewPRSignedByKeyPath(ktGPG, "/this/does/not/exist", prm) + }, + } { + pr, err := fn() + require.NoError(t, err) + // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. + sar, parsedSig, err := pr.isSignatureAuthorAccepted(context.Background(), nil, nil) + assertSARRejected(t, sar, parsedSig, err) } - // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. - sar, parsedSig, err = prSB.isSignatureAuthorAccepted(context.Background(), nil, nil) - assertSARRejected(t, sar, parsedSig, err) - - // Invalid KeyPath - pr, err = NewPRSignedByKeyPath(ktGPG, "/this/does/not/exist", prm) - require.NoError(t, err) - // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. - sar, parsedSig, err = pr.isSignatureAuthorAccepted(context.Background(), nil, nil) - assertSARRejected(t, sar, parsedSig, err) // Errors initializing the temporary GPG directory and mechanism are not obviously easy to reach. // KeyData has no public keys. - pr, err = NewPRSignedByKeyData(ktGPG, []byte{}, prm) + pr, err := NewPRSignedByKeyData(ktGPG, []byte{}, prm) require.NoError(t, err) // Pass nil pointers to, kind of, test that the return value does not depend on the parameters. - sar, parsedSig, err = pr.isSignatureAuthorAccepted(context.Background(), nil, nil) + sar, parsedSig, err := pr.isSignatureAuthorAccepted(context.Background(), nil, nil) assertSARRejectedPolicyRequirement(t, sar, parsedSig, err) // A signature which does not GPG verify From 4cb7f876a0e10ee140f2cc5fd424cb5639949141 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 13 Jul 2022 08:26:12 +0200 Subject: [PATCH 7/8] Accept multiple keyrings in newEphemeralGPGSigningMechanism MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user will be added momentarily. For now, should not change behavior. Signed-off-by: Miloslav Trmač --- signature/fixtures/public-key-1.gpg | Bin 0 -> 653 bytes signature/fixtures/public-key-2.gpg | Bin 0 -> 737 bytes signature/mechanism.go | 2 +- signature/mechanism_gpgme.go | 14 +++++++++----- signature/mechanism_openpgp.go | 13 +++++++++---- signature/mechanism_test.go | 12 +++++++++++- 6 files changed, 30 insertions(+), 11 deletions(-) create mode 100644 signature/fixtures/public-key-1.gpg create mode 100644 signature/fixtures/public-key-2.gpg diff --git a/signature/fixtures/public-key-1.gpg b/signature/fixtures/public-key-1.gpg new file mode 100644 index 0000000000000000000000000000000000000000..c97f2e0add6351bf929fae32365501b2063ccf44 GIT binary patch literal 653 zcmV;80&@MBjRaQe^Qi#@0KMvvTDU7^5w#_i4T;cD&^;Z~J+h>>cmU5@7*12#?LJc>4dc*FJW7NhHeOYmqy%3}AzeWNz^{avASM@u>% zLK;N#?&FX*N_S-og+H|NHUnkmx zTQG!DKJAo5yh6(zxTmXkLCcp5T@|xHr%7bdiCR!UKNFr6ZwN4ChKMM5k&Npb#1O$xpG2bMIzOQo81E=b$*{xQzr>>hq}q z1OUhj|3a_{PMHu3xrHW@7b+4vg~EOCD?Obc81sVq=5 zZO}k>FgUEYGoWwjQz3nr-TPDZdR`CF@>1|&W`$%yl5-96F>He>AwlXcL%)1PQE&Hx ztj+YTHhTYQ?=46KokSW3n&SM9Xz nmU@Vz5Ge@esl0J~z^$#I`Ki5Rh10T#!+eSe&Y$5uBZ0keaU$l3HA%sbEtMly)e9OY5cPCT8Z?bzEW*X5?V7 z5mRJ&{JKo=hNz;vh@{)r4QHQmJB1dzu`+@D$Ry3o%FW5a&cv+5#K_=bBcs{P>%R6S%H<$9CoJOyt`-L(HA_q6~}TKiW*#(h+b+?yLeG1$#~iBX(A^s zPx+d!WPI_T_BrPj^{#7E?wV(5yZLVPZ)uZU^=0BBZncaj$J_(wotYN*Pg*8!#twMQ z+?c%LMBwc8=PhqWHI#3))D7;gopS0%MC`KSC%+RC1v@v^2TWO`m7%EC9iq=sVKGCu zneX|Om+}&u6Wllao1J)f;_M<$+sl`O#UEEK-jKOmtgmC|-i&nie>ok87^dHlJl>+o z*=n&U;LoHNQ~RSV^$e#*8zncy%oA~EmnsgrQ!NgTnT~BN62Kr+902O8Z}2zPh}LtY;cw4{xboW1h hUu@oJ_vCwr$nBhP9x3Amt6XMm`XW{CFjcB?H2_8dJ2L

Date: Wed, 13 Jul 2022 08:33:27 +0200 Subject: [PATCH 8/8] Allow accepting multiple GPG keyrings via signedBy.keyPaths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- docs/containers-policy.json.5.md | 3 +- signature/fixtures/policy.json | 7 +++ signature/policy_config.go | 39 +++++++++---- signature/policy_config_test.go | 81 +++++++++++++++++++++++--- signature/policy_eval_signedby.go | 21 +++++-- signature/policy_eval_signedby_test.go | 30 ++++++++-- signature/policy_types.go | 8 ++- 7 files changed, 157 insertions(+), 32 deletions(-) diff --git a/docs/containers-policy.json.5.md b/docs/containers-policy.json.5.md index 703bb8396..8611bd714 100644 --- a/docs/containers-policy.json.5.md +++ b/docs/containers-policy.json.5.md @@ -156,13 +156,14 @@ This requirement requires an image to be signed using “simple signing” with "type": "signedBy", "keyType": "GPGKeys", /* The only currently supported value */ "keyPath": "/path/to/local/keyring/file", + "keyPaths": ["/path/to/local/keyring/file1","/path/to/local/keyring/file2"…], "keyData": "base64-encoded-keyring-data", "signedIdentity": identity_requirement } ``` -Exactly one of `keyPath` and `keyData` must be present, containing a GPG keyring of one or more public keys. Only signatures made by these keys are accepted. +Exactly one of `keyPath`, `keyPaths` and `keyData` must be present, containing a GPG keyring of one or more public keys. Only signatures made by these keys are accepted. The `signedIdentity` field, a JSON object, specifies what image identity the signature claims about the image. One of the following alternatives are supported: diff --git a/signature/fixtures/policy.json b/signature/fixtures/policy.json index 8d488feb0..22d68d8d1 100644 --- a/signature/fixtures/policy.json +++ b/signature/fixtures/policy.json @@ -75,6 +75,13 @@ } } ], + "registry.redhat.io/beta": [ + { + "type": "signedBy", + "keyType": "GPGKeys", + "keyPaths": ["/keys/RH-production-signing-key-gpg-keyring", "/keys/RH-beta-signing-key-gpg-keyring"] + } + ], "private-mirror:5000/vendor-mirror": [ { "type": "signedBy", diff --git a/signature/policy_config.go b/signature/policy_config.go index 9c36dca98..8c6f603cb 100644 --- a/signature/policy_config.go +++ b/signature/policy_config.go @@ -316,7 +316,7 @@ func (pr *prReject) UnmarshalJSON(data []byte) error { } // newPRSignedBy returns a new prSignedBy if parameters are valid. -func newPRSignedBy(keyType sbKeyType, keyPath string, keyData []byte, signedIdentity PolicyReferenceMatch) (*prSignedBy, error) { +func newPRSignedBy(keyType sbKeyType, keyPath string, keyPaths []string, keyData []byte, signedIdentity PolicyReferenceMatch) (*prSignedBy, error) { if !keyType.IsValid() { return nil, InvalidPolicyFormatError(fmt.Sprintf("invalid keyType \"%s\"", keyType)) } @@ -324,11 +324,14 @@ func newPRSignedBy(keyType sbKeyType, keyPath string, keyData []byte, signedIden if keyPath != "" { keySources++ } + if keyPaths != nil { + keySources++ + } if keyData != nil { keySources++ } if keySources != 1 { - return nil, InvalidPolicyFormatError("exactly one of keyPath and keyData must be specified") + return nil, InvalidPolicyFormatError("exactly one of keyPath, keyPaths and keyData must be specified") } if signedIdentity == nil { return nil, InvalidPolicyFormatError("signedIdentity not specified") @@ -337,6 +340,7 @@ func newPRSignedBy(keyType sbKeyType, keyPath string, keyData []byte, signedIden prCommon: prCommon{Type: prTypeSignedBy}, KeyType: keyType, KeyPath: keyPath, + KeyPaths: keyPaths, KeyData: keyData, SignedIdentity: signedIdentity, }, nil @@ -344,7 +348,7 @@ func newPRSignedBy(keyType sbKeyType, keyPath string, keyData []byte, signedIden // newPRSignedByKeyPath is NewPRSignedByKeyPath, except it returns the private type. func newPRSignedByKeyPath(keyType sbKeyType, keyPath string, signedIdentity PolicyReferenceMatch) (*prSignedBy, error) { - return newPRSignedBy(keyType, keyPath, nil, signedIdentity) + return newPRSignedBy(keyType, keyPath, nil, nil, signedIdentity) } // NewPRSignedByKeyPath returns a new "signedBy" PolicyRequirement using a KeyPath @@ -352,9 +356,19 @@ func NewPRSignedByKeyPath(keyType sbKeyType, keyPath string, signedIdentity Poli return newPRSignedByKeyPath(keyType, keyPath, signedIdentity) } +// newPRSignedByKeyPaths is NewPRSignedByKeyPaths, except it returns the private type. +func newPRSignedByKeyPaths(keyType sbKeyType, keyPaths []string, signedIdentity PolicyReferenceMatch) (*prSignedBy, error) { + return newPRSignedBy(keyType, "", keyPaths, nil, signedIdentity) +} + +// NewPRSignedByKeyPaths returns a new "signedBy" PolicyRequirement using KeyPaths +func NewPRSignedByKeyPaths(keyType sbKeyType, keyPaths []string, signedIdentity PolicyReferenceMatch) (PolicyRequirement, error) { + return newPRSignedByKeyPaths(keyType, keyPaths, signedIdentity) +} + // newPRSignedByKeyData is NewPRSignedByKeyData, except it returns the private type. func newPRSignedByKeyData(keyType sbKeyType, keyData []byte, signedIdentity PolicyReferenceMatch) (*prSignedBy, error) { - return newPRSignedBy(keyType, "", keyData, signedIdentity) + return newPRSignedBy(keyType, "", nil, keyData, signedIdentity) } // NewPRSignedByKeyData returns a new "signedBy" PolicyRequirement using a KeyData @@ -369,7 +383,7 @@ var _ json.Unmarshaler = (*prSignedBy)(nil) func (pr *prSignedBy) UnmarshalJSON(data []byte) error { *pr = prSignedBy{} var tmp prSignedBy - var gotKeyPath, gotKeyData = false, false + var gotKeyPath, gotKeyPaths, gotKeyData = false, false, false var signedIdentity json.RawMessage if err := internal.ParanoidUnmarshalJSONObject(data, func(key string) interface{} { switch key { @@ -380,6 +394,9 @@ func (pr *prSignedBy) UnmarshalJSON(data []byte) error { case "keyPath": gotKeyPath = true return &tmp.KeyPath + case "keyPaths": + gotKeyPaths = true + return &tmp.KeyPaths case "keyData": gotKeyData = true return &tmp.KeyData @@ -408,14 +425,16 @@ func (pr *prSignedBy) UnmarshalJSON(data []byte) error { var res *prSignedBy var err error switch { - case gotKeyPath && !gotKeyData: + case gotKeyPath && !gotKeyPaths && !gotKeyData: res, err = newPRSignedByKeyPath(tmp.KeyType, tmp.KeyPath, tmp.SignedIdentity) - case !gotKeyPath && gotKeyData: + case !gotKeyPath && gotKeyPaths && !gotKeyData: + res, err = newPRSignedByKeyPaths(tmp.KeyType, tmp.KeyPaths, tmp.SignedIdentity) + case !gotKeyPath && !gotKeyPaths && gotKeyData: res, err = newPRSignedByKeyData(tmp.KeyType, tmp.KeyData, tmp.SignedIdentity) - case !gotKeyPath && !gotKeyData: - return InvalidPolicyFormatError("At least one of keyPath and keyData must be specified") + case !gotKeyPath && !gotKeyPaths && !gotKeyData: + return InvalidPolicyFormatError("Exactly one of keyPath, keyPaths and keyData must be specified, none of them present") default: - return fmt.Errorf("Exactly one of keyPath and keyData must be specified, more than one present") + return fmt.Errorf("Exactly one of keyPath, keyPaths and keyData must be specified, more than one present") } if err != nil { return err diff --git a/signature/policy_config_test.go b/signature/policy_config_test.go index 24c8f20cd..a8e9a76a1 100644 --- a/signature/policy_config_test.go +++ b/signature/policy_config_test.go @@ -71,6 +71,11 @@ var policyFixtureContents = &Policy{ "/keys/RH-key-signing-key-gpg-keyring", NewPRMMatchRepoDigestOrExact()), }, + "registry.redhat.io/beta": { + xNewPRSignedByKeyPaths(SBKeyTypeGPGKeys, + []string{"/keys/RH-production-signing-key-gpg-keyring", "/keys/RH-beta-signing-key-gpg-keyring"}, + newPRMMatchRepoDigestOrExact()), + }, "private-mirror:5000/vendor-mirror": { xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "/keys/vendor-gpg-keyring", @@ -379,6 +384,15 @@ func xNewPRSignedByKeyPath(keyType sbKeyType, keyPath string, signedIdentity Pol return pr } +// xNewPRSignedByKeyPaths is like NewPRSignedByKeyPaths, except it must not fail. +func xNewPRSignedByKeyPaths(keyType sbKeyType, keyPaths []string, signedIdentity PolicyReferenceMatch) PolicyRequirement { + pr, err := NewPRSignedByKeyPaths(keyType, keyPaths, signedIdentity) + if err != nil { + panic("xNewPRSignedByKeyPaths failed") + } + return pr +} + // xNewPRSignedByKeyData is like NewPRSignedByKeyData, except it must not fail. func xNewPRSignedByKeyData(keyType sbKeyType, keyData []byte, signedIdentity PolicyReferenceMatch) PolicyRequirement { pr, err := NewPRSignedByKeyData(keyType, keyData, signedIdentity) @@ -708,41 +722,62 @@ func TestPRRejectUnmarshalJSON(t *testing.T) { func TestNewPRSignedBy(t *testing.T) { const testPath = "/foo/bar" + testPaths := []string{"/path/1", "/path/2"} testData := []byte("abc") testIdentity := NewPRMMatchRepoDigestOrExact() // Success - pr, err := newPRSignedBy(SBKeyTypeGPGKeys, testPath, nil, testIdentity) + pr, err := newPRSignedBy(SBKeyTypeGPGKeys, testPath, nil, nil, testIdentity) require.NoError(t, err) assert.Equal(t, &prSignedBy{ prCommon: prCommon{prTypeSignedBy}, KeyType: SBKeyTypeGPGKeys, KeyPath: testPath, + KeyPaths: nil, + KeyData: nil, + SignedIdentity: testIdentity, + }, pr) + pr, err = newPRSignedBy(SBKeyTypeGPGKeys, "", testPaths, nil, testIdentity) + require.NoError(t, err) + assert.Equal(t, &prSignedBy{ + prCommon: prCommon{prTypeSignedBy}, + KeyType: SBKeyTypeGPGKeys, + KeyPath: "", + KeyPaths: testPaths, KeyData: nil, SignedIdentity: testIdentity, }, pr) - pr, err = newPRSignedBy(SBKeyTypeGPGKeys, "", testData, testIdentity) + pr, err = newPRSignedBy(SBKeyTypeGPGKeys, "", nil, testData, testIdentity) require.NoError(t, err) assert.Equal(t, &prSignedBy{ prCommon: prCommon{prTypeSignedBy}, KeyType: SBKeyTypeGPGKeys, KeyPath: "", + KeyPaths: nil, KeyData: testData, SignedIdentity: testIdentity, }, pr) // Invalid keyType - _, err = newPRSignedBy(sbKeyType(""), testPath, nil, testIdentity) + _, err = newPRSignedBy(sbKeyType(""), testPath, nil, nil, testIdentity) assert.Error(t, err) - _, err = newPRSignedBy(sbKeyType("this is invalid"), testPath, nil, testIdentity) + _, err = newPRSignedBy(sbKeyType("this is invalid"), testPath, nil, nil, testIdentity) assert.Error(t, err) - // Both keyPath and keyData specified - _, err = newPRSignedBy(SBKeyTypeGPGKeys, testPath, testData, testIdentity) + // Invalid keyPath/keyPaths/keyData combinations + _, err = newPRSignedBy(SBKeyTypeGPGKeys, testPath, testPaths, testData, testIdentity) + assert.Error(t, err) + _, err = newPRSignedBy(SBKeyTypeGPGKeys, testPath, testPaths, nil, testIdentity) + assert.Error(t, err) + _, err = newPRSignedBy(SBKeyTypeGPGKeys, testPath, nil, testData, testIdentity) + assert.Error(t, err) + _, err = newPRSignedBy(SBKeyTypeGPGKeys, "", testPaths, testData, testIdentity) + assert.Error(t, err) + _, err = newPRSignedBy(SBKeyTypeGPGKeys, "", nil, nil, testIdentity) assert.Error(t, err) // Invalid signedIdentity - _, err = newPRSignedBy(SBKeyTypeGPGKeys, testPath, nil, nil) + _, err = newPRSignedBy(SBKeyTypeGPGKeys, testPath, nil, nil, nil) assert.Error(t, err) } @@ -756,6 +791,16 @@ func TestNewPRSignedByKeyPath(t *testing.T) { // Failure cases tested in TestNewPRSignedBy. } +func TestNewPRSignedByKeyPaths(t *testing.T) { + testPaths := []string{"/path/1", "/path/2"} + _pr, err := NewPRSignedByKeyPaths(SBKeyTypeGPGKeys, testPaths, NewPRMMatchRepoDigestOrExact()) + require.NoError(t, err) + pr, ok := _pr.(*prSignedBy) + require.True(t, ok) + assert.Equal(t, testPaths, pr.KeyPaths) + // Failure cases tested in TestNewPRSignedBy. +} + func TestNewPRSignedByKeyData(t *testing.T) { testData := []byte("abc") _pr, err := NewPRSignedByKeyData(SBKeyTypeGPGKeys, testData, NewPRMMatchRepoDigestOrExact()) @@ -799,12 +844,19 @@ func TestPRSignedByUnmarshalJSON(t *testing.T) { func(v mSI) { delete(v, "keyType") }, // Invalid "keyType" field func(v mSI) { v["keyType"] = "this is invalid" }, - // Both "keyPath" and "keyData" is missing + // All three of "keyPath", "keyPaths" and "keyData" are missing func(v mSI) { delete(v, "keyData") }, - // Both "keyPath" and "keyData" is present + // All three of "keyPath", "keyPaths" and "keyData" are present + func(v mSI) { v["keyPath"] = "/foo/bar"; v["keyPaths"] = []string{"/1", "/2"} }, + // Two of "keyPath", "keyPaths" and "keyData" are present + func(v mSI) { v["keyPath"] = "/foo/bar"; v["keyPaths"] = []string{"/1", "/2"}; delete(v, "keyData") }, func(v mSI) { v["keyPath"] = "/foo/bar" }, + func(v mSI) { v["keyPaths"] = []string{"/1", "/2"} }, // Invalid "keyPath" field func(v mSI) { delete(v, "keyData"); v["keyPath"] = 1 }, + // Invalid "keyPaths" field + func(v mSI) { delete(v, "keyData"); v["keyPaths"] = 1 }, + func(v mSI) { delete(v, "keyData"); v["keyPaths"] = []int{1} }, // Invalid "keyData" field func(v mSI) { v["keyData"] = 1 }, func(v mSI) { v["keyData"] = "this is invalid base64" }, @@ -827,6 +879,17 @@ func TestPRSignedByUnmarshalJSON(t *testing.T) { }, duplicateFields: []string{"type", "keyType", "keyPath", "signedIdentity"}, }.run(t) + // Test the keyPaths-specific aspects + policyJSONUmarshallerTests{ + newDest: func() json.Unmarshaler { return &prSignedBy{} }, + newValidObject: func() (interface{}, error) { + return NewPRSignedByKeyPaths(SBKeyTypeGPGKeys, []string{"/1", "/2"}, NewPRMMatchRepoDigestOrExact()) + }, + otherJSONParser: func(validJSON []byte) (interface{}, error) { + return newPolicyRequirementFromJSON(validJSON) + }, + duplicateFields: []string{"type", "keyType", "keyPaths", "signedIdentity"}, + }.run(t) var pr prSignedBy diff --git a/signature/policy_eval_signedby.go b/signature/policy_eval_signedby.go index 38ca2750b..ef98b8b83 100644 --- a/signature/policy_eval_signedby.go +++ b/signature/policy_eval_signedby.go @@ -26,7 +26,7 @@ func (pr *prSignedBy) isSignatureAuthorAccepted(ctx context.Context, image priva } // FIXME: move this to per-context initialization - var data []byte + var data [][]byte keySources := 0 if pr.KeyPath != "" { keySources++ @@ -34,18 +34,29 @@ func (pr *prSignedBy) isSignatureAuthorAccepted(ctx context.Context, image priva if err != nil { return sarRejected, nil, err } - data = d + data = [][]byte{d} + } + if pr.KeyPaths != nil { + keySources++ + data = [][]byte{} + for _, path := range pr.KeyPaths { + d, err := os.ReadFile(path) + if err != nil { + return sarRejected, nil, err + } + data = append(data, d) + } } if pr.KeyData != nil { keySources++ - data = pr.KeyData + data = [][]byte{pr.KeyData} } if keySources != 1 { - return sarRejected, nil, errors.New(`Internal inconsistency: not exactly one of "keyPath" and "keyData" specified`) + return sarRejected, nil, errors.New(`Internal inconsistency: not exactly one of "keyPath", "keyPaths" and "keyData" specified`) } // FIXME: move this to per-context initialization - mech, trustedIdentities, err := NewEphemeralGPGSigningMechanism(data) + mech, trustedIdentities, err := newEphemeralGPGSigningMechanism(data) if err != nil { return sarRejected, nil, err } diff --git a/signature/policy_eval_signedby_test.go b/signature/policy_eval_signedby_test.go index c12f47505..0e9bb83f7 100644 --- a/signature/policy_eval_signedby_test.go +++ b/signature/policy_eval_signedby_test.go @@ -58,11 +58,18 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) { keyData, err := os.ReadFile("fixtures/public-key.gpg") require.NoError(t, err) - // Successful validation, with KeyData and KeyPath + // Successful validation, with KeyPath, KeyPaths and KeyData. for _, fn := range []func() (PolicyRequirement, error){ func() (PolicyRequirement, error) { return NewPRSignedByKeyPath(ktGPG, "fixtures/public-key.gpg", prm) }, + // Test the files in both orders, to make sure the correct public keys accepted in either position. + func() (PolicyRequirement, error) { + return NewPRSignedByKeyPaths(ktGPG, []string{"fixtures/public-key-1.gpg", "fixtures/public-key-1.gpg"}, prm) + }, + func() (PolicyRequirement, error) { + return NewPRSignedByKeyPaths(ktGPG, []string{"fixtures/public-key-2.gpg", "fixtures/public-key-1.gpg"}, prm) + }, func() (PolicyRequirement, error) { return NewPRSignedByKeyData(ktGPG, keyData, prm) }, @@ -95,17 +102,32 @@ func TestPRSignedByIsSignatureAuthorAccepted(t *testing.T) { // Invalid KeyPath/KeyPaths/KeyData combinations. for _, fn := range []func() (PolicyRequirement, error){ - // Both KeyPath and KeyData set. Do not use NewPRSignedBy*, because it would reject this. + // Two or more of KeyPath, KeyPaths and KeyData set. Do not use NewPRSignedBy*, because it would reject this. + func() (PolicyRequirement, error) { + return &prSignedBy{KeyType: ktGPG, KeyPath: "fixtures/public-key.gpg", KeyPaths: []string{"fixtures/public-key-1.gpg", "fixtures/public-key-2.gpg"}, KeyData: keyData, SignedIdentity: prm}, nil + }, func() (PolicyRequirement, error) { - return &prSignedBy{KeyType: ktGPG, KeyPath: "fixtures/public-key.gpg", KeyData: []byte("abc"), SignedIdentity: prm}, nil + return &prSignedBy{KeyType: ktGPG, KeyPath: "fixtures/public-key.gpg", KeyPaths: []string{"fixtures/public-key-1.gpg", "fixtures/public-key-2.gpg"}, SignedIdentity: prm}, nil }, - // Neither KeyPath nor KeyData set. Do not use NewPRSignedBy*, because it would reject this. + func() (PolicyRequirement, error) { + return &prSignedBy{KeyType: ktGPG, KeyPath: "fixtures/public-key.gpg", KeyData: keyData, SignedIdentity: prm}, nil + }, + func() (PolicyRequirement, error) { + return &prSignedBy{KeyType: ktGPG, KeyPaths: []string{"fixtures/public-key-1.gpg", "fixtures/public-key-2.gpg"}, KeyData: keyData, SignedIdentity: prm}, nil + }, + // None of KeyPath, KeyPaths and KeyData set. Do not use NewPRSignedBy*, because it would reject this. func() (PolicyRequirement, error) { return &prSignedBy{KeyType: ktGPG, SignedIdentity: prm}, nil }, func() (PolicyRequirement, error) { // Invalid KeyPath return NewPRSignedByKeyPath(ktGPG, "/this/does/not/exist", prm) }, + func() (PolicyRequirement, error) { // Invalid KeyPaths + return NewPRSignedByKeyPaths(ktGPG, []string{"/this/does/not/exist"}, prm) + }, + func() (PolicyRequirement, error) { // One of the KeyPaths is invalid + return NewPRSignedByKeyPaths(ktGPG, []string{"fixtures/public-key.gpg", "/this/does/not/exist"}, prm) + }, } { pr, err := fn() require.NoError(t, err) diff --git a/signature/policy_types.go b/signature/policy_types.go index 68dcd9892..9e837452a 100644 --- a/signature/policy_types.go +++ b/signature/policy_types.go @@ -67,14 +67,16 @@ type prReject struct { type prSignedBy struct { prCommon - // KeyType specifies what kind of key reference KeyPath/KeyData is. + // KeyType specifies what kind of key reference KeyPath/KeyPaths/KeyData is. // Acceptable values are “GPGKeys” | “signedByGPGKeys” “X.509Certificates” | “signedByX.509CAs” // FIXME: eventually also support GPGTOFU, X.509TOFU, with KeyPath only KeyType sbKeyType `json:"keyType"` - // KeyPath is a pathname to a local file containing the trusted key(s). Exactly one of KeyPath and KeyData must be specified. + // KeyPath is a pathname to a local file containing the trusted key(s). Exactly one of KeyPath, KeyPaths and KeyData must be specified. KeyPath string `json:"keyPath,omitempty"` - // KeyData contains the trusted key(s), base64-encoded. Exactly one of KeyPath and KeyData must be specified. + // KeyPaths if a set of pathnames to local files containing the trusted key(s). Exactly one of KeyPath, KeyPaths and KeyData must be specified. + KeyPaths []string `json:"keyPaths,omitempty"` + // KeyData contains the trusted key(s), base64-encoded. Exactly one of KeyPath, KeyPaths and KeyData must be specified. KeyData []byte `json:"keyData,omitempty"` // SignedIdentity specifies what image identity the signature must be claiming about the image.