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 #584

Merged
merged 27 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
a988f0a
JENKINS-73941 - Option to hide "Use Groovy Sandbox" for users without…
jgarciacloudbees Oct 16, 2024
c090d1c
JENKINS-73941 - Readme
jgarciacloudbees Oct 16, 2024
1671669
JENKINS-73941 - Renaming objects + Readme
jgarciacloudbees Oct 16, 2024
a42afca
JENKINS-73941 - Block saving Script Approval when Force Sandbox is en…
jgarciacloudbees Oct 16, 2024
b6b3290
JENKINS-73941 - Testing
jgarciacloudbees Oct 17, 2024
83b33c0
JENKINS-73941 - Text Formatting
jgarciacloudbees Oct 17, 2024
7beb61e
JENKINS-73941 - HideSandbox help improvement
jgarciacloudbees Oct 17, 2024
c621bd4
JENKINS-73941 - Avoid disabling the ScriptApproval screen with forceS…
jgarciacloudbees Oct 17, 2024
7b0e0ef
JENKINS-73941 - Fix the new option 'Force to use the Sandbox globally…
jgarciacloudbees Oct 17, 2024
e7215e6
JENKINS-73941 - New forceSandbox logic does not apply for admin users…
jgarciacloudbees Oct 18, 2024
9676a9b
JENKINS-73941 - New forceSandbox logic does not apply for admin user …
jgarciacloudbees Oct 21, 2024
2d9c65e
JENKINS-73941 - Additional messages for Sandbox mode
jgarciacloudbees Oct 21, 2024
7469530
JENKINS-73941 - New forceSandbox logic - Testing fixing.
jgarciacloudbees Oct 21, 2024
16ccea1
JENKINS-73941 - New forceSandbox logic - Messages changing.
jgarciacloudbees Oct 21, 2024
2ff801c
JENKINS-73941 - New forceSandbox logic - test improvement
jgarciacloudbees Oct 21, 2024
93722aa
JENKINS-73941 - New forceSandbox logic - Messages
jgarciacloudbees Oct 21, 2024
dc58b05
JENKINS-73941 - New forceSandbox logic - Messages + tests
jgarciacloudbees Oct 21, 2024
661356d
JENKINS-73941 - New forceSandbox logic - Messages + tests
jgarciacloudbees Oct 22, 2024
4cbf832
JENKINS-73941 - New forceSandbox logic - Add CASC support + tests
jgarciacloudbees Oct 22, 2024
af7eee1
JENKINS-73941 - Apply suggestions from code review
jgarciacloudbees Oct 24, 2024
fec6912
JENKINS-73941 - Apply suggestions from code review
jgarciacloudbees Oct 24, 2024
e7d0edd
JENKINS-73941 - Additional changes required from suggestions
jgarciacloudbees Oct 24, 2024
9907110
JENKINS-73941 - Additional changes required from suggestions
jgarciacloudbees Oct 24, 2024
2a9fe7c
JENKINS-73941 - Apply suggestions from code review
jgarciacloudbees Oct 25, 2024
4e97712
JENKINS-73941 - Add forceSandbox logic to SecureGroovyScript
jgarciacloudbees Oct 25, 2024
aad4d51
JENKINS-73941 - Apply suggestions from code review
jgarciacloudbees Oct 25, 2024
64f4584
JENKINS-73941 - test changes after suggestions
jgarciacloudbees Oct 25, 2024
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
5 changes: 5 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@ Administrators in security-sensitive environments should carefully consider whic
operations to whitelist. Operations which change state of persisted objects (such as
Jenkins jobs) should generally be denied. Most `getSomething` methods are harmless.

In case of highly secured environments, where only sandbox scripts are allowed, the
option "Force the use of the sandbox globally in the system" allows forcing the use of the
sandbox globally in the system and will block the creation of new items in the
"In-process Script Approval" screen.

### ACL-aware methods
Be aware however that even some “getter” methods are designed to check specific
permissions (using an ACL: access control list), whereas scripts are often run by a system
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,14 @@
return sandbox ? FormValidation.ok() : ScriptApproval.get().checking(value, GroovyLanguage.get(), !StringUtils.equals(oldScript, value));
}

@Restricted(NoExternalUse.class) // stapler
public boolean shouldHideSandbox(@CheckForNull SecureGroovyScript 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 ScriptApproval.get().isForceSandboxForCurrentUser()

Check warning on line 470 in src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 470 is only partially covered, 3 branches are missing
&& (instance == null || instance.sandbox);
}

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,9 @@
/** All external classpath entries allowed used for scripts. */
private /*final*/ TreeSet<ApprovedClasspathEntry> approvedClasspathEntries;

/** when this mode is enabled, the full logic for accepting/rejecting scripts will be hidden **/
jgarciacloudbees marked this conversation as resolved.
Show resolved Hide resolved
private boolean forceSandbox;

/* for test */ synchronized void addApprovedClasspathEntry(ApprovedClasspathEntry acp) {
approvedClasspathEntries.add(acp);
}
Expand Down Expand Up @@ -514,7 +517,9 @@
}

/* for test */ void addPendingClasspathEntry(PendingClasspathEntry pcp) {
pendingClasspathEntries.add(pcp);
if (!isForceSandboxForCurrentUser()) {
pendingClasspathEntries.add(pcp);
}
}

@DataBoundConstructor
Expand Down Expand Up @@ -652,7 +657,9 @@
if (key != null) {
pendingScripts.removeIf(pendingScript -> key.equals(pendingScript.getContext().getKey()));
}
pendingScripts.add(new PendingScript(script, language, context));
if (!isForceSandboxForCurrentUser()) {
pendingScripts.add(new PendingScript(script, language, context));
}
}
save();
}
Expand Down Expand Up @@ -733,7 +740,7 @@
approvedClasspathEntries.add(acp);
shouldSave = true;
} else {
if (pendingClasspathEntries.add(pcp)) {
if (!isForceSandboxForCurrentUser() && pendingClasspathEntries.add(pcp)) {
LOG.log(Level.FINE, "{0} ({1}) is pending", new Object[] {url, result.newHash});
shouldSave = true;
}
Expand Down Expand Up @@ -784,7 +791,7 @@
if (!result.approved) {
// Never approve classpath here.
ApprovalContext context = ApprovalContext.create();
if (pendingClasspathEntries.add(new PendingClasspathEntry(result.newHash, url, context))) {
if (!isForceSandboxForCurrentUser() && pendingClasspathEntries.add(new PendingClasspathEntry(result.newHash, url, context))) {
LOG.log(Level.FINE, "{0} ({1}) is pending.", new Object[]{url, result.newHash});
save();
}
Expand Down Expand Up @@ -815,7 +822,9 @@
}

if (!Jenkins.get().hasPermission(Jenkins.ADMINISTER)) {
return FormValidation.warningWithMarkup("A Jenkins administrator will need to approve this script before it can be used");
return FormValidation.warningWithMarkup(isForceSandboxForCurrentUser() ?
Messages.ScriptApproval_ForceSandBoxMessage() :
Messages.ScriptApproval_PipelineMessage());
} else {
if ((ALLOW_ADMIN_APPROVAL_ENABLED && (willBeApproved || ADMIN_AUTO_APPROVAL_ENABLED)) || !Jenkins.get().isUseSecurity()) {
return FormValidation.okWithMarkup("The script has not yet been approved, but it will be approved on save.");
Expand Down Expand Up @@ -888,7 +897,7 @@
@Deprecated
public synchronized RejectedAccessException accessRejected(@NonNull RejectedAccessException x, @NonNull ApprovalContext context) {
String signature = x.getSignature();
if (signature != null && pendingSignatures.add(new PendingSignature(signature, x.isDangerous(), context))) {
if (signature != null && !isForceSandboxForCurrentUser() && pendingSignatures.add(new PendingSignature(signature, x.isDangerous(), context))) {

Check warning on line 900 in src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 900 is only partially covered, 2 branches are missing
save();
}
return x;
Expand Down Expand Up @@ -982,6 +991,22 @@
reconfigure();
}

@DataBoundSetter
public void setForceSandbox(boolean forceSandbox) {
this.forceSandbox = forceSandbox;
save();
}


public boolean isForceSandbox() {
return forceSandbox;
}

//ForceSandbox restrictions does not apply to ADMINISTER users.
public boolean isForceSandboxForCurrentUser() {
return forceSandbox && !Jenkins.get().hasPermission(Jenkins.ADMINISTER);
}

@Restricted(NoExternalUse.class) // Jelly, tests, implementation
public synchronized String[] getApprovedScriptHashes() {
return approvedScriptHashes.toArray(new String[approvedScriptHashes.size()]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ public class ScriptApprovalNote extends ConsoleNote<Object> {

public static void print(TaskListener listener, RejectedAccessException x) {
try {
String text = Messages.ScriptApprovalNote_message();
String text = ScriptApproval.get().isForceSandbox() ?
Messages.ScriptApprovalNoteForceSandBox_message() : Messages.ScriptApprovalNote_message();
listener.getLogger().println(x.getMessage() + ". " + new ScriptApprovalNote(text.length()).encode() + text);
} catch (IOException x2) {
LOGGER.log(Level.WARNING, null, x2);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public final class UnapprovedUsageException extends SecurityException {
private final String hash;

UnapprovedUsageException(String hash) {
super("script not yet approved for use");
super(ScriptApproval.get().isForceSandbox() ? Messages.UnapprovedUsage_ForceSandBox() : Messages.UnapprovedUsage_NonApproved());
this.hash = hash;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@ THE SOFTWARE.
<!-- TODO https://github.com/stapler/stapler-adjunct-codemirror/issues/1 means no true Groovy support -->
<f:textarea checkMethod="post"/> <!-- TODO codemirror-mode="clike" codemirror-config="'onBlur': cmChange" -->
</f:entry>
<f:entry field="sandbox">
<f:checkbox title="${%Use Groovy Sandbox}" default="${!h.hasPermission(app.ADMINISTER)}" />
<f:entry field="sandbox" class="${descriptor.shouldHideSandbox(instance) ? 'jenkins-hidden' : ''}">
<f:checkbox title="${%Use Groovy Sandbox}"
default="${!h.hasPermission(app.ADMINISTER) || descriptor.shouldHideSandbox(instance)}" />
jgarciacloudbees marked this conversation as resolved.
Show resolved Hide resolved
</f:entry>
<f:entry title="${%Additional classpath}" field="classpath">
<f:entry title="${%Additional classpath}" field="classpath"
class="${descriptor.shouldHideSandbox(instance) ?'jenkins-hidden' : ''}">
<f:repeatableProperty add="${%Add entry}" header="${%Classpath entry}" field="classpath"/>
</f:entry>
</j:jelly>
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@ ClasspathEntry.path.notExists=Specified path does not exist
ClasspathEntry.path.notApproved=This classpath entry is not approved. Require an approval before execution.
ClasspathEntry.path.noDirsAllowed=Class directories are not allowed as classpath entries.
ScriptApprovalNote.message=Administrators can decide whether to approve or reject this signature.
ScriptApprovalNoteForceSandBox.message=Script signature is not in the default whitelist.
ScriptApprovalLink.outstandingScript={0} scripts pending approval
ScriptApprovalLink.outstandingSignature={0} signatures pending approval
ScriptApprovalLink.outstandingClasspath={0} classpath entries pending approval
ScriptApprovalLink.dangerous={0} approved dangerous signatures
ScriptApprovalLink.dangerous={0} approved dangerous signatures
ScriptApproval.PipelineMessage=A Jenkins administrator will need to approve this script before it can be used
ScriptApproval.ForceSandBoxMessage=Running scripts out of the sandbox is not allowed in the system
UnapprovedUsage.NonApproved=script not yet approved for use
UnapprovedUsage.ForceSandBox=Running scripts out of the sandbox is not allowed in the system
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
<!-- Just ti satisfy GlobalConfiguration.getGlobalConfigPage() -->
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:l="/lib/layout" xmlns:f="/lib/form">
<f:section title="${%Sandbox Configuration}">
<f:entry field="forceSandbox" title="${%Force the use of the sandbox globally in the system}">
<f:checkbox/>
jgarciacloudbees marked this conversation as resolved.
Show resolved Hide resolved
</f:entry>
</f:section>
</j:jelly>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<div>
<p>Controls whether the "Use Groovy Sandbox" is shown in the system to users without Overall/Administer permission.</p>
<p>This can be used to simplify the UX in highly secured environments where all Pipelines and any other Groovy execution are
required to run in the sandbox (i.e., running arbitrary code is never approved).</p>
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.apache.tools.ant.AntClassLoader;
import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
import org.jenkinsci.plugins.scriptsecurity.scripts.ClasspathEntry;
import org.jenkinsci.plugins.scriptsecurity.scripts.Messages;
import org.htmlunit.html.HtmlForm;
import org.htmlunit.html.HtmlFormUtil;
import org.htmlunit.html.HtmlPage;
Expand Down Expand Up @@ -182,6 +183,51 @@ private void addPostBuildAction(HtmlPage page) throws IOException {
assertEquals("P#3", r.assertBuildStatusSuccess(p.scheduleBuild2(0)).getDescription());
}

/**
* Basic approval test where the user doesn't have ADMINISTER privs and forceSandbox is enabled
* Sandbox checkbox should not be visible, but set to true by default
*/
@Test public void basicApproval_ForceSandbox() throws Exception {
ScriptApproval.get().setForceSandbox(true);

r.jenkins.setSecurityRealm(r.createDummySecurityRealm());

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

FreeStyleProject p = r.createFreeStyleProject("p");
JenkinsRule.WebClient wc = r.createWebClient();
wc.login("devel");
HtmlPage page = wc.getPage(p, "configure");
HtmlForm config = page.getFormByName("config");
HtmlFormUtil.getButtonByCaption(config, "Add post-build action").click(); // lib/hudson/project/config-publishers2.jelly
addPostBuildAction(page);
wc.waitForBackgroundJavaScript(10000);
List<HtmlTextArea> 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 = "build.externalizableId";
script.setText(groovy);

//As the user is not admin and we are forcing Sandbox use,
// the Sandbox checkbox should be hidden and enabled by default.
List<HtmlInput> sandboxes = config.getInputsByName("_.sandbox");
HtmlCheckBoxInput sandboxcb = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1);
assertTrue(sandboxcb.isChecked());

r.submit(config);

List<Publisher> publishers = p.getPublishersList();
assertEquals(1, publishers.size());
TestGroovyRecorder publisher = (TestGroovyRecorder) publishers.get(0);
assertEquals(groovy, publisher.getScript().getScript());
assertTrue(publisher.getScript().isSandbox());
}


/**
* Test where the user has ADMINISTER privs, default to non sandbox mode, but require approval
Expand Down Expand Up @@ -224,7 +270,22 @@ private void addPostBuildAction(HtmlPage page) throws IOException {
assertEquals(1, pendingScripts.size());

// Test that the script is executable. If it's not, we will get an UnapprovedUsageException
assertThrows(UnapprovedUsageException.class, () -> ScriptApproval.get().using(groovy, GroovyLanguage.get()));
Exception ex = assertThrows(UnapprovedUsageException.class,
() -> ScriptApproval.get().using(groovy,GroovyLanguage.get()));
assertEquals(ScriptApproval.get().isForceSandbox()
? Messages.UnapprovedUsage_ForceSandBox()
: Messages.UnapprovedUsage_NonApproved()
, ex.getMessage());
}

/**
* Test where the user has ADMINISTER privs + forceSandboxEnabled
* logic should not change to the default admin behavior
* Except for the messages
*/
@Test public void testSandboxDefault_with_ADMINISTER_privs_ForceSandbox() throws Exception {
ScriptApproval.get().setForceSandbox(true);
testSandboxDefault_with_ADMINISTER_privs();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.hamcrest.collection.IsIterableContainingInAnyOrder.containsInAnyOrder;
import static org.hamcrest.core.StringContains.containsString;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class JcascTest {

Expand All @@ -43,6 +44,7 @@ public void smokeTestEntry() throws Exception {
assertThat(logger.getMessages(), containsInAnyOrder(
containsString("Adding deprecated script hash " +
"that will be converted on next use: fccae58c5762bdd15daca97318e9d74333203106")));
assertTrue(ScriptApproval.get().isForceSandbox());
}

@Test
Expand Down
Loading