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

Upgrade and Pin Netty 4.1.100.Final #1022

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

apupier
Copy link
Contributor

@apupier apupier commented Oct 17, 2023

  • it is a dependency of Zookeeper
  • Netty 4.1.100.Final contains a fix for important CVE
  • Citrus is then affected but given the context of usage of Citrus with good chance of not being vulnerable
  • Zookeeper has not integrated the fixed version yet
  • Pinning here will avoid having Citrus to be flagged and ease integration for consumers of Citrus

@bbortt
Copy link
Collaborator

bbortt commented Oct 17, 2023

what do you mean by the following?

Pinning here will avoid having Citrus to be flagged and ease integration for consumers of Citrus

Copy link
Member

@christophd christophd left a comment

Choose a reason for hiding this comment

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

Many thanks! I think it would be good to add this into the dependencyManagement section in the root pom.xml and to introduce a new property ${netty.version}

@apupier
Copy link
Contributor Author

apupier commented Oct 17, 2023

Many thanks! I think it would be good to add this into the dependencyManagement section in the root pom.xml

I tried to put it in parent pom in dependency management with an import bom and it wasn't working.
But I can try with the dependencies directly

@apupier apupier force-pushed the nettyUpgrade4.1.100 branch from 4ece231 to 8163b25 Compare October 17, 2023 13:40
Copy link
Member

@christophd christophd left a comment

Choose a reason for hiding this comment

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

LGTM now, thank you

@apupier
Copy link
Contributor Author

apupier commented Oct 17, 2023

what do you mean by the following?

Pinning here will avoid having Citrus to be flagged and ease integration for consumers of Citrus

A new release of netty has been provided mentioning potential security issue https://netty.io/news/2023/10/10/4-1-100-Final.html . The previous versions will surely soon be flagged as affected/vulnerable which will lead Citrus to be flagged too.
Consumers of Citrus will have the same kind of problem.
For now Zookeeper has not integrated a newer version of Netty: apache/zookeeper#2082

@apupier
Copy link
Contributor Author

apupier commented Oct 17, 2023

argh wait, zookeeper is used in citrus-kafka too

- it is a dependency of Zookeeper
- Netty 4.1.100.Final contains a fix for important CVE
- Citrus is then affected but given the context of usage of Citrus with
good chance of not being vulnerable
- Zookeeper has not integrated the fixed version yet
- Pinning here will avoid having Citrus to be flagged and ease
integration for consumers of Citrus
- Zookeeper is used in citrus-zookeeper and citrus-kafka

Signed-off-by: Aurélien Pupier <[email protected]>
@apupier apupier force-pushed the nettyUpgrade4.1.100 branch from 8163b25 to 3b39ea0 Compare October 17, 2023 13:48
@christophd
Copy link
Member

I see netty also in vert.x, docker-java and selenium 😄

@apupier
Copy link
Contributor Author

apupier commented Oct 17, 2023

vert.x, docker-java and selenium

@apupier
Copy link
Contributor Author

apupier commented Oct 17, 2023

not confident for docker-java, the PR to upgrade in docker-java is failing: https://github.com/docker-java/docker-java/actions/runs/6534289853/job/17741156863?pr=2222
but in fact most (all PRs) are failigg with same error and the main branch too https://github.com/docker-java/docker-java/actions/runs/6042954543/job/16844724905

@christophd
Copy link
Member

Your update PRs for vert.x and selenium got merged already, thanks. How about pinning the netty version for those, too. It will avoid diverging netty versions and different versions being mixed by different citrus-* modules. WDYT?

Also have you tried to pin the netty version for citrus-docker module already?

@apupier
Copy link
Contributor Author

apupier commented Oct 17, 2023

How about pinning the netty version for those, too. It will avoid diverging netty versions and different versions being mixed by different citrus-* modules. WDYT?

Pinning for all will add quite a bunch of maintenance. i'm nt sure that it is worthy (but I do no think that I will be the one to do most of the maintenance for this project so more your call).
Also i'm not sure that it occurs that often that many citrus-x are used in the same project. So end-users might not have often troubles.

Also have you tried to pin the netty version for citrus-docker module already?

Not yet.
Given that it means to list a longer list of netty dependency, I wanted to investigate a bit more. it seems that we could slim down from docker-java to docker-java-core and get rid of Netty dependency. At least, it compiles. I created this PR to have tests running. if it is not working, I will try to pin the version for citrus-kubernetes and citrus-docker (which are both using docker-java)

@christophd christophd merged commit f16e8f6 into citrusframework:main Oct 18, 2023
1 check passed
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.

3 participants