JENKINS-73941 - Option to hide "Use Groovy Sandbox" for users without…

… Administer permission globally in the system (#584) * JENKINS-73941 - Option to hide "Use Groovy Sandbox" for users without Administer permission globally in the system * JENKINS-73941 - Readme * JENKINS-73941 - Renaming objects + Readme * JENKINS-73941 - Block saving Script Approval when Force Sandbox is enabled * JENKINS-73941 - Testing * JENKINS-73941 - Text Formatting * JENKINS-73941 - HideSandbox help improvement * JENKINS-73941 - Avoid disabling the ScriptApproval screen with forceSandbox mode enabled * JENKINS-73941 - Fix the new option 'Force to use the Sandbox globally in the system' help * JENKINS-73941 - New forceSandbox logic does not apply for admin users, disabling it. * JENKINS-73941 - New forceSandbox logic does not apply for admin user + Tests * JENKINS-73941 - Additional messages for Sandbox mode * JENKINS-73941 - New forceSandbox logic - Testing fixing. * JENKINS-73941 - New forceSandbox logic - Messages changing. * JENKINS-73941 - New forceSandbox logic - test improvement * JENKINS-73941 - New forceSandbox logic - Messages * JENKINS-73941 - New forceSandbox logic - Messages + tests * JENKINS-73941 - New forceSandbox logic - Messages + tests * JENKINS-73941 - New forceSandbox logic - Add CASC support + tests * JENKINS-73941 - Apply suggestions from code review Co-authored-by: michael cirioli <[email protected]> * JENKINS-73941 - Apply suggestions from code review Co-authored-by: Antonio Muniz <[email protected]> * JENKINS-73941 - Additional changes required from suggestions * JENKINS-73941 - Additional changes required from suggestions * JENKINS-73941 - Apply suggestions from code review Co-authored-by: Jesse Glick <[email protected]> * JENKINS-73941 - Add forceSandbox logic to SecureGroovyScript * JENKINS-73941 - Apply suggestions from code review Co-authored-by: Jesse Glick <[email protected]> * JENKINS-73941 - test changes after suggestions --------- Co-authored-by: michael cirioli <[email protected]> Co-authored-by: Antonio Muniz <[email protected]> Co-authored-by: Jesse Glick <[email protected]>
jenkinsci · Oct 25, 2024 · d44b49a · d44b49a
1 parent 4778ca8
commit d44b49a
Show file tree

Hide file tree

Showing 14 changed files with 267 additions and 14 deletions.
diff --git a/README.md b/README.md
@@ -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 

diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java
@@ -463,6 +463,14 @@ public FormValidation doCheckScript(@QueryParameter String value, @QueryParamete
             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()
+                   && (instance == null || instance.sandbox);
+        }
+
     }
 
 }
diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
@@ -285,6 +285,9 @@ String hashClasspathEntry(URL entry) throws IOException {
     /** 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 */
+    private boolean forceSandbox;
+
     /* for test */ synchronized void addApprovedClasspathEntry(ApprovedClasspathEntry acp) {
         approvedClasspathEntries.add(acp);
     }
@@ -514,7 +517,9 @@ private PendingClasspathEntry getPendingClasspathEntry(@NonNull String hash) {
     }
 
     /* for test */ void addPendingClasspathEntry(PendingClasspathEntry pcp) {
-        pendingClasspathEntries.add(pcp);
+        if (!isForceSandboxForCurrentUser()) {
+            pendingClasspathEntries.add(pcp);
+        }
     }
 
     @DataBoundConstructor
@@ -652,7 +657,9 @@ public synchronized String configuring(@NonNull String script, @NonNull Language
                 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();
         }
@@ -733,7 +740,7 @@ public synchronized void configuring(@NonNull ClasspathEntry entry, @NonNull App
                 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;
                 }
@@ -784,7 +791,7 @@ public synchronized void using(@NonNull ClasspathEntry entry) throws IOException
         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();
             }
@@ -815,7 +822,9 @@ public synchronized FormValidation checking(@NonNull String script, @NonNull Lan
         }
 
         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.");
@@ -888,7 +897,7 @@ public synchronized void preapproveAll() {
     @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))) {
             save();
         }
         return x;
@@ -982,6 +991,22 @@ public synchronized void setApprovedScriptHashes(String[] scriptHashes) throws I
         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()]);

diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java
@@ -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);

diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/UnapprovedUsageException.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/UnapprovedUsageException.java
@@ -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;
     }
 

diff --git a/...urces/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly b/...urces/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly
@@ -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="true"/>
     </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>
diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties
@@ -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
diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/config.jelly b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/config.jelly
@@ -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/>
+        </f:entry>
+    </f:section>
 </j:jelly>
diff --git a/...ources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html b/...ources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html
@@ -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>
diff --git a/...test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java b/...test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java
@@ -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;
@@ -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
@@ -210,6 +256,11 @@ private void addPostBuildAction(HtmlPage page) throws IOException {
         HtmlTextArea script = scripts.get(scripts.size() - 1);
         String groovy = "build.externalizableId";
         script.setText(groovy);
+        List<HtmlInput> sandboxes = config.getInputsByName("_.sandbox");
+        HtmlCheckBoxInput sandboxcb =  (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1);
+        assertTrue(sandboxcb.isChecked());
+        sandboxcb.setChecked(false);
+
         r.submit(config);
         List<Publisher> publishers = p.getPublishersList();
         assertEquals(1, publishers.size());
@@ -224,7 +275,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();
     }
 
     /**
@@ -1325,6 +1391,9 @@ public void testScriptAtFieldInitializers() throws Exception {
             HtmlTextArea script = scripts.get(scripts.size() - 1);
             String groovy = "build.externalizableId";
             script.setText(groovy);
+            List<HtmlInput> sandboxes = config.getInputsByName("_.sandbox");
+            HtmlCheckBoxInput sandboxcb =  (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1);
+            sandboxcb.setChecked(false);
             // nothing is approved or pending (no save)
             assertThat(ScriptApproval.get().getPendingScripts(), is(empty()));
             assertThat(ScriptApproval.get().getApprovedScriptHashes(), is(emptyArray()));

diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/JcascTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/JcascTest.java
@@ -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 {
 
@@ -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