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

Removing the possibly-temporary test change from LoggingAuditTrailTests.java #81849

Open
ChrisHegarty opened this issue Dec 17, 2021 · 0 comments
Assignees
Labels

Comments

@ChrisHegarty
Copy link
Contributor

Removing the possibly-temporary test change from LoggingAuditTrailTests.java in PR #81709, results in a surprising test failure. But why?

Steps to reproduce.

  1. Remove the possibly-temporary test change:
x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java
@@ -247,7 +249,12 @@ public class LoggingAuditTrailTests extends ESTestCase {
         assertThat(properties.getProperty("appender.audit_rolling.layout.type"), is("PatternLayout"));
         final String patternLayoutFormat = properties.getProperty("appender.audit_rolling.layout.pattern");
         assertThat(patternLayoutFormat, is(notNullValue()));
+        patternLayout = PatternLayout.newBuilder().withPattern(patternLayoutFormat).withCharset(StandardCharsets.UTF_8).build();
-        patternLayout = AccessController.doPrivileged(
-            (PrivilegedAction<PatternLayout>) () -> PatternLayout.newBuilder()
-                .withPattern(patternLayoutFormat)
                .withCharset(StandardCharsets.UTF_8)
-                .build()
-        );
         customAnonymousUsername = randomAlphaOfLength(8);
         reservedRealmEnabled = randomBoolean();
     }
  1. Run the test:
    ./gradlew ':x-pack:plugin:security:test' --tests "org.elasticsearch.xpack.security.audit.logfile.LoggingAuditTrailTests"

  2. Observe the failure:

java.lang.AssertionError: 
Expected: an empty collection
   but: <[JNDI lookup class is not available because this JRE does not support JNDI. \
          JNDI string lookups will not be available, continuing configuration. \
          Ignoring java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "setContextClassLoader"), \
          JMX runtime input lookup class is not available because this JRE does not support JMX. \
          JMX lookups will not be available, continuing configuration. \
          Ignoring java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "setContextClassLoader")]>
	at __randomizedtesting.SeedInfo.seed([24CDAE861F3AF941:2C9011C23DEF8A1E]:0)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
	at org.junit.Assert.assertThat(Assert.java:956)
	at org.junit.Assert.assertThat(Assert.java:923)
        ...

Why did this not happed before, and why is the test needing a doPriv?

Between log4j 2.11.1 and 2.15.0 the org.apache.logging.log4j.core.util.Loader::newInstanceOf method has been changed [1] to set the thread context class loader, when attempting to loading given named class, e.g.

  ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
   try {
       Thread.currentThread().setContextClassLoader(getClassLoader());
      return clazz.cast(newInstanceOf(className));
   } finally {
       Thread.currentThread().setContextClassLoader(contextClassLoader);
   }

When running under a security manager, as the test does, setting the thread context class loader is not permitted. Since newInstanceOf is called from Interpolator::init from within a try-catch LinkageError | Exception, the exception is caught and logged, rather than being fatal.

[1] apache/logging-log4j2@497b409#diff-a486c14ecd59f815bff99401912bc40600b13074239116906de4b9568c531877

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant