[JENKINS-73470] Option to hide "Use Groovy Sandbox" #907

amuniz · 2024-07-17T12:29:33Z

See JENKINS-73470 for details.

Testing done

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

See [JENKINS-73470](https://issues.jenkins.io/browse/JENKINS-73470) for details.

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/config/CPSConfiguration.java

jglick · 2024-07-17T16:07:25Z

...n/resources/org/jenkinsci/plugins/workflow/cps/config/CPSConfiguration/help-showSandbox.html

+    <p>Note that this is not a security configuration, as it does not prevent users to configure and run
+        pipelines in some other ways without using the sandbox.</p>


Enhancements under discusssion:

Throw AbortException if an attempt is made to run a non-sandboxed pipeline.

Throw something from the @DataBoundConstructor if !sandbox.

The first option may not work though, because we allow an admin to define a non-sandboxed pipeline (maybe some special admin/diagnostic job), and we would not know when the build starts who edited it.

Throw something from the @DataBoundConstructor if !sandbox.

It should also check if the current user is an admin (as they are allowed to disable the sandbox), but jobs can be created by other means which do not necesasrily have a user context (job-dsl for example, or CasC for items when using CloudBees CI). So this would not be a complete solution either. So given that there is not a complete solution, I decided to fix the minimum required to solve the UX issue that happens when using the UI.

jobs can be created by other means which do not necessarily have a user context (job-dsl for example)

In this case the person configuring the DSL job (or approving changes to its source code) is effectively a Jenkins admin whom we trust to decide whether to allow the sandbox to be disabled. Checking hasPermission would work uniformly, since SYSTEM would pass.

I've been trying to block item creation/update by failing on the @DataBoundConstructor of CpsFlowDefinition. However UX is horrible, getting Angry Jenkins (no matter what exception type I use - tried with RuntimeException and Failure, same result).

An alternative solution would be to unconditionally set sandbox = true when the right conditions are met (CPSConfiguration.get().isHideSandbox() && !sandbox && !Jenkins.get().hasPermission(Jenkins.ADMINISTER)). But then the sandbox would be silently enabled when non-admin users save a job which had the sandbox disabled previously.

Given all that, I think we should not open that can of worms and just hide the checkbox (current state of the PR).

(Discussed out of band: core might have seen a regression in handling of Failure as of jenkinsci/jenkins#4389. https://github.com/jenkinsci/jenkins/blob/cc97ad8f330aff43f412a95235a5386194803b14/core/src/main/java/hudson/model/Failure.java#L36-L41 is supposed to render https://github.com/jenkinsci/jenkins/blob/cc97ad8f330aff43f412a95235a5386194803b14/core/src/main/resources/hudson/model/AbstractModelObject/error.jelly#L33-L41; that is the whole purpose of this class.)

diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java index aeae1865..153b2352 100644 --- a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java +++ b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java @@ -91,6 +91,7 @@ public class CpsFlowDefinition extends FlowDefinition { this.script = sandbox ? script : ScriptApproval.get().configuring(script, GroovyLanguage.get(), ApprovalContext.create().withCurrentUser().withItemAsKey(req != null ? req.findAncestorObject(Item.class) : null), req == null); this.sandbox = sandbox; + throw new Failure("Cannot save!!"); } private Object readResolve() {

And try to save a pipeline.

Ah, because it is wrapped in Descriptor.newInstance as you can see in the stack trace. I tried Descriptor.FormException, which is supposed to be for this purpose. (In fact we could even revert the code to hide the checkbox, and instead use FormValidation.error—this is arguably better UX.) Unfortunately right now it is getting wrapped by RequestImpl.invokeConstructor then RequestImpl.TypePair.convertJSON. Seems like a Stapler bug 👀

Try jenkinsci/stapler#571 + jenkinsci/jenkins#9495.

Picked jenkinsci/stapler#571 + jenkinsci/jenkins#9495 up. It's working.

However, I just realized that we can't block the create-job CLI as it's not going through the data-bound constructor (and I think the same will happen with the HTTP API).

Yes, both of those would go through readResolve as called by XStream. At least the CLI will treat certain exception types like IllegalArgumentException specially for purposes of error handling, if it matters.

…e it

amuniz · 2024-07-26T10:54:15Z

Build is failing with:

10:51:55  [INFO] Rule 2: org.apache.maven.enforcer.rules.version.RequireJavaVersion passed
10:51:55  [INFO] Restricted to JDK 11 yet org.jenkins-ci.main:websocket-spi:jar:2.470-rc35157.fa_9b_5f84f785:provided contains jenkins/websocket/Provider$Handler.class targeted to JDK 17
10:51:55  [INFO] Restricted to JDK 11 yet org.kohsuke.stapler:stapler-groovy:jar:1891.v4efde02f1509:provided contains org/kohsuke/stapler/jelly/groovy/GroovierJellyScript$AliasMetaMethod.class targeted to JDK 17
10:51:55  [INFO] Restricted to JDK 11 yet org.kohsuke.stapler:stapler-jelly:jar:1891.v4efde02f1509:provided contains org/kohsuke/stapler/framework/adjunct/Adjunct$Kind.class targeted to JDK 17
10:51:55  [INFO] Restricted to JDK 11 yet org.kohsuke.stapler:stapler:jar:1891.v4efde02f1509:provided contains org/apache/commons/fileupload/FileItem$1.class targeted to JDK 17
10:51:55  [INFO] Restricted to JDK 11 yet org.jenkins-ci.main:cli:jar:2.470-rc35157.fa_9b_5f84f785:provided contains hudson/cli/CLI$1.class targeted to JDK 17
10:51:55  [INFO] Restricted to JDK 11 yet org.jenkins-ci.main:jenkins-core:jar:2.470-rc35157.fa_9b_5f84f785:provided contains antlr/ANTLRException.class targeted to JDK 17

Coming from the incremental release of jenkinsci/jenkins#9495 - are incrementals building without JDK 11 release compatibility?

jglick · 2024-07-26T12:02:43Z

Coming from the incremental release of jenkins

Which requires Java 17, so to specify such a jenkins.version you would need to switch the plugin to Java 17 using instructions defined in the parent POM docs (I hope).

amuniz · 2024-07-26T14:36:33Z

How do you usually handle this type of changes? Wait until the change is integrated in a weekly and bump to the weekly here? Or wait until the change is in an LTS? (regarless the required Java version, that's an aside).

jglick · 2024-07-26T14:51:28Z

Neither. Revert the jenkins.version bump, and skip the test unless the version in fact includes the fix. You are not using a new API. Someone with an older core will merely see an angry Jenkins rather than a polite message.

amuniz · 2024-07-26T15:01:23Z

Ready for final review.

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionTest.java

jglick · 2024-07-29T12:47:07Z

Core PR merged.

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionTest.java

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

jglick · 2024-07-29T16:41:18Z

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java

        this(script, false);
    }

    @DataBoundConstructor
-    public CpsFlowDefinition(String script, boolean sandbox) {
+    public CpsFlowDefinition(String script, boolean sandbox) throws Descriptor.FormException {


Note: source-incompatible but binary-compatible.

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/config/CPSConfiguration.java

plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition/config.jelly

jglick · 2024-07-29T16:43:49Z

...n/resources/org/jenkinsci/plugins/workflow/cps/config/CPSConfiguration/help-hideSandbox.html

+<div>
+    <p>Controls whether the "Use Groovy Sandbox" is shown in pipeline jobs configuration page to users without Overall/Administer permission.</p>
+    <p>This can be used to get a better UX in highly secured environments where all pipelines are required to run in the sandbox (ie. running arbitrary code is never approved)</p>
+    <p>Note that this does not prevent users to configure and run pipelines with sandbox disabled if they create or update jobs by other means (like CLI or HTTP API).


So for now we are not touching readResolve, deliberately?

Yes. I don't see how to differentiate from a regular call during start up and a call from a CLI/HTTP API call.

Deserialization during startup would run as SYSTEM, which of course has ADMINISTER; deserialization during a CLI/HTTP call would run as some user authentication.

IMO, we should leave this as is and revisit it separately if/when needed/requested.

jglick · 2024-07-30T20:39:26Z

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java

        StaplerRequest req = Stapler.getCurrentRequest();
        this.script = sandbox ? script : ScriptApproval.get().configuring(script, GroovyLanguage.get(),
                ApprovalContext.create().withCurrentUser().withItemAsKey(req != null ? req.findAncestorObject(Item.class) : null), req == null);
        this.sandbox = sandbox;
+
    }

    private Object readResolve() {


plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition/config.jelly

@jglick

Thanks @jglick for catching it during code review!

amuniz added 2 commits July 17, 2024 14:26

[JENKINS-73470] Option to hide "Use Groovy Sandbox"

25ddbd4

See [JENKINS-73470](https://issues.jenkins.io/browse/JENKINS-73470) for details.

Add license header

9db533b

amuniz requested a review from a team as a code owner July 17, 2024 12:29

jglick reviewed Jul 17, 2024

View reviewed changes

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java Outdated Show resolved Hide resolved

jglick reviewed Jul 17, 2024

View reviewed changes

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/config/CPSConfiguration.java Show resolved Hide resolved

jglick reviewed Jul 17, 2024

View reviewed changes

jglick added the enhancement label Jul 17, 2024

amuniz added 2 commits July 22, 2024 12:05

Always show the sandbox checkbox if it's disabled, so users can enabl…

5b9eccd

…e it

Move the configuration to the security section

c83b5b3

This was referenced Jul 25, 2024

Define HttpResponses.wrap and use from RequestImpl.invokeConstructor jenkinsci/stapler#571

Merged

Tolerate HttpResponse thrown from Descriptor.newInstance jenkinsci/jenkins#9495

Merged

amuniz marked this pull request as draft July 26, 2024 10:27

amuniz added 2 commits July 26, 2024 12:50

Prevent saving a job with sandbox disabled for non-admins

7f93fdc

Merge remote-tracking branch 'origin/master' into JENKINS-73470

f4b03ba

amuniz force-pushed the JENKINS-73470 branch from 2ae1c3d to f4b03ba Compare July 26, 2024 10:51

Revert to 2.426.3 and wit for jenkinsci/jenkins#9495

4d1b8b9

amuniz marked this pull request as ready for review July 26, 2024 15:01

amuniz marked this pull request as draft July 26, 2024 15:38

jglick reviewed Jul 26, 2024

View reviewed changes

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionTest.java Outdated Show resolved Hide resolved

Adapt test behaviour

786eedb

jglick reviewed Jul 29, 2024

View reviewed changes

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionTest.java Outdated Show resolved Hide resolved

@jglick's review

fee6ede

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

amuniz marked this pull request as ready for review July 29, 2024 13:51

jglick reviewed Jul 29, 2024

View reviewed changes

Better symbol and less diff

d0dc3a0

jglick reviewed Jul 30, 2024

View reviewed changes

bugfix: the checkbox was always shown for newly created jobs

418ece3

Thanks @jglick for catching it during code review!

jglick approved these changes Jul 31, 2024

View reviewed changes

jglick merged commit af73b7c into jenkinsci:master Jul 31, 2024
14 checks passed

jglick mentioned this pull request Oct 24, 2024

JENKINS-73941 - Option to hide "Use Groovy Sandbox" for users without Administer permission globally in the system #948

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73470] Option to hide "Use Groovy Sandbox" #907

[JENKINS-73470] Option to hide "Use Groovy Sandbox" #907

amuniz commented Jul 17, 2024 •

edited

Loading

jglick Jul 17, 2024

amuniz Jul 18, 2024

jglick Jul 18, 2024

amuniz Jul 22, 2024

jglick Jul 22, 2024

amuniz Jul 25, 2024

jglick Jul 25, 2024 •

edited

Loading

jglick Jul 25, 2024

amuniz Jul 26, 2024 •

edited

Loading

jglick Jul 26, 2024

amuniz commented Jul 26, 2024

jglick commented Jul 26, 2024

amuniz commented Jul 26, 2024 •

edited

Loading

jglick commented Jul 26, 2024

amuniz commented Jul 26, 2024

jglick commented Jul 29, 2024

jglick Jul 29, 2024

jglick Jul 29, 2024

amuniz Jul 30, 2024

jglick Jul 30, 2024

amuniz Jul 31, 2024

jglick Jul 30, 2024

		<p>Note that this is not a security configuration, as it does not prevent users to configure and run
		pipelines in some other ways without using the sandbox.</p>

[JENKINS-73470] Option to hide "Use Groovy Sandbox" #907

[JENKINS-73470] Option to hide "Use Groovy Sandbox" #907

Conversation

amuniz commented Jul 17, 2024 • edited Loading

Testing done

Submitter checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuniz Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuniz commented Jul 26, 2024

jglick commented Jul 26, 2024

amuniz commented Jul 26, 2024 • edited Loading

jglick commented Jul 26, 2024

amuniz commented Jul 26, 2024

jglick commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuniz commented Jul 17, 2024 •

edited

Loading

jglick Jul 25, 2024 •

edited

Loading

amuniz Jul 26, 2024 •

edited

Loading

amuniz commented Jul 26, 2024 •

edited

Loading