michaeljmarshall commented on a change in pull request #14044:
URL: https://github.com/apache/pulsar/pull/14044#discussion_r803299066
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -363,4 +369,63 @@ public boolean isExpired() {
return expiration < System.currentTimeMillis();
}
}
+
+ private static final class TokenAuthenticationHttpState implements
AuthenticationState {
+
+ private final AuthenticationProviderToken provider;
+ private final AuthenticationDataSource authenticationDataSource;
+ private final Jwt<?, Claims> jwt;
+ private final long expiration;
+
+ TokenAuthenticationHttpState(AuthenticationProviderToken provider,
HttpServletRequest request)
Review comment:
Nit: I think we could avoid creating this class by adding a new
constructor to the `TokenAuthenticationState` class. As far as I can tell, the
only differences come in the constructor (and in the `authenticate` method, but
as @tuteng pointed out, that method can return `null` by capturing the relevant
fields in the constructor).
##########
File path:
pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/service/api/Functions.java
##########
@@ -42,7 +42,29 @@ void registerFunction(final String tenant,
final String functionPkgUrl,
final FunctionConfig functionConfig,
final String clientRole,
- AuthenticationDataHttps
clientAuthenticationDataHttps);
+ AuthenticationDataSource
clientAuthenticationDataHttps);
+
+ @Deprecated
Review comment:
On these deprecated methods, can we add a comment indicating which
method should be used instead?
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/AuthenticationFilter.java
##########
@@ -71,10 +74,25 @@ public void doFilter(ServletRequest request,
ServletResponse response, FilterCha
if (!isSaslRequest(httpRequest)) {
// not sasl type, return role directly.
- String role =
authenticationService.authenticateHttpRequest((HttpServletRequest) request);
+ String authMethodName =
httpRequest.getHeader(PULSAR_AUTH_METHOD_NAME);
+ AuthenticationState authenticationState = null;
+ if (authMethodName != null &&
authenticationService.getAuthenticationProvider(authMethodName) != null) {
+ authenticationState = authenticationService
+
.getAuthenticationProvider(authMethodName).newHttpAuthState(httpRequest);
+ request.setAttribute(AuthenticatedDataAttributeName,
authenticationState.getAuthDataSource());
+ } else {
+ request.setAttribute(AuthenticatedDataAttributeName,
+ new AuthenticationDataHttps((HttpServletRequest)
request));
+ }
+ String role;
+ if (authenticationState != null) {
Review comment:
Nit: I think we could consolidate this conditional and the one above
starting on line 79. That could help make it clear that the `else` block is a
legacy use case to handle calls that are missing the `PULSAR_AUTH_METHOD_NAME`
header. Do you agree?
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -363,4 +369,59 @@ public boolean isExpired() {
return expiration < System.currentTimeMillis();
}
}
+
+ private static final class TokenAuthenticationHttpState implements
AuthenticationState {
+
+ private final AuthenticationProviderToken provider;
+ private AuthenticationDataSource authenticationDataSource;
+ private Jwt<?, Claims> jwt;
+ private long expiration;
+
+ TokenAuthenticationHttpState(AuthenticationProviderToken provider,
HttpServletRequest request)
+ throws AuthenticationException {
+ this.provider = provider;
+ String httpHeaderValue = request.getHeader(HTTP_HEADER_NAME);
+ if (httpHeaderValue == null ||
!httpHeaderValue.startsWith(HTTP_HEADER_VALUE_PREFIX)) {
+ throw new AuthenticationException("Invalid HTTP Authorization
header");
+ }
+
+ // Remove prefix
+ String token =
httpHeaderValue.substring(HTTP_HEADER_VALUE_PREFIX.length());
+ this.jwt = provider.authenticateToken(token);
+ this.authenticationDataSource = new
AuthenticationDataHttps(request);
+ if (jwt.getBody().getExpiration() != null) {
+ this.expiration = jwt.getBody().getExpiration().getTime();
+ } else {
+ // Disable expiration
+ this.expiration = Long.MAX_VALUE;
+ }
+ }
+
+ @Override
+ public String getAuthRole() throws AuthenticationException {
+ return provider.getPrincipal(jwt);
+ }
+
+ @Override
+ public AuthenticationDataSource getAuthDataSource() {
+ return authenticationDataSource;
+ }
+
+ @Override
+ public AuthData authenticate(AuthData authData) throws
AuthenticationException {
+ return null;
Review comment:
This is an interesting nuance. Currently, the `TokenAuthenticationState`
class authenticates every token twice: once on class initialization, and again
when `authenticate` is called. I think it is good this way.
##########
File path:
pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationDataTls.java
##########
@@ -87,6 +90,11 @@ public boolean hasDataForTls() {
return true;
}
+ @Override
+ public Set<Map.Entry<String, String>> getHttpHeaders() {
+ return Collections.singletonMap(PULSAR_AUTH_METHOD_NAME,
AuthenticationTls.AUTH_METHOD_NAME).entrySet();
Review comment:
Similar question. Why not create this map just once?
##########
File path:
pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationDataOAuth2.java
##########
@@ -44,7 +42,10 @@ public boolean hasDataForHttp() {
@Override
public Set<Map.Entry<String, String>> getHttpHeaders() {
- return this.headers;
Review comment:
What is the purpose of replacing the stored headers map with a computed
map? The `accessToken` is `final` in this class, so I wouldn't think we need to
compute the map each time.
##########
File path:
pulsar-websocket/src/main/java/org/apache/pulsar/websocket/admin/WebSocketWebResource.java
##########
@@ -81,9 +83,16 @@ public String clientAppId() {
return clientId;
}
- public AuthenticationDataHttps authData() {
+ public AuthenticationDataSource authData() throws AuthenticationException {
Review comment:
Instead of updating this method, it might make more sense to update the
`clientAppId()` method in this class so that it sets the `authData`. (We'd need
to rename it to indicate that it is authenticating the request, but I think
that's a reasonable change, given that the method is already doing
authentication.) That method is already completing authentication and calls
`service().getAuthenticationService().authenticateHttpRequest(httpRequest)`
currently. The current design will lead to double authentication.
##########
File path:
pulsar-client-api/src/main/java/org/apache/pulsar/client/api/AuthenticationDataProvider.java
##########
@@ -36,6 +36,8 @@
@InterfaceAudience.LimitedPrivate
@InterfaceStability.Stable
public interface AuthenticationDataProvider extends Serializable {
+
+ String PULSAR_AUTH_METHOD_NAME = "X-Pulsar-Auth-Method-Name";
Review comment:
Thanks.
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java
##########
@@ -84,17 +85,21 @@ public AuthenticationService(ServiceConfiguration conf)
throws PulsarServerExcep
}
}
- public String authenticateHttpRequest(HttpServletRequest request) throws
AuthenticationException {
+ public String authenticateHttpRequest(HttpServletRequest request,
AuthenticationDataSource authData)
+ throws AuthenticationException {
AuthenticationException authenticationException = null;
- AuthenticationDataSource authData = new
AuthenticationDataHttps(request);
- String authMethodName = request.getHeader("X-Pulsar-Auth-Method-Name");
+ String authMethodName =
request.getHeader(AuthenticationFilter.PULSAR_AUTH_METHOD_NAME);
if (authMethodName != null) {
AuthenticationProvider providerToUse =
providers.get(authMethodName);
if (providerToUse == null) {
throw new AuthenticationException(
String.format("Unsupported authentication method:
[%s].", authMethodName));
}
+ if (authData == null) {
+ AuthenticationState authenticationState =
providerToUse.newHttpAuthState(request);
Review comment:
`newHttpAuthState` throws `AuthenticationException`, especially for a
class like the `TokenAuthenticationHttpState` because it calls
`provider.authenticate` in the constructor. We should wrap it in a `try` block.
Also, wouldn't it work to use the `getAuthRole()` method on the
`authenticationState` to bypass a second authentication call?
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationService.java
##########
@@ -137,6 +143,10 @@ public String authenticateHttpRequest(HttpServletRequest
request) throws Authent
}
}
+ public String authenticateHttpRequest(HttpServletRequest request) throws
AuthenticationException {
Review comment:
Is this still used? It looks like it is used in
`AbstractWebSocketHandler`. Was it intentional to skip updating that reference?
Should we annotate this with deprecated?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
