[PIP-167][Authorization] Make it Configurable to Require Subscription Permission #15576

michaeljmarshall · 2022-05-13T05:05:25Z

Motivation

See PIP-167 for more detail. At a high level, this PR seeks to make it possible to configure whether subscription level permission is required to consume from a subscription in a namespace where a subscription does not have any permission defined.

Modifications

Update the PulsarAuthorizationProvider#canConsume logic so that when subscription permission is required, it will reject all roles that do not explicitly have permission to consume from a subscription in the subscription permission map for the namespace.
Update the AuthPolicies and the AuthPoliciesImpl classes to store a new boolean field named subscription_auth_required.
Add new endpoints for getting permission, granting permission, and revoking permission to the Admin API server for both v1 and v2 APIs.
Similarly, update the Admin Java Client and the Admin CLI tool to use call those new API endpoints.

Verifying this change

Once the design is solidified, I will add tests to the pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java class.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API: yes
The schema: no
The default values of configurations: no
The wire protocol: no
The rest endpoints: yes
The admin cli options: yes
Anything that affects deployment: no

This change adds a new feature discussed in the associated PIP.

Documentation

doc-required
I will add docs as a part of this work, but I haven't yet because the design isn't solidified.

eolivelli · 2022-05-13T06:36:30Z

pulsar-common/src/main/java/org/apache/pulsar/client/admin/internal/data/AuthPoliciesImpl.java

@@ -42,6 +42,10 @@ public final class AuthPoliciesImpl implements AuthPolicies {
    @JsonProperty("subscription_auth_roles")
    private Map<String, Set<String>> subscriptionAuthentication = new TreeMap<>();

+    // Default value is set in the builder
+    @JsonProperty(value = "implicit_subscription_auth")
+    private boolean implicitSubscriptionAuth;


adding a field with default value "true" is problematic, thinking to the upgrade path.
This is because on storage (ZK) all the old Policies do not have this field, but we are considering it "true" if it is not present.
This may make us fall into some subtle problems with clusters upgraded to the latest version.

I suggest to go down these ways:

use Boolean (nullable boolean) and deal explicitly with the case "unknown"

negate the flag, this way the default will be "false" and this value will also be applied in case of null/non existing field (so old cluster upgraded, or updates happening during rolling upgrades of the cluster

I prefer 2) as it is easier to deal with both code changes and also with the migration

Good points Enrico. Yes this should be addressed.

Thank you for the review, @eolivelli. My preference is also 2. I'll replace implicit with explicit for the whole PR/PIP.

@eolivelli - on second thought, are you suggesting that I need to update the variable names or just that the stored metadata needs to default to false? The semantics of "granting permission" on the API will be different if we change the endpoint names to align with a default value where "false" means additional permission is granted roles. For example, an alternate name for the /implicitPermissionOnSubscription endpoint is /permissionOnSubscriptionRequired. In that case, granting permission would use a PUT/POST to set the value to false, which seems counter intuitive.

The main point is that the default value must be 'false' or null because this is what is on existing data.

That makes sense. I think the likely alternative name is permissionOnSubscriptionRequired. If someone has a better name, though, I'll use that.

@eolivelli - I updating the naming by using permissionOnSubscriptionRequired. Please let me know what you think about the name.

works for me

lhotari · 2022-05-13T07:35:18Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/Namespaces.java

+    @PUT
+    @Path("/{property}/{cluster}/{namespace}/implicitPermissionOnSubscription")
+    @ApiOperation(hidden = true, value = "Allow a consumer's role to have implicit permission to consume from a"
+            + " subscription.")
+    @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+            @ApiResponse(code = 404, message = "Property or cluster or namespace doesn't exist"),
+            @ApiResponse(code = 409, message = "Concurrent modification"),
+            @ApiResponse(code = 501, message = "Authorization is not enabled")})
+    public void grantImplicitPermissionOnSubscription(
+            @PathParam("property") String property, @PathParam("cluster") String cluster,
+            @PathParam("namespace") String namespace) {
+        validateNamespaceName(property, cluster, namespace);
+        internalSetImplicitPermissionOnSubscription(true);
+    }
+
+    @DELETE
+    @Path("/{property}/{cluster}/{namespace}/implicitPermissionOnSubscription")
+    @ApiOperation(hidden = true, value = "Require a consumer's role to have explicit permission to consume from a"
+            + " subscription.")
+    @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+            @ApiResponse(code = 404, message = "Property or cluster or namespace doesn't exist"),
+            @ApiResponse(code = 409, message = "Concurrent modification"),
+            @ApiResponse(code = 501, message = "Authorization is not enabled")})
+    public void revokeImplicitPermissionOnSubscription(
+            @PathParam("property") String property, @PathParam("cluster") String cluster,
+            @PathParam("namespace") String namespace) {
+        validateNamespaceName(property, cluster, namespace);
+        internalSetImplicitPermissionOnSubscription(false);
+    }
+
+    @GET
+    @Path("/{property}/{cluster}/{namespace}/implicitPermissionOnSubscription")
+    @ApiOperation(value = "Get permission on subscription required for namespace.")
+    @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+            @ApiResponse(code = 404, message = "Property or cluster or namespace doesn't exist"),
+            @ApiResponse(code = 409, message = "Namespace is not empty")})
+    public boolean getImplicitPermissionOnSubscription(@PathParam("property") String property,
+                                                       @PathParam("cluster") String cluster,
+                                                       @PathParam("namespace") String namespace) {
+        validateNamespaceName(property, cluster, namespace);
+        return getImplicitPermissionOnSubscription();
+    }
+


Does this have to be added to the v1 API at all?

Good question, I'm not sure. I don't know how we're approaching new features like this. I'm open to removing it, if that's the consensus.

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java

lhotari · 2022-05-13T07:47:40Z

pulsar-common/src/main/java/org/apache/pulsar/client/admin/internal/data/AuthPoliciesImpl.java

@@ -42,6 +42,10 @@ public final class AuthPoliciesImpl implements AuthPolicies {
    @JsonProperty("subscription_auth_roles")
    private Map<String, Set<String>> subscriptionAuthentication = new TreeMap<>();

+    // Default value is set in the builder
+    @JsonProperty(value = "implicit_subscription_auth")
+    private boolean implicitSubscriptionAuth;


Good points Enrico. Yes this should be addressed.

eolivelli

+1
(there are some TODOs, but I don't think we can address them with this work, maybe you can remove the TODO, or at least like a GH issue near the TODO)

github-actions · 2022-06-23T02:14:57Z

The pr had no activity for 30 days, mark with Stale label.

tisonkun · 2022-12-06T11:08:39Z

@michaeljmarshall do you continue this work?

cc @nodece

nodece

Makes sense.

Signed-off-by: tison <[email protected]>

This reverts commit 96be50c.

Signed-off-by: tison <[email protected]>

codecov-commenter · 2022-12-10T15:40:37Z

Codecov Report

Merging #15576 (c6a4b3a) into master (1b5722d) will increase coverage by 1.47%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master   #15576      +/-   ##
============================================
+ Coverage     46.34%   47.82%   +1.47%     
- Complexity    10394    11213     +819     
============================================
  Files           703      763      +60     
  Lines         68838    73640    +4802     
  Branches       7379     7915     +536     
============================================
+ Hits          31905    35217    +3312     
- Misses        33324    34557    +1233     
- Partials       3609     3866     +257

Flag	Coverage Δ
unittests	`47.82% <0.00%> (+1.47%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...pache/pulsar/broker/admin/impl/NamespacesBase.java	`62.85% <0.00%> (-0.24%)`	⬇️
.../org/apache/pulsar/broker/admin/v1/Namespaces.java	`48.47% <0.00%> (-0.48%)`	⬇️
.../org/apache/pulsar/broker/admin/v2/Namespaces.java	`57.80% <0.00%> (+0.45%)`	⬆️
...rg/apache/pulsar/broker/lookup/v1/TopicLookup.java	`60.00% <0.00%> (-13.34%)`	⬇️
...pulsar/broker/service/PulsarCommandSenderImpl.java	`75.38% <0.00%> (-3.08%)`	⬇️
...roker/service/persistent/MessageDeduplication.java	`50.65% <0.00%> (-3.06%)`	⬇️
...rg/apache/pulsar/proxy/server/ProxyConnection.java	`55.21% <0.00%> (-1.13%)`	⬇️
...lsar/broker/loadbalance/impl/ThresholdShedder.java	`30.32% <0.00%> (-0.82%)`	⬇️
...tent/PersistentDispatcherSingleActiveConsumer.java	`59.24% <0.00%> (-0.63%)`	⬇️
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java	`37.06% <0.00%> (-0.06%)`	⬇️
... and 182 more

tisonkun · 2022-12-11T01:16:10Z

Merging...

Thanks for your all efforts.

tisonkun · 2022-12-11T01:23:51Z

@michaeljmarshall @eolivelli @nodece @lhotari

Although, I noticed that this proposal didn't pass a formal PIP vote yet, while there is a thread without objection. I suggest you continue that thread to make a formal resolution.

…cription Permission (#15576)" This reverts commit bf29a4b.

… Permission (apache#15576)

momo-jun · 2023-01-18T10:23:57Z

I will add docs as a part of this work, but I haven't yet because the design isn't solidified.

Hi @michaeljmarshall, do you have any updates for adding the docs?
Since 2.11 has been released, now it's good timing to start the doc update in the next version.

michaeljmarshall · 2023-01-18T14:56:04Z

@momo-jun this PR was reverted, so no docs are needed.

michaeljmarshall added area/security doc-required Your PR changes impact docs and you will update later. component/authorization labels May 13, 2022

michaeljmarshall added this to the 2.11.0 milestone May 13, 2022

michaeljmarshall self-assigned this May 13, 2022

Make Implicit Subscription Permission Configurable

52438cc

michaeljmarshall force-pushed the implicit-subscription-permission branch from 4c56cf8 to 52438cc Compare May 13, 2022 05:20

eolivelli requested changes May 13, 2022

View reviewed changes

lhotari reviewed May 13, 2022

View reviewed changes

michaeljmarshall mentioned this pull request May 13, 2022

PIP-167: Make it Configurable to Require Subscription Permission #15597

Closed

sijie mentioned this pull request May 14, 2022

ISSUE-15597: PIP-167: Make it Configurable to Require Subscription Permission streamnative/pulsar-archived#4216

Open

Replace implicit with required in feature name

93c9940

eolivelli approved these changes May 18, 2022

View reviewed changes

michaeljmarshall added 3 commits May 18, 2022 13:30

Replace PUT with POST

e017e62

Add test

879047f

Make admin server operations completely async

ae4d425

michaeljmarshall requested a review from lhotari May 18, 2022 20:06

michaeljmarshall changed the title ~~[PIP][Authorization] Make Implicit Subscription Permission Configurable~~ [PIP-167][Authorization] Make Implicit Subscription Permission Configurable May 19, 2022

michaeljmarshall marked this pull request as ready for review May 19, 2022 18:41

michaeljmarshall changed the title ~~[PIP-167][Authorization] Make Implicit Subscription Permission Configurable~~ [PIP-167][Authorization] Make it Configurable to Require Subscription Permission May 23, 2022

github-actions bot added the Stale label Jun 23, 2022

codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022

tisonkun added area/authn and removed component/authorization labels Dec 6, 2022

github-actions bot removed the Stale label Dec 7, 2022

nodece approved these changes Dec 7, 2022

View reviewed changes

Merge branch 'master' into implicit-subscription-permission

61edf83

tisonkun added 3 commits December 10, 2022 23:11

checkstyle

96be50c

Signed-off-by: tison <[email protected]>

Revert "checkstyle"

3d69525

This reverts commit 96be50c.

revert style changes

c6a4b3a

Signed-off-by: tison <[email protected]>

tisonkun force-pushed the implicit-subscription-permission branch from 33001f4 to c6a4b3a Compare December 10, 2022 15:24

tisonkun merged commit bf29a4b into apache:master Dec 11, 2022

tisonkun added a commit that referenced this pull request Dec 11, 2022

Revert "[PIP-167][Authorization] Make it Configurable to Require Subs…

0abf263

…cription Permission (#15576)" This reverts commit bf29a4b.

tisonkun mentioned this pull request Dec 11, 2022

Revert "[PIP-167][Authorization] Make it Configurable to Require Subscription Permission" #18867

Merged

1 task

Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 26, 2022

[PIP-167][Authorization] Make it Configurable to Require Subscription…

1a8e4c6

… Permission (apache#15576)

Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 29, 2022

[PIP-167][Authorization] Make it Configurable to Require Subscription…

dc66031

… Permission (apache#15576)

lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023

[PIP-167][Authorization] Make it Configurable to Require Subscription…

1a2495f

… Permission (apache#15576)

michaeljmarshall added doc-not-needed Your PR changes do not impact docs and removed doc-required Your PR changes impact docs and you will update later. labels Jan 18, 2023

michaeljmarshall deleted the implicit-subscription-permission branch March 15, 2024 21:20

michaeljmarshall restored the implicit-subscription-permission branch March 15, 2024 21:20

michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Apr 4, 2024

Copy test from apache#15576

85e8409

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PIP-167][Authorization] Make it Configurable to Require Subscription Permission #15576

[PIP-167][Authorization] Make it Configurable to Require Subscription Permission #15576

michaeljmarshall commented May 13, 2022 •

edited

Loading

eolivelli May 13, 2022

lhotari May 13, 2022

michaeljmarshall May 13, 2022

michaeljmarshall May 13, 2022

eolivelli May 13, 2022

michaeljmarshall May 13, 2022

michaeljmarshall May 18, 2022

eolivelli May 18, 2022

lhotari May 13, 2022

michaeljmarshall May 13, 2022

lhotari May 13, 2022

eolivelli left a comment

github-actions bot commented Jun 23, 2022

tisonkun commented Dec 6, 2022

nodece left a comment •

edited

Loading

codecov-commenter commented Dec 10, 2022 •

edited

Loading

tisonkun commented Dec 11, 2022

tisonkun commented Dec 11, 2022

momo-jun commented Jan 18, 2023

michaeljmarshall commented Jan 18, 2023

[PIP-167][Authorization] Make it Configurable to Require Subscription Permission #15576

[PIP-167][Authorization] Make it Configurable to Require Subscription Permission #15576

Conversation

michaeljmarshall commented May 13, 2022 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

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

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 23, 2022

tisonkun commented Dec 6, 2022

nodece left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Dec 10, 2022 • edited Loading

Codecov Report

tisonkun commented Dec 11, 2022

tisonkun commented Dec 11, 2022

momo-jun commented Jan 18, 2023

michaeljmarshall commented Jan 18, 2023

michaeljmarshall commented May 13, 2022 •

edited

Loading

nodece left a comment •

edited

Loading

codecov-commenter commented Dec 10, 2022 •

edited

Loading