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.

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-73941 - Option to hide "Use Groovy Sandbox" for users without Administer permission globally in the system #948

Merged
merged 14 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions plugin/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@
<scope>import</scope>
<type>pom</type>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<version>1367.vdf2fc45f229c</version>
</dependency>
</dependencies>
</dependencyManagement>
<dependencies>
Expand Down Expand Up @@ -246,6 +251,16 @@
<version>4.2.2</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.jenkins</groupId>
<artifactId>configuration-as-code</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.jenkins.configuration-as-code</groupId>
<artifactId>test-harness</artifactId>
<scope>test</scope>
</dependency>
jgarciacloudbees marked this conversation as resolved.
Show resolved Hide resolved
</dependencies>
<build>
<resources>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.plugins.workflow.cps.config.CPSConfiguration;
import org.jenkinsci.plugins.workflow.cps.persistence.PersistIn;
import org.jenkinsci.plugins.workflow.flow.DurabilityHintProvider;
import org.jenkinsci.plugins.workflow.flow.FlowDefinition;
Expand Down Expand Up @@ -89,7 +88,7 @@ public CpsFlowDefinition(String script) throws Descriptor.FormException {

@DataBoundConstructor
public CpsFlowDefinition(String script, boolean sandbox) throws Descriptor.FormException {
if (CPSConfiguration.get().isHideSandbox() && !sandbox && !Jenkins.get().hasPermission(Jenkins.ADMINISTER)) {
if (!sandbox && ScriptApproval.get().isForceSandboxForCurrentUser()) {
// this will end up in the /oops page until https://github.com/jenkinsci/jenkins/pull/9495 is picked up
throw new Descriptor.FormException("Sandbox cannot be disabled. This Jenkins instance has been configured to not " +
"allow regular users to disable the sandbox in pipelines", "sandbox");
Expand All @@ -98,7 +97,6 @@ public CpsFlowDefinition(String script, boolean sandbox) throws Descriptor.FormE
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;

}

private Object readResolve() {
Expand Down Expand Up @@ -196,7 +194,7 @@ public JSON doCheckScriptCompile(@AncestorInPath Item job, @QueryParameter Strin
public boolean shouldHideSandbox(@CheckForNull CpsFlowDefinition instance) {
// sandbox checkbox is shown to admins even if the global configuration says otherwise
// it's also shown when sandbox == false, so regular users can enable it
return CPSConfiguration.get().isHideSandbox() && !Jenkins.get().hasPermission(Jenkins.ADMINISTER)
return ScriptApproval.get().isForceSandboxForCurrentUser()
&& (instance == null || instance.sandbox);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,33 +24,55 @@

package org.jenkinsci.plugins.workflow.cps.config;

import java.io.IOException;
import java.io.UncheckedIOException;

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Extension;
import hudson.ExtensionList;
import jenkins.model.GlobalConfiguration;
import jenkins.model.GlobalConfigurationCategory;
import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
import org.jenkinsci.Symbol;

/**
* @deprecated
* This class has been deprecated and its only configuration value is ignored. Do not rely on it or use it in any way.
* In order to force using the system sandbox for pipelines, please use {@link ScriptApproval#isForceSandbox} or {@link ScriptApproval#isForceSandboxForCurrentUser}
**/
@Symbol("cps")
@Extension
@Deprecated
public class CPSConfiguration extends GlobalConfiguration {

/**
* Whether to show the sandbox checkbox in jobs to users without Jenkins.ADMINISTER
*/
private boolean hideSandbox;
private transient boolean hideSandbox;

public CPSConfiguration() {

load();
if (hideSandbox) {
ScriptApproval.get().setForceSandbox(hideSandbox);
}

// Data migration from this configuration to ScriptApproval should be done only once,
// so removing the config file after the previous migration
try {
this.getConfigFile().delete();
} catch (IOException e) {
throw new UncheckedIOException(e);
jgarciacloudbees marked this conversation as resolved.
Show resolved Hide resolved
}
}

public boolean isHideSandbox() {
return hideSandbox;
return ScriptApproval.get().isForceSandbox();
}

public void setHideSandbox(boolean hideSandbox) {
jgarciacloudbees marked this conversation as resolved.
Show resolved Hide resolved
this.hideSandbox = hideSandbox;
save();
ScriptApproval.get().setForceSandbox(hideSandbox);
}

@NonNull
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,4 @@ THE SOFTWARE.

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
<f:section title="${%Pipeline Sandbox}">
jgarciacloudbees marked this conversation as resolved.
Show resolved Hide resolved
<f:entry field="hideSandbox" title="${%Hide Sandbox checkbox in pipeline jobs}">
<f:checkbox/>
</f:entry>
</f:section>
</j:jelly>

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

package org.jenkinsci.plugins.workflow.cps;

import hudson.model.Result;
import hudson.util.VersionNumber;
import org.htmlunit.FailingHttpStatusCodeException;
import org.htmlunit.HttpMethod;
Expand All @@ -45,12 +46,15 @@
import org.jenkinsci.plugins.scriptsecurity.scripts.languages.GroovyLanguage;
import org.jenkinsci.plugins.workflow.cps.config.CPSConfiguration;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.MockAuthorizationStrategy;
import org.jvnet.hudson.test.recipes.LocalData;

import java.io.File;
import java.nio.charset.StandardCharsets;
import java.util.List;

Expand All @@ -59,8 +63,6 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
Expand Down Expand Up @@ -316,7 +318,7 @@ public void cpsScriptSandboxHide() throws Exception {
}

// non-admins cannot see the sandbox checkbox in jobs if hideSandbox is On globally
CPSConfiguration.get().setHideSandbox(true);
ScriptApproval.get().setForceSandbox(true);
{
HtmlForm config = wcDevel.getPage(p, "configure").getFormByName("config");
assertThat(config.getVisibleText(), not(containsStringIgnoringCase("Use Groovy Sandbox")));
Expand All @@ -328,15 +330,15 @@ public void cpsScriptSandboxHide() throws Exception {
}

// admins can always see the sandbox checkbox
CPSConfiguration.get().setHideSandbox(false);
ScriptApproval.get().setForceSandbox(false);
wcDevel.login("admin");
{
HtmlForm config = wcDevel.getPage(p, "configure").getFormByName("config");
assertThat(config.getVisibleText(), containsStringIgnoringCase("Use Groovy Sandbox"));
}

// even when set to hide globally
CPSConfiguration.get().setHideSandbox(true);
ScriptApproval.get().setForceSandbox(true);
{
HtmlForm config = wcDevel.getPage(p, "configure").getFormByName("config");
assertThat(config.getVisibleText(), containsStringIgnoringCase("Use Groovy Sandbox"));
Expand Down Expand Up @@ -370,4 +372,67 @@ public void cpsScriptSandboxHide() throws Exception {

}
}

@Test
public void cpsConfigurationSandboxToScriptApprovalSandbox() throws Exception{
//Deprecated CPSConfiguration should update ScriptApproval forceSandbox logic to keep casc compatibility
ScriptApproval.get().setForceSandbox(false);

CPSConfiguration.get().setHideSandbox(true);
assertTrue(ScriptApproval.get().isForceSandbox());

ScriptApproval.get().setForceSandbox(false);
assertFalse(CPSConfiguration.get().isHideSandbox());
}

@Test
public void cpsScriptSignatureException() throws Exception {
ScriptApproval.get().setForceSandbox(false);
WorkflowJob p = jenkins.createProject(WorkflowJob.class);
String script = "jenkins.model.Jenkins.instance";
p.setDefinition(new CpsFlowDefinition(script, true));
WorkflowRun b = jenkins.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get());
jenkins.assertLogContains("Scripts not permitted to use staticMethod jenkins.model.Jenkins getInstance. "
+ "Administrators can decide whether to approve or reject this signature.", b);

ScriptApproval.get().setForceSandbox(true);
b = jenkins.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get());
jenkins.assertLogContains("Scripts not permitted to use staticMethod jenkins.model.Jenkins getInstance. "
+ "Script signature is not in the default whitelist.", b);
}

@Test
@LocalData
public void cpsLoadConfiguration() throws Exception {
//CPSConfiguration file containing <hideSandbox>true</hideSandbox>
// should be promoted to ScriptApproval.get().isForceSandbox()
assertTrue(ScriptApproval.get().isForceSandbox());

//Once the info is promoted, we are removing the config file, so should no longer exist.
//We are checking the injected localData is removed
assertFalse(new File(jenkins.jenkins.getRootDir(),
"org.jenkinsci.plugins.workflow.cps.config.CPSConfiguration.xml").exists());
}

@Test
public void cpsRoundTrip() throws Exception {
jenkins.jenkins.setSecurityRealm(jenkins.createDummySecurityRealm());

MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
mockStrategy.grant(Jenkins.READ).everywhere().to("dev");
for (Permission p : Item.PERMISSIONS.getPermissions()) {
mockStrategy.grant(p).everywhere().to("dev");
}

WorkflowJob p = jenkins.createProject(WorkflowJob.class);
p.setDefinition(new CpsFlowDefinition("echo 'Hello'", true));

WorkflowJob roundTrip = jenkins.configRoundtrip(p);

assertEquals(((CpsFlowDefinition)p.getDefinition()).isSandbox(),
((CpsFlowDefinition)roundTrip.getDefinition()).isSandbox());

assertEquals(((CpsFlowDefinition)p.getDefinition()).getScript(),
((CpsFlowDefinition)roundTrip.getDefinition()).getScript());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package org.jenkinsci.plugins.workflow.cps;

import io.jenkins.plugins.casc.ConfigurationContext;
import io.jenkins.plugins.casc.ConfiguratorRegistry;
import io.jenkins.plugins.casc.misc.ConfiguredWithCode;
import io.jenkins.plugins.casc.misc.JenkinsConfiguredWithCodeRule;
import io.jenkins.plugins.casc.model.CNode;
import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
import org.junit.ClassRule;
import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static io.jenkins.plugins.casc.misc.Util.getSecurityRoot;
import static io.jenkins.plugins.casc.misc.Util.toStringFromYamlFile;
import static io.jenkins.plugins.casc.misc.Util.toYamlString;

public class JcascTest {

@ClassRule(order = 1)
@ConfiguredWithCode("casc_test.yaml")
public static JenkinsConfiguredWithCodeRule j = new JenkinsConfiguredWithCodeRule();

/**
* Check that CASC for security.cps.hideSandbox is sending the value to ScriptApproval.get().isForceSandbox()
* @throws Exception
*/
@Test
public void cascHideSandBox() throws Exception {
assertTrue(ScriptApproval.get().isForceSandbox());
}

@Test
public void cascExport() throws Exception {
ConfiguratorRegistry registry = ConfiguratorRegistry.get();
ConfigurationContext context = new ConfigurationContext(registry);
CNode yourAttribute = getSecurityRoot(context).get("cps");
Copy link
Member

Choose a reason for hiding this comment

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

I think more interesting would be to check what is then exported from the script-security version.

String exported = toYamlString(yourAttribute);
String expected = toStringFromYamlFile(this, "casc_test_expected.yaml");
assertEquals(exported, expected);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?xml version='1.1' encoding='UTF-8'?>
<org.jenkinsci.plugins.workflow.cps.config.CPSConfiguration plugin="workflow-cps">
<hideSandbox>true</hideSandbox>
</org.jenkinsci.plugins.workflow.cps.config.CPSConfiguration>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
security:
cps:
hideSandbox: true
jgarciacloudbees marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hideSandbox: true
Loading