Skip to content

Conversation

@rich7420
Copy link
Contributor

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?

  • Dependency Tree Verification: Run mvn dependency:tree | grep commons-collections to confirm that commons-collections:commons-collections:3.2.2 no longer appears in the dependency tree, and only commons-collections4:4.4 is used.
  • Build Tests: Executed mvn clean install to ensure the project builds successfully without errors.
  • Unit Tests: Run mvn test to verify that all unit tests
    Please review Ambari Contributing Guide before opening a pull request.

@jojochuang jojochuang changed the title [Ambari 26209] Upgrade vulnerable commons-collections 3.2.x Ambari 26209. Upgrade vulnerable commons-collections 3.2.x Apr 15, 2025
@jojochuang jojochuang changed the title Ambari 26209. Upgrade vulnerable commons-collections 3.2.x AMBARI-26209: Upgrade vulnerable commons-collections 3.2.x Apr 15, 2025
@jojochuang
Copy link
Contributor

+1 precommit failing due to AMBARI-26454

Changing title because Jenkins couldn't find it:

    ======failed to add link to jira
    Ooops, no jira id found :(
    =============================

Will open another jira to track the error.

Copy link
Contributor

@jojochuang jojochuang left a 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.


<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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<commons-collections.version>4.4</commons-collections.version>
<commons-collections4.version>4.4</commons-collections.version>

<artifactId>commons-collections</artifactId>
<groupId>org.apache.commons</groupId>
<artifactId>commons-collections4</artifactId>
<version>${commons-collections.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<version>${commons-collections.version}</version>
<version>${commons-collections4.version}</version>

@JiaLiangC
Copy link
Contributor

@rich7420 I suggest you deploy a cluster for testing, as this dependency upgrade has a relatively large impact scope.

@rich7420
Copy link
Contributor Author

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

@JiaLiangC
Copy link
Contributor

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.

@rich7420
Copy link
Contributor Author

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.

@JiaLiangC
Copy link
Contributor

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.

@rich7420
Copy link
Contributor Author

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 mvn clean package, the test problems occur:

HeadlessChrome 0.0.0 (Mac OS X 10.15.7) Ambari Web Unit tests test/views/common/controls_view_test App.ServiceConfigRadioButton #onChecked invoked with click "before each" hook for "property value" FAILED
        TypeError: Cannot read properties of undefined (reading 'call')
            at Ember.run.<anonymous> (test/views/common/controls_view_test.js:533:22) 
HeadlessChrome 0.0.0 (Mac OS X 10.15.7) Ambari Web Unit tests test/views/common/controls_view_test App.ServiceConfigRadioButton #onChecked not invoked with click "before each" hook for "property value" FAILED
        TypeError: Cannot read properties of undefined (reading 'call')
            at Ember.run.<anonymous> (test/views/common/controls_view_test.js:533:22)

Once I finish this PR, I’ll work on fixing these issues.

Merge remote-tracking branch 'origin/trunk' into AMBARI-26209-test
@rich7420
Copy link
Contributor Author

rich7420 commented Apr 16, 2025

  1. According to the WARN it occured :
 [WARNING] The following patterns were never triggered in this artifact inclusion filter:
o  'commons-collections:commons-collections'
o  'commons-configuration:commons-configuration'

So I removed them.

  1. Though skipping unit test, I built ambari successfully. But the error comes out when I didn't skip test. like this:
[ERROR] Errors: 
[ERROR]   AvoidTransactionalOnPrivateMethodsCheckTest.transactionalOnPrivateMethod:42->AbstractModuleTestSupport.verify:220->AbstractModuleTestSupport.createChecker:118->AbstractModuleTestSupport.createChecker:137 » NoClassDefFound org/apache/commons/collections/FastHashMap
[ERROR]   AmbariSwaggerReaderTest.swaggerApiThatIsBothTopLevelAndNestedIsCountedAsTopLevel:172 NullPointer Cannot invoke "java.util.Map.keySet()" because the return value of "io.swagger.models.Swagger.getPaths()" is null
[ERROR]   AmbariSwaggerReaderTest.swaggerBasicCase:70 » NoClassDefFound com/sun/jersey/api/core/InjectParam
[ERROR]   AmbariSwaggerReaderTest.swaggerConflictingNestedApis:85 » NoClassDefFound com/sun/jersey/api/core/InjectParam
[ERROR]   AmbariSwaggerReaderTest.swaggerConflictingNestedApisWithBadPreferredParent:139 » NoClassDefFound com/sun/jersey/api/core/InjectParam
[ERROR]   AmbariSwaggerReaderTest.swaggerConflictingNestedApisWithPreferredParent:102 » NoClassDefFound com/sun/jersey/api/core/InjectParam
[ERROR]   AmbariSwaggerReaderTest.swaggerConflictingNestedApisWithSamePreferredParent:120 » NoClassDefFound com/sun/jersey/api/core/InjectParam
[ERROR]   AmbariSwaggerReaderTest.swaggerNestedApisWithOverwrite:158 NullPointer Cannot invoke "java.util.Map.keySet()" because the return value of "io.swagger.models.Swagger.getPaths()" is null

The reason why it comes is we removed commons-collections3.2.2. It seems fromAbstractModuleTestSupport for checkstyle package because AbstractModuleTestSupport depends on FastHashMap(from commons-collections3.2.2).
in https://github.com/apache/ambari/blob/e94d90f5b7f7bee64c87b96508441d954bb94b22/ambari-utility/src/test/java/org/apache/ambari/checkstyle/AvoidTransactionalOnPrivateMethodsCheckTest.java#L24C1-L25C1

@jojochuang
Copy link
Contributor

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.

Copy link
Contributor

@jojochuang jojochuang left a 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)

@rich7420
Copy link
Contributor Author

  1. According to the WARN it occured :
 [WARNING] The following patterns were never triggered in this artifact inclusion filter:
o  'commons-collections:commons-collections'
o  'commons-configuration:commons-configuration'

So I removed them.

  1. Though skipping unit test, I built ambari successfully. But the error comes out when I didn't skip test. like this:
[ERROR] Errors: 
[ERROR]   AvoidTransactionalOnPrivateMethodsCheckTest.transactionalOnPrivateMethod:42->AbstractModuleTestSupport.verify:220->AbstractModuleTestSupport.createChecker:118->AbstractModuleTestSupport.createChecker:137 » NoClassDefFound org/apache/commons/collections/FastHashMap
[ERROR]   AmbariSwaggerReaderTest.swaggerApiThatIsBothTopLevelAndNestedIsCountedAsTopLevel:172 NullPointer Cannot invoke "java.util.Map.keySet()" because the return value of "io.swagger.models.Swagger.getPaths()" is null
[ERROR]   AmbariSwaggerReaderTest.swaggerBasicCase:70 » NoClassDefFound com/sun/jersey/api/core/InjectParam
[ERROR]   AmbariSwaggerReaderTest.swaggerConflictingNestedApis:85 » NoClassDefFound com/sun/jersey/api/core/InjectParam
[ERROR]   AmbariSwaggerReaderTest.swaggerConflictingNestedApisWithBadPreferredParent:139 » NoClassDefFound com/sun/jersey/api/core/InjectParam
[ERROR]   AmbariSwaggerReaderTest.swaggerConflictingNestedApisWithPreferredParent:102 » NoClassDefFound com/sun/jersey/api/core/InjectParam
[ERROR]   AmbariSwaggerReaderTest.swaggerConflictingNestedApisWithSamePreferredParent:120 » NoClassDefFound com/sun/jersey/api/core/InjectParam
[ERROR]   AmbariSwaggerReaderTest.swaggerNestedApisWithOverwrite:158 NullPointer Cannot invoke "java.util.Map.keySet()" because the return value of "io.swagger.models.Swagger.getPaths()" is null

The reason why it comes is we removed commons-collections3.2.2. It seems fromAbstractModuleTestSupport for checkstyle package because AbstractModuleTestSupport depends on FastHashMap(from commons-collections3.2.2). in https://github.com/apache/ambari/blob/e94d90f5b7f7bee64c87b96508441d954bb94b22/ambari-utility/src/test/java/org/apache/ambari/checkstyle/AvoidTransactionalOnPrivateMethodsCheckTest.java#L24C1-L25C1

I think the reason is checkstyle still depends on FastHashMap. I'll try to change the version of checkstyle.

@jojochuang
Copy link
Contributor

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

@sandeep318kumar
Copy link
Contributor

@rich7420 Thanks for your contribution.
Commons collections has been upgraded to 4.4[Ref: https://github.com//pull/3936, AMBARI-26185]. Kindly check existing PR before working. Thank you!

@jonaswu2000
Copy link

@sandeep318kumar this PR is built on top your work. There were a few remaining version changes required thus this PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants