You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/05/24 06:52:34 UTC
[GitHub] [pulsar] RobertIndie opened a new pull request #10685: [Broker] Add multi roles support for authentication and authorization
RobertIndie opened a new pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685
### Motivation
In https://github.com/apache/pulsar/pull/10375, we add multi roles support for JWT authentication. But the authorization does not support multi roles currently. Only the first one in the roles array will be used during authorization. This PR adds multi roles support for both authentication and authorization.
### Modifications
* Change the `AuthenticationState` interface to make it keep an array of principals
* Change the authorization logic to do the permission verification over an array of principals iteratively. Return true when the principal has permission, or return false if all principals verified failed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r643340844
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -66,6 +69,10 @@ default String authenticate(AuthenticationDataSource authData) throws Authentica
throw new AuthenticationException("Not supported");
}
+ default List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
Review comment:
I do agree that the nomenclature we are using is very confusing. Some times we interchangeably use "principal" and "role" when by definition they can and should mean different things. However, this seems like an existing issue not necessarily introduced in this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cckellogg commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r640748222
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,36 @@
public class OneStageAuthenticationState implements AuthenticationState {
private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private List<String> authRoles;
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ try {
+ this.authRoles = provider.authenticate(authenticationDataSource, true);
+ } catch (AuthenticationException e) {
+ if (e.getMessage().equals(MULTI_ROLE_NOT_SUPPORTED)) {
+ this.authRoles = Collections.singletonList(provider.authenticate(authenticationDataSource));
+ } else {
+ throw e;
+ }
+ }
}
@Override
public String getAuthRole() {
- return authRole;
+ if (authRoles == null || authRoles.isEmpty()) {
+ return null;
+ }
+ return authRoles.get(0);
+ }
+
+ @Override
+ public List<String> getAuthRoles() {
+ return authRoles;
Review comment:
Avoid returning a possible empty collection
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -144,14 +145,29 @@ public String getAuthMethodName() {
@Override
public String authenticate(AuthenticationDataSource authData) throws AuthenticationException {
+ List<String> principals = authenticate(authData, false);
+ if (principals == null) {
+ return null;
+ }
+ return principals.get(0);
+ }
+
+ @Override
+ public List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
try {
// Get Token
String token;
token = getToken(authData);
// Parse Token by validating
- String role = getPrincipal(authenticateToken(token));
+ List<String> principals = getPrincipals(authenticateToken(token));
AuthenticationMetrics.authenticateSuccess(getClass().getSimpleName(), getAuthMethodName());
- return role;
+ if (multiRoles) {
+ return principals;
+ } else if (principals == null) { // Empty list check has been done in getPrincipals.
+ return null;
Review comment:
Avoid returning null for collections always return an empty collection instead
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,33 @@
public class OneStageAuthenticationState implements AuthenticationState {
private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private List<String> authRoles;
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ try {
+ this.authRoles = provider.authenticate(authenticationDataSource, true);
Review comment:
This will always throw an exception with older providers and I think it's better to have a flag instead of checking a string value in an exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r640251663
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -66,6 +69,10 @@ default String authenticate(AuthenticationDataSource authData) throws Authentica
throw new AuthenticationException("Not supported");
}
+ default List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
Review comment:
In some use cases, users need to use a set of roles for authorization instead of just one role. Authentication Provider should return all the roles and pass them to the authorization provider. In ServereCnx, we are using the auth Roles that returned from the authentication provider to authorize. I think it is necessary to let the authentication provider return a set of roles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r643342945
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -66,6 +69,10 @@ default String authenticate(AuthenticationDataSource authData) throws Authentica
throw new AuthenticationException("Not supported");
}
+ default List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
Review comment:
However, I don't understand why we need to pass in the boolean argument `multiRoles`. There is already a method, i.e. `isSupportMultiRoles()`, that returns whether the authenication provider supports multi-role or not
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#issuecomment-847595432
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#issuecomment-849427218
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cckellogg commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r639732409
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -66,6 +69,10 @@ default String authenticate(AuthenticationDataSource authData) throws Authentica
throw new AuthenticationException("Not supported");
}
+ default List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
Review comment:
Why do the roles need to be returned here?
This is confusing to me `roles` are a set of permissions assigned to a principal. The job of authentication is to identify the user/app and get the principal/app id. For any given authData there should only be one identifier for it. Im my opinion this in the the right way to go about this.
Roles should only be applicable when performing an authorization check. These roles can be accessed in the authorization provider by reading them from the authdata (this contains the token or cert). The default authorization provider could be updated to have some type of adapter to return the `roles` based on the authdata and principal that gets passed in. Then the default authorization provider can make checks against those roles.
I see this is related to (https://github.com/apache/pulsar/pull/10375)
According the the microsoft docs
https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens.
I think the `AuthenticationProviderToken.java` probably needs to be adjusted first.
Something other than roles like `sub` should be used to identify the principal or app id
https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cckellogg commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r639732409
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -66,6 +69,10 @@ default String authenticate(AuthenticationDataSource authData) throws Authentica
throw new AuthenticationException("Not supported");
}
+ default List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
Review comment:
Why do the roles need to be returned here?
This is confusing to me `roles` are a set of permissions assigned to a principal. The job of authentication is to identify the user/app and get the principal/app id. For any given authData there should only be one identifier for it. Im my opinion this in the the right way to go about this.
Roles should only be applicable when performing an authorization check. These roles can be accessed in the authorization provider by reading them from the authdata (this contains the token or cert). The default authorization provider could be updated to have some type of adapter to return the `roles` based on the authdata and principal that gets passed in. Then the default authorization provider can make checks against those roles.
I see this is related to (https://github.com/apache/pulsar/pull/10375)
According the the microsoft docs https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens) `roles` are the set of roles that were assigned to the user who is logging in. The `sub` or `oid` can be used to get the identifier of the token
I think the `AuthenticationProviderToken.java` probably needs to be adjusted first.
Something other than roles like `sub` should be used to identify the principal or app id.
https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#issuecomment-849302674
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cckellogg commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r643214504
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,36 @@
public class OneStageAuthenticationState implements AuthenticationState {
private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private List<String> authRoles;
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ try {
+ this.authRoles = provider.authenticate(authenticationDataSource, true);
+ } catch (AuthenticationException e) {
+ if (e.getMessage().equals(MULTI_ROLE_NOT_SUPPORTED)) {
+ this.authRoles = Collections.singletonList(provider.authenticate(authenticationDataSource));
+ } else {
+ throw e;
+ }
+ }
}
@Override
public String getAuthRole() {
- return authRole;
+ if (authRoles == null || authRoles.isEmpty()) {
+ return null;
+ }
+ return authRoles.get(0);
+ }
+
+ @Override
+ public List<String> getAuthRoles() {
+ return authRoles;
Review comment:
Yes, that was a mistake. It's better to return an empty collection instead of null.
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -66,6 +69,10 @@ default String authenticate(AuthenticationDataSource authData) throws Authentica
throw new AuthenticationException("Not supported");
}
+ default List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
Review comment:
I think this should be done right and we should not push through something without thinking about the design and what role the authorization should play. What you are trying to do can already be accomplished with the current framework (no code changes to any core pieces) by implementing a custom authorization provider. If we are going to make interfaces changes a design should be proposed to the community. I think it's better to think about this and come up with a design to support other potential use cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#issuecomment-848391836
@cckellogg Thanks for the review. And @RobertIndie, Would you please help to handle the comments of @cckellogg?
Regarding split into 2 PRs, From my view, the current PR works fine, because the main change of it is the API change, and other changes aimed to make the auth implementations backward compatible. This PR does not bring in new auth provider. It would be good to keep them in a single PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cckellogg commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r639000237
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,33 @@
public class OneStageAuthenticationState implements AuthenticationState {
private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private List<String> authRoles;
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ try {
+ this.authRoles = provider.authenticate(authenticationDataSource, true);
Review comment:
We should check multi is implemented and then call the appropriated method. This change will cause users who have custom implementations to always throw an exception
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -63,7 +66,11 @@
* if the credentials are not valid
*/
default String authenticate(AuthenticationDataSource authData) throws AuthenticationException {
- throw new AuthenticationException("Not supported");
+ return authenticate(authData, false).get(0);
Review comment:
Keep the AuthenticationException("Not supported").
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,33 @@
public class OneStageAuthenticationState implements AuthenticationState {
private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private List<String> authRoles;
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ try {
+ this.authRoles = provider.authenticate(authenticationDataSource, true);
+ } catch (AuthenticationException e) {
+ if (e.getMessage().equals(MULTI_ROLE_NOT_SUPPORTED)) {
+ this.authRoles = Lists.newArrayList(provider.authenticate(authenticationDataSource));
+ } else {
+ throw e;
+ }
+ }
}
@Override
public String getAuthRole() {
- return authRole;
+ return authRoles.get(0);
Review comment:
What is the list is empty or null?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -346,8 +348,18 @@ private boolean invalidOriginalPrincipal(String originalPrincipal) {
} else {
isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
}
- isAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
- topicName, operation, authRole, authenticationData);
+ isAuthorizedFuture = CompletableFuture.supplyAsync(() -> {
+ for (String authRole : authRoles) {
+ try {
+ if (service.getAuthorizationService().allowTopicOperationAsync(
+ topicName, operation, authRole, authenticationData).get()) {
+ return true;
+ }
+ } catch (InterruptedException | ExecutionException e) {
Review comment:
Will this hide any errors we could be interested in?
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java
##########
@@ -37,6 +39,10 @@
*/
String getAuthRole() throws AuthenticationException;
+ default List<String> getAuthRoles() throws AuthenticationException {
+ return Collections.singletonList(getAuthRole());
Review comment:
What if the getAuthRole returns null or empty string I think this should return an empty list
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#issuecomment-852996661
Thanks @jerrypeng and @cckellogg for your suggestions. I will rethink the implementation of multi roles support.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#issuecomment-850596875
@RobertIndie Can you take a look at @cckellogg 's latest comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r643335535
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,37 @@
public class OneStageAuthenticationState implements AuthenticationState {
private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private List<String> authRoles;
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ if (provider.isSupportMultiRoles()) {
Review comment:
Why are we making this change here? The AuthenticationProviderToken doesn't even use this AuthenticationState Impl. The AuthenticationProviderToken returns TokenAuthenticationState from its `newAuthState()` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cckellogg commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r639732409
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -66,6 +69,10 @@ default String authenticate(AuthenticationDataSource authData) throws Authentica
throw new AuthenticationException("Not supported");
}
+ default List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
Review comment:
Why do the roles need to be returned here?
This is confusing to me `roles` are a set of permissions assigned to a principal. The job of authentication is to identify the user/app and get the principal/app id. For any given authData there should only be one identifier for it. Im my opinion this in the the right way to go about this.
Roles should only be applicable when performing an authorization check. These roles can be accessed in the authorization provider by reading them from the authdata (this contains the token or cert). The default authorization provider could be updated to have some type of adapter to return the `roles` based on the authdata and principal that gets passed in. Then the default authorization provider can make checks against those roles.
I see this is related to (https://github.com/apache/pulsar/pull/10375)
According the the microsoft docs https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens) `roles` are the set of roles that were assigned to the user who is logging in. The `sub` or `oid` can be used to get the identifier of the token
https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cckellogg commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r639707307
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,36 @@
public class OneStageAuthenticationState implements AuthenticationState {
private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private List<String> authRoles;
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ try {
+ this.authRoles = provider.authenticate(authenticationDataSource, true);
+ } catch (AuthenticationException e) {
+ if (e.getMessage().equals(MULTI_ROLE_NOT_SUPPORTED)) {
+ this.authRoles = Lists.newArrayList(provider.authenticate(authenticationDataSource));
Review comment:
Collections.singletonList(provider.authenticate(authenticationDataSource)));
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -19,12 +19,15 @@
package org.apache.pulsar.broker.authentication;
+import com.google.common.collect.Lists;
Review comment:
This is not needed
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -778,9 +794,12 @@ protected void handleConnect(CommandConnect connect) {
// Not find provider named authMethod. Most used for tests.
// In AuthenticationDisabled, it will set authMethod "none".
if (authenticationProvider == null) {
- authRole = getBrokerService().getAuthenticationService().getAnonymousUserRole()
Review comment:
What is the value of this authRole if the `AnonymousUserRole` is not set? is it null. We should avoid any lists where null is an element.
```
final String anonymousUserRole = getBrokerService().getAuthenticationService().getAnonymousUserRole().orElseThrow(() -> new AuthenticationException("No anonymous role, and no authentication provider " + "configured")));
authRoles = StringUtils.isNotEmpty(anonymousUserRole) ? Collections.singletonList(anonymousUserRole) : Collections.emptyList()
```
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -66,6 +69,10 @@ default String authenticate(AuthenticationDataSource authData) throws Authentica
throw new AuthenticationException("Not supported");
}
+ default List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
Review comment:
This is confusing to me `roles` are a set of permissions assigned to a principal. The job of authentication is to identify the user/app and get the principal/app id. For any given authData there should only be one identifier for it.
Roles should only be applicable with performing an authorization check. These roles can be accessed in the authorization provider by reading them from the authdata with will have the token.
I see other changes are references from the (https://github.com/apache/pulsar/pull/10375)
https://docs.microsoft.com/en-us/azure/active-directory/develop/id-tokens.
I think the `AuthenticationProviderToken.java` probably needs to be adjusted first.
The `sub` claim should be used to identify the principal or app id
https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.2
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -346,8 +348,19 @@ private boolean invalidOriginalPrincipal(String originalPrincipal) {
} else {
isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
}
- isAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
- topicName, operation, authRole, authenticationData);
+ isAuthorizedFuture = CompletableFuture.supplyAsync(() -> {
+ for (String authRole : authRoles) {
+ try {
+ if (service.getAuthorizationService().allowTopicOperationAsync(
+ topicName, operation, authRole, authenticationData).get()) {
+ return true;
+ }
+ } catch (InterruptedException | ExecutionException e) {
+ e.printStackTrace();
Review comment:
remove `e.printStackTrace()` and add log.warn or log.info
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -144,14 +145,29 @@ public String getAuthMethodName() {
@Override
public String authenticate(AuthenticationDataSource authData) throws AuthenticationException {
+ List<String> principals = authenticate(authData, false);
+ if (principals == null) {
+ return null;
+ }
+ return principals.get(0);
+ }
+
+ @Override
+ public List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
try {
// Get Token
String token;
token = getToken(authData);
// Parse Token by validating
- String role = getPrincipal(authenticateToken(token));
+ List<String> principals = getPrincipals(authenticateToken(token));
AuthenticationMetrics.authenticateSuccess(getClass().getSimpleName(), getAuthMethodName());
- return role;
+ if(multiRoles){
Review comment:
spacing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r637751818
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -149,7 +150,13 @@ public String authenticate(AuthenticationDataSource authData) throws Authenticat
String token;
token = getToken(authData);
// Parse Token by validating
- String role = getPrincipal(authenticateToken(token));
+ String role;
+ List<String> principals = getPrincipals(authenticateToken(token));
+ if (principals == null) { // Empty list check has been done in getPrincipals.
+ role = null;
+ } else {
+ role = principals.get(0);
Review comment:
Just curious, if we always get the first role in the role list, so what's the motivation for passing a role sets?
If we only want to support get the role from an `Array`, why don't just parse it in the related auth provider?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r642313669
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,36 @@
public class OneStageAuthenticationState implements AuthenticationState {
private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private List<String> authRoles;
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ try {
+ this.authRoles = provider.authenticate(authenticationDataSource, true);
+ } catch (AuthenticationException e) {
+ if (e.getMessage().equals(MULTI_ROLE_NOT_SUPPORTED)) {
+ this.authRoles = Collections.singletonList(provider.authenticate(authenticationDataSource));
+ } else {
+ throw e;
+ }
+ }
}
@Override
public String getAuthRole() {
- return authRole;
+ if (authRoles == null || authRoles.isEmpty()) {
+ return null;
+ }
+ return authRoles.get(0);
+ }
+
+ @Override
+ public List<String> getAuthRoles() {
+ return authRoles;
Review comment:
Maybe returning an empty collection is better than returning a null value?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r642315990
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -66,6 +69,10 @@ default String authenticate(AuthenticationDataSource authData) throws Authentica
throw new AuthenticationException("Not supported");
}
+ default List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
Review comment:
Thank you for your suggestions. This involves a lot of changes. Maybe we can implement this solution in another PR after this PR is merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#issuecomment-848424496
Thanks @cckellogg. I have fixed some comments. PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#issuecomment-846945419
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r639372972
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java
##########
@@ -35,20 +38,33 @@
public class OneStageAuthenticationState implements AuthenticationState {
private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private List<String> authRoles;
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), UTF_8), remoteAddress, sslSession);
- this.authRole = provider.authenticate(authenticationDataSource);
+ try {
+ this.authRoles = provider.authenticate(authenticationDataSource, true);
Review comment:
I think this is OK. When multi roles authenticate is not supported, the original authenticate method will be called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jiazhai commented on pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#issuecomment-851725174
Hi @cckellogg Would you please help review this again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r637806980
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -149,7 +150,13 @@ public String authenticate(AuthenticationDataSource authData) throws Authenticat
String token;
token = getToken(authData);
// Parse Token by validating
- String role = getPrincipal(authenticateToken(token));
+ String role;
+ List<String> principals = getPrincipals(authenticateToken(token));
+ if (principals == null) { // Empty list check has been done in getPrincipals.
+ role = null;
+ } else {
+ role = principals.get(0);
Review comment:
Thanks for your review.
This is for being compatible with the original single role authentication. I just changed it to multi roles authenticate method in https://github.com/apache/pulsar/pull/10685/commits/cf646e47ea6f83dd596fff882c644471698045c1 to make it support both single role and multi roles authentication.
For authorization, we use `AuthenticationState.getAuthRoles()` to get the roles array in [`ServerCnx`](https://github.com/RobertIndie/pulsar/blob/cf646e47ea6f83dd596fff882c644471698045c1/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L630), then use this array to authorize. It doesn't go through this `authenticate` method. ([Detail](https://github.com/RobertIndie/pulsar/blob/cf646e47ea6f83dd596fff882c644471698045c1/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java#L203))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cckellogg commented on a change in pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#discussion_r641165597
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java
##########
@@ -66,6 +69,10 @@ default String authenticate(AuthenticationDataSource authData) throws Authentica
throw new AuthenticationException("Not supported");
}
+ default List<String> authenticate(AuthenticationDataSource authData, boolean multiRoles) throws AuthenticationException {
Review comment:
I think the confusing part is we are using too many terms (role, principal id, app id, etc..) that are being overloaded. In my opinion the job of the authentication provider is to identity the client and this should be a single distinct value (principal id). So I guess the question is what should the function be for the authorization provider? Should it identity the client or provide the client roles? I think it should identity the client because multiple clients could have the same roles and then it can become tricky from the logs and and auditing perspective what client is doing what specific action.
This change forces the broker server context and other classes to iterate over a list in multiple places and compare/sort lists. I think that complicates the code, makes it harder to maintain and reason about. The auth checks in the broker are already complicated and we should try to simply that. We should be trying to abstract and delegate these type of things to the auth providers, especially since this feature can already be done without any changes to the broker code or auth interfaces. In my opinion there are better ways to go about this.
Here are a few options:
1 - have the authorization provider discover the roles it should check for permissions. This requires none or minimal changes to the broker code. The authdata passed in to the authorization provider contains everything that is needed.
2 - Create an AuthInfo class or something similar that is returned by the authentication provider and that will contain a principal id and possible list of roles. However, this will involve changing the authorization interface to accept this new object and will be much larger change. The positive is this makes things much more flexible and extensible in the future.
On a side note there probably needs to be a cleanup of these terms (principal, role, app-id, client-id, etc...) within the code base so were consistent on what they mean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685#issuecomment-851358022
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie closed pull request #10685: [Broker] Add multi roles support for authentication and authorization
Posted by GitBox <gi...@apache.org>.
RobertIndie closed pull request #10685:
URL: https://github.com/apache/pulsar/pull/10685
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org