Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PIP-167: Make it Configurable to Require Subscription Permission #15597

Closed
michaeljmarshall opened this issue May 13, 2022 · 1 comment · Fixed by #15576
Closed

PIP-167: Make it Configurable to Require Subscription Permission #15597

michaeljmarshall opened this issue May 13, 2022 · 1 comment · Fixed by #15576

Comments

@michaeljmarshall
Copy link
Member

michaeljmarshall commented May 13, 2022

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:

// validate if role is authorized to access subscription. (skip validation if authorization
// list is empty)
Set<String> roles = policies.get().auth_policies
.getSubscriptionAuthentication().get(subscription);
if (roles != null && !roles.isEmpty() && !roles.contains(role)) {
log.warn("[{}] is not authorized to subscribe on {}-{}", role, topicName, subscription);
return CompletableFuture.completedFuture(false);
}

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: #15576 (comment).

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:

    @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:

    @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:

    /**
     * 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: #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: #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.

@github-actions
Copy link

The issue had no activity for 30 days, mark with Stale label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant