Skip to content

Use GroovySourceFileAllowlist to adapt to SECURITY-359 changes #256

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.

Sign up for GitHub

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

Merged
dwnusbaum merged 6 commits into from
Jun 20, 2022
Merged

Use GroovySourceFileAllowlist to adapt to SECURITY-359 changes #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5d68fc4
Use GroovySourceFileAllowlist to adapt to SECURITY-359 changes
dwnusbaum May 17, 2022
8ca0bc8
Docker buildkit breaks FromFingerprintStepTest
dwnusbaum May 19, 2022
e5fcd97
Update image used in DeclarativeAgentTest.dockerPullLocalImage to avo…
dwnusbaum May 19, 2022
b0a7634
Simplify resource loading based on upstream review feedback
dwnusbaum May 26, 2022
52add09
Update BOM and pipeline-model-definition
dwnusbaum Jun 16, 2022
089a373
Simplify resource loading based on upstream review feedback
dwnusbaum Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@
<properties>
<revision>1.29</revision>
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.222.4</jenkins.version>
<jenkins.version>2.332.1</jenkins.version>
<java.level>8</java.level>
<pipeline-model-definition-plugin.version>1.8.1</pipeline-model-definition-plugin.version>
<pipeline-model-definition-plugin.version>2.2097.v33db_b_de764b_e</pipeline-model-definition-plugin.version> <!-- TODO: Delete this and related dependencyManagement entries once this version is included in BOM -->
</properties>
<repositories>
<repository>
Expand All @@ -51,12 +51,26 @@
<dependencies>
<dependency>
<groupId>io.jenkins.tools.bom</groupId>
<artifactId>bom-2.222.x</artifactId>
<version>887.vae9c8ac09ff7</version>
<artifactId>bom-2.332.x</artifactId>
<version>1409.v7659b_c072f18</version>
<scope>import</scope>
<type>pom</type>
</dependency>

<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-api</artifactId>
<version>${pipeline-model-definition-plugin.version}</version>
</dependency>
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-extensions</artifactId>
<version>${pipeline-model-definition-plugin.version}</version>
</dependency>
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-stage-tags-metadata</artifactId>
<version>${pipeline-model-definition-plugin.version}</version>
</dependency>
</dependencies>
</dependencyManagement>
<dependencies>
Expand Down Expand Up @@ -116,12 +130,12 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>config-file-provider</artifactId>
<version>2.10.1</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-definition</artifactId>
<version>${pipeline-model-definition-plugin.version}</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
Expand All @@ -143,6 +157,7 @@
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-definition</artifactId>
<version>${pipeline-model-definition-plugin.version}</version>
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/org/jenkinsci/plugins/docker/workflow/DockerDSL.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import hudson.Extension;
import org.jenkinsci.plugins.workflow.cps.CpsScript;
import org.jenkinsci.plugins.workflow.cps.GlobalVariable;
import org.jenkinsci.plugins.workflow.cps.GroovySourceFileAllowlist;

/**
* Something you should <strong>not copy</strong>. Write plain old {@code Step}s and leave it at that.
Expand All @@ -53,4 +54,13 @@
return docker;
}

@Extension
public static class DockerDSLAllowlist extends GroovySourceFileAllowlist {
private final String scriptUrl = DockerDSL.class.getResource("Docker.groovy").toString();

@Override
public boolean isAllowed(String groovyResourceUrl) {
return groovyResourceUrl.equals(scriptUrl);
}
}
}
16 changes: 16 additions & 0 deletions src/main/java/org/jenkinsci/plugins/docker/workflow/declarative/AbstractDockerAgent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@

import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.Nullable;
import hudson.Extension;
import org.jenkinsci.plugins.pipeline.modeldefinition.agent.DeclarativeAgent;
import org.jenkinsci.plugins.pipeline.modeldefinition.options.DeclarativeOption;
import org.jenkinsci.plugins.workflow.cps.GroovySourceFileAllowlist;
import org.kohsuke.stapler.DataBoundSetter;

public abstract class AbstractDockerAgent<D extends AbstractDockerAgent<D>> extends DeclarativeAgent<D> {
Expand Down Expand Up @@ -123,4 +125,18 @@ public boolean reuseRootAgent(Map<String, DeclarativeOption> options) {
return options.get(ContainerPerStage.SYMBOL) != null;
}

/**
* AbstractDockerPipelineScript.groovy is a superclass of the Groovy scripts for subclasses of
* {@link AbstractDockerAgent}, but does not have any direct equivalent Java class, so we just allow it here.
*/
@Extension
public static class ChangelogConditionalScriptAllowlist extends GroovySourceFileAllowlist {
private final String scriptUrl = AbstractDockerAgent.class.getResource("AbstractDockerPipelineScript.groovy").toString();
Copy link

@yaroslavafenkin yaroslavafenkin Jun 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I get it right, that if AbstractDockerPipelineScript is allowed, then all classes that extend it are also allowed?

Copy link
Member Author

@dwnusbaum dwnusbaum Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they have to be allowed individually. However, the concrete classes are all used by subclasses of WithScriptDescribable, which are handled automatically by jenkinsci/pipeline-model-definition-plugin#529. See specifically WithScriptAllowlist: jenkinsci/pipeline-model-definition-plugin#529 (comment).

The reason we need this entry is that without it, the concrete classes are allowed by WithScriptAllowist, but they cannot be instantiated because their superclass AbstractDockerPipelineScript cannot be found.

Copy link

@yaroslavafenkin yaroslavafenkin Jun 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see it now, thanks.


@Override
public boolean isAllowed(String groovyResourceUrl) {
return groovyResourceUrl.equals(scriptUrl);
}
}

}
2 changes: 2 additions & 0 deletions src/test/java/org/jenkinsci/plugins/docker/workflow/FromFingerprintStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,11 @@ public class FromFingerprintStepTest {
String script = "node {\n" +
" sh 'mkdir buildWithFROMArgs'\n" +
" writeFile file: 'Dockerfile', text: '" + dockerFile + "'\n" +
" withEnv(['DOCKER_BUILDKIT=0']) {\n" +
Copy link
Member Author

@dwnusbaum dwnusbaum May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using BuildKit (which IIUC is currently the default), the built image does not list the FROM image as its parent in docker inspect metadata, and the FROM image is not even present in docker image ls -a, so the test doesn't work at all. dockerFingerprintFrom doesn't run by default as of #180, so I could also see an argument for just deleting the whole test file at this point.

" def built = docker.build('my-tag') \n" +
" dockerFingerprintFrom dockerfile: 'Dockerfile', image: 'my-tag' \n" +
" echo \"built ${built.id}\"\n" +
" }\n" +
"}";

assertBuild("build", script, BUSYBOX_IMAGE);
Expand Down
6 changes: 3 additions & 3 deletions ...t/resources/org/jenkinsci/plugins/docker/workflow/declarative/dockerPullLocalImage.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ pipeline {
stages {
stage("build image") {
steps {
sh 'docker build -t maven:3-alpine .'
sh 'docker build -t maven:3-jdk-8-slim .'
}
}
stage("in built image") {
agent {
docker {
image "maven:3-alpine"
image "maven:3-jdk-8-slim"
args "-v /tmp:/tmp"
reuseNode true
}
Expand All @@ -46,7 +46,7 @@ pipeline {
stage("in pulled image") {
agent {
docker {
image "maven:3-alpine"
image "maven:3-jdk-8-slim"
Copy link
Member Author

@dwnusbaum dwnusbaum May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3-alpine hasn't been updated for around 5 years, and for whatever reason mvn hangs in the container running from the pulled image (but not in the container running from the built image). I suspect it has something to do with QEMU emulation since I am using an M1 Mac, but I really have no idea. I switched to 3-jdk-8-slim, which is actively maintained and has native arm64 builds, and the test passes locally now.

alwaysPull true
}
}
Expand Down