-
Notifications
You must be signed in to change notification settings - Fork 1.7k
AMBARI-26209: Upgrade vulnerable commons-collections 3.2.x #3981
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.
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
base: trunk
Are you sure you want to change the base?
Conversation
|
+1 precommit failing due to AMBARI-26454 Changing title because Jenkins couldn't find it: Will open another jira to track the error. |
jojochuang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we're switching to commons-collections4, a different package name, I'd suggest to change the variable name.
ambari-agent/pom.xml
Outdated
|
|
||
| <commons-cli.version>1.5.0</commons-cli.version> | ||
| <commons-collections.version>3.2.2</commons-collections.version> | ||
| <commons-collections.version>4.4</commons-collections.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <commons-collections.version>4.4</commons-collections.version> | |
| <commons-collections4.version>4.4</commons-collections.version> |
ambari-agent/pom.xml
Outdated
| <artifactId>commons-collections</artifactId> | ||
| <groupId>org.apache.commons</groupId> | ||
| <artifactId>commons-collections4</artifactId> | ||
| <version>${commons-collections.version}</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <version>${commons-collections.version}</version> | |
| <version>${commons-collections4.version}</version> |
|
@rich7420 I suggest you deploy a cluster for testing, as this dependency upgrade has a relatively large impact scope. |
OK, I'll try in local. thanks! |
|
Currently, compiling, deploying, and testing the cluster is quite troublesome, and the unit tests are still failing. I will try to find time to fix the unit tests and add the capability for one-click Docker cluster deployment. |
Sure, I can help you work on it as well. Let me know what parts you’d like me to support. |
Could you help fix the failed unit test of ambari server? You can read the Jenkinsfile to find relative test cmds. |
When I tried to test this PR use Once I finish this PR, I’ll work on fixing these issues. |
Merge remote-tracking branch 'origin/trunk' into AMBARI-26209-test
So I removed them.
The reason why it comes is we removed commons-collections3.2.2. It seems from |
|
Every single test failure you mentioned already exist before the PR so if there are no net new failures I think this is good to go. |
jojochuang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.269 s <<< FAILURE! -- in org.apache.ambari.checkstyle.AvoidTransactionalOnPrivateMethodsCheckTest
[ERROR] org.apache.ambari.checkstyle.AvoidTransactionalOnPrivateMethodsCheckTest.transactionalOnPrivateMethod -- Time elapsed: 0.166 s <<< ERROR!
java.lang.NoClassDefFoundError: org/apache/commons/collections/FastHashMap
at org.apache.commons.beanutils.PropertyUtilsBean.getPropertyDescriptor(PropertyUtilsBean.java:964)
at org.apache.commons.beanutils.BeanUtilsBean.copyProperty(BeanUtilsBean.java:391)
at com.puppycrawl.tools.checkstyle.api.AutomaticBean.tryCopyProperty(AutomaticBean.java:229)
at com.puppycrawl.tools.checkstyle.api.AutomaticBean.contextualize(AutomaticBean.java:260)
at com.puppycrawl.tools.checkstyle.Checker.setupChild(Checker.java:458)
at com.puppycrawl.tools.checkstyle.api.AutomaticBean.configure(AutomaticBean.java:199)
at com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport.createChecker(AbstractModuleTestSupport.java:137)
at com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport.createChecker(AbstractModuleTestSupport.java:118)
at com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport.verify(AbstractModuleTestSupport.java:220)
at org.apache.ambari.checkstyle.AvoidTransactionalOnPrivateMethodsCheckTest.transactionalOnPrivateMethod(AvoidTransactionalOnPrivateMethodsCheckTest.java:42)
I think the reason is |
|
checkstyle is a compile time only, not runtime. AvoidTransactionalOnPrivateMethodsCheckTest is a test. I think it's fine to include commons-collections in this case. We should also consider updating checkstyle version. It's very outdated and is associated with a few CVEs too. Consider at least 8.29, and ideally 10.x https://mvnrepository.com/artifact/com.puppycrawl.tools/checkstyle |
|
@rich7420 Thanks for your contribution. |
|
@sandeep318kumar this PR is built on top your work. There were a few remaining version changes required thus this PR. |
What changes were proposed in this pull request?
This pull request addresses a potential security vulnerability by removing the dependency on commons-collections:commons-collections:3.2.2, which is known to have security issues (e.g., CVE-2015-7501). The changes include:
Added exclusion rules for commons-collections:commons-collections in the ambari-server and ambari-funtest modules' pom.xml files to prevent the inclusion of version 3.2.2 via org.apache.directory.api:api-ldap-client-api:2.0.0.AM1.
Ensured all modules use org.apache.commons:commons-collections4:4.4, a secure and modern alternative, either by adding it as a direct dependency where needed or relying on existing declarations.
Updated the parent pom.xml (if applicable) to enforce commons-collections4:4.4 in to prevent future accidental inclusion of older versions.
How was this patch tested?
mvn dependency:tree | grep commons-collectionsto confirm that commons-collections:commons-collections:3.2.2 no longer appears in the dependency tree, and only commons-collections4:4.4 is used.mvn clean installto ensure the project builds successfully without errors.mvn testto verify that all unit testsPlease review Ambari Contributing Guide before opening a pull request.