You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by Michael Marshall <mm...@apache.org> on 2022/05/19 18:50:33 UTC
[VOTE][PIP-167] Make it Configurable to Require Subscription Permission
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.
Re: [VOTE][PIP-167] Make it Configurable to Require Subscription Permission
Posted by Enrico Olivelli <eo...@gmail.com>.
+1 (binding)
Enrico
Il Gio 19 Mag 2022, 20:51 Michael Marshall <mm...@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.
>