Chains adds chains.tekton.dev/signed=true annotation even if no keys supplied

[PuneetPunamiya]

• When chains is installed, it adds the signed: true annotation to the taskruns even though the controller is filled with logs around not being able to configure a signer and not having any configured keys.

For example :

I create this simple taskrun which is mentioned in the getting-started tutorial

apiVersion: tekton.dev/v1beta1
kind: TaskRun
metadata:
  name: build-push-run-output-image-test
spec:
  serviceAccountName: ""
  taskSpec:
    results:
    - name: IMAGE_URL
      type: string
    - name: IMAGE_DIGEST
      type: string
    steps:
    - name: create-image
      image: busybox
      script: |-
        #!/usr/bin/env sh
        echo 'gcr.io/foo/bar' | tee $(results.IMAGE_URL.path)
        echo 'sha256:05f95b26ed10668b7183c1e2da98610e91372fa9f510046d4ce5812addad86b5' | tee $(results.IMAGE_DIGEST.path)

kubectl create -f https://raw.githubusercontent.com/tektoncd/chains/main/examples/taskruns/task-output-image.yaml

Now even if no field is provided in the the chain-config configMap it adds annotation chains.tekton.dev/signed=true in the taskrun

╰─ tkn tr describe --last 
Name:              build-push-run-output-image-test
Namespace:         default
Service Account:   default
Timeout:           1h0m0s
Labels:
 app.kubernetes.io/managed-by=tekton-pipelines
Annotations:
 chains.tekton.dev/signed=true
 pipeline.tekton.dev/release=c802069

🌡️  Status

STARTED         DURATION    STATUS
2 minutes ago   28s         Succeeded

📝 Results

 NAME             VALUE
 ∙ IMAGE_DIGEST   sha256:05f95b26ed10668b7183c1e2da98610e91372fa9f510046d4ce5812addad86b5
 ∙ IMAGE_URL      gcr.io/foo/bar

🦶 Steps

 NAME             STATUS
 ∙ create-image   Completed

• Chains Controller Logs

INFO  16:30:15 Unable to read vcs.revision from binary
INFO  16:30:15 Profiling enabled: false
INFO  16:30:15 Running with Standard leader election
INFO  16:30:15 Starting configuration manager...
INFO  16:30:16 Starting informers...
INFO  16:30:16 Starting controllers...
INFO  16:30:16 Probes server listening on port 8080
INFO  16:30:16 watcher.github.com.tektoncd.chains.pkg.reconciler.taskrun.reconciler.00-of-01 will run in leader-elected mode with id "tekton-chains-controller-7496dbbf48-65szk_4883f100-ff04-404b-b839-980bcf877307"
INFO  16:30:16 watcher.github.com.tektoncd.chains.pkg.reconciler.pipelinerun.reconciler.00-of-01 will run in leader-elected mode with id "tekton-chains-controller-7496dbbf48-65szk_53b3a4f5-e4f9-4708-85d6-e25991caf5af"
INFO  16:30:16 Starting controller and workers
INFO  16:30:16 Started workers
INFO  16:30:16 Starting controller and workers
INFO  16:30:16 Started workers
I0707 16:30:16.106152       1 leaderelection.go:248] attempting to acquire leader lease tekton-pipelines/watcher.github.com.tektoncd.chains.pkg.reconciler.taskrun.reconciler.00-of-01...
I0707 16:30:16.106152       1 leaderelection.go:248] attempting to acquire leader lease tekton-pipelines/watcher.github.com.tektoncd.chains.pkg.reconciler.pipelinerun.reconciler.00-of-01...
I0707 16:31:24.126499       1 leaderelection.go:258] successfully acquired lease tekton-pipelines/watcher.github.com.tektoncd.chains.pkg.reconciler.taskrun.reconciler.00-of-01
INFO  16:31:24 "tekton-chains-controller-7496dbbf48-65szk_4883f100-ff04-404b-b839-980bcf877307" has started leading "watcher.github.com.tektoncd.chains.pkg.reconciler.taskrun.reconciler.00-of-01"
INFO  16:31:24 Event(v1.ObjectReference{Kind:"TaskRun", Namespace:"default", Name:"build-push-run-output-image-test", UID:"33fa4c9f-b73b-4f16-88d6-37a4752b4ba1", APIVersion:"tekton.dev/v1beta1", ResourceVersion:"2993", FieldPath:""}): type: 'Normal' reason: 'FinalizerUpdate' Updated "build-push-run-output-image-test" finalizers
WARN  16:31:24 error configuring x509 signer: no valid private key found, looked for: [x509.pem, cosign.key]
INFO  16:31:24 Created payload of type in-toto for tekton.dev/v1beta1/TaskRun default/build-push-run-output-image-test
WARN  16:31:24 No signer x509 configured for tekton
INFO  16:31:24 Created payload of type simplesigning for tekton.dev/v1beta1/TaskRun default/build-push-run-output-image-test
WARN  16:31:24 No signer x509 configured for oci
INFO  16:31:24 Reconcile succeeded
INFO  16:31:24 taskrun default/build-push-run-output-image-test has been reconciled
INFO  16:31:24 Reconcile succeeded
I0707 16:31:26.991194       1 leaderelection.go:258] successfully acquired lease tekton-pipelines/watcher.github.com.tektoncd.chains.pkg.reconciler.pipelinerun.reconciler.00-of-01
INFO  16:31:26 "tekton-chains-controller-7496dbbf48-65szk_53b3a4f5-e4f9-4708-85d6-e25991caf5af" has started leading "watcher.github.com.tektoncd.chains.pkg.reconciler.pipelinerun.reconciler.00-of-01"

