Add field KeyPaths to prSigstoreSigned #2456

dcermak · 2024-06-20T15:03:20Z

The new field KeyPaths is taken directly from /etc/containers/policy.json and allows users to provide multiple signature keys to be used to verify images. Only one of the keys has to verify, thereby this mechanism allows us to have support seamless key rotation on a registry.

This fixes #2319

mtrmac

Thanks! We definitely want this feature.

I realize this is a quite early version — so just some higher-level comments; I didn’t carefully read the code line by line.

I think it makes sense to add KeyDatas at the same time. (It is not necessary for GPG, because the data format natively supports multiple keys.)
We might also want the same multi-key feature for rekorPublicKey*. That does not need to happen in this PR, but it’s something to potentially keep in mind WRT sharing code (e.g. using loadBytesFromDataOrPath for both).
Ultimately, every line of code (and every user-visible feature) in signature should either be covered by unit tests, or have a comment documenting why the line is unreachable. It would probably make sense to delay that after the basic implementation is OK, but something to keep in mind when structuring it.

signature/policy_config_sigstore.go

signature/policy_eval_sigstore.go

mtrmac · 2024-07-18T17:40:54Z

Just a quick ACK — I definitely want this feature to happen short-term, and I appreciate your work on it. Right now I need to finish some other work (related to the zstd:chunked), so I’m afraid I’ll only be able to review this in detail in a week or two. I just wanted to be transparent about it, while reaffirming that this is important both to me personally and to my employer, and that the effort is not wasted.

dcermak · 2024-07-18T20:18:09Z

Miloslav Trmač ***@***.***> writes:

Just a quick ACK — I definitely want this feature to happen short-term, and I appreciate your work on it. Right now I need to finish some other work (related to the zstd:chunked), so I’m afraid I’ll only be able to review this in detail in a week or two. I just wanted to be transparent about it, while reaffirming that this _is_ important both to me personally and to my employer, and that the effort is not wasted.

No worries, myself and Danish haven't been that responsive either. We'll continue to work on improving the test coverage in the meantime.

mtrmac

Thanks!

I’m finally back to this project — this looks very good overall, most of the comments are stylistic nitpicks.

I’ll work tomorrow on making those adjustments.

signature/policy_types.go

signature/policy_config_sigstore.go

signature/policy_types.go

mtrmac · 2024-08-14T19:24:46Z

signature/policy_eval_sigstore_test.go

