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

Update to log4j 2.19.0 #2654

Merged
merged 6 commits into from
Jan 3, 2023
Merged

Update to log4j 2.19.0 #2654

merged 6 commits into from
Jan 3, 2023

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Dec 8, 2022

this is a possible method to remove log4j jars from the resulting build

I ran

./apollo deploy
cd target
mkdir output
cp apollo-2.6.7-SNAPSHOT.war output/apollo.zip
cd output
unzip apollo.zip
find .|grep log4j

output

WEB-INF/lib/log4j-1.2-api-2.19.0.jar
WEB-INF/lib/log4j-api-2.19.0.jar
WEB-INF/lib/log4j-core-2.19.0.jar
WEB-INF/lib/tomcat-embed-logging-log4j-7.0.70.jar

these are up to date versions of log4j v2

I then deployed this war file and it did not crash on startup

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Dec 8, 2022

some implementation notes just for detail

this link was helpful for explaining the exclude method in BuildConfig

https://stackoverflow.com/questions/70801702/remove-old-log4j-dependency-from-grails

this blogpost from the grails team explained how to find transitive dependencies on log4j using the command ./grailsw dependencyReport
https://grails.org/blog/2021-12-14-log4j2-cve.html

this led me to delete the "export" plugin which does have a transitive dependency https://gpc.github.io/export/docs/export-2.0.0.pdf

it's possible this PR changeset could be made slightly more minimal but wanted to create this pr as a save point

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Dec 8, 2022

note that without this PR, the log4j artifacts are this

WEB-INF/lib/grails-plugin-log4j-2.5.5.jar
WEB-INF/lib/log4j-1.2.17.jar
WEB-INF/lib/tomcat-embed-logging-log4j-7.0.70.jar

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Dec 8, 2022

just as a random note, the WEB-INF/lib/log4j-1.2-api-2.19.0.jar is called the 'log4j bridge' which allows log4j v1 code to use v2

@rbuels
Copy link
Collaborator

rbuels commented Dec 8, 2022

Wow, awesome! Does this have any bad side effects at all? @nathandunn what do you think of this?

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Dec 8, 2022

the removal of the export plugin is the biggest unknown to me. the app compiles but it could be worth looking more thoroughly if anything in the codebase relies on this

@garrettjstevens
Copy link
Contributor

It looks like the export plugin allows "instructor" reports to be exported in CSV, Excel, and XML format (ref #1862).

image

That seems to be the only type of report that has an export, and I think removing the export is reasonable.

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Dec 9, 2022

thanks so much for checking into that :) I'll see if I can manually edit the page to remove the exports (maybe could try to keep a csv export with manual code or something)

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Dec 9, 2022

just went ahead and removed from instructor report for now

@garrettjstevens
Copy link
Contributor

Also grype seems to agree that vulnerable log4j code is gone.

@childers
Copy link
Collaborator

@cmdcolin This is awesome! Apologies but I broke my leg in multiple places last week and am still in recovery. I will work on testing this PR and get back to you.

@cmdcolin
Copy link
Collaborator Author

Dang!!! I'm so sorry to hear that, I hope you're doin ok! I followed up with you via email too

@childers
Copy link
Collaborator

Update: @g8tor and I tested this PR in our fork

Steps To Reproduce:

  • Copy raw versions of the modified files into out src directory
  • Ran normal deploy process to build the apollo.war file (see the gmod/apollo:release-2.6.6 image)
  • Copy the resulting apollo.war file to tomcat webapps directory as apollo_test.war
  • Restart Tomcat Instance
  • Access the newly deployed apollo_test app at http://localhost:8080/apollo_test/jbrowse
  • Tested a variety of normal operations including creating, modifying and exporting annotations

Notes:

  • The log files show no errors and the application did not crash.
  • We have verified that the correct version of log4j are in use (:
-rw-r----- 1 apollo apollo  348997 Dec 14 17:46 log4j-1.2-api-2.19.0.jar
-rw-r----- 1 apollo apollo  317566 Dec 14 17:46 log4j-api-2.19.0.jar
-rw-r----- 1 apollo apollo 1864386 Dec 14 17:46 log4j-core-2.19.0.jar
-rw-r----- 1 apollo apollo   84238 Jun 24  2016 tomcat-embed-logging-log4j-7.0.70.jar

@childers
Copy link
Collaborator

childers commented Jan 3, 2023

Hi all, any updates on this PR?

@garrettjstevens
Copy link
Contributor

I am going to merge this and try to run a release later today.

@garrettjstevens garrettjstevens merged commit 672a90a into develop Jan 3, 2023
@garrettjstevens garrettjstevens deleted the remove_log4j branch January 3, 2023 21:07
@garrettjstevens
Copy link
Contributor

This has been released now in version 2.7.0.

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Jan 4, 2023

thanks much @garrettjstevens :) excellent

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.

4 participants