[Doc] Add workspace access control design proposal

[zengyan-amazon]

Description

Add workspace access control design proposal

Issues Resolved

#4615

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
  • yarn test:ftr
• New functionality includes testing.
• New functionality has been documented.
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

[peternied]

Thanks for writing this up. I might have some misunderstandings about the system evolved, could you add some more flow diagrams / sequence diagrams to clarify how parts of this system interact?

[peternied]

👍 Thanks for making this a pull request, much easier to put inline feedback on the design and for follow up.

[peternied]

These personas are defined by their level of access to the system, not by their goals. I'd recommend abstracting what/how of permissions in favor of what the personas are trying to accomplish within a workspace.

[peternied]

I've pondered my feedback and here's why focusing on user goals matters. If we rely heavily on pre-existing permission structures, our solution's scope might be unintentionally limited.

Starting from scratch for workspaces lets us independently define requirements, leading to a flexible and efficient system tailored to users' needs. We should strive to understand user needs and goals, and not be overly bound by existing permission systems.

[ruanyl]

I'm not sure if I fully get what you recommended, I'll try to reply based on my understanding.

If we rely heavily on pre-existing permission structures, our solution's scope might be unintentionally limited.

If the pre-existing permission structures you're referring is the personas defined here, I think our implementation won't rely on that.

For a workspace, we abstracted 4 types of permission:

1. write, user can write(include delete) to the workspace object as well as the ACL defined on the the object
2. read, user can read the workspace object
3. library_write, user can create/update objects, such as dashboards, visualizations associated with the workspace
4. library_write, can can read objects, such as dashboards, visualizations associated with the workspace

A workspace admin is write + library_write
A workspace operator is read + library_write
A workspace viewer is read + library_read

Does this address your concern?

[peternied]

Down a couple of paragraphs you mention not using RBAC, maybe this belongs elsewhere or incorporated in another way?

Who a super-admin is seems like something that shouldn't be statically defined as such I don't think a config file that requires a restart of the service to pick up is a good place to manage this informatino. Maybe breakout another doc/section for how to address items like this.

[ruanyl]

This part needs to update because super-admin can not only be configured statically in opensearch_dashboards.yml, but also can be configured via dynamic application configuration: #5855

[peternied]

Add a diagram to help illustrate the relationship, here is a diagram along with the code to add it inline. BTW If I got this relationship wrong please update :D

BTW I asked ChatGPT to generate an outline for this diagram if you want to streamline their creation

graph LR
    subgraph Dashboard
        DashboardObject["Dashboard Object"]
    end

    subgraph ACL1
        ACL11["Access Control List Entry"]
        ACL12["Access Control List Entry"]
    end

    subgraph ACL2
        ACL21["Access Control List Entry"]
    end

    DashboardObject --> |Associated with| ACL1
    DashboardObject --> |Associated with| ACL2

    style DashboardObject fill:#FFE4B5,stroke:#333,stroke-width:2px
    style ACL1 fill:#ADD8E6,stroke:#333,stroke-width:2px
    style ACL2 fill:#ADD8E6,stroke:#333,stroke-width:2px

Loading

Suggested change

	```mermaid
	graph LR
	subgraph Dashboard
	DashboardObject["Dashboard Object"]
	end
	subgraph ACL1
	ACL11["Access Control List Entry"]
	ACL12["Access Control List Entry"]
	end
	subgraph ACL2
	ACL21["Access Control List Entry"]
	end
	DashboardObject --> |Associated with| ACL1
	DashboardObject --> |Associated with| ACL2
	style DashboardObject fill:#FFE4B5,stroke:#333,stroke-width:2px
	style ACL1 fill:#ADD8E6,stroke:#333,stroke-width:2px
	style ACL2 fill:#ADD8E6,stroke:#333,stroke-width:2px

[davidlago]

+1, but perhaps an even higher level one that has both workspaces and objects and how the overlapping permissions work

[ruanyl]

one that has both workspaces and objects and how the overlapping permissions work

@davidlago The current implementation is it should either pass the permission check of workspace or the object's ACL validation

[ruanyl]

@peternied We store the ACL data as part of the object(as a property), not with a separate object.

[peternied]

I don't think 'roles' are a fundamental problem, I think the problem is calculating what permissions a user has, with the current system there are no APIs do to this making the only avenue to attempt to perform an action and check for 403 (bad user experience).

Might want to leave the door open in the design to allow permissions to be source from other logical groups such as roles that might be useful in the future for larger numbers of workspace users.

IMO I think this problem is mitigated if you could see a projection of an individual's users permissions, e.g. potential way to see permissions where you can get a full list, and that list has references to the source of the permission:

GET /user/{USER_ID}/_permissions
{
  "user": "{USER_ID}",
  "permissions": [
    {
      "name": "WORKSPACE.VIEW",
      "scope": "*",
      "from": {
        "source": "role",
        "roleName": "AllUsers",
        "ref": "/roles/AllUsers"
      }
    },
    {
      "name": "WORKSPACE.EDIT",
      "scope": "index",
      "from": {
        "source": "user",
        "ref": "/user/{USER_ID}"
      }
    }
  ]
}

[peternied]

When you say inside, that seems more like a logical reference to 'associated with'. I'm concerned about inheritance when there could be multiple workspace with conflicting rules. As there are difference kinds of access read vs readwrite, if so this could make associating a saved object with multiple works spaces cause trouble - which permission do you get the lowest vs highest?

This inheritance creates side effects where modifying a workspace permission can change the permissions on objects inside other workspaces

[davidlago]

This phrase is missing a key verb:

One saved object can potentially in multiple workspaces

can be in? can be referenced in?

I echo @peternied's sentiments about the undesired side effects. Will saved objects be addressable outside of a workspace, or will they always be nested inside of a workspace? i.e. if object1 is associated with workspace1 and workspace2, can I link to object1 directly, or do I need to qualify with workspace1/object1? If so, could we skip the "it inherits all permissions across workspaces" so that it only gets those in the context it is being referenced?

[peternied]

Is there a system that will translate this into a principal/set of principals? Or is this assumed to be a principal part of the AuthN system?

[ruanyl]

As state at the beginning:

We can assume the users are authenticated and user profile (user name and group info) has been injected into each request

It expects AuthN system to inject these information

[peternied]

While named differently, these ACLs look like roles, how are they different?

[ruanyl]

As both of them are mechanisms used in access control systems, they do share similarities, but they operate at different level.

ACLs focus on defining permissions at the object level for individual resources, roles abstract away permissions into named collections that can be assigned to users or groups based on their roles.

Does this answer your question?

[peternied]

Requiring the intervention of a super-admin for orphaned objects seems like its going to cause maintenance/operation issues, can we alter the design to handle/avoid these scenarios?

[ruanyl]

Objects not associate with workspace and do not have ACL defined, such objects can be considered "public", I think such objects won't require permission check. These saved objects behave like they are nowadays.

[peternied]

If a user can 'write' to an object, it seems like they can alter its ACLs, this seems like a way to circumvent the permissions? Seems like the ACL associate needs to be protected from this level of access - otherwise anyone with a write allowed connection string is effectively a workspace super admin

[ruanyl]

anyone with a write allowed connection string is effectively a workspace super admin

Yes, this is the current design. We only have read and write, and write permission is currently the highest privilege, with write permission, a user can delete the object.

I understand the permission model can go deeper and more complex, but we started with a simple one which serves our current use-cases and the upcoming use-cases which is under design. We try to avoid over design, you could see this as a trade-off(which is a practical choice I think ;))

[peternied]

After sleeping on this design, a couple more ideas came to mind.

[peternied]

It would be helpful to weight the recommended design against other approach(es), I see that this has been accounted for inline for key features and if there was a wholistic case made for another approach it would help illustrate the tradeoffs and benefits of your recommendation.

[ruanyl]

It's clear that saved object inherit the permissions from the workspace, but if I attach ACL to a dashboard object, should visualizations and index patterns inherit dashboard object's ACL? Or the same ACL should attached to all visualization and index pattern objects specifically?

[peternied]

While authentication is not in scope for this design - but there are plans to decouple authentication, lets add requirements on the current AuthN system expectations.

We've found internal features in the security plugin were actually being used as if they were part of a public interface, since this design depends on the security plugin accounting for these areas and making hard requirements will ensure no breaking changes and make migrating to the end-state Trinity authentication system more preditable.

[ruanyl]

We've found internal features in the security plugin were actually being used as if they were part of a public interface

Could you share the example? Just want to make sure our implementation doesn't rely on security plugin internals, though I believe we're only using publicly accessible APIs.

[ruanyl]

lets add requirements on the current AuthN system expectations.

What's only required is just as it describes: user profile (user name and group info) has been injected into each request.

[peternied]

Let's be thoughtful about communicating this features status in the security review process [1] since this feature security focused. There is a balance when communicating details, but as contributions to this feature will have impact on the security posture and the review process we should be prepared to handle those.

• [1] https://github.com/opensearch-project/.github/blob/main/RELEASING.md#security-reviews

[peternied]

Speaking of security reviews, I recommend starting the internal security review process early as this features complexity and risk potential is likely to require more lead time and time consuming activities.

[kavilla]

comment here related to scaling: #5084 (review)

[peternied]

@zengyan-amazon, I see that a pull request [1] has been initiated to implement parts of this design. I noticed there was a push for a review from the security plugin team, and there is substantial feedback on this document yet to be addressed, which could significantly impact the design.

I understand this is your feature to build, and I genuinely appreciate the opportunity to provide feedback and guidance. However, it’s important to note that when feedback isn’t incorporated or addressed, it may deter me and my peers from engaging in future reviews. It’s crucial for the success of our collaborative efforts that we see tangible outcomes from the review process.

Looking forward to seeing how this develops and hoping we can address the outstanding feedback for a robust design.

• [1] [Workspace][Feature] Add ACL related functions #5084

[ruanyl]

@zengyan-amazon It seems management is not necessary in practice, it's basically write + library_write, the library_write give the user full access to all the saved objects associated with the workspace. The write permission will give a user full access to a workspace object includes the ACLs on the object. To my understanding, this equals to management, am I right?

[kavilla]

@zengyan-amazon @gaobinlong @ruanyl @SuZhou-Joe

Do we know the status of this? Can I assign someone responsible for responding to @peternied and for getting this merged or closed?

[ananzh]

Convert to draft due to no progress.