Add TrustRoot crd. #291

vaikas · 2022-10-05T22:14:19Z

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

Summary

This PR starts adding support for being able to support multiple TUF Roots / repositories as well as bring your own keys.
The compress/uncompress will be used to serialize/unserialize air-gapped entries provided by user.
For now only supports the bring your own keys case. Since everything will be boiled down to that, it's the first step to get that wired, then will need to add grabbing the keys/certs from the actual TUF roots / repositories, but since this is getting large as it is, but demonstrates e2e going from a CRD -> Reconcile -> ConfigMap should have the necessary pieces in place to add the TUF Remote / Repository support as a follow up.

High level design document is here:
https://docs.google.com/document/d/1a-tV9YyXHvtoaZXm-eMcqP-zFTWg6Pqfjyl8W4uS9F4/edit#heading=h.ynb0pmylorbd

Release Note

Add TrustRoot CRD for support multiple TUF roots as well as bring your own keys.

Documentation

JAORMX

Could you add some docs about this?

vaikas · 2022-11-08T17:01:39Z

@JAORMX yeah, I'm just trying to get the rough chainsaw size bits up while sorting how this may look.

mattmoor

Was mostly looking at broad strokes controller stuff, not looking too much at the details.

cmd/trustedkeys/main.go

pkg/reconciler/testing/v1alpha1/trustedkeys.go

pkg/reconciler/trustedkeys/controller.go

pkg/reconciler/trustedkeys/trustedkeys.go

codecov-commenter · 2022-12-09T06:37:56Z

Codecov Report

Merging #291 (6307a7b) into main (2bc604f) will decrease coverage by 5.79%.
The diff coverage is 24.01%.

@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
- Coverage   58.91%   53.11%   -5.80%     
==========================================
  Files          31       38       +7     
  Lines        3052     3660     +608     
==========================================
+ Hits         1798     1944     +146     
- Misses       1152     1585     +433     
- Partials      102      131      +29

Impacted Files	Coverage Δ
cmd/policy_webhook/main.go	`0.00% <0.00%> (ø)`
pkg/apis/policy/v1alpha1/register.go	`0.00% <0.00%> (ø)`
pkg/apis/policy/v1alpha1/trustroot_defaults.go	`0.00% <0.00%> (ø)`
pkg/apis/policy/v1alpha1/trustroot_types.go	`0.00% <0.00%> (ø)`
pkg/apis/policy/v1alpha1/zz_generated.deepcopy.go	`13.33% <0.00%> (-7.62%)`	⬇️
pkg/tuf/repo.go	`26.84% <26.84%> (ø)`
pkg/apis/config/sigstore_keys.go	`28.81% <28.81%> (ø)`
pkg/apis/policy/v1alpha1/trustroot_validation.go	`30.18% <30.18%> (ø)`
pkg/reconciler/trustroot/trustroot.go	`36.14% <36.14%> (ø)`
pkg/reconciler/trustroot/controller.go	`86.66% <86.66%> (ø)`
... and 1 more

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>

hectorj2f · 2022-12-10T18:06:32Z

pkg/apis/config/sigstore_keys.go

+	// The time the *entire* chain was valid. This is at max the
+	// longest interval when *all* certificates in the chain where valid,
+	// but it MAY be shorter.
+	//       dev.sigstore.common.v1.TimeRange valid_for = 4;


Wouldn't this be a TTL ?

I think the problem is that it will require a start and optionally an end :) So, I'd like to figure out how this should work hence the TODO :)

pkg/apis/config/sigstore_keys.go

hectorj2f · 2022-12-10T18:10:38Z

pkg/apis/config/sigstore_keys.go

+	// Certificate Transparency Log
+	CTLog []TransparencyLogInstance `json:"ctLog"`
+	// Trusted timestamping authorities
+	TimeStampAuthorities []CertificateAuthority `json:"timestampAuthorities"`


Are these optionals ? If so we could use pointers []*CertificateAuthority and add omitempty to the json fields

In the protos definition, this appears as timestamp_authorities

yeah, I think we'll need either tlog or tsa here. Though, I suppose anything but CA could be optional.

I think though that array type has a native nil, so I think the entries in it do not need to be pointers. Just need to add the // +optional above this.

I am wondering now whether this conflicts with https://github.com/sigstore/policy-controller/pull/391/files#diff-83af2b17cb5361a5b3357d63c88e4965bfa07cb58917fbc0b88dd071cea39ccaR117 .

So for consistency, I think it might be better to just specify the trusted certchain in one place (trustroot) rather than bake it into each CIP, just like for Fulcio/Rekor we don't bake the certs/keys in.

re having the underscore in the name, that's not right :) For the JSON fields, the convention is camelcase.

I think in the fullness of time the URLs from the keyless/ctlog might just go away and you just refer to the trust root, since the entries in the TrustRoot have the URLs necessary to construct the clients as well as all the other necessary bits.

So for consistency, I think it might be better to just specify the trusted certchain in one place (trustroot) rather than bake it into each CIP, just like for Fulcio/Rekor we don't bake the certs/keys in.

But we bake in the keys, and ctlog references 🤔 .

