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

authorize tenant level and namespace level access from the authorization provider #5720

Closed
Geal opened this issue Nov 21, 2019 · 8 comments · Fixed by #6428
Closed

authorize tenant level and namespace level access from the authorization provider #5720

Geal opened this issue Nov 21, 2019 · 8 comments · Fixed by #6428
Labels
type/feature The PR added a new feature or issue requested a new feature

Comments

@Geal
Copy link
Contributor

Geal commented Nov 21, 2019

Is your feature request related to a problem? Please describe.
I am currently writing an authentication provider and an authorization provider, and the authorization API only provides the isSuperUser method for admin level usage. There is no way to authorise a client depending on the affected tenant or namespace. In my use case, I want my clients to have their own tenant, and be able to give limited access to some namespaces to their developers.

Describe the solution you'd like
Adding new methods to AuthorizationProvider:

  • for tenant level access (different from superuser access: can't create, delete or update a tenant, but manage every namespace under the tenant): CompletableFuture<Boolean> canManageNamespace(NamespaceName namespaceName, String role, NamespaceOperation operation, AuthenticationDataSource authenticationData)
    • NamespaceOperation is an enum that can be any of the namespace management tasks, like create, delete, grant-permissions, etc. The default implementation could ignore it, but other providers could manage more granular access (like, I can affect subscriptions, but not quotas and affinity). I think some operations should still require superuser access, like set-clustersor unload?
    • the default implementation would check that either we're a super user, or the role is in the list returned by tenant.getAdminRoles()
    • maybe a separate function for list, since it would not take a namespace name as argument?
  • for namespace level access: CompletableFuture<Boolean> canManageTopic(TopicName topic, String role, TopicOperation operation, AuthenticationDataSource authenticationData)
    • same as tenant level access, TopicOperation is an enum listing all the possible operations
    • same as tenant level access, the default provider would ignore the operation argument, and check that either we're a super user, or the role is in the list returned by tenant.getAdminRoles()

Add corresponding methods to PulsarWebResource. Right now it only has validateSuperUserAccess and validateAdminAccessForTenant. We must modify validateAdminAccessForTenant to add the operation, and add validateAdminAccessForTopic
And the biggest part of the task: go through every usage of validateSuperUserAccess and validateAdminAccessForTenant and check if they can be replaced with finer grain access

Describe alternatives you've considered
I do not know any alternative way to get more granular admin access authorization

@Geal Geal added the type/feature The PR added a new feature or issue requested a new feature label Nov 21, 2019
@jiazhai
Copy link
Member

jiazhai commented Nov 26, 2019

Hi Geal, there was a discussion of role and authorize in pulsar dev mail list. And also a PIP is here: https://github.com/apache/pulsar/wiki/PIP-49%3A-Permission-levels-and-inheritance

Would you please check if these could help your case.

@Geal
Copy link
Contributor Author

Geal commented Nov 29, 2019

I agree with the thread, the current access levels are problematic for multi tenant systems. The underlying point I'd like to add is that there should be check for individual operations or resources, not just for super user, tenant admin (we already have the canProduceAsync function and others that work like that).

@tuteng
Copy link
Member

tuteng commented Feb 4, 2020

It seems too heavy to implement such fine-grained permission management in pulsar. I think it can be implemented in an external system, such as pulsar-manager https://github.com/apache/pulsar-manager. This external service can be used to proxy all HTTP requests and complete all permission management. I have considered the above fine-grained authorization management before. This is the PIP of pulsar-manager's permission management https://github.com/apache/pulsar-manager/wiki/Authentication-and-Authorization-in-Pulsar-Manager. I think you can consider implementing this feature based on this. @Geal

@Geal
Copy link
Contributor Author

Geal commented Feb 4, 2020

How heavy would it be? From what I see, that would mean:

  • adding the various methods to AuthorizationProvider, with the basic implementation calling isSuperUser under the hood
  • going through the admin API and adapt the levels. I see 12 calls for validateAdminAccessForTenant and 17 calls for validateSuperUserAccess (outside of tests). It's largely doable

I could probably get the same feature throught the manager, but it is very surprising to me that a multitenant system could not expose basic management features to tenants without admin access

@sijie
Copy link
Member

sijie commented Feb 4, 2020

@jiazhai @tuteng

I think the issue is asking for interfaces to be added to AuthorizationProvider. The default implementation can remain the same. The interfaces allow external parties to customize their own authorization implementation.

The authorization provider can be enhanced into an extensible interface. What an authorization provider provides is if a role is able to apply a verb/action to a given resource.

The resources are:

  • tenant
  • namespace
  • namespace_policy
  • topic
  • subscription
  • functions
  • connectors

For each resource, there are certain verbs and actions available for operating those resources. The authorization provider provides an implementation to check if a role is allowed to apply a certain verb over a resource.

If we can abstract the authorization provider, it allows people to customize its own authorization provider implementation to allow finer granularity access controls.

For the default implementation, Pulsar has, we can keep it as is due to the concerns raised around PIP-49.

Does that make sense?

@jiazhai
Copy link
Member

jiazhai commented Feb 28, 2020

Thanks for @KannarFr 's help on this issue.

@zymap
Copy link
Member

zymap commented Mar 18, 2020

@KannarFr Thanks for your help.

Currently, @KannarFr had added the basic interface of the authorization provider. We need to make sure all the places that called for validating the auth support the new way to validate.

Here a list about that:

  • ServerCnx
  • v2 / Brokers
  • v2 / BrokerStats
  • v2 / Clusters
  • v2 / Functions
  • v2 / Namespaces
  • v2 / NonPersistentTopics
  • v2 / PersistentTopics
  • v2 / ResourceQuotas
  • v2 / SchemaResource
  • v2 / Tenants
  • v2 / Worker
  • v2 / WorkerStats
  • SourcesBase
  • SinksBase

KannarFr has done a part of those at #6428 .

@KannarFr
Copy link
Contributor

KannarFr commented Apr 3, 2020

Hi @zymap,

The way to add these features is done on #6428. It comes with its usage on Namespaces & ServerCnx.

sijie pushed a commit that referenced this issue May 18, 2020
Fixes #5720

### Motivation

Provide "real" authz abilities to pulsar resources.

### Modifications

Add stuff to `AuthorizationProvider` interface, and use them on every pulsar resource management auth (tenant, namespace, topics, functions, connectors, ...)
Huanli-Meng pushed a commit to Huanli-Meng/pulsar that referenced this issue May 27, 2020
Fixes apache#5720

### Motivation

Provide "real" authz abilities to pulsar resources.

### Modifications

Add stuff to `AuthorizationProvider` interface, and use them on every pulsar resource management auth (tenant, namespace, topics, functions, connectors, ...)
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this issue Aug 24, 2020
Fixes apache#5720

### Motivation

Provide "real" authz abilities to pulsar resources.

### Modifications

Add stuff to `AuthorizationProvider` interface, and use them on every pulsar resource management auth (tenant, namespace, topics, functions, connectors, ...)
sijie pushed a commit that referenced this issue May 8, 2021
Fixes a part of #5720 

### Motivation

add granularity in topics api authz
eolivelli pushed a commit to eolivelli/pulsar that referenced this issue May 11, 2021
Fixes a part of apache#5720 

### Motivation

add granularity in topics api authz
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants