diff --git a/pom.xml b/pom.xml index f7557e82c..711c9d72a 100644 --- a/pom.xml +++ b/pom.xml @@ -101,6 +101,7 @@ org.jenkins-ci.plugins script-security + 1172.v35f6a_0b_8207e org.jenkins-ci.plugins @@ -148,6 +149,7 @@ org.jenkins-ci.plugins.workflow workflow-job test + 1181.va_25d15548158 org.jenkins-ci.plugins.workflow diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java index ebf62225c..e69bfb32c 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java @@ -24,6 +24,7 @@ package org.jenkinsci.plugins.workflow.cps; +import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Extension; import hudson.model.Action; import hudson.model.Item; @@ -33,6 +34,8 @@ import hudson.model.TaskListener; import hudson.util.FormValidation; import hudson.util.StreamTaskListener; +import net.sf.json.JSONObject; +import org.apache.commons.lang.StringUtils; import org.jenkinsci.plugins.workflow.cps.persistence.PersistIn; import org.jenkinsci.plugins.workflow.flow.DurabilityHintProvider; import org.jenkinsci.plugins.workflow.flow.FlowDefinition; @@ -80,7 +83,8 @@ public CpsFlowDefinition(String script) { @DataBoundConstructor public CpsFlowDefinition(String script, boolean sandbox) { StaplerRequest req = Stapler.getCurrentRequest(); - this.script = sandbox ? script : ScriptApproval.get().configuring(script, GroovyLanguage.get(), ApprovalContext.create().withCurrentUser().withItemAsKey(req != null ? req.findAncestorObject(Item.class) : null)); + this.script = sandbox ? script : ScriptApproval.get().configuring(script, GroovyLanguage.get(), + ApprovalContext.create().withCurrentUser().withItemAsKey(req != null ? req.findAncestorObject(Item.class) : null), req == null); this.sandbox = sandbox; } @@ -123,14 +127,41 @@ public CpsFlowExecution create(FlowExecutionOwner owner, TaskListener listener, @Extension public static class DescriptorImpl extends FlowDefinitionDescriptor { + /* In order to fix SECURITY-2450 without causing significant UX regressions, we decided to continue to + * automatically approve scripts on save if the script was modified by an administrator. To make this possible, + * we added a new hidden input field to the config.jelly to track the pre-save version of the script. Since + * CpsFlowDefinition calls ScriptApproval.configuring in its @DataBoundConstructor, the normal way to handle + * things would be to add an oldScript parameter to the constructor and perform the relevant logic there. + * + * However, that would have compatibility implications for tools like JobDSL, since @DataBoundConstructor + * parameters are required. We cannot use a @DataBoundSetter with a corresponding field and getter to trivially + * make oldScript optional, because we would need to call ScriptApproval.configuring after all + * @DataBoundSetters have been invoked (rather than in the @DataBoundConstructor), which is why we use Descriptor.newInstance. + */ + @Override + public FlowDefinition newInstance(@NonNull StaplerRequest req, @NonNull JSONObject formData) throws FormException { + CpsFlowDefinition cpsFlowDefinition = (CpsFlowDefinition) super.newInstance(req, formData); + if (!cpsFlowDefinition.sandbox && formData.get("oldScript") != null) { + String oldScript = formData.getString("oldScript"); + boolean approveIfAdmin = !StringUtils.equals(oldScript, cpsFlowDefinition.script); + if (approveIfAdmin) { + ScriptApproval.get().configuring(cpsFlowDefinition.script, GroovyLanguage.get(), + ApprovalContext.create().withCurrentUser().withItemAsKey(req.findAncestorObject(Item.class)), true); + } + } + return cpsFlowDefinition; + } + @Override public String getDisplayName() { return "Pipeline script"; } @RequirePOST - public FormValidation doCheckScript(@QueryParameter String value, @QueryParameter boolean sandbox) { - return sandbox ? FormValidation.ok() : ScriptApproval.get().checking(value, GroovyLanguage.get()); + public FormValidation doCheckScript(@QueryParameter String value, @QueryParameter String oldScript, + @QueryParameter boolean sandbox) { + return sandbox ? FormValidation.ok() : + ScriptApproval.get().checking(value, GroovyLanguage.get(), !StringUtils.equals(oldScript, value)); } @RequirePOST diff --git a/src/main/resources/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition/config.jelly b/src/main/resources/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition/config.jelly index 1e0ef0360..c075a1fd2 100644 --- a/src/main/resources/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition/config.jelly @@ -24,6 +24,7 @@ --> + diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java index 0f4938c45..aac0bcafb 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java @@ -25,19 +25,29 @@ package org.jenkinsci.plugins.workflow.cps; import com.cloudbees.groovy.cps.CpsTransformer; +import com.gargoylesoftware.htmlunit.html.HtmlCheckBoxInput; +import com.gargoylesoftware.htmlunit.html.HtmlForm; +import com.gargoylesoftware.htmlunit.html.HtmlInput; +import com.gargoylesoftware.htmlunit.html.HtmlTextArea; import hudson.Functions; import hudson.model.Computer; import hudson.model.Describable; import hudson.model.Executor; +import hudson.model.Item; import hudson.model.Result; import java.io.Serializable; import java.util.Collections; +import java.util.List; import java.util.Set; import java.util.logging.Level; + +import hudson.security.Permission; import jenkins.model.Jenkins; import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException; +import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval; +import org.jenkinsci.plugins.scriptsecurity.scripts.languages.GroovyLanguage; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.jenkinsci.plugins.workflow.steps.Step; @@ -58,13 +68,16 @@ import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.LoggerRule; +import org.jvnet.hudson.test.MockAuthorizationStrategy; import org.jvnet.hudson.test.TestExtension; import org.kohsuke.stapler.DataBoundConstructor; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.instanceOf; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; public class CpsFlowDefinition2Test { @@ -89,7 +102,7 @@ public void endlessRecursion() throws Exception { WorkflowRun r = jenkins.assertBuildStatus(Result.FAILURE, job.scheduleBuild2(0).get()); jenkins.assertLogContains("look for unbounded recursion", r); - Assert.assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty()); + assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty()); for (Computer c : jenkins.jenkins.getComputers()) { for (Executor ex : c.getExecutors()) { @@ -117,7 +130,7 @@ public void endlessRecursionNonCPS() throws Exception { // Should have failed with error about excessive recursion depth WorkflowRun r = jenkins.assertBuildStatus(Result.FAILURE, job.scheduleBuild2(0).get()); - Assert.assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty()); + assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty()); for (Computer c : jenkins.jenkins.getComputers()) { for (Executor ex : c.getExecutors()) { @@ -852,6 +865,141 @@ public void scriptInitializerCallsCpsTransformedMethod() throws Exception { assertNull(Jenkins.get().getDescription()); } + @Issue("SECURITY-2450") + @Test + public void cpsScriptNonAdminConfiguration() throws Exception { + jenkins.jenkins.setSecurityRealm(jenkins.createDummySecurityRealm()); + + MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); + mockStrategy.grant(Jenkins.READ).everywhere().to("devel"); + for (Permission p : Item.PERMISSIONS.getPermissions()) { + mockStrategy.grant(p).everywhere().to("devel"); + } + jenkins.jenkins.setAuthorizationStrategy(mockStrategy); + + JenkinsRule.WebClient wcDevel = jenkins.createWebClient(); + wcDevel.login("devel"); + + WorkflowJob p = jenkins.createProject(WorkflowJob.class); + + HtmlForm config = wcDevel.getPage(p, "configure").getFormByName("config"); + List scripts = config.getTextAreasByName("_.script"); + // Get the last one, because previous ones might be from Lockable Resources during PCT. + HtmlTextArea script = scripts.get(scripts.size() - 1); + String groovy = "echo 'hi from cpsScriptNonAdminConfiguration'"; + script.setText(groovy); + + List sandboxes = config.getInputsByName("_.sandbox"); + // Get the last one, because previous ones might be from Lockable Resources during PCT. + HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1); + assertTrue(sandbox.isChecked()); + sandbox.setChecked(false); + + jenkins.submit(config); + + assertEquals(1, ScriptApproval.get().getPendingScripts().size()); + assertFalse(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get())); + } + + @Issue("SECURITY-2450") + @Test + public void cpsScriptAdminConfiguration() throws Exception { + jenkins.jenkins.setSecurityRealm(jenkins.createDummySecurityRealm()); + + MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); + mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin"); + for (Permission p : Item.PERMISSIONS.getPermissions()) { + mockStrategy.grant(p).everywhere().to("admin"); + } + jenkins.jenkins.setAuthorizationStrategy(mockStrategy); + + JenkinsRule.WebClient admin = jenkins.createWebClient(); + admin.login("admin"); + + WorkflowJob p = jenkins.createProject(WorkflowJob.class); + + HtmlForm config = admin.getPage(p, "configure").getFormByName("config"); + List scripts = config.getTextAreasByName("_.script"); + // Get the last one, because previous ones might be from Lockable Resources during PCT. + HtmlTextArea script = scripts.get(scripts.size() - 1); + String groovy = "echo 'hi from cpsScriptAdminConfiguration'"; + script.setText(groovy); + + List sandboxes = config.getInputsByName("_.sandbox"); + // Get the last one, because previous ones might be from Lockable Resources during PCT. + HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1); + assertTrue(sandbox.isChecked()); + sandbox.setChecked(false); + + jenkins.submit(config); + + assertTrue(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get())); + } + + @Issue("SECURITY-2450") + @Test + public void cpsScriptAdminModification() throws Exception { + jenkins.jenkins.setSecurityRealm(jenkins.createDummySecurityRealm()); + + MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); + mockStrategy.grant(Jenkins.READ).everywhere().to("devel"); + mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin"); + for (Permission p : Item.PERMISSIONS.getPermissions()) { + mockStrategy.grant(p).everywhere().to("devel"); + mockStrategy.grant(p).everywhere().to("admin"); + } + jenkins.jenkins.setAuthorizationStrategy(mockStrategy); + + JenkinsRule.WebClient wc = jenkins.createWebClient(); + wc.login("devel"); + + WorkflowJob p = jenkins.createProject(WorkflowJob.class); + String userGroovy = "echo 'hi from devel'"; + String adminGroovy = "echo 'hi from admin'"; + + // initial configuration by user, script ends up in pending + { + HtmlForm config = wc.getPage(p, "configure").getFormByName("config"); + List scripts = config.getTextAreasByName("_.script"); + // Get the last one, because previous ones might be from Lockable Resources during PCT. + HtmlTextArea script = scripts.get(scripts.size() - 1); + script.setText(userGroovy); + + List sandboxes = config.getInputsByName("_.sandbox"); + // Get the last one, because previous ones might be from Lockable Resources during PCT. + HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1); + assertTrue(sandbox.isChecked()); + sandbox.setChecked(false); + + jenkins.submit(config); + + assertFalse(ScriptApproval.get().isScriptApproved(userGroovy, GroovyLanguage.get())); + } + + wc.login("admin"); + + // modification by admin, script gets approved automatically + { + HtmlForm config = wc.getPage(p, "configure").getFormByName("config"); + List scripts = config.getTextAreasByName("_.script"); + // Get the last one, because previous ones might be from Lockable Resources during PCT. + HtmlTextArea script = scripts.get(scripts.size() - 1); + script.setText(adminGroovy); + + List sandboxes = config.getInputsByName("_.sandbox"); + // Get the last one, because previous ones might be from Lockable Resources during PCT. + HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1); + assertFalse(sandbox.isChecked()); + + jenkins.submit(config); + + // script content was modified by admin, so it should be approved upon save + // the one that had been submitted by the user previously stays in pending + assertTrue(ScriptApproval.get().isScriptApproved(adminGroovy, GroovyLanguage.get())); + assertFalse(ScriptApproval.get().isScriptApproved(userGroovy, GroovyLanguage.get())); + } + } + public static class UnsafeParameterStep extends Step implements Serializable { private final UnsafeDescribable val; @DataBoundConstructor diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionRJRTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionRJRTest.java new file mode 100644 index 000000000..f1ef48d35 --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionRJRTest.java @@ -0,0 +1,24 @@ +package org.jenkinsci.plugins.workflow.cps; + +import org.jenkinsci.plugins.workflow.job.WorkflowJob; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.RealJenkinsRule; + +public class CpsFlowDefinitionRJRTest { + + @Rule + public RealJenkinsRule rjr = new RealJenkinsRule(); + + @Test + public void smokes() throws Throwable { + rjr.then(CpsFlowDefinitionRJRTest::doesItSmoke); + } + + private static void doesItSmoke(JenkinsRule r) throws Exception { + WorkflowJob p = r.createProject(WorkflowJob.class, "p"); + p.setDefinition(new CpsFlowDefinition("print Jenkins.get().getRootDir().toString()", false)); + r.assertBuildStatusSuccess(p.scheduleBuild2(0)); + } +}