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.

Sign up for GitHub

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

Allow Plugins to request to perform cluster actions and index actions with their assigned PluginSubject and prompt on install #15778

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Allow Plugins to request to perform cluster actions and index actions with their assigned PluginSubject and prompt on install #15778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
43e8974
WIP on requesting perms and displaying on install
cwperks Sep 5, 2024
a841f06
Actually compile a fake plugin in InstallPluginCommandTests
cwperks Sep 5, 2024
edfbe36
Merge branch 'main' into identity-aware-plugin-request-perms
cwperks Sep 16, 2024
5821632
Add test for prompt when installing IdentityAwarePlugin
cwperks Sep 16, 2024
1106978
Update warning message
cwperks Sep 16, 2024
4a29367
Get requested actions from plugin-permissions.yml
cwperks Sep 17, 2024
f4bd921
Allow plugin dev to configure a description to describe why plugin is…
cwperks Sep 17, 2024
454a5dd
Add requested actions to PluginInfo and pass PluginInfo IdentityPlugi…
cwperks Sep 19, 2024
c031a1b
Merge branch 'main' into identity-aware-plugin-request-perms
cwperks Sep 19, 2024
a2fb0fd
Add null check
cwperks Sep 19, 2024
758b671
Check stream version
cwperks Sep 19, 2024
4cdd593
Check version when serializing
cwperks Sep 19, 2024
394c47a
Handle case where requestedActions is null
cwperks Sep 19, 2024
fde386d
Merge branch 'main' into identity-aware-plugin-request-perms
cwperks Sep 20, 2024
157740b
Ensure requestedActions is non-null
cwperks Sep 20, 2024
bf44a29
Simplify tests
cwperks Sep 20, 2024
8b16051
modify correct build.gradle
cwperks Sep 20, 2024
c12df2d
Remove unused code
cwperks Sep 20, 2024
abbb6f0
Remove unused import
cwperks Sep 20, 2024
df6853b
Merge branch 'main' into identity-aware-plugin-request-perms
cwperks Oct 2, 2024
6b64364
Merge branch 'main' into identity-aware-plugin-request-perms
cwperks Oct 25, 2024
51a6cde
Merge branch 'main' into identity-aware-plugin-request-perms
cwperks Oct 29, 2024
5b8c079
Update ToXContent
cwperks Oct 29, 2024
ddeb16b
Set to CURRENT until backport
cwperks Oct 29, 2024
ea1af1f
Merge branch 'main' into identity-aware-plugin-request-perms
cwperks Nov 5, 2024
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
33 changes: 32 additions & 1 deletion distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/InstallPluginCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import org.opensearch.common.SuppressForbidden;
import org.opensearch.common.collect.Tuple;
import org.opensearch.common.hash.MessageDigests;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.io.IOUtils;
import org.opensearch.env.Environment;

Expand Down Expand Up @@ -104,6 +105,9 @@
import java.util.stream.Collectors;

import static org.opensearch.cli.Terminal.Verbosity.VERBOSE;
import static org.opensearch.plugins.PluginInfo.CLUSTER_ACTIONS_SETTING;
import static org.opensearch.plugins.PluginInfo.DESCRIPTION_SETTING;
import static org.opensearch.plugins.PluginInfo.INDEX_ACTIONS_SETTING;

