Add groovy SimpleTemplateEngine utility step #162

martinda · 2022-08-22T19:21:29Z

Add a pipeline utility step that calls the groovy SimpleTemplateEngine.

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
[n/a] Link to relevant issues in GitHub or Jira
[n/a] Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

Added the pipeline step, the step execution and the unit tests for success and exception cases.

Completed an automated test for SimpleTemplateEngineStep plugin.

martinda · 2022-08-23T17:12:51Z

@basil can you please review or inform us about the next steps to get this feature accepted? Thanks!

basil · 2022-08-23T18:22:09Z

I am not a maintainer of this plugin.

wjohnson8 · 2022-08-23T20:07:32Z

Do you know who is?

martinda · 2022-08-24T13:59:55Z

@rsandell can you please review this PR?

rsandell · 2022-08-24T18:56:05Z

I'm not sure but it looks like a big security concern here that you could bypass the groovy sandbox with this step as it is implemented now.
You need to add a whole script approval and/or make the template engine run in the sandbox.
Iirc the extended email plugin has the same template support and uses at least one of the alternatives if you need examples of how to do it.

martinda · 2022-08-25T01:07:17Z

I think you are talking about the code in this method. Wow. I had no idea.

What do you mean by "uses at least one of the alternatives"? Is that's the conditional here?

rsandell · 2022-09-01T10:49:25Z

The two alternatives you can implement is the same as any other groovy script in Jenkins; Either run in the Groovy sandbox where each thing that gets run is done against a white/black list. Or it can be run with whole script approval, where the entire script is sent to an administrator to approve before it is allowed to be executed on the controller, if it is already approved it will of course be allowed to run without involving an admin the second time.
You can read more about it here https://www.jenkins.io/doc/book/managing/script-approval/

rsandell · 2022-09-01T10:57:53Z

You don't need to make it as complicated as email-ext. You could make it behave more like pipeline itself with a run in sandbox parameter to the step. Default you run with whole script approval and fail the step if the script is not approved. Or if the parameter is true, then run it in the sandbox. A lot of the complexity in email ext where it tries to run it in the sandbox if not approved is because it was done like that to be as backwards compatible as possible.
In this case you are making a completely new step, so you can define it's behavior as you(/we) wish. And I think the simpler approach with a parameter is better.

And even simpler approach would be to just implement one of them (sandbox or whole script approval) and I would be fine with that as well. It could then be enhanced at a later date with the other if someone wants it.

martinda · 2022-09-09T02:29:45Z

@rsandell I added a runInSandbox input, and code to run in the sandbox.

I am not sure of what I am doing, a code review would be much appreciated.

Also, I would like to do proper testing to prove the sandbox code I wrote is correct. I added TODOs in the tests to indicate what I am missing. Can you please provide advice? Thanks!

rsandell · 2022-09-09T10:09:29Z

...org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStepExecution.java

+                new ProxyWhitelist(Whitelist.all())
+            );
+        } else {
+            renderedTemplate = templateR.make(bindings).toString();


I believe this still allows for running unprotected code in the controller jvm, just set runInSandbox to false and you have bypassed the sandbox and now anyone that can edit a Jenkinsfile or configure a job effectively have administrator privileges. See https://javadoc.jenkins.io/plugin/script-security/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.html

Suggested change

renderedTemplate = templateR.make(bindings).toString();

ScriptApproval.get().using(script, GroovyLanguage.get()); //The code above needs to be reworked so that we can have the script string for this line.

renderedTemplate = templateR.make(bindings).toString();

rsandell · 2022-09-09T10:11:08Z

...org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStepExecution.java

+            FilePath f = ws.child(step.getFile());
+            if (f.exists() && !f.isDirectory()) {
+                try (InputStream is = f.read()) {
+                    template = engine.createTemplate(IOUtils.toString(is, StandardCharsets.UTF_8));


I don't know but it could be that static code in the template (if that is possible) could be run here. Needs to be tested.

Needs to be checked if it is allowed to be run if it is not running in the sandbox.

Suggested change

template = engine.createTemplate(IOUtils.toString(is, StandardCharsets.UTF_8));

String tmpl = IOUtils.toString(is, StandardCharsets.UTF_8);

if (!isRunInSandBox()) {

ScriptApproval.get().configuring(tmpl, GroovyLanguage.get(), ApprovalContext.create().withItem(getContext().get(Item.class)), false);

}

template = engine.createTemplate(tmpl);

Looking at the code for SimpleTemplateEngine it is in createTemplate that the script is parsed, so potentially some static code could run outside of the sandbox, but I am not sure.

Yeah, it is not safe to call createTemplate outside of the sandbox.

It turns out that there is more nuance to this that I forgot. The fixes for SECURITY-2020 mean that at worst, users who try to create very complex templates that use a wide range of Groovy features may get a false-positive RejectedAccessException with the current code that does not have an active sandbox when createTemplate is called.

...ain/java/org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStep.java

...org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStepExecution.java

rsandell · 2022-09-09T10:33:57Z

@daniel-beck @dwnusbaum @Wadeck I would appreciate some help in reviewing this to make sure we are secure introducing this feature.

daniel-beck

I recommend this not be merged unless approved by Jesse or Devin.

daniel-beck · 2022-09-09T11:14:06Z

...org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStepExecution.java

+        } else {
+            renderedTemplate = templateR.make(bindings).toString();
+        }


Users must not be able to opt out of Script Security protection. The only choice they get is:

Whole-script approval

Sandboxing

dwnusbaum

Like Bobby noticed, there are security issues with this PR in its current form.

For what it's worth, I have access to a proprietary Jenkins plugin which encapsulates SimpleTemplateEngine, and in that plugin, the source code of SimpleTemplateEngine itself was copied into the plugin and various methods were modified to interact with the sandbox. I would have to investigate things more carefully to know if SimpleTemplateEngine can be used safely just with external sandbox protection.

dwnusbaum · 2022-09-09T14:04:09Z

...org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStepExecution.java

+            FilePath f = ws.child(step.getFile());
+            if (f.exists() && !f.isDirectory()) {
+                try (InputStream is = f.read()) {
+                    template = engine.createTemplate(IOUtils.toString(is, StandardCharsets.UTF_8));


Yeah, it is not safe to call createTemplate outside of the sandbox.

martinda · 2022-09-09T20:30:34Z

@dwnusbaum What would be really helpful for me is have a test scenario that verifies that an unapproved signature is effectively rejected at run time. I am not sure how to do that. Can you give me some advice by looking at my unit test code please? Thanks!

dwnusbaum · 2022-09-09T22:12:32Z

@martinda The problems here are kind of obscure because SimpleTemplateEngine interacts directly with APIs like GroovyShell.parse and Script.run, and I am not super familiar with SimpleTemplateEngine in the first place, so I cannot easily provide a test scenario off the top of my head. It will probably be a few days or so before I have time to look into this further.

martinda · 2022-09-09T23:28:19Z

Sorry for the noise, I pushed a commit too soon and forced it back. Thanks for all your help so far. I am still working on it.

martinda · 2022-09-10T18:25:13Z

If I understand correctly, I should be following the guidelines here and I should pass the "user written template" as a SecureGroovyScript. I should also put the script up for approval with ScriptApproval.get().configuring(...), then I need to check if it has been approved with ScriptApproval.get().isScriptApproved(...). Would that be a good start?

martinda · 2022-09-12T13:52:21Z

I am completely overwhelmed by what needs to be done here. It looks like I have to learn a LOT of things. I am trying to convert the text input to a SecureGroovyScript input, which takes me to re-write the descriptor... next I am finding myself diving deep into how to use the CustomDescribableModel and I am wondering... am I getting lost?

dwnusbaum · 2022-09-12T13:58:17Z

@martinda In this case, you cannot use SecureGroovyScript, because you are not just directly running a user-provided Groovy script, you are running it as part of SimpleTemplateEngine. You need to follow the recommendations in "The hard way" section. That will fix the issue that Daniel noted, and then the other issue is that you probably need to call GroovySandbox.enter in other places, for example around createTemplate. In either case, I will try to take a deeper look in a few days to give you more specific recommendations.

dwnusbaum

Sorry it took so long, but I added some comments. I think that right now, the template is not being sandbox-transformed at all.

As a general note, I am somewhat apprehensive of steps like this. I think that in most cases, users would be better off applying templates on a build agent using their templating tool of choice in a sh or bat step. Users who really like SimpleTemplateEngine or one of Groovy's many templating engines could use the withGroovy step from jenkinsci/groovy-plugin#22 to simplify things compared to a plain sh step.

That kind of approach prevents the controller from needing to parse and run the template, which means that security concerns become irrelevant, and it means that we do not have to maintain another instance of embedded Groovy scripting in Jenkins that is slightly different from other instances and may have unique security issues.

dwnusbaum · 2022-09-26T20:41:28Z

...ain/java/org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStep.java

+ */
+public class SimpleTemplateEngineStep extends AbstractFileOrTextStep {
+
+    protected Map<String, Object> bindings;


Map<String, Object> is not a legal data binding type. It works fine in the actual Pipeline script today when using workflow-cps, but it will not work with tools like the Snippet Generator or other things that analyze steps reflectively (e.g. the step documentation generator, or the display of the step's arguments in the Pipeline Steps view, Blue Ocean, or similar plugins). See jenkinsci/structs-plugin#52 (comment) and other comments in that thread.

The "correct" option would be to introduce a Describable type that has a String name and MyDescribable value parameter where MyDescribable is a second Describable type with subclasses for all types values you want to support (unless you just want to support String). That would add quite a bit of complexity though. In practice many steps have parameters with illegal types and IDK how much users care about the issues that creates.

dwnusbaum · 2022-09-26T20:52:06Z

...ain/java/org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStep.java

+public class SimpleTemplateEngineStep extends AbstractFileOrTextStep {
+
+    protected Map<String, Object> bindings;
+    protected boolean runInSandbox;


IDK if it even makes sense to provide this as an option. I would probably just always use the sandbox. Users with very complex templates where the sandbox causes problems should probably just use a sh step to run whatever templating tool they want on an agent.

Otherwise, you will have to interact with ScriptApproval in any code path where sandbox is false, which adds some complexity. In this case you would need to follow the instructions in The Hard Way because this is not a standard case.

dwnusbaum · 2022-09-26T20:59:11Z

...org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStepExecution.java

+            FilePath f = ws.child(step.getFile());
+            if (f.exists() && !f.isDirectory()) {
+                try (InputStream is = f.read()) {
+                    template = engine.createTemplate(IOUtils.toString(is, StandardCharsets.UTF_8));


It turns out that there is more nuance to this that I forgot. The fixes for SECURITY-2020 mean that at worst, users who try to create very complex templates that use a wide range of Groovy features may get a false-positive RejectedAccessException with the current code that does not have an active sandbox when createTemplate is called.

dwnusbaum · 2022-09-26T21:03:57Z

...org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStepExecution.java

+            throw new IllegalArgumentException(Messages.SimpleTemplateEngineStepExecution_missingBindings(fName));
+        }
+
+        SimpleTemplateEngine engine = new SimpleTemplateEngine();


The basic constructor here will create a brand-new GroovyShell with a fresh GroovyClassLoader, which is unsafe. You need to use GroovySandbox.createSecureCompilerConfiguration and GroovySandbox. createSecureClassLoader to create a secure GroovyShell and pass that to the constructor.

I think that right now, the template is not sandbox-transformed at all, so even basic exploits like <% out.print(System.getProperties()) %> would not be blocked.

I think also that the straightforward code may introduce memory leaks for users with complex templates. Addressing that would require copying a lot of code from SecureGroovyScript (all of the cleanup* methods, but we would probably want to just create an API since at that point it would exist in 3 places (workflow-cps has similar code))

dwnusbaum · 2022-09-26T21:08:54Z

...org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStepExecution.java

+            renderedTemplate = GroovySandbox.runInSandbox(
+                () -> {
+                    return templateR.make(bindings).toString();
+                },
+                new ProxyWhitelist(Whitelist.all())
+            );


In this case, the preferred APIs to use would be something like this:

try (GroovySandbox.Scope scope = new GroovySandbox() .withWhitelist(new ProxyWhitelist( new ClassLoaderWhitelist(... the result of GroovySandbox.createSecureClassLoader up above ...), Whitelist.all()) .enter()) { return templateR.make(bindings).toString(); }

dwnusbaum · 2022-09-26T21:11:38Z

...java/org/jenkinsci/plugins/pipeline/utility/steps/template/SimpleTemplateEngineStepTest.java

+        j.assertLogContains("hudson.remoting.ProxyException: groovy.lang.MissingPropertyException: No such property: wrong for class: SimpleTemplateScript", run);
+    }
+
+    // TODO: The next test needs input stimulus that only works when runInSandbox is true


Typically this should be something that the sandbox prevents, e.g. have the template be <% out.print(System.getProperties) %> or <% Jenkins.getInstance() %>, and then assert that the build fails and check the log messages for a RejectedAccessException for the relevant method.

martinda · 2022-09-29T15:45:20Z

Thank you for all the comments and advice. I have worked on it quite a bit and it looks very different now. I will try to push my changes as soon as I can.

martinda · 2022-09-29T16:02:06Z

Users who really like SimpleTemplateEngine or one of Groovy's many templating engines could use the withGroovy step from jenkinsci/groovy-plugin#22 to simplify things compared to a plain sh step.

I tried the following:

node() {
    String text = "<%= value %>"
    Map binding = ["value":"hello"]
    String result = withGroovy {
        def engine = new groovy.text.SimpleTemplateEngine()
        return engine.createTemplate(text).make(binding).toString()
    }
    echo(result)
}

But I get the same problems as reported here: https://issues.jenkins.io/browse/JENKINS-38432

As shown on this Stackoverflow entry, Many people attempt to use SimpleTemplateEngine, and it would save me time if it just worked. This is why I wanted to create this step.

Again thank you very much for all your advice and comments. I will try to make this PR better.

daniel-beck · 2022-09-29T17:13:50Z

I tried the following:

This won't work since the block content is still evaluated in the Pipeline. It's basically a convenient wrapper block to make Groovy available on the PATH for shell/batch steps. See what the tests are doing for usage examples:

https://github.com/jenkinsci/groovy-plugin/pull/22/files#diff-965daf852d92fd8a6c1372216241b76e3dda319a52f160d84849265552e6fca7R39-R40
https://github.com/jenkinsci/groovy-plugin/pull/22/files#diff-965daf852d92fd8a6c1372216241b76e3dda319a52f160d84849265552e6fca7R64-R65

martinda · 2022-11-24T15:41:10Z

The simpler solution is to use the Groovy StreamingTemplateEngine as it does not cause problems.

martinda and others added 5 commits August 22, 2022 14:00

Add SimpleTemplateEngineStep

bcbf851

Completed an automated test for SimpleTemplateEngineStep plugin.

5609430

Merge pull request #1 from wjohnson8/test/template

769c2ab

Completed an automated test for SimpleTemplateEngineStep plugin.

Minor fixes

564f327

Fixed Windows test error.

64cdabc

Add runInSandbox and added tests

c9f2218

rsandell requested changes Sep 9, 2022

View reviewed changes

rsandell requested review from daniel-beck, dwnusbaum and Wadeck September 9, 2022 10:32

daniel-beck requested changes Sep 9, 2022

View reviewed changes

Fix java bean boolean getter

ecf90cc

dwnusbaum suggested changes Sep 9, 2022

View reviewed changes

martinda force-pushed the groovy-simple-template-engine branch from 982240b to ecf90cc Compare September 9, 2022 23:18

dwnusbaum reviewed Sep 26, 2022

View reviewed changes

martinda closed this Nov 24, 2022

	renderedTemplate = templateR.make(bindings).toString();
	ScriptApproval.get().using(script, GroovyLanguage.get()); //The code above needs to be reworked so that we can have the script string for this line.
	renderedTemplate = templateR.make(bindings).toString();

-                    template = engine.createTemplate(IOUtils.toString(is, StandardCharsets.UTF_8));
+                    String tmpl = IOUtils.toString(is, StandardCharsets.UTF_8);
+                    if (!isRunInSandBox()) {
+                        ScriptApproval.get().configuring(tmpl, GroovyLanguage.get(), ApprovalContext.create().withItem(getContext().get(Item.class)), false);
+                    }
+                    template = engine.createTemplate(tmpl);

Add groovy SimpleTemplateEngine utility step #162

Add groovy SimpleTemplateEngine utility step #162

Conversation

martinda commented Aug 22, 2022

martinda commented Aug 23, 2022

basil commented Aug 23, 2022

wjohnson8 commented Aug 23, 2022

martinda commented Aug 24, 2022

rsandell commented Aug 24, 2022

martinda commented Aug 25, 2022

rsandell commented Sep 1, 2022

rsandell commented Sep 1, 2022

martinda commented Sep 9, 2022

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

Choose a reason for hiding this comment

rsandell commented Sep 9, 2022

daniel-beck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinda commented Sep 9, 2022

dwnusbaum commented Sep 9, 2022

martinda commented Sep 9, 2022

martinda commented Sep 10, 2022

martinda commented Sep 12, 2022

dwnusbaum commented Sep 12, 2022

dwnusbaum left a comment • edited

Choose a reason for hiding this comment

dwnusbaum Sep 26, 2022 • edited

Choose a reason for hiding this comment

dwnusbaum Sep 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum Sep 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinda commented Sep 29, 2022

martinda commented Sep 29, 2022

daniel-beck commented Sep 29, 2022

martinda commented Nov 24, 2022

dwnusbaum left a comment •

edited

dwnusbaum Sep 26, 2022 •

edited

dwnusbaum Sep 26, 2022 •

edited

dwnusbaum Sep 26, 2022 •

edited