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

Identify extension Transport requests and permit handshake and extension registration actions #2599

Merged
merged 20 commits into from
May 3, 2023
Merged

Identify extension Transport requests and permit handshake and extension registration actions #2599

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e2b750d
WIP on rest layer authz
cwperks Mar 27, 2023
478523a
WIP on rest-layer authz
cwperks Mar 28, 2023
3444295
Extension handshake
cwperks Mar 28, 2023
eb03f38
Extension TLS
cwperks Mar 28, 2023
85dba00
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
3419692
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
9ef4e0f
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
b7686dd
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
298aa27
Remove SecurityRestFilterChanges to isolate extension TLS change
cwperks Mar 29, 2023
5a64ef7
Remove local check
cwperks Apr 2, 2023
62a076c
Use extensions feature flag
cwperks Apr 3, 2023
87c421b
Merge branch 'main' into extension-tls
cwperks Apr 10, 2023
40b9c98
Merge branch 'main' into extension-tls
cwperks Apr 22, 2023
b1f7369
Switch to values.iterator
cwperks Apr 22, 2023
a740ea4
Merge branch 'main' into extension-tls
cwperks Apr 27, 2023
6cdda66
Remove ImmutableOpenMap
cwperks Apr 27, 2023
bec4270
Merge branch 'main' into extension-tls
cwperks Apr 28, 2023
bca8b36
Check in extensions registry
cwperks May 1, 2023
e3e45ef
Remove Feature Flag check
cwperks May 2, 2023
67a541e
Remove unused import
cwperks May 2, 2023
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
2 changes: 2 additions & 0 deletions src/main/java/org/opensearch/security/support/ConfigConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ public class ConfigConstants {

public static final String OPENDISTRO_SECURITY_SSL_TRANSPORT_TRUSTED_CLUSTER_REQUEST = OPENDISTRO_SECURITY_CONFIG_PREFIX+"ssl_transport_trustedcluster_request";

public static final String OPENDISTRO_SECURITY_SSL_TRANSPORT_EXTENSION_REQUEST = OPENDISTRO_SECURITY_CONFIG_PREFIX+"ssl_transport_extension_request";


/**
* Set by the SSL plugin, this is the peer node certificate on the transport layer
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/org/opensearch/security/support/HeaderHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ public static boolean isDirectRequest(final ThreadContext context) {
return "direct".equals(context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_CHANNEL_TYPE))
|| context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_CHANNEL_TYPE) == null;
}

public static boolean isExtensionRequest(final ThreadContext context) {
return context.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_EXTENSION_REQUEST) == Boolean.TRUE;
}


public static String getSafeFromHeader(final ThreadContext context, final String headerName) {
Expand Down
12 changes: 9 additions & 3 deletions src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.opensearch.action.support.replication.TransportReplicationAction.ConcreteShardRequest;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.transport.TransportAddress;
import org.opensearch.common.util.FeatureFlags;
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.search.internal.ShardSearchRequest;
import org.opensearch.security.auditlog.AuditLog;
Expand Down Expand Up @@ -195,6 +196,7 @@ else if(!Strings.isNullOrEmpty(injectedUserHeader)) {
//also allow when issued from a remote cluster for cross cluster search
if ( !HeaderHelper.isInterClusterRequest(getThreadContext())
&& !HeaderHelper.isTrustedClusterRequest(getThreadContext())
&& !HeaderHelper.isExtensionRequest(getThreadContext())
&& !task.getAction().equals("internal:transport/handshake")
&& (task.getAction().startsWith("internal:") || task.getAction().contains("["))) {

Expand All @@ -216,14 +218,14 @@ else if(!Strings.isNullOrEmpty(injectedUserHeader)) {
transportChannel.sendResponse(ex);
return;
} else {

if(getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN) == null) {
getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN, Origin.TRANSPORT.toString());
}

//network intercluster request or cross search cluster request
if(HeaderHelper.isInterClusterRequest(getThreadContext())
|| HeaderHelper.isTrustedClusterRequest(getThreadContext())) {
|| HeaderHelper.isTrustedClusterRequest(getThreadContext())
|| HeaderHelper.isExtensionRequest(getThreadContext())) {

final String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER);
final String injectedRolesHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER);
Expand Down Expand Up @@ -256,7 +258,6 @@ else if(!Strings.isNullOrEmpty(injectedUserHeader)) {
}

} else {

//this is a netty request from a non-server node (maybe also be internal: or a shard request)
//and therefore issued by a transport client

Expand Down Expand Up @@ -326,6 +327,11 @@ protected void addAdditionalContextValues(final String action, final TransportRe
}
}

String extensionUniqueId = getThreadContext().getHeader("extension_unique_id");
Copy link
Member

Choose a reason for hiding this comment

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

For follow up PR, can we put this in a constant in core and use that constant here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@peternied yes that's a good idea to import the constant from core. After this PR, I plan to expand on the TLS logic to introduce extension_dns similar to node_dns that make this check stronger by verifying that the principal extracted from the extension certificate is present in a list of known principals and will include the change to make this a constant.

For the FeatureFlags, I do think it makes sense to include a reference to the feature flag since this feature is directly related to extensions though its not necessarily needed.

if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS) && extensionUniqueId != null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Can this check be stronger?

Absolutely, this check is basic at the moment and only checks for the presence of the header. A simple improvement on this would be to check not only the existence of the header, but also that it is a value contained in extensions/extensions.yml.

There should also be a check similar to nodes_dn that verifies that the principal extracted from the certificate of the extension matches a list of known extensions_dns that would be placed in the appropriate area of the extensions/extensions.yml configuration for the extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

I modified this to check for the ID in the extensions registry.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we could remove the FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS) check? It seems redundant as the extension_unique_id value won't be set and the ExtensionsManager won't find any matching extensions

Copy link
Member

Choose a reason for hiding this comment

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

+1 to removing redundant check. Also, we should add automated tests to verify this behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

@DarshitChanpura sure I will remove the check. I do think the check should remain, but I'll remove it since it if it seems redundant.

I'll look into how a test can be written to verify this behavior.

Copy link
Member

Choose a reason for hiding this comment

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

@cwperks Seems like there is a principal you'd like to see in how some features work that is at odds with our other practices. If you were to ask me what I think your principal is I'd state it as "Functionality for extensions should be behind a feature flag(s) to prevent issues on clusters that do not have extensions enabled". Does this seem correct, did you want to expand on this?

This is a counterpoint to "Reduce coupling whenever possible". When we make a call to a static function call to determine global state the responsibility of the code has been broadly increased, making maintenance more difficult.

I think this dialog represents different principals we'd like to see prioritized in the code, it might documenting and discussing as a tenant, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

@peternied Yes that's correct. In this particular case its new functionality in the security plugin that would directly let an extension handshake with an opensearch node and also submit transport requests to initialize the extension like registering its REST handlers with OpenSearch - all of this is specific to extensions and is not permitted for anything other than an extension. In this case, I can't see any abstraction to add here because it is functionality for extensions and since other extension functionality is gated with a feature flag I think its appropriate to include here too.

I don't see this as coupling necessarily except that there are 3 places that now rely on the same threadcontext header and I'm not sure how to do that otherwise.

Cross cluster scenarios are simpler because its the security plugin that populates the threadcontext header so the security plugin is the only codebase that needs to be aware of what that header is. I can't see a better way here currently.

Copy link
Member Author

@cwperks cwperks May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@DarshitChanpura @peternied I removed the feature flag check. There currently is no great way to test this change in an automated test. I think the best way to automate a check for this change will be an integration test that verifies that an extension can be initialized when connecting with an opensearch node with the security plugin installed. There is an open issue on the sdk repo for integration tests that will eventually validate this change.

getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_EXTENSION_REQUEST, Boolean.TRUE);
}

super.addAdditionalContextValues(action, request, localCerts, peerCerts, principal);
}
}