Skip to content

fix authorization not working when subscription permission is empty #11113

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.

Sign up for GitHub

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

Closed
wuzhanpeng wants to merge 7 commits into from
Closed

fix authorization not working when subscription permission is empty #11113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c2a0a14
fix authorization not working when subscription permission is empty
Jun 26, 2021
f73bab7
Merge remote-tracking branch 'upstream/master' into bugfix_subscripti…
Jun 29, 2021
e9d0802
fix testcase
Jun 29, 2021
102b22e
fix testcase
Jun 29, 2021
cf6faf5
fix testcase
Jun 29, 2021
af517b8
fix testcase
Jun 30, 2021
e81aec8
fix testcase
Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...mon/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public CompletableFuture<Boolean> canConsumeAsync(TopicName topicName, String ro
// list is empty)
Copy link
Member

@michaeljmarshall michaeljmarshall Aug 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that the original intent of this code was to consider an empty list as a special case that had special logic. I don't have enough context to know why, but it might be worth reviewing #2981, where this code was first introduced. If I had to guess, it's because it doesn't make much sense to have a subscription that cannot be consumed.

If this change is accepted, the comment on this line and the one above need to be updated:

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java

Lines 119 to 120 in 879ab3a

// validate if role is authorize to access subscription. (skip validatation if authorization
// list is empty)

Set<String> roles = policies.get().auth_policies
.getSubscriptionAuthentication().get(subscription);
if (roles != null && !roles.isEmpty() && !roles.contains(role)) {
if (roles == null || roles.isEmpty() || !roles.contains(role)) {
log.warn("[{}] is not authorized to subscribe on {}-{}", role, topicName, subscription);
permissionFuture.complete(false);
return;
Expand Down
2 changes: 1 addition & 1 deletion pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ public void checkPermissions() {
return;
}
} catch (Exception e) {
log.warn("[{}] Get unexpected error while autorizing [{}] {}", appId, subscription.getTopicName(),
log.warn("[{}] Get unexpected error while authorizing [{}] {}", appId, subscription.getTopicName(),
e.getMessage(), e);
}
log.info("[{}] is not allowed to consume from topic [{}] anymore", appId, subscription.getTopicName());
Expand Down
5 changes: 5 additions & 0 deletions pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/AuthorizationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,11 @@ public void simple() throws Exception {
// tests for subscription auth mode
admin.namespaces().grantPermissionOnNamespace("p1/c1/ns1", "*", EnumSet.of(AuthAction.consume));
admin.namespaces().setSubscriptionAuthMode("p1/c1/ns1", SubscriptionAuthMode.Prefix);

admin.namespaces().grantPermissionOnSubscription("p1/c1/ns1", "sub1", Sets.newHashSet("role1"));
admin.namespaces().grantPermissionOnSubscription("p1/c1/ns1", "sub2", Sets.newHashSet("role2"));
admin.namespaces().grantPermissionOnSubscription("p1/c1/ns1", "role1-sub1", Sets.newHashSet("role1"));
admin.namespaces().grantPermissionOnSubscription("p1/c1/ns1", "role2-sub2", Sets.newHashSet("role2"));
waitForChange();

assertTrue(auth.canLookup(TopicName.get("persistent://p1/c1/ns1/ds1"), "role1", null));
Expand Down
17 changes: 12 additions & 5 deletions ...-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,16 @@ public void testSubscriberPermission() throws Exception {
.authentication(authentication));

// (1) Create subscription name
Consumer<byte[]> consumer = pulsarClient.newConsumer().topic(topicName).subscriptionName(subscriptionName)
.subscribe();
consumer.close();
// should fail with empty subscription permission
try {
Consumer<byte[]> consumer = pulsarClient.newConsumer().topic(topicName).subscriptionName(subscriptionName)
.subscribe();
fail("should have fail with authorization exception");
} catch (org.apache.pulsar.client.api.PulsarClientException.AuthorizationException e) {
// OK
}
superAdmin.topics().createNonPartitionedTopic(topicName);
superAdmin.topics().createSubscription(topicName, subscriptionName, MessageId.latest);

// verify tenant is able to perform all subscription-admin api
tenantAdmin.topics().skipAllMessages(topicName, subscriptionName);
Expand All @@ -228,8 +235,8 @@ public void testSubscriberPermission() throws Exception {
tenantAdmin.topics().resetCursor(topicName, subscriptionName, MessageId.earliest);

// grant namespace-level authorization to the subscriptionRole
tenantAdmin.namespaces().grantPermissionOnNamespace(namespace, subscriptionRole,
Collections.singleton(AuthAction.consume));
tenantAdmin.namespaces().grantPermissionOnSubscription(namespace, subscriptionName,
Collections.singleton(subscriptionRole));

// subscriptionRole has namespace-level authorization
sub1Admin.topics().resetCursor(topicName, subscriptionName, 10);
Expand Down
2 changes: 2 additions & 0 deletions pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyForwardAuthDataTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ public void testForwardAuthData() throws Exception {
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnNamespace(namespaceName, "client",
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnSubscription(namespaceName, subscriptionName,
Sets.newHashSet("proxy", "client"));

// Step 2: Run Pulsar Proxy without forwarding authData - expect Exception
ProxyConfiguration proxyConfig = new ProxyConfiguration();
Expand Down
2 changes: 2 additions & 0 deletions pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyRolesEnforcementTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ public void testIncorrectRoles() throws Exception {
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnNamespace(namespaceName, "client",
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnSubscription(namespaceName, subscriptionName,
Sets.newHashSet("proxy", "client"));

// Step 2: Try to use proxy Client as a normal Client - expect exception
PulsarClient proxyClient = createPulsarClient(pulsar.getBrokerServiceUrl(), proxyAuthParams);
Expand Down
2 changes: 2 additions & 0 deletions pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyWithAuthorizationNegTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ public void testProxyAuthorization() throws Exception {
admin.namespaces().grantPermissionOnNamespace(namespaceName, "Proxy", Sets.newHashSet(AuthAction.produce));
admin.namespaces().grantPermissionOnNamespace(namespaceName, "Client",
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnSubscription(namespaceName, "my-subscriber-name",
Sets.newHashSet("Proxy", "Client"));

Consumer<byte[]> consumer;
try {
Expand Down
8 changes: 8 additions & 0 deletions pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyWithAuthorizationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,8 @@ public void testProxyAuthorization() throws Exception {
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnNamespace(namespaceName, "Client",
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnSubscription(namespaceName, "my-subscriber-name",
Sets.newHashSet("Proxy", "Client"));

Consumer<byte[]> consumer = proxyClient.newConsumer()
.topic("persistent://my-property/proxy-authorization/my-ns/my-topic1")
Expand Down Expand Up @@ -305,6 +307,8 @@ public void testTlsHostVerificationProxyToClient(boolean hostnameVerificationEna
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnNamespace(namespaceName, "Client",
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnSubscription(namespaceName, "my-subscriber-name",
Sets.newHashSet("Proxy", "Client"));

try {
proxyClient.newConsumer().topic("persistent://my-property/proxy-authorization/my-ns/my-topic1")
Expand Down Expand Up @@ -357,6 +361,8 @@ public void testTlsHostVerificationProxyToBroker(boolean hostnameVerificationEna
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnNamespace(namespaceName, "Client",
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnSubscription(namespaceName, "my-subscriber-name",
Sets.newHashSet("Proxy", "Client"));

try {
proxyClient.newConsumer().topic("persistent://my-property/proxy-authorization/my-ns/my-topic1")
Expand Down Expand Up @@ -394,6 +400,8 @@ public void tlsCiphersAndProtocols(Set<String> tlsCiphers, Set<String> tlsProtoc
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnNamespace(namespaceName, "Client",
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnSubscription(namespaceName, "my-subscriber-name",
Sets.newHashSet("Proxy", "Client"));

ProxyConfiguration proxyConfig = new ProxyConfiguration();
proxyConfig.setAuthenticationEnabled(true);
Expand Down
9 changes: 8 additions & 1 deletion pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyWithJwtAuthorizationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ public void testProxyAuthorization() throws Exception {
// excepted
admin.namespaces().grantPermissionOnNamespace(namespaceName, CLIENT_ROLE,
Sets.newHashSet(AuthAction.consume));
admin.namespaces().grantPermissionOnSubscription(namespaceName, "my-subscriber-name",
Sets.newHashSet(CLIENT_ROLE));
log.info("-- Admin permissions {} ---", admin.namespaces().getPermissions(namespaceName));
consumer = proxyClient.newConsumer()
.topic("persistent://my-property/proxy-authorization/my-ns/my-topic1")
Expand Down Expand Up @@ -232,6 +234,8 @@ public void testUpdatePartitionNumAndReconnect() throws Exception {
admin.topics().createPartitionedTopic(topicName, 2);
admin.topics().grantPermission(topicName, CLIENT_ROLE,
Sets.newHashSet(AuthAction.consume, AuthAction.produce));
admin.namespaces().grantPermissionOnSubscription(namespaceName, subscriptionName,
Sets.newHashSet(CLIENT_ROLE));

Consumer<byte[]> consumer = proxyClient.newConsumer()
.topic(topicName)
Expand Down Expand Up @@ -333,9 +337,12 @@ public void testProxyAuthorizationWithPrefixSubscriptionAuthMode() throws Except
Assert.fail("should have failed with authorization error");
} catch (Exception ex) {
// excepted
String sub = CLIENT_ROLE + "-sub1";
admin.namespaces().grantPermissionOnSubscription(namespaceName, sub,
Sets.newHashSet(CLIENT_ROLE));
consumer = proxyClient.newConsumer()
.topic("persistent://my-property/proxy-authorization/my-ns/my-topic1")
.subscriptionName(CLIENT_ROLE + "-sub1").subscribe();
.subscriptionName(sub).subscribe();
}

Producer<byte[]> producer = proxyClient.newProducer(Schema.BYTES)
Expand Down