[DISCUSS] PIP-167: Make it Configurable to Require Subscription Permission

[Michael Marshall <mm...@apache.org>]

Hello Pulsar Community,

Here is a PIP to add a new namespace policy to configure how the
PulsarAuthorizationProvider handles the default subscription
permission value (null/an empty set). I look forward to your feedback.

PIP: https://github.com/apache/pulsar/issues/15597

Thanks,
Michael

## 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.

## 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.

## 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`.

Re: [DISCUSS] PIP-167: Make it Configurable to Require Subscription Permission

[Enrico Olivelli <eo...@gmail.com>]

The proposal makes sense to me.

+1

Enrico

Il giorno mer 18 mag 2022 alle ore 07:18 Michael Marshall
<mm...@apache.org> ha scritto:
>
> I switched to the name "permissionOnSubscriptionRequired" for this
> feature [0]. It describes the feature while satisfying the requirement
> to default to false.
>
> I plan to address the PR's remaining feedback and write tests tomorrow
> (Wednesday). If there isn't any other discussion, I'll start the vote
> once I get tests passing.
>
> Thanks,
> Michael
>
> [0] https://github.com/apache/pulsar/pull/15576
>
> On Fri, May 13, 2022 at 2:53 PM Michael Marshall <mm...@apache.org> wrote:
> >
> > Hello Pulsar Community,
> >
> > Here is a PIP to add a new namespace policy to configure how the
> > PulsarAuthorizationProvider handles the default subscription
> > permission value (null/an empty set). I look forward to your feedback.
> >
> > PIP: https://github.com/apache/pulsar/issues/15597
> >
> > Thanks,
> > Michael
> >
> > ## 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.
> >
> > ## 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.
> >
> > ## 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`.

Re: [DISCUSS] PIP-167: Make it Configurable to Require Subscription Permission

[Michael Marshall <mm...@apache.org>]

I switched to the name "permissionOnSubscriptionRequired" for this
feature [0]. It describes the feature while satisfying the requirement
to default to false.

I plan to address the PR's remaining feedback and write tests tomorrow
(Wednesday). If there isn't any other discussion, I'll start the vote
once I get tests passing.

Thanks,
Michael

[0] https://github.com/apache/pulsar/pull/15576

On Fri, May 13, 2022 at 2:53 PM Michael Marshall <mm...@apache.org> wrote:
>
> Hello Pulsar Community,
>
> Here is a PIP to add a new namespace policy to configure how the
> PulsarAuthorizationProvider handles the default subscription
> permission value (null/an empty set). I look forward to your feedback.
>
> PIP: https://github.com/apache/pulsar/issues/15597
>
> Thanks,
> Michael
>
> ## 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.
>
> ## 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.
>
> ## 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`.