From 41b2621725581437dfd0bb9dbf3ecd83227de1fa Mon Sep 17 00:00:00 2001 From: "Meiswinkel, Jan SF/HZA-ZC2S" Date: Mon, 27 Nov 2023 12:59:08 +0100 Subject: [PATCH 1/7] fix: auto re-request approval once dismissed [JENKINS-63668] --- .../scripts/ScriptApproval.java | 5 ++- .../scripts/ScriptApprovalTest.java | 39 +++++++++++++++++++ 2 files changed, 43 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 ff5e2f453..2e1c5d34a 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -664,7 +664,10 @@ public synchronized String using(@NonNull String script, @NonNull Language langu } ConversionCheckResult result = checkAndConvertApprovedScript(script, language); if (!result.approved) { - // Probably need not add to pendingScripts, since generally that would have happened already in configuring. + // Usually. this method is called once the job configuration with the script is saved. + // If a script was previously pending and is now deleted, however, it would require to re-configure the job. + // That's why we call it again if it is unapproved in a running job. + this.configuring(script, language, ApprovalContext.create(), false); throw new UnapprovedUsageException(result.newHash); } return script; 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..7ec874b47 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -53,7 +53,11 @@ 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.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; public class ScriptApprovalTest extends AbstractApprovalTest { @@ -67,6 +71,41 @@ public class ScriptApprovalTest extends AbstractApprovalTest Date: Mon, 27 Nov 2023 13:28:27 +0100 Subject: [PATCH 2/7] prevent accidential admin auto-approval --- .../scripts/ScriptApproval.java | 22 ++++++++++++++----- 1 file changed, 17 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 2e1c5d34a..610c74fa7 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -604,7 +604,8 @@ synchronized boolean isEmpty() { pendingClasspathEntries.isEmpty(); } - /** + + /** * Used when someone is configuring a script. * Typically you would call this from a {@link DataBoundConstructor}. * It should also be called from a {@code readResolve} method (which may then simply return {@code this}), @@ -617,15 +618,16 @@ synchronized boolean isEmpty() { * @param language the language in which it is written * @param context any additional information about how where or by whom this is being configured * @param approveIfAdmin indicates whether script should be approved if current user has admin permissions + * @param ignoreAdmin indicates whether auto approval should be ignored, regardless of any configurations. * @return {@code script}, for convenience */ - public synchronized String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context, boolean approveIfAdmin) { + public synchronized String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context, boolean approveIfAdmin, boolean ignoreAdmin) { final ConversionCheckResult result = checkAndConvertApprovedScript(script, language); if (!result.approved) { - if (!Jenkins.get().isUseSecurity() || + if (!Jenkins.get().isUseSecurity() || (ALLOW_ADMIN_APPROVAL_ENABLED && ((Jenkins.getAuthentication2() != ACL.SYSTEM2 && Jenkins.get().hasPermission(Jenkins.ADMINISTER)) - && (ADMIN_AUTO_APPROVAL_ENABLED || approveIfAdmin)))) { + && (ADMIN_AUTO_APPROVAL_ENABLED || approveIfAdmin) && !ignoreAdmin))) { approvedScriptHashes.add(result.newHash); //Pending scripts are not stored with a precalculated hash, so no need to remove any old hashes removePendingScript(result.newHash); @@ -641,6 +643,14 @@ public synchronized String configuring(@NonNull String script, @NonNull Language return script; } + /** + * @deprecated Use {@link #configuring(String, Language, ApprovalContext, boolean, boolean)} instead + */ + @Deprecated + public synchronized String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context, boolean approveIfAdmin) { + return configuring(script, language, context, approveIfAdmin, false); + } + /** * @deprecated Use {@link #configuring(String, Language, ApprovalContext, boolean)} instead */ @@ -667,7 +677,9 @@ public synchronized String using(@NonNull String script, @NonNull Language langu // Usually. this method is called once the job configuration with the script is saved. // If a script was previously pending and is now deleted, however, it would require to re-configure the job. // That's why we call it again if it is unapproved in a running job. - this.configuring(script, language, ApprovalContext.create(), false); + // 'ignoreAdmin' is set to true, so that administrators + // do not accidentally approve scripts when running a job. + this.configuring(script, language, ApprovalContext.create(), false, true); throw new UnapprovedUsageException(result.newHash); } return script; From 59f62e890513d23700823cf22e4043664e492010 Mon Sep 17 00:00:00 2001 From: "Meiswinkel, Jan SF/HZA-ZC2S" Date: Mon, 27 Nov 2023 13:53:01 +0100 Subject: [PATCH 3/7] prettify --- .../scriptsecurity/sandbox/groovy/SecureGroovyScript.java | 3 ++- .../plugins/scriptsecurity/scripts/ScriptApproval.java | 6 +++--- .../plugins/scriptsecurity/scripts/ScriptApprovalTest.java | 3 ++- 3 files changed, 7 insertions(+), 5 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..8e70701ea 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 @@ -141,7 +141,8 @@ public boolean isScriptAutoApprovalEnabled() { public SecureGroovyScript configuring(ApprovalContext context) { calledConfiguring = true; if (!sandbox) { - ScriptApproval.get().configuring(script, GroovyLanguage.get(), context, !StringUtils.equals(this.oldScript, this.script)); + ScriptApproval.get().configuring(script, GroovyLanguage.get(), context, + !StringUtils.equals(this.oldScript, this.script), false); } for (ClasspathEntry entry : getClasspath()) { ScriptApproval.get().configuring(entry, context); 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 610c74fa7..ccae67f73 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java @@ -605,7 +605,7 @@ synchronized boolean isEmpty() { } - /** + /** * Used when someone is configuring a script. * Typically you would call this from a {@link DataBoundConstructor}. * It should also be called from a {@code readResolve} method (which may then simply return {@code this}), @@ -652,11 +652,11 @@ public synchronized String configuring(@NonNull String script, @NonNull Language } /** - * @deprecated Use {@link #configuring(String, Language, ApprovalContext, boolean)} instead + * @deprecated Use {@link #configuring(String, Language, ApprovalContext, boolean, boolean)} instead */ @Deprecated public String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context) { - return this.configuring(script, language, context, false); + return this.configuring(script, language, context, false, false); } /** 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 7ec874b47..2881ef291 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -259,7 +259,8 @@ static final class Script extends Approvable