/**
* A command for the plugin cli to install a plugin into opensearch.
Expand Down Expand Up @@ -880,7 +884,34 @@
} else {
permissions = Collections.emptySet();
}
PluginSecurity.confirmPolicyExceptions(terminal, permissions, isBatch);

Path actions = tmpRoot.resolve(PluginInfo.OPENSEARCH_PLUGIN_ACTIONS);
Settings requestedActions = Settings.EMPTY;

if (Files.exists(actions)) {
requestedActions = PluginSecurity.parseRequestedActions(actions);
}

final Map<String, List<String>> requestedIndexActions = new HashMap<>();

final List<String> requestedClusterActions = CLUSTER_ACTIONS_SETTING.get(requestedActions);
final Settings requestedIndexActionsGroup = INDEX_ACTIONS_SETTING.get(requestedActions);
final String pluginActionDescription = DESCRIPTION_SETTING.get(requestedActions);
if (!requestedIndexActionsGroup.keySet().isEmpty()) {
for (String indexPattern : requestedIndexActionsGroup.keySet()) {
List<String> indexActionsForPattern = requestedIndexActionsGroup.getAsList(indexPattern);
requestedIndexActions.put(indexPattern, indexActionsForPattern);
}

Check warning on line 904 in distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/InstallPluginCommand.java

View check run for this annotation

Codecov / codecov/patch

distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/InstallPluginCommand.java#L902-L904 

Added lines #L902 - L904 were not covered by tests
}

PluginSecurity.confirmPolicyExceptions(
terminal,
permissions,
pluginActionDescription,
requestedClusterActions,
requestedIndexActions,
isBatch
);

String targetFolderName = info.getTargetFolderName();
final Path destination = env.pluginsDir().resolve(targetFolderName);
Expand Down
109 changes: 78 additions & 31 deletions ...tion/tools/plugin-cli/src/test/java/org/opensearch/plugins/InstallPluginCommandTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,11 @@ static String createPluginUrl(String name, Path structure, String... additionalP
return createPlugin(name, structure, additionalProps).toUri().toURL().toString();
}

/** creates a plugin .zip and returns the url for testing */
static String createPluginWithRequestedActionsUrl(String name, Path structure, String... additionalProps) throws IOException {
return createPluginWithRequestedActions(name, structure, additionalProps).toUri().toURL().toString();
}

static void writePlugin(String name, Path structure, String... additionalProps) throws IOException {
String[] properties = Stream.concat(
Stream.of(
Expand All @@ -286,6 +291,31 @@ static void writePlugin(String name, Path structure, String... additionalProps)
writeJar(structure.resolve("plugin.jar"), className);
}

static void writePluginWithRequestedActions(String name, Path structure, String... additionalProps) throws IOException {
String[] properties = Stream.concat(
Stream.of(
"description",
"fake desc",
"name",
name,
"version",
"1.0",
"opensearch.version",
Version.CURRENT.toString(),
"java.version",
System.getProperty("java.specification.version"),
"classname",
"FakePlugin"
),
Arrays.stream(additionalProps)
).toArray(String[]::new);

PluginTestUtil.writePluginProperties(structure, properties);
writePluginPermissionsYaml(structure);
String className = name.substring(0, 1).toUpperCase(Locale.ENGLISH) + name.substring(1) + "Plugin";
writeJar(structure.resolve("plugin.jar"), className);
}

static void writePlugin(String name, Path structure, SemverRange opensearchVersionRange, String... additionalProps) throws IOException {
String[] properties = Stream.concat(
Stream.of(
Expand Down Expand Up @@ -326,11 +356,25 @@ static void writePluginSecurityPolicy(Path pluginDir, String... permissions) thr
Files.write(pluginDir.resolve("plugin-security.policy"), securityPolicyContent.toString().getBytes(StandardCharsets.UTF_8));
}

static void writePluginPermissionsYaml(Path pluginDir, String... permissions) throws IOException {
String permissionsYamlContent = "cluster.actions:\n"
+ " - cluster:monitor/health\n"
+ "indices.actions:\n"
+ " example-index*:\n"
+ " - indices:data/write/index*";
Files.write(pluginDir.resolve("plugin-permissions.yml"), permissionsYamlContent.getBytes(StandardCharsets.UTF_8));
}

static Path createPlugin(String name, Path structure, String... additionalProps) throws IOException {
writePlugin(name, structure, additionalProps);
return writeZip(structure, null);
}

static Path createPluginWithRequestedActions(String name, Path structure, String... additionalProps) throws IOException {
writePluginWithRequestedActions(name, structure, additionalProps);
return writeZip(structure, null);
}

void installPlugin(String pluginUrl, Path home) throws Exception {
installPlugin(pluginUrl, home, skipJarHellCommand);
}
Expand Down Expand Up @@ -1540,43 +1584,37 @@ private String signature(final byte[] bytes, final PGPSecretKey secretKey) {
// checks the plugin requires a policy confirmation, and does not install when that is rejected by the user
// the plugin is installed after this method completes
private void assertPolicyConfirmation(Tuple<Path, Environment> env, String pluginZip, String... warnings) throws Exception {
for (int i = 0; i < warnings.length; ++i) {
String warning = warnings[i];
for (int j = 0; j < i; ++j) {
terminal.addTextInput("y"); // accept warnings we have already tested
}
// default answer, does not install
terminal.addTextInput("");
UserException e = expectThrows(UserException.class, () -> installPlugin(pluginZip, env.v1()));
assertEquals("installation aborted by user", e.getMessage());

assertThat(terminal.getErrorOutput(), containsString("WARNING: " + warning));
try (Stream<Path> fileStream = Files.list(env.v2().pluginsDir())) {
assertThat(fileStream.collect(Collectors.toList()), empty());
}
// default answer, does not install
terminal.addTextInput("");
UserException e = expectThrows(UserException.class, () -> installPlugin(pluginZip, env.v1()));
assertEquals("installation aborted by user", e.getMessage());

// explicitly do not install
terminal.reset();
for (int j = 0; j < i; ++j) {
terminal.addTextInput("y"); // accept warnings we have already tested
}
terminal.addTextInput("n");
e = expectThrows(UserException.class, () -> installPlugin(pluginZip, env.v1()));
assertEquals("installation aborted by user", e.getMessage());
assertThat(terminal.getErrorOutput(), containsString("WARNING: " + warning));
try (Stream<Path> fileStream = Files.list(env.v2().pluginsDir())) {
assertThat(fileStream.collect(Collectors.toList()), empty());
}
try (Stream<Path> fileStream = Files.list(env.v2().pluginsDir())) {
assertThat(fileStream.collect(Collectors.toList()), empty());
}

// allow installation
for (String warning : warnings) {
assertThat(terminal.getErrorOutput(), containsString(warning));
}

// explicitly do not install
terminal.reset();
for (int j = 0; j < warnings.length; ++j) {
terminal.addTextInput("y");
terminal.addTextInput("n");
e = expectThrows(UserException.class, () -> installPlugin(pluginZip, env.v1()));
assertEquals("installation aborted by user", e.getMessage());
try (Stream<Path> fileStream = Files.list(env.v2().pluginsDir())) {
assertThat(fileStream.collect(Collectors.toList()), empty());
}
for (String warning : warnings) {
assertThat(terminal.getErrorOutput(), containsString(warning));
}

// allow installation
terminal.reset();
terminal.addTextInput("y");
installPlugin(pluginZip, env.v1());
for (String warning : warnings) {
assertThat(terminal.getErrorOutput(), containsString("WARNING: " + warning));
assertThat(terminal.getErrorOutput(), containsString(warning));
}
}

Expand All @@ -1586,7 +1624,16 @@ public void testPolicyConfirmation() throws Exception {
writePluginSecurityPolicy(pluginDir, "setAccessible", "setFactory");
String pluginZip = createPluginUrl("fake", pluginDir);

assertPolicyConfirmation(env, pluginZip, "plugin requires additional permissions");
assertPolicyConfirmation(env, pluginZip, "WARNING: plugin requires additional permissions");
assertPlugin("fake", pluginDir, env.v2());
}

public void testRequestedActionsConfirmation() throws Exception {
Tuple<Path, Environment> env = createEnv(fs, temp);
Path pluginDir = createPluginDir(temp);
String pluginZip = createPluginWithRequestedActionsUrl("fake", pluginDir);

assertPolicyConfirmation(env, pluginZip, "WARNING: plugin requires additional permissions", "Cluster Actions", "Index Actions");
assertPlugin("fake", pluginDir, env.v2());
}

Expand Down
4 changes: 3 additions & 1 deletion plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroIdentityPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.opensearch.plugins.ActionPlugin;
import org.opensearch.plugins.IdentityPlugin;
import org.opensearch.plugins.Plugin;
import org.opensearch.plugins.PluginInfo;
import org.opensearch.repositories.RepositoriesService;
import org.opensearch.rest.BytesRestResponse;
import org.opensearch.rest.RestChannel;
Expand Down Expand Up @@ -138,7 +139,8 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
}
}

public PluginSubject getPluginSubject(Plugin plugin) {
@Override
public PluginSubject getPluginSubject(PluginInfo pluginInfo) {
return new ShiroPluginSubject(threadPool);
}
}
10 changes: 6 additions & 4 deletions server/src/main/java/org/opensearch/identity/IdentityService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@
import org.apache.logging.log4j.Logger;
import org.opensearch.OpenSearchException;
import org.opensearch.common.annotation.InternalApi;
import org.opensearch.common.collect.Tuple;
import org.opensearch.common.settings.Settings;
import org.opensearch.identity.noop.NoopIdentityPlugin;
import org.opensearch.identity.tokens.TokenManager;
import org.opensearch.plugins.IdentityAwarePlugin;
import org.opensearch.plugins.IdentityPlugin;
import org.opensearch.plugins.Plugin;
import org.opensearch.plugins.PluginInfo;
import org.opensearch.threadpool.ThreadPool;

import java.util.List;
Expand Down Expand Up @@ -63,11 +65,11 @@ public TokenManager getTokenManager() {
return identityPlugin.getTokenManager();
}

public void initializeIdentityAwarePlugins(final List<IdentityAwarePlugin> identityAwarePlugins) {
public void initializeIdentityAwarePlugins(final List<Tuple<PluginInfo, Plugin>> identityAwarePlugins) {
if (identityAwarePlugins != null) {
for (IdentityAwarePlugin plugin : identityAwarePlugins) {
PluginSubject pluginSubject = identityPlugin.getPluginSubject((Plugin) plugin);
plugin.assignSubject(pluginSubject);
for (Tuple<PluginInfo, Plugin> pluginTuple : identityAwarePlugins) {
PluginSubject pluginSubject = identityPlugin.getPluginSubject(pluginTuple.v1());
((IdentityAwarePlugin) pluginTuple.v2()).assignSubject(pluginSubject);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/opensearch/identity/noop/NoopIdentityPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import org.opensearch.identity.Subject;
import org.opensearch.identity.tokens.TokenManager;
import org.opensearch.plugins.IdentityPlugin;
import org.opensearch.plugins.Plugin;
import org.opensearch.plugins.PluginInfo;
import org.opensearch.threadpool.ThreadPool;

/**
Expand Down Expand Up @@ -49,7 +49,7 @@ public TokenManager getTokenManager() {
}

@Override
public PluginSubject getPluginSubject(Plugin plugin) {
public PluginSubject getPluginSubject(PluginInfo pluginInfo) {
return new NoopPluginSubject(threadPool);
}
}
6 changes: 4 additions & 2 deletions server/src/main/java/org/opensearch/node/Node.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
import org.opensearch.common.StopWatch;
import org.opensearch.common.cache.module.CacheModule;
import org.opensearch.common.cache.service.CacheService;
import org.opensearch.common.collect.Tuple;
import org.opensearch.common.inject.Injector;
import org.opensearch.common.inject.Key;
import org.opensearch.common.inject.Module;
Expand Down Expand Up @@ -211,6 +212,7 @@
import org.opensearch.plugins.NetworkPlugin;
import org.opensearch.plugins.PersistentTaskPlugin;
import org.opensearch.plugins.Plugin;
import org.opensearch.plugins.PluginInfo;
import org.opensearch.plugins.PluginsService;
import org.opensearch.plugins.RepositoryPlugin;
import org.opensearch.plugins.ScriptPlugin;
Expand Down Expand Up @@ -1025,8 +1027,8 @@ protected Node(
// Add the telemetryAwarePlugin components to the existing pluginComponents collection.
pluginComponents.addAll(telemetryAwarePluginComponents);

List<IdentityAwarePlugin> identityAwarePlugins = pluginsService.filterPlugins(IdentityAwarePlugin.class);
identityService.initializeIdentityAwarePlugins(identityAwarePlugins);
List<Tuple<PluginInfo, Plugin>> identityAwarePluginTuples = pluginsService.filterPluginTuples(IdentityAwarePlugin.class);
identityService.initializeIdentityAwarePlugins(identityAwarePluginTuples);

final QueryGroupResourceUsageTrackerService queryGroupResourceUsageTrackerService = new QueryGroupResourceUsageTrackerService(
taskResourceTrackingService
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/opensearch/plugins/IdentityPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ public interface IdentityPlugin {
* Gets a subject corresponding to the passed plugin that can be utilized to perform transport actions
* in the plugin system context
*
* @param plugin The corresponding plugin
* @param pluginInfo The corresponding pluginInfo
* @return Subject corresponding to the plugin
*/
PluginSubject getPluginSubject(Plugin plugin);
PluginSubject getPluginSubject(PluginInfo pluginInfo);
Copy link
Collaborator

@reta reta Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we need to use PluginInfo? Plugin does not need to be told what permissions it is allowed to use: it comes from its permissions policy.

Copy link
Member Author

@cwperks cwperks Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Its lighter-weight than Plugin and its the vehicle that carries the actions that a plugin may request to perform with its PluginSubject.

The security plugin would need these 2 pieces of information to form the PluginSubject:

  1. Plugin Identifier (by convention it would be the canonical class name of the plugin)
  2. Requested Cluster/Index Actions <- This is what this PR is introducing. f.e. The security plugin is requesting permissions to write to the security auditlog index with its pluginSubject.

Copy link
Collaborator

@reta reta Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Got it, this is indeed the conceptual problem (at least I overlooked). I think this pull request is a bit ahead of its time, here is my reasoning:

  • the core has no notion of permissions whatsoever
  • as of today, this is purely and only security plugin feature, and
  • any alternative (identity?) plugin implementation would have known nothing about permissions

I still think that the idea to expose the plugin permissions is sound but we need a way to formalize the permissions first and (may be?) make core / other plugins aware of them (basically, do what we have done with Subject etc).

Why I think that is important, when we install any plugin right now, the security policy will be enforced no matter what. With permissions, as it stands now, this is purely hint, nothing will happen if security plugin is not installed / disabled (or any other its replacement is present).

@peternied sorry to dragging you here, but I think the subject of permissions what discussed before, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW I think presenting this information to an administrator installing a plugin empowers them with more information about what actions a plugin will perform in the cluster regardless if the security plugin is installed or not.

I left a comment here about complications of putting system index protection directly in the core. The biggest challenge with that is that operators may need to be able to perform invasive actions on system indices and do so by presenting the admin certificate which is another feature of the security plugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

JSM Permissions actually work the same way. A cluster operator is prompted with the permissions on install even if they run opensearch with JSM disabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW I think presenting this information to an administrator installing a plugin empowers them with more information about what actions a plugin will perform in the cluster regardless if the security plugin is installed or not.

This is right, and I agree with that. The question is how to do that in a way that if administrator says "Looks good" but nothing is enforced?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right its not possible to disable outside of the tests. JSM being removed in JDK 24 is a disruptive change. lmk if I can help with review on any items.

Copy link
Collaborator

@reta reta Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please join the discussion #1687 (with respect to JDK-24 subject)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @reta, I opened a draft PR in core to add a new sandbox module for system index protection: #16695

In its current state it is a crude implementation and would need to be expanded upon to match the functionality that the security plugin provides.

I wanted to open a draft to demonstrate some of the challenges in moving system index protection to the core. Ultimately, I think it is a good idea so that they are protected regardless if the security plugin is installed and enabled.

One of the biggest challenges is generic ActionRequest -> indices() resolution in an action filter. The way this works in the security plugin is the IndexResolverReplacer.getOrReplaceAllIndices().

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @reta, I opened a draft PR in core to add a new sandbox module for system index protection: #16695

Apologies for the delay @cwperks , I will try to find the time next week to look at it, thanks !

}
Loading
Loading