From 97f37cae197f778104fe88378dd450fb0511e6f0 Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Thu, 1 Jun 2023 22:44:49 +0000 Subject: [PATCH] update taskrun and pipelinerun condition based on VerificationResult This commits updates taskrun and pipelinerun condition based on VerificationResult to add TrustedResourcesVerified condition. The condition will be marked as false if verification policy fails, or no matching policies when feature flag is set to fail. The condition will be set to true if verification passes. No condition is added when verification is skipped. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- docs/trusted-resources.md | 43 +++++++++++++ pkg/reconciler/pipelinerun/pipelinerun.go | 48 +++++++++++--- .../pipelinerun/pipelinerun_test.go | 62 ++++++++++++++----- pkg/reconciler/taskrun/taskrun.go | 29 +++++++-- pkg/reconciler/taskrun/taskrun_test.go | 61 +++++++++++++----- pkg/trustedresources/verify.go | 5 +- 6 files changed, 202 insertions(+), 46 deletions(-) diff --git a/docs/trusted-resources.md b/docs/trusted-resources.md index 282f7dce654..f8fca4f74d5 100644 --- a/docs/trusted-resources.md +++ b/docs/trusted-resources.md @@ -73,6 +73,49 @@ Or patch the new values: kubectl patch configmap feature-flags -n tekton-pipelines -p='{"data":{"trusted-resources-verification-no-match-policy":"fail"}} ``` + #### TaskRun and PipelineRun status update +Trusted resources will update the taskrun's [condition](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties) to indicate if it passes verification or not. + +The following tables illustrate how the conditions are impacted by feature flag and verification result. Note that if not `true` or `false` means this case doesn't update the corresponding condition. +**No Matching Policies:** +| | `Conditions.TrustedResourcesVerified` | `Conditions.Succeeded` | +|-----------------------------|---------------------------------------|------------------------| +| `no-match-policy`: "ignore" | | | +| `no-match-policy`: "warn" | False | | +| `no-match-policy`: "fail" | False | False | + +**Matching Policies(no matter what `trusted-resources-verification-no-match-policy` value is):** +| | `Conditions.TrustedResourcesVerified` | `Conditions.Succeeded` | +|--------------------------|---------------------------------------|------------------------| +| all policies pass | True | | +| any enforce policy fails | False | False | +| only warn policies fail | False | | + + +A successful sample `TrustedResourcesVerified` condition is: +```yaml +status: + conditions: + - lastTransitionTime: "2023-03-01T18:17:05Z" + message: Trusted resource verification passed + status: "True" + type: TrustedResourcesVerified +``` + +Failed sample `TrustedResourcesVerified` and `Succeeded` conditions are: +```yaml +status: + conditions: + - lastTransitionTime: "2023-03-01T18:17:05Z" + message: resource verification failed # This will be filled with detailed error message. + status: "False" + type: TrustedResourcesVerified + - lastTransitionTime: "2023-03-01T18:17:10Z" + message: resource verification failed + status: "False" + type: Succeeded +``` + #### Config key at VerificationPolicy VerificationPolicy supports SecretRef or encoded public key data. diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index dee03436b64..4c20b536a21 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -375,11 +375,11 @@ func (c *Reconciler) resolvePipelineState( return nil, controller.NewPermanentError(err) } if resolvedTask.ResolvedTask != nil && resolvedTask.ResolvedTask.VerificationResult != nil { - vr := resolvedTask.ResolvedTask.VerificationResult - if vr.VerificationResultType == trustedresources.VerificationError { - err := fmt.Errorf("PipelineRun %s/%s referred pipeline failed signature verification for task %s : %w", pr.Namespace, pr.Name, resolvedTask.ResolvedTask.TaskName, vr.Err) + cond, err := conditionFromVerificationResult(resolvedTask.ResolvedTask.VerificationResult, pr, task.Name) + pr.Status.SetCondition(cond) + if err != nil { pr.Status.MarkFailed(ReasonResourceVerificationFailed, err.Error()) - return nil, controller.NewPermanentError(vr.Err) + return nil, controller.NewPermanentError(err) } } pst = append(pst, resolvedTask) @@ -419,10 +419,13 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get } } - if pipelineMeta.VerificationResult != nil && pipelineMeta.VerificationResult.VerificationResultType == trustedresources.VerificationError { - err := fmt.Errorf("PipelineRun %s/%s referred pipeline failed signature verification: %w", pr.Namespace, pr.Name, pipelineMeta.VerificationResult.Err) - pr.Status.MarkFailed(ReasonResourceVerificationFailed, err.Error()) - return controller.NewPermanentError(err) + if pipelineMeta.VerificationResult != nil { + cond, err := conditionFromVerificationResult(pipelineMeta.VerificationResult, pr, pipelineMeta.Name) + pr.Status.SetCondition(cond) + if err != nil { + pr.Status.MarkFailed(ReasonResourceVerificationFailed, err.Error()) + return controller.NewPermanentError(err) + } } d, err := dag.Build(v1beta1.PipelineTaskList(pipelineSpec.Tasks), v1beta1.PipelineTaskList(pipelineSpec.Tasks).Deps()) @@ -1411,3 +1414,32 @@ func updatePipelineRunStatusFromChildRefs(logger *zap.SugaredLogger, pr *v1beta1 } pr.Status.ChildReferences = newChildRefs } + +// conditionFromVerificationResult returns the ConditionTrustedResourcesVerified condition based on the VerificationResult, err is returned when the VerificationResult type is VerificationError +func conditionFromVerificationResult(verificationResult *trustedresources.VerificationResult, pr *v1beta1.PipelineRun, resourceName string) (*apis.Condition, error) { + var condition *apis.Condition + var err error + switch verificationResult.VerificationResultType { + case trustedresources.VerificationError: + err = fmt.Errorf("PipelineRun %s/%s referred resource %s failed signature verification: %w", pr.Namespace, pr.Name, resourceName, verificationResult.Err) + condition = &apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionFalse, + Message: err.Error(), + } + case trustedresources.VerificationWarn: + condition = &apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionFalse, + Message: verificationResult.Err.Error(), + } + case trustedresources.VerificationPass: + condition = &apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionTrue, + } + case trustedresources.VerificationSkip: + // do nothing + } + return condition, err +} diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index edbf15bf241..004916d8b91 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -46,6 +46,8 @@ import ( "github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim" resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common" remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource" + "github.com/tektoncd/pipeline/pkg/trustedresources" + "github.com/tektoncd/pipeline/pkg/trustedresources/verifier" "github.com/tektoncd/pipeline/test" "github.com/tektoncd/pipeline/test/diff" "github.com/tektoncd/pipeline/test/names" @@ -11609,28 +11611,46 @@ spec: pipelineRef: resolver: %s `, resolverName)) - + failNoMatchCondition := &apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionFalse, + Message: fmt.Sprintf("failed to get matched policies: %s: no matching policies are found for resource: %s against source: %s", trustedresources.ErrNoMatchedPolicies, ts.Name, ""), + } + passCondition := &apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionTrue, + } + failNoKeysCondition := &apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionFalse, + Message: fmt.Sprintf("failed to get verifiers for resource %s from namespace %s: %s", ts.Name, ts.Namespace, verifier.ErrEmptyPublicKeys), + } testCases := []struct { - name string - task []*v1beta1.Task - noMatchPolicy string - verificationPolicies []*v1alpha1.VerificationPolicy + name string + task []*v1beta1.Task + noMatchPolicy string + verificationPolicies []*v1alpha1.VerificationPolicy + wantTrustedResourcesCondition *apis.Condition }{{ - name: "ignore no match policy", - noMatchPolicy: config.IgnoreNoMatchPolicy, - verificationPolicies: noMatchPolicy, + name: "ignore no match policy", + noMatchPolicy: config.IgnoreNoMatchPolicy, + verificationPolicies: noMatchPolicy, + wantTrustedResourcesCondition: nil, }, { - name: "warn no match policy", - noMatchPolicy: config.WarnNoMatchPolicy, - verificationPolicies: noMatchPolicy, + name: "warn no match policy", + noMatchPolicy: config.WarnNoMatchPolicy, + verificationPolicies: noMatchPolicy, + wantTrustedResourcesCondition: failNoMatchCondition, }, { - name: "pass enforce policy", - noMatchPolicy: config.FailNoMatchPolicy, - verificationPolicies: vps, + name: "pass enforce policy", + noMatchPolicy: config.FailNoMatchPolicy, + verificationPolicies: vps, + wantTrustedResourcesCondition: passCondition, }, { - name: "only fail warn policy", - noMatchPolicy: config.FailNoMatchPolicy, - verificationPolicies: warnPolicy, + name: "only fail warn policy", + noMatchPolicy: config.FailNoMatchPolicy, + verificationPolicies: warnPolicy, + wantTrustedResourcesCondition: failNoKeysCondition, }, } for _, tc := range testCases { @@ -11659,6 +11679,10 @@ spec: reconciledRun, _ := prt.reconcileRun("foo", "test-pipelinerun", []string{}, false) checkPipelineRunConditionStatusAndReason(t, reconciledRun, corev1.ConditionUnknown, v1beta1.PipelineRunReasonRunning.String()) + gotVerificationCondition := reconciledRun.Status.GetCondition(trustedresources.ConditionTrustedResourcesVerified) + if d := cmp.Diff(tc.wantTrustedResourcesCondition, gotVerificationCondition, ignoreLastTransitionTime); d != "" { + t.Error(diff.PrintWantGot(d)) + } }) } } @@ -11838,6 +11862,10 @@ spec: reconciledRun, _ := prt.reconcileRun("foo", "test-pipelinerun", []string{}, true) checkPipelineRunConditionStatusAndReason(t, reconciledRun, corev1.ConditionFalse, ReasonResourceVerificationFailed) + gotVerificationCondition := reconciledRun.Status.GetCondition(trustedresources.ConditionTrustedResourcesVerified) + if gotVerificationCondition == nil || gotVerificationCondition.Status != corev1.ConditionFalse { + t.Errorf("Expected to have false condition, but had %v", gotVerificationCondition) + } }) } } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 100ee1dd258..75f6dcd7e8b 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -345,10 +345,31 @@ func (c *Reconciler) prepare(ctx context.Context, tr *v1beta1.TaskRun) (*v1beta1 } } - if taskMeta.VerificationResult != nil && taskMeta.VerificationResult.VerificationResultType == trustedresources.VerificationError { - logger.Errorf("TaskRun %s/%s referred task failed signature verification", tr.Namespace, tr.Name) - tr.Status.MarkResourceFailed(podconvert.ReasonResourceVerificationFailed, taskMeta.VerificationResult.Err) - return nil, nil, controller.NewPermanentError(taskMeta.VerificationResult.Err) + if taskMeta.VerificationResult != nil { + switch taskMeta.VerificationResult.VerificationResultType { + case trustedresources.VerificationError: + logger.Errorf("TaskRun %s/%s referred task failed signature verification", tr.Namespace, tr.Name) + tr.Status.MarkResourceFailed(podconvert.ReasonResourceVerificationFailed, taskMeta.VerificationResult.Err) + tr.Status.SetCondition(&apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionFalse, + Message: taskMeta.VerificationResult.Err.Error(), + }) + return nil, nil, controller.NewPermanentError(taskMeta.VerificationResult.Err) + case trustedresources.VerificationSkip: + // do nothing + case trustedresources.VerificationWarn: + tr.Status.SetCondition(&apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionFalse, + Message: taskMeta.VerificationResult.Err.Error(), + }) + case trustedresources.VerificationPass: + tr.Status.SetCondition(&apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionTrue, + }) + } } rtr := &resources.ResolvedTask{ diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 0bbf49b5ecc..a5bd2ab7a06 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -49,6 +49,7 @@ import ( resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common" remoteresource "github.com/tektoncd/pipeline/pkg/resolution/resource" "github.com/tektoncd/pipeline/pkg/trustedresources" + "github.com/tektoncd/pipeline/pkg/trustedresources/verifier" "github.com/tektoncd/pipeline/pkg/workspace" "github.com/tektoncd/pipeline/test" "github.com/tektoncd/pipeline/test/diff" @@ -4958,27 +4959,47 @@ spec: status: podName: the-pod `, resolverName)) + + failNoMatchCondition := &apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionFalse, + Message: fmt.Sprintf("failed to get matched policies: %s: no matching policies are found for resource: %s against source: %s", trustedresources.ErrNoMatchedPolicies, ts.Name, ""), + } + passCondition := &apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionTrue, + } + failNoKeysCondition := &apis.Condition{ + Type: trustedresources.ConditionTrustedResourcesVerified, + Status: corev1.ConditionFalse, + Message: fmt.Sprintf("failed to get verifiers for resource %s from namespace %s: %s", ts.Name, ts.Namespace, verifier.ErrEmptyPublicKeys), + } testCases := []struct { - name string - task []*v1beta1.Task - noMatchPolicy string - verificationPolicies []*v1alpha1.VerificationPolicy + name string + task []*v1beta1.Task + noMatchPolicy string + verificationPolicies []*v1alpha1.VerificationPolicy + wantTrustedResourcesCondition *apis.Condition }{{ - name: "ignore no match policy", - noMatchPolicy: config.IgnoreNoMatchPolicy, - verificationPolicies: noMatchPolicy, + name: "ignore no match policy", + noMatchPolicy: config.IgnoreNoMatchPolicy, + verificationPolicies: noMatchPolicy, + wantTrustedResourcesCondition: nil, }, { - name: "warn no match policy", - noMatchPolicy: config.WarnNoMatchPolicy, - verificationPolicies: noMatchPolicy, + name: "warn no match policy", + noMatchPolicy: config.WarnNoMatchPolicy, + verificationPolicies: noMatchPolicy, + wantTrustedResourcesCondition: failNoMatchCondition, }, { - name: "pass enforce policy", - noMatchPolicy: config.FailNoMatchPolicy, - verificationPolicies: vps, + name: "pass enforce policy", + noMatchPolicy: config.FailNoMatchPolicy, + verificationPolicies: vps, + wantTrustedResourcesCondition: passCondition, }, { - name: "only fail warn policy", - noMatchPolicy: config.FailNoMatchPolicy, - verificationPolicies: warnPolicy, + name: "only fail warn policy", + noMatchPolicy: config.FailNoMatchPolicy, + verificationPolicies: warnPolicy, + wantTrustedResourcesCondition: failNoKeysCondition, }, } for _, tc := range testCases { @@ -5018,6 +5039,10 @@ status: if condition != nil && condition.Reason != v1beta1.TaskRunReasonRunning.String() { t.Errorf("Expected reason %q but was %s", v1beta1.TaskRunReasonRunning.String(), condition.Reason) } + gotVerificationCondition := reconciledRun.Status.GetCondition(trustedresources.ConditionTrustedResourcesVerified) + if d := cmp.Diff(tc.wantTrustedResourcesCondition, gotVerificationCondition, ignoreLastTransitionTime); d != "" { + t.Error(diff.PrintWantGot(d)) + } }) } } @@ -5122,6 +5147,10 @@ status: if condition.Type != apis.ConditionSucceeded || condition.Status != corev1.ConditionFalse || condition.Reason != podconvert.ReasonResourceVerificationFailed { t.Errorf("Expected TaskRun to fail with reason \"%s\" but it did not. Final conditions were:\n%#v", podconvert.ReasonResourceVerificationFailed, tr.Status.Conditions) } + gotVerificationCondition := reconciledRun.Status.GetCondition(trustedresources.ConditionTrustedResourcesVerified) + if gotVerificationCondition == nil || gotVerificationCondition.Status != corev1.ConditionFalse { + t.Errorf("Expected to have false condition, but had %v", gotVerificationCondition) + } }) } } diff --git a/pkg/trustedresources/verify.go b/pkg/trustedresources/verify.go index cdf1c23a02e..49ceb0d4a78 100644 --- a/pkg/trustedresources/verify.go +++ b/pkg/trustedresources/verify.go @@ -33,12 +33,15 @@ import ( "github.com/tektoncd/pipeline/pkg/trustedresources/verifier" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" + "knative.dev/pkg/apis" "knative.dev/pkg/logging" ) const ( // SignatureAnnotation is the key of signature in annotation map SignatureAnnotation = "tekton.dev/signature" + // ConditionTrustedResourcesVerified specifies that the resources pass trusted resources verification or not. + ConditionTrustedResourcesVerified apis.ConditionType = "TrustedResourcesVerified" ) const ( @@ -191,7 +194,7 @@ func verifyResource(ctx context.Context, resource metav1.Object, k8s kubernetes. for _, p := range warnPolicies { verifiers, err := verifier.FromPolicy(ctx, k8s, p) if err != nil { - warn := fmt.Errorf("fails to get verifiers for resource %s from namespace %s: %w", resource.GetName(), resource.GetNamespace(), err) + warn := fmt.Errorf("failed to get verifiers for resource %s from namespace %s: %w", resource.GetName(), resource.GetNamespace(), err) logger.Warnf(warn.Error()) return VerificationResult{VerificationResultType: VerificationWarn, Err: warn} }