Plumb TrustRoot CRD through to CIP CRDs. Make TrustRoot available to webhook, clean up and refactor checkOpts logic. #436

vaikas · 2022-12-10T06:17:42Z

Summary

Wire the configmap through to the webhook so that we can start pulling the keys from it for validation. Add new fields to CIP to refer to TrustRoot so that you can specify which keys/certs should be used for validation.
Pretty big cleanup / refactoring on the validation logic to clean it up and not pass so much stuff through all over the place, unify by using cosign.CheckOpts for passing arguments, and construct them as appropriate.

Fixes #133

Release Note

add TrustRootRef to Keyless and CTLog specs so that you can specify which TrustRoot should be used for validation.

Documentation

…vailable to webhook. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

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

codecov-commenter · 2022-12-12T06:18:57Z

Codecov Report

Merging #436 (e14c9c8) into main (96dde0c) will decrease coverage by 1.13%.
The diff coverage is 34.35%.

@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
- Coverage   53.11%   51.97%   -1.14%     
==========================================
  Files          38       38              
  Lines        3660     3771     +111     
==========================================
+ Hits         1944     1960      +16     
- Misses       1585     1672      +87     
- Partials      131      139       +8

Impacted Files	Coverage Δ
...g/apis/policy/v1alpha1/clusterimagepolicy_types.go	`0.00% <ø> (ø)`
...kg/apis/policy/v1beta1/clusterimagepolicy_types.go	`0.00% <ø> (ø)`
pkg/webhook/validator.go	`58.56% <27.87%> (-5.72%)`	⬇️
...s/policy/v1alpha1/clusterimagepolicy_conversion.go	`70.27% <33.33%> (-1.70%)`	⬇️
pkg/webhook/validation.go	`79.48% <90.00%> (+13.69%)`	⬆️
pkg/apis/config/sigstore_keys.go	`28.81% <100.00%> (ø)`
pkg/apis/config/store.go	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

…h case use offline. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

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

hectorj2f

Generally it looks good to me. Is it ready for a final review ?

vaikas · 2022-12-13T15:46:17Z

I'm just adding some more tests :)

…

On Tue, Dec 13, 2022 at 7:14 AM Hector Fernandez ***@***.***> wrote: ***@***.**** commented on this pull request. Generally it looks good to me. Is it ready for a final review ? — Reply to this email directly, view it on GitHub <#436 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACWB45DPVRPMCDT65OJX7STWNCHENANCNFSM6AAAAAAS2CBXM4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

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

vaikas · 2022-12-13T21:15:22Z

@hectorj2f ready to go now.

hectorj2f · 2022-12-13T21:23:55Z

config/300-clusterimagepolicy.yaml

                        type: object
                        properties:
+                          trustRootRef:
+                            description: Use the Public Key from the referred TrustRoot.TLog


I realized we are using a confusing mix of terms between CTLog and TLog imho. CTLog sets the configuration to verify the authority against a Rekor instance.For the SigstoreKey, we refer to TLog as that rekor instance. Aren't we mixing these two terms ? Or it is just me :).

You're not the only one confused :) That was an early screw up in the namings. I reckon we should fix this in the move to say v1beta2 or whatever the next one is that allows us to break the namings. But rather than keep the bad naming going, I tried to fix it here to be consistent with what we want it to be.

I feel my confusion comes from the usage of ctlog here https://github.com/sigstore/policy-controller/pull/436/files#diff-83af2b17cb5361a5b3357d63c88e4965bfa07cb58917fbc0b88dd071cea39ccaR113 when we refer to a TLog resource type.

The top level (the one on L103 is the broken name, it should be TLog). The object it points to internally is Tlog, and there we use TLog.

Spec.CTLog Should really be Spec.TLog.

I opened #442
to track this for the next API rev.

hectorj2f · 2022-12-13T21:24:57Z

docs/api-types/index.md

 | url | URL defines a url to the keyless instance. | apis.URL | false |
 | identities | Identities sets a list of identities. | [][Identity](#identity) | false |
 | ca-cert | CACert sets a reference to CA certificate | [KeyRef](#keyref) | false |
+| trustRootRef | Use the Certificate Chain from the referred TrustRoot.CertificateAuthorities and TrustRoot.CTLog | string | false |


I thought you wrote TrustRoot.TLog in the description. You use TrustRoot.CTLog here.

I see you refer to the keyless, so you use CTLog although I will assume you refer to the rekor instance instead and so use TLog.

Well, this one is actually the CTLog, since it's the CA for Fulcio, and the CTLog for the Trillian that Fulcio writes to.

hectorj2f · 2022-12-13T21:56:33Z

pkg/apis/policy/v1alpha1/clusterimagepolicy_types.go

 	URL *apis.URL `json:"url,omitempty"`
+	// Use the Public Key from the referred TrustRoot.TLog
+	// +optional
+	TrustRootRef string `json:"trustRootRef,omitempty"`


I assume this value is referred to the name of the TrustRoot, don't you prefer to use an ObjectReference instead ?

Felt like overkill here since then we'd need to plumb in the gvk stuff as well. If you feel strongly, I can change it, just felt it would convolute the API unnecessarily.

Also, because where it gets used it's been normalized to configmap, just felt wonky too.

Definitely it isn't a blocker. You could use a LocalObjectReference due to the cluster scope of trustroots.

vaikas added 2 commits December 11, 2022 15:49

Start plumbing through the use of TrustRoot to CIPs. Make TrustRoot a…

b950421

…vailable to webhook. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

refactor validation code, cleanup & unify. plumb rekor/fulcio keys th…

ca1f955

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

vaikas force-pushed the tuf-part-2 branch from 8e3869a to ca1f955 Compare December 12, 2022 06:10

vaikas added 3 commits December 12, 2022 14:08

Add ctlog entries

279d12e

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

when no rekor / rekorpubkeys, skip tlog unless there are keys in whic…

e14c9c8

…h case use offline. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

Add tests for fetching fulcio certs from TrustRoot.

0841dfb

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

hectorj2f reviewed Dec 13, 2022

View reviewed changes

Add UT

a5e7806

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

vaikas changed the title ~~[WIP]: Start plumbing through the TrustRoot CRD to CIPs. Make TrustRoot available to webhook.~~ Plumb TrustRoot CRD through to CIP CRDs. Make TrustRoot available to webhook, clean up and refactor checkOpts logic. Dec 13, 2022

hectorj2f reviewed Dec 13, 2022

View reviewed changes

vaikas enabled auto-merge December 13, 2022 21:47

hectorj2f reviewed Dec 13, 2022

View reviewed changes

hectorj2f approved these changes Dec 13, 2022

View reviewed changes

vaikas merged commit 7c2c29f into sigstore:main Dec 13, 2022

vaikas deleted the tuf-part-2 branch December 13, 2022 22:14

vaikas mentioned this pull request Dec 21, 2022

Add support for non-public, and multiple TUF roots and a way to configure them. #138

Closed

Plumb TrustRoot CRD through to CIP CRDs. Make TrustRoot available to webhook, clean up and refactor checkOpts logic. #436

Plumb TrustRoot CRD through to CIP CRDs. Make TrustRoot available to webhook, clean up and refactor checkOpts logic. #436

Uh oh!

Conversation

vaikas commented Dec 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Release Note

Documentation

Uh oh!

codecov-commenter commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hectorj2f left a comment

Choose a reason for hiding this comment

Uh oh!

vaikas commented Dec 13, 2022 via email

Uh oh!

vaikas commented Dec 13, 2022

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vaikas commented Dec 10, 2022 •

edited

Loading

codecov-commenter commented Dec 12, 2022 •

edited

Loading