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.

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

Cache GroovySourceFileAllowlist responses and remove entries from the default allowlist that are unrelated to WithScriptDescriptor.WithScriptAllowlist #829

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Cache GroovySourceFileAllowlist responses and remove entries from the default allowlist that are unrelated to WithScriptDescriptor.WithScriptAllowlist #829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b66fae
Cache GroovySourceFileAllowlist return values
dwnusbaum Jan 9, 2024
7ad5413
Remove entries from default-allowlist that are unrelated to WithScrip…
dwnusbaum Jan 9, 2024
777deff
Do not cache negative results in GroovySourceFileAllowlist.AllowedGro…
dwnusbaum Jan 17, 2024
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
31 changes: 24 additions & 7 deletions plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,11 @@
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Enumeration;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.util.SystemProperties;
Expand Down Expand Up @@ -138,12 +139,7 @@ public Enumeration<URL> getResources(String name) throws IOException {

private static boolean isAllowed(URL url) {
String urlString = url.toString();
for (GroovySourceFileAllowlist allowlist : GroovySourceFileAllowlist.all()) {
if (allowlist.isAllowed(urlString)) {
return true;
}
}
return false;
return ExtensionList.lookupSingleton(AllowedGroovyResourcesCache.class).isAllowed(urlString);
}

private static boolean endsWithIgnoreCase(String value, String suffix) {
Expand All @@ -152,6 +148,27 @@ private static boolean endsWithIgnoreCase(String value, String suffix) {
}
}

@Extension
@SuppressFBWarnings(value = "NP_BOOLEAN_RETURN_NULL", justification = "intentionally not caching negative results")
Copy link
Member Author

@dwnusbaum dwnusbaum Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure why, but this annotation actually does have to be on the class. If it's on the isAllowed method, SpotBugs still complains.

public static class AllowedGroovyResourcesCache {
jglick marked this conversation as resolved.
Show resolved Hide resolved
private final Map<String, Boolean> cache = new ConcurrentHashMap<>();
Copy link
Member Author

Choose a reason for hiding this comment

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

At most there would be around 50 entries in the map if you used every single plugin that has Groovy source files, so I don't think we need anything fancy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually IDK if that's right, let me see if we get here with Groovy's speculative class loading as well.

Copy link
Member Author

@dwnusbaum dwnusbaum Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Never mind, we do really only get here for the weird Groovy DSL script files in Jenkins plugins or other attempts to directly load Groovy source files on the classpath. I guess that is obvious in hindsight, otherwise we would see a ton of log spam for otherwise non-existent classes.

(SandboxResolvingClassLoader in script-security on the other hand does see a bunch of class loading attempts for random non-existent classes, but I guess that has to do with overriding loadClass, which we do not do here.)


public boolean isAllowed(String groovySourceFileUrl) {
Boolean cachedResult = cache.computeIfAbsent(groovySourceFileUrl, url -> {
for (GroovySourceFileAllowlist allowlist : GroovySourceFileAllowlist.all()) {
if (allowlist.isAllowed(url)) {
return true;
}
}
// In practice we should only get here with files that are allowed, so we don't cache negative
// results in case it would cause problems with unusual Pipelines that reference Groovy source
// files directly in combination with dynamically installed plugins.
return null;
});
return Boolean.TRUE.equals(cachedResult);
}
}

/**
* Allows Groovy source files used to implement DSLs in plugins that were created before
* {@link GroovySourceFileAllowlist} was introduced.
Expand Down
6 changes: 0 additions & 6 deletions .../resources/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist/default-allowlist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
# This list is ordered from most popular to least popular plugin to minimize performance impact.
# pipeline-model-definition
/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy
Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/jenkinsci/pipeline-model-definition-plugin/blob/f921b4e72c73e4b60564b8f6610f35bf8bbe9720/pipeline-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ModelStepLoader.java#L77-L78

/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/AnyScript.groovy
/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/LabelScript.groovy
/org/jenkinsci/plugins/pipeline/modeldefinition/agent/impl/NoneScript.groovy
/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/AbstractChangelogConditionalScript.groovy
Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/jenkinsci/pipeline-model-definition-plugin/blob/f921b4e72c73e4b60564b8f6610f35bf8bbe9720/pipeline-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/ChangeLogConditional.java#L100-L101

/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/AllOfConditionalScript.groovy
/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/AnyOfConditionalScript.groovy
/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/BranchConditionalScript.groovy
Expand All @@ -18,11 +16,7 @@
/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/NotConditionalScript.groovy
/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/TagConditionalScript.groovy
/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/TriggeredByConditionalScript.groovy
# pipeline-model-extensions
/org/jenkinsci/plugins/pipeline/modeldefinition/agent/CheckoutScript.groovy
Comment on lines -21 to -22
Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/jenkinsci/pipeline-model-definition-plugin/blob/f921b4e72c73e4b60564b8f6610f35bf8bbe9720/pipeline-model-extensions/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/agent/DeclarativeAgent.java#L110-L111

# docker-workflow
/org/jenkinsci/plugins/docker/workflow/Docker.groovy
Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/jenkinsci/docker-workflow-plugin/blob/dc3cf1327f1b8c9967a56aa0e1a1f3f0ff79b57c/src/main/java/org/jenkinsci/plugins/docker/workflow/DockerDSL.java#L58-L59

/org/jenkinsci/plugins/docker/workflow/declarative/AbstractDockerPipelineScript.groovy
Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/jenkinsci/docker-workflow-plugin/blob/dc3cf1327f1b8c9967a56aa0e1a1f3f0ff79b57c/src/main/java/org/jenkinsci/plugins/docker/workflow/declarative/AbstractDockerAgent.java#L133-L134

/org/jenkinsci/plugins/docker/workflow/declarative/DockerPipelineFromDockerfileScript.groovy
/org/jenkinsci/plugins/docker/workflow/declarative/DockerPipelineScript.groovy
# kubernetes
Expand Down
Loading