Yeah, that's why I say in fullness of time :) I think CTLog / Fulcio URL will go away and they will just refer to the TrustRoot.

hectorj2f · 2022-12-10T18:11:47Z

pkg/apis/config/sigstore_keys.go

+type CertificateAuthority struct {
+	// The root certificate MUST be self-signed, and so the subject and
+	// issuer are the same.
+	Subject DistinguishedName `json:"subject"`


Don't you wanna use subject,omitempty

I think this is required as per the proto comments so that we can ensure the root cert is self signed.

pkg/apis/config/sigstore_keys.go

hectorj2f · 2022-12-10T18:21:01Z

pkg/tuf/repo.go

+	local := client.MemoryLocalStore()
+	tufClient := client.NewClient(local, remote)
+
+	// TODO(vaikas): What should we do with above tufClient validation


maybe check the targets

Yeah, I'll add that as part of when I add support for bringing those in (next PR or the one after) since this is getting big already 🤣

pkg/apis/config/sigstore_keys.go

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

mattmoor

Broad strokes of all of the controller stuff generally lgtm, but I haven't reviewed all of the TUF/reconciler details.

.github/workflows/kind-e2e-trustroot-crd.yaml

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

hectorj2f

lgtm

vaikas force-pushed the tuf-roots-crd branch from 39448b8 to 53054fc Compare October 7, 2022 19:10

JAORMX suggested changes Nov 8, 2022

View reviewed changes

vaikas force-pushed the tuf-roots-crd branch from 9c2ba8b to 8c8d85e Compare November 14, 2022 22:32

vaikas mentioned this pull request Nov 15, 2022

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

Closed

vaikas force-pushed the tuf-roots-crd branch 4 times, most recently from 11d31de to 6e9a02e Compare December 8, 2022 20:49

mattmoor reviewed Dec 8, 2022

View reviewed changes

vaikas force-pushed the tuf-roots-crd branch 2 times, most recently from 5cc4704 to 669df0b Compare December 9, 2022 03:44

vaikas changed the title ~~WIP: Add tuf_root crd.~~ Add TrustRoot crd. Dec 9, 2022

CRD work for multi-tuf.

4699a06

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

vaikas force-pushed the tuf-roots-crd branch from e17cd9f to 4699a06 Compare December 9, 2022 18:13

vaikas mentioned this pull request Dec 10, 2022

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

Merged

vaikas enabled auto-merge December 10, 2022 06:18

hectorj2f reviewed Dec 10, 2022

View reviewed changes

pkg/apis/config/sigstore_keys.go Outdated Show resolved Hide resolved

hectorj2f reviewed Dec 10, 2022

View reviewed changes

pkg/apis/config/sigstore_keys.go Outdated Show resolved Hide resolved

hectorj2f reviewed Dec 10, 2022

View reviewed changes

pkg/apis/config/sigstore_keys.go Show resolved Hide resolved

Address PR feedback, pluralize fields.

c55ee46

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

mattmoor previously approved these changes Dec 10, 2022

View reviewed changes

.github/workflows/kind-e2e-trustroot-crd.yaml Outdated Show resolved Hide resolved

fix the whitespace.

6307a7b

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

vaikas dismissed mattmoor’s stale review via 6307a7b December 10, 2022 23:50

vaikas disabled auto-merge December 11, 2022 05:25

vaikas enabled auto-merge December 11, 2022 19:58

hectorj2f approved these changes Dec 11, 2022

View reviewed changes

vaikas merged commit 96dde0c into sigstore:main Dec 11, 2022

vaikas deleted the tuf-roots-crd branch December 11, 2022 22:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TrustRoot crd. #291

Add TrustRoot crd. #291

vaikas commented Oct 5, 2022 •

edited

JAORMX left a comment

vaikas commented Nov 8, 2022

mattmoor left a comment

codecov-commenter commented Dec 9, 2022 •

edited

hectorj2f Dec 10, 2022

vaikas Dec 10, 2022

hectorj2f Dec 10, 2022

hectorj2f Dec 10, 2022

vaikas Dec 10, 2022

vaikas Dec 10, 2022

hectorj2f Dec 10, 2022

vaikas Dec 10, 2022

vaikas Dec 10, 2022

vaikas Dec 10, 2022

hectorj2f Dec 11, 2022

vaikas Dec 11, 2022

hectorj2f Dec 10, 2022

vaikas Dec 10, 2022

hectorj2f Dec 10, 2022

vaikas Dec 10, 2022

mattmoor left a comment

hectorj2f left a comment

Add TrustRoot crd. #291

Add TrustRoot crd. #291

Conversation

vaikas commented Oct 5, 2022 • edited

Summary

Release Note

Documentation

JAORMX left a comment

Choose a reason for hiding this comment

vaikas commented Nov 8, 2022

mattmoor left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 9, 2022 • edited

Codecov Report

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

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

mattmoor left a comment

Choose a reason for hiding this comment

hectorj2f left a comment

Choose a reason for hiding this comment

vaikas commented Oct 5, 2022 •

edited

codecov-commenter commented Dec 9, 2022 •

edited