qos-ch / reload4j

reload4j is a drop-in replacement for log4j 1.2.17
Apache License 2.0
148 stars 22 forks source link

OSGI bundle manifest file #27

Closed ohmhamurg closed 2 years ago

ohmhamurg commented 2 years ago

Would it be possible to generate a META-INF/MANIFEST.MF with OSGI bundle information for use in an OSGI environment? (log4j 1.2.17 has).

Regards Olaf

ceki commented 2 years ago

I thought reload4j already had OSGi bundle information.

ceki commented 2 years ago

I just checked it does have OSGI bundle information. Can you please point at what is missing?

ohmhamurg commented 2 years ago

This is the MANIFEST.MF from https://mvnrepository.com/artifact/ch.qos.reload4j/reload4j/1.2.18.2

Manifest-Version: 1.0 Build-Jdk-Spec: 1.8 Created-By: Maven Jar Plugin 3.2.0

Name: org.apache.log4j Implementation-Title: log4j Implementation-Version: 1.2.18.0 DynamicImport-Package: * Implementation-Vendor: "Apache Software Foundation"

Please take a look to MANIFEST.MF from log4j-1.2.17.jar to see the difference. Export-Package:, Ignore-Package:, Import-Package:, Bundle-XXX:, etc. is missing

ceki commented 2 years ago

The OSGi bundle information is present at build time but gets somehow lost during transfer to Maven Central.

ceki commented 2 years ago

Fixed in c2455a1c42

jkleff commented 2 years ago

I was attempting to use 1.2.18.0 in OSGi and was having issues. Just saw this and updated the maven POMs to 1.2.18.3 but got this error:

Unable to resolve 31.0: missing requirement [31.0] osgi.wiring.package; (osgi.wiring.package=org.apache.log4j) [caused by: Execution environment not supported: JavaSE-1.5]

The application builds with JDK 1.7, and runs in a JRE 1.8 environment. I see this targets 1.5 which I thought would be fine. The app is happy with Log4j-1.2.16.

I got past this with a config.properties update: org.osgi.framework.executionenvironment=JavaSE-1.8, JavaSE-1.7, JavaSE-1.6, JavaSE-1.5

However, the framework now can't find OSGi wiring package: com.ibm.uvm.tools, and com.sun.jdmk.comm

can these be marked as optional in the MANIFEST import statement? I patched it myself as a test and everything now runs fine, so something in log4j requires them that I am not calling?

Open to better solutions. thanks for your work.

ceki commented 2 years ago

I think log4j 1.2.7 does not specify anything regarding the execution environment.

ceki commented 2 years ago

J2SE-1.5 should work better than JavaSE-1.5

grgrzybek commented 2 years ago

