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

Getting rid of log4j1 in the war file #8377

Merged
merged 3 commits into from
Feb 2, 2022
Merged

Getting rid of log4j1 in the war file #8377

merged 3 commits into from
Feb 2, 2022

Conversation

landreev
Copy link
Contributor

@landreev landreev commented Jan 28, 2022

What this PR does / why we need it:

Self-explanatory - this PR gets rid of the log4j-1.2.14 jar we've been shipping as part of the war file.

Which issue(s) this PR closes:

Closes IQSS/dataverse-security#48

Special notes for your reviewer:

The surprising part here is that it really looks like none of our 3rd party dependencies are actually using log4j; with the single exception of XOAI - the unsupported dependency that we built ourselves. It's long overdue for an overhaul; this will be handled as a separate issue.

Suggestions on how to test this:

The main (and easily verifiable) goal is for the .war file to no longer contain log4j-1.2.14.jar. Jenkins/restassured tests still passing should ensure that nothing has been broken in the process.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?:

Additional documentation:

@landreev
Copy link
Contributor Author

(part of the reason for making this a draft PR, I wanted to see the automated tests run on it)

Copy link
Contributor

@poikilotherm poikilotherm left a comment

Choose a reason for hiding this comment

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

I left a few comments... 😄

Another question, @landreev: should we make sure no future (accidential) usages of Log4J slip through via

<module name="IllegalImport">
-            <property name="illegalPkgs" value="org.apache.commons.lang"/>
+            <property name="illegalPkgs" value="org.apache.commons.lang, org.apache.log4j"/>
        </module>

in checkstyle.xml? Our good doggo @reviewdog would bark at those (newly added lines).

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@landreev
Copy link
Contributor Author

@poikilotherm One random question for you; since it looks like you've been looking into all this in parallel: Have you by any chance figured out why we are downloading the version 1.5.6 of slf4j-parent, slf4j-api, etc. during the build (in addition to 1.7.35, specified as ${slf4j.version})?
I figured I'd ask, in case you've already found the answer.

@landreev landreev marked this pull request as ready for review January 31, 2022 16:05
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

At standup I volunteered to take a look and everything seems fine to me so I'm moving it to QA.

I didn't deploy the war file but I compared develop-984219b5be to ds48-l4j-upgrade-aae7bbb955 and found that the former has ./WEB-INF/lib/log4j-1.2.14.jar but the latter (this pull request) does not. Both have ./WEB-INF/classes/log4j.properties but this is just a config file.

@poikilotherm I see you had various comments but they seem to be are flagged as "outdated" now that @landreev has addressed them.

It's not a bad idea to add something to checkstyle but I'm ok waiting so as not to hold up this pull request.

@pdurbin pdurbin removed their assignment Jan 31, 2022
@landreev
Copy link
Contributor Author

@pdurbin - thanks for approving! The part about adding an explicit "Illegal Import" rule for log4j - I agree that it's a good idea; but we can't do it just yet. Maybe after we overhaul that XOAI library dependency, via #IQSS/dataverse/8372.

@poikilotherm
Copy link
Contributor

poikilotherm commented Jan 31, 2022

@poikilotherm One random question for you; since it looks like you've been looking into all this in parallel: Have you by any chance figured out why we are downloading the version 1.5.6 of slf4j-parent, slf4j-api, etc. during the build (in addition to 1.7.35, specified as ${slf4j.version})? I figured I'd ask, in case you've already found the answer.

I tried to reproduce this on develop by removing all in ~/.m2/repository/org/slf4j/* and running Maven again. Reviewed the download logs, but no SLF4J v1.5.6, but v1.7.5 etc... 🤷 I tried to resolve this via the respective POMs of all SLF4J components, but I have no idea - nothing in there.

It's likely that some other dependency uses an older version, so it gets downloaded, but as we use <dependencyManagement> to enforce a common version, this is overridden. If you disable all of this and take a look via Maven Enforcer, that might reveal the true troublemakers.

@landreev
Copy link
Contributor Author

I tried to reproduce this on develop ... Reviewed the download logs, but no SLF4J v1.5.6 but v1.7.5 etc...

Interesting. I'm clearly seeing it on this branch...

% ls -d ~/.m2/repository/org/slf4j/*/1.5.6
/Users/landreev/.m2/repository/org/slf4j/jcl-over-slf4j/1.5.6
/Users/landreev/.m2/repository/org/slf4j/slf4j-api/1.5.6
/Users/landreev/.m2/repository/org/slf4j/slf4j-jdk14/1.5.6
/Users/landreev/.m2/repository/org/slf4j/slf4j-parent/1.5.6
% rm -rf ~/.m2/repository/org/slf4j/*/1.5.6

then run maven build and see it downloaded again:

Downloaded from central: https://repo.maven.apache.org/maven2/org/slf4j/slf4j-parent/1.5.6/slf4j-parent-1.5.6.pom (7.9 kB at 158 kB/s)
Downloading from central: https://repo.maven.apache.org/maven2/org/slf4j/slf4j-api/1.5.6/slf4j-api-1.5.6.pom
Progress (1): 2.7/3.0 kB
... etc.

I was assuming it was maven itself that needed this for some internal purposes. (trying to remember if I saw this on develop too). Not 100% sure what to make out of the fact that you appear to be getting a different different version ...
Not the most important mystery - just curious. (I'll trace it if I have a free minute)

@kcondon kcondon self-assigned this Feb 1, 2022
@landreev landreev dismissed poikilotherm’s stale review February 2, 2022 15:25

For some reason it's still showing this as "change requested", even though I ended up adding that IllegalImport rule to checstyle.xml

@kcondon kcondon merged commit eac963e into develop Feb 2, 2022
@kcondon kcondon deleted the ds48-l4j-upgrade branch February 2, 2022 15:27
@landreev landreev mentioned this pull request Feb 9, 2022
poikilotherm added a commit to poikilotherm/dataverse that referenced this pull request Feb 9, 2022
@pdurbin pdurbin added this to the 5.10 milestone Feb 18, 2022
@pdurbin pdurbin mentioned this pull request Mar 4, 2022
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