[TrustTask] Trusted taskspec verification #833
Conversation
tekton-robot
added
do-not-merge/work-in-progress
Yongxuanzhang
changed the title
Yongxuanzhang
changed the title
pipeline/trusted-resources/README.md
Outdated
|
|
||
| [](https://github.com/tektoncd/experimental/blob/master/LICENSE) | ||
|
|
||
| This is an experimental project to provide a seperate webhook for remote resources verification. |
There was a problem hiding this comment.
Can you elaborate on this? Is there a doc or issue I can read to catch up on the goal of this project? It sounds very interesting 👍
There was a problem hiding this comment.
Yes! This is proposed by @wlynch. The idea is to allow users to install/deploy a webhook in current tekton pipeline, where we can verify the remote resources (taskrun, pipelinerun, oci bundle etc.) with Cosign.
Maybe he can share the doc with you?
There was a problem hiding this comment.
super rough notes here: https://hackmd.io/93mfJPyDQKCyn0IKwjzgWQ#Design-Details - tl;dr it's basically a modification of Jason's original trusted task TEP, but using validation webhooks so we can prototype something before needing to make modifications to the Pipelines API.
Yongxuanzhang
force-pushed
the
trusted-resources
branch
from
861d9ff
to
d015dbf
Compare
tekton-robot
removed
the
do-not-merge/work-in-progress
pipeline/trusted-resources/cmd/webhook_trusted_resource/main.go
Outdated
Show resolved
Hide resolved
pipeline/trusted-resources/cmd/webhook_trusted_resource/main.go
Outdated
Show resolved
Hide resolved
pipeline/trusted-resources/pkg/webhook/validation_admit_test.go
Outdated
Show resolved
Hide resolved
pipeline/trusted-resources/pkg/webhook/validation_admit_test.go
Outdated
Show resolved
Hide resolved
| serviceAccount = "tekton-pipelines-webhook" | ||
| ) | ||
|
|
||
| func Test_verifyResources(t *testing.T) { |
There was a problem hiding this comment.
It's useful to have tests for both individual funcs (e.g. just the signature generation, etc.) as well as the overall verification. This way it's easier to figure out where exactly changes have been made if/when something breaks.
|
/test tekton-experimental-unit-tests |
pipeline/trusted-resources/OWNERS
Outdated
| approvers: | ||
| - afrittoli | ||
| - bobcatfish | ||
| - ImJasonH | ||
| - jerop |
There was a problem hiding this comment.
This is copy/paste from pipeline in pipeline?
There was a problem hiding this comment.
yes, I will update them!
pipeline/trusted-resources/config/webhook-trusted-resources.yaml
Outdated
Show resolved
Hide resolved
|
|
||
| // Look up the webhook secret, and fetch the CA cert bundle. | ||
| secret, err := ac.secretlister.Secrets(system.Namespace()).Get(ac.secretName) | ||
| if err != nil { | ||
| logger.Errorw("Error fetching secret", zap.Error(err)) | ||
| return err | ||
| } | ||
| caCert, ok := secret.Data[certresources.CACert] | ||
| if !ok { | ||
| return fmt.Errorf("secret %q is missing %q key", ac.secretName, certresources.CACert) | ||
| } | ||
|
|
||
| // Reconcile the webhook configuration. | ||
| return ac.reconcileValidatingWebhook(ctx, caCert) | ||
| } | ||
|
|
||
| func (ac *reconciler) reconcileValidatingWebhook(ctx context.Context, caCert []byte) error { | ||
| logger := logging.FromContext(ctx) | ||
|
|
||
| rules := make([]admissionregistrationv1.RuleWithOperations, 0, len(ac.handlers)) | ||
| for gvk := range ac.handlers { | ||
| plural := strings.ToLower(flect.Pluralize(gvk.Kind)) | ||
|
|
||
| rules = append(rules, admissionregistrationv1.RuleWithOperations{ | ||
| Operations: []admissionregistrationv1.OperationType{ | ||
| admissionregistrationv1.Create, | ||
| admissionregistrationv1.Update, | ||
| admissionregistrationv1.Delete, | ||
| }, | ||
| Rule: admissionregistrationv1.Rule{ | ||
| APIGroups: []string{gvk.Group}, | ||
| APIVersions: []string{gvk.Version}, | ||
| Resources: []string{plural, plural + "/status"}, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| // Sort the rules by Group, Version, Kind so that things are deterministically ordered. | ||
| sort.Slice(rules, func(i, j int) bool { | ||
| lhs, rhs := rules[i], rules[j] | ||
| if lhs.APIGroups[0] != rhs.APIGroups[0] { | ||
| return lhs.APIGroups[0] < rhs.APIGroups[0] | ||
| } | ||
| if lhs.APIVersions[0] != rhs.APIVersions[0] { | ||
| return lhs.APIVersions[0] < rhs.APIVersions[0] | ||
| } | ||
| return lhs.Resources[0] < rhs.Resources[0] | ||
| }) | ||
|
|
||
| configuredWebhook, err := ac.vwhlister.Get(ac.key.Name) | ||
| if err != nil { | ||
| return fmt.Errorf("error retrieving webhook: %w", err) | ||
| } | ||
|
|
||
| current := configuredWebhook.DeepCopy() | ||
|
|
||
| // Set the owner to namespace. | ||
| ns, err := ac.client.CoreV1().Namespaces().Get(ctx, system.Namespace(), metav1.GetOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to fetch namespace: %w", err) | ||
| } | ||
| nsRef := *metav1.NewControllerRef(ns, corev1.SchemeGroupVersion.WithKind("Namespace")) | ||
| current.OwnerReferences = []metav1.OwnerReference{nsRef} | ||
|
|
||
| for i, wh := range current.Webhooks { | ||
| if wh.Name != current.Name { | ||
| continue | ||
| } | ||
| cur := ¤t.Webhooks[i] | ||
| cur.Rules = rules | ||
|
|
||
| cur.NamespaceSelector = webhook.EnsureLabelSelectorExpressions( | ||
| cur.NamespaceSelector, | ||
| &metav1.LabelSelector{ | ||
| MatchExpressions: []metav1.LabelSelectorRequirement{{ | ||
| Key: "webhooks.knative.dev/exclude", | ||
| Operator: metav1.LabelSelectorOpDoesNotExist, | ||
| }}, | ||
| }) | ||
|
|
||
| cur.ClientConfig.CABundle = caCert | ||
| if cur.ClientConfig.Service == nil { | ||
| return fmt.Errorf("missing service reference for webhook: %s", wh.Name) | ||
| } | ||
| cur.ClientConfig.Service.Path = ptr.String(ac.Path()) | ||
| } | ||
|
|
||
| if ok, err := kmp.SafeEqual(configuredWebhook, current); err != nil { | ||
| return fmt.Errorf("error diffing webhooks: %w", err) | ||
| } else if !ok { | ||
| logger.Info("Updating webhook") | ||
| vwhclient := ac.client.AdmissionregistrationV1().ValidatingWebhookConfigurations() | ||
| if _, err := vwhclient.Update(ctx, current, metav1.UpdateOptions{}); err != nil { | ||
| return fmt.Errorf("failed to update webhook: %w", err) | ||
| } | ||
| } else { | ||
| logger.Info("Webhook is valid") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I think this should be handled by knative (e.g. this is just duplicate of https://github.com/knative/pkg/blob/4fcbc1bc12e800965853ec9d6eafd293c2ae347a/webhook/resourcesemantics/validation/reconcile_config.go)?
There was a problem hiding this comment.
After reading through this and the other file, I think I get what you're trying to do here.
Instead of forking the resourcesemantics code, I think what we should try to do is lean on the handlers field and inject our own Validatable object.
e.g. I'm thinking something like...
func New(...) *controller.Impl {
return validation.NewAdmissionController(ctx,
"webhook.trustedtasks.tekton.dev",
"/resource-validation",
// List the types to validate, this from knative.dev/sample-controller
map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
v1beta1.SchemeGroupVersion.WithKind("TaskRun"): &trustedTaskRun{},
},
nil,
true,
)
}
type trustedTaskRun = v1beta.TaskRun
func (*trustedTaskRun) Validate(ctx context.Context) {
// validate here
...
}
func (*trustedTaskRun) SetDefaults(ctx context.Context) {
// Purposely do nothing
}I think this should do what we want, but still let us rely on the knative controller code for cert management and webhook setup. We could also look into the callbacks field if handlers doesn't quite fit what we want.
There was a problem hiding this comment.
And if we're still stuck we can always reach out to the Knative folks on slack. 😃
There was a problem hiding this comment.
Ah this looks interesting, but when we call the validate in webhook, how do we make sure it is calling this Validate not the one we have in v1beta.TaskRun?
and this code will get
invalid receiver type *"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1".TaskRun (type not defined in this package)
Do you mean something like this? (I tried to correct some syntax error but not sure if this is what we want)
type trustedTaskRun struct{
v1beta1.TaskRun
}
func (tr *trustedTaskRun) Validate(ctx context.Context) (errs *apis.FieldError){
// validate here
fmt.Println("!!!! validate new")
return nil
}
func (tr *trustedTaskRun) SetDefaults(ctx context.Context) {
// do nothing
fmt.Println("!!!! set defaults new")
}
There was a problem hiding this comment.
This is resolved and can refer to this repo:
https://github.com/knative-sandbox/net-istio/blob/edc2d2a327/pkg/defaults/deployment_defaults.go
| var verifier sigstoresignature.Verifier | ||
| if tr.ObjectMeta.Annotations[kmsKey] != "" { | ||
| // Fetch key from kms | ||
| verifier, err = kms.Get(ctx, tr.ObjectMeta.Annotations[kmsKey], crypto.SHA256) |
There was a problem hiding this comment.
Are we sure this will always be SHA256?
There was a problem hiding this comment.
I also doubt there, this code is from chains...
https://github.com/tektoncd/chains/blob/122fe23242fd185c642fb82c042ef710de35850d/pkg/chains/signing/kms/kms.go#L41
| // Fetch public key from annotation | ||
| cosignPublicKeypath := filepath.Join(tr.ObjectMeta.Annotations[publicKey], "cosign.pub") | ||
| verifier, err = cosignsignature.LoadPublicKey(ctx, cosignPublicKeypath) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
A local path on a remote resource is problematic, since this depends on the remote state to match what was done locally at signing.
This might make more sense if this was a reference to a secret?
There was a problem hiding this comment.
refactored this a little bit
| tmpPrivFile, err := os.Create(filepath.Join(tmpDir, "cosign.key")) | ||
| if err != nil { | ||
| t.Fatalf("failed to create temp key file: %v", err) | ||
| } | ||
| defer tmpPrivFile.Close() | ||
| tmpPubFile, err := os.Create(filepath.Join(tmpDir, "cosign.pub")) | ||
| if err != nil { | ||
| t.Fatalf("failed to create temp pub file: %v", err) | ||
| } | ||
| defer tmpPubFile.Close() | ||
|
|
||
| // Generate a valid keypair. | ||
| keys, err := cosign.GenerateKeyPair(pf) | ||
| if err != nil { | ||
| t.Fatalf("failed to generate keypair: %v", err) | ||
| } | ||
|
|
||
| if _, err := tmpPrivFile.Write(keys.PrivateBytes); err != nil { | ||
| t.Fatalf("failed to write key file: %v", err) | ||
| } | ||
| if _, err := tmpPubFile.Write(keys.PublicBytes); err != nil { | ||
| t.Fatalf("failed to write pub file: %v", err) | ||
| } | ||
| return tmpPrivFile.Name(), tmpPubFile.Name() |
There was a problem hiding this comment.
I'd have 2 versions of this - one that's completely in memory and one that writes out the file. For most of the verify funcs, we can actually just generate the keys in memory and pass back a SignerVerifier. It's only for testing the e2e flows when reading from a file that we need to write this out.
Yongxuanzhang
force-pushed
the
trusted-resources
branch
from
d11986e
to
2e70bf5
Compare
| // Validate here | ||
| func (tr *TrustedTaskRun) Validate(ctx context.Context) (errs *apis.FieldError) { | ||
| k8sclient := kubeclient.Get(ctx) | ||
| //tektonClient := tkclient.Get(ctx) |
There was a problem hiding this comment.
not sure why this doesn't work, the error msg is
"Unable to fetch github.com/tektoncd/pipeline/pkg/client/resource/clientset/versioned.Interface from context. This context is not the application context (which is typically given to constructors via sharedmain).")
There was a problem hiding this comment.
Client injection needs to be done somewhere - typically this would be done by the sharedmain func in the controller, but in the unit test you need to do this yourself.
Luckily, knative client-gen gives us an easy solution for this! If you use this package instead of the normal fake client, it should work. https://pkg.go.dev/github.com/tektoncd/pipeline@v0.33.1/pkg/client/injection/client/fake
pipeline/trusted-resources/pkg/trustedtask/trustedtaskrun_test.go
Outdated
Show resolved
Hide resolved
|
/assign @wlynch |
| // Validate here | ||
| func (tr *TrustedTaskRun) Validate(ctx context.Context) (errs *apis.FieldError) { | ||
| k8sclient := kubeclient.Get(ctx) | ||
| //tektonClient := tkclient.Get(ctx) |
There was a problem hiding this comment.
Client injection needs to be done somewhere - typically this would be done by the sharedmain func in the controller, but in the unit test you need to do this yourself.
Luckily, knative client-gen gives us an easy solution for this! If you use this package instead of the normal fake client, it should work. https://pkg.go.dev/github.com/tektoncd/pipeline@v0.33.1/pkg/client/injection/client/fake
pipeline/trusted-resources/pkg/trustedtask/trustedtaskrun_test.go
Outdated
Show resolved
Hide resolved
pipeline/trusted-resources/pkg/trustedtask/trustedtaskrun_test.go
Outdated
Show resolved
Hide resolved
pipeline/trusted-resources/pkg/trustedtask/trustedtaskrun_test.go
Outdated
Show resolved
Hide resolved
pipeline/trusted-resources/pkg/trustedtask/trustedtaskrun_test.go
Outdated
Show resolved
Hide resolved
pipeline/trusted-resources/pkg/trustedtask/trustedtaskrun_test.go
Outdated
Show resolved
Hide resolved
|
Also make sure to add a line here so that this can be included in presubmit tests - experimental/test/presubmit-tests.sh Line 61 in 2d43123 |
Yongxuanzhang
force-pushed
the
trusted-resources
branch
from
d57e1ab
to
17e3d7e
Compare
| tektonClient versioned.Interface, | ||
| ) (errs *apis.FieldError) { | ||
| logger := logging.FromContext(ctx) | ||
| logger.Info("Verifying Resources") |
There was a problem hiding this comment.
My rule of thumb is - "would this be useful for a random person reading this in the logs with little/no context, or would this just be noise?"
Sometimes it means adding some more details / identifiers to be able to see what is going on with multiple requests.
Sometimes it's redundant and it's easiest to just remove them.
No wrong answer here, as long as we're being intentional about it! Use your best judgement.
pipeline/trusted-resources/pkg/trustedtask/trustedtaskrun_test.go
Outdated
Show resolved
Hide resolved
pipeline/trusted-resources/pkg/trustedtask/trustedtaskrun_test.go
Outdated
Show resolved
Hide resolved
pipeline/trusted-resources/pkg/trustedtask/trustedtaskrun_test.go
Outdated
Show resolved
Hide resolved
pipeline/trusted-resources/pkg/trustedtask/trustedtaskrun_test.go
Outdated
Show resolved
Hide resolved
pipeline/trusted-resources/pkg/trustedtask/trustedtaskrun_test.go
Outdated
Show resolved
Hide resolved
| bundle string, | ||
| verifier signature.Verifier, | ||
| signatureDecoding []byte, | ||
| k8sclient kubernetes.Interface, |
There was a problem hiding this comment.
The thinking was we don't actually need the k8sclient - it's just a means to create the k8schain which eventually becomes an Option, so we can just plumb it all the way through.
It's not that big of a deal though. We can ignore this for now.
Yongxuanzhang
force-pushed
the
trusted-resources
branch
2 times, most recently
from
cec049f
to
36487b6
Compare
Yongxuanzhang
changed the title
Yongxuanzhang
force-pushed
the
trusted-resources
branch
from
9a48edc
to
e706c15
Compare
Yongxuanzhang
changed the title
There was a problem hiding this comment.
Looking good! Please update the commit message / PR description to describe the changes we're making in this PR - see https://github.com/tektoncd/community/blob/main/standards.md#commits
pipeline/trusted-resources/README.md
Outdated
| ## Install | ||
|
|
||
| Install and configure [`ko`](https://github.com/google/ko). | ||
|
|
||
| Install tekton pipeline. To install from source, checkout to pipeline repo and execute: | ||
| ```bash | ||
| ko apply -f config/ | ||
| ``` | ||
|
|
||
| Then install the new admission webhook: | ||
| ```bash | ||
| kubectl create namespace trusted-task | ||
|
|
||
| # cosign generate-key-pair k8s://tekton-pipelines/signing-secrets | ||
| cosign generate-key-pair | ||
| kubectl create secret generic signing-secrets \ | ||
| --from-file=cosign.key=./cosign.key \ | ||
| --from-literal=cosign.password='1234'\ | ||
| --from-file=cosign.pub=./cosign.pub \ | ||
| -n trusted-task | ||
|
|
||
| ko apply -f config/ | ||
| ``` |
There was a problem hiding this comment.
README needs to be updated, since the config files don't actually exist yet.
| if err != nil { | ||
| t.Fatalf("Unexpected err %v", err) | ||
| } | ||
| signed.Annotations["tekton.dev/signature"] = signTaskSpec(t, signer, &ts.Spec) |
There was a problem hiding this comment.
Yay tests! This makes it much easier to see what is going on.
I'm okay with this as a first pass, but we'll want to address this in another PR.
As is, this is setting the signature of the TaskRun as the signature of the underlying Task - this means that you can actually tamper the TaskRun and still pass validation if you do something like...
apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
name: read-repo-run
spec:
taskRef:
name: foo
params:
- bar: asdf
+ bar: <bad modified thing>Since the signature is only capturing the TaskRef Task and not the entire TaskRun spec. This is also true for OCI and TaskSpec variants.
This would make sense as an annotation for the Task itself though!
There was a problem hiding this comment.
Ah yes, so we should not only verify the embedded tasks but also the taskrun.spec.
Yongxuanzhang
force-pushed
the
trusted-resources
branch
from
7532c43
to
a098f4e
Compare
Yongxuanzhang
changed the title
Yongxuanzhang
force-pushed
the
trusted-resources
branch
from
a098f4e
to
8398dfb
Compare
Yongxuanzhang
force-pushed
the
trusted-resources
branch
from
8398dfb
to
6d901ea
Compare
Yongxuanzhang
force-pushed
the
trusted-resources
branch
from
6d901ea
to
12a72e2
Compare
This commit is an initial step of work for Trust Task.(Forked from tektoncd/community#537) Prior this commit we cannot verify if the taskrun is manipulated or not before applying to the cluster. This commit leverages cosign to do verification on taskspec. Admission webhook and verification on taskrun.spec will be introduced in following PRs. With these work we can deploy a standalone webhook for verification without changing current Tekton Pipeline APIs.
Yongxuanzhang
force-pushed
the
trusted-resources
branch
from
12a72e2
to
6f44cbe
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wlynch The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
Changes
This commit is an initial step of work for Trust Task.(Forked from tektoncd/community#537)
Prior this commit we cannot verify if the taskrun is manipulated or not before applying to the cluster.
This commit leverages cosign to do verification on taskspec. Admission webhook and verification on taskrun.spec will be introduced in following PRs. With these work we can deploy a standalone webhook for verification without changing current Tekton Pipeline APIs.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide
for more details.