Chains creates attestation for non-successful TaskRuns

[lcarva]

Expected Behavior

The Tekton Chains controller should only create TaskRun attestations if the TaskRun succeeds.

Actual Behavior

The controller creates TaskRun attestations regardless of the final state of the TaskRun.

Steps to Reproduce the Problem

It's easier to verify this behavior when the transparency log is enabled. The steps below assume this has been done.

1. Create a task, for example git-clone from the catalog:

curl 'https://raw.githubusercontent.com/tektoncd/catalog/main/task/git-clone/0.7/git-clone.yaml' | kubectl create -f -

2. Create a taskrun from the task. Provide incorrect parameter values to cause it to fail. For example provide a revision value that does not exist:

tkn task start git-clone --param url=https://github.com/tektoncd/chains --param revision=yolo --use-param-defaults --workspace name=output,emptyDir= --showlog

3. Verify the taskrun fails
4. Verify an attestation for the taskrun was created by verifying the taskrun contains a valid value for the annotation "chains.tekton.dev/transparency". This should not happen.

Additional Info

• Kubernetes version:

$ kubectl version --short
Flag --short has been deprecated, and will be removed in the future. The --short output will become the default.
Client Version: v1.24.2
Kustomize Version: v4.5.4
Server Version: v1.23.5+3afdacb

• Tekton Pipeline version:

$ tkn version
Client version: 0.21.0
Pipeline version: v0.33.2
Triggers version: v0.19.0

[wlynch]

This might be a feature?
I'm imagining a case where an entire release pipeline is completed except for some non-critical last step (i.e. notify slack, etc.) - I think we would still want provenance information generated so we can trace back metadata for any pushed artifacts.

For the same reason I don't think it's a bad idea to generate attestations for failed TaskRuns, since an attestation is just a signed statement saying that something happened.

Let me know what you think! Do have a case in mind where publishing attestations for failed TaskRun might be problematic?

[lcarva]

In the case of a non-successful TaskRun, its attestation will always in include a null value for the subject. This is because a non-successful TaskRun produces no results, thus Chains cannot use type-hinting to identify if an image was built. I think that reduces some of the value from the attestation since we can't map it back to an image, for example.

The current TaskRun attestation also has no indication that the task actually failed (at least I can't tell). Here's an example from a failed git clone task:

{
  "_type": "https://in-toto.io/Statement/v0.1",
  "predicateType": "https://slsa.dev/provenance/v0.2",
  "subject": null,
  "predicate": {
    "builder": {
      "id": "https://tekton.dev/chains/v2"
    },
    "buildType": "https://tekton.dev/attestations/chains@v2",
    "invocation": {
      "configSource": {},
      "parameters": {
        "deleteExisting": "\"true\"",
        "depth": "\"1\"",
        "gitInitImage": "\"gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/git-init:v0.29.0\"",
        "httpProxy": "\"\"",
        "httpsProxy": "\"\"",
        "noProxy": "\"\"",
        "refspec": "\"\"",
        "revision": "\"maine\"",
        "sparseCheckoutDirectories": "\"\"",
        "sslVerify": "\"true\"",
        "subdirectory": "\"\"",
        "submodules": "\"true\"",
        "url": "\"https://github.com/lcarva/minimal-container\"",
        "userHome": "\"/tekton/home\"",
        "verbose": "\"true\""
      }
    },
    "buildConfig": {
      "steps": [
        {
          "entryPoint": "#!/usr/bin/env sh\nset -eu\n\nif [ \"${PARAM_VERBOSE}\" = \"true\" ] ; then\n  set -x\nfi\n\n\nif [ \"${WORKSPACE_BASIC_AUTH_DIRECTORY_BOUND}\" = \"true\" ] ; then\n  cp \"${WORKSPACE_BASIC_AUTH_DIRECTORY_PATH}/.git-credentials\" \"${PARAM_USER_HOME}/.git-credentials\"\n  cp \"${WORKSPACE_BASIC_AUTH_DIRECTORY_PATH}/.gitconfig\" \"${PARAM_USER_HOME}/.gitconfig\"\n  chmod 400 \"${PARAM_USER_HOME}/.git-credentials\"\n  chmod 400 \"${PARAM_USER_HOME}/.gitconfig\"\nfi\n\nif [ \"${WORKSPACE_SSH_DIRECTORY_BOUND}\" = \"true\" ] ; then\n  cp -R \"${WORKSPACE_SSH_DIRECTORY_PATH}\" \"${PARAM_USER_HOME}\"/.ssh\n  chmod 700 \"${PARAM_USER_HOME}\"/.ssh\n  chmod -R 400 \"${PARAM_USER_HOME}\"/.ssh/*\nfi\n\nif [ \"${WORKSPACE_SSL_CA_DIRECTORY_BOUND}\" = \"true\" ] ; then\n   export GIT_SSL_CAPATH=\"${WORKSPACE_SSL_CA_DIRECTORY_PATH}\"\nfi\nCHECKOUT_DIR=\"${WORKSPACE_OUTPUT_PATH}/${PARAM_SUBDIRECTORY}\"\n\ncleandir() {\n  # Delete any existing contents of the repo directory if it exists.\n  #\n  # We don't just \"rm -rf ${CHECKOUT_DIR}\" because ${CHECKOUT_DIR} might be \"/\"\n  # or the root of a mounted volume.\n  if [ -d \"${CHECKOUT_DIR}\" ] ; then\n    # Delete non-hidden files and directories\n    rm -rf \"${CHECKOUT_DIR:?}\"/*\n    # Delete files and directories starting with . but excluding ..\n    rm -rf \"${CHECKOUT_DIR}\"/.[!.]*\n    # Delete files and directories starting with .. plus any other character\n    rm -rf \"${CHECKOUT_DIR}\"/..?*\n  fi\n}\n\nif [ \"${PARAM_DELETE_EXISTING}\" = \"true\" ] ; then\n  cleandir\nfi\n\ntest -z \"${PARAM_HTTP_PROXY}\" || export HTTP_PROXY=\"${PARAM_HTTP_PROXY}\"\ntest -z \"${PARAM_HTTPS_PROXY}\" || export HTTPS_PROXY=\"${PARAM_HTTPS_PROXY}\"\ntest -z \"${PARAM_NO_PROXY}\" || export NO_PROXY=\"${PARAM_NO_PROXY}\"\n\n/ko-app/git-init \\\n  -url=\"${PARAM_URL}\" \\\n  -revision=\"${PARAM_REVISION}\" \\\n  -refspec=\"${PARAM_REFSPEC}\" \\\n  -path=\"${CHECKOUT_DIR}\" \\\n  -sslVerify=\"${PARAM_SSL_VERIFY}\" \\\n  -submodules=\"${PARAM_SUBMODULES}\" \\\n  -depth=\"${PARAM_DEPTH}\" \\\n  -sparseCheckoutDirectories=\"${PARAM_SPARSE_CHECKOUT_DIRECTORIES}\"\ncd \"${CHECKOUT_DIR}\"\nRESULT_SHA=\"$(git rev-parse HEAD)\"\nEXIT_CODE=\"$?\"\nif [ \"${EXIT_CODE}\" != 0 ] ; then\n  exit \"${EXIT_CODE}\"\nfi\nprintf \"%s\" \"${RESULT_SHA}\" > \"$(results.commit.path)\"\nprintf \"%s\" \"${PARAM_URL}\" > \"$(results.url.path)\"\n",
          "arguments": null,
          "environment": {
            "container": "clone",
            "image": "gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/git-init@sha256:45dca0972541546d3625d99ee8a8fbcc768b01fc9c199d1251ebd7dfd1b8874c"
          },
          "annotations": null
        }
      ]
    },
    "metadata": {
      "buildStartedOn": "2022-07-06T17:38:48Z",
      "buildFinishedOn": "2022-07-06T17:38:57Z",
      "completeness": {
        "parameters": false,
        "environment": false,
        "materials": false
      },
      "reproducible": false
    }
  }
}

With TEP-84, an attestation for a non-successful PipelineRun might be of some use, since it would cover the non-criticial step scenario you explained.

[priyawadhwa]

So the current expectation is that only Successful TaskRuns would go through the Chains workflow. I thought we take care of that here --

chains/pkg/reconciler/taskrun/taskrun.go

Lines 51 to 54 in a86f18b

	if !tr.IsDone() {
	logging.FromContext(ctx).Infof("taskrun %s/%s is still running", tr.Namespace, tr.Name)
	return nil
	}

but if it's not working then this is currently a bug! As @wlynch mentioned, there might be use cases for generating attestations for failed runs but we haven't really considered that as of yet 😄

[wlynch]

Some of this might be wrapped up in tektoncd/pipeline#3749 (ensuring results are published from failed Runs).

I guess a related question here is is it worth publishing attestations with no subjects? And if we consider the TaskRun itself to be an artifact, should this be listed as a subject? 🤔

[ywluogg]

This is because a non-successful TaskRun produces no results, thus Chains cannot use type-hinting to identify if an image was built.

This makes me wonder if we can use another field we can generate not at the end of a Run. This would be similar to the discussion about adding a new field to Run status to capture input/output artifacts to make this more discoverable / extensible by chains

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[tekton-robot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

[tekton-robot]

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vdemeester]

/reopen
I don't think anything was decided on this yet.