Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-34650] Added a trusted classloader that runs CPS code outside sandbox #33

Merged
merged 8 commits into from Aug 4, 2016

Conversation

Projects
None yet
2 participants
@kohsuke
Copy link
Member

kohsuke commented Jul 26, 2016

JENKINS-34650

See doc/classloader.md for the details.

kohsuke added some commits Jul 26, 2016

Added a trusted classloader that runs CPS code outside sandbox
See doc/classloader.md for more details.
Merge remote-tracking branch 'origin/master' into trusted-classloader
Conflicts:
	src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java
@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jul 26, 2016

This change depends on cloudbees/groovy-cps#36

@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jul 26, 2016

Pushed groovy-cps:1.8-SNAPSHOT. retrying.

@kohsuke kohsuke closed this Jul 26, 2016

@kohsuke kohsuke reopened this Jul 26, 2016

@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jul 26, 2016

I guess there's no easy way to hook up this PR build into CB's snapshot repo.

@kohsuke kohsuke closed this Jul 28, 2016

@kohsuke kohsuke reopened this Jul 28, 2016

@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jul 28, 2016

Deployed SNAPSHOT to repo.jenkins-ci.org now, so we should be able to see the status now.

Scripts loaded in TCL, OTOH, does not live in the security sandbox. This
classloader is meant to be used to load Groovy code packaged inside
plugins and global libraries. Write access to these sources should be
restricted to `RUN_SCRIPT` permission.

This comment has been minimized.

@jglick

jglick Jul 29, 2016

Member

🐜 RUN_SCRIPTS

It's also possible to augument `GroovyClassLoader` classpath via
`addURL()` so that scripts are loaded on-demand whenever needed.
This is more suitable for scripts that are considered a part of the
system, such as global libraries or plugin code.

This comment has been minimized.

@jglick

jglick Jul 29, 2016

Member

What if a build is started, it begins using some global library, then the library is drastically modified, Jenkins is restarted, and the resumed build tries to use code from the same library? Will the build possibly break?

This comment has been minimized.

@kohsuke

kohsuke Aug 4, 2016

Author Member

Added some more text after this to cover that topic.

GroovyClassLoaderWhitelist(GroovyClassLoader scriptLoader, Whitelist delegate) {
this.scriptLoader = scriptLoader;
GroovyClassLoaderWhitelist(Whitelist delegate, GroovyClassLoader... scriptLoaders) {
this.scriptLoaders = Arrays.<ClassLoader>asList(scriptLoaders);

This comment has been minimized.

@jglick

jglick Jul 29, 2016

Member

🐜 Make this an IdentityHashMap<ClassLoader,Void> so that permits is not calling Object.equals, which makes me nervous.

This comment has been minimized.

@kohsuke

kohsuke Aug 4, 2016

Author Member

IdentityHashMap<ClassLoader,Void> obscures the code and I have never seen any funny ClassLoader.equals() implementation, so I don't think it's a problem.

* Obtains a contextualized {@link GroovyShellDecorator} used to decorate the trusted shell.
*
* <p>
* By default, this method returns null decorator that doesn't do anything.

This comment has been minimized.

@jglick

jglick Jul 29, 2016

Member

Not obvious to me that this is a compatible change. In particular, did you try running docker-workflow tests against this?

This comment has been minimized.

@kohsuke

kohsuke Aug 4, 2016

Author Member

Previously there was no trusted classloader, so old plugins like docker-workflow that implements GroovyShellDecorator will continue to insert classpath into the untrusted classloader. So I think this is compatible.

I'll run the docker workflow test to verify.

@Override public void evaluate() throws Throwable {
WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition("new foo().attempt()", true));
WorkflowRun b = story.j.waitForCompletion(p.scheduleBuild2(0).get());

This comment has been minimized.

@jglick

jglick Jul 29, 2016

Member

🐜 the waitForCompletion call is gratuitous.

} else {
// should have failed with RejectedAccessException trying to touch 'SECRET'
story.j.assertBuildStatus(Result.FAILURE, b);
story.j.assertLogContains("RejectedAccessException: Scripts not permitted to use staticField org.jenkinsci.plugins.workflow.cps.CpsFlowExecutionTest SECRET",b);

This comment has been minimized.

@jglick

jglick Jul 29, 2016

Member

🐜 Can do various things to avoid hardcoding this much text here: use CpsFlowExecutionTest.class.getName(); create an actual RejectedAccessException and toString it (IIRC)

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jul 29, 2016

🐝 AFAIUI

in the middle, then the program could fail. (For example, if a program
call stack includes a class from a global library and that class goes away,
then the program fails to survive the restart because the call stack cannot
be deserialized.

This comment has been minimized.

@jglick

jglick Aug 4, 2016

Member

🐜 missing rparen

so if they change while the program is running, with or without Jenkins restarts
in the middle, then the program could fail. (For example, if a program
call stack includes a class from a global library and that class goes away,
then the program fails to survive the restart because the call stack cannot

This comment has been minimized.

@jglick

jglick Aug 4, 2016

Member

…which is a serious flaw IIUC, since the global lib feature makes no attempt to ensure that libraries are not refactored or deleted while builds are running. So its replacement will need to do better than this.

This comment has been minimized.

@kohsuke

kohsuke Aug 4, 2016

Author Member

I consider that a part of another ticket where we support locking in on a specific commit of a global library. In the documentation I'm merely describing the current behaviour.

I don't consider it a "serious flaw" - the same problem happens if you upgrade plugins that incompatibly change the behaviours, etc.

if (pos) {
story.j.assertBuildStatusSuccess(b);
assertTrue(SECRET);
} else {
// should have failed with RejectedAccessException trying to touch 'SECRET'
story.j.assertBuildStatus(Result.FAILURE, b);
story.j.assertLogContains("RejectedAccessException: Scripts not permitted to use staticField org.jenkinsci.plugins.workflow.cps.CpsFlowExecutionTest SECRET",b);
story.j.assertLogContains(
new RejectedAccessException("staticField",CpsFlowExecutionTest.class.getName()+" SECRET").getMessage(),

This comment has been minimized.

@jglick

jglick Aug 4, 2016

Member

🐜 use StaticWhitelist.rejectStaticField to avoid hard-coding the format.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Aug 4, 2016

re🐝

@jglick jglick merged commit 3a380e7 into master Aug 4, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the trusted-classloader branch Aug 4, 2016

@@ -160,7 +160,7 @@
<dependency>
<groupId>com.cloudbees</groupId>
<artifactId>groovy-cps</artifactId>
<version>1.6</version>
<version>1.8-SNAPSHOT</version>

This comment has been minimized.

@jglick

jglick Aug 8, 2016

Member

Unfortunately this pulls in the broken changes in 1.7 reported in JENKINS-34064 which cloudbees/groovy-cps@ae43475 tries to deal with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.