Restrict automatic runs of PR jobs #129

bensze01 · 2023-10-05T18:46:40Z

Only run PR job if one of the following is true:

The author of the PR has write access to the repo the PR is targeting
The job is triggered by the Github merge queue (thus the PR has been reviewed by a gatekeeper)
The job was started manually from the Jenkins UI

Test runs:

Push to a PR authored by a non-member (PR-merge):
- Internal CI
- OpenCI
Manual run of a PR by a non-member (PR-merge):
- Internal CI
- OpenCI
Manual run of a PR with an expected failure (PR-head):
- Internal CI
Push to a PR authored by a team-member (PR-head).
- Internal CI
- OpenCI

Only run PR job if one of the following is true: - The author of the PR is a member of a trusted Github team - The job is triggered by the Github merge queue (thus the PR has been reviewed by a gatekeeper) - The job was started manually from the Jenkins UI Signed-off-by: Bence Szépkúti <[email protected]>

gilles-peskine-arm

I'd like a couple of clarifications, and I want to see some test runs, but the approach looks right to me.

gilles-peskine-arm · 2023-10-06T07:42:34Z

vars/common.groovy

+
+@NonCPS
+boolean is_pr_author_member_of_team(String repo, int pr) {
+    String credentials = is_open_ci_env ? 'mbedtls-github-token' : 'd015f9b1-4800-4a81-86b3-9dbadc18ee00'


Please explain the UUID.

On the internal CI we don't have user-friendly names for our credential IDs, only UUIDs - we should probably ask the Infra team to rename the credentials and adopt the names the OpenCI uses at some point so we can get rid of all similar code fragments.

gilles-peskine-arm · 2023-10-06T08:02:36Z

vars/mbedtls.groovy

+        environ.set_tls_pr_environment(is_production)
+        boolean is_merge_queue = env.BRANCH_NAME ==~ /gh-readonly-queue\/.*/
+
+        if (!is_merge_queue && currentBuild.rawBuild.getCause(Cause.UserIdCause) == null) {


Where can I find documentation that explains getCause(Cause.UserIdCause)?

The documentation of Cause.UserIdCause states

A build is started by an user action.

It doesn't explain what a user action is, and the documentation of getCause doesn't explain when what that function returns (and in particular when it returns null).

From what you've written, I guess that:

A user action is something that happens on Jenkins. Only team members have access to jenkins, so if getCause(Cause.UserIdCause) returns a non-null value, it means someone with a Jenkins account explicitly triggered the build.

The automation that triggers builds from GitHub events is not a Jenkins user.

getCause returns null if there is no Jenkins user associated with the build trigger.

How can I validate these guesses?

getCause iterates over all causes of a build, and returns a cause that is an instance of type - if no such cause is found, it returns null.

The following are all the causes produced by the various triggers used on our CI:

Cause.UserIdCause - used for builds started through the web UI or the REST api using a user's personal access token

ReplayCause - used in conjunction with Cause.UserIdCause for replayed builds

RebuildCause - used on the OpenCI in conjuction with Cause.UserIdCause and all the causes of the original build (!) if you click the other, "triangular" rebuild button on their web UI, (this one picks up the most recent CI scripts, instead of "replaying" like the other button does. Unfortunately both are labelled "Rebuild" on the OpenCI.)

BranchIndexingCause - used for builds stared while scanning a repository, eg. the nightly scan of the repo for PR-merge jobs

BranchEventCause - used for builds started by a github webhook

TimerTrigger.TimerTriggerCause - used for builds started by a plain timer

ParameterizedTimerTriggerCause - used for builds started by a parameterized timer

On the internal CI, these causes each have their own pictograms that you can hover over for a short description of the build cause on the page listing all the runs of a single branch.

On both CIs, you can get a list of all causes of a build, including their full qualified classnames by appending api/json?pretty=true to the classic web UI's build page.

Examples for each build cause:

Cause.UserIdCause

ReplayCause + Cause.UserIdCause

RebuildCause + Cause.UserIdCause +BranchEventCause copied from the original build

BranchIndexingCause

BranchEventCause

TimerTrigger.TimerTriggerCause

ParameterizedTimerTriggerCause

gilles-peskine-arm · 2023-10-06T08:11:42Z

vars/common.groovy

+    String credentials = is_open_ci_env ? 'mbedtls-github-token' : 'd015f9b1-4800-4a81-86b3-9dbadc18ee00'
+    def github = Connector.connect(null, Connector.lookupScanCredentials(currentBuild.rawBuild.parent, null, credentials))
+    def author = github.getRepository(repo).getPullRequest(pr).user
+    return github.getOrganization('Mbed-TLS').getTeamBySlug('mbed-tls-reviewers').hasMember(author)


As discussed in chat, we should be more robust here and also allow mbed-tls-developers (without depending on how the lists may be nested on GitHub).

Alternatively, does the GitHub API let us test “does the user have write access on the repository?”? That would be an acceptable condition here.

Both are possible - I went with checking for write access, because that approach makes half as many calls to the Github REST api.

Signed-off-by: Bence Szépkúti <[email protected]>

gilles-peskine-arm

Looks good to me on code reading. Though I'd like to see some test jobs before this is merged.

paul-elliott-arm

Code looks good to me, but test runs still required before full approval.

bensze01 · 2023-10-09T17:04:36Z

@gilles-peskine-arm @paul-elliott-arm I've updated the PR descriptions with the test runs I've run so far.
Is there anything else I didn't cover that you would like to see tested before we merge this?

gilles-peskine-arm · 2023-10-09T17:17:01Z

I started a test run with an expected failure, to make sure we haven't somehow broken the error reporting path. If that works as expected (as well as the pass-expected test runs that are still running), then I'm happy with the testing.

gilles-peskine-arm · 2023-10-09T20:50:15Z

I've looked at the CI results and they look fine to me. Ok to merge if Paul agrees.

paul-elliott-arm · 2023-10-10T11:18:56Z

Agreed, everything looks good. Merging.

bensze01 added needs: ci needs: review needs: reviewer bug Something isn't working priority-high security size-s Estimated task size: small (~2d) labels Oct 5, 2023

bensze01 force-pushed the dev/bensze01/auto-build-allowlist branch from 746e532 to f9b30a7 Compare October 5, 2023 19:18

bensze01 force-pushed the dev/bensze01/auto-build-allowlist branch from f9b30a7 to d59acb6 Compare October 5, 2023 19:56

gilles-peskine-arm requested changes Oct 6, 2023

View reviewed changes

bensze01 force-pushed the dev/bensze01/auto-build-allowlist branch 2 times, most recently from bdd5c2d to 674f6ef Compare October 6, 2023 17:08

Check for write access instead of team membership

7c69a2c

Signed-off-by: Bence Szépkúti <[email protected]>

bensze01 force-pushed the dev/bensze01/auto-build-allowlist branch from 674f6ef to 7c69a2c Compare October 6, 2023 17:18

gilles-peskine-arm approved these changes Oct 7, 2023

View reviewed changes

paul-elliott-arm removed the needs: reviewer label Oct 9, 2023

paul-elliott-arm approved these changes Oct 9, 2023

View reviewed changes

bensze01 added approved Approved in review. May need additional CI. and removed needs: review labels Oct 9, 2023

paul-elliott-arm merged commit a006309 into master Oct 10, 2023
1 check passed

This was referenced Oct 11, 2023

Restore logic for tests skipped in merge queue #131

Merged

CI hardening project #132

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict automatic runs of PR jobs #129

Restrict automatic runs of PR jobs #129

bensze01 commented Oct 5, 2023 •

edited by gilles-peskine-arm

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm Oct 6, 2023

bensze01 Oct 6, 2023

gilles-peskine-arm Oct 6, 2023

bensze01 Oct 6, 2023 •

edited

Loading

gilles-peskine-arm Oct 6, 2023

bensze01 Oct 6, 2023

gilles-peskine-arm left a comment

paul-elliott-arm left a comment

bensze01 commented Oct 9, 2023

gilles-peskine-arm commented Oct 9, 2023

gilles-peskine-arm commented Oct 9, 2023

paul-elliott-arm commented Oct 10, 2023

Restrict automatic runs of PR jobs #129

Restrict automatic runs of PR jobs #129

Conversation

bensze01 commented Oct 5, 2023 • edited by gilles-peskine-arm Loading

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Oct 6, 2023

Choose a reason for hiding this comment

bensze01 Oct 6, 2023

Choose a reason for hiding this comment

gilles-peskine-arm Oct 6, 2023

Choose a reason for hiding this comment

bensze01 Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

gilles-peskine-arm Oct 6, 2023

Choose a reason for hiding this comment

bensze01 Oct 6, 2023

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

paul-elliott-arm left a comment

Choose a reason for hiding this comment

bensze01 commented Oct 9, 2023

gilles-peskine-arm commented Oct 9, 2023

gilles-peskine-arm commented Oct 9, 2023

paul-elliott-arm commented Oct 10, 2023

bensze01 commented Oct 5, 2023 •

edited by gilles-peskine-arm

Loading

bensze01 Oct 6, 2023 •

edited

Loading