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

Introduce Application Scopes for Extension Management #7850

Closed

Conversation

stephen-crawford
Copy link
Contributor

@stephen-crawford stephen-crawford commented May 31, 2023

Description

This PR introduces a basic implementation of Scopes for extensions. Scopes are a form of basic control for extensions which can be used by administrators to control the basic operation permissions an extensions has. For example, an extension with the scope ActionScope.READ would have the capabilities to read an index.

What differentiates scopes from other permission types is the level at which they apply. While an extension may have permissions associated with its service account or with the user who it is making a request on the behalf-of, scopes are the root functionality permissions of an extension.

A good analogy for scopes can be core abilities. For example, as a parent you may not let your child read a Stephen King novel but they may fundamentally be able to read it. In this example, your choice to prevent the child from reading the novel is a granular permission which would be enforced on the service account or users associated with an on-behalf-of token. The child's literacy or lack thereof, is similar to a scope in that they are capable of reading the book.

Related Issues

#8873
Resolves opensearch-project/security#2587

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This is pretty straightforward.

How will scope add up with a type of permission? I imagine we'll have "Index:read" and "Search:execute", the second part being fairly constrained (e.g. index has read/write, but search has only execute).

@stephen-crawford
Copy link
Contributor Author

Hi @dblock, thank you for following up.

I am taking over this effort for Peter since he is busy with some other things at the moment. The implementation of scopes is pretty straightforward like you mentioned. I know that part of the inspiration for these changes were the Slack Scopes and GitHub scopes which are similarly basic. The general purpose is for administrators to be able to control what extensions are able to do on a base level. Scopes then provide some safeguards against unforeseen interactions between extensions and a cluster.

As far as lining up with fine-grained access controls, I believe it will be a process of funneling the different access controls down to the associated scope. Generally speaking, there should be more fine-grained access control permissions then scopes, but I doubt there is a programmatic way to translate between the two. I will follow-up if I find anything that looks promising for converting between the two, but I suspect it will need to be done manually for the time being, with future extensions perhaps listing the scopes required for the extension to run inside their extension.yml files.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Gradle Check (Jenkins) Run Completed with:

@stephen-crawford
Copy link
Contributor Author

stephen-crawford commented Jun 2, 2023

I worked on this some more and have finished setting up most of what I imagine Peter had in mind. I did not speak to him in detail about Scopes so I am using a bit of creative liberty here, but the current state of things is:

There are two types of Scopes introduced as part of the IdentityPlugin: ActionScopes & ExtensionPointScopes

  1. ActionScopes are scopes which translate to broad action 'themes.' The best way to think about these may be the end use case and this is how they are named. The created ActionScopes are: Cluster_Read(), Cluster_ALL(), Index_Read(), Index_ReadWrite(), Index_Search(), Index_ALL().

Cluster_Read is associated with all Cluster-level actions which correspond to something like Read, Get, List, etc.
Cluster_All is associated with all Cluster-level actions
Index_Read is associated with all Index-level actions which are similar to Reading
Index_ReadWrite corresponds to all the actions that Index_Read does as well as actions that allow you to modify or write to an Index
Index_All is associated with all index-level actions

  1. ExtensionPointScopes are scopes which control the ability to extend the corresponding extension point. There is one for each of the extension points.

Right now there is not much set up around the ExtensionPointScopes but Peter had created a ScopeProtectedActionPlugin which validates that the subject has the appropriate scope to be able to access the RestHandlers when returning the handlers.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2023

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on

@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

I'm really confused about why I have to include all the "or broader scope" scopes in my allowed list. I should be able to specify a minimum required scope, e.g., if I need "read" then something authorized for "all" should pass.

@stephen-crawford
Copy link
Contributor Author

I'm really confused about why I have to include all the "or broader scope" scopes in my allowed list. I should be able to specify a minimum required scope, e.g., if I need "read" then something authorized for "all" should pass.

Hi @dbwiddis, thanks for leaving feed back here. This is still a work in progress and I have not been able to work on it this past week as I am the core oncall.

I will take the time to review your comments now however.

With regard to this initial comment, I take it you are referring to an action needing to specify the entire list of applicable scopes? Perhaps this is not the deal way of going about things, though I think factors make the hierarchical strategy you mentioned problematic.

First, as a security principal, I think we generally want to avoid non explicit privilege granting. By using a hierarchical strategy, we simplify the listing process for action developers and save a few minutes. However, we introduce a lot of uncertainty in what a specific scope does or does not allow. While some hierarchies are pretty obvious such as READ < ALL -- as soon as you move beyond READ, WRITE, ALL, things get more complicated. For instance, if we have a SEARCH or MONITOR scope, do those include READ and WRITE? I would assume READ but if you have a monitoring application it probably wants to write to an index as well. So now you have READ < SEARCH < ALL and a different branch of READ < MONITOR < ALL as well as the WRITE counterparts. I don't think it would be realistic for upholding an intuitive permission granting model under this structure. On both a developer's side and more importantly a user's, I think a hierarchical model would quickly become difficult to manage.

A hierarchical model also will only work with the ActionScopes and not the ExtensionPointScopes. The ExtensionPointScopes are designed to restrict use of the interfaces defined in core. There is not any way to layer these so we would need to have two different mental models for scopes which would probably make use confusing.

Let me know if you think this makes sense or if I am totally off. This is certainly not final but I hope it explains some of the design process behind the structure.

@dbwiddis
Copy link
Member

While some hierarchies are pretty obvious such as READ < ALL -- as soon as you move beyond READ, WRITE, ALL, things get more complicated.

Fair point, I think it may be reasonable to have special treatment for ALL, but I still think there's room for some hierarchical something somewhere.

For example, it's odd that we might have Index_Read and Index_ALL but not have Index_ReadWrite. I'm OK with requiring an explicit list but is there a way we can sanity check these to point out that this might not be what was intended?

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@peternied
Copy link
Member

I'm really confused about why I have to include all the "or broader scope" scopes in my allowed list. I should be able to specify a minimum required scope, e.g., if I need "read" then something authorized for "all" should pass.

@dbwiddis I believe your concerns have been addressed, as you are on vacation (enjoy!) I am going to dismiss your review. If this is a surprise when you come back, please open issue on any items you'd like to see follow up on or submit pull requests.

@peternied peternied dismissed dbwiddis’s stale review July 17, 2023 21:18

dbwiddis is unavailable for follow up on this review

Signed-off-by: Peter Nied <petern@amazon.com>
@stephen-crawford
Copy link
Contributor Author

Thank you @peternied for taking this over for me while I am away this week!

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.multipart.RemoteStoreMultipartIT.testStaleCommitDeletionWithInvokeFlush
      1 org.opensearch.remotestore.RemoteStoreIT.testStaleCommitDeletionWithInvokeFlush

Signed-off-by: Peter Nied <petern@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Peter Nied <petern@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Peter Nied <petern@amazon.com>
@peternied
Copy link
Member

I've added several integration tests that should be implemented before this is merged to ensure the scopes are operating as expected, I'll try to make sure the test declaration makes it easer to follow what users/operators should be able to do / expect.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@stephen-crawford
Copy link
Contributor Author

@peternied @reta thank you both for your consistent engagement.

I am just wondering what is required to close on this? I see Peter made changes that removed the unit tests in favor of integration tests. However, I am not sure what is expected for those and am going to revert that change if there is not a clear path forward there. If I can get some guidelines on what is expected, I can meet those expectations but without further information I really don't know what is required. I am going to revert the changes on 7/27 if I don't hear back.

I would like to drive this to a close efficiently as this has become a very large change. I would appreciate more feedback from other maintainers as well as there has been very minimal interaction which makes it hard to know what is expected.

@reta
Copy link
Collaborator

reta commented Jul 25, 2023

@peternied @reta thank you both for your consistent engagement.

Thanks @scrawfor99 , I feel that I am lacking the big picture with respect to this change. The scopes are looking good to me, but specifically the subjects part I didn't get (sorry about that, I will try to go over the change once more).

Signed-off-by: Peter Nied <petern@amazon.com>
@peternied peternied requested a review from sohami as a code owner July 26, 2023 22:39
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@peternied
Copy link
Member

Got waylaid last week, I've created a structure around the integration tests, I think that will clarify the end user perspective, still looking into a clean way to do that - to me that is the biggest hurdle to move forward.

I'm going to pull out the thread context related changes, I was creeping in features, I think we should come back separately to handle that scenario with out holding up the whole feature, I'll slice out most of the code around the extension points so we can add it all in at once when it works end to end

@peternied
Copy link
Member

BTW @scrawfor99 I consider forward motion on this PR waiting on me, not you - but thanks for cracking the whip to get motion here.

import static org.hamcrest.Matchers.equalTo;


public class ScopeAccessIT extends OpenSearchIntegTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

Reached out to a couple members of the project and here was their recommendation on how to handle mocking/replacing the ApplicationManager

There are ways to inject custom behavior in tests. Random example:

@Override
protected Collection<Class<? extends Plugin>> nodePlugins() {
return Arrays.asList(
MockTransportService.TestPlugin.class,
MockIndexEventListener.TestPlugin.class,
MockFSIndexStore.TestPlugin.class,
InternalSettingsPlugin.class
);
}

There are lots of examples of injecting custom stuff for integ tests, but fundamentally the injection point has to be pluggable in the production code I think (edited)

Thanks @mch2 & @andrross for pointing me in this direction, I'll iterate on this and see if I can reuse this pattern

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity. Remove stalled label or comment or this will be closed in 7 days.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Aug 28, 2023
@stephen-crawford
Copy link
Contributor Author

stephen-crawford commented Aug 28, 2023

Still pending work per @peternied's comment above

@peternied
Copy link
Member

@scrawfor99 I think we've stalled out and there is an open question if now is the right time to build and deliver this feature. I think this is good work, but we might not be ready to invest in getting it merged as is, what do you think about closing this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stalled Issues that have stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Question] How do administrators control what extensions can do?
7 participants