Skip to content

Enable security manager for active directory tests #112411

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion test/fixtures/testcontainer-utils/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ configurations.all {
}

dependencies {
testImplementation project(':test:framework')
implementation project(':libs:elasticsearch-core')
implementation project(':test:framework')
api "junit:junit:${versions.junit}"
api "org.testcontainers:testcontainers:${versions.testcontainer}"
implementation "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

package org.elasticsearch.test.fixtures.testcontainers;

import org.elasticsearch.bootstrap.BootstrapForTesting;
import org.elasticsearch.test.fixtures.CacheableTestFixture;
import org.junit.Assume;
import org.junit.rules.TestRule;
Expand Down Expand Up @@ -66,7 +67,7 @@ public void start() {
Assume.assumeFalse("Docker support excluded on OS", EXCLUDED_OS);
Assume.assumeTrue("Docker probing succesful", DOCKER_PROBING_SUCCESSFUL);
withLogConsumer(new Slf4jLogConsumer(LOGGER));
super.start();
BootstrapForTesting.doWithSecurityManagerDisabled(super::start);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,14 @@
import org.elasticsearch.jdk.JarHell;
import org.elasticsearch.plugins.PluginDescriptor;
import org.elasticsearch.secure_sm.SecureSM;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.PrivilegedOperations;
import org.elasticsearch.test.mockito.SecureMockMaker;
import org.junit.Assert;

import java.io.Closeable;
import java.io.IOException;
import java.io.InputStream;
import java.io.UncheckedIOException;
import java.lang.invoke.MethodHandles;
import java.net.SocketPermission;
import java.net.URL;
Expand All @@ -53,6 +54,7 @@
import java.util.Objects;
import java.util.Properties;
import java.util.Set;
import java.util.function.Supplier;
import java.util.stream.Collectors;

import static com.carrotsearch.randomizedtesting.RandomizedTest.systemPropertyAsBoolean;
Expand Down Expand Up @@ -350,19 +352,19 @@ static Set<URL> parseClassPathWithSymlinks() throws Exception {
public static void ensureInitialized() {}

/**
* Temporarily dsiables security manager for a test.
*
* <p> This method is only callable by {@link org.elasticsearch.test.ESTestCase}.
* Temporarily disables security manager for a test.
*
* @return A closeable object which restores the test security manager
*/
@SuppressWarnings("removal")
public static Closeable disableTestSecurityManager() {
Copy link
Member

Choose a reason for hiding this comment

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

We could change this to return Releaseable so we don't need to catch IOException in the methods below?

var caller = Thread.currentThread().getStackTrace()[2];
if (ESTestCase.class.getName().equals(caller.getClassName()) == false) {
throw new SecurityException("Cannot disable test SecurityManager directly. Use @NoSecurityManager to disable on a test suite");
Copy link
Member Author

Choose a reason for hiding this comment

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

Note for core/infra: I've relaxed this requirement to enable broader use of disabling security manager for parts of testing. The most important thing is to ensure security manager is not disabled and forgotten. We already return a releasable object, and additional validation ensures we haven't forgotten to re-enable security manager between tests.

}
final var sm = System.getSecurityManager();
if (sm == null) {
throw new SecurityException(
"SecurityManager already disabled. This is indicative of a test bug. "
+ "Please ensure callers to this method close the returned Closeable."
);
}
AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
Security.setSecurityManager(null);
return null;
Expand All @@ -372,4 +374,28 @@ public static Closeable disableTestSecurityManager() {
return null;
});
}

/**
* Runs the given action, returning the produced object, without the security manager.
* This is a convenience method for {@link #disableTestSecurityManager()}.
*/
public static <T> T doWithSecurityManagerDisabled(Supplier<T> action) {
try (var ignore = BootstrapForTesting.disableTestSecurityManager()) {
return action.get();
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}

/**
* Runs the given action without the security manager.
* This is a convenience method for {@link #disableTestSecurityManager()}.
*/
public static void doWithSecurityManagerDisabled(Runnable action) {
try (var ignore = BootstrapForTesting.disableTestSecurityManager()) {
action.run();
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}
}
1 change: 0 additions & 1 deletion x-pack/qa/third-party/active-directory/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ tasks.named("forbiddenPatterns").configure {
}

tasks.named("test").configure {
systemProperty 'tests.security.manager', 'false'
include '**/*IT.class'
include '**/*Tests.class'
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
public abstract class AbstractActiveDirectoryTestCase extends ESTestCase {

@ClassRule
public static final SmbTestContainer smbFixture = new SmbTestContainer();
public static final SmbTestContainer smbFixture = SmbTestContainer.create();
// follow referrals defaults to false here which differs from the default value of the setting
// this is needed to prevent test logs being filled by errors as the default configuration of
// the tests run against a vagrant samba4 instance configured as a domain controller with the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public abstract class AbstractAdLdapRealmTestCase extends SecurityIntegTestCase
public static final String SECURITY_INDEX = "security";

@ClassRule
public static final SmbTestContainer smbFixture = new SmbTestContainer();
public static final SmbTestContainer smbFixture = SmbTestContainer.create();

private static final RoleMappingEntry[] AD_ROLE_MAPPING = new RoleMappingEntry[] {
new RoleMappingEntry("SHIELD: [ \"CN=SHIELD,CN=Users,DC=ad,DC=test,DC=elasticsearch,DC=com\" ]", """
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public class ActiveDirectoryGroupsResolverTests extends GroupsResolverTestCase {
private static final RealmConfig.RealmIdentifier REALM_ID = new RealmConfig.RealmIdentifier("active_directory", "ad");

@ClassRule
public static final SmbTestContainer smbFixture = new SmbTestContainer();
public static final SmbTestContainer smbFixture = SmbTestContainer.create();

@Before
public void setReferralFollowing() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class UserAttributeGroupsResolverTests extends GroupsResolverTestCase {
private static final RealmConfig.RealmIdentifier REALM_ID = new RealmConfig.RealmIdentifier("ldap", "realm1");

@ClassRule
public static final SmbTestContainer smbFixture = new SmbTestContainer();
public static final SmbTestContainer smbFixture = SmbTestContainer.create();

public void testResolve() throws Exception {
// falling back on the 'memberOf' attribute
Expand Down
1 change: 1 addition & 0 deletions x-pack/test/smb-fixture/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ apply plugin: 'elasticsearch.java'
apply plugin: 'elasticsearch.cache-test-fixtures'

dependencies {
implementation project(':test:framework')
api project(':test:fixtures:testcontainer-utils')
api "junit:junit:${versions.junit}"
api "org.testcontainers:testcontainers:${versions.testcontainer}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

package org.elasticsearch.test.fixtures.smb;

import org.elasticsearch.bootstrap.BootstrapForTesting;
import org.elasticsearch.test.fixtures.testcontainers.DockerEnvironmentAwareTestContainer;
import org.testcontainers.images.builder.ImageFromDockerfile;

Expand All @@ -16,7 +17,7 @@ public final class SmbTestContainer extends DockerEnvironmentAwareTestContainer
public static final int AD_LDAP_PORT = 636;
public static final int AD_LDAP_GC_PORT = 3269;

public SmbTestContainer() {
private SmbTestContainer() {
super(
new ImageFromDockerfile("es-smb-fixture").withDockerfileFromBuilder(
builder -> builder.from(DOCKER_BASE_IMAGE)
Expand All @@ -43,6 +44,10 @@ public SmbTestContainer() {
addExposedPort(AD_LDAP_GC_PORT);
}

public static SmbTestContainer create() {
return BootstrapForTesting.doWithSecurityManagerDisabled(SmbTestContainer::new);
}

public String getAdLdapUrl() {
return "ldaps://localhost:" + getMappedPort(AD_LDAP_PORT);
}
Expand Down