tektoncd / tektoncd/pipelines-as-code
watcher: git-auth secret AlreadyExists path reuses and adopts a secret it does not own
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 214
- Forks
- 144
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 27
Description
Spotted while chasing #2955, which turned out to be a logging defect. This one is real, though it stops at the namespace boundary.
When CreateSecret returns AlreadyExists, we reuse whatever secret already carries that name:
https://github.com/tektoncd/pipelines-as-code/blob/main/pkg/reconciler/reconciler.go#L306-L318
ee9da52a5 added that branch for a genuine case: slow etcd makes the API server retry a create for the same PipelineRun. But it matches on the name alone. It never checks that the existing secret belongs to this PipelineRun or this Repository.
We then call UpdateSecretWithOwnerRef on it, which replaces the owner references wholesale:
https://github.com/tektoncd/pipelines-as-code/blob/main/pkg/kubeinteraction/secrets.go#L52
The first consequence is a run bound to a secret it does not own. Secret names are pac-gitauth- plus 6 random letters, lowercased by GenerateBasicAuthSecretName, so the space is 26^6. On a collision inside a namespace, a PipelineRun for repo A proceeds with the token minted for repo B and the only signal is a warning that carries the wrong context (#2955).
The second is worse: PaC can be talked into adopting an arbitrary secret. The reconciler acts on annotations that sit on the PipelineRun, so anyone who can create a PipelineRun in a namespace can write:
metadata:
annotations:
pipelinesascode.tekton.dev/repository: <an existing Repository CR in that namespace>
pipelinesascode.tekton.dev/git-auth-secret: <any existing secret in that namespace>
pipelinesascode.tekton.dev/secret-created: "false"
CreateSecret fails with AlreadyExists, the reuse branch tolerates it, and UpdateSecretWithOwnerRef stamps that unrelated secret with an owner reference to the attacker's PipelineRun. Delete the PipelineRun and the secret gets garbage collected with it. CleanupPipelines deletes by the same annotation value:
https://github.com/tektoncd/pipelines-as-code/blob/main/pkg/kubeinteraction/cleanups.go#L52-L59
It needs create-PipelineRun rights in the target namespace, so no cross-tenant escape. Still a destructive primitive we hand out for free, and it applies to the PaC install namespace too wherever users can create PipelineRuns there.
Suggested fix
Reuse the existing secret only when it provably belongs to this PipelineRun or Repository: owner UID matching the PipelineRun, or matching pipelinesascode.tekton.dev/url-org, url-repository and sha annotations. That covers the etcd-retry case the branch was written for and rejects the rest, which should then fail closed with a Warning Event on the Repository CR instead of running with someone else's credential.
UpdateSecretWithOwnerRef should also stop flattening OwnerReferences, and refuse a secret already owned by another object.
While in there: GenerateBasicAuthSecretName lowercases the generated string, dropping the space from 52^6 to 26^6 for nothing.
Goes back at least to v0.48.1 (reconciler.go:226-238 and kubeinteraction/secrets.go:51 there).
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Read the CreateSecret AlreadyExists branch in pkg/reconciler/reconciler.go and the ownership logic in pkg/kubeinteraction/secrets.go, then inspect cleanup handling in pkg/kubeinteraction/cleanups.go. Verify the reuse path accepts only a matching PipelineRun or Repository, refuses secrets owned by another object, preserves ownership safely, and fails closed with the described warning behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- backend, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100