[lcarva]

Hi @PuneetPunamiya! Thanks for reporting this.

An empty config value means the default value is loaded. The config values are documented here. The default configuration does require that the signing-secret is created which appears to be missing in your case: error configuring x509 signer: no valid private key found, looked for: [x509.pem, cosign.key]

Chains uses the mentioned annotation to indicate that it is done processing the resource. So if it sees a resource with that annotation, then it just ignores it. Otherwise, it may constantly keep retrying. signed is probably not the best name for this though.

I think the question here is how should Chains behave if it is misconfigured. Should it stay out of the way, or should it make this obvious to users creating TaskRun and PipelineRun resources? Or maybe, the Chains controller should have some sort of config validation at start up, making it obvious to whoever is administrating the controller.

[PuneetPunamiya]

Thanks @lcarva for the response 👍🏻

An empty config value means the default value is loaded.

Yep agreed, I think in that case we should add those fields in the config map, so that user is aware which config values are set by default. wdyt ?

The default configuration does require that the signing-secret is created which appears to be missing in your case: error configuring x509 signer: no valid private key found, looked for: [x509.pem, cosign.key]

So if I'm not getting you wrong, we do have signing-secret created once chains is installed, but I think what you meant is the signing-secret was not updated/configured with the cosign cli and that is why it gives the following error. Also once we update the keys by executing cosign generate .. command, the signing fails 👍🏻

I think the question here is how should Chains behave if it is misconfigured.

Yes exactly, I purposely added no config values from whic are documented here assuming the signing should fail

the Chains controller should have some sort of config validation at start up, making it obvious to whoever is administrating the controller.

Yeah, so if user doesn't provide any config values, validation returns a warning sort of and doesn't reconcile to take further actions. wdyt ?

[lcarva]

Yep agreed, I think in that case we should add those fields in the config map, so that user is aware which config values are set by default. wdyt ?

Do you mean here? If so, +1.

Yeah, so if user doesn't provide any config values, validation returns a warning sort of and doesn't reconcile to take further actions. wdyt ?

A warning may be easy to miss. Is preventing the controller from starting (by crashing it?) too heavy handed?

[PuneetPunamiya]

Yep agreed, I think in that case we should add those fields in the config map, so that user is aware which config values are set by default. wdyt ?

Do you mean here? If so, +1.

Yeah, I meant here only

Yeah, so if user doesn't provide any config values, validation returns a warning sort of and doesn't reconcile to take further actions. wdyt ?

A warning may be easy to miss. Is preventing the controller from starting (by crashing it?) too heavy handed?

How about erroring out in the controller logs and controller not proceeding further steps instead of crashing it ?

[concaf]

An empty config value means the default value is loaded. The config values are documented here. The default configuration does require that the signing-secret is created which appears to be missing in your case: error configuring x509 signer: no valid private key found, looked for: [x509.pem, cosign.key]

Chains uses the mentioned annotation to indicate that it is done processing the resource. So if it sees a resource with that annotation, then it just ignores it. Otherwise, it may constantly keep retrying. signed is probably not the best name for this though.

@lcarva i agree the signed is not the best name to indicate the processing of that resource; like you said, to a user chains.tekton.dev/signed=true indicates that chains was able to successfully sign the resource, hence this is very misleading.

what's the downside of setting chains.tekton.dev/signed=false/failed in this case?

[lcarva]

what's the downside of setting chains.tekton.dev/signed=false/failed in this case?

Setting it to "failed" is probably the right behavior. It will be challenging to get it right as we need to distinguish from use cases where there's just nothing to sign, e.g. #458.

cc @wlynch, @chitrangpatel

[concaf]

wdyt about specifying a list of values that accurately describe the state? e.g.:

• chains.tekton.dev/signed=true
• chains.tekton.dev/signed=failed
• chains.tekton.dev/signed=skipped

[concaf]

while i agree that chains.tekton.dev/signed should have multiple states as above ^ , but also in the absence of any keys (which means chains has nothing to sign with), the controller should not watch / process any taskruns/pipelineruns at all - why waste resources in going through each workload and add chains.tekton.dev/signed=skipped annotation

one data point to support this is #979 where we're already seeing slowness from chains controller due to needing to process a large number of tekton workloads - while that issue will be fixed differently, the chains controller should try to not watch workloads if not required, wdyt?

[lcarva]

I agree with @concaf here. Another way to look at it is the different personas that rely on Chains.

There's the user who is running Pipelines/Tasks, and the admin who is responsible for configuring Tekton Chains. In some cases, these may be the same person, but think we should assume that they are separate people with different access levels.

If Chains is misconfigured, there's nothing the user can do. The admin is the one responsible. With this premise, if Chains is misconfigured, we should make that obvious to the admin, while also minimizing the impact to the user.

This is why I suggested earlier on that if Chains is misconfigured, it should just crash. This should effectively allow the admin to fix things and delay Chains from processing any *Run resources. Once the issue is resolved, Chains will catch up processing the *Run resources.

[lcarva]

We discussed this in today's meeting. The overall consensus is to not proceed with the chains.tekton.dev/signed=skipped annotation approach. One of the promises of Chains is that it processes every resource in the cluster (if Chains is properly configured). This would violate that.

An approach that makes it obvious the controller is misconfigured is ideal, e.g. performing a basic config check at start up and crash if anything seems wrong (missing cosign key for example). This won't catch all the cases, but it's a starting point.

Additionally, we could create a mechanism that propagates errors back to the user. This could be done by using a new annotation or by adding a new status condition on the resource. This should bridge the gap of issues which are within the user's control to fix, e.g. malformatted linked secret.