@@ -320,13 +348,18 @@ func TestPRrSigstoreSignedIsSignatureAccepted(t *testing.T) {

 	// Successful key+Rekor use
 	pr, err = newPRSigstoreSigned(


Also test: the second key, not the first one, matches the Rekor data.

mtrmac · 2024-08-14T19:28:49Z

signature/policy_eval_sigstore_test.go

 	assertAccepted(sar, err)
+	tr, err := pr.prepareTrustRoot()
+	assert.NoError(t, err)
+	assert.Equal(t, sar.pk, tr.publicKeys[1])

 	// key+Rekor, missing Rekor SET annotation
 	sar, err = pr.isSignatureAccepted(context.Background(), nil,


Also test: two keys + invalid Rekor SET

mtrmac · 2024-08-14T19:32:57Z

signature/internal/sigstore_payload.go

+	// at this point we must have failed to verify the signature with every key
+	// => there must be at least one error
+	if len(errs) == 0 {
+		return nil, nil, fmt.Errorf("Internal error: signature verification failed but no errors have been recorded")


By convention in c/image/signature, this should have a // Coverage: … comment justifying why it is not covered by tests, maybe instead of the comment above.

(Alternatively, we could drop the len(publicKeys) check at the start of the function and only determine that here. That’s not obviously better.)

mtrmac · 2024-08-14T19:46:54Z

signature/policy_config_sigstore_test.go

+			PRSigstoreSignedWithKeyDatas([][]byte{[]byte("foo")}),
+			PRSigstoreSignedWithKeyData([]byte("bar")),
+			PRSigstoreSignedWithSignedIdentity(testIdentity),
+		},


Also test: duplicate keyPaths, duplicate keyDatas

mtrmac · 2024-08-14T19:47:45Z

signature/policy_config_sigstore.go

+		PRSigstoreSignedWithSignedIdentity(signedIdentity),
+	)
+}
+
 // Compile-time check that prSigstoreSigned implements json.Unmarshaler.
 var _ json.Unmarshaler = (*prSigstoreSigned)(nil)


That implementation also needs to be extended with the new fields, otherwise using them in policy.json wouldn’t work.

The new fields `KeyPaths` and `KeyDatas` is taken directly from `/etc/containers/policy.json` and allows users to provide multiple signature keys to be used to verify images. Only one of the keys has to verify, thereby this mechanism allows us to have support seamless key rotation on a registry. This fixes containers#2319 Signed-off-by: Dan Čermák <dcermak@suse.com> Co-authored-by: Danish Prakash <danish.prakash@suse.com>

…ayload Signed-off-by: Dan Čermák <dcermak@suse.com> Co-authored-by: Danish Prakash <danish.prakash@suse.com>

It also contains the public key that verified the image signature and thereby allows us to check easier which key was picked in the presence of multiple keys Signed-off-by: Dan Čermák <dcermak@suse.com> Co-authored-by: Danish Prakash <danish.prakash@suse.com>

mtrmac

Thanks so much for a quick update!

As promised, I have continued updating some of the style + test nits. See #2524, perhaps best without the last commit.

It is very much a WIP, e.g. we probably don’t need all those individual commits. Still, I hope that is useful.

mtrmac · 2024-08-15T22:09:23Z

docs/containers-policy.json.5.md

@@ -316,11 +316,13 @@ used with `exactReference` or `exactRepository`.

 This requirement requires an image to be signed using a sigstore signature with an expected identity and key.

-```js
+```json


Using JSON here causes the identity_requirement value to be highlighted as an error.

mtrmac · 2024-08-15T22:11:15Z

docs/containers-policy.json.5.md


 If `keyPath` or `keyData` is present, it contains a sigstore public key.
 Only signatures made by this key are accepted.

+If `keyPaths` or `keyDatas` is present, it contains sigstore public keys that
+sign the images. Signatures from any key in the list is accepted.


“signatures … are accepted”.

I don’t feel too strongly about the “that sign the images” part, but it should probably consistent with the previous paragraph, so that users don’t try to look for a difference in meaning.

mtrmac · 2024-08-15T22:13:06Z

signature/policy_config_sigstore.go

 	if res.Fulcio != nil {
 		keySources++
 	}
+


(Drop this line as well.)

mtrmac · 2024-08-15T22:15:23Z

signature/internal/sigstore_payload.go

+		// unmarshal after the signature has been verified, this is more
+		// resilient against bugs in the JSON parser
+		// Only perform the unmarshal in the first loop iteration
+		if unmatchedPayload == nil {


Once we enter this area, we either succeed or fail; we never iterate the loop again.

So this condition is unnecessary…

… really I think this would be easier to follow moving the cryptographic verification loop into a separate function, and having VerifySigstorePayload be a straight line code, instead of the primary code path ending inside a loop. That’s non-blocking, but it would avoid these situations.

dcermak · 2024-08-16T14:39:22Z

@mtrmac Do you want to address all your comments in #2524 or shall we address them? Either is fine, but I'd like to avoid duplicate work.

mtrmac · 2024-08-17T01:52:06Z

@dcermak For the lengthy test methods, it’s easier for me add the missing cases immediately while reading them, so I have done the work now.

Shall we continue in #2524? Thanks so much for your work, it was absolutely essential.

mtrmac · 2024-08-20T15:06:04Z

#2524 was merged. Thanks again!

mtrmac · 2024-08-20T19:03:17Z

… and this is included in https://github.com/containers/image/releases/tag/v5.32.2 .

dcermak marked this pull request as draft June 20, 2024 15:03

mtrmac reviewed Jun 20, 2024

View reviewed changes

signature/policy_config_sigstore.go Outdated Show resolved Hide resolved

signature/policy_config_sigstore.go Outdated Show resolved Hide resolved

signature/policy_eval_sigstore.go Outdated Show resolved Hide resolved

signature/policy_eval_sigstore.go Show resolved Hide resolved

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Jun 20, 2024

dcermak force-pushed the main branch 2 times, most recently from 6b6274e to 3354891 Compare July 3, 2024 10:47

mtrmac reviewed Jul 3, 2024

View reviewed changes

signature/policy_eval_sigstore.go Show resolved Hide resolved

signature/policy_eval_sigstore.go Outdated Show resolved Hide resolved

signature/policy_eval_sigstore.go Outdated Show resolved Hide resolved

dcermak force-pushed the main branch 5 times, most recently from 6dde503 to 87a954e Compare July 17, 2024 10:27

mtrmac reviewed Aug 14, 2024

View reviewed changes

dcermak and others added 3 commits August 15, 2024 15:57

Return the publicKey that verified the signature from VerifySigstoreP…

c6e826d

…ayload Signed-off-by: Dan Čermák <dcermak@suse.com> Co-authored-by: Danish Prakash <danish.prakash@suse.com>

dcermak force-pushed the main branch from cc6fddf to 3d87168 Compare August 15, 2024 14:00

mtrmac mentioned this pull request Aug 16, 2024

Add keyPaths, keyDatas to prSigstoreSigned #2524

Merged

mtrmac reviewed Aug 16, 2024

View reviewed changes

mtrmac closed this Aug 20, 2024

mtrmac mentioned this pull request Aug 30, 2024

[release-5.24] Backport #2418 + #2453 #2548

Merged

		@@ -320,13 +348,18 @@ func TestPRrSigstoreSignedIsSignatureAccepted(t *testing.T) {

		// Successful key+Rekor use
		pr, err = newPRSigstoreSigned(

Add field KeyPaths to prSigstoreSigned #2456

Add field KeyPaths to prSigstoreSigned #2456

Uh oh!

Conversation

dcermak commented Jun 20, 2024

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtrmac commented Jul 18, 2024

Uh oh!

dcermak commented Jul 18, 2024 via email

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac Aug 14, 2024 • 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!

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!

dcermak commented Aug 16, 2024

Uh oh!

mtrmac commented Aug 17, 2024

Uh oh!

mtrmac commented Aug 20, 2024

Uh oh!

mtrmac commented Aug 20, 2024

Uh oh!

Uh oh!

mtrmac Aug 14, 2024 •

edited

Loading