Removed explicit dependency on log4j #1563

davidlago · 2022-01-06T16:22:08Z

Signed-off-by: Dave Lago [email protected]

Description

Remove the explicit dependency on log4j. It was accidental rather than intentional, and it makes our life easier if we just remove it.

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Maintenance

Why these changes are required?

Improve overall project release speed as no changes are needed in the security plugin if there are new vulnerabilities found in log4j.

What is the old behavior before changes and new behavior after changes?

There should be no behavioral changes. The only updates were in 2 test classes.

Issues Resolved

#1519

Testing

mvn -B package -Padvanced, manually ran the test with code changes.

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Dave Lago <[email protected]>

stockholmux · 2022-01-06T16:26:17Z

Nuke. it. from. space.

dblock · 2022-01-06T16:29:14Z

My hero.

davidlago · 2022-01-06T17:24:59Z

Got The hosted runner: GitHub Actions 56 lost communication with the server. Anything in your workflow that terminates the runner process, starves it for CPU/Memory, or blocks its network access can cause this error., re-running CI.

dlvenable · 2022-01-06T17:26:22Z

src/test/java/com/amazon/dlic/auth/ldap/srv/LdapServer.java

@@ -29,8 +29,8 @@
 import java.util.concurrent.locks.ReentrantLock;

 import org.apache.commons.lang3.exception.ExceptionUtils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+import org.apache.logging.log4j.LogManager;


Why are you using Log4j explicitly here? You could still use SLF4J and swap out the underlying logger, even using the slf4j-simple if desired for tests.

The SLF4J was added explicitly in this plugin and is not provided from OpenSearch. Only Log4J is. The purpose of this PR is to clear any explicit logger dependencies in our POM and use whatever is provided in core. That's why I switched to Log4J

Just chiming in from the peanut gallery, but my two cents is that is generally better to declare explicit dependencies on things that you use as opposed to implicitly using transitive dependencies. In this case, I would think the ideal solution is like what @dlvenable mentioned, which is to declare an explicit dependency on a thin API (something like slf4j or log4j-api) and use classes defined in that API while letting core bring in the actual concrete implementation.

So first, I do see that this is a test. It does seem that if you really want to use log4j here, you should probably include log4j as an explicit test dependency in the Maven pom file. But, I don't think any of us want that. :)

I suggest that you add slf4j-api as an explicit dependency for this project in the Maven pom. Then use only that in code. You can get an slf4j implementation from OpenSearch core which will work for your tests when they run.

Thanks @dlvenable and @andrross for the help here! It turns out these are not just test dependencies, they are used all over (100+ files). I've taken a stab at replacing our uses with SLF4J... it builds locally, now letting it run the whole CI suite.

Unrelated, this CI job has been failing running out of memory... first time I see this (or at least a few times in a row), and not really sure this PR is the culprit...

Signed-off-by: Dave Lago <[email protected]>

davidlago · 2022-01-06T20:08:31Z

src/main/java/org/opensearch/security/auditlog/sink/Log4JSink.java

        }
        return true;
    }

+    private boolean isLogLevelEnabled(Logger logger, Level level) {


I just did this in a pinch to get this going... log4j has .isEnabled() and .log(), where SLF4J segregates by level. Perhaps there is a more idiomatic way of doing this.

I don't know of a better way, but I also think this is a good solution for now. Maybe there is a more extensible solution which could come later.

You could simplify the case by returning rather than using isEnabled if you wanted to make the code a little tighter.

dlvenable · 2022-01-07T00:51:45Z

src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java

@@ -194,7 +194,7 @@ protected AbstractConfigurationValidator postProcessApplyPatchResult(RestChannel
                builder.endObject();
                retVal = getValidator(request, BytesReference.bytes(builder), resourceName);
            } catch (IOException e) {
-                log.error(e);
+                log.error(e.toString());


I find the best way to handle logging exceptions to be:

log.error("relevant message", ex);

This approach will write out the stack trace.

dlvenable · 2022-01-07T00:53:42Z

src/main/java/org/opensearch/security/filter/SecurityFilter.java

@@ -305,7 +305,7 @@ public int order() {
            final PrivilegesEvaluatorResponse pres = eval.evaluate(user, action, request, task, injectedRoles);

            if (log.isDebugEnabled()) {
-                log.debug(pres);
+                log.debug(pres.toString());


This is creating a log message, which you probably don't want. It could be interpolated.

The preferred equivalent way to do this is:

log.debug("{}", pres);

But, I recommend making the log line even more readable:

log.debug("privileges response: {}", pres);

Here is a reference: https://www.slf4j.org/faq.html#string_contents

davidlago · 2022-01-07T02:30:31Z

So it turns out that the failing CI's OOM was a red herring, and we have been getting these errors since the first commit :(

2022-01-06T20:14:39.1381629Z ##[error]Exception in thread "opensearch[node_utest_n5_ffork_1_t795466787413_num3][clusterApplierService#updateTask][T#1]" java.lang.NoClassDefFoundError: org/apache/logging/log4j/core/pattern/LogEventPatternConverter
2022-01-06T20:14:39.1412693Z 	at org.opensearch.common.logging.NodeAndClusterIdStateListener.onNewClusterState(NodeAndClusterIdStateListener.java:82)
2022-01-06T20:14:39.1416097Z 	at org.opensearch.cluster.ClusterStateObserver$ContextPreservingListener.onNewClusterState(ClusterStateObserver.java:355)
2022-01-06T20:14:39.1419190Z 	at org.opensearch.cluster.ClusterStateObserver$ObserverClusterStateListener.clusterChanged(ClusterStateObserver.java:223)
2022-01-06T20:14:39.1422305Z 	at org.opensearch.cluster.service.ClusterApplierService.callClusterStateListener(ClusterApplierService.java:610)
2022-01-06T20:14:39.1425897Z 	at org.opensearch.cluster.service.ClusterApplierService.callClusterStateListeners(ClusterApplierService.java:598)
2022-01-06T20:14:39.1429119Z 	at org.opensearch.cluster.service.ClusterApplierService.applyChanges(ClusterApplierService.java:562)
2022-01-06T20:14:39.1431762Z 	at org.opensearch.cluster.service.ClusterApplierService.runTask(ClusterApplierService.java:469)
2022-01-06T20:14:39.1434213Z 	at org.opensearch.cluster.service.ClusterApplierService.access$000(ClusterApplierService.java:81)
2022-01-06T20:14:39.1436572Z 	at org.opensearch.cluster.service.ClusterApplierService$UpdateTask.run(ClusterApplierService.java:180)
2022-01-06T20:14:39.1438863Z 	at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:733)
2022-01-06T20:14:39.1442615Z 	at org.opensearch.common.util.concurrent.PrioritizedOpenSearchThreadPoolExecutor$TieBreakingPrioritizedRunnable.runAndClean(PrioritizedOpenSearchThreadPoolExecutor.java:275)
2022-01-06T20:14:39.1447556Z 	at org.opensearch.common.util.concurrent.PrioritizedOpenSearchThreadPoolExecutor$TieBreakingPrioritizedRunnable.run(PrioritizedOpenSearchThreadPoolExecutor.java:238)
2022-01-06T20:14:39.1455252Z 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
2022-01-06T20:14:39.1457071Z 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
2022-01-06T20:14:39.1458243Z 	at java.base/java.lang.Thread.run(Thread.java:832)

davidlago · 2022-01-07T15:12:29Z

I've installed the plugin as produced by this PR into a new server, and it works just fine, no complains about missing log4j-core as it is provided by the server. It looks like the error we are seeing in CI is just a product of how ITs are run.

dlvenable · 2022-01-07T17:11:50Z

Regarding the error, it does seem that log4j might be missing from your test classpath. A couple possible solutions:

Add log4j back, but keep the scope set the test. Your library would not add a transitive dependency to other libraries for log4j in this case.
Use slf4j-simple as your SLF4J binding in the test scope. I'm not sure if this will work or not because there may already log4j SLF4J binding already in your classpath. Again, this will not add a transitive dependency.

Both solutions would have a dependency with <scope>test</scope>.

Here is how it could look for the slf4j-simple approach:

<dependency>
    <groupId>org.slf4j</groupId>
    <artifactId>slf4j-simple</artifactId>
    <version>${slf4j.version}</version>
    <scope>test</scope>
</dependency>

Signed-off-by: Dave Lago <[email protected]>

davidlago · 2022-01-07T20:41:02Z

Thanks @dlvenable, submitted an update adding it back as a test dependency.

So just check me here to make sure I'm reasoning about this the right way:

Before this PR, log4j was a provided dependency. Our distribution did not include it. Now we have it as a test dependency, which is also not included in our distribution.

The difference is that, by updating every use to SLF4J instead, we don't rely on a specific version of log4j-core being provided, so if a new vulnerability is discovered and OpenSearch needs to move to 2.17.3, they can just upgrade and re-package with the same security plugin (minus a version bump).

davidlago · 2022-01-07T21:54:00Z

OK, that did the trick. CI passing now.

dlvenable · 2022-01-07T22:37:18Z

The current pom.xml in main has:

<dependency>
    <groupId>org.apache.logging.log4j</groupId>
    <artifactId>log4j-slf4j-impl</artifactId>
    <version>${log4j.version}</version>
    <exclusions>
        <exclusion>
            <groupId>org.apache.logging.log4j</groupId>
            <artifactId>log4j-api</artifactId>
        </exclusion>
    </exclusions>
</dependency>

So the project was adding a transitive dependency on the log4j SLF4J binding that other projects would take in. Aside from the security concerns with log4j, this is also not good practice. SLF4J lets libraries log statements without caring which logger the application will end up using.

Your PR now makes this a test dependency. So it is only needed and used in the test runtime paths. It is good to isolate it there.

I packaged this plugin using the build instructions: mvn clean package -Padvanced -DskipTests on both main and your branch.

Then I unzipped this file: target/releases/opensearch-security-1.3.0.0-SNAPSHOT.zip

main:

ls -s | grep log4j
   48 log4j-slf4j-impl-2.17.1.jar

Your branch:

ls -s | grep log4j

You can see that the log4j-slf4j-impl is no longer present. But, it appears that log4j-core was never present.

peternied

Great contributions from everyone! 🥳

In OpenSearch 1.3.0 we removed an explicit dependency on log4j opensearch-project#1563 this caused the log4j-slf4j-impl-X.XX.X.jar file no longer to be included in the plugin. When the plugin started up the default no-op logger was used instead. This prevented the security plugin from logging anything, yikes. When looking at the other opensearch plugins, none of them use slf4. Rather than continue using a seperate logging process, moving to the standard log4j Logger/LogManager. Tested this change on 2.0.0-alpha distribution and logging works as expected. Signed-off-by: Peter Nied <[email protected]>

In OpenSearch 1.3.0 we removed an explicit dependency on log4j #1563 this caused the log4j-slf4j-impl-X.XX.X.jar file no longer to be included in the plugin. When the plugin started up the default no-op logger was used instead. This prevented the security plugin from logging anything, yikes. When looking at the other opensearch plugins, none of them use slf4. Rather than continue using a seperate logging process, moving to the standard log4j Logger/LogManager. Tested this change on 2.0.0-alpha distribution and logging works as expected. Signed-off-by: Peter Nied <[email protected]>

In OpenSearch 1.3.0 we removed an explicit dependency on log4j opensearch-project#1563 this caused the log4j-slf4j-impl-X.XX.X.jar file no longer to be included in the plugin. When the plugin started up the default no-op logger was used instead. This prevented the security plugin from logging anything, yikes. When looking at the other opensearch plugins, none of them use slf4. Rather than continue using a seperate logging process, moving to the standard log4j Logger/LogManager. Tested this change on 2.0.0-alpha distribution and logging works as expected. Signed-off-by: Peter Nied <[email protected]> (cherry picked from commit 54a920b)

* Switch to log4j logger (#1751) In OpenSearch 1.3.0 we removed an explicit dependency on log4j #1563 this caused the log4j-slf4j-impl-X.XX.X.jar file no longer to be included in the plugin. When the plugin started up the default no-op logger was used instead. This prevented the security plugin from logging anything, yikes. When looking at the other opensearch plugins, none of them use slf4. Rather than continue using a seperate logging process, moving to the standard log4j Logger/LogManager. Tested this change on 2.0.0-alpha distribution and logging works as expected. Signed-off-by: Peter Nied <[email protected]> (cherry picked from commit 54a920b) * Fix 2 files that missed the update Signed-off-by: Peter Nied <[email protected]> * Adds logger to missed files Signed-off-by: Darshit Chanpura <[email protected]> Co-authored-by: Peter Nied <[email protected]>

Revert some changes introduced by #1563 to correct work with log4j. Signed-off-by: Andrey Pustovetov <[email protected]>

Revert some changes introduced by #1563 to correct work with log4j. Signed-off-by: Andrey Pustovetov <[email protected]> (cherry picked from commit 68f5624)

Revert some changes introduced by #1563 to correct work with log4j. Signed-off-by: Andrey Pustovetov <[email protected]> (cherry picked from commit 68f5624) Co-authored-by: Andrey Pustovetov <[email protected]>

…t#1996) Revert some changes introduced by opensearch-project#1563 to correct work with log4j. Signed-off-by: Andrey Pustovetov <[email protected]> Signed-off-by: Stephen Crawford <[email protected]>

* Removed explicit dependency on log4j Signed-off-by: Dave Lago <[email protected]>

In OpenSearch 1.3.0 we removed an explicit dependency on log4j opensearch-project#1563 this caused the log4j-slf4j-impl-X.XX.X.jar file no longer to be included in the plugin. When the plugin started up the default no-op logger was used instead. This prevented the security plugin from logging anything, yikes. When looking at the other opensearch plugins, none of them use slf4. Rather than continue using a seperate logging process, moving to the standard log4j Logger/LogManager. Tested this change on 2.0.0-alpha distribution and logging works as expected. Signed-off-by: Peter Nied <[email protected]>

…t#1996) (opensearch-project#2024) Revert some changes introduced by opensearch-project#1563 to correct work with log4j. Signed-off-by: Andrey Pustovetov <[email protected]> (cherry picked from commit 68f5624) Co-authored-by: Andrey Pustovetov <[email protected]>

Removed explicit dependency on log4j

2994831

Signed-off-by: Dave Lago <[email protected]>

davidlago requested review from a team, cliu123 and dblock January 6, 2022 16:22

Removed extra space

63915b5

Signed-off-by: Dave Lago <[email protected]>

dblock previously approved these changes Jan 6, 2022

View reviewed changes

peternied previously approved these changes Jan 6, 2022

View reviewed changes

dlvenable reviewed Jan 6, 2022

View reviewed changes

Replaced log4j with slf4j-api

482855f

Signed-off-by: Dave Lago <[email protected]>

davidlago dismissed stale reviews from peternied and dblock via 482855f January 6, 2022 20:03

davidlago commented Jan 6, 2022

View reviewed changes

dlvenable reviewed Jan 7, 2022

View reviewed changes

Adding log4j-core as test dependency

0a81c13

Signed-off-by: Dave Lago <[email protected]>

dlvenable approved these changes Jan 7, 2022

View reviewed changes

peternied approved these changes Jan 7, 2022

View reviewed changes

cliu123 approved these changes Jan 8, 2022

View reviewed changes

davidlago merged commit 12ae0cb into opensearch-project:main Jan 8, 2022

davidlago deleted the remove-slf4j branch January 8, 2022 00:07

peternied mentioned this pull request Mar 28, 2022

[BUG] Unable to configure debug/trace logging for security plugins #1689

Closed

peternied mentioned this pull request Apr 7, 2022

Switch to log4j logger #1751

Merged

3 tasks

DarshitChanpura mentioned this pull request Apr 25, 2022

[Backport 1.x] Switch to log4j logger (#1751) #1789

Closed

3 tasks

DarshitChanpura mentioned this pull request Apr 25, 2022

[Backport 1.3] Switch to log4j logger (#1751) #1791

Merged

3 tasks

jchipmunk mentioned this pull request Aug 5, 2022

Triple audit logging fix (#1995) #1996

Merged

2 tasks

peternied pushed a commit that referenced this pull request Aug 12, 2022

Triple audit logging fix (#1995) (#1996)

68f5624

Revert some changes introduced by #1563 to correct work with log4j. Signed-off-by: Andrey Pustovetov <[email protected]>

wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023

Removed explicit dependency on log4j (opensearch-project#1563)

48b6447

* Removed explicit dependency on log4j Signed-off-by: Dave Lago <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed explicit dependency on log4j #1563

Removed explicit dependency on log4j #1563

davidlago commented Jan 6, 2022 •

edited

Loading

stockholmux commented Jan 6, 2022

dblock commented Jan 6, 2022

davidlago commented Jan 6, 2022

dlvenable Jan 6, 2022

davidlago Jan 6, 2022

andrross Jan 6, 2022

dlvenable Jan 6, 2022

davidlago Jan 6, 2022

davidlago Jan 6, 2022

dlvenable Jan 7, 2022

dlvenable Jan 7, 2022

dlvenable Jan 7, 2022

dlvenable Jan 7, 2022

davidlago commented Jan 7, 2022

davidlago commented Jan 7, 2022

dlvenable commented Jan 7, 2022

davidlago commented Jan 7, 2022

davidlago commented Jan 7, 2022

dlvenable commented Jan 7, 2022

peternied left a comment

Removed explicit dependency on log4j #1563

Removed explicit dependency on log4j #1563

Conversation

davidlago commented Jan 6, 2022 • edited Loading

Description

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Why these changes are required?

What is the old behavior before changes and new behavior after changes?

Issues Resolved

Testing

Check List

stockholmux commented Jan 6, 2022

dblock commented Jan 6, 2022

davidlago commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidlago commented Jan 7, 2022

davidlago commented Jan 7, 2022

dlvenable commented Jan 7, 2022

davidlago commented Jan 7, 2022

davidlago commented Jan 7, 2022

dlvenable commented Jan 7, 2022

peternied left a comment

Choose a reason for hiding this comment

davidlago commented Jan 6, 2022 •

edited

Loading