[JENKINS-65237] Bump Groovy from 2.4.12 to 2.4.21

[basil]

See JENKINS-65237 and jenkinsci/jenkins#5116 (comment). Upgrades Groovy from 2.4.12 to 2.4.21. Changelogs:

• Groovy 2.4.13
• Groovy 2.4.14
• Groovy 2.4.15
• Groovy 2.4.16
• Groovy 2.4.17
• Groovy 2.4.18
• Groovy 2.4.19
• Groovy 2.4.20
• Groovy 2.4.21

The changelogs and apache/groovy@GROOVY_2_4_12...GROOVY_2_4_21 did not appear to contain anything frightening.

Testing done

• BOM tests pass
• groovy-sandbox tests pass
• groovy-cps tests pass (ran locally)
• Verified that a Pipeline job started before this PR resumes correctly after this PR

Fixes jenkinsci/configuration-as-code-plugin#1703

Proposed changelog entries

Upgrade Groovy from 2.4.12 (released on June 21, 2017) to 2.4.21 (released on November 29, 2020).

Proposed upgrade guidelines

N/A

Submitter checklist

• (If applicable) Jira issue is well described
• Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
  • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
• Appropriate autotests or explanation to why this change has no tests
• For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

• There are at least 2 approvals for the pull request and no outstanding requests for change
• Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
• Changelog entries in the PR title and/or Proposed changelog entries are correct
• Proper changelog labels are set so that the changelog can be generated automatically
• If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
• If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[jglick]

I…guess this is OK. I looked over the changelogs and did not see anything alarming. I scanned through the aggregate diff looking at source files which would be relevant to groovy-sandbox and did not see anything alarming there either, though of course there were a lot of detailed changes that I could not follow. @dwnusbaum is probably in the best position to review this.

[car-roll]

I was looking into this bump a while back and this is what I had found going through the changelogs. Nothing earth shattering, of course this was just changelogs, not actual code changes.
https://issues.jenkins.io/browse/JENKINS-65237

[dwnusbaum]

@dwnusbaum is probably in the best position to review this.

🤷‍♂️ I looked through the changes and everything is probably harmless, but I don't know for sure. There are some bug fixes and subtle changes to method/property selection-related code like apache/groovy@cf51a3f where I think it is pretty hard to tell at a glance whether the issue being fixed is relevant to us without a more detailed investigation, i.e. do groovy-sandbox and groovy-cps intercept the right thing after the fix, or do they at least behave the same way with both versions of Groovy? In general I am not concerned about functional regressions, but about potential security issues due to behavior changes in things that could not have been tested because they did not work previously.

GROOVY-8539 is slightly interesting in that no one could have ever used compound assignment syntax for boolean arrays before the fix, so it has never been tested in groovy-sandbox or groovy-cps, but I did a quick test locally and everything seems to be intercepted as expected with 2.4.21.

GROOVY-8428 appears to be the only new APIs that could be relevant to Jenkins users. I think it would make sense to add them to generic-whitelist in script-security when groovy-sandbox is updated assuming the Jenkins baseline in the plugin is updated as well.

[basil]

do groovy-sandbox and groovy-cps intercept the right thing after the fix, or do they at least behave the same way with both versions of Groovy?

I ran each test case from apache/groovy@cf51a3f through org.kohsuke.groovy.sandbox.TheTest#assertIntercept and com.cloudbees.groovy.cps.sandbox.SandboxInvokerTest#{evalCpsSandbox,assertIntercept} and verified that the return values were the same for both versions of Groovy (modulo the bug fix) and that the intercepted calls were the same for both versions of Groovy.

[dwnusbaum]

I ran each test case from apache/groovy@cf51a3f through org.kohsuke.groovy.sandbox.TheTest#assertIntercept and com.cloudbees.groovy.cps.sandbox.SandboxInvokerTest#{evalCpsSandbox,assertIntercept} and verified that the return values were the same for both versions of Groovy (modulo the bug fix) and that the intercepted calls were the same for both versions of Groovy.

