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

log4j v2 update #1970

Merged
merged 5 commits into from
Jan 11, 2022
Merged

log4j v2 update #1970

merged 5 commits into from
Jan 11, 2022

Conversation

anthonysena
Copy link
Collaborator

@anthonysena anthonysena commented Dec 13, 2021

Aims to address #1969 by updating log4j to V2.15.0. This is a draft of a PR since it requires additional work to migrate log4j.xml to use the new format. I've included src/main/resources/log4j2.xml as part of this PR to give us a starting point.

@anthonysena anthonysena marked this pull request as ready for review December 14, 2021 19:29
@anthonysena
Copy link
Collaborator Author

I've made my best attempt to migrate log4j.xml to the new log4j v2 format. As part of these changes, we can/should remove the log4j.xml when this is merged so I've deleted that file on this branch. If easier for review purposes, I can restore that file so please let me know your thoughts.

@anthonysena
Copy link
Collaborator Author

Also worth noting: I've done my best to preserve backwards compatibility while also incorporating the changes that were recently introduced in #1913.

@anthonysena
Copy link
Collaborator Author

anthonysena commented Dec 15, 2021

In comparing the logs from an instance of WebAPI v2.10.1 vs an instance running this branch I have two observations:

  • On v2.10.1 with auditing enabled, I see both the audit.log and audit-extra.log while on this branch I am missing the audit-extra.log. Currently, the audit-extra.log is empty but the file exists. Here is a snippet of the settings.xml for both environments:
<audit.trail.enabled>true</audit.trail.enabled>
<audit.trail.log.file>D:/apache-tomcat-8.5.63/logs/audit/audit.log</audit.trail.log.file>
<audit.trail.log.extraFile>D:/apache-tomcat-8.5.63/logs/audit/audit-extra.log</audit.trail.log.extraFile>
  • In this branch, some (but not all) logging messages are duplicated. For example:

  .   ____          _            __ _ _
 /\\ / ___'_ __ _ _(_)_ __  __ _ \ \ \ \
( ( )\___ | '_ | '_| | '_ \/ _` | \ \ \ \
 \\/  ___)| |_)| | | | | || (_| |  ) ) ) )
  '  |____| .__|_| |_|_| |_\__, | / / / /
 =========|_|==============|___/=/_/_/_/
 :: Spring Boot ::       (v1.5.22.RELEASE)

2021-12-14 20:58:47.110 INFO localhost-startStop-1 org.springframework.boot.StartupInfoLogger - [] - Starting WebApi on <server> with PID 2884 (D:\apache-tomcat-8.5.63\webapps\WebAPI\WEB-INF\classes started by <server>$ in D:\apache-tomcat-8.5.63)
2021-12-14 20:58:47.110 INFO localhost-startStop-1 org.springframework.boot.StartupInfoLogger - [] - Starting WebApi on <server> with PID 2884 (D:\apache-tomcat-8.5.63\webapps\WebAPI\WEB-INF\classes started by <server>$ in D:\apache-tomcat-8.5.63)
2021-12-14 20:58:47.119 INFO localhost-startStop-1 org.springframework.boot.SpringApplication - [] - The following profiles are active: default
2021-12-14 20:58:47.119 INFO localhost-startStop-1 org.springframework.boot.SpringApplication - [] - The following profiles are active: default

My guess is that there may be duplicate loggers registered in the code base. This feels fairly minor but noting it for completeness.

pom.xml Outdated
@@ -13,7 +13,7 @@
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<!-- Spring Boot manages spring.version as well -->
<spring.boot.version>1.5.22.RELEASE</spring.boot.version>
<log4j2.version>2.15.0</log4j2.version>
<log4j2.version>2.16.0</log4j2.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use 2.17.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed - as a matter of fact, we should probably keep this branch open until the release cycle slows a bit. There have been 3 releases in the past 3 weeks alone: https://logging.apache.org/log4j/2.x/changes-report.html.

@konstjar konstjar mentioned this pull request Dec 22, 2021
@chrisknoll
Copy link
Collaborator

Ok, for the sake of expediting this fix, let's settle on a confirm fix for the reported vulnerability, and get it into master, and then cherry pick it for a hotfix into our maintenance branch. Sound good?

@alex-odysseus
Copy link
Contributor

@chrisknoll I had to make it properly via a merge to the 'master' and then cherry-picking to the 'master-2.10'. You are right. In the same time Anthony mentioned this branch is going to be open for some time. I merged parts which belong to log4j (didn't include removing of no longer used profiels) right away into 'master-2.10'. There was an intention to release 2.10.2 in the nearest days which include log4j 1.x -> 2.x migration

Known issues:

  • duplication of log entries

@alondhe
Copy link
Contributor

alondhe commented Jan 4, 2022

Is this going to use 2.17.1? I'm seeing that this is needed now, as 2.17.0 has yet another vulnerability.

Never mind -- I see it now

@anthonysena
Copy link
Collaborator Author

We'll keep this PR open until the release cycle from Apache log4j slows a bit.

@anthonysena anthonysena merged commit fd8a6da into master Jan 11, 2022
@delete-merged-branch delete-merged-branch bot deleted the log4j-v2-update branch January 11, 2022 13:47
m0nhawk pushed a commit to uc-cdis/WebAPI that referenced this pull request Jan 26, 2022
* Update log4j2.xml configuration
* Bump to log4j v2.17.1
@anthonysena anthonysena linked an issue Feb 8, 2022 that may be closed by this pull request
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.

log4j and Google OAuth security updates
4 participants