@jkleff why do you need reload4j in OSGi environment? Not that I want to advertise ops4j/org.ops4j.pax.logging project, but it has a long history of OSGi adjustments to log4j1 and other "frameworks". Log4j has several static blocks and does classloader discovery without bothering to check bundle resources. Pax Logging does all of that. pax-logging-api gives you access to log4j1/reload4j packages without doing the backend magic. You can use log4j1 API (probably that's what you want) but the actual logging backend can be Logback or Log4j2.

reload4j-1.2.18.3 indeed requires com.ibm.uvm.tools,com.sun.jdmk.comm...

grgrzybek commented 2 years ago

com.ibm.uvm.tools import is generated because of this fragment in org.apache.log4j.spi.LocationInfo:

// Check if we are running in IBM's visual age.
static boolean inVisualAge = false;
static {
try {
    inVisualAge = Class.forName("com.ibm.uvm.tools.DebugSupport") != null;
    LogLog.debug("Detected IBM VisualAge environment.");
} catch (Throwable e) {
    // nothing to do
}

com.sun.jdmk.comm import is generated because of this fragment in org.apache.log4j.jmx.Agent:

try {
    newInstance = Class.forName("com.sun.jdmk.comm.HtmlAdapterServer").newInstance();
} catch (ClassNotFoundException ex) {
    throw new RuntimeException(ex.toString());
} catch (InstantiationException ex) {
    throw new RuntimeException(ex.toString());
} catch (IllegalAccessException ex) {
    throw new RuntimeException(ex.toString());
}

javax.jmdns import is generated because of this fragment in org.apache.log4j.net.ZeroConfSupport#initializeJMDNS():

jmDNSClass = Class.forName("javax.jmdns.JmDNS");
serviceInfoClass = Class.forName("javax.jmdns.ServiceInfo");

Probably no one remembers what Visual Age is and org.apache.log4j.jmx.Agent has this JavaDoc:

Manages an instance of com.sun.jdmk.comm.HtmlAdapterServer which was provided
for demonstration purposes in the Java Management Extensions Reference
Implementation 1.2.1. This class is provided to maintain compatibility with
earlier versions of log4j and use in new code is discouraged.

I've never heard about Java multicast DNS (it's javax package, but I don't see related JSR...).

I'd remove these code fragments anyway (except jmdns one) - and I'm preparing an OSGi fix right now.

jkleff commented 2 years ago

Interesting and good to know. Sadly, I'm maintaining a legacy application that builds from an offline maven repo, and parts of it that rely on log4j can no longer be built as-is (many years of mistakes, don't get me started...). So this drop in fork of log4j-1.2 caught my eye and is the perfect usage for me right now... despite my dislike of OSGi. This of course was only necessitated by the v1.2 vulnerabilities coming to light after "the big one" did. The app wasn't even using any logging, in deployed form. Just imports it with all logs disabled :(

thanks again.

ceki commented 2 years ago

@jkleff release 1.2.18.4 fixes this issue. Can you please try it out?

jkleff commented 2 years ago

@ceki 1.2.18.4 pulled from maven central works out of the box now, as long as I set: org.osgi.framework.executionenvironment=J2SE-1.5 thanks for that!

ceki commented 2 years ago

@jkleff Thank you for the quick feedback. Am I correct to assume that in your environment setting org.osgi.framework.executionenvironment is always a requirement? is there anything that can be done in reload4j so that you would not need to set the aforementioned variable?

jkleff commented 2 years ago

@ceki I'm not sure yet. I'm not the original developer, nor an Apache Felix expert. just maintaining some legacy tools. I was under the impression that setting Bundle-RequiredExecutionEnvironment was a minimum value, and so my JavaSE-1.7/1.8 deployment would work. but the presence of it seems to necessitate my setting the above in config.properties.

I think it's no big deal, but I had to figure it out the hard way from errors like: [caused by: Execution environment not supported: JavaSE-1.5]

Maybe just an OSGi blurb in your README would suffice? But there certainly could be a more eloquent way of preparing the JAR for OSGi that I'm not aware of.

ceki commented 2 years ago

By the way, as I understand it, "JavaSE-1.5" is an incorrect value for Bundle-RequiredExecutionEnvironment. Reload4j 1.2.18.4 uses the correct value: "J2SE-1.5"

I should also add that the slf4j 1.7.x series also sets "J2SE-1.5" for the value of Bundle-RequiredExecutionEnvironment.

jkleff commented 2 years ago

Interesting and good to know. the variable didn't exist before I needed to with reload4j.jar...so I suppose it was using some default value provided by the framework prior to this.

ceki commented 2 years ago

What happens if you do not set org.osgi.framework.executionenvironment with reload4j version 1.2.18.4? Have you tried that?

jkleff commented 2 years ago

Ha, of course I forgot that. It works fine. so was only necessary with 1.2.18.3. nevermind the above :)

grgrzybek commented 2 years ago

Just for completeness, maven-bundle-plugin (and underlying bndlib) looks at maven-compiler-plugin configuration (source, target) and generates relevant Require-Capability header like this (pax-logging 1.10):

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

or this (pax-logging 1.12):

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

reload4j 1.2.18.3 has two headers:

Bundle-RequiredExecutionEnvironment: JavaSE-1.5
...
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.5))"

and while I wanted to show how it's done in Karaf, I actually realized I've rarely seen Bundle-RequiredExecutionEnvironment. And that's true - please check chapter 3.4.1 Bundle-RequiredExecutionEnvironment of OSGi Core R7 specification:

