From 2f24adadb871de4e802e45755d469b53a3931270 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Fri, 12 May 2023 22:04:07 +0000 Subject: [PATCH] Remove how extensions identiity is related to scope access Signed-off-by: Peter Nied --- .../identity/shiro/ShiroSubject.java | 11 --- .../extensions/ExtensionsManager.java | 3 - .../ExtensionTransportActionsHandler.java | 1 - .../opensearch/identity/IdentityService.java | 8 +- .../java/org/opensearch/identity/Subject.java | 14 ++- .../opensearch/identity/noop/NoopSubject.java | 6 -- .../main/java/org/opensearch/node/Node.java | 15 ++-- .../bootstrap/test-framework.policy | 1 - .../plugins/ExtensionPointScopeTests.java | 88 ------------------- 9 files changed, 15 insertions(+), 132 deletions(-) delete mode 100644 server/src/test/java/org/opensearch/plugins/ExtensionPointScopeTests.java diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java index 5c2896713a6a3..3208d2bb5d8ca 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java @@ -88,15 +88,4 @@ public void authenticate(AuthToken authenticationToken) { .orElseThrow(() -> new UnsupportedAuthenticationToken()); shiroSubject.login(authToken); } - - @Override - public Principal getAssociatedApplication() { - return null; - } - - @Override - public boolean isAllowed(final List scope) { - // Noop subject is always allowed for any scope checks - return true; - } } diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index f471a23239d90..11d40c8d7a42e 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -485,9 +485,6 @@ public void onIndexModule(IndexModule indexModule) throws UnknownHostException { private void onIndexModule(IndexModule indexModule, DiscoveryNode extensionNode) throws UnknownHostException { - // Authenticate subject as extension - // check if allowed - logger.info("onIndexModule index:" + indexModule.getIndex()); final CompletableFuture inProgressFuture = new CompletableFuture<>(); final CompletableFuture inProgressIndexNameFuture = new CompletableFuture<>(); diff --git a/server/src/main/java/org/opensearch/extensions/action/ExtensionTransportActionsHandler.java b/server/src/main/java/org/opensearch/extensions/action/ExtensionTransportActionsHandler.java index 956493db5ab60..3fba76b7d3c59 100644 --- a/server/src/main/java/org/opensearch/extensions/action/ExtensionTransportActionsHandler.java +++ b/server/src/main/java/org/opensearch/extensions/action/ExtensionTransportActionsHandler.java @@ -78,7 +78,6 @@ void registerAction(String action, String uniqueId) throws IllegalArgumentExcept if (actionToIdMap.putIfAbsent(action, uniqueId) != null) { throw new IllegalArgumentException("The action [" + action + "] you are trying to register is already registered"); } - // Register the action in the action module's dynamic actions map dynamicActionRegistry.registerDynamicAction( new ExtensionAction(uniqueId, action), diff --git a/server/src/main/java/org/opensearch/identity/IdentityService.java b/server/src/main/java/org/opensearch/identity/IdentityService.java index 807a0c15786b1..0b58ca72589de 100644 --- a/server/src/main/java/org/opensearch/identity/IdentityService.java +++ b/server/src/main/java/org/opensearch/identity/IdentityService.java @@ -9,9 +9,7 @@ import org.apache.logging.log4j.Logger; import org.opensearch.OpenSearchException; import org.opensearch.identity.noop.NoopIdentityPlugin; -import org.opensearch.extensions.ExtensionsManager; import java.util.List; - import org.opensearch.common.settings.Settings; import org.opensearch.plugins.IdentityPlugin; import java.util.stream.Collectors; @@ -26,16 +24,12 @@ public class IdentityService { private final Settings settings; private final IdentityPlugin identityPlugin; - private final ExtensionsManager extensionManager; private static IdentityService instance; public static IdentityService getInstance() { return instance; } - public IdentityService(final Settings settings, - final List identityPlugins, - final ExtensionsManager extensionManager) { + public IdentityService(final Settings settings, final List identityPlugins) { this.settings = settings; - this.extensionManager = extensionManager; if (identityPlugins.size() == 0) { log.debug("Identity plugins size is 0"); diff --git a/server/src/main/java/org/opensearch/identity/Subject.java b/server/src/main/java/org/opensearch/identity/Subject.java index ce30419d3a757..3e03a21946719 100644 --- a/server/src/main/java/org/opensearch/identity/Subject.java +++ b/server/src/main/java/org/opensearch/identity/Subject.java @@ -33,18 +33,16 @@ public interface Subject { */ void authenticate(final AuthToken token); - - /** - * Get the application-wide uniquely identifying associated application's principal - * @return Null, if there is not associated application - */ - @Nullable - Principal getAssociatedApplication(); - /** * Checks scopes of the current subject if they are allowed for any of the listed scopes * @param scope The scopes to check against the subject * @return true if allowed, false if none of the scopes are allowed. */ + /// Draft Pull Request Remarks + // Permissions haven't been implemented yet, and there are good reasons to have permissions and scopes overlap, + // as well as have disconnected. For the moment lets look past that debate and get feedback around how + // scope might be added inside of OpenSearch and connected into various systems create security barriers between + // systems. + // This will need to be addressed before this change can come out of draft boolean isAllowed(final List scope); } diff --git a/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java b/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java index 61d4184cba63e..3db993f0e3970 100644 --- a/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java +++ b/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java @@ -57,12 +57,6 @@ public void authenticate(AuthToken AuthToken) { // Do nothing as noop subject is always logged in } - @Override - public Principal getAssociatedApplication() { - // Noop subject is not aware of an application - return null; - } - @Override public boolean isAllowed(final List scope) { // Noop subject is always allowed for any scope checks diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index d68910b36ca41..f9dc6442b2232 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -457,19 +457,20 @@ protected Node( // Ensure to initialize Feature Flags via the settings from opensearch.yml FeatureFlags.initializeFeatureFlags(settings); - if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - this.extensionsManager = new ExtensionsManager(initialEnvironment.extensionDir()); - } else { - this.extensionsManager = new NoopExtensionsManager(); - } - final List identityPlugins = new ArrayList<>(); if (FeatureFlags.isEnabled(FeatureFlags.IDENTITY)) { // If identity is enabled load plugins implementing the extension point logger.info("Identity on so found plugins implementing: " + pluginsService.filterPlugins(IdentityPlugin.class).toString()); identityPlugins.addAll(pluginsService.filterPlugins(IdentityPlugin.class)); } - final IdentityService identityService = new IdentityService(settings, identityPlugins, extensionsManager); + + final IdentityService identityService = new IdentityService(settings, identityPlugins); + + if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { + this.extensionsManager = new ExtensionsManager(initialEnvironment.extensionDir()); + } else { + this.extensionsManager = new NoopExtensionsManager(); + } final Set additionalRoles = pluginsService.filterPlugins(Plugin.class) .stream() diff --git a/server/src/main/resources/org/opensearch/bootstrap/test-framework.policy b/server/src/main/resources/org/opensearch/bootstrap/test-framework.policy index 02237dcad831b..41076569a3c50 100644 --- a/server/src/main/resources/org/opensearch/bootstrap/test-framework.policy +++ b/server/src/main/resources/org/opensearch/bootstrap/test-framework.policy @@ -153,5 +153,4 @@ grant codeBase "file:${gradle.worker.jar}" { grant { // since the gradle test worker jar is on the test classpath, our tests should be able to read it permission java.io.FilePermission "${gradle.worker.jar}", "read"; - permission java.security.AllPermission; }; diff --git a/server/src/test/java/org/opensearch/plugins/ExtensionPointScopeTests.java b/server/src/test/java/org/opensearch/plugins/ExtensionPointScopeTests.java deleted file mode 100644 index a2fa4fbfbd9f7..0000000000000 --- a/server/src/test/java/org/opensearch/plugins/ExtensionPointScopeTests.java +++ /dev/null @@ -1,88 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.plugins; - -import org.opensearch.cluster.node.DiscoveryNode; -import org.opensearch.cluster.routing.ShardRouting; -import org.opensearch.common.settings.Settings; -import org.opensearch.index.IndexModule; -import org.opensearch.index.store.FsDirectoryFactory; -import org.opensearch.indices.recovery.RecoveryState; -import org.opensearch.node.MockNode; -import org.opensearch.test.OpenSearchTestCase; -import org.opensearch.plugins.*; - -import java.util.Arrays; -import java.util.Collections; -import java.util.Map; -import java.util.Set; -import java.util.stream.Collectors; -import java.lang.reflect.Modifier; -import java.util.Arrays; -import java.util.HashSet; -import java.util.Set; -import java.net.URL; -import java.util.Enumeration; -import java.util.jar.JarFile; -import java.util.jar.JarEntry; -// import com.google.common.reflect.ClassPath; - -import static org.opensearch.test.hamcrest.RegexMatcher.matches; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.hasToString; -import static org.hamcrest.Matchers.contains; - -public class ExtensionPointScopeTests extends OpenSearchTestCase { - - public void testAllExtensionPointsHaveAssociatedScope() throws Exception { - // - final Set extensionPointScopes = Arrays.stream(ExtensionPointScopes.values()).map(e -> e.name()).collect(Collectors.toSet()); - final String packageName = getClass().getPackage().getName(); - final ClassLoader classLoader = ClassLoader.getSystemClassLoader(); - final Set pluginClassNames = new HashSet<>(); - Enumeration resources = classLoader.getResources(packageName.replaceAll("\\.", "/")); - while (resources.hasMoreElements()) { - final URL resource = resources.nextElement(); - // Open the jar file and search for class files - String jarFileName = resource.getPath(); - try { - jarFileName = jarFileName.substring(0, jarFileName.indexOf('!')); - } catch (Exception e) { - System.err.println(e); -// continue; - } - - try (JarFile jarFile = new JarFile(jarFileName)) { - Enumeration entries = jarFile.entries(); - while (entries.hasMoreElements()) { - JarEntry entry = entries.nextElement(); - String entryName = entry.getName(); - if (entryName.endsWith(".class") && entryName.startsWith(packageName.replace('.', '/'))) { - String className = entryName.substring(0, entryName.length() - ".class".length()).replace('/', '.'); - pluginClassNames.add(className); - } - } - } catch (Exception e) { - System.err.println(e); - // Ignore any exceptions - // continue; - } - - } - System.out.println(pluginClassNames); - System.out.println(extensionPointScopes); - pluginClassNames.forEach(name -> { - assertThat(extensionPointScopes, contains(name)); - }); - - extensionPointScopes.forEach(name -> { - assertThat(pluginClassNames, contains(name)); - }); - } -} \ No newline at end of file