Skip to content
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

Remove commons-collections; Replace FastHashMap with ConcurrentHashMap #53

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chrismaeda
Copy link

We needed to eliminate commons-collections 3 for a large corp. due to security concerns.

This replaces FastHashMap with ConcurrentHashMap and removes commons-collections.

FYI We also submitted a PR to commons-beanutils2 to modify that repo to be a commons-collections -free version that can replace beanutils 1.9.4.

@garydgregory
Copy link
Member

-1 due to binary compatibility. Also the POM changes are invalid.

@arturobernalg
Copy link
Member

HI @chrismaeda
You can check if the change brake the binary by running japicmp:cmp
Regards,

@rdifrango
Copy link

I came over to common-validator to make the same change as @chrismaeda as we're similar boat as @chrismaeda that we have a desire to eliminate the usage of commons-collections. I do understand from this that it involves much than just a change to commons-validator.

Is there any plan to upgrade this and/or commons-beanutils in the near-term? Is this something that an outside contributor could help with?

@garydgregory
Copy link
Member

I came over to common-validator to make the same change as @chrismaeda as we're similar boat as @chrismaeda that we have a desire to eliminate the usage of commons-collections. I do understand from this that it involves much than just a change to commons-validator.

Is there any plan to upgrade this and/or commons-beanutils in the near-term? Is this something that an outside contributor could help with?

Yes: We need to get Apache Commons BeanUtils 2 out the door and part of that is folding the "2" types into their supertypes, I think the last remaining one is BeanUtilsBean2. The last time I tried this caused unit test failures. This likely requires study of the BeanUtils internals.

@rdifrango
Copy link

I came over to common-validator to make the same change as @chrismaeda as we're similar boat as @chrismaeda that we have a desire to eliminate the usage of commons-collections. I do understand from this that it involves much than just a change to commons-validator.
Is there any plan to upgrade this and/or commons-beanutils in the near-term? Is this something that an outside contributor could help with?

Yes: We need to get Apache Commons BeanUtils 2 out the door and part of that is folding the "2" types into their supertypes, I think the last remaining one is BeanUtilsBean2. The last time I tried this caused unit test failures. This likely requires study of the BeanUtils internals.

Happy to take a look at that if you point me to the JIRA story and/or any back-story on the work.

@chtompki
Copy link
Member

Not sure we have a jira on this work: https://issues.apache.org/jira/projects/COLLECTIONS/issues/COLLECTIONS-714?filter=allopenissues @garydgregory do you know if there is one...if not, we'll want to get you a jira account @rdifrango: https://selfserve.apache.org/jira-account.html

@garydgregory
Copy link
Member

In general, we have: a user's and developer's mailing list, JIRA, and GitHub. You can search browse email history on https://lists.apache.org/[email protected].

@JonnySchnittger
Copy link

Sorry to resurrect this thread, but has there been any progress or decision on deprecating FastHashMap @garydgregory / @chtompki ?

@garydgregory
Copy link
Member

garydgregory commented Aug 6, 2024

Same answer as before: We cannot break binary compatibility within a major release line. This change requires a major version change to 2.0, which means a package name change (...validator2) and Maven coordinate change; which does not seem worth it IMO. If there were other BC breaking changes required for a future 2.0, then we could talk about that. I don't see a driver for that though.

We can deprecated elements and add new ones though.

@rdifrango
Copy link

@garydgregory, while I understand the desire to keep binary compatibility, as @chrismaeda mentioned, we have a desire to eliminate the use of commons-collections from use within our organization given that it was last released Nov 12, 2015 so there's concern about the age along with the potential new security issues not being patched.

Is there a way @JonnySchnittger and I can help contribute either a patch to the current stream or help release a new version of commons-validator that doesn't depend upon commons-collections?

@chtompki
Copy link
Member

We could always just shade in FastHashMap to maintain binary compatibility. I know that's kinda hacky, but it does give us the ability to eliminate the dependency on comms-collections

@garydgregory
Copy link
Member

Shading is not going to work with JPMS in a software stack that includes both jars due to split packages. Let's not create a jar hell scenario ;-)

The only path forward IMO is to create a Validator 2 that does not depend on this type of Map, and depends on BeanUtils 2. I should create a BU 2.0.0-M1 soon or whenever the remaining "2" classes are pulled up into their parent. See the mailing list.

@alex-nt
Copy link

alex-nt commented Aug 24, 2024

Just to piggyback on top of this and give a bit of bureaucratic context, we also encountered this during a scan (by way of Kafka). I expect more and more companies to hit it once DORA comes closer to go into effect (17 January 2025, as most of them use the same code scanners, and they see this as a potential vulnerability).

@sebbASF
Copy link
Contributor

sebbASF commented Dec 28, 2024

Marked as draft: the PR cannot be applied to the current code line

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.

8 participants