The Bundle-RequiredExecutionEnvironment manifest header provides the same function as the osgi.ee Namespace. It allows a bundle to depend on the execution environment. This header is deprecated but must be fully supported by a compliant framework. Bundles should not mix these headers but use either an osgi.ee requirement or this header.

So first - a PR: #31 that simply removes Bundle-RequiredExecutionEnvironment header generation.

Because maven-bundle-plugin doesn't contain _noee instruction, maven-compiler-plugin configuration is used and this is generated:

Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.5))"

And finally - after all it's not J2SE, but JavaSE ;) See 8.2 osgi.ee Namespace chapter:

JavaSE – Should be used for all Java SE execution environments since Java 1.2. The name for the Java Runtime environment has changed several times but for all environments the Java SE name must be used.

And @jkleff Karaf sets this in etc/config.properties:

...
eecap-10= osgi.ee; osgi.ee="OSGi/Minimum"; version:List<Version>="1.0,1.1,1.2", \
 osgi.ee; osgi.ee="JavaSE"; version:List<Version>="1.0,1.1,1.2,1.3,1.4,1.5,1.6,1.7,1.8,9.0,10.0", \
 osgi.ee; osgi.ee="JRE"; version:List<Version>="1.0,1.1", \
 osgi.ee; osgi.ee="JavaSE/compact1"; version:List<Version>="1.8,9.0,10.0", \
 osgi.ee; osgi.ee="JavaSE/compact2"; version:List<Version>="1.8,9.0,10.0", \
 osgi.ee; osgi.ee="JavaSE/compact3"; version:List<Version>="1.8,9.0,10.0"
eecap-9= osgi.ee; osgi.ee="OSGi/Minimum"; version:List<Version>="1.0,1.1,1.2", \
 osgi.ee; osgi.ee="JavaSE"; version:List<Version>="1.0,1.1,1.2,1.3,1.4,1.5,1.6,1.7,1.8,9.0", \
 osgi.ee; osgi.ee="JRE"; version:List<Version>="1.0,1.1", \
 osgi.ee; osgi.ee="JavaSE/compact1"; version:List<Version>="1.8,9.0", \
 osgi.ee; osgi.ee="JavaSE/compact2"; version:List<Version>="1.8,9.0", \
 osgi.ee; osgi.ee="JavaSE/compact3"; version:List<Version>="1.8,9.0"
eecap-1.8= osgi.ee; osgi.ee="OSGi/Minimum"; version:List<Version>="1.0,1.1,1.2", \
 osgi.ee; osgi.ee="JavaSE"; version:List<Version>="1.0,1.1,1.2,1.3,1.4,1.5,1.6,1.7,1.8", \
 osgi.ee; osgi.ee="JRE"; version:List<Version>="1.0,1.1", \
 osgi.ee; osgi.ee="JavaSE/compact1"; version:List<Version>="1.8", \
 osgi.ee; osgi.ee="JavaSE/compact2"; version:List<Version>="1.8", \
 osgi.ee; osgi.ee="JavaSE/compact3"; version:List<Version>="1.8"

So each next runtime has JavaSE capabilities of this + all previous runtimes - praise Java™ backward compatibility!

fipro78 commented 2 years ago

Hi, I tested reload4j 1.2.18.4 in a minimal OSGi example and it works AFAICS. I just noticed that the Bundle-SymbolicName is ch.qos.reload4j. In general that should be correct and not an issue. But it is for older releases of Eclipse.

Eclipse is re-bundling jars to add missing or incorrect OSGi meta-data. It did this also for log4j 1.2.15 (don't ask me why). With the re-bundling the Bundle-SymbolicName was set to org.apache.log4j. Today every plug-in developer is told to use Import-Package to specify the dependency to a logging library. But in the past (and unfortunately even today) some developers are not aware of that rule and use Require-Bundle. With that the dependency is specified on the Bundle-SymbolicName and not the exported packages.

My first idea was to re-bundle reload4j to set the Bundle-SymbolicName and re-publish it in Eclipse Orbit. It basically works, but I faced two issues:

I first was not considering to ask you to change the Bundle-SymbolicName, as log4j 1.2.17 in Maven Central specifies the Bundle-SymbolicName only as log4j. But taking the above issues into account and the fact that actually OSGi developers have used Import-Package for a long time, and only Eclipse developers used Require-Bundle in the past, I thought it would be at least worth asking if you could change the Bundle-SymbolicName to org.apache.log4j, so reload4j could be taken directly from Maven Central as a drop-in replacement for the re-bundled org.apache.log4j from Eclipse Orbit. This way it would not be necessary to re-bundle and re-publish reload4j in Eclipse and we could even help older Eclipse based applications to solve the CVEs directly.

What do you think?

grgrzybek commented 2 years ago

@fipro78 I'd use this Log4j CVE apocalypse opportunity to stop treating Require-Bundle as something normal. I'd NOT make it easy for devs using Require-Bundle to use reload4j as if it was normal log4j bundle. They have to do something, so it'd be better if they switch to Import-Package.

fipro78 commented 2 years ago

@grgrzybek I totally agree and I am discussing that topic for years now. And we again started that discussion so several Eclipse projects are even more convinced to change now. To be honest, some projects didn't even know as the maintainers changed over time. Or to say it differently, they are checking again and fix the remaining places. The issue is that you can not change the past. And if projects are not able to consume the latest version of a library that consumes log4j via Require-Bundle, how can they fix the Log4j CVEs without a drop-in replacement of log4j with the matching symbolic name?

But as I said, just a question. I don't want to push the idea too much if you don't like the idea. In that case I probably need to find a way to re-bundle the jar for Eclipse Orbit and deal with the issues somehow.

grgrzybek commented 2 years ago

@fipro78 I only commented about OSGi ;) I'm not affiliated at all with the decision making process for reload4j ;)

ceki commented 2 years ago

I was unaware of the 4th position in the version string causing problems. Will stick to incrementing the 3rd position from now on.

As for Bundle-SymbolicName, it is now being set manually to org.apache.log4j. See c08e26552d5fb1

@fipro78 Please let me know if this would satisfy older Eclipse releases. @grgrzybek Please let me know if you object to this change.

grgrzybek commented 2 years ago

@ceki I agree with the change - my decision is based on pros vs cons and there are no cons here ;) About the 4th position, 1.2.18.5 is proper OSGi version, but 5 is treated as qualifier, so the problem may be that 1.2.18.5 is newer than 1.2.18.10 for example.

This OSGi code fragment prints "5":

org.osgi.framework.Version.parseVersion("1.2.18.5").getQualifier()

This however throws java.lang.IllegalArgumentException: invalid version "1.2.18.5.6": invalid qualifier "5.6" exception:

org.osgi.framework.Version.parseVersion("1.2.18.5.6").getQualifier()
fipro78 commented 2 years ago

@ceki The issue with the 4th position is actually as @grgrzybek already explained. It is a qualifier that can be actually anything. For example, the Eclipse build uses the 4th position to add the build timestamp. And some tools therefore interprete it differently.

I just ran the build locally and there are is no OSGi metadata in the MANIFEST.MF anymore. I wanted to test locally to give you a verified feedback if it works now or not. Any idea why the metadata is lost now?

grgrzybek commented 2 years ago

@fipro78 75907003bbf4e5ebfc860605e7f2510158727d47 removed:

<manifestFile>${project.build.outputDirectory}/META-INF/MANIFEST.MF</manifestFile>

and that's the problem (oh, I see your comment ;)

ceki commented 2 years ago

@fipro78 Sorry. fixed in 8095168f35958819b268

fipro78 commented 2 years ago

@ceki Please don't apologize for mistakes that happen while fixing issues. :) Such things happen and I really appreciate your responsiveness.

I was able to build locally and the MANIFEST.MF looks good so far. I did a very small local test that succeeded. I will perform some more tests in a more complex scenario to be sure it works and come back with the feedback, hopefully before you are publishing the next release.

ivo-kolev commented 2 years ago

Hi, is there clarity when this issue will be resolved? Regards, Ivo Kolev

ceki commented 2 years ago

@ivo-kolev It is already resolved. What issue are you referring to?

ivo-kolev commented 2 years ago

@ceki Thank you. The status of 'OSGI bundle manifest file #27' shows as open, this is why I thought there is still pending fix specifically to the OSGi manifest entries.