Allow subscribers to access subscription admin-api #2981

rdhabalia · 2018-11-13T20:32:14Z

Motivation

Addressing #2964

Result

Subscribers will be able to access subscribe admin-api.

merlimat · 2018-11-13T20:55:10Z

@rdhabalia Could this solved with the per-subscription auth mechanism which is already available?

rdhabalia · 2018-11-13T21:33:07Z

per-subscription auth mechanism which is already available?

@merlimat I didn't see a placeholder which keeps map of authorized roles for a given subscription so, broker can authorize admin-api request. also, I am not exactly aware about per-sub auth, can you please point me the PR or change.

merlimat · 2018-11-13T22:10:02Z

@rdhabalia #899

rdhabalia · 2018-11-13T22:21:29Z

Oh.. no, that will not help because in our useacase, recently many of the existing subscribers are reaching out to system-admin to execute subscription admin-api (eg: skip-all, skip, reset-cursor) due to multiple reasons (eg: backlog quota exceeded, reseting cursor with retention enable, etc..). So, in that case it's hard to handle multiple requests from subscribers so, it's better to let subscriber handle those admin-api.

#899 restricts subscription-name creation with the role-name prefix but it will not address authorization of existing subscription name which doesn't have role-name prefix.

merlimat · 2018-11-13T22:26:40Z

#899 restricts subscription-name creation with the role-name prefix but it will not address authorization of existing subscription name which doesn't have role-name prefix.

My only concern is to end up with 2 ways to do (almost) the same thing, which might be confusing to users.

merlimat · 2018-11-13T22:27:59Z

Isn't it possible to give permissions to these subscriptions individually (with the existing solution and without changing subscription names) ?

rdhabalia · 2018-11-13T22:36:13Z

Isn't it possible to give permissions to these subscriptions individually

Umm.. #899 doesn't store authorized subscriber role-names. it authorizes request based on the sub-name (eg. myRoleName-mysub). so, if sub-auth mode is enabled then sub-name has to start with myRoleName so, user with myRoleName role is authorized for mysub.
But existing subscription mysub can't be accessed by myRoleName because it doesn't start with role-name.
So, I am not sure how can we use this solution in a straight forward way to address above issue. Can you please let me know if I am missing anything here.

rdhabalia · 2018-11-14T01:39:33Z

@merlimat I think #899 can be only useful to avoid conflicting name because for authorization it enforces role name prefix and it has to be part of sub-name and that is something not feasible.
So, for user, sub-auth-mode feature can be used to avoid conflicting name and sub-auth policies required for actual authorization. ??

merlimat · 2018-11-14T05:23:14Z

@rdhabalia I see. Then this change would be a superset of #899 (in the sense that it would allow to achieve that goal as well), am I right?

Would it then make sense to deprecate the sub prefix mode feature and just rely on "per-subscription" authorization?

cc/ @nkurihar

rdhabalia · 2018-11-14T07:23:22Z

Would it then make sense to deprecate the sub prefix mode feature and just rely on "per-subscription" authorization?

Yes, next I wanted to integrate this policy with consumer authorization so, at that time we can deprecate sub-prefix-mode and utilize per-sub authorization mechanism. I will try to address it in next PR.

merlimat · 2018-11-14T18:15:21Z

...roker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java

+     *            the subscription name defined by the client
+     * @return
+     */
+    CompletableFuture<Set<String>> getAuthorizedRolesOnSubscription(TopicName topicName,


Why getting a list of roles here instead of behaving like the rest of the methods?

I actually think that this should be handled by canConsumeAsync(). Since we might not be able to fetch that info from the Authorization plugin implementation.

Actually, broker stores authorization metadata by calling AuthorizationServiceProvider::grantPermission(). and right now, broker is having default ZK-AuthorizationProvider which stores metadata into same global-ZK under policies so, if we want to retrieve then we can see it in policies.

However, if one implements different provider which stores this metadata somewhere else then broker will not have the way to get authorization metadata and user will not be able to see stored authorization-roles.

So, I added API by considering that we can utilize this API later to fetch auth metadata from Authorization provider. As this interface can be implemented by user, ideally we should not try to break so, I tried to keep it generic enough (though I am breaking the interface by adding new API but it seems it might be fine right now).

Does it make sense? or we still want to change api to isAuthorizedOnSubscription(..) instead getAuthorizedRolesOnSubscription(..)?

I would say to not add a new method, but rather handle that internally in the canConsume(). That method is already taking the subscription name (along with the client credentials) and should be able to make the assessment.

There might be cases in which the permission might be already encrypted into the credentials, or that the check needs to be performed against external system.

If authorization is stored in default zk-store, then doing pulsar-admin namespaces policies will already print the authorized roles.

I would say to not add a new method, but rather handle that internally in the canConsume().

hmm.. yes, we can do that. let me fix it.

If authorization is stored in default zk-store, then doing pulsar-admin namespaces policies will already print the authorized roles

But what if authorization is not using default zk-store. in that case, we may have to add get() method in auth-provider. but that we can do later as well.

I would say to not add a new method, but rather handle that internally in the canConsume().

@merlimat this will not work because we want to explicitly give access to certain role that can use subscription-based admin-api (eg: reset-cursor, skip) for a given subscription. If the role is not configured/present into authorization list then admin-api request should fail.

whereas canConsume() can not fail if subscription-authorization list is not configured. If we enforce subscription-authorization in canConsume method then all existing consumer will start failing because we don't set this permission explicitly.

I can make change in canConsume() where if sub-authorization is configured then validate role else ignore the check. but we can't use canConsume() for admin-api validation (once we have this check then we deprecate sub-prefix-auth).

I don't see admin api permissions as different from "consume" permissions.

If a use has consume access to some topic (on all the subscriptions) than it can "mess up" some other consumer by attaching to the wrong subscription and stealing someone else's messages.

It's exactly the same as opening the admin API to all the users who have consume access on the topic.

To solve that, you can :

Grant per-subscription authorization to each user on its subscription

Remove the per-namespace consume authorization

If a use has consume access to some topic (on all the subscriptions) than it can "mess up" some other consumer by attaching to the wrong subscription and stealing someone else's messages.

which is already incorrect. right? we should have a way to prevent this and with this PR it can be possible. But by default we can't enforce Per-Sub for consumers as it can create issue for existing topics. But may be pulsar can have a flag to enforce it in the system.

I don't see admin api permissions as different from "consume" permissions.

Actually then we don't need this PR at all. No need of introducing "Per-Subscription" authorization if "Namespace-auth(consume)" is anyway present so, "Namespace-auth" will always win and all subscribers can access each other's cursor state.
So, I thought admin-api right now doesn't have any authorization present, we can make it right atleast for admin-api by enforcing "Per-Subscription" validation.

But Let me know if you still think admin-api should not enforce "Per-subscription" then I will make your suggested change and all api will use one canConsume()-api.

which is already incorrect. right? we should have a way to prevent this and with this PR it can be possible.

I think having per-subscription ACLs should be enough, right?

But by default we can't enforce Per-Sub for consumers as it can create issue for existing topics. But may be pulsar can have a flag to enforce it in the system.

Even for existing topic, we can just add the per-subscription ACLs, no?

Actually then we don't need this PR at all. No need of introducing "Per-Subscription" authorization if "Namespace-auth(consume)" is anyway present so, "Namespace-auth" will always win and all subscribers can access each other's cursor state.

But you just need to grant one permission, either at the namespace level or at the subscription level if you want to have a user to be restricted.

I think this change is good. Otherwise having 2 different ways to enforce authorization for consume vs admin makes things very confusing.

Let's stick with 3 levels of authorization (namespace, topic, subscription). Whoever has "consume" access on any of these, will be able to consume and perform admin related operations on that topic/subscription.

Whoever has "consume" access on any of these, will be able to consume and perform admin related operations on that topic/subscription.

Alright. I will make the change.

@merlimat made the change.

nkurihar · 2018-11-15T02:39:01Z

Yes, next I wanted to integrate this policy with consumer authorization so, at that time we can deprecate sub-prefix-mode and utilize per-sub authorization mechanism. I will try to address it in next PR.

I almost agree with deprecating the sub prefix mode and introducing per-subscription auth mode.

Let me confirm:

Whoever has "consume" access on any of these, will be able to consume and perform admin related operations on that topic/subscription.

If we grant * on consume role, can any consumer subscribe regardless of per-subscription role?

rdhabalia · 2018-11-15T03:27:25Z

If we grant * on consume role, can any consumer subscribe regardless of per-subscription role?

right now, it's not supporting wild-card char such as * but

consumer can subscribe to any subscription if consumer's principal/role is authorized at namespace-level (consume) or per-topic (consume).
however, if per-subscription auth is configured for a specific subscription-name then consumer's principal/role has to be authorized role as per per-subscription to connect to that specific subscription. so, in pre-fix sub usecase, user can grant access to set of roles for a specific subscription and only those roles can access the subscription. so, we can avoid sub-name conflict.

nkurihar · 2018-11-15T07:34:58Z

however, if per-subscription auth is configured for a specific subscription-name then consumer's principal/role has to be authorized role as per per-subscription to connect to that specific subscription.

OK, thanks.

massakam · 2018-11-16T06:52:32Z

...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java

@@ -109,6 +112,16 @@ public void initialize(ServiceConfiguration conf, ConfigurationCacheService conf
                        log.debug("Policies node couldn't be found for topic : {}", topicName);
                    }
                } else {
+                    // check if role is authorize to access subscription. (skip validatation if authorization list is empty)     
+                    Set<String> roles = policies.get().auth_policies.subscription_auth_roles.get(subscription);
+                    if (roles != null && !roles.isEmpty() && !roles.contains(role)) {


Even if the role is superuser, access will be denied unless it is included in subscription_auth_roles, right?

Even if the role is superuser, access will be denied

No, checkout PersistentTopicsBase:: validateAdminAccessForSubscriber(), broker allows super-user/tenant to access any topic api and only validate on subscription if user is not super-user/tenant.

Since ServerCnx etc calls AuthorizationService#canConsumeAsync() directly, superuser seems to fail to subscribe.

$ grep '^superUserRoles=' conf/standalone.conf superUserRoles=superuser $ ./bin/pulsar-admin namespaces policies auth/standalone/test { "auth_policies" : { "namespace_auth" : { "ns-user" : [ "produce", "consume" ] }, "destination_auth" : { }, "subscription_auth_roles" : { "sub1" : [ "massakam" ] } }, ... } $ ./bin/pulsar-client consume -s sub1 -n 10 persistent://auth/standalone/test/t1

17:35:48.140 [pulsar-io-49-3] INFO org.apache.pulsar.broker.service.ServerCnx - New connection from /127.0.0.1:43666 17:35:48.162 [pulsar-io-49-3] INFO org.apache.pulsar.broker.service.ServerCnx - [/127.0.0.1:43666] Client successfully authenticated with basic role superuser and originalPrincipal null 17:35:48.182 [pulsar-io-49-3] WARN org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider - [superuser] is not authorized to subscribe on persistent://auth/standalone/test/t1-sub1 17:35:48.184 [pulsar-io-49-3] WARN org.apache.pulsar.broker.service.ServerCnx - [/127.0.0.1:43666] Client is not authorized to subscribe with role superuser

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

Lines 542 to 548 in 0da4e4a

if (service.isAuthorizationEnabled()) {

authorizationFuture = service.getAuthorizationService().canConsumeAsync(topicName,

originalPrincipal != null ? originalPrincipal : authRole, authenticationData,

subscription);

} else {

authorizationFuture = CompletableFuture.completedFuture(true);

}

@massakam good catch. fixed it.

@rdhabalia Now the super user can subscribe, but consumers which are allowed access at the level of namespace, topic, or subscription fail to subscribe yet.

@massakam yes, that's correct. that's what we want here right?

first broker validates subscription-permission (if it's configured) over namespace-auth so, only authorized principal can access the subscription. and that will also cover "pre-fix" sub usecase where only authorized sub can consume on specific subscription.

@merlimat can you also review this PR one more time.

massakam · 2018-11-16T09:09:34Z

...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java

+                    if (roles != null && !roles.isEmpty() && !roles.contains(role)) {
+                        log.warn("[{}] is not authorized to subscribe on {}-{}", role, topicName, subscription);
+                        PulsarServerException ex = new PulsarServerException(
+                                String.format("%s is not authorized to access subscription %s on topic", role,


One more %s is required.

merlimat

👍

…6122) ### Motivation In #2981, we have added support to grant subscriber-permission to manage subscription based apis. However, grant-subscription-permission api requires super-user access and it creates too much dependency on system-admin when many tenants want to grant subscription permission. So, allow each tenant to manage subscription permission in order to reduce administrative efforts for super user.

…pache#6122) ### Motivation In apache#2981, we have added support to grant subscriber-permission to manage subscription based apis. However, grant-subscription-permission api requires super-user access and it creates too much dependency on system-admin when many tenants want to grant subscription permission. So, allow each tenant to manage subscription permission in order to reduce administrative efforts for super user. (cherry picked from commit 254e54b)

…6122) ### Motivation In #2981, we have added support to grant subscriber-permission to manage subscription based apis. However, grant-subscription-permission api requires super-user access and it creates too much dependency on system-admin when many tenants want to grant subscription permission. So, allow each tenant to manage subscription permission in order to reduce administrative efforts for super user. (cherry picked from commit 254e54b)

…pache#6122) ### Motivation In apache#2981, we have added support to grant subscriber-permission to manage subscription based apis. However, grant-subscription-permission api requires super-user access and it creates too much dependency on system-admin when many tenants want to grant subscription permission. So, allow each tenant to manage subscription permission in order to reduce administrative efforts for super user. (cherry picked from commit 254e54b)

…pache#6122) ### Motivation In apache#2981, we have added support to grant subscriber-permission to manage subscription based apis. However, grant-subscription-permission api requires super-user access and it creates too much dependency on system-admin when many tenants want to grant subscription permission. So, allow each tenant to manage subscription permission in order to reduce administrative efforts for super user.

Allow subscribers to access subscription admin-api

9bba9a2

rdhabalia requested review from merlimat, sijie, saandrews and yush1ga November 13, 2018 20:32

merlimat reviewed Nov 14, 2018

View reviewed changes

add sub-authorization check on canConsume

0879b59

merlimat added this to the 2.3.0 milestone Nov 14, 2018

merlimat added the type/feature The PR added a new feature or issue requested a new feature label Nov 14, 2018

merlimat assigned rdhabalia Nov 14, 2018

make per-sub optional

775894a

massakam reviewed Nov 16, 2018

View reviewed changes

fix: allowe super-admin to consume

44b5a5a

rdhabalia mentioned this pull request Nov 19, 2018

Allow subscriber to access cursor admin-api #2964

Closed

merlimat approved these changes Nov 30, 2018

View reviewed changes

merlimat merged commit 02df3d2 into apache:master Dec 3, 2018

rdhabalia deleted the sub_per branch February 19, 2019 00:32

candlerb mentioned this pull request Nov 2, 2019

Reader method to locate message ID by event time #5537

Closed

rdhabalia mentioned this pull request Jan 22, 2020

[pulsar-admin] allow tenant admin to manage subscription permission #6122

Merged

michaeljmarshall mentioned this pull request Aug 27, 2021

fix authorization not working when subscription permission is empty #11113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow subscribers to access subscription admin-api #2981

Allow subscribers to access subscription admin-api #2981

rdhabalia commented Nov 13, 2018

merlimat commented Nov 13, 2018

rdhabalia commented Nov 13, 2018

merlimat commented Nov 13, 2018

rdhabalia commented Nov 13, 2018

merlimat commented Nov 13, 2018

merlimat commented Nov 13, 2018

rdhabalia commented Nov 13, 2018

rdhabalia commented Nov 14, 2018

merlimat commented Nov 14, 2018

rdhabalia commented Nov 14, 2018

merlimat Nov 14, 2018

rdhabalia Nov 14, 2018

merlimat Nov 14, 2018

rdhabalia Nov 14, 2018

rdhabalia Nov 14, 2018

merlimat Nov 14, 2018

rdhabalia Nov 14, 2018

merlimat Nov 15, 2018

rdhabalia Nov 15, 2018

rdhabalia Nov 15, 2018

nkurihar commented Nov 15, 2018

rdhabalia commented Nov 15, 2018

nkurihar commented Nov 15, 2018 •

edited

massakam Nov 16, 2018

rdhabalia Nov 16, 2018

massakam Nov 16, 2018

rdhabalia Nov 19, 2018

massakam Nov 19, 2018 •

edited

rdhabalia Nov 19, 2018

massakam Nov 16, 2018

merlimat left a comment

	if (service.isAuthorizationEnabled()) {
	authorizationFuture = service.getAuthorizationService().canConsumeAsync(topicName,
	originalPrincipal != null ? originalPrincipal : authRole, authenticationData,
	subscription);
	} else {
	authorizationFuture = CompletableFuture.completedFuture(true);
	}

Allow subscribers to access subscription admin-api #2981

Allow subscribers to access subscription admin-api #2981

Conversation

rdhabalia commented Nov 13, 2018

Motivation

Result

merlimat commented Nov 13, 2018

rdhabalia commented Nov 13, 2018

merlimat commented Nov 13, 2018

rdhabalia commented Nov 13, 2018

merlimat commented Nov 13, 2018

merlimat commented Nov 13, 2018

rdhabalia commented Nov 13, 2018

rdhabalia commented Nov 14, 2018

merlimat commented Nov 14, 2018

rdhabalia commented Nov 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkurihar commented Nov 15, 2018

rdhabalia commented Nov 15, 2018

nkurihar commented Nov 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

massakam Nov 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

nkurihar commented Nov 15, 2018 •

edited

massakam Nov 19, 2018 •

edited