+1 (binding) Enrico
Il Gio 19 Mag 2022, 20:51 Michael Marshall <mmarsh...@apache.org> ha scritto: > 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. >
