The proposal makes sense to me. +1
Enrico Il giorno mer 18 mag 2022 alle ore 07:18 Michael Marshall <mmarsh...@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 <mmarsh...@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`.
