[JENKINS-46795] Abort builds with untrusted Jenkinsfile, but only given passive cause

[jglick]

JENKINS-46795 amending #69. Downstream of jenkinsci/scm-api-plugin#180.

Note that some much less common Jenkins features use getTrustedRevision and would not pick up this logic: https://github.com/jenkinsci/pipeline-groovy-lib-plugin/blob/b44ce045b582f60e5b76d4a9c4cbf52dfa4184de/src/main/java/org/jenkinsci/plugins/workflow/libs/MultibranchScmRevisionVerifier.java#L62 and https://github.com/jenkinsci/basic-branch-build-strategies-plugin/blob/a76ef8f7fcb9ee0c97b9f8408c1c11e0acd29154/src/main/java/jenkins/branch/buildstrategies/basic/ChangeRequestBuildStrategyImpl.java#L138

[jtnord]

as per the jira

It should override checkTrusted(GitHubSCMSourceRequest, PullRequestSCMRevision) to check the GitHub API to see if the current revision has been approved by a maintainer.

IIUC approval is at the PR level not the commit level.
so making this change would introduce a security issue for all existing setups where the user has not setup the github branch to dismiss approvals on push?
https://docs.github.com/en/rest/pulls/reviews?apiVersion=2022-11-28#get-a-review-for-a-pull-request is not clear on this subject.

If that is the case then this needs to be opt in and not the default behaviour.

looking at the output of hub api repos/jenkinsci/kubernetes-credentials-provider-plugin/pulls/67/reviews | jq . the commit id that was approved is available in the API, however it is unclear if this was the commit at the time of the submission of the review or what you had open in the web browser..
Additionally - wouldn;t this need some improvement in in github-branch source

(I am a bit confused - the PR looks ok - but does not as far as I can see seem to address the referenced Jira?

UPDATE: so this code does not "trust PR revisions when approved" which is what the Jira title is. That is confusing and should be changed (ideally this should be rebased to reference a new Jira to be more explicit) - as the desired Jira is still relevant.
What this does is not fail the build if the file is not trusted but only in a few special cases. (branch indexing, pushing, cron or scm trigger).

The previous code just logged a warning and continued - so nothing bad is introduced here, but I am not so sure about not trusting causes based on a denylist rather than an include list (esp give Jenkins' extensibility?).

[jglick]

this code does not "trust PR revisions when approved" which is what the Jira title is. That is confusing and should be changed

Right, the original proposal in the Jira description would have required changes to every branch source with a notion of an untrusted change request, and furthermore would have required expanded webhooks since Jenkins would need to listen to the PR being approved after it had already rejected the attempt to build the commit the first time, and would not anyway offer repository owners as much flexibility since they might wish to see CI results without necessarily approving the PR for final merge. (OTOH it would allow triggering CI without App authn or a Jenkins login.)

[jglick]

it is unclear if this was the commit at the time of the submission of the review or what you had open in the web browser.

In my experience it is the former—which is potentially unsafe.

[jglick]

not so sure about not trusting causes based on a denylist rather than an include list

Putting into draft while I investigate options.

[jglick]

Tested interactively:

• ran this PR hooked up to an App with webhook via ngrok
• set up a multibranch project (PR-head strategy) with default trust settings
• filed a PR as a user with no write permission editing Jenkinsfile ⇒ check failed as expected
• used Build Now from Jenkins web UI ⇒ passed (using edited Jenkinsfile)
• used Replay after that ⇒ passed (note that you cannot currently replay an aborted build because it never got to the point of loading a Jenkinsfile)
• edited PR as author ⇒ failed as expected
• used Re-run check from GH UI ⇒ passed (using edited Jenkinsfile)

[daniel-beck]

Since this relaxes security to support the use case of Jenkinsfile contributions from untrusted contributors, I strongly recommend this be accompanied by end-user documentation. Ideally there'd be a place in the UI for that, but I'm not sure where that would be, except perhaps wordier log messages when not trusting, to point out that manual re-builds would result in trust.

[daniel-beck]

@Restricted(NoExternalUse.class)?

[jglick]

I do not see why; it was not used elsewhere to begin with.

[daniel-beck]

Nvm, misread the diff since I didn't expect removal here.

[daniel-beck]

That said, removing this class might cause trouble with old build logs containing non-deserializable annotations? Or is this going somewhere else? Or have you tested it and it'll just silently drop them in build logs nobody cares about anyway given the early abort?

[jglick]

https://github.com/jenkinsci/jenkins/blob/aa83562568b44aa85ff17bdfab831bbe39c2fd15/core/src/main/java/hudson/console/ConsoleAnnotationOutputStream.java#L112-L125 should just drop the annotation, i.e. make the text be plain rather than yellow. Not specifically tested.

[jglick]

wordier log messages when not trusting

jenkinsci/scm-api-plugin@f6ec0d0 should help some. I agree that this sort of thing deserves more formal documentation, though. Can think about suitable spots in the GUI, and/or jenkins.io/doc.

[daniel-beck]

Given e.g. occasional infra hiccups necessitating rebuilds of projects for reasons outside the actual code and build script, would it make sense (or even be feasible) to limit elevating trust to situations when the previous build failed because of this lack of trust? Or would that overcomplicate things?

Thinking of cases when an admin notices an infra problem (potentially even their fault) and then checks recent builds, triggering rebuilds of any that had failed, to least inconvenience the developers. That would be something we'd need to discourage with the current implementation.

Perhaps we could make this at least explicit, e.g. with AlternativeUiTextProvider labeling the button as "Trust and Build Now" or so?

[jglick]

with AlternativeUiTextProvider labeling the button as "Trust and Build Now"

🤔 could be possible with a custom CauseOfInterruption if workflow-cps were made to look for that and offer a different version of Replay. In that case I suppose rather than trusting UserIdCause we would trust the new Cause added by that gesture (in addition to UserIdCause).

Integration of github-checks would remain as is: Re-run from Checks would be considered trusted regardless of original cause of failure.

[daniel-beck]

Here's an idea for re-building from the Jenkins UI: How about an Action attached to the aborted build, that rebuilds it with trust (e.g. a new cause, perhaps a UserIdCause subclass if it's non-final?), and it has separate UI? That way, you can look at the log and click a link or button right there in the "I'm rejecting this" message (not unlike the "Terminate all running steps" etc. messages), or have a new sidepanel link for the build (in addition to Build Now to minimize "rebuild all" risk), or a button on the main build page? This way, no need to place trust in the basic UserIdCause, and admins trying to be helpful are not a security risk.

Integration of github-checks would remain as is: Re-run from Checks would be considered trusted regardless of original cause of failure.

Does GitHub support multiple different "Re-run" gestures, with different behavior, and to what extent can the rejection of the Jenkinsfile be reported clearly to GH checks? Maybe this is clear (haven't gotten around to manually running this yet), so if the result reported to GH is unambiguous (e.g., marked as aborted, with log excerpt, etc.), this should be fine. The only potential issue I see would be in case of unclear results reported to the GH check ("Failed" status same as any failed build or infra hiccup, no or unclear log excerpt etc.), as it may be difficult to tell that a complex PR changing lots of files also changes the Jenkinsfile.

[jglick]

look at the log and click a link or button right there in the "I'm rejecting this" message

Yes, that would be helpful for users of the classic log display.

a new sidepanel link for the build

Right, basically the

different version of Replay

that I implied in #220 (comment).

Does GitHub support multiple different "Re-run" gestures, with different behavior

No. You can attach “actions” to a check but these have a completely different UX. IIRC you cannot even control whether Re-run is displayed; if a check fails, the link is shown, and if clicked, sends an event which the App can respond to or not.

to what extent can the rejection of the Jenkinsfile be reported clearly to GH checks?

It has to be displayed as a “failure” (unfortunately the “cancelled” conclusion does not offer Re-run), but the associated text can say whatever you want. Would require some hacking around in github-checks to specialize the display in this way, however, and currently it is not straightforward to make changes to that plugin because of its nonstandard POM (jenkinsci/github-checks-plugin#307).

[daniel-beck]

look at the log and click a link or button right there in the "I'm rejecting this" message

Yes, that would be helpful for users of the classic log display.

a new sidepanel link for the build

Right, basically the

different version of Replay

that I implied in #220 (comment).

👍

to what extent can the rejection of the Jenkinsfile be reported clearly to GH checks?

It has to be displayed as a “failure” (unfortunately the “cancelled” conclusion does not offer Re-run), but the associated text can say whatever you want. Would require some hacking around in github-checks to specialize the display in this way, however, and currently it is not straightforward to make changes to that plugin because of its nonstandard POM (jenkinsci/github-checks-plugin#307).

Would it be a viable step forward to proceed with support for Jenkins UI only for now, if the state of github-checks is a problem in making the integration with this failure nicer?

[jglick]

Would it be a viable step forward to proceed with support for Jenkins UI only

I do not think so. At least, it would make this feature useless for ci.jenkins.io, since most repository maintainers lack Job/Build permissions on the server.

if the state of github-checks is a problem in making the integration with this failure nicer

IMO just making Re-run be considered trusted, as is already done upstream, suffices. This would be unrelated to the concern you raised in #220 (comment).

[daniel-beck]

IMO just making Re-run be considered trusted, as is already done upstream, suffices. This would be unrelated to the concern you raised in #220 (comment).

Right. Re-run is less prone to admin accidents than Build Now, although complex PRs might still manage to trick reviewers into assuming flakiness when it's actually lack of trust.

I do not object to going ahead with trust in Re-Run, though IMO it would be very useful to be able to use this functionality from within Jenkins as well.