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

[cwperks]

Description

In #14630, a new extension point was created called IdentityAwarePlugin that has a single method called assignSubject. The subject that is given to IdentityAwarePlugin is intended to be a replacement for try (ThreadContext.StoredContext ctx = threadContext.stashContext() { ... }) and ensures that the security plugin can enforce authz checks on transport actions in this privileged block. The replacement is utilizing the assigned plugin subject and instead wrapping a block with pluginSubject.runAs(() -> { ... }) which injects an identity associated with the plugin so that the Security plugin (i.e. the IdentityPlugin) can authorize the transport actions instead of allowing the plugin to operate unrestricted.

This PR allows a plugin to define a plugin-permissions.yml file on the same level as plugin-security.policy file that can have 2 keys: 1) cluster.actions and 2) indices.actions

Example

description: >
  This plugin needs permission to write to the security audit log index when configured
  to use an OpenSearch Index for the audit log. By default the index will match the pattern
  'security-auditlog-'YYYY.MM.dd, but can be configured with the plugins.security.audit.config.index
  setting
cluster.actions:
  - cluster:monitor/health
index.actions:
  security-auditlog*:
    - indices:data/write/index*

This PR modifies the plugin-cli to prompt a cluster admin with the requested permissions. See an example below where I modified the security plugin to be an IdentityAwarePlugin and request permission to write to the security-auditlog-* index pattern. The security plugin always needs to be able to write to this index pattern regardless of the authenticated user's permissions.

➜  opensearch-3.0.0-SNAPSHOT git:(identity-aware-plugin-request-perms) ./bin/opensearch-plugin install file:/Users/cwperx/Projects/opensearch/security/build/distributions/opensearch-security-3.0.0.0-SNAPSHOT.zip
-> Installing file:/Users/cwperx/Projects/opensearch/security/build/distributions/opensearch-security-3.0.0.0-SNAPSHOT.zip
-> Downloading file:/Users/cwperx/Projects/opensearch/security/build/distributions/opensearch-security-3.0.0.0-SNAPSHOT.zip
[=================================================] 100%
Sep 05, 2024 4:45:01 PM org.apache.lucene.internal.vectorization.VectorizationProvider lookup
WARNING: Java vector incubator module is not readable. For optimal vector performance, pass '--add-modules jdk.incubator.vector' to enable Vector API.
16:45:01.758 [main] INFO  org.opensearch.security.ssl.transport.SSLConfig - SSL dual mode is disabled
16:45:01.759 [main] WARN  org.opensearch.security.OpenSearchSecurityPlugin - OpenSearch Security plugin installed but disabled. This can expose your configuration (including passwords) to the public.
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@     WARNING: plugin requires additional permissions     @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
* java.io.FilePermission /proc/sys/net/core/somaxconn#plus read
* java.lang.RuntimePermission accessClassInPackage.com.sun.jndi.*
* java.lang.RuntimePermission accessClassInPackage.sun.misc
* java.lang.RuntimePermission accessClassInPackage.sun.nio.ch
* java.lang.RuntimePermission accessClassInPackage.sun.security.x509
* java.lang.RuntimePermission accessDeclaredMembers
* java.lang.RuntimePermission accessUserInformation
* java.lang.RuntimePermission createClassLoader
* java.lang.RuntimePermission getClassLoader
* java.lang.RuntimePermission setContextClassLoader
* java.lang.RuntimePermission shutdownHooks
* java.lang.reflect.ReflectPermission suppressAccessChecks
* java.net.NetPermission getNetworkInformation
* java.net.NetPermission getProxySelector
* java.net.SocketPermission * connect,accept,resolve
* java.security.SecurityPermission getProperty.org.bouncycastle.ec.max_f2m_field_size
* java.security.SecurityPermission getProperty.org.bouncycastle.pkcs12.default
* java.security.SecurityPermission getProperty.org.bouncycastle.rsa.max_mr_tests
* java.security.SecurityPermission getProperty.org.bouncycastle.rsa.max_size
* java.security.SecurityPermission getProperty.ssl.KeyManagerFactory.algorithm
* java.security.SecurityPermission insertProvider.BC
* java.security.SecurityPermission org.apache.xml.security.register
* java.security.SecurityPermission putProviderProperty.BC
* java.security.SecurityPermission removeProviderProperty.BC
* java.security.SecurityPermission setProperty.ocsp.enable
* java.util.PropertyPermission * read,write
* javax.security.auth.AuthPermission doAs
* javax.security.auth.AuthPermission modifyPrivateCredentials
* javax.security.auth.kerberos.ServicePermission * accept
See http://docs.oracle.com/javase/8/docs/technotes/guides/security/permissions.html
for descriptions of what these permissions allow and the associated risks.

Plugin requests permission to perform the following transport actions. Any index
pattern that appears below is a default value and may change depending on plugin settings.

Cluster Actions
---------------

* cluster:monitor/health

Index Actions
-------------

Index Pattern: security-auditlog-*

* indices:data/write/index

Continue with installation? [y/N]

In order to extract the requested actions in plugin-cli I had to create a new method in the PluginsService to try instantiating a plugin with empty settings.

Related Issues

#15958

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[cwperks]

@sohami I opened this Draft PR to show how a mechanism can work for plugins to request to perform an enumerable set of transport actions within a pluginSubject.runAs(() -> { ... }) block (replacement for try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { ... }).

opensearch-project/opensearch-plugins#238 (comment)'

@prudhvigodithi You may be interested in this PR as well.

[github-actions]

❌ Gradle check result for a841f06: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for c12df2d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for abbb6f0: SUCCESS

[github-actions]

❌ Gradle check result for df6853b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❕ Gradle check result for df6853b: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[github-actions]

❌ Gradle check result for 6b64364: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 5b8c079: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for ddeb16b: SUCCESS

[cwperks]

@reta I'd like to bring this PR back up in the near future. I addressed the comments to read the requested perms from a file instead of having a plugin define them in code. This also changes IdentityPlugin.getPluginSubject extension point to accept a PluginInfo option instead of the full Plugin object.

[reta]

@reta I'd like to bring this PR back up in the near future.

Thanks @cwperks , I will take a look within next few days, thanks!

[github-actions]

❕ Gradle check result for ea1af1f: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[reta]

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.

[cwperks]

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.

[reta]

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?

[cwperks]

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.

[cwperks]

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.

[reta]

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?

[cwperks]

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.

[reta]

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

[cwperks]

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().

[reta]

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 !

[reta]

Why do we need this explicit permission groups? Every permission is prefixed with indices:* or cluster:* anyway, right? The groping could be deducted from the suffixes?

[reta]

Oh, I think it comes from the security plugin [1], right? It certainly would make sense to keep it the same. Could we extract this logic somewhere (if it makes sense, for permissions only) so core and security would not duplicate it?

[1] https://github.com/opensearch-project/security/blob/main/config/roles.yml

[cwperks]

Its not quite coming from the security plugin. The security plugin defines permissions like this: https://github.com/opensearch-project/security/blob/main/src/main/resources/static_config/static_roles.yml#L11-L17

all_access:
  static: true
  description: "Allow full access to all indices and all cluster APIs"
  cluster_permissions:
    - "*"
  index_permissions:
    - index_patterns:
        - "*"
      allowed_actions:
        - "*"

Cluster actions would be a list of action names, but index actions need a corresponding index pattern that the actions would be applicable to. For ultimate flexibility, different index patterns may have a different set of index actions that a plugin would be permitted to perform on the target index pattern.

[nibix]

I figured that it might make sense to re-post a comment I wrote at opensearch-project/security#4896 (comment)

OpenSearch core already contains ground works for a protection of system indices. It works like this:

• In case a REST request is issues against a system index, the following message will be logged: this request accesses system indices: [{}], but in a future major version, direct access to system indices will be prevented by default
• Exception: The HTTP header X-opensearch-product-origin is present in the REST request
• Other exception: The RestHandler implementation overrides allowSystemIndexAccessByDefault() and returns true.

See this commit for details: 5c8b066

These concepts still come from the ES times. If I understand it correctly, the goal was to replace the warning message by an error as a breaking change with a major release.

IMHO, this concept makes totally sense for core. As @cwperks has pointed out, core has no safe means of authentication. What core can do is a kind of cooperative check: If a client assures that they need to perform operations on a system index, they are allowed to. If the client signals no concrete intent to operate on system index, they are not allowed to. This avoids unwanted operations in case of bugs on side of the client - be it an overly broad index pattern or a similar case. Of course, it does not provide real security. But that just takes into account the fact that core has no means to perform a secure authentication.

IMHO, a good strategy would be to build on the present ground works and to complete them. That would be also useful from a code hygiene point of view, as otherwise that existing code would be there without real purpose.

Relevant code pieces:

OpenSearch/server/src/main/java/org/opensearch/cluster/metadata/IndexNameExpressionResolver.java

Lines 359 to 378 in 57a6605

	private void checkSystemIndexAccess(Context context, Metadata metadata, Set<Index> concreteIndices, String[] originalPatterns) {
	if (context.isSystemIndexAccessAllowed() == false) {
	final List<String> resolvedSystemIndices = concreteIndices.stream()
	.map(metadata::index)
	.filter(IndexMetadata::isSystem)
	.map(i -> i.getIndex().getName())
	.sorted() // reliable order for testing
	.collect(Collectors.toList());
	if (resolvedSystemIndices.isEmpty() == false) {
	resolvedSystemIndices.forEach(
	systemIndexName -> deprecationLogger.deprecate(
	"open_system_index_access_" + systemIndexName,
	"this request accesses system indices: [{}], but in a future major version, direct access to system "
	+ "indices will be prevented by default",
	systemIndexName
	)
	);
	}
	}
	}

OpenSearch/server/src/main/java/org/opensearch/rest/RestController.java

Lines 374 to 381 in 57a6605

	if (handler.allowSystemIndexAccessByDefault() == false && request.header(OPENSEARCH_PRODUCT_ORIGIN_HTTP_HEADER) == null) {
	// The OPENSEARCH_PRODUCT_ORIGIN_HTTP_HEADER indicates that the request is coming from an OpenSearch product with a plan
	// to move away from direct access to system indices, and thus deprecation warnings should not be emitted.
	// This header is intended for internal use only.
	client.threadPool().getThreadContext().putHeader(SYSTEM_INDEX_ACCESS_CONTROL_HEADER_KEY, Boolean.FALSE.toString());
	}
	handler.handleRequest(request, responseChannel, client);

OpenSearch/server/src/main/java/org/opensearch/cluster/metadata/IndexNameExpressionResolver.java

Lines 767 to 774 in 57a6605

	/**
	* Determines whether or not system index access should be allowed in the current context.
	*
	* @return True if system index access should be allowed, false otherwise.
	*/
	public boolean isSystemIndexAccessAllowed() {
	return Booleans.parseBoolean(threadContext.getHeader(SYSTEM_INDEX_ACCESS_CONTROL_HEADER_KEY), true);
	}