Validate authority keys #1623

coyote240 · 2022-03-16T17:58:09Z

Resolves #1419

Summary

Begin integration of clusterimagepolicy into cosigned webhook.

DennyHoang · 2022-03-16T19:26:58Z

pkg/cosign/kubernetes/webhook/validator.go

+			tempKeys := keys
+			config := config.FromContext(ctx)
+			if config != nil {
+				authorities, err := config.ImagePolicyConfig.GetAuthorities(ref.Name())


Question: Wondering how I could structure a unit-test to mock out ctx such that config.FromContext(ctx) returns a config with mocked data for authorities[].key.data. Currently, I tried to use context.WithValue(ctx, config.CfgKey{}, <ImagePolicyConfig populated struct>) and this did not work (I made cfgKey{} public in store.go to temporarily try this).

@dlorenc @vaikas

@DennyHoang Did you try to create a fake ConfigMap with the desired configuration in the tests ?

Yup, pulling out the logic into a function and going to test that specifically. I will be passing in a mocked config now (and not consider mocking out context) so I can isolate the testing to the logic there.

As for testing the integration of that function, I'll look into that further afterwards.

I think using this should work for this:
https://github.com/sigstore/cosign/blob/main/pkg/apis/config/store.go#L53

Oh my, how did I not see that... Thanks, will give it a try!

@vaikas @hectorj2f I tried to implement a test with the config.ToContext
1aa88d1

Please lemme know what you think and if I should/need to replicate this test case to the other test cases i.e TestValidateCronJob, TestResolvePodSpec, etc. I feel like it may not be required as this one test has enough coverage, but wanted a more concrete opinion from others who are more knowledgeable about how this all works.

(In the meantime, just noticed your comments below about error handling and will take a look now)

It looks good to me.

codecov-commenter · 2022-03-16T19:41:08Z

Codecov Report

Merging #1623 (07e946d) into main (2fcf30e) will increase coverage by 0.30%.
The diff coverage is 73.77%.

@@            Coverage Diff             @@
##             main    #1623      +/-   ##
==========================================
+ Coverage   27.97%   28.28%   +0.30%     
==========================================
  Files         137      137              
  Lines        7820     7864      +44     
==========================================
+ Hits         2188     2224      +36     
- Misses       5403     5404       +1     
- Partials      229      236       +7

Impacted Files	Coverage Δ
pkg/cosign/kubernetes/webhook/validation.go	`78.57% <66.66%> (+4.54%)`	⬆️
pkg/cosign/kubernetes/webhook/validator.go	`81.71% <70.83%> (-1.84%)`	⬇️
pkg/apis/config/image_policies.go	`69.38% <75.00%> (-1.35%)`	⬇️
...cosigned/v1alpha1/clusterimagepolicy_validation.go	`88.99% <100.00%> (+0.64%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fcf30e...07e946d. Read the comment docs.

vaikas · 2022-03-18T11:38:50Z

Fantastic! Thanks for doing this. We also need to add validation here (since we didn't support it at the time, I made it disallowed).
https://github.com/sigstore/cosign/blob/main/pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go#L61

Then we should probably in the validation function see if the regex compiles, wdyt? I think this can then we caught earlier, during the creation of the resource.
https://github.com/sigstore/cosign/pull/1623/files#diff-5cd49ec5dc4f031cb339593e719593fd2d06d1ee635e62360629fd048a9f0610R104

If we use something like this during the validation?
https://cs.opensource.google/go/go/+/refs/tags/go1.18:src/regexp/regexp.go;l=134

And then maybe add a valid regex in here (or create a new file with just regex?):
https://github.com/sigstore/cosign/blob/main/test/testdata/cosigned/valid/valid-policy.yaml

And maybe add an invalid regex file here as well?
https://github.com/sigstore/cosign/tree/main/test/testdata/cosigned/invalid

vaikas

Thanks for doing this, and thanks for cleaning up the tests! I'm fine with this going on if you want to do the errors in the followup, but please create an issue to track that.

vaikas · 2022-03-21T16:29:51Z

pkg/cosign/kubernetes/webhook/validator.go

@@ -153,6 +161,29 @@ func (v *Validator) validatePodSpec(ctx context.Context, ps *corev1.PodSpec, opt
 	return errs
 }

+func getAuthorityKeys(ctx context.Context, ref name.Reference, config *config.Config) []*ecdsa.PublicKey {


I think this should not fail silently (L175). I'd first add err in the signature here. If we want to silently not fail things while we're working though, then where we call that, we can make that and create an issue to track that. But at least that way the flipping of the err/silent does not require changing the signature here.

But if you want to do this as a followup, I'm fine with this going in now (so it won't break anything), but we should create an issue to ensure we don't forget about it :)

+1 we should return an error here.

Updated to surface errs and it is currently logged as a warning.
Tracking issue to actually enforce the errors in:
#1642

vaikas · 2022-03-21T16:37:25Z

pkg/cosign/kubernetes/webhook/validator.go

+			containerKeys := keys
+			config := config.FromContext(ctx)
+			if config != nil {
+				containerKeys = append(containerKeys, getAuthorityKeys(ctx, ref, config)...)


So here, I think we should (if you change getAuthorityKeys to return a possible error), we could skip enforcing it now if we're worried about breaking things. That way when we flip things to fail on errors (which I think we should), the change is localiced to here only.

vaikas · 2022-03-21T16:37:26Z

pkg/cosign/kubernetes/webhook/validator.go

+			containerKeys := keys
+			config := config.FromContext(ctx)
+			if config != nil {
+				containerKeys = append(containerKeys, getAuthorityKeys(ctx, ref, config)...)


So here, I think we should (if you change getAuthorityKeys to return a possible error), we could skip enforcing it now if we're worried about breaking things. That way when we flip things to fail on errors (which I think we should), the change is localiced to here only.

hectorj2f · 2022-03-21T17:46:41Z

pkg/cosign/kubernetes/webhook/validation.go

+			keys = append(keys, key.(*ecdsa.PublicKey))
+		}
+	}
+	if keys == nil {


Suggestion: you may want to do len(keys) == 0 instead.

vaikas

Do you want to switch from Draft? I think it's in a shape where we can get it in and continue working on it, wdyt?

pkg/cosign/kubernetes/webhook/validator.go

vaikas · 2022-03-21T23:36:27Z

pkg/cosign/kubernetes/webhook/validator.go

@@ -153,6 +166,26 @@ func (v *Validator) validatePodSpec(ctx context.Context, ps *corev1.PodSpec, opt
 	return errs
 }

+func getAuthorityKeys(ctx context.Context, ref name.Reference, config *config.Config) (keys []*ecdsa.PublicKey, errs *apis.FieldError) {


I'm not sure we want to make these as apis.FieldError. We usually use those as validation time errors because they are user facing directly. I'd just been thinking that we'll use err for the return value, not apis.FieldError. But then as I started thinking about it some more, the are user facing at least in some cases because they will be returned to the user when they try to deploy something.

I also felt similar that it should have been errors initially. I followed using apis.ErrGeneric because getAuthorityKeys/parseAuthorityKeys followed the same logic from getKeys. getKeys returned errors using apis.ErrGeneric. I just tried to surface the lowest level error up and made it a common type. Should I change it such that getAuthorityKeys returns errors only and then have validatePodSpec determine how to interpret generic error such as into apis.FieldError?

Or is this something that can be changed later when we actually start erroring out more concretely? The change seems trivial enough either way you want it.

Question: why do we even need to parse the authority keys here ? The function call comes from validatePodSpec, i assume the parsing of the authorities has been done when creating the image policy config, rather when validating the pod spec.

When we get the ImagePolicyConfig, it contains Policies.[]ClusterImagePolicySpec.[]Authority. The returned Authority contains KeyRef.Data which is a string. parseAuthorityKeys takes that string and parses it into ecdsa.PublicKey. I do not believe we have a ecdsa.PublicKey representation of the key earlier.

Well, sorry I was thinking that we could be defining the signature as error, I think at least (too lazy to look up as I'm on the phone) we could still be returning apierrors as they are errors but not bleed that into the signature of the function.

vaikas · 2022-03-21T23:43:43Z

Also I've been working on adding the Keyless stuff into this, so if we get this merged, we can keep working on respective pieces :)

Signed-off-by: Denny Hoang <dhoang@vmware.com>

DennyHoang · 2022-03-22T00:14:22Z

@vaikas @hectorj2f I can't update the PR from Draft since "Only those with write access to this repository can mark a draft pull request as ready for review."

Could either of you two help me by updating it?

coyote240 · 2022-03-22T00:17:38Z

Set to "Ready for Review"

Signed-off-by: Denny Hoang <dhoang@vmware.com>

DennyHoang · 2022-03-22T00:50:20Z

@hectorj2f @vaikas
Can you please re-run the workflows? I fixed the lint issues and updated e2e test script to delete the applied CRD between runs as the second run of the kubectl create was erroring from the previous successful run.

dlorenc · 2022-03-22T00:52:31Z

Can you please re-run the workflows? I fixed the lint issues and updated e2e test script to delete the applied CRD between runs as the second run of the kubectl create was erroring from the previous successful run.

Rerunning!

dlorenc · 2022-03-22T02:38:40Z

reran it one more time, looked like a GCP flake.

hectorj2f · 2022-03-22T09:09:59Z

Yes, it was a GCP flake.

* Validate signature with authority keys Signed-off-by: Denny Hoang <dhoang@vmware.com> * Remove TODO log Signed-off-by: Denny Hoang <dhoang@vmware.com> * Add authority key info log & ko-apply to makefile Signed-off-by: Denny Hoang <dhoang@vmware.com> * Abstract and test getting authority keys Signed-off-by: Denny Hoang <dhoang@vmware.com> * Add authority regex matching for images Signed-off-by: Denny Hoang <dhoang@vmware.com> * Add more get authority key scenarios Signed-off-by: Denny Hoang <dhoang@vmware.com> * Rename and use variables Signed-off-by: Denny Hoang <dhoang@vmware.com> * Validate and compile regex Signed-off-by: Denny Hoang <dhoang@vmware.com> * Add regex cosigned testdata Signed-off-by: Denny Hoang <dhoang@vmware.com> * Fix lint error Signed-off-by: Denny Hoang <dhoang@vmware.com> * Test authority key integration Signed-off-by: Denny Hoang <dhoang@vmware.com> * Surface non-enforced errors Signed-off-by: Denny Hoang <dhoang@vmware.com> * Fix linting errors Signed-off-by: Denny Hoang <dhoang@vmware.com> * Fix e2e crd test to remove crd between tests Signed-off-by: Denny Hoang <dhoang@vmware.com> Co-authored-by: Denny Hoang <dhoang@vmware.com>

DennyHoang force-pushed the validate-authority-keys branch 3 times, most recently from cb824e3 to db8c7a7 Compare March 16, 2022 19:19

DennyHoang reviewed Mar 16, 2022

View reviewed changes

DennyHoang force-pushed the validate-authority-keys branch from 2073be5 to 12f0c6b Compare March 17, 2022 18:58

vaikas approved these changes Mar 21, 2022

View reviewed changes

hectorj2f reviewed Mar 21, 2022

View reviewed changes

DennyHoang mentioned this pull request Mar 21, 2022

[cosigned] Enforce authority key errors #1642

Closed

vaikas reviewed Mar 21, 2022

View reviewed changes

DennyHoang and others added 12 commits March 21, 2022 20:00

Validate signature with authority keys

c870cd8

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Remove TODO log

02911ec

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Add authority key info log & ko-apply to makefile

6cf2bcf

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Abstract and test getting authority keys

58587db

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Add authority regex matching for images

11e5b9a

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Add more get authority key scenarios

dd38cd5

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Rename and use variables

0fbfc50

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Validate and compile regex

4e05279

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Add regex cosigned testdata

6cf43be

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Fix lint error

0105abc

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Test authority key integration

76a2db8

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Surface non-enforced errors

f0f659e

Signed-off-by: Denny Hoang <dhoang@vmware.com>

DennyHoang force-pushed the validate-authority-keys branch from f149c6a to f0f659e Compare March 22, 2022 00:13

coyote240 marked this pull request as ready for review March 22, 2022 00:17

hectorj2f approved these changes Mar 22, 2022

View reviewed changes

Fix linting errors

2ccea62

Signed-off-by: Denny Hoang <dhoang@vmware.com>

Fix e2e crd test to remove crd between tests

07e946d

Signed-off-by: Denny Hoang <dhoang@vmware.com>

hectorj2f merged commit 45dbd39 into sigstore:main Mar 22, 2022

github-actions bot added this to the v1.7.0 milestone Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate authority keys #1623

Validate authority keys #1623

coyote240 commented Mar 16, 2022

DennyHoang Mar 16, 2022

hectorj2f Mar 17, 2022

DennyHoang Mar 17, 2022 •

edited

vaikas Mar 18, 2022

DennyHoang Mar 18, 2022

DennyHoang Mar 21, 2022

hectorj2f Mar 21, 2022

codecov-commenter commented Mar 16, 2022 •

edited

vaikas commented Mar 18, 2022

vaikas left a comment

vaikas Mar 21, 2022

vaikas Mar 21, 2022

hectorj2f Mar 21, 2022

DennyHoang Mar 21, 2022

vaikas Mar 21, 2022

vaikas Mar 21, 2022

hectorj2f Mar 21, 2022

vaikas left a comment

vaikas Mar 21, 2022

DennyHoang Mar 21, 2022

hectorj2f Mar 22, 2022

DennyHoang Mar 22, 2022 •

edited

vaikas Mar 22, 2022

vaikas commented Mar 21, 2022

DennyHoang commented Mar 22, 2022 •

edited

coyote240 commented Mar 22, 2022

DennyHoang commented Mar 22, 2022

dlorenc commented Mar 22, 2022

dlorenc commented Mar 22, 2022

hectorj2f commented Mar 22, 2022

Validate authority keys #1623

Validate authority keys #1623

Conversation

coyote240 commented Mar 16, 2022

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DennyHoang Mar 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 16, 2022 • edited

Codecov Report

vaikas commented Mar 18, 2022

vaikas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaikas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DennyHoang Mar 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaikas commented Mar 21, 2022

DennyHoang commented Mar 22, 2022 • edited

coyote240 commented Mar 22, 2022

DennyHoang commented Mar 22, 2022

dlorenc commented Mar 22, 2022

dlorenc commented Mar 22, 2022

hectorj2f commented Mar 22, 2022

DennyHoang Mar 17, 2022 •

edited

codecov-commenter commented Mar 16, 2022 •

edited

DennyHoang Mar 22, 2022 •

edited

DennyHoang commented Mar 22, 2022 •

edited