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

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

Jump to bottom

Open

dwnusbaum wants to merge 4 commits into jenkinsci:master from dwnusbaum:cache-groovysourcefileallowlist

Member

dwnusbaum commented Jan 9, 2024

See #538 (comment).

GroovySourceFileAllowlist can be consulted many times, but the allowed files are effectively static and just depend on which plugins are installed. Once a file has been allowed once, we don't need to check all of the allowlists again.

Testing done

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

dwnusbaum added 2 commits

January 9, 2024 12:54


          Cache GroovySourceFileAllowlist return values

6b66fae

GroovySourceFileAllowlist can be consulted many times, but the allowed files
are effectively static and just depend on which plugins are installed. Once a
file has been allowed once, we don't need to check all of the allowlists again.


          Remove entries from default-allowlist that are unrelated to WithScrip…

7ad5413

…tDescriptor.WithScriptAllowlist

dwnusbaum requested a review from a team as a code owner

January 9, 2024 18:04

dwnusbaum added the internal label

dwnusbaum mentioned this pull request

Make WithScriptAllowlist look up instances of WithScriptDescriptor dynamically instead of registering allowed scripts directly jenkinsci/pipeline-model-definition-plugin#686

Draft

dwnusbaum commented

View reviewed changes

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

    
                  @Extension

                  public static class AllowedGroovyResourcesCache {

                      private final Map<String, Boolean> cache = new ConcurrentHashMap<>();

Member Author

dwnusbaum Jan 9, 2024

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.

Member Author

dwnusbaum Jan 9, 2024

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

Member Author

dwnusbaum Jan 9, 2024 •

edited

Loading

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.)

dwnusbaum commented

View reviewed changes

...ain/resources/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist/default-allowlist

    
              /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

Member Author

dwnusbaum Jan 9, 2024

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

...ain/resources/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist/default-allowlist

Comment on lines -21 to -22

    
              # pipeline-model-extensions

              /org/jenkinsci/plugins/pipeline/modeldefinition/agent/CheckoutScript.groovy

Member Author

dwnusbaum Jan 9, 2024

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

...ain/resources/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist/default-allowlist

    
            @@ -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

Member Author

dwnusbaum Jan 9, 2024

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

...ain/resources/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist/default-allowlist

    
              /org/jenkinsci/plugins/pipeline/modeldefinition/agent/CheckoutScript.groovy

              # docker-workflow

              /org/jenkinsci/plugins/docker/workflow/Docker.groovy

              /org/jenkinsci/plugins/docker/workflow/declarative/AbstractDockerPipelineScript.groovy

Member Author

dwnusbaum Jan 9, 2024

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

...ain/resources/org/jenkinsci/plugins/workflow/cps/GroovySourceFileAllowlist/default-allowlist

    
              # pipeline-model-extensions

              /org/jenkinsci/plugins/pipeline/modeldefinition/agent/CheckoutScript.groovy

              # docker-workflow

              /org/jenkinsci/plugins/docker/workflow/Docker.groovy

Member Author

dwnusbaum Jan 9, 2024

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

jglick approved these changes

View reviewed changes

Member

jglick left a comment

Seems safe enough IIUC.

jglick requested changes

View reviewed changes

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


          Do not cache negative results in GroovySourceFileAllowlist.AllowedGro…

777deff

…ovyResourcesCache

dwnusbaum commented

View reviewed changes

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

    
                  }

                  @Extension

                  @SuppressFBWarnings(value = "NP_BOOLEAN_RETURN_NULL", justification = "intentionally not caching negative results")

Member Author

dwnusbaum Jan 17, 2024 •

edited

Loading

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

jglick approved these changes

View reviewed changes

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


          Merge branch 'master' into cache-groovysourcefileallowlist

4f21ca9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels