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.

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

[fix][broker] Fix MultiRoles token provider NPE when using anonymous clients #21429

Merged
merged 3 commits into from
Oct 25, 2023
Merged

[fix][broker] Fix MultiRoles token provider NPE when using anonymous clients #21429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
38c2db1
Fix MultiRoles token provider NPE when using anonymous clients
Technoboy- Oct 24, 2023
96d2704
Check super-user first
Technoboy- Oct 25, 2023
a00ed88
fix test
Technoboy- Oct 25, 2023
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
4 changes: 4 additions & 0 deletions ...src/main/java/org/apache/pulsar/broker/authentication/AuthenticationDataSubscription.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,8 @@ public boolean hasSubscription() {
public String getSubscription() {
return subscription;
}

public AuthenticationDataSource getAuthData() {
Copy link
Member

Choose a reason for hiding this comment

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

Why not you check the authData in the constructor or override methods?

    @Override
    public boolean hasDataFromTls() {
        return authData != null && authData.hasDataFromTls();
    }

We should avoid type checking.

(authData instanceof AuthenticationDataSubscription && ((AuthenticationDataSubscription) authData).getAuthData() == null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the getRoles method will return empty

Copy link
Member

Choose a reason for hiding this comment

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

I have communicated with @Technoboy- T offline. I don't suggest that add getAuthData() method, and makes the code difficult to maintain.

The following are my ideas:

  • idea 1
    Add a new AuthenticationDataSource for the anonymous role, and then check the authData type.

  • idea 2
    Change the org.apache.pulsar.broker.authorization.MultiRolesTokenAuthorizationProvider#getRoles logic to quickly return role when role equals anonymous role:

    private Set<String> getRoles(String role, AuthenticationDataSource authData) {
        if (authData == null || role.equals(conf.getAnonymousUserRole())) {
            return Collections.singleton(role);
        }

There will be a pitfall here, if the client role is equal to the anonymous role, the real roles cannot be obtained from authData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idea 1 is more pretty, I will create a issue to track this.

return authData;
}
}
24 changes: 16 additions & 8 deletions ...ain/java/org/apache/pulsar/broker/authorization/MultiRolesTokenAuthorizationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.apache.commons.lang3.StringUtils;
import org.apache.pulsar.broker.ServiceConfiguration;
import org.apache.pulsar.broker.authentication.AuthenticationDataSource;
import org.apache.pulsar.broker.authentication.AuthenticationDataSubscription;
import org.apache.pulsar.broker.resources.PulsarResources;
import org.apache.pulsar.common.naming.NamespaceName;
import org.apache.pulsar.common.naming.TopicName;
Expand Down Expand Up @@ -144,7 +145,8 @@ public CompletableFuture<Boolean> validateTenantAdminAccess(String tenantName, S
}

private Set<String> getRoles(String role, AuthenticationDataSource authData) {
if (authData == null) {
if (authData == null || (authData instanceof AuthenticationDataSubscription
&& ((AuthenticationDataSubscription) authData).getAuthData() == null)) {
return Collections.singleton(role);
}

Expand Down Expand Up @@ -198,13 +200,19 @@ private Set<String> getRoles(String role, AuthenticationDataSource authData) {

public CompletableFuture<Boolean> authorize(String role, AuthenticationDataSource authenticationData,
Function<String, CompletableFuture<Boolean>> authorizeFunc) {
Set<String> roles = getRoles(role, authenticationData);
if (roles.isEmpty()) {
return CompletableFuture.completedFuture(false);
}
List<CompletableFuture<Boolean>> futures = new ArrayList<>(roles.size());
roles.forEach(r -> futures.add(authorizeFunc.apply(r)));
return FutureUtil.waitForAny(futures, ret -> (boolean) ret).thenApply(v -> v.isPresent());
return isSuperUser(role, authenticationData, conf)
.thenCompose(superUser -> {
if (superUser) {
return CompletableFuture.completedFuture(true);
}
Set<String> roles = getRoles(role, authenticationData);
if (roles.isEmpty()) {
return CompletableFuture.completedFuture(false);
}
List<CompletableFuture<Boolean>> futures = new ArrayList<>(roles.size());
roles.forEach(r -> futures.add(authorizeFunc.apply(r)));
return FutureUtil.waitForAny(futures, ret -> (boolean) ret).thenApply(v -> v.isPresent());
});
}

/**
Expand Down
22 changes: 22 additions & 0 deletions ...java/org/apache/pulsar/broker/authorization/MultiRolesTokenAuthorizationProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import lombok.Cleanup;
import org.apache.pulsar.broker.ServiceConfiguration;
import org.apache.pulsar.broker.authentication.AuthenticationDataSource;
import org.apache.pulsar.broker.authentication.AuthenticationDataSubscription;
import org.apache.pulsar.broker.authentication.utils.AuthTokenUtils;
import org.apache.pulsar.broker.resources.PulsarResources;
import org.testng.annotations.Test;
Expand All @@ -46,6 +47,8 @@ public void testMultiRolesAuthz() throws Exception {
String token = Jwts.builder().claim("sub", new String[]{userA, userB}).signWith(secretKey).compact();

MultiRolesTokenAuthorizationProvider provider = new MultiRolesTokenAuthorizationProvider();
ServiceConfiguration conf = new ServiceConfiguration();
provider.initialize(conf, mock(PulsarResources.class));

AuthenticationDataSource ads = new AuthenticationDataSource() {
@Override
Expand Down Expand Up @@ -85,6 +88,8 @@ public void testMultiRolesAuthzWithEmptyRoles() throws Exception {
String token = Jwts.builder().claim("sub", new String[]{}).signWith(secretKey).compact();

MultiRolesTokenAuthorizationProvider provider = new MultiRolesTokenAuthorizationProvider();
ServiceConfiguration conf = new ServiceConfiguration();
provider.initialize(conf, mock(PulsarResources.class));

AuthenticationDataSource ads = new AuthenticationDataSource() {
@Override
Expand Down Expand Up @@ -112,6 +117,8 @@ public void testMultiRolesAuthzWithSingleRole() throws Exception {
String token = Jwts.builder().claim("sub", testRole).signWith(secretKey).compact();

MultiRolesTokenAuthorizationProvider provider = new MultiRolesTokenAuthorizationProvider();
ServiceConfiguration conf = new ServiceConfiguration();
provider.initialize(conf, mock(PulsarResources.class));

AuthenticationDataSource ads = new AuthenticationDataSource() {
@Override
Expand Down Expand Up @@ -141,6 +148,9 @@ public String getHttpHeader(String name) {
public void testMultiRolesAuthzWithAnonymousUser() throws Exception {
@Cleanup
MultiRolesTokenAuthorizationProvider provider = new MultiRolesTokenAuthorizationProvider();
ServiceConfiguration conf = new ServiceConfiguration();

provider.initialize(conf, mock(PulsarResources.class));

Function<String, CompletableFuture<Boolean>> authorizeFunc = (String role) -> {
if (role.equals("test-role")) {
Expand All @@ -150,13 +160,16 @@ public void testMultiRolesAuthzWithAnonymousUser() throws Exception {
};
assertTrue(provider.authorize("test-role", null, authorizeFunc).get());
assertFalse(provider.authorize("test-role-x", null, authorizeFunc).get());
assertTrue(provider.authorize("test-role", new AuthenticationDataSubscription(null, "test-sub"), authorizeFunc).get());
}

@Test
public void testMultiRolesNotFailNonJWT() throws Exception {
String token = "a-non-jwt-token";

MultiRolesTokenAuthorizationProvider provider = new MultiRolesTokenAuthorizationProvider();
ServiceConfiguration conf = new ServiceConfiguration();
provider.initialize(conf, mock(PulsarResources.class));

AuthenticationDataSource ads = new AuthenticationDataSource() {
@Override
Expand Down Expand Up @@ -246,5 +259,14 @@ public String getHttpHeader(String name) {
};

assertTrue(provider.isSuperUser(testAdminRole, ads, conf).get());
Function<String, CompletableFuture<Boolean>> authorizeFunc = (String role) -> {
if (role.equals("admin1")) {
return CompletableFuture.completedFuture(true);
}
return CompletableFuture.completedFuture(false);
};
assertTrue(provider.authorize(testAdminRole, ads, (String role) -> CompletableFuture.completedFuture(false)).get());
assertTrue(provider.authorize("admin1", null, authorizeFunc).get());
assertFalse(provider.authorize("admin2", null, authorizeFunc).get());
}
}