From a988f0a00600e7b4bd83decd0239b3ac21eb41c4 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Wed, 16 Oct 2024 13:15:42 +0200 Subject: [PATCH 01/27] JENKINS-73941 - Option to hide "Use Groovy Sandbox" for users without Administer permission globally in the system --- .../scriptsecurity/scripts/ScriptApproval.java | 13 +++++++++++++ .../scriptsecurity/scripts/ScriptApprovalLink.java | 2 +- .../scripts/ScriptApproval/config.jelly | 6 ++++++ .../scripts/ScriptApproval/help-hideSandbox.html | 8 ++++++++ 4 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-hideSandbox.html diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 4a44c4729..95a1641db 100644 --- 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 approvedClasspathEntries; + /** when this mode is enabled, the full logic for accepting/rejecting scripts will be hidden **/ + private boolean hideSandbox; + /* for test */ synchronized void addApprovedClasspathEntry(ApprovedClasspathEntry acp) { approvedClasspathEntries.add(acp); } @@ -982,6 +985,16 @@ public synchronized void setApprovedScriptHashes(String[] scriptHashes) throws I reconfigure(); } + @DataBoundSetter + public synchronized void setHideSandbox(boolean hideSandbox) { + this.hideSandbox = hideSandbox; + save(); + } + + public boolean isHideSandbox() { + return hideSandbox; + } + @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/ScriptApprovalLink.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java index 38e73c99f..6dafb5fac 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java @@ -37,7 +37,7 @@ @Extension public final class ScriptApprovalLink extends ManagementLink { @Override public String getIconFileName() { - if (ScriptApproval.get().isEmpty()) { + if (ScriptApproval.get().isEmpty() || ScriptApproval.get().isHideSandbox()) { return null; } return "symbol-edit-note"; 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 index 43f54ce00..8d5167c4a 100644 --- 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,10 @@ + + + + + + diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-hideSandbox.html b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-hideSandbox.html new file mode 100644 index 000000000..fe20af4f4 --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-hideSandbox.html @@ -0,0 +1,8 @@ +
+

Controls whether the "Use Groovy Sandbox" is shown in the system to users without Overall/Administer permission.

+

This can be used to get a better UX in highly secured environments where all pipelines and other logics are + required to run in the sandbox (ie. running arbitrary code is never approved)

+

Note that this does not prevent users to configure and run pipelines or other logic with sandbox disabled if + they create or update these configurations by other means (like CLI or HTTP API). + This option is only hiding the checkbox from the HTML UI

+
\ No newline at end of file From c090d1c4950dda71e20da36ff50a558485b28298 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Wed, 16 Oct 2024 13:51:35 +0200 Subject: [PATCH 02/27] JENKINS-73941 - Readme --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index 00c335a39..6bf9b3ddc 100644 --- a/README.md +++ b/README.md @@ -85,6 +85,9 @@ 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 +"System Sandbox enablement" allows you to disable 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 From 16716699ac86cdbd4b3b0bbba2773e2123a5aa9a Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Wed, 16 Oct 2024 16:17:23 +0200 Subject: [PATCH 03/27] JENKINS-73941 - Renaming objects + Readme --- README.md | 5 +++-- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 10 +++++----- .../scriptsecurity/scripts/ScriptApprovalLink.java | 2 +- .../scriptsecurity/scripts/ScriptApproval/config.jelly | 2 +- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 6bf9b3ddc..df368f1c8 100644 --- a/README.md +++ b/README.md @@ -85,8 +85,9 @@ 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 -"System Sandbox enablement" allows you to disable the "In-process Script Approval screen". +In case of highly secured environments, where only sandbox scripts are allowed, the +option "Force to use the Sandbox globally in the system" allows forcing the use of the +sandbox globally in the system and will disable the "In-process Script Approval" screen. ### ACL-aware methods Be aware however that even some “getter” methods are designed to check specific diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 95a1641db..26c4b0f6f 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -286,7 +286,7 @@ String hashClasspathEntry(URL entry) throws IOException { private /*final*/ TreeSet approvedClasspathEntries; /** when this mode is enabled, the full logic for accepting/rejecting scripts will be hidden **/ - private boolean hideSandbox; + private boolean forceSandbox; /* for test */ synchronized void addApprovedClasspathEntry(ApprovedClasspathEntry acp) { approvedClasspathEntries.add(acp); @@ -986,13 +986,13 @@ public synchronized void setApprovedScriptHashes(String[] scriptHashes) throws I } @DataBoundSetter - public synchronized void setHideSandbox(boolean hideSandbox) { - this.hideSandbox = hideSandbox; + public synchronized void setforceSandbox(boolean forceSandbox) { + this.forceSandbox = forceSandbox; save(); } - public boolean isHideSandbox() { - return hideSandbox; + public boolean isForceSandbox() { + return forceSandbox; } @Restricted(NoExternalUse.class) // Jelly, tests, implementation diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java index 6dafb5fac..e85909bf2 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java @@ -37,7 +37,7 @@ @Extension public final class ScriptApprovalLink extends ManagementLink { @Override public String getIconFileName() { - if (ScriptApproval.get().isEmpty() || ScriptApproval.get().isHideSandbox()) { + if (ScriptApproval.get().isForceSandbox() || ScriptApproval.get().isEmpty()) { return null; } return "symbol-edit-note"; 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 index 8d5167c4a..7557a8153 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/config.jelly @@ -2,7 +2,7 @@ - + From a42afcacb7a34232f3a38365e6154d4bcb41ba9b Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Wed, 16 Oct 2024 16:42:59 +0200 Subject: [PATCH 04/27] JENKINS-73941 - Block saving Script Approval when Force Sandbox is enabled --- .../scriptsecurity/scripts/ScriptApproval.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 26c4b0f6f..0eead8ae3 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -517,7 +517,9 @@ private PendingClasspathEntry getPendingClasspathEntry(@NonNull String hash) { } /* for test */ void addPendingClasspathEntry(PendingClasspathEntry pcp) { - pendingClasspathEntries.add(pcp); + if(!isForceSandbox()) { + pendingClasspathEntries.add(pcp); + } } @DataBoundConstructor @@ -655,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(!isForceSandbox()) { + pendingScripts.add(new PendingScript(script, language, context)); + } } save(); } @@ -736,7 +740,7 @@ public synchronized void configuring(@NonNull ClasspathEntry entry, @NonNull App approvedClasspathEntries.add(acp); shouldSave = true; } else { - if (pendingClasspathEntries.add(pcp)) { + if (!isForceSandbox() && pendingClasspathEntries.add(pcp)) { LOG.log(Level.FINE, "{0} ({1}) is pending", new Object[] {url, result.newHash}); shouldSave = true; } @@ -787,7 +791,8 @@ 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 (!isForceSandbox() && pendingClasspathEntries.add(new PendingClasspathEntry(result.newHash, url, + context))) { LOG.log(Level.FINE, "{0} ({1}) is pending.", new Object[]{url, result.newHash}); save(); } @@ -891,7 +896,8 @@ 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 && !isForceSandbox() && pendingSignatures.add(new PendingSignature(signature, x.isDangerous(), + context))) { save(); } return x; From b6b32902a18e49f52d4b137272dfdee075c5bab6 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 17 Oct 2024 10:36:31 +0200 Subject: [PATCH 05/27] JENKINS-73941 - Testing --- .../scripts/ScriptApprovalTest.java | 44 +++++++++++++++++++ .../scriptApproval.xml | 11 +++++ .../scriptApproval.xml | 11 +++++ .../forceSandboxAddScript/scriptApproval.xml | 11 +++++ 4 files changed, 77 insertions(+) create mode 100644 src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddClasspath/scriptApproval.xml create mode 100644 src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddPendingSignature/scriptApproval.xml create mode 100644 src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddScript/scriptApproval.xml diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java index 1405c0141..6ffb37d29 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -31,6 +31,7 @@ import hudson.util.VersionNumber; import jenkins.model.Jenkins; import org.hamcrest.Matchers; +import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException; import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist; import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScript; import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.TestGroovyRecorder; @@ -44,6 +45,7 @@ import org.xml.sax.SAXException; import java.io.IOException; +import java.net.URL; import java.util.List; import java.util.concurrent.atomic.AtomicLong; import java.util.logging.Level; @@ -54,6 +56,7 @@ import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; public class ScriptApprovalTest extends AbstractApprovalTest { @@ -195,6 +198,47 @@ public void reload() throws Exception { r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get())); } + @LocalData + @Test + public void forceSandboxAddScript() throws Exception { + configureSecurity(); + ScriptApproval sa = ScriptApproval.get(); + + final ApprovalContext ac = ApprovalContext.create(); + sa.configuring("testScript", GroovyLanguage.get(), ac, true); + + assertTrue(sa.getPendingScripts().isEmpty()); + } + + @LocalData + @Test + public void forceSandboxAddClasspath() throws Exception { + configureSecurity(); + ScriptApproval sa = ScriptApproval.get(); + + final ApprovalContext ac = ApprovalContext.create(); + ClasspathEntry cpe = new ClasspathEntry("https://www.jenkins.io"); + + sa.configuring(cpe, ac); + sa.addPendingClasspathEntry(new ScriptApproval.PendingClasspathEntry("hash", new URL("https://www.jenkins.io"), ac)); + assertThrows(UnapprovedClasspathException.class, () -> sa.using(cpe)); + + //As we are forcing sandbox, none of the previous operations are able to crete new pending ClasspathEntries + assertTrue(sa.getPendingClasspathEntries().isEmpty()); + } + + @LocalData + @Test + public void forceSandboxAddPendingSignature() throws Exception { + configureSecurity(); + ScriptApproval sa = ScriptApproval.get(); + + final ApprovalContext ac = ApprovalContext.create(); + sa.accessRejected(new RejectedAccessException("testSignatureType", "testSignatureDetails"),ac); + + assertTrue(sa.getPendingSignatures().isEmpty()); + } + private Script script(String groovy) { return new Script(groovy); } diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddClasspath/scriptApproval.xml b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddClasspath/scriptApproval.xml new file mode 100644 index 000000000..661381f38 --- /dev/null +++ b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddClasspath/scriptApproval.xml @@ -0,0 +1,11 @@ + + + + + + + true + + + + diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddPendingSignature/scriptApproval.xml b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddPendingSignature/scriptApproval.xml new file mode 100644 index 000000000..661381f38 --- /dev/null +++ b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddPendingSignature/scriptApproval.xml @@ -0,0 +1,11 @@ + + + + + + + true + + + + diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddScript/scriptApproval.xml b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddScript/scriptApproval.xml new file mode 100644 index 000000000..661381f38 --- /dev/null +++ b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddScript/scriptApproval.xml @@ -0,0 +1,11 @@ + + + + + + + true + + + + From 83b33c0d57124226bf8453f935e7e52406122f50 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 17 Oct 2024 11:07:57 +0200 Subject: [PATCH 06/27] JENKINS-73941 - Text Formatting --- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 6 ++---- .../scriptsecurity/scripts/ScriptApproval/config.jelly | 1 - 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 0eead8ae3..9905e99bf 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -791,8 +791,7 @@ public synchronized void using(@NonNull ClasspathEntry entry) throws IOException if (!result.approved) { // Never approve classpath here. ApprovalContext context = ApprovalContext.create(); - if (!isForceSandbox() && pendingClasspathEntries.add(new PendingClasspathEntry(result.newHash, url, - context))) { + if (!isForceSandbox() && pendingClasspathEntries.add(new PendingClasspathEntry(result.newHash, url, context))) { LOG.log(Level.FINE, "{0} ({1}) is pending.", new Object[]{url, result.newHash}); save(); } @@ -896,8 +895,7 @@ public synchronized void preapproveAll() { @Deprecated public synchronized RejectedAccessException accessRejected(@NonNull RejectedAccessException x, @NonNull ApprovalContext context) { String signature = x.getSignature(); - if (signature != null && !isForceSandbox() && pendingSignatures.add(new PendingSignature(signature, x.isDangerous(), - context))) { + if (signature != null && !isForceSandbox() && pendingSignatures.add(new PendingSignature(signature, x.isDangerous(), context))) { save(); } return x; 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 index 7557a8153..278e5c6b2 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/config.jelly @@ -7,4 +7,3 @@ - From 7beb61e8d206baa548f4968b6240f574f417aec9 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 17 Oct 2024 11:12:47 +0200 Subject: [PATCH 07/27] JENKINS-73941 - HideSandbox help improvement --- .../scripts/ScriptApproval/help-hideSandbox.html | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-hideSandbox.html b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-hideSandbox.html index fe20af4f4..614b0e079 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-hideSandbox.html +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-hideSandbox.html @@ -2,7 +2,4 @@

Controls whether the "Use Groovy Sandbox" is shown in the system to users without Overall/Administer permission.

This can be used to get a better UX in highly secured environments where all pipelines and other logics are required to run in the sandbox (ie. running arbitrary code is never approved)

-

Note that this does not prevent users to configure and run pipelines or other logic with sandbox disabled if - they create or update these configurations by other means (like CLI or HTTP API). - This option is only hiding the checkbox from the HTML UI

\ No newline at end of file From c621bd441d464eba5ffbf27161a2b6bea7c4ffd6 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 17 Oct 2024 11:15:26 +0200 Subject: [PATCH 08/27] JENKINS-73941 - Avoid disabling the ScriptApproval screen with forceSandbox mode enabled --- README.md | 3 ++- .../plugins/scriptsecurity/scripts/ScriptApprovalLink.java | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index df368f1c8..7f5bbe10f 100644 --- a/README.md +++ b/README.md @@ -87,7 +87,8 @@ Jenkins jobs) should generally be denied. Most `getSomething` methods are harmle In case of highly secured environments, where only sandbox scripts are allowed, the option "Force to use the Sandbox globally in the system" allows forcing the use of the -sandbox globally in the system and will disable the "In-process Script Approval" screen. +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 diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java index e85909bf2..38e73c99f 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java @@ -37,7 +37,7 @@ @Extension public final class ScriptApprovalLink extends ManagementLink { @Override public String getIconFileName() { - if (ScriptApproval.get().isForceSandbox() || ScriptApproval.get().isEmpty()) { + if (ScriptApproval.get().isEmpty()) { return null; } return "symbol-edit-note"; From 7b0e0ef94ffe4788ecfd0d7396a357d0e4cd7361 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 17 Oct 2024 11:42:15 +0200 Subject: [PATCH 09/27] JENKINS-73941 - Fix the new option 'Force to use the Sandbox globally in the system' help --- .../{help-hideSandbox.html => help-forceSandbox.html} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/{help-hideSandbox.html => help-forceSandbox.html} (100%) diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-hideSandbox.html b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html similarity index 100% rename from src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-hideSandbox.html rename to src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html From e7215e69fc01ae07f5e368a332598c0a14211fed Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Fri, 18 Oct 2024 16:31:59 +0200 Subject: [PATCH 10/27] JENKINS-73941 - New forceSandbox logic does not apply for admin users, disabling it. --- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 9905e99bf..f70b248cb 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -995,8 +995,9 @@ public synchronized void setforceSandbox(boolean forceSandbox) { save(); } + //ForceSandbox restrictions does not apply to ADMINISTER users. public boolean isForceSandbox() { - return forceSandbox; + return forceSandbox && !Jenkins.get().hasPermission(Jenkins.ADMINISTER); } @Restricted(NoExternalUse.class) // Jelly, tests, implementation From 9676a9b4cc608e76029f10fd8d185b446d187e25 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Mon, 21 Oct 2024 11:38:10 +0200 Subject: [PATCH 11/27] JENKINS-73941 - New forceSandbox logic does not apply for admin user + Tests --- .../scripts/ScriptApproval.java | 17 ++-- .../groovy/SecureGroovyScriptTest.java | 84 ++++++++++++++++ .../scripts/ScriptApprovalTest.java | 97 +++++++++++++------ .../scriptApproval.xml | 0 .../scriptApproval.xml | 0 .../scriptApproval.xml | 0 6 files changed, 164 insertions(+), 34 deletions(-) rename src/test/resources/org/jenkinsci/plugins/scriptsecurity/{scripts/ScriptApprovalTest/forceSandboxAddClasspath => sandbox/groovy/SecureGroovyScriptTest/basicApproval_ForceSandbox}/scriptApproval.xml (100%) rename src/test/resources/org/jenkinsci/plugins/scriptsecurity/{scripts/ScriptApprovalTest/forceSandboxAddPendingSignature => sandbox/groovy/SecureGroovyScriptTest/testSandboxDefault_with_ADMINISTER_privs_ForceSandBox}/scriptApproval.xml (100%) rename src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/{forceSandboxAddScript => forceSandboxTests}/scriptApproval.xml (100%) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index f70b248cb..0c2b118ec 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -517,7 +517,7 @@ private PendingClasspathEntry getPendingClasspathEntry(@NonNull String hash) { } /* for test */ void addPendingClasspathEntry(PendingClasspathEntry pcp) { - if(!isForceSandbox()) { + if(!forceSandboxForCurrentUser()) { pendingClasspathEntries.add(pcp); } } @@ -657,7 +657,7 @@ public synchronized String configuring(@NonNull String script, @NonNull Language if (key != null) { pendingScripts.removeIf(pendingScript -> key.equals(pendingScript.getContext().getKey())); } - if(!isForceSandbox()) { + if(!forceSandboxForCurrentUser()) { pendingScripts.add(new PendingScript(script, language, context)); } } @@ -740,7 +740,7 @@ public synchronized void configuring(@NonNull ClasspathEntry entry, @NonNull App approvedClasspathEntries.add(acp); shouldSave = true; } else { - if (!isForceSandbox() && pendingClasspathEntries.add(pcp)) { + if (!forceSandboxForCurrentUser() && pendingClasspathEntries.add(pcp)) { LOG.log(Level.FINE, "{0} ({1}) is pending", new Object[] {url, result.newHash}); shouldSave = true; } @@ -791,7 +791,7 @@ public synchronized void using(@NonNull ClasspathEntry entry) throws IOException if (!result.approved) { // Never approve classpath here. ApprovalContext context = ApprovalContext.create(); - if (!isForceSandbox() && pendingClasspathEntries.add(new PendingClasspathEntry(result.newHash, url, context))) { + if (!forceSandboxForCurrentUser() && pendingClasspathEntries.add(new PendingClasspathEntry(result.newHash, url, context))) { LOG.log(Level.FINE, "{0} ({1}) is pending.", new Object[]{url, result.newHash}); save(); } @@ -895,7 +895,7 @@ public synchronized void preapproveAll() { @Deprecated public synchronized RejectedAccessException accessRejected(@NonNull RejectedAccessException x, @NonNull ApprovalContext context) { String signature = x.getSignature(); - if (signature != null && !isForceSandbox() && pendingSignatures.add(new PendingSignature(signature, x.isDangerous(), context))) { + if (signature != null && !forceSandboxForCurrentUser() && pendingSignatures.add(new PendingSignature(signature, x.isDangerous(), context))) { save(); } return x; @@ -995,8 +995,13 @@ public synchronized void setforceSandbox(boolean forceSandbox) { save(); } - //ForceSandbox restrictions does not apply to ADMINISTER users. + public boolean isForceSandbox() { + return forceSandbox; + } + + //ForceSandbox restrictions does not apply to ADMINISTER users. + public boolean forceSandboxForCurrentUser() { return forceSandbox && !Jenkins.get().hasPermission(Jenkins.ADMINISTER); } diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java index 9518cac43..177c46c0a 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java @@ -29,6 +29,7 @@ import org.htmlunit.html.HtmlInput; import groovy.lang.Binding; import groovy.lang.Script; +import hudson.ExtensionList; import hudson.remoting.Which; import hudson.security.ACLContext; import org.apache.tools.ant.AntClassLoader; @@ -44,6 +45,7 @@ import hudson.model.Result; import hudson.model.User; import hudson.security.ACL; +import hudson.security.GlobalSecurityConfiguration; import hudson.security.Permission; import hudson.tasks.BuildStepDescriptor; import hudson.tasks.Publisher; @@ -89,6 +91,7 @@ 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 org.kohsuke.groovy.sandbox.impl.Checker; import static org.junit.Assert.assertEquals; @@ -1346,6 +1349,87 @@ public void testScriptAtFieldInitializers() throws Exception { } } + /** + * Basic approval test where the user doesn't have ADMINISTER privs but has unchecked the sandbox checkbox. + * As we have enabled the flag ForceSandbox, the script is not send to approval. + */ + @LocalData + @Test public void basicApproval_ForceSandbox() throws Exception { + 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 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); + + // The fact that the user doesn't have RUN_SCRIPT privs means sandbox mode should be on by default. + // We need to switch it off to force it into approval. + List sandboxes = config.getInputsByName("_.sandbox"); + // Get the last one, because previous ones might be from Lockable Resources during PCT. + HtmlCheckBoxInput sandboxRB = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1); + sandboxRB.setChecked(false); // uncheck sandbox mode => forcing script approval + + r.submit(config); + + List publishers = p.getPublishersList(); + assertEquals(1, publishers.size()); + TestGroovyRecorder publisher = (TestGroovyRecorder) publishers.get(0); + assertEquals(groovy, publisher.getScript().getScript()); + assertFalse(publisher.getScript().isSandbox()); + Set pendingScripts = ScriptApproval.get().getPendingScripts(); + assertTrue(ScriptApproval.get().isForceSandbox()); + assertEquals(0, pendingScripts.size()); + } + + /** + * Test where the user has ADMINISTER privs, default to non sandbox mode, but require approval + * ForceSandbox does not apply to admin users, so the logic should behave the same way. + */ + @LocalData + @Test public void testSandboxDefault_with_ADMINISTER_privs_ForceSandBox() throws Exception { + //ForceSandbox does not apply to admin users, so should work in the same way. + testSandboxDefault_with_ADMINISTER_privs(); + + //Checking also the ForceSandbox option is enabled in the system Security configuration + r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); + + MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); + mockStrategy.grant(Jenkins.READ).everywhere().to("admin"); + mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin"); + for (Permission p : Item.PERMISSIONS.getPermissions()) { + mockStrategy.grant(p).everywhere().to("admin"); + } + r.jenkins.setAuthorizationStrategy(mockStrategy); + + JenkinsRule.WebClient wc = r.createWebClient(); + wc.login("admin"); + + HtmlPage page = wc.goTo(ExtensionList.lookupSingleton(GlobalSecurityConfiguration.class).getUrlName()); + HtmlForm config = page.getFormByName("config"); + List sandboxes = config.getInputsByName("_.forceSandbox"); + // Get the last one, because previous ones might be from Lockable Resources during PCT. + HtmlCheckBoxInput forceSandboxCheckBox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1); + assertTrue(forceSandboxCheckBox.isChecked()); + } + + + public static class HasMainMethod { @Whitelisted public HasMainMethod() { } diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java index 6ffb37d29..3ccadd73a 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -27,7 +27,12 @@ import org.htmlunit.html.HtmlPage; import org.htmlunit.html.HtmlTextArea; import hudson.model.FreeStyleProject; +import hudson.model.Item; import hudson.model.Result; +import hudson.model.User; +import hudson.security.ACL; +import hudson.security.ACLContext; +import hudson.security.Permission; import hudson.util.VersionNumber; import jenkins.model.Jenkins; import org.hamcrest.Matchers; @@ -41,6 +46,7 @@ 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.recipes.LocalData; import org.xml.sax.SAXException; @@ -55,6 +61,7 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -200,43 +207,77 @@ public void reload() throws Exception { @LocalData @Test - public void forceSandboxAddScript() throws Exception { - configureSecurity(); - ScriptApproval sa = ScriptApproval.get(); + public void forceSandboxTests() throws Exception { + r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); - final ApprovalContext ac = ApprovalContext.create(); - sa.configuring("testScript", GroovyLanguage.get(), ac, true); + MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); + mockStrategy.grant(Jenkins.READ).everywhere().to("devel"); + for (Permission p : Item.PERMISSIONS.getPermissions()) { + mockStrategy.grant(p).everywhere().to("devel"); + } - assertTrue(sa.getPendingScripts().isEmpty()); - } + mockStrategy.grant(Jenkins.READ).everywhere().to("admin"); + mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin"); + for (Permission p : Item.PERMISSIONS.getPermissions()) { + mockStrategy.grant(p).everywhere().to("admin"); + } - @LocalData - @Test - public void forceSandboxAddClasspath() throws Exception { - configureSecurity(); - ScriptApproval sa = ScriptApproval.get(); + r.jenkins.setAuthorizationStrategy(mockStrategy); - final ApprovalContext ac = ApprovalContext.create(); - ClasspathEntry cpe = new ClasspathEntry("https://www.jenkins.io"); + try (ACLContext ctx = ACL.as(User.getById("devel", true))) { + assertTrue(ScriptApproval.get().isForceSandbox()); + assertTrue(ScriptApproval.get().forceSandboxForCurrentUser()); - sa.configuring(cpe, ac); - sa.addPendingClasspathEntry(new ScriptApproval.PendingClasspathEntry("hash", new URL("https://www.jenkins.io"), ac)); - assertThrows(UnapprovedClasspathException.class, () -> sa.using(cpe)); + //Insert new PendingScript - As the user is not admin and ForceSandbox is enabled, nothing should be added + final ApprovalContext ac = ApprovalContext.create(); - //As we are forcing sandbox, none of the previous operations are able to crete new pending ClasspathEntries - assertTrue(sa.getPendingClasspathEntries().isEmpty()); - } + ScriptApproval.get().configuring("testScript", GroovyLanguage.get(), ac, true); - @LocalData - @Test - public void forceSandboxAddPendingSignature() throws Exception { - configureSecurity(); - ScriptApproval sa = ScriptApproval.get(); + assertTrue(ScriptApproval.get().getPendingScripts().isEmpty()); - final ApprovalContext ac = ApprovalContext.create(); - sa.accessRejected(new RejectedAccessException("testSignatureType", "testSignatureDetails"),ac); + //Insert new PendingSignature - As the user is not admin and ForceSandbox is enabled, nothing should be added + ScriptApproval.get().accessRejected(new RejectedAccessException("testSignatureType", "testSignatureDetails"),ac); - assertTrue(sa.getPendingSignatures().isEmpty()); + assertTrue(ScriptApproval.get().getPendingSignatures().isEmpty()); + + //Insert new Pending ClassPatch - As the user is not admin and ForceSandbox is enabled, nothing should be added + ClasspathEntry cpe = new ClasspathEntry("https://www.jenkins.io"); + + ScriptApproval.get().configuring(cpe, ac); + ScriptApproval.get().addPendingClasspathEntry( + new ScriptApproval.PendingClasspathEntry("hash", new URL("https://www.jenkins.io"), ac)); + assertThrows(UnapprovedClasspathException.class, () -> ScriptApproval.get().using(cpe)); + + //As we are forcing sandbox, none of the previous operations are able to crete new pending ClasspathEntries + assertTrue(ScriptApproval.get().getPendingClasspathEntries().isEmpty()); + } + + try (ACLContext ctx = ACL.as(User.getById("admin", true))) { + assertTrue(ScriptApproval.get().isForceSandbox()); + assertFalse(ScriptApproval.get().forceSandboxForCurrentUser()); + + //Insert new PendingScript - As the user is admin, the behavior does not change + final ApprovalContext ac = ApprovalContext.create(); + + ScriptApproval.get().configuring("testScript", GroovyLanguage.get(), ac, true); + + assertEquals(1, ScriptApproval.get().getPendingScripts().size()); + + //Insert new PendingSignature - - As the user is admin, the behavior does not change + ScriptApproval.get().accessRejected(new RejectedAccessException("testSignatureType", "testSignatureDetails"),ac); + + assertEquals(1, ScriptApproval.get().getPendingSignatures().size()); + + //Insert new Pending ClassPatch - - As the user is admin, the behavior does not change + ClasspathEntry cpe = new ClasspathEntry("https://www.jenkins.io"); + + ScriptApproval.get().configuring(cpe, ac); + ScriptApproval.get().addPendingClasspathEntry( + new ScriptApproval.PendingClasspathEntry("hash", new URL("https://www.jenkins.io"), ac)); + + //As we are forcing sandbox, none of the previous operations are able to crete new pending ClasspathEntries + assertEquals(1, ScriptApproval.get().getPendingClasspathEntries().size()); + } } private Script script(String groovy) { diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddClasspath/scriptApproval.xml b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/basicApproval_ForceSandbox/scriptApproval.xml similarity index 100% rename from src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddClasspath/scriptApproval.xml rename to src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/basicApproval_ForceSandbox/scriptApproval.xml diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddPendingSignature/scriptApproval.xml b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/testSandboxDefault_with_ADMINISTER_privs_ForceSandBox/scriptApproval.xml similarity index 100% rename from src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddPendingSignature/scriptApproval.xml rename to src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/testSandboxDefault_with_ADMINISTER_privs_ForceSandBox/scriptApproval.xml diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddScript/scriptApproval.xml b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxTests/scriptApproval.xml similarity index 100% rename from src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxAddScript/scriptApproval.xml rename to src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxTests/scriptApproval.xml From 2d9c65e2e58f3d29aa55e06405e82f33ade5d4eb Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Mon, 21 Oct 2024 12:55:34 +0200 Subject: [PATCH 12/27] JENKINS-73941 - Additional messages for Sandbox mode --- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 4 +++- .../plugins/scriptsecurity/scripts/ScriptApprovalNote.java | 3 ++- .../plugins/scriptsecurity/scripts/Messages.properties | 5 ++++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 0c2b118ec..8584d57ce 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -822,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(forceSandboxForCurrentUser()? + 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."); diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java index eb03de04b..460ddffa4 100644 --- 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 { public static void print(TaskListener listener, RejectedAccessException x) { try { - String text = Messages.ScriptApprovalNote_message(); + String text = ScriptApproval.get().forceSandboxForCurrentUser()? + 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/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties index 2d79849a4..0b6f696fb 100644 --- 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,10 @@ 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 not supported when running in Sandbox mode. 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 \ No newline at end of file +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 without Sandbox is not allowed in the system" From 7469530d2d949adb291e133715cd9328674af0b2 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Mon, 21 Oct 2024 13:27:26 +0200 Subject: [PATCH 13/27] JENKINS-73941 - New forceSandbox logic - Testing fixing. --- .../groovy/SecureGroovyScriptTest.java | 84 ------------------- .../scriptApproval.xml | 11 --- .../scriptApproval.xml | 11 --- 3 files changed, 106 deletions(-) delete mode 100644 src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/basicApproval_ForceSandbox/scriptApproval.xml delete mode 100644 src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/testSandboxDefault_with_ADMINISTER_privs_ForceSandBox/scriptApproval.xml diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java index 177c46c0a..9518cac43 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java @@ -29,7 +29,6 @@ import org.htmlunit.html.HtmlInput; import groovy.lang.Binding; import groovy.lang.Script; -import hudson.ExtensionList; import hudson.remoting.Which; import hudson.security.ACLContext; import org.apache.tools.ant.AntClassLoader; @@ -45,7 +44,6 @@ import hudson.model.Result; import hudson.model.User; import hudson.security.ACL; -import hudson.security.GlobalSecurityConfiguration; import hudson.security.Permission; import hudson.tasks.BuildStepDescriptor; import hudson.tasks.Publisher; @@ -91,7 +89,6 @@ 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 org.kohsuke.groovy.sandbox.impl.Checker; import static org.junit.Assert.assertEquals; @@ -1349,87 +1346,6 @@ public void testScriptAtFieldInitializers() throws Exception { } } - /** - * Basic approval test where the user doesn't have ADMINISTER privs but has unchecked the sandbox checkbox. - * As we have enabled the flag ForceSandbox, the script is not send to approval. - */ - @LocalData - @Test public void basicApproval_ForceSandbox() throws Exception { - 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 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); - - // The fact that the user doesn't have RUN_SCRIPT privs means sandbox mode should be on by default. - // We need to switch it off to force it into approval. - List sandboxes = config.getInputsByName("_.sandbox"); - // Get the last one, because previous ones might be from Lockable Resources during PCT. - HtmlCheckBoxInput sandboxRB = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1); - sandboxRB.setChecked(false); // uncheck sandbox mode => forcing script approval - - r.submit(config); - - List publishers = p.getPublishersList(); - assertEquals(1, publishers.size()); - TestGroovyRecorder publisher = (TestGroovyRecorder) publishers.get(0); - assertEquals(groovy, publisher.getScript().getScript()); - assertFalse(publisher.getScript().isSandbox()); - Set pendingScripts = ScriptApproval.get().getPendingScripts(); - assertTrue(ScriptApproval.get().isForceSandbox()); - assertEquals(0, pendingScripts.size()); - } - - /** - * Test where the user has ADMINISTER privs, default to non sandbox mode, but require approval - * ForceSandbox does not apply to admin users, so the logic should behave the same way. - */ - @LocalData - @Test public void testSandboxDefault_with_ADMINISTER_privs_ForceSandBox() throws Exception { - //ForceSandbox does not apply to admin users, so should work in the same way. - testSandboxDefault_with_ADMINISTER_privs(); - - //Checking also the ForceSandbox option is enabled in the system Security configuration - r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); - - MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); - mockStrategy.grant(Jenkins.READ).everywhere().to("admin"); - mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin"); - for (Permission p : Item.PERMISSIONS.getPermissions()) { - mockStrategy.grant(p).everywhere().to("admin"); - } - r.jenkins.setAuthorizationStrategy(mockStrategy); - - JenkinsRule.WebClient wc = r.createWebClient(); - wc.login("admin"); - - HtmlPage page = wc.goTo(ExtensionList.lookupSingleton(GlobalSecurityConfiguration.class).getUrlName()); - HtmlForm config = page.getFormByName("config"); - List sandboxes = config.getInputsByName("_.forceSandbox"); - // Get the last one, because previous ones might be from Lockable Resources during PCT. - HtmlCheckBoxInput forceSandboxCheckBox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1); - assertTrue(forceSandboxCheckBox.isChecked()); - } - - - public static class HasMainMethod { @Whitelisted public HasMainMethod() { } diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/basicApproval_ForceSandbox/scriptApproval.xml b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/basicApproval_ForceSandbox/scriptApproval.xml deleted file mode 100644 index 661381f38..000000000 --- a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/basicApproval_ForceSandbox/scriptApproval.xml +++ /dev/null @@ -1,11 +0,0 @@ - - - - - - - true - - - - diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/testSandboxDefault_with_ADMINISTER_privs_ForceSandBox/scriptApproval.xml b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/testSandboxDefault_with_ADMINISTER_privs_ForceSandBox/scriptApproval.xml deleted file mode 100644 index 661381f38..000000000 --- a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest/testSandboxDefault_with_ADMINISTER_privs_ForceSandBox/scriptApproval.xml +++ /dev/null @@ -1,11 +0,0 @@ - - - - - - - true - - - - From 16ccea10a290dbefe118867626e6de4ee7a83f62 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Mon, 21 Oct 2024 13:37:34 +0200 Subject: [PATCH 14/27] JENKINS-73941 - New forceSandbox logic - Messages changing. --- .../plugins/scriptsecurity/scripts/Messages.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties index 0b6f696fb..6e1fdad4c 100644 --- 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,7 @@ 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 not supported when running in Sandbox mode. +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 From 2ff801cd3b96bc47d1236f19cf2c6fcf83f2b38e Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Mon, 21 Oct 2024 13:59:13 +0200 Subject: [PATCH 15/27] JENKINS-73941 - New forceSandbox logic - test improvement --- .../scripts/ScriptApprovalTest.java | 70 ++++++++++--------- .../forceSandboxTests/scriptApproval.xml | 11 --- 2 files changed, 38 insertions(+), 43 deletions(-) delete mode 100644 src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxTests/scriptApproval.xml diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java index 3ccadd73a..85b16e900 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -205,11 +205,12 @@ public void reload() throws Exception { r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get())); } - @LocalData @Test public void forceSandboxTests() throws Exception { r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); + ScriptApproval.get().setforceSandbox(true); + MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); mockStrategy.grant(Jenkins.READ).everywhere().to("devel"); for (Permission p : Item.PERMISSIONS.getPermissions()) { @@ -228,55 +229,60 @@ public void forceSandboxTests() throws Exception { assertTrue(ScriptApproval.get().isForceSandbox()); assertTrue(ScriptApproval.get().forceSandboxForCurrentUser()); - //Insert new PendingScript - As the user is not admin and ForceSandbox is enabled, nothing should be added final ApprovalContext ac = ApprovalContext.create(); - ScriptApproval.get().configuring("testScript", GroovyLanguage.get(), ac, true); - - assertTrue(ScriptApproval.get().getPendingScripts().isEmpty()); + //Insert new PendingScript - As the user is not admin and ForceSandbox is enabled, nothing should be added + { + ScriptApproval.get().configuring("testScript", GroovyLanguage.get(), ac, true); + assertTrue(ScriptApproval.get().getPendingScripts().isEmpty()); + } //Insert new PendingSignature - As the user is not admin and ForceSandbox is enabled, nothing should be added - ScriptApproval.get().accessRejected(new RejectedAccessException("testSignatureType", "testSignatureDetails"),ac); - - assertTrue(ScriptApproval.get().getPendingSignatures().isEmpty()); + { + ScriptApproval.get().accessRejected( + new RejectedAccessException("testSignatureType", "testSignatureDetails"), ac); + assertTrue(ScriptApproval.get().getPendingSignatures().isEmpty()); + } //Insert new Pending ClassPatch - As the user is not admin and ForceSandbox is enabled, nothing should be added - ClasspathEntry cpe = new ClasspathEntry("https://www.jenkins.io"); - - ScriptApproval.get().configuring(cpe, ac); - ScriptApproval.get().addPendingClasspathEntry( - new ScriptApproval.PendingClasspathEntry("hash", new URL("https://www.jenkins.io"), ac)); - assertThrows(UnapprovedClasspathException.class, () -> ScriptApproval.get().using(cpe)); - - //As we are forcing sandbox, none of the previous operations are able to crete new pending ClasspathEntries - assertTrue(ScriptApproval.get().getPendingClasspathEntries().isEmpty()); + { + ClasspathEntry cpe = new ClasspathEntry("https://www.jenkins.io"); + ScriptApproval.get().configuring(cpe, ac); + ScriptApproval.get().addPendingClasspathEntry( + new ScriptApproval.PendingClasspathEntry("hash", new URL("https://www.jenkins.io"), ac)); + assertThrows(UnapprovedClasspathException.class, () -> ScriptApproval.get().using(cpe)); + //As we are forcing sandbox, none of the previous operations are able to crete new pending ClasspathEntries + assertTrue(ScriptApproval.get().getPendingClasspathEntries().isEmpty()); + } } try (ACLContext ctx = ACL.as(User.getById("admin", true))) { assertTrue(ScriptApproval.get().isForceSandbox()); assertFalse(ScriptApproval.get().forceSandboxForCurrentUser()); - //Insert new PendingScript - As the user is admin, the behavior does not change final ApprovalContext ac = ApprovalContext.create(); - ScriptApproval.get().configuring("testScript", GroovyLanguage.get(), ac, true); - - assertEquals(1, ScriptApproval.get().getPendingScripts().size()); + //Insert new PendingScript - As the user is admin, the behavior does not change + { + ScriptApproval.get().configuring("testScript", GroovyLanguage.get(), ac, true); + assertEquals(1, ScriptApproval.get().getPendingScripts().size()); + } //Insert new PendingSignature - - As the user is admin, the behavior does not change - ScriptApproval.get().accessRejected(new RejectedAccessException("testSignatureType", "testSignatureDetails"),ac); - - assertEquals(1, ScriptApproval.get().getPendingSignatures().size()); + { + ScriptApproval.get().accessRejected( + new RejectedAccessException("testSignatureType", "testSignatureDetails"), ac); + assertEquals(1, ScriptApproval.get().getPendingSignatures().size()); + } //Insert new Pending ClassPatch - - As the user is admin, the behavior does not change - ClasspathEntry cpe = new ClasspathEntry("https://www.jenkins.io"); - - ScriptApproval.get().configuring(cpe, ac); - ScriptApproval.get().addPendingClasspathEntry( - new ScriptApproval.PendingClasspathEntry("hash", new URL("https://www.jenkins.io"), ac)); - - //As we are forcing sandbox, none of the previous operations are able to crete new pending ClasspathEntries - assertEquals(1, ScriptApproval.get().getPendingClasspathEntries().size()); + { + ClasspathEntry cpe = new ClasspathEntry("https://www.jenkins.io"); + ScriptApproval.get().configuring(cpe, ac); + ScriptApproval.get().addPendingClasspathEntry( + new ScriptApproval.PendingClasspathEntry("hash", new URL("https://www.jenkins.io"), ac)); + assertEquals(1, ScriptApproval.get().getPendingClasspathEntries().size()); + } } } diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxTests/scriptApproval.xml b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxTests/scriptApproval.xml deleted file mode 100644 index 661381f38..000000000 --- a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/forceSandboxTests/scriptApproval.xml +++ /dev/null @@ -1,11 +0,0 @@ - - - - - - - true - - - - From 93722aab99202584236cf58b1f3f42dd29285c59 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Mon, 21 Oct 2024 16:21:48 +0200 Subject: [PATCH 16/27] JENKINS-73941 - New forceSandbox logic - Messages --- .../plugins/scriptsecurity/scripts/ScriptApprovalNote.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java index 460ddffa4..4abad5980 100644 --- 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,7 @@ public class ScriptApprovalNote extends ConsoleNote { public static void print(TaskListener listener, RejectedAccessException x) { try { - String text = ScriptApproval.get().forceSandboxForCurrentUser()? + 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) { From dc58b0529f98f46b87974229f73c192349a762f3 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Mon, 21 Oct 2024 18:05:15 +0200 Subject: [PATCH 17/27] JENKINS-73941 - New forceSandbox logic - Messages + tests --- .../scripts/UnapprovedUsageException.java | 2 +- .../plugins/scriptsecurity/scripts/Messages.properties | 4 +++- .../sandbox/groovy/SecureGroovyScriptTest.java | 10 +++++++++- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/UnapprovedUsageException.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/UnapprovedUsageException.java index bfb7a8a87..bc02ac56e 100644 --- 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/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties index 6e1fdad4c..15ba6818f 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties @@ -8,4 +8,6 @@ ScriptApprovalLink.outstandingSignature={0} signatures pending approval ScriptApprovalLink.outstandingClasspath={0} classpath entries pending approval 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 without Sandbox is not allowed in the system" +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/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java index 9518cac43..93ffb83bf 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java +++ b/src/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; @@ -224,7 +225,14 @@ 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(Messages.UnapprovedUsage_NonApproved(), ex.getMessage()); + + ScriptApproval.get().setforceSandbox(true); + ex = assertThrows(UnapprovedUsageException.class, + () -> ScriptApproval.get().using(groovy,GroovyLanguage.get())); + assertEquals(Messages.UnapprovedUsage_ForceSandBox(), ex.getMessage()); } /** From 661356d8dc9433b7d41c424a9579ca445e5257ad Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Tue, 22 Oct 2024 11:16:45 +0200 Subject: [PATCH 18/27] JENKINS-73941 - New forceSandbox logic - Messages + tests --- .../scripts/Messages.properties | 8 ++--- .../scripts/ScriptApprovalTest.java | 34 +++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties index 15ba6818f..4e7a1b5c0 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties @@ -7,7 +7,7 @@ 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 -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" +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/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java index 85b16e900..b9a5a0c38 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -26,6 +26,7 @@ import org.htmlunit.html.HtmlPage; import org.htmlunit.html.HtmlTextArea; +import hudson.model.FreeStyleBuild; import hudson.model.FreeStyleProject; import hudson.model.Item; import hudson.model.Result; @@ -34,6 +35,7 @@ import hudson.security.ACLContext; import hudson.security.Permission; import hudson.util.VersionNumber; +import hudson.util.FormValidation; import jenkins.model.Jenkins; import org.hamcrest.Matchers; import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException; @@ -286,6 +288,38 @@ public void forceSandboxTests() throws Exception { } } + @Test + public void forceSandboxScriptSignatureException() throws Exception { + ScriptApproval.get().setforceSandbox(true); + FreeStyleProject p = r.createFreeStyleProject("p"); + p.getPublishersList().add(new TestGroovyRecorder(new SecureGroovyScript("jenkins.model.Jenkins.instance", true, null))); + FreeStyleBuild b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get()); + r.assertLogContains("Scripts not permitted to use staticMethod jenkins.model.Jenkins getInstance. " + Messages.ScriptApprovalNoteForceSandBox_message(), b); + } + + @Test + public void forceSandboxFormValidation() throws Exception { + r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); + r.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy(). + grant(Jenkins.READ, Item.READ).everywhere().to("dev")); + + try (ACLContext ctx = ACL.as(User.getById("devel", true))) { + ScriptApproval.get().setforceSandbox(true); + { + FormValidation result = ScriptApproval.get().checking("test", GroovyLanguage.get(), false); + assertEquals(FormValidation.Kind.WARNING, result.kind); + assertEquals(Messages.ScriptApproval_ForceSandBoxMessage(), result.getMessage()); + } + + ScriptApproval.get().setforceSandbox(false); + { + FormValidation result = ScriptApproval.get().checking("test", GroovyLanguage.get(), false); + assertEquals(FormValidation.Kind.WARNING, result.kind); + assertEquals(Messages.ScriptApproval_PipelineMessage(), result.getMessage()); + } + } + } + private Script script(String groovy) { return new Script(groovy); } From 4cbf8321c66b6837a332b307d82c681858f296c5 Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Tue, 22 Oct 2024 13:15:02 +0200 Subject: [PATCH 19/27] JENKINS-73941 - New forceSandbox logic - Add CASC support + tests --- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 2 +- .../sandbox/groovy/SecureGroovyScriptTest.java | 2 +- .../plugins/scriptsecurity/scripts/JcascTest.java | 2 ++ .../scriptsecurity/scripts/ScriptApprovalTest.java | 8 ++++---- .../plugins/scriptsecurity/scripts/smoke_test.yaml | 1 + .../scriptsecurity/scripts/smoke_test_expected.yaml | 1 + 6 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 8584d57ce..81ce24eae 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -992,7 +992,7 @@ public synchronized void setApprovedScriptHashes(String[] scriptHashes) throws I } @DataBoundSetter - public synchronized void setforceSandbox(boolean forceSandbox) { + public synchronized void setForceSandbox(boolean forceSandbox) { this.forceSandbox = forceSandbox; save(); } diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java index 93ffb83bf..2dfdbc90e 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java @@ -229,7 +229,7 @@ private void addPostBuildAction(HtmlPage page) throws IOException { () -> ScriptApproval.get().using(groovy,GroovyLanguage.get())); assertEquals(Messages.UnapprovedUsage_NonApproved(), ex.getMessage()); - ScriptApproval.get().setforceSandbox(true); + ScriptApproval.get().setForceSandbox(true); ex = assertThrows(UnapprovedUsageException.class, () -> ScriptApproval.get().using(groovy,GroovyLanguage.get())); assertEquals(Messages.UnapprovedUsage_ForceSandBox(), ex.getMessage()); diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/JcascTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/JcascTest.java index 67763a443..59b9fd11f 100644 --- 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 diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java index b9a5a0c38..b93d54c7b 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -211,7 +211,7 @@ public void reload() throws Exception { public void forceSandboxTests() throws Exception { r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); - ScriptApproval.get().setforceSandbox(true); + ScriptApproval.get().setForceSandbox(true); MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); mockStrategy.grant(Jenkins.READ).everywhere().to("devel"); @@ -290,7 +290,7 @@ public void forceSandboxTests() throws Exception { @Test public void forceSandboxScriptSignatureException() throws Exception { - ScriptApproval.get().setforceSandbox(true); + ScriptApproval.get().setForceSandbox(true); FreeStyleProject p = r.createFreeStyleProject("p"); p.getPublishersList().add(new TestGroovyRecorder(new SecureGroovyScript("jenkins.model.Jenkins.instance", true, null))); FreeStyleBuild b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get()); @@ -304,14 +304,14 @@ public void forceSandboxFormValidation() throws Exception { grant(Jenkins.READ, Item.READ).everywhere().to("dev")); try (ACLContext ctx = ACL.as(User.getById("devel", true))) { - ScriptApproval.get().setforceSandbox(true); + ScriptApproval.get().setForceSandbox(true); { FormValidation result = ScriptApproval.get().checking("test", GroovyLanguage.get(), false); assertEquals(FormValidation.Kind.WARNING, result.kind); assertEquals(Messages.ScriptApproval_ForceSandBoxMessage(), result.getMessage()); } - ScriptApproval.get().setforceSandbox(false); + ScriptApproval.get().setForceSandbox(false); { FormValidation result = ScriptApproval.get().checking("test", GroovyLanguage.get(), false); assertEquals(FormValidation.Kind.WARNING, result.kind); diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/smoke_test.yaml b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/smoke_test.yaml index 366aa49de..ee6e9902c 100644 --- a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/smoke_test.yaml +++ b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/smoke_test.yaml @@ -4,3 +4,4 @@ security: - method java.net.URI getHost approvedScriptHashes: - fccae58c5762bdd15daca97318e9d74333203106 + forceSandbox: true \ No newline at end of file diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/smoke_test_expected.yaml b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/smoke_test_expected.yaml index 73bc4b2da..2eb11955a 100644 --- a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/smoke_test_expected.yaml +++ b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/scripts/smoke_test_expected.yaml @@ -2,3 +2,4 @@ approvedScriptHashes: - "fccae58c5762bdd15daca97318e9d74333203106" approvedSignatures: - "method java.net.URI getHost" +forceSandbox: true From af7eee14819294ea056f1abc683ca0d8f70dae7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Garc=C3=ADa?= <60674455+jgarciacloudbees@users.noreply.github.com> Date: Thu, 24 Oct 2024 08:57:13 +0200 Subject: [PATCH 20/27] JENKINS-73941 - Apply suggestions from code review Co-authored-by: michael cirioli --- README.md | 2 +- .../plugins/scriptsecurity/scripts/Messages.properties | 4 ++-- .../scriptsecurity/scripts/ScriptApproval/config.jelly | 2 +- .../scripts/ScriptApproval/help-forceSandbox.html | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 7f5bbe10f..8f7b29e9b 100644 --- a/README.md +++ b/README.md @@ -86,7 +86,7 @@ operations to whitelist. Operations which change state of persisted objects (suc 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 to use the Sandbox globally in the system" allows forcing the use of 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. diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties index 4e7a1b5c0..dc0dcce18 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/Messages.properties @@ -8,6 +8,6 @@ ScriptApprovalLink.outstandingSignature={0} signatures pending approval ScriptApprovalLink.outstandingClasspath={0} classpath entries pending approval 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 +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 +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 index 278e5c6b2..a44129b03 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/config.jelly @@ -2,7 +2,7 @@ - + diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html index 614b0e079..24cd92853 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html @@ -1,5 +1,5 @@

Controls whether the "Use Groovy Sandbox" is shown in the system to users without Overall/Administer permission.

-

This can be used to get a better UX in highly secured environments where all pipelines and other logics are +

This can be used to simplify the UX in highly secured environments where all pipelines and other logics are required to run in the sandbox (ie. running arbitrary code is never approved)

\ No newline at end of file From fec6912cd33245f71403d2570439bf96ca9b2cc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Garc=C3=ADa?= <60674455+jgarciacloudbees@users.noreply.github.com> Date: Thu, 24 Oct 2024 13:47:33 +0200 Subject: [PATCH 21/27] JENKINS-73941 - Apply suggestions from code review Co-authored-by: Antonio Muniz --- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 10 +++++----- .../scriptsecurity/scripts/ScriptApprovalNote.java | 4 ++-- .../scripts/UnapprovedUsageException.java | 2 +- .../scriptsecurity/scripts/ScriptApproval/config.jelly | 2 +- .../scripts/ScriptApproval/help-forceSandbox.html | 2 +- .../scriptsecurity/scripts/ScriptApprovalTest.java | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 81ce24eae..470463f24 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -517,7 +517,7 @@ private PendingClasspathEntry getPendingClasspathEntry(@NonNull String hash) { } /* for test */ void addPendingClasspathEntry(PendingClasspathEntry pcp) { - if(!forceSandboxForCurrentUser()) { + if (!forceSandboxForCurrentUser()) { pendingClasspathEntries.add(pcp); } } @@ -657,7 +657,7 @@ public synchronized String configuring(@NonNull String script, @NonNull Language if (key != null) { pendingScripts.removeIf(pendingScript -> key.equals(pendingScript.getContext().getKey())); } - if(!forceSandboxForCurrentUser()) { + if (!forceSandboxForCurrentUser()) { pendingScripts.add(new PendingScript(script, language, context)); } } @@ -822,8 +822,8 @@ public synchronized FormValidation checking(@NonNull String script, @NonNull Lan } if (!Jenkins.get().hasPermission(Jenkins.ADMINISTER)) { - return FormValidation.warningWithMarkup(forceSandboxForCurrentUser()? - Messages.ScriptApproval_ForceSandBoxMessage(): + return FormValidation.warningWithMarkup(forceSandboxForCurrentUser() ? + Messages.ScriptApproval_ForceSandBoxMessage() : Messages.ScriptApproval_PipelineMessage()); } else { if ((ALLOW_ADMIN_APPROVAL_ENABLED && (willBeApproved || ADMIN_AUTO_APPROVAL_ENABLED)) || !Jenkins.get().isUseSecurity()) { @@ -1003,7 +1003,7 @@ public boolean isForceSandbox() { } //ForceSandbox restrictions does not apply to ADMINISTER users. - public boolean forceSandboxForCurrentUser() { + public boolean isForceSandboxForCurrentUser() { return forceSandbox && !Jenkins.get().hasPermission(Jenkins.ADMINISTER); } diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java index 4abad5980..adaa19953 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalNote.java @@ -51,8 +51,8 @@ public class ScriptApprovalNote extends ConsoleNote { public static void print(TaskListener listener, RejectedAccessException x) { try { - String text = ScriptApproval.get().isForceSandbox()? - Messages.ScriptApprovalNoteForceSandBox_message():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 index bc02ac56e..0d9ce627c 100644 --- 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(ScriptApproval.get().isForceSandbox()?Messages.UnapprovedUsage_ForceSandBox():Messages.UnapprovedUsage_NonApproved()); + super(ScriptApproval.get().isForceSandbox() ? Messages.UnapprovedUsage_ForceSandBox() : Messages.UnapprovedUsage_NonApproved()); this.hash = hash; } 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 index a44129b03..1550989f7 100644 --- 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,7 +1,7 @@ - + diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html index 24cd92853..917581ef1 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html @@ -1,5 +1,5 @@

Controls whether the "Use Groovy Sandbox" is shown in the system to users without Overall/Administer permission.

-

This can be used to simplify the UX in highly secured environments where all pipelines and other logics are +

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 (ie. running arbitrary code is never approved)

\ No newline at end of file diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java index b93d54c7b..ce3ffa9cc 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -246,14 +246,14 @@ public void forceSandboxTests() throws Exception { assertTrue(ScriptApproval.get().getPendingSignatures().isEmpty()); } - //Insert new Pending ClassPatch - As the user is not admin and ForceSandbox is enabled, nothing should be added + //Insert new Pending Classpath - As the user is not admin and ForceSandbox is enabled, nothing should be added { ClasspathEntry cpe = new ClasspathEntry("https://www.jenkins.io"); ScriptApproval.get().configuring(cpe, ac); ScriptApproval.get().addPendingClasspathEntry( new ScriptApproval.PendingClasspathEntry("hash", new URL("https://www.jenkins.io"), ac)); assertThrows(UnapprovedClasspathException.class, () -> ScriptApproval.get().using(cpe)); - //As we are forcing sandbox, none of the previous operations are able to crete new pending ClasspathEntries + // As we are forcing sandbox, none of the previous operations are able to create new pending ClasspathEntries assertTrue(ScriptApproval.get().getPendingClasspathEntries().isEmpty()); } } From e7d0edd705ac42060fc63f033e94567337fc560b Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 24 Oct 2024 13:51:53 +0200 Subject: [PATCH 22/27] JENKINS-73941 - Additional changes required from suggestions --- .../scriptsecurity/scripts/ScriptApproval.java | 12 ++++++------ .../scriptsecurity/scripts/ScriptApprovalTest.java | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 470463f24..06b2c0ddd 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -517,7 +517,7 @@ private PendingClasspathEntry getPendingClasspathEntry(@NonNull String hash) { } /* for test */ void addPendingClasspathEntry(PendingClasspathEntry pcp) { - if (!forceSandboxForCurrentUser()) { + if (!isForceSandboxForCurrentUser()) { pendingClasspathEntries.add(pcp); } } @@ -657,7 +657,7 @@ public synchronized String configuring(@NonNull String script, @NonNull Language if (key != null) { pendingScripts.removeIf(pendingScript -> key.equals(pendingScript.getContext().getKey())); } - if (!forceSandboxForCurrentUser()) { + if (!isForceSandboxForCurrentUser()) { pendingScripts.add(new PendingScript(script, language, context)); } } @@ -740,7 +740,7 @@ public synchronized void configuring(@NonNull ClasspathEntry entry, @NonNull App approvedClasspathEntries.add(acp); shouldSave = true; } else { - if (!forceSandboxForCurrentUser() && pendingClasspathEntries.add(pcp)) { + if (!isForceSandboxForCurrentUser() && pendingClasspathEntries.add(pcp)) { LOG.log(Level.FINE, "{0} ({1}) is pending", new Object[] {url, result.newHash}); shouldSave = true; } @@ -791,7 +791,7 @@ public synchronized void using(@NonNull ClasspathEntry entry) throws IOException if (!result.approved) { // Never approve classpath here. ApprovalContext context = ApprovalContext.create(); - if (!forceSandboxForCurrentUser() && 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(); } @@ -822,7 +822,7 @@ public synchronized FormValidation checking(@NonNull String script, @NonNull Lan } if (!Jenkins.get().hasPermission(Jenkins.ADMINISTER)) { - return FormValidation.warningWithMarkup(forceSandboxForCurrentUser() ? + return FormValidation.warningWithMarkup(isForceSandboxForCurrentUser() ? Messages.ScriptApproval_ForceSandBoxMessage() : Messages.ScriptApproval_PipelineMessage()); } else { @@ -897,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 && !forceSandboxForCurrentUser() && pendingSignatures.add(new PendingSignature(signature, x.isDangerous(), context))) { + if (signature != null && !isForceSandboxForCurrentUser() && pendingSignatures.add(new PendingSignature(signature, x.isDangerous(), context))) { save(); } return x; diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java index ce3ffa9cc..b8f1f218e 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -229,7 +229,7 @@ public void forceSandboxTests() throws Exception { try (ACLContext ctx = ACL.as(User.getById("devel", true))) { assertTrue(ScriptApproval.get().isForceSandbox()); - assertTrue(ScriptApproval.get().forceSandboxForCurrentUser()); + assertTrue(ScriptApproval.get().isForceSandboxForCurrentUser()); final ApprovalContext ac = ApprovalContext.create(); @@ -260,7 +260,7 @@ public void forceSandboxTests() throws Exception { try (ACLContext ctx = ACL.as(User.getById("admin", true))) { assertTrue(ScriptApproval.get().isForceSandbox()); - assertFalse(ScriptApproval.get().forceSandboxForCurrentUser()); + assertFalse(ScriptApproval.get().isForceSandboxForCurrentUser()); final ApprovalContext ac = ApprovalContext.create(); From 9907110c2f802ca0eeef8f02ce7c8d23b0ea91ea Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Thu, 24 Oct 2024 17:20:31 +0200 Subject: [PATCH 23/27] JENKINS-73941 - Additional changes required from suggestions --- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index 06b2c0ddd..ecf34d3c4 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -992,7 +992,7 @@ public synchronized void setApprovedScriptHashes(String[] scriptHashes) throws I } @DataBoundSetter - public synchronized void setForceSandbox(boolean forceSandbox) { + public void setForceSandbox(boolean forceSandbox) { this.forceSandbox = forceSandbox; save(); } From 2a9fe7c61db4cfc9d0af45dfa3aad5799f3c4bad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Garc=C3=ADa?= <60674455+jgarciacloudbees@users.noreply.github.com> Date: Fri, 25 Oct 2024 08:59:32 +0200 Subject: [PATCH 24/27] JENKINS-73941 - Apply suggestions from code review Co-authored-by: Jesse Glick --- .../scriptsecurity/scripts/ScriptApproval/config.jelly | 1 - .../scripts/ScriptApproval/help-forceSandbox.html | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) 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 index 1550989f7..2c8047c19 100644 --- 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,3 @@ - diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html index 917581ef1..06275feea 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/help-forceSandbox.html @@ -1,5 +1,5 @@

Controls whether the "Use Groovy Sandbox" is shown in the system to users without Overall/Administer permission.

-

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 (ie. running arbitrary code is never approved)

+

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).

\ No newline at end of file From 4e97712ed732a2bf8019ac65292116c037e5e54c Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Fri, 25 Oct 2024 15:02:53 +0200 Subject: [PATCH 25/27] JENKINS-73941 - Add forceSandbox logic to SecureGroovyScript --- .../sandbox/groovy/SecureGroovyScript.java | 8 +++ .../groovy/SecureGroovyScript/config.jelly | 8 ++- .../groovy/SecureGroovyScriptTest.java | 61 +++++++++++++++++-- 3 files changed, 70 insertions(+), 7 deletions(-) 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 index b3d6b4a92..c831054ad 100644 --- 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/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly index 057504ee7..ad17c3ef8 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly @@ -31,10 +31,12 @@ THE SOFTWARE. - - + + - + diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java index 2dfdbc90e..af30c267a 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java @@ -183,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 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 sandboxes = config.getInputsByName("_.sandbox"); + HtmlCheckBoxInput sandboxcb = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1); + assertTrue(sandboxcb.isChecked()); + + r.submit(config); + + List 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 @@ -227,12 +272,20 @@ private void addPostBuildAction(HtmlPage page) throws IOException { // Test that the script is executable. If it's not, we will get an UnapprovedUsageException Exception ex = assertThrows(UnapprovedUsageException.class, () -> ScriptApproval.get().using(groovy,GroovyLanguage.get())); - assertEquals(Messages.UnapprovedUsage_NonApproved(), ex.getMessage()); + 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); - ex = assertThrows(UnapprovedUsageException.class, - () -> ScriptApproval.get().using(groovy,GroovyLanguage.get())); - assertEquals(Messages.UnapprovedUsage_ForceSandBox(), ex.getMessage()); + testSandboxDefault_with_ADMINISTER_privs(); } /** From aad4d5163ea627564242c25da755f0c50784c6d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Garc=C3=ADa?= <60674455+jgarciacloudbees@users.noreply.github.com> Date: Fri, 25 Oct 2024 16:10:43 +0200 Subject: [PATCH 26/27] JENKINS-73941 - Apply suggestions from code review Co-authored-by: Jesse Glick --- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 2 +- .../sandbox/groovy/SecureGroovyScript/config.jelly | 2 +- .../plugins/scriptsecurity/scripts/ScriptApproval/config.jelly | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java index ecf34d3c4..386198274 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -285,7 +285,7 @@ String hashClasspathEntry(URL entry) throws IOException { /** All external classpath entries allowed used for scripts. */ private /*final*/ TreeSet approvedClasspathEntries; - /** when this mode is enabled, the full logic for accepting/rejecting scripts will be hidden **/ + /** 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) { diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly index ad17c3ef8..221e597ce 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly @@ -33,7 +33,7 @@ THE SOFTWARE. + default="true"/> 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 index 2c8047c19..8d0d0ca76 100644 --- 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,7 +1,7 @@ - + From 64f458436d13691e6b9ecb67200d36e1949523eb Mon Sep 17 00:00:00 2001 From: Javier Garcia Date: Fri, 25 Oct 2024 16:23:19 +0200 Subject: [PATCH 27/27] JENKINS-73941 - test changes after suggestions --- .../sandbox/groovy/SecureGroovyScriptTest.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java index af30c267a..8d99740b9 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java @@ -256,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 sandboxes = config.getInputsByName("_.sandbox"); + HtmlCheckBoxInput sandboxcb = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1); + assertTrue(sandboxcb.isChecked()); + sandboxcb.setChecked(false); + r.submit(config); List publishers = p.getPublishersList(); assertEquals(1, publishers.size()); @@ -1386,6 +1391,9 @@ public void testScriptAtFieldInitializers() throws Exception { HtmlTextArea script = scripts.get(scripts.size() - 1); String groovy = "build.externalizableId"; script.setText(groovy); + List 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()));