Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ public static void setTriviallyAllowingTestCode(boolean newValue) {
policyManager.setTriviallyAllowingTestCode(newValue);
}

public static void addEntitledTestPackages(String[] entitledTestPackages) {
policyManager.addEntitledTestPackages(entitledTestPackages);
}

public static void reset() {
if (policyManager != null) {
policyManager.reset();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

package org.elasticsearch.entitlement.runtime.policy;

import org.elasticsearch.common.util.ArrayUtils;
import org.elasticsearch.entitlement.runtime.policy.entitlements.Entitlement;
import org.elasticsearch.test.ESTestCase;

Expand All @@ -17,6 +18,7 @@
import java.nio.file.Path;
import java.security.CodeSource;
import java.security.ProtectionDomain;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Map;
Expand All @@ -29,6 +31,7 @@ public class TestPolicyManager extends PolicyManager {

boolean isActive;
boolean isTriviallyAllowingTestCode;
String[] entitledTestPackages = TEST_FRAMEWORK_PACKAGE_PREFIXES;

/**
* We don't have modules in tests, so we can't use the inherited map of entitlements per module.
Expand Down Expand Up @@ -60,6 +63,12 @@ public void setTriviallyAllowingTestCode(boolean newValue) {
this.isTriviallyAllowingTestCode = newValue;
}

public void addEntitledTestPackages(String[] entitledTestPackages) {
String[] packages = ArrayUtils.concat(this.entitledTestPackages, entitledTestPackages);
Arrays.sort(packages);
this.entitledTestPackages = packages;
}

/**
* Called between tests so each test is not affected by prior tests
*/
Expand Down Expand Up @@ -110,17 +119,14 @@ private boolean isEntitlementClass(Class<?> requestingClass) {
&& (requestingClass.getName().contains("Test") == false);
}

@Deprecated // TODO: reevaluate whether we want this.
// If we can simply check for dependencies the gradle worker has that aren't
// declared in the gradle config (namely org.gradle) that would be simpler.
private boolean isTestFrameworkClass(Class<?> requestingClass) {
String packageName = requestingClass.getPackageName();
for (String prefix : TEST_FRAMEWORK_PACKAGE_PREFIXES) {
if (packageName.startsWith(prefix)) {
return true;
}
int idx = Arrays.binarySearch(entitledTestPackages, packageName);
if (idx >= 0) {
return true;
}
return false;
idx = -idx - 2; // candidate package (insertion point - 1)
return idx >= 0 && idx < entitledTestPackages.length && packageName.startsWith(entitledTestPackages[idx]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably use a unit test, given the opportunity for off-by-one errors.

It inherits an existing bug where org.example.pack will be viewed as a prefix of org.example.package even though the two packages are unrelated. We somewhat got away with this before because it was a fixed set of packages known not to have this problem, but now that the API can add them dynamically, it becomes a possibility.

(We've overdue for writing a proper string prefix trie. 😅)

Also, I think our experience with FileAccessTree shows that this will fail for looking up org.example.foo if the array already contains org.example and org.example.bar. In this case, the binary search will land on org.example.bar which is not a prefix. In FileAccessTree we deal with this by removing entries that have a prefix already in the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @prdoyle! I've added respective tests and fixed the issues mentioned above. Forbidding adding redundant entries here, which I think is sufficient for the use case.

}

private boolean isTestCode(Class<?> requestingClass) {
Expand Down Expand Up @@ -163,6 +169,10 @@ private boolean isTestCode(Class<?> requestingClass) {
"org.bouncycastle.jsse.provider" // Used in test code if FIPS is enabled, support more fine-grained config in ES-12128
};

static {
Arrays.sort(TEST_FRAMEWORK_PACKAGE_PREFIXES);
}

@Override
protected ModuleEntitlements getEntitlements(Class<?> requestingClass) {
return classEntitlementsMap.computeIfAbsent(requestingClass, this::computeEntitlements);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -517,16 +517,30 @@ protected void afterIfSuccessful() throws Exception {}
public @interface WithEntitlementsOnTestCode {
}

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
@Inherited
public @interface EntitledTestPackages {
String[] value();
}

@BeforeClass
public static void setupEntitlementsForClass() {
boolean withoutEntitlements = getTestClass().isAnnotationPresent(WithoutEntitlements.class);
boolean withEntitlementsOnTestCode = getTestClass().isAnnotationPresent(WithEntitlementsOnTestCode.class);
EntitledTestPackages entitledPackages = getTestClass().getAnnotation(EntitledTestPackages.class);

if (TestEntitlementBootstrap.isEnabledForTest()) {
TestEntitlementBootstrap.setActive(false == withoutEntitlements);
TestEntitlementBootstrap.setTriviallyAllowingTestCode(false == withEntitlementsOnTestCode);
if (entitledPackages != null) {
assert withEntitlementsOnTestCode == false : "Cannot use @WithEntitlementsOnTestCode together with @EntitledTestPackages";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these contradict each other

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Why is that? I can imagine a case where we want entitlements enforced on test code, but also want to grant certain test packages.

assert entitledPackages.value().length > 0 : "No test packages specified in @EntitledTestPackages";
TestEntitlementBootstrap.addEntitledTestPackages(entitledPackages.value());
}
} else if (withEntitlementsOnTestCode) {
throw new AssertionError(
"Cannot use WithEntitlementsOnTestCode on tests that are not configured to use entitlements for testing"
"Cannot use @WithEntitlementsOnTestCode on tests that are not configured to use entitlements for testing"
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ org.apache.httpcomponents.httpasyncclient:
- manage_threads
unboundid.ldapsdk:
- set_https_connection_properties # TODO: review if we need this once we have proper test coverage
- inbound_network # For com.unboundid.ldap.listener.LDAPListener
- outbound_network
- manage_threads
- write_system_properties:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.script.mustache.MustacheScriptEngine;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.ESTestCase.EntitledTestPackages;
import org.elasticsearch.threadpool.TestThreadPool;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.watcher.ResourceWatcherService;
Expand Down Expand Up @@ -106,6 +107,7 @@
* The username used to authenticate then has to be in the form of CN=user. Finally the username needs to be added as an
* additional bind DN with a password in the test setup since it really is not a DN in the ldif file
*/
@EntitledTestPackages(value = { "com.unboundid.ldap.listener" }) // tests start LDAP server that listens for incoming connections
public class ActiveDirectoryRealmTests extends ESTestCase {

private static final String PASSWORD = "password";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.env.TestEnvironment;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.ESTestCase.EntitledTestPackages;
import org.elasticsearch.watcher.ResourceWatcherService;
import org.elasticsearch.xpack.core.XPackSettings;
import org.elasticsearch.xpack.core.security.authc.RealmConfig;
Expand Down Expand Up @@ -73,6 +74,7 @@
import static org.elasticsearch.xpack.core.security.authc.ldap.support.SessionFactorySettings.URLS_SETTING;
import static org.hamcrest.Matchers.is;

@EntitledTestPackages(value = { "com.unboundid.ldap.listener" }) // tests start LDAP server that listens for incoming connections
public abstract class LdapTestCase extends ESTestCase {

protected static final RealmConfig.RealmIdentifier REALM_IDENTIFIER = new RealmConfig.RealmIdentifier("ldap", "ldap1");
Expand Down