search

ops4j / org.ops4j.pax.logging

The OSGi Logging framework implementation. Supports SLF4J,LOG4J,JCL etc.
https://ops4j1.jira.com/wiki/spaces/paxlogging/overview
Apache License 2.0
47 stars 79 forks source link

Upgrade to log4j2 at least 2.13.1 (1.10.x) #411

Closed bertdewolf closed 2 years ago

bertdewolf commented 2 years ago

We need JsonLayout support in the 1.10.x releases

grgrzybek commented 2 years ago

Hmm, the refactoring was made only to versions 1.11.x and 2.0.x. I'll see what can I do.

grgrzybek commented 2 years ago

@bertdewolf by the way - are there any problems for you to migrate to pax-logging 1.11? These should be compatible.

bertdewolf commented 2 years ago

@grgrzybek We are using osgi 5 at the moment (yes old I know, but we have to deal with it). Upgrading to pax-logging 1.11 requires us to upgrade to osgi 6, which is a higher effort.

grgrzybek commented 2 years ago

Understand. Please give me a bit of time and I'll try to upgrade.

bertdewolf commented 2 years ago

Do you have an idea when it could be finished? (no pressure)

grgrzybek commented 2 years ago

no way I can check today (daily work...), but I can try next week, ok?

bertdewolf commented 2 years ago

Next week is totally fine! Thank you for the extremely quick response.

jbonofre commented 2 years ago

I will take a look as well as I need pax-logging release for Karaf 4.3.4.

grgrzybek commented 2 years ago

@jbonofre but Karaf 4.3.x is on Pax Logging 2.0.x...

jbonofre commented 2 years ago

Yes, but cherry picking there if needed.

grgrzybek commented 2 years ago

@bertdewolf I did quick check using a tool I wrote some time ago. And the only thing that could prevent me from upgrading is that with Log4j2 2.12.1 I see (pax-logging-log4j2 bundle):

Require-Capability:
    osgi.ee
        filter := (&(osgi.ee=JavaSE)(version=1.7))

While with Log4j2 2.14.1, I see:

Require-Capability:
    osgi.ee
        filter := (&(osgi.ee=JavaSE)(version=1.8))

I'm not sure we can change this between 1.10.7 and 1.10.9... WDYT @jbonofre ?

bertdewolf commented 2 years ago

@grgrzybek I'm a bit biased in that decision since we really want this change.

It really depends on the principles you want to apply in the versioning of this project.

I think that 2.13.x versions of log4j2 also depend on jdk8? I hope you decide to do it :-D

jbonofre commented 2 years ago

I would target log4j update only on pax-logging 2.0.x as it could break 1.10.x existing users.

jbonofre commented 2 years ago

By the way, Pax Logging 2.0.x already supports log4j 2.14.1.

bertdewolf commented 2 years ago

Can a decision be made? So we know whether we have to do a high effort or not.

grgrzybek commented 2 years ago

While the upgrade sounds easy at first glance (and it technically is), we have to consider wider impact. Because you @bertdewolf are using old 1.10.x version of pax-logging, there may be other users staying at this version as well. So if we release 1.10.x breaking JDK7 compatibility, it'll help you use Log4j2 with JsonLayout, but it may break many other silent users ;)

In your situation, I'd fork pax-logging and release the upgraded version with log4j2 upgrade and some internal number, like 1.10.8.1 or something.

WDYT @jbonofre ?

Prototyped commented 2 years ago

With today's remote code injection vulnerability found in log4j 2.x <2.15.0, I suggest updating to >= 2.15.0.

https://github.com/advisories/GHSA-jfh8-c2jp-5v3q

grgrzybek commented 2 years ago

Done with #414

grgrzybek commented 2 years ago

But there's no decision about 1.10.x - did you try forking in your organization?

bertdewolf commented 2 years ago

No, we still have to make a decision on that. Not sure if I know enough to fork and include the right things correctly.

grgrzybek commented 2 years ago

@bertdewolf I can prepare a branch for you ;) Your only task would be to build it and mvn deploy to your private repository.

bertdewolf commented 2 years ago

@grgrzybek If you could do that with the >=2.15.0 version, it would help a lot!

ottlinger commented 2 years ago

btw: https://nvd.nist.gov/vuln/detail/CVE-2021-44228 seems to be dealt with by https://github.com/ops4j/org.ops4j.pax.logging/security/advisories/GHSA-xxfh-x98p-j8fr

Thanks for the release!

grgrzybek commented 2 years ago

@bertdewolf I'm on PTO right now, but I'll find a time to prepare a branch for you

grgrzybek commented 2 years ago

@ottlinger the issue for the CVE that has shaken the world of Java logging is #414

bertdewolf commented 2 years ago

@grgrzybek No problem, looking forward to the branch ;)

grgrzybek commented 2 years ago

@bertdewolf (cc: @jbonofre , @masanasa) I've pushed an upgrade to Log4j2 2.16.0 into special pax-logging-1.10.x-jdk8 branch. No one should release a version from this branch to Maven Central!

@bertdewolf please use this branch to release/build/deploy your own version of pax-logging 1.10.8 (last 1.10.x in Maven Central is 1.10.7). You can set any version you like and deploy to your own Artifactory/Nexus/Whatever.

grgrzybek commented 2 years ago

The branch is https://github.com/ops4j/org.ops4j.pax.logging/tree/pax-logging-1.10.x-jdk8

bertdewolf commented 2 years ago

@grgrzybek I'm experiencing problems using this special branch

java.lang.NoSuchFieldError: EMPTY_BYTE_ARRAY at org.apache.logging.log4j.core.config.ConfigurationSource.<clinit>(ConfigurationSource.java:56) at org.apache.logging.log4j.core.config.NullConfiguration.<init>(NullConfiguration.java:32) at org.apache.logging.log4j.core.LoggerContext.<clinit>(LoggerContext.java:85) at org.ops4j.pax.logging.log4j2.internal.PaxLoggingServiceImpl.configureDefaults(PaxLoggingServiceImpl.java:280) at org.ops4j.pax.logging.log4j2.internal.PaxLoggingServiceImpl.<init>(PaxLoggingServiceImpl.java:114) at org.ops4j.pax.logging.log4j2.internal.Activator.start(Activator.java:128) at org.apache.felix.framework.util.SecureAction.startActivator(SecureAction.java:698) at org.apache.felix.framework.Felix.activateBundle(Felix.java:2402) at org.apache.felix.framework.Felix.startBundle(Felix.java:2308) at org.apache.felix.framework.Felix.setActiveStartLevel(Felix.java:1539) at org.apache.felix.framework.FrameworkStartLevelImpl.run(FrameworkStartLevelImpl.java:308) at java.base/java.lang.Thread.run(Thread.java:834)

I think the pax-logging-api on that branch needs to be upgraded to 2.17.0 a well? Do not know if there are other parts that need some upgrade.

grgrzybek commented 2 years ago

hmm, i'm sure i checked it initially but not sure about 2.17.0... sorry i don't have access to my github now (Christmas trip) but i'm back on dec 29th.

cheers grzegorz

czw., 23 gru 2021, 16:49 użytkownik bertdewolf @.***> napisał:

@grgrzybek https://github.com/grgrzybek I'm experiencing problems using this special branch

java.lang.NoSuchFieldError: EMPTY_BYTE_ARRAY at org.apache.logging.log4j.core.config.ConfigurationSource.(ConfigurationSource.java:56) at org.apache.logging.log4j.core.config.NullConfiguration.(NullConfiguration.java:32) at org.apache.logging.log4j.core.LoggerContext.(LoggerContext.java:85) at org.ops4j.pax.logging.log4j2.internal.PaxLoggingServiceImpl.configureDefaults(PaxLoggingServiceImpl.java:280) at org.ops4j.pax.logging.log4j2.internal.PaxLoggingServiceImpl.(PaxLoggingServiceImpl.java:114) at org.ops4j.pax.logging.log4j2.internal.Activator.start(Activator.java:128) at org.apache.felix.framework.util.SecureAction.startActivator(SecureAction.java:698) at org.apache.felix.framework.Felix.activateBundle(Felix.java:2402) at org.apache.felix.framework.Felix.startBundle(Felix.java:2308) at org.apache.felix.framework.Felix.setActiveStartLevel(Felix.java:1539) at org.apache.felix.framework.FrameworkStartLevelImpl.run(FrameworkStartLevelImpl.java:308) at java.base/java.lang.Thread.run(Thread.java:834)

