Remove default-allowlist entries for Groovy source files related to Declarative
#538
Conversation
|
Is there a reason this is still in draft? |
Yes, because it should not be released until the downstream PRs are in widespread use on the relevant baseline or else users who update will be unable to use |
|
Currently, for users running Jenkins 2.332.1 or newer:
We can tell users to upgrade things simultaneously in the release notes, but given that this is just a developer-oriented cleanup and the result of not upgrading things simultaneously is pretty severe (even if it is easy to fix once you know what to do), I am inclined to wait another few weeks until at least a majority of eligible users are running the required versions. |
|
As of the August update to https://stats.jenkins.io/, for users running Jenkins 2.332.1 or newer:
I know users don't like upgrading plugins, but I am still somewhat surprised that uptake is so low after 3 months. I am inclined to wait a bit longer for the same reasons as in my last comment. |
|
As of the October update to https://stats.jenkins.io/, for users running Jenkins 2.332.1 or newer:
Maybe that is good enough to go ahead and release this along with a warning in the release notes? It's not really burning a hole in my pocket or anything, so we could keep waiting. |
|
The question which our stats does not really answer is what portion of users update some but not all plugins. |
|
@dwnusbaum ping, should we merge this finally? |
|
Yes I think it should be fine now. |
|
Hello, for information, I got an issue with the kubernetes plugin due to this. Here is the stack trace I have : Re-installing version |
|
@chickenkiller have you neglected to update some other plugins? This PR should only have mattered for people running extremely outdated versions. |
No I updated absolutely everything. The stack trace shows that the kubernetes plugin is not even called in the stack, so I'm a bit puzzled. It seems to use a generic mechanism to load the groovy script stored in its resources. My jenkins version is 2.426.2, deployed via the latest Helm Chart. |
|
@jglick This line is the culprit: https://github.com/jenkinsci/workflow-cps-plugin/pull/538/files#diff-b12766f1439ade0a5b3ad1be7d7e93437350225fc182a52f178c8e75972d2bffL29 |
|
https://github.com/jenkinsci/kubernetes-plugin/blob/4230d0ccd951f44657b16fd8f867defb03180fc6/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesDeclarativeAgentScript.groovy#L30 but hmm it seems that the downstream PRs were forgotten for |
|
I mean, I found no occurrence of loadClass() method inside kubernetes-plugin repo, so I don't know where to override the |
|
I am checking this now. We do cross-check compatibility of plugin updates in |
|
Implementations of |
|
Thank you very much @jglick and @dwnusbaum ! Let me know if I can help testing anything. |
|
CC @Vlatombe and any other @jenkinsci/kubernetes-plugin-developers |
|
|
|
See |
|
@dwnusbaum unless you know what the problem is and how to fix it soon, would you mean filing a temporary revert PR? |
|
Yes, I am looking at this but will file a PR to revert things if I don't know what is happening in the next half hour or so. |
It occurs to me that it might not exercise the right class loader code since it is not using |
|
@chickenkiller If you still have your Jenkins logs from earlier, do you see a message like "Preventing /org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesDeclarativeAgentScript.groovy from being loaded without sandbox protection in . To allow access to this file, add any suffix of its URL to the system property ..."? I tried to reproduce this in a non-test Jenkins instance but was not able to, at least with my simple case. For now I will file a PR to add the scripts for the plugins that depend on Declarative back to the default allowlist. |
@dwnusbaum indeed I get the following message: I haven't spotted the message before, sorry for not communicating. |
|
No luck so far either (jenkinsci/kubernetes-plugin#1494). My only suspicion so far is the use of an imperative idiom in |
Revert "Merge pull request #538 from dwnusbaum/post-SECURITY-359"
|
IDK. While testing I sometimes saw two instances of
I am not sure how performance-sensitive this code is, or how much of a difference that approach would make. The allowlists are only checked for resources that end with |
Or to |
|
@chickenkiller please verify https://github.com/jenkinsci/workflow-cps-plugin/releases/tag/3837.v305192405b_c0 (now on the update center). |
|
@jglick & @dwnusbaum, I confirm the latest version 3837.v305192405b_c0 works and fixes my issue! Thank you very much for your help on the subject and for your amazing reactivity. Jenkins community is awesome! |
|
Good. Unfortunately we remain unable to reproduce the problem and do not have a solid hypothesis regarding its cause, making it hard to know when if ever we could clean up the obsolete entries. |
In 76a7681, we added an allowlist mechanism for loading arbitrary Groovy source files from the classpath. This included a
default-allowlistfile with entries related to various plugins that relied on the previous behavior (typically to implement some kind of DSL). Now that the security fix is out, we can update Declarative to use the new API and remove a lot of the entries indefault-allowlist.We will also need to update
docker-workflowbecause of/org/jenkinsci/plugins/docker/workflow/declarative/AbstractDockerPipelineScript.groovy, butkubernetesandamazon-ecswill be fixed indirectly via Declarative without requiring any code changes.Even once the downstream fixes are merged and released, we should wait a while to merge and release this to avoid unnecessary upgrade complexity for users, so I plan on leaving this in draft state for a few weeks even after downstream changes get released.
Downstream PRs: