Hi Pulsar Community, This is the voting thread for PIP 167.
GitHub Issue: https://github.com/apache/pulsar/issues/15597. Discussion Thread: https://lists.apache.org/thread/x6zg2l7hrtopd0yty93fhctsnm9n0wbt Thanks, Michael ------ Mailing list thread: https://lists.apache.org/thread/x6zg2l7hrtopd0yty93fhctsnm9n0wbt ## Motivation Pulsar supports subscription level authorization. When combined with topic level authorization, a user can configure Pulsar to limit which roles can consume from which topic subscriptions. However, when this feature is left unconfigured for a subscription, a role that has permission to consume from a topic is, by default, implicitly granted permission to consume from any subscription on that topic. As a consequence, a missed security configuration could lead to accidental privilege escalation. Here is a reference to the code responsible for the current behavior: https://github.com/apache/pulsar/blob/6864b0ae5520e06b9d0fc5dcfa5a0a0a44feee87/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L115-L122 ## Goal I propose we add a namespace policy to configure a Pulsar namespace to either allow all or reject all roles when there is no configuration for a specific subscription’s permission. This way, a missed configuration results in a rejected request due to insufficient permission. This PIP will not change the current behavior and will be backwards compatible. It will add a new boolean field to the existing `auth_policies` namespace policy to configure how the `PulsarAuthorizationProvider` handles an empty set of allowed roles in the `canConsume` method. ## Naming I am not settled on the right name for this feature/namespace policy yet. Hopefully this thread can help identify the right name. First, the existing subscription level authorization feature has several names. The Admin API calls this feature `PermissionOnSubscription`, the Pulsar Admin CLI tool calls it `subscription-permission`, the AuthPolicies interface calls it `SubscriptionAuthentication`, and the value is stored in the metadata store as `subscription_auth_roles`. My preferred names for this feature are `implicit_subscription_auth` and `implicitPermissionOnSubscription` because they work well with the “grant” and “revoke” actions, e.g. `grantImplicitPermissionOnSubscription` would be a PUT/POST call to the `/implicitPermissionOnSubscription` endpoint to set the policy value to true. However, that policy name requires the default value to be true to maintain backwards compatibility. Enrico expressed concern that defaulting to true is problematic for the upgrade path: https://github.com/apache/pulsar/pull/15576#discussion_r872045946. Alternatively, we could use the names `PermissionOnSubscriptionRequired` and `subscription_auth_required`. In that case, I would switch the admin API so that the admin API has a single setter endpoint that takes the configuration as a part of the body instead of relying on PUT to mean grant permission and DELETE to mean revoke permission. Please let me know if you have thoughts on what name(s) make sense for this feature. ## Naming Conclusion The current conclusion is to use `PermissionOnSubscriptionRequired` and `subscription_auth_required`. ## API Changes The API changes include updating the Admin API to enable getting and modifying the namespace policy, as well as updating the namespace AuthPolicy interface to store this new metadata field. There are also analogous updates to the admin client and the `pulsar-admin` cli tool. New endpoint for v1: ```java @POST @Path("/{property}/{cluster}/{namespace}/permissionOnSubscriptionRequired") @ApiOperation(hidden = true, value = "Set whether a role requires explicit permission to consume from a " + "subscription that has no subscription permission defined in the 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 = "Concurrent modification"), @ApiResponse(code = 501, message = "Authorization is not enabled")}) public void setPermissionOnSubscriptionRequired( @Suspended final AsyncResponse asyncResponse, @PathParam("property") String property, @PathParam("cluster") String cluster, @PathParam("namespace") String namespace, boolean permissionOnSubscriptionRequired) { validateNamespaceName(property, cluster, namespace); internalSetPermissionOnSubscriptionRequired(asyncResponse, permissionOnSubscriptionRequired); } @GET @Path("/{property}/{cluster}/{namespace}/permissionOnSubscriptionRequired") @ApiOperation(value = "Get whether a role requires explicit permission to consume from a " + "subscription that has no subscription permission defined in the 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 void getPermissionOnSubscriptionRequired(@Suspended final AsyncResponse asyncResponse, @PathParam("property") String property, @PathParam("cluster") String cluster, @PathParam("namespace") String namespace) { validateNamespaceName(property, cluster, namespace); getPermissionOnSubscriptionRequired(asyncResponse); } ``` New endpoint for v2: ```java @POST @Path("/{property}/{namespace}/permissionOnSubscriptionRequired") @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 setPermissionOnSubscriptionRequired( @Suspended final AsyncResponse asyncResponse, @PathParam("property") String property, @PathParam("namespace") String namespace, boolean required) { validateNamespaceName(property, namespace); internalSetPermissionOnSubscriptionRequired(asyncResponse, required); } @GET @Path("/{property}/{namespace}/permissionOnSubscriptionRequired") @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 void getPermissionOnSubscriptionRequired(@Suspended final AsyncResponse asyncResponse, @PathParam("property") String property, @PathParam("namespace") String namespace) { validateNamespaceName(property, namespace); getPermissionOnSubscriptionRequired(asyncResponse); } ``` New update to the `AuthPolicies` interface: ```java /** * Whether an empty set of subscription authentication roles returned by {@link #getSubscriptionAuthentication()} * requires explicit permission to consume from the target subscription. * @return */ boolean isSubscriptionAuthRequired(); void setSubscriptionAuthRequired(boolean subscriptionAuthRequired); ``` ## Implementation Draft implementation: https://github.com/apache/pulsar/pull/15576 The core update is to the `PulsarAuthorizationProvider#canConsumeAsync` method so that when `implicit_subscription_auth` is true, a null or empty set of roles for a subscription’s permission will result in granted permission to consume from the subscription, and when `implicit_subscription_auth` is false, a null or empty set of roles for a subscription’s permission will result in rejected permission to consume from the subscription. Note that if we negate the meaning of the variable name, the logic will also be inverted appropriately. ## Rejected Alternatives First, we have already received a PR proposing to change the default behavior for all use cases: https://github.com/apache/pulsar/pull/11113. This PR went stale because changing the default would break many deployments. By making the behavior configurable, we satisfy the requirements requested in that PR without breaking existing deployments. Second, it’s possible to implement a new `AuthorizationProvider` to get this feature. However, I believe this feature will benefit users and is a natural development on the existing Pulsar features around subscription authorization, so I think we should include it in the default `PulsarAuthorizationProvider`. Third, the initial name for this feature was `implicitPermissionOnSubscription`. It defaulted to `true`, which is problematic for backwards compatibility.