I think the pax-logging-api on that branch needs to be upgraded to 2.17.0 a well? Do not know if there are other parts that need some upgrade.

— Reply to this email directly, view it on GitHub https://github.com/ops4j/org.ops4j.pax.logging/issues/411#issuecomment-1000388901, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACK3BNAAJDKWLWZILRFB73USNAITANCNFSM5IY3O5FA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: <ops4j/org. @.***>

mron7 commented 2 years ago

I agree with @bertdewolf . The code on this branch compiles fine. However, when you deploy pax-logging-api from this special branch and pax-logging-log4j2 from this special branch, then it does not run properly because the org.apache.logging.log4j.util.Constants class (that is now provided by pax-logging-api, instead of being provided by log4j2 2.17.0 as it was during compilation) is missing EMPTY_OBJECT_ARRAY (and EMPTY_BYTE_ARRAY), leading to the stack trace about NoSuchFieldError that was posted above. I didn't check what other changes there were between log4j2 2.12.1 and 2.17.0 that might cause trouble with running). pax-logging-api on the 1.10.x branch provides its own copy of Constants.java, and pax-logging-api's own codebase does not include the changes that log4j2 made between 2.12.1 and 2.17.0.

In the 1.11.x branch of pax-logging, the Constants class doesn't exist in pax-logging-api (because of the code cleanup you did on that branch), so the original Constants from log4j2 2.17.0 gets used (Constants.class is located in the pax-logging-api jar, but actual code is from 2.17.0 log4j2, not overridden with pax-logging-api codebase), and it does run.

grgrzybek commented 2 years ago

Thanks @mron7 for the analysis! I just got back and I'm reviewing the branch again. I'll update to 2.17.1 as well.

grgrzybek commented 2 years ago

yikes. I completely forgot that pax-logging 1.10.x has a copy of all (?) classes from log4j2! In my code cleanup, I've shaded only these that are changed in Pax Logging due to OSGi reasons...

grgrzybek commented 2 years ago

ok, I've updated https://github.com/ops4j/org.ops4j.pax.logging/tree/pax-logging-1.10.x-jdk8 branch - this time copying all the shaded classes from Log4j2 to Pax Logging API. @bertdewolf please check. I also had to change source&target params for maven-compiler-plugin to 1.8...

bertdewolf commented 2 years ago

@grgrzybek At first glance it seemed to work fine, untill I noticed that the MDC properties were no longer in the logs.

After some debugging I found out that the ThreadContextDataProvider was not present.

I can be traced back to PAXLOGGING-312 where some changes to the MDC functionality were required when upgrading from 2.13.1 to 2.13.2. (https://github.com/ops4j/org.ops4j.pax.logging/commit/5f9d55d7657dcc4dcf45b600a2d16dcae29b1395#diff-6f683be2ded8ad827f63c7633db489d745b93b68ace42f21253dd950ccd49163R87)

Is it enough to add those lines or are other changes needed?

Btw, thanks a lot for your efforts.

grgrzybek commented 2 years ago

Hmm, you mean it doesn't work in pax-logging-1.10.x-jdk8 branch or in 1.11.x? 1.11.x contains tests and I'm pretty sure MDC tests are also covered...

bertdewolf commented 2 years ago

Sorry for the confusion. It does not work on the pax-logging-1.10.x-jdk8 branch because the ThreadContextDataProvider isn't added in the Activator.

grgrzybek commented 2 years ago

ah, got it

grgrzybek commented 2 years ago

Please check now.