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.

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

[FEATURE] [DISCUSS] Implement Proxy Actions #525

Closed
4 tasks done
dbwiddis opened this issue Mar 6, 2023 · 14 comments · Fixed by #588
Closed
4 tasks done

[FEATURE] [DISCUSS] Implement Proxy Actions #525

dbwiddis opened this issue Mar 6, 2023 · 14 comments · Fixed by #588
Assignees
Labels
discuss enhancement New feature or request

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Mar 6, 2023

Is your feature request related to a problem?

The getActions() extension point has been implemented and allows an extension to execute its own actions. On OpenSearch, the ProxyAction handlers have been implemented to register these actions and forward requests to extensions. However, there is no handling of these actions on the SDK.

What solution would you like?

  • Change ExtensionsRunner to always instantiate SDKActionModule. Move the ActionExtension conditional into the module where the getActions() extension point is read.
  • Create a new action in the SDK to send a ProxyAction to OpenSearch to be executed on another extension. Handle the response.
  • Always register the action created in the step above in the SDKActionModule
  • Add handling of proxy actions received from OpenSearch, the handler should look up the SDKActionModule's getActions() to find the action to execute. Send the appropriate response.
  • (Not needed) Update SDKClient handling of actions exceptions: there will never be a null action map; and change the "this action isn't registered in this extension" to a fallback where the action is sent to OpenSearch via the (registered) action from the steps above.
    • Removed; there will always be a ProxyAction for this purpose so no fallback is needed/desired

What alternatives have you considered?

Forward requests to OpenSearch in a way other than sending an action.

Do you have any additional context?

See #107 for long term improvements on this approach.

@joshpalis
Copy link
Member

Thanks @dbwiddis for writing this up, I support this approach to triggering extension actions from another extension. I believe we can utilize a similar approach to support executing non-extension actions registered in OpenSearch as well.

For instance, in Anomaly Detection ADTaskManager, there are many instances in which AD needs to execute actions registered by the ReindexModule here:

        client.execute(UpdateByQueryAction.INSTANCE, updateByQueryRequest, ActionListener.wrap(r -> {
            List<BulkItemResponse.Failure> bulkFailures = r.getBulkFailures();
            if (isNullOrEmpty(bulkFailures)) {
                logger.debug("Updated {} child entity tasks state for detector task {}", r.getUpdated(), detectorTaskId);
            } else {
                logger.error("Failed to update child entity task's state for detector task {} ", detectorTaskId);
            }
        }, e -> logger.error("Exception happened when update child entity task's state for detector task " + detectorTaskId, e)));

Since the ReindexModule is a component of the min-distribution of OpenSearch, its actions will always be registered within the OpenSearch ActionModule here. It would be necessary to add support to not only execute extension actions from another extension, but to also trigger actions registered in OpenSearch by default.

Hooking onto your approach for bullet point four :

Update SDKClient handling of actions exceptions: there will never be a null action map; and change the "this action isn't registered in this extension" to a fallback where the action is sent to OpenSearch via the (registered) action from the two steps above.

We can add a check if the underlying action is executable via the provided node client within ExtensionsManager (ie checking if the action is registered within the action module of OpenSearch) and trigger the action from there. I see that there is a request class provided by ExtensionsManager : TransportActionRequestFromExtension that would be perfect for this use case. Since this includes the extensionId as a field, we have the information we need to return a response back to the originating extension.

@dbwiddis dbwiddis changed the title [FEATURE] Implement Proxy Actions [FEATURE] [DISCUSS] Implement Proxy Actions Mar 6, 2023
@dbwiddis
Copy link
Member Author

dbwiddis commented Mar 6, 2023

Thanks, @joshpalis, using OpenSearch actions was definitely one of the use cases I considered, and am contemplating how to potentially register some actions as "proxy actions" so they can be executed.

One concern I have is along the security front, though. How do we prevent any extension from arbitrarily executing actions on other extensions (or on OpenSearch if we enable that)? We're trying to use the Rest API as a single point of contact for "executing" things because it will include the appropriate authorization checks, but these transport-based calls completely bypass that. I'd even think we'd want to "opt in" to any particular action being enabled for remote execution, so it's more complex than just registering a list.

@saratvemulapalli
Copy link
Member

Thanks @dbwiddis and @joshpalis for writing this up.

The feature @dbwiddis talks about is to abstract the handling which is directly an overhead for extension instead move it over to SDKActionModule and have the boiler plate code in the SDK. 💯 It absolutely makes sense.

@joshpalis I understand you are hitting into a problem with extension to call an action registered on OpenSearch which is not supported. Also extension to extension communication can be done via ProxyAction.

These are 2 separate problems.
But overall it feels we are ending up in these discussions trying to understand when to use Rest vs transport.

Here are my thoughts:

  1. Extension to OpenSearch: Prefer using a Rest API to keep extension independent of OpenSearch core data structures.
  2. Extension to Extension: We dont have a way to reach an extension (as of now) other than using transport. For all extension to extension communication it will be over transport and via OpenSearch (this may change in future when extensions become a fleet/service by themselves).
  3. OpenSearch to Extension: Same as 2, we just use transport.

Let know me your thoughts @dbwiddis @joshpalis

@saratvemulapalli
Copy link
Member

Also this makes me think we have write up some design tenets for extensions. I can pick this up to provide general guidelines for all extension developers if that helps.

@cwperks
Copy link
Member

cwperks commented Mar 9, 2023

@dbwiddis In response to:

Thanks, @joshpalis, using OpenSearch actions was definitely one of the use cases I considered, and am contemplating how to potentially register some actions as "proxy actions" so they can be executed.

One concern I have is along the security front, though. How do we prevent any extension from arbitrarily executing actions on other extensions (or on OpenSearch if we enable that)? We're trying to use the Rest API as a single point of contact for "executing" things because it will include the appropriate authorization checks, but these transport-based calls completely bypass that. I'd even think we'd want to "opt in" to any particular action being enabled for remote execution, so it's more complex than just registering a list.

See discussions here and here and please chime in with any ideas. The issue is discussing creating policy files for extensions to define the actions an extension is permitted to perform. If I'm reading this thread correctly it sounds like before the ProxyAction destined for another extension gets routed to the other extension that it will go through core first. When it gets to core, could that trigger the authz checks to determine if the extension is permitted to perform the action?

@dbwiddis
Copy link
Member Author

dbwiddis commented Mar 9, 2023

Hey @scrawfor99 thanks for chiming in and linking those two issues. We definitely need to integrate with this process.

See discussions here and here and please chime in with any ideas. The issue is discussing creating policy files for extensions to define the actions an extension is permitted to perform. If I'm reading this thread correctly it sounds like before the ProxyAction destined for another extension gets routed to the other extension that it will go through core first. When it gets to core, could that trigger the authz checks to determine if the extension is permitted to perform the action?

(waving hands vaguely) Yes.

More specifically, there are going to be multiple layers of permissions.

  1. There will be a very coarse "is this action even executable" filter. This can be integrated in OpenSearch without even asking the extension, it can just look at the action name and action filters that have been registered and say "Nope!"
  2. For the subset of actions that are forwarded to an extension, there does need to be some additional layer of identity/access checks
    • This is similar to how we expect the REST api to work, where an initial request comes in and all the appropriate authorization tokens are generated and sent to the extension to use with any intermediate rest requests.
    • so when a proxy action is forwarded, it should include any relevant token lookup, probably associated with the proxy action request from the other extension asking for it (any Principal be it another extension or user etc.)

On top of this there should be some tenets for how/when we trigger remote things, like "prioritize using REST API for invoking remote actions elsewhere, if that's possible to do". Proxy actions should probably be the exception rather than the rule.

@minalsha
Copy link
Collaborator

@peternied , @cwperks do share your thoughts from security end.

@peternied
Copy link
Member

The idea that an extension might provide functionality via that aren't exposed as REST APIs seems broken, why shouldn't external callers be able to perform the same kinds of actions as an extension?

From a security perspective, if we have additional ways to trigger actions REST API vs proxy we should still have consistent security enforcement. Sounds like there will be additional security requirements for this enforcement - I think we would need more design detail to figure out how to support a new require, more detailed scenario and data flow diagram. Please @ mention me if you'd like a more in depth review.

@stephen-crawford
Copy link

scrawfor

I think this should tag @cwperks :)

@dbwiddis
Copy link
Member Author

Hey @peternied @scrawfor99 @cwperks and anyone else following along. :)

I need to do something to implement this in order to enable Job Scheduler -> Anomaly Detector actions. I'll probably do that later this week.

My plan is to:

  • use getActionFilters() to allow only a subset of actions to be triggered remotely.
  • add some TODO notes on where I think a permissions check will fit in the future, ideally using the same check as REST requests.

I know (and we all know) this isn't the final solution.

@dbwiddis
Copy link
Member Author

To everyone who has liked, subscribed, and purchased a product from my sponsor, see opensearch-project/OpenSearch#6734 for the OpenSearch side of this implementation and yell if you see any problems.

@dbwiddis
Copy link
Member Author

The draft OpenSearch action is in a mostly finished state. I'll start working on the SDK side (this issue) which should work with either the existing (ProxyAction) or new (Dynamic Actions) arrangement, as the handler transport will be the same for both.

Key point is I like the new dynamic actions from the standpoint of funneling all remote actions through the NodeClient's execute() method, thus enabling (OpenSearch side) ActionFilters including security plugin, which was my main concern with implementing this as-is.

@dbwiddis
Copy link
Member Author

dbwiddis commented Mar 19, 2023

Have this done as far as sending the request to OpenSearch, fixing some bugs along the way. Just need to handle the forwarded request.

Making notes of the sequence here since I've confused myself several times with the 4-way communication, this will eventually find its way into a sequence diagram:

  • Extension sends TransportActionRequestFromExtension
  • ExtensionTransportActionsHandler.handleTransportActionRequestFromExtension handles it
    • invokes ExtensionAction -> ExtensionTransportAction
    • Generates ExtensionActionRequest
      • ExtensionTransportActionsHandler.sendTransportRequestToExtension()
      • Sent to Extension which must handle REQUEST_EXTENSION_HANDLE_TRANSPORT_ACTION
    • Response is ExtensionActionResponse
  • Bytes are transferred to TransportActionResponseToExtension

@dbwiddis
Copy link
Member Author

dbwiddis commented Mar 20, 2023

$ curl -X GET http://localhost:9200/_extensions/_hw/hello/test
Received greeting from remote extension: Hello, test% 

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants