Attestations + policy in cip. #1772

vaikas · 2022-04-18T20:30:21Z

Summary

Start of adding support for parity with verify-attestations in cosigned webhook
Add support for cue in validating a CIP after signatures / attestations have been verified
Fix a bug where CTLog was not plumbed through for custom verification
Add 'name' field to authorities (gets defaulted to authority-) for making it easier to write CIP level policies

Missing:

E2E test for doing cue validation with CIP policy. I can't figure this out, but @hectorj2f has for example rego in chore: add rego function to consume modules and evaluate them #1787 that we could start testing with.
We should add parallel validation of Policies and then within policies parallel authorities so everything doesn't have to happen sequentially.

I'd like to get this in and start experimenting so we can get real policies implemented and get
experience on what works.

Starts putting scaffolding in place for this:
#1769

Ticket Link

Fixes

Release Note

 * Start of adding support for parity with verify-attestations in cosigned webhook
 * Add support for cue in validating a CIP _after_ signatures / attestations have been verified
 * Fix a bug where CTLog was not plumbed through for custom verification
 * Add 'name' field to authorities (gets defaulted to authority-<index>) for making it easier to write CIP level policies

codecov-commenter · 2022-04-18T22:07:08Z

Codecov Report

Merging #1772 (1541a0c) into main (8368bad) will increase coverage by 0.53%.
The diff coverage is 56.21%.

@@            Coverage Diff             @@
##             main    #1772      +/-   ##
==========================================
+ Coverage   31.45%   31.98%   +0.53%     
==========================================
  Files         144      145       +1     
  Lines        8896     9161     +265     
==========================================
+ Hits         2798     2930     +132     
- Misses       5761     5888     +127     
- Partials      337      343       +6

Impacted Files	Coverage Δ
...apis/cosigned/v1alpha1/clusterimagepolicy_types.go	`0.00% <0.00%> (ø)`
...kg/apis/cosigned/v1alpha1/zz_generated.deepcopy.go	`0.00% <0.00%> (ø)`
pkg/policy/eval.go	`51.85% <51.85%> (ø)`
pkg/cosign/kubernetes/webhook/validation.go	`74.00% <56.00%> (-6.77%)`	⬇️
pkg/cosign/kubernetes/webhook/validator.go	`74.44% <63.18%> (-8.13%)`	⬇️
pkg/apis/config/image_policies.go	`69.38% <100.00%> (ø)`
...s/cosigned/v1alpha1/clusterimagepolicy_defaults.go	`100.00% <100.00%> (+100.00%)`	⬆️
...cosigned/v1alpha1/clusterimagepolicy_validation.go	`94.11% <100.00%> (+1.55%)`	⬆️

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 8368bad...1541a0c. Read the comment docs.

vaikas · 2022-04-20T15:12:21Z

test/testdata/cosigned/e2e/cip-requires-two-signatures-and-two-attestations.yaml

-        keylessAttestationsErr: "error"
-        keylessAttestationsErr: "Did not get both keyless attestations"
+        keylessAttestationsErr: 1
+        keylessAttestationsErrMsg: "Did not get both keyless attestations"


Haha, this was my super hack to raise an error because I don't understand enough of cue yet :) So, we only execute this if if there's a failure and because there's a mismatch the policy will fail. So, all of these 'branches' really should be:
if len(...) < expected {
raiseEvaluationError
}

But, I don't know how to do the raiseEvaluationError, so I just added a statement that will result in an error.

mattmoor

Thanks for all the tests and detailed comments 🙏

mattmoor · 2022-04-22T22:06:22Z

config/300-clusterimagepolicy.yaml

@@ -99,6 +129,9 @@ spec:
                                  type: string
                          url:
                            type: string
+                      name:
+                        description: Name is the name for this authority. Used by the CIP Policy validator to be able to reference matching signature or attestation verifications. If not specified, the name will be attestation-<index in array>


attestation-%d? Why not authority-%d?

I failed to update comment, I originally had it as attestation, and realized it was not good so I changed it to authority-%d, but forgot to update the comment.

mattmoor · 2022-04-22T22:07:49Z

pkg/apis/cosigned/v1alpha1/clusterimagepolicy_defaults.go

+func (spec *ClusterImagePolicySpec) SetDefaults(ctx context.Context) {
+	for i, authority := range spec.Authorities {
+		if authority.Name == "" {
+			spec.Authorities[i].Name = fmt.Sprintf("authority-%d", i)


Same question... attestation seems weird.

lol, I originally had it as attestation, thought it seemed wierd, so changed it to authority-%d like here, but it's correct here? Or how do you mean? Above, yes I had neglected to update the comment, but the code is right here, no?

mattmoor · 2022-04-22T22:11:12Z

pkg/apis/cosigned/v1alpha1/clusterimagepolicy_validation.go

+	if a.PredicateType == "" {
+		errs = errs.Also(apis.ErrMissingField("predicateType"))
+	} else if a.PredicateType != "custom" && a.PredicateType != "slsaprovenance" && a.PredicateType != "spdx" && a.PredicateType != "link" && a.PredicateType != "vuln" {


seems like a good use of switch?

Doesn't cosign attest support taking a full predicate type URL as well? Should we also allow full-blown URLs?

I'd rather not pollute this with the switch only because I'd rather use the TODO (refactor where the options come from and use that), also attest may take the uri but verify-attest does not. It looks for the short name in the map and pulls the URL from it. Again, I think that should be done as part of a follow on PR to refactor that code a bit to pull the map out of the cli/options so it can be reused here.
https://github.com/sigstore/cosign/blob/main/pkg/policy/attestation.go#L43

pkg/policy/eval_test.go

pkg/cosign/kubernetes/webhook/validator.go

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

…policy. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Start adding UT for ValidatePolicy. Getting ready for rebase. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Signed-off-by: hectorj2f <hectorf@vmware.com>

Add start of UT for ValidatePolicy. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

CIP tests, will follow up with those. Add validation for attestations in CIPs Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

from the remoteopts instead consistently. Address PR feedback. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

hectorj2f · 2022-04-23T07:48:09Z

test/testdata/cosigned/e2e/cip-requires-two-signatures-and-two-attestations.yaml

+      url: http://rekor.rekor-system.svc
+  policy:
+    type: cue
+    data: |


I can get this working in a separate PR, this PR one is big enough for now.

hectorj2f · 2022-04-23T07:50:11Z

pkg/policy/eval.go

+	logging.FromContext(ctx).Infof("Evaluating attestation: %s", string(attestation))
+	cueCtx := cuecontext.New()
+	v := cueCtx.CompileString(evaluator)
+	return cuejson.Validate(attestation, v)


As discussed, we have to use a different set of functions here, as cuejson.Validate expects the CUE to be self-contained and applied as a schema to the JSON.

We can improve this function in a different PR.

hectorj2f

LGTM from now, let's iterate over it.

* Modify types, introduce name defaults, codegen. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Start refactoring and adding verify-attestation pieces. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Sort of works e2e. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * E2E tests. Plumb rekor through for key-ful signing too. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * bad rebase. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * more bad rebases. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * regen keys. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * at the right place. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * forgot one rekor-url. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Lint. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * remove these manually. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Cleanups, do not unnecessarily eval empty json bytes for attestation policy. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Add name to attestations for easier referencing from cip policy. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Do not return empty PolicyResult when errors. Start adding UT for ValidatePolicy. Getting ready for rebase. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * fix: cue policy missing double-quotes Signed-off-by: hectorj2f <hectorf@vmware.com> * Add start of unit tests for policy validation stuff. Add start of UT for ValidatePolicy. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Refactor policy eval code. Remove the attestation CIP from normal CIP tests, will follow up with those. Add validation for attestations in CIPs Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * loving yaml, thanks validation! Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Starting to break apart the attestation tests. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * checkpoint Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Remove unused keychain from the Validate* calls and use one from the remoteopts instead consistently. Address PR feedback. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * lint. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> Co-authored-by: hectorj2f <hectorf@vmware.com>

vaikas force-pushed the attestations-in-cip branch 2 times, most recently from e0e7287 to e6f3573 Compare April 18, 2022 21:52

vaikas force-pushed the attestations-in-cip branch from a959510 to 3290b2e Compare April 18, 2022 23:29

vaikas changed the title ~~[WIP] Attestations + policy in cip.~~ Attestations + policy in cip. Apr 19, 2022

vaikas changed the title ~~Attestations + policy in cip.~~ [WIP] Attestations + policy in cip. Apr 19, 2022

vaikas commented Apr 20, 2022

View reviewed changes

hectorj2f force-pushed the attestations-in-cip branch from 2f9cf6f to 8610c0d Compare April 20, 2022 15:27

vaikas force-pushed the attestations-in-cip branch 2 times, most recently from 12cd3d1 to 654f9fe Compare April 21, 2022 22:17

hectorj2f mentioned this pull request Apr 22, 2022

chore: add rego function to consume modules and evaluate them #1787

Merged

vaikas force-pushed the attestations-in-cip branch from 22086bb to ac9c492 Compare April 22, 2022 22:06

mattmoor reviewed Apr 22, 2022

View reviewed changes

vaikas and others added 17 commits April 22, 2022 16:42

Modify types, introduce name defaults, codegen.

e942146

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Start refactoring and adding verify-attestation pieces.

b85eeb9

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Sort of works e2e.

4ec9e8d

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

E2E tests. Plumb rekor through for key-ful signing too.

4e4fdb8

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

bad rebase.

af699bf

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

more bad rebases.

8157fca

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

regen keys.

4160ffb

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

at the right place.

7233326

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

forgot one rekor-url.

f1a0426

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Lint.

b2776f9

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

remove these manually.

1438463

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Cleanups, do not unnecessarily eval empty json bytes for attestation …

a17bcfa

…policy. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Add name to attestations for easier referencing from cip policy.

3718151

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Do not return empty PolicyResult when errors.

ddeb422

Start adding UT for ValidatePolicy. Getting ready for rebase. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

fix: cue policy missing double-quotes

9e5688f

Signed-off-by: hectorj2f <hectorf@vmware.com>

Add start of unit tests for policy validation stuff.

d6e0a42

Add start of UT for ValidatePolicy. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Refactor policy eval code. Remove the attestation CIP from normal

2a60a43

CIP tests, will follow up with those. Add validation for attestations in CIPs Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

vaikas added 4 commits April 22, 2022 16:42

loving yaml, thanks validation!

f7efb9d

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Starting to break apart the attestation tests.

f6fcb5c

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

checkpoint

588e023

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Remove unused keychain from the Validate* calls and use one

d402a02

from the remoteopts instead consistently. Address PR feedback. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

vaikas force-pushed the attestations-in-cip branch from ac9c492 to d402a02 Compare April 23, 2022 02:00

lint.

1541a0c

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

vaikas changed the title ~~[WIP] Attestations + policy in cip.~~ Attestations + policy in cip. Apr 23, 2022

hectorj2f reviewed Apr 23, 2022

View reviewed changes

hectorj2f approved these changes Apr 23, 2022

View reviewed changes

dlorenc merged commit e1469ba into sigstore:main Apr 23, 2022

github-actions bot added this to the v1.8.0 milestone Apr 23, 2022

vaikas deleted the attestations-in-cip branch April 23, 2022 16:11

hectorj2f mentioned this pull request Apr 29, 2022

feat: add rego policy support #1817

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attestations + policy in cip. #1772

Attestations + policy in cip. #1772

vaikas commented Apr 18, 2022 •

edited

codecov-commenter commented Apr 18, 2022 •

edited

vaikas Apr 20, 2022

mattmoor left a comment

mattmoor Apr 22, 2022

vaikas Apr 23, 2022

mattmoor Apr 22, 2022

vaikas Apr 23, 2022

mattmoor Apr 22, 2022

mattmoor Apr 22, 2022

vaikas Apr 23, 2022

hectorj2f Apr 23, 2022 •

edited

hectorj2f Apr 23, 2022

hectorj2f Apr 23, 2022

hectorj2f left a comment

Attestations + policy in cip. #1772

Attestations + policy in cip. #1772

Conversation

vaikas commented Apr 18, 2022 • edited

Summary

Ticket Link

Release Note

codecov-commenter commented Apr 18, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

mattmoor 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

hectorj2f Apr 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorj2f left a comment

Choose a reason for hiding this comment

vaikas commented Apr 18, 2022 •

edited

codecov-commenter commented Apr 18, 2022 •

edited

hectorj2f Apr 23, 2022 •

edited