Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1d80e65
Revert "Downgrading to a warning at @abayer’s request."
jglick Dec 14, 2022
84b1852
[JENKINS-46795] Abort builds with untrusted `Jenkinsfile`, but only g…
jglick Dec 14, 2022
9f63e0d
Use `NOT_BUILT` to emphasize that there need not have been anything w…
jglick Dec 14, 2022
18291d7
Merge branch 'master' into trust-JENKINS-46795
jtnord Dec 15, 2022
e0cc54b
Pick up https://github.com/jenkinsci/scm-api-plugin/pull/180
jglick Dec 15, 2022
3e58055
bump
jglick Dec 21, 2022
7b5acb2
Making `SCMBinderTest` faster & clearer by replacing `waitUntilNoActi…
jglick Dec 22, 2022
2c7bebf
Testing https://github.com/jenkinsci/scm-api-plugin/pull/180/commits/…
jglick Dec 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ THE SOFTWARE.
<scope>import</scope>
<type>pom</type>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>scm-api</artifactId>
<version>636.veb_e3d6f5b_820</version> <!-- TODO https://github.com/jenkinsci/scm-api-plugin/pull/180 -->
</dependency>
</dependencies>
</dependencyManagement>
<dependencies>
Expand Down Expand Up @@ -212,6 +217,12 @@ THE SOFTWARE.
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.awaitility</groupId>
<artifactId>awaitility</artifactId>
<version>4.2.0</version>
<scope>test</scope>
</dependency>
</dependencies>
<build>
<plugins>
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jenkinsci/plugins/workflow/multibranch/ReadTrustedStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ public static class Execution extends AbstractSynchronousNonBlockingStepExecutio
}
build.addAction(new SCMRevisionAction(scmSource, tip));
}
SCMRevision trusted = scmSource.getTrustedRevision(tip, listener);
SCMRevision trusted = scmSource.getTrustedRevisionForBuild(tip, listener, build);
boolean trustCheck = !tip.equals(trusted);
String untrustedFile = null;
String content;
Expand Down
46 changes: 15 additions & 31 deletions src/main/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,15 @@
package org.jenkinsci.plugins.workflow.multibranch;

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.AbortException;
import hudson.Extension;
import hudson.Functions;
import hudson.MarkupText;
import hudson.console.ConsoleAnnotationDescriptor;
import hudson.console.ConsoleAnnotator;
import hudson.console.ConsoleNote;
import hudson.model.Action;
import hudson.model.Descriptor;
import hudson.model.DescriptorVisibilityFilter;
import hudson.model.ItemGroup;
import hudson.model.Queue;
import hudson.model.Result;
import hudson.model.TaskListener;
import hudson.scm.SCM;
import java.io.IOException;
Expand All @@ -46,6 +44,7 @@
import jenkins.scm.api.SCMRevision;
import jenkins.scm.api.SCMRevisionAction;
import jenkins.scm.api.SCMSource;
import jenkins.util.SystemProperties;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.cps.CpsScmFlowDefinition;
import org.jenkinsci.plugins.workflow.flow.FlowDefinition;
Expand All @@ -61,7 +60,11 @@
class SCMBinder extends FlowDefinition {

/** Kill switch for JENKINS-33273 in case of problems. */
static /* not final */ boolean USE_HEAVYWEIGHT_CHECKOUT = Boolean.getBoolean(SCMBinder.class.getName() + ".USE_HEAVYWEIGHT_CHECKOUT"); // TODO 2.4+ use SystemProperties
static /* not final */ boolean USE_HEAVYWEIGHT_CHECKOUT = SystemProperties.getBoolean(SCMBinder.class.getName() + ".USE_HEAVYWEIGHT_CHECKOUT");

/** Kill switch for making this as strict as {@link ReadTrustedStep} about untrusted modifications. */
static /* not final */ boolean IGNORE_UNTRUSTED_EDITS = SystemProperties.getBoolean(SCMBinder.class.getName() + ".IGNORE_UNTRUSTED_EDITS");

private String scriptPath = WorkflowBranchProjectFactory.SCRIPT;

public Object readResolve() {
Expand Down Expand Up @@ -100,7 +103,7 @@ public SCMBinder(String scriptPath) {
SCM scm;
if (tip != null) {
build.addAction(new SCMRevisionAction(scmSource, tip));
SCMRevision rev = scmSource.getTrustedRevision(tip, listener);
SCMRevision rev = scmSource.getTrustedRevisionForBuild(tip, listener, build);
try (SCMFileSystem fs = USE_HEAVYWEIGHT_CHECKOUT ? null : SCMFileSystem.of(scmSource, head, rev)) {
if (fs != null) { // JENKINS-33273
String script = null;
Expand All @@ -111,10 +114,10 @@ public SCMBinder(String scriptPath) {
listener.error("Could not do lightweight checkout, falling back to heavyweight").println(Functions.printThrowable(x).trim());
}
if (script != null) {
if (!rev.equals(tip)) {
// Print a warning in builds where an untrusted contributor has tried to edit Jenkinsfile.
// If we fail to check this (e.g., due to heavyweight checkout), a warning will still be printed to the log
// by the SCM, but that is less apparent.
if (!IGNORE_UNTRUSTED_EDITS && !rev.equals(tip)) {
// Make a best effort to abort builds where an untrusted contributor has tried to edit Jenkinsfile.
// If we fail to check this (e.g., due to heavyweight checkout), a warning will be printed to the log
// and the build will continue with the trusted variant, which is safe but confusing.
SCMFileSystem tipFS = SCMFileSystem.of(scmSource, head, tip);
if (tipFS != null) {
String tipScript = null;
Expand All @@ -124,9 +127,8 @@ public SCMBinder(String scriptPath) {
listener.error("Could not compare lightweight checkout of trusted revision").println(Functions.printThrowable(x).trim());
}
if (tipScript != null && !script.equals(tipScript)) {
listener.annotate(new WarningNote());
listener.getLogger().println(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis(scriptPath));
// TODO JENKINS-45970 consider aborting instead, at least optionally
build.setResult(Result.NOT_BUILT);
throw new AbortException(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis(scriptPath));
}
}
}
Expand Down Expand Up @@ -165,22 +167,4 @@ public SCMBinder(String scriptPath) {

}

// TODO seems there is no general-purpose ConsoleNote which simply wraps markup in specified HTML
@SuppressWarnings("rawtypes")
public static class WarningNote extends ConsoleNote {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Restricted(NoExternalUse.class)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@daniel-beck daniel-beck Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@Override public ConsoleAnnotator annotate(Object context, MarkupText text, int charPos) {
text.addMarkup(0, text.length(), "<span class='warning-inline'>", "</span>");
return null;
}

@Extension public static final class DescriptorImpl extends ConsoleAnnotationDescriptor {
@NonNull
@Override public String getDisplayName() {
return "Multibranch warnings";
}
}

}

}
79 changes: 62 additions & 17 deletions src/test/java/org/jenkinsci/plugins/workflow/multibranch/SCMBinderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,15 @@
import com.cloudbees.hudson.plugins.folder.computed.DefaultOrphanedItemStrategy;
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Util;
import hudson.model.Cause;
import hudson.model.CauseAction;
import hudson.model.Item;
import hudson.model.Result;
import hudson.model.TaskListener;
import hudson.model.User;
import hudson.plugins.git.util.BuildData;
import hudson.scm.ChangeLogSet;
import hudson.security.ACL;
import java.io.File;
import java.io.IOException;
import java.util.HashSet;
Expand All @@ -55,17 +58,20 @@

import jenkins.scm.impl.subversion.SubversionSampleRepoRule;
import org.acegisecurity.Authentication;
import static org.awaitility.Awaitility.await;
import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
import org.junit.Test;
import static org.junit.Assert.*;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Rule;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.MockAuthorizationStrategy;

public class SCMBinderTest {

Expand All @@ -74,6 +80,10 @@ public class SCMBinderTest {
@Rule public GitSampleRepoRule sampleGitRepo = new GitSampleRepoRule();
@Rule public SubversionSampleRepoRule sampleSvnRepo = new SubversionSampleRepoRule();

@Before public void runImmediately() throws Exception {
r.jenkins.setQuietPeriod(0);
}

@Test public void exactRevisionGit() throws Exception {
sampleGitRepo.init();
ScriptApproval sa = ScriptApproval.get();
Expand Down Expand Up @@ -182,9 +192,7 @@ public static void assertRevisionAction(WorkflowRun build) {
mp.getSourcesList().add(new BranchSource(new GitSCMSource(null, sampleGitRepo.toString(), "", "*", "", false)));
WorkflowJob p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, "master");
assertEquals(1, mp.getItems().size());
r.waitUntilNoActivity();
WorkflowRun b1 = p.getLastBuild();
assertEquals(1, b1.getNumber());
WorkflowRun b1 = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 1);
sampleGitRepo.git("rm", "Jenkinsfile");
sampleGitRepo.git("commit", "--all", "--message=remove");
WorkflowRun b2 = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get());
Expand All @@ -209,9 +217,7 @@ public static void assertRevisionAction(WorkflowRun build) {
mp.setOrphanedItemStrategy(new DefaultOrphanedItemStrategy(false, "", ""));
WorkflowJob p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, "feature");
assertEquals(2, mp.getItems().size());
r.waitUntilNoActivity();
WorkflowRun b1 = p.getLastBuild();
assertEquals(1, b1.getNumber());
WorkflowRun b1 = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 1);
Authentication auth = User.getById("dev", true).impersonate();
assertFalse(p.getACL().hasPermission(auth, Item.DELETE));
assertTrue(p.isBuildable());
Expand All @@ -235,37 +241,76 @@ public static void assertRevisionAction(WorkflowRun build) {
assertEquals(1, mp.getItems().size());
}

@Issue("JENKINS-46795")
@Test public void untrustedRevisions() throws Exception {
r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
r.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy().
grant(Jenkins.READ, Item.READ).everywhere().toAuthenticated().
grant(Item.BUILD).everywhere().to("alice").
grant(Item.BUILD, Item.CONFIGURE).everywhere().to("bob"));
sampleGitRepo.init();
sampleGitRepo.write("Jenkinsfile", "node {checkout scm; echo readFile('file')}");
String masterJenkinsfile = "node {checkout scm; echo readFile('file')}";
sampleGitRepo.write("Jenkinsfile", masterJenkinsfile);
sampleGitRepo.write("file", "initial content");
sampleGitRepo.git("add", "Jenkinsfile");
sampleGitRepo.git("commit", "--all", "--message=flow");
WorkflowMultiBranchProject mp = r.jenkins.createProject(WorkflowMultiBranchProject.class, "p");
mp.getSourcesList().add(new BranchSource(new WarySource(null, sampleGitRepo.toString(), "", "*", "", false)));
WorkflowJob p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, "master");
r.waitUntilNoActivity();
WorkflowRun b = p.getLastBuild();
assertNotNull(b);
assertEquals(1, b.getNumber());
WorkflowRun b = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 1);
assertRevisionAction(b);
r.assertBuildStatusSuccess(b);
r.assertLogContains("initial content", b);
String branch = "some-other-branch-from-Norway";
sampleGitRepo.git("checkout", "-b", branch);
sampleGitRepo.write("Jenkinsfile", "error 'ALL YOUR BUILD STEPS ARE BELONG TO US'");
sampleGitRepo.write("file", "subsequent content");
sampleGitRepo.git("commit", "--all", "--message=big evil laugh");
p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, branch);
r.waitUntilNoActivity();
b = p.getLastBuild();
assertNotNull(b);
assertEquals(1, b.getNumber());
b = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 1);
assertRevisionAction(b);
r.assertBuildStatusSuccess(b);
r.assertBuildStatus(Result.NOT_BUILT, b);
r.assertLogContains(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis("Jenkinsfile"), b);
r.assertLogContains("not trusting", b);
SCMBinder.IGNORE_UNTRUSTED_EDITS = true;
try {
sampleGitRepo.write("file", "subsequent content");
sampleGitRepo.git("commit", "--all", "--message=edits");
p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, branch);
b = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 2);
r.assertLogContains("subsequent content", b);
r.assertLogContains("not trusting", b);
} finally {
SCMBinder.IGNORE_UNTRUSTED_EDITS = false;
}
sampleGitRepo.write("Jenkinsfile", masterJenkinsfile);
sampleGitRepo.git("commit", "--all", "--message=meekly submitting");
p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, branch);
b = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 3);
r.assertLogContains("subsequent content", b);
r.assertLogContains("not trusting", b);
sampleGitRepo.write("Jenkinsfile", "node {checkout scm; echo readTrusted('file').toUpperCase()}");
sampleGitRepo.git("commit", "--all", "--message=changes to be approved");
p = WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject(mp, branch);
b = await().until(p::getLastCompletedBuild, lb -> lb != null && lb.getNumber() == 4);
r.assertBuildStatus(Result.NOT_BUILT, b);
r.assertLogContains(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis("Jenkinsfile"), b);
r.assertLogContains("not trusting", b);
try (var ctx = ACL.as(User.getById("alice", true))) {
b = p.scheduleBuild2(0, new CauseAction(new Cause.UserIdCause())).get();
}
assertEquals(5, b.getNumber());
r.assertBuildStatus(Result.NOT_BUILT, b);
r.assertLogContains(Messages.ReadTrustedStep__has_been_modified_in_an_untrusted_revis("Jenkinsfile"), b);
r.assertLogContains("Not trusting build since user ‘alice’ lacks Run/Replay permission", b);
r.assertLogContains("not trusting", b);
try (var ctx = ACL.as(User.getById("bob", true))) {
b = p.scheduleBuild2(0, new CauseAction(new Cause.UserIdCause())).get();
}
assertEquals(6, b.getNumber());
r.assertBuildStatusSuccess(b);
r.assertLogContains("SUBSEQUENT CONTENT", b);
r.assertLogContains("Trusting build since it was started by user ‘bob’", b);
r.assertLogNotContains("not trusting", b);
}
public static class WarySource extends GitSCMSource {
public WarySource(String id, String remote, String credentialsId, String includes, String excludes, boolean ignoreOnPushNotifications) {
Expand Down