Yes, sorry I should have clarified that I did that as well before I wrote my comment, but it is not obvious to me whether the tests in question cover all relevant code paths, or only cover the most direct code path that shows the difference. Metaprogramming-related Groovy APIs are forbidden in the Jenkins sandbox, so those tests as-is are not really relevant to us. The thing that is not clear to me is whether the change affects otherwise normal attribute access, e.g. super.@x in some cases. IIUC, this is the code path we need to trigger in AsmClassGenerator to potentially exercise the bug/fix, but I was not able to hit it while testing (the only two things I was able to make AsmClassGenerator emit were direct superclass field access and a call to ScriptBytecodeAdapter.invokeMethodOnSuper0 on an automatically-generated getter).

I am not blocking this, all I mean is that I cannot give a meaningfully more detailed review beyond what you and Jesse have already done by looking at the commits and changelogs without spending significantly more time looking through everything and doing some exploratory testing. If everyone thinks that we need a detailed review like that, I can ask my employer whether I can spend a couple of days really looking into this, but otherwise all I can say is that the update seems like it would probably be fine but I'm not completely sure.

[basil]

I am not blocking this, all I mean is that I cannot give a meaningfully more detailed review beyond what you and Jesse have already done by looking at the commits and changelogs without spending significantly more time looking through everything and doing some exploratory testing. If everyone thinks that we need a detailed review like that, I can ask my employer whether I can spend a couple of days really looking into this, but otherwise all I can say is that the update seems like it would probably be fine but I'm not completely sure.

Thanks for the quick reply! I appreciate the concerns that have been raised, but is it possible we might be overthinking this? If there are any issues with this upgrade between now and the next LTS, we can always simply revert this PR. We are at the beginning of a new LTS cycle, so my vote would be to merge this PR and monitor Jira for reported regressions. If issues are discovered, we can revert this PR and then go through the detailed process you are describing. What do you think?

[dwnusbaum]

If there are any issues with this upgrade between now and the next LTS, we can always simply revert this PR

Yeah, I am not really worried about functional regressions, since I think our test coverage is decent for most typical use cases, if some obscure case breaks there's a decent chance that it's something we don't really want people to be doing anyway, and if some common scenario breaks then reverting is no big deal like you said.

My concern is pretty much solely in relation to introducing potential security vulnerabilities, since I think these are extremely unlikely to affect regular usage and so will go unnoticed unless someone is actively looking for them. I don't see anything obviously concerning here, unlike say a 3.x upgrade where there is lots of new syntax that would obviously need to be tested, but the sandbox escapes we have seen can be pretty subtle, so I just want to make sure that I do not imply that I have thoroughly reviewed every single change in the update looking for sandbox-related issues.

[daniel-beck]

the sandbox escapes we have seen can be pretty subtle

I think https://www.jenkins.io/security/advisory/2019-11-21/#SECURITY-1658 or https://www.jenkins.io/security/advisory/2019-09-12/#SECURITY-1538 are examples of that?

[daniel-beck]

2.4.14 adds StringGroovyMethods#startsWithAny and StringGroovyMethods#endsWithAny, do we need to adapt groovy-cps?

[timja]

2.4.14 adds StringGroovyMethods#startsWithAny and StringGroovyMethods#endsWithAny, do we need to adapt groovy-cps?

#5939 (review) says that we can add those to the allow list when bumping the version there but it doesn’t block this

[jglick]

we can add those to the allow list

Well, not just the sandbox whitelist, but the list of DGM methods to process—if anyone ever cares. cloudbees/groovy-cps#115 (comment)

[dwnusbaum]

The new methods don’t have a Closure parameter, so I don’t think there is any reason to translate them.

…

On Thu, Dec 2, 2021 at 09:09 Jesse Glick ***@***.***> wrote: we can add those to the allow list Well, not just the sandbox whitelist, but the list of DGM methods to process—if anyone ever cares. cloudbees/groovy-cps#115 (comment) <cloudbees/groovy-cps#115 (comment)> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5939 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIE7KHEEODNYULYWYEED73UO54XXANCNFSM5IOJ7BJA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[oleg-nenashev]

There is still some risk of setting Pipeline and Script Security on fire again, but the best possible effort was done to prevent regressions. Based on that, I am +1 for merging. Taking lack of blocking negative feedback, I will mark it as ready for merge

We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process