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

[broker][authentication]Support pass http auth status #14044

Merged
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
77b049f
Support pass http auth status
tuteng Jan 29, 2022
ce3c5d3
Fixed comment and style
tuteng Jan 29, 2022
1badfaa
Add method name for oauth2 auth
tuteng Jan 29, 2022
cbc79e2
Change var type to private from public
tuteng Jan 29, 2022
8924aa6
Fixed style check
tuteng Jan 29, 2022
3cc0a9d
Fixed function interface and client api var
tuteng Feb 3, 2022
3407283
Update auth method name
tuteng Feb 3, 2022
a4d8718
Backward compatible
tuteng Feb 3, 2022
78c7726
Merge branch 'master' into fixed/support-pass-http-auth-status
tuteng Feb 3, 2022
713b1d1
Fixed style
tuteng Feb 3, 2022
6c34f62
Delete whitespace
tuteng Feb 3, 2022
8c2ef67
Reduce length
tuteng Feb 3, 2022
c9f8000
Update test
tuteng Feb 3, 2022
3fb6e3b
Update remote addr
tuteng Feb 3, 2022
97ae44d
Fixed authentication token test
tuteng Feb 4, 2022
197411f
Fixed authentication token test
tuteng Feb 4, 2022
b20a7d5
Update var type, add final field, add comment for authentication method
tuteng Feb 9, 2022
0467c96
Update comment
tuteng Feb 10, 2022
3fc7d57
Fixed test
tuteng Feb 10, 2022
5b08617
Fixed auth test
tuteng Feb 11, 2022
ae5b447
Update comment
tuteng Feb 16, 2022
e4efa86
Revert code
tuteng Feb 16, 2022
bb1ebfe
Remove no used var
tuteng Feb 23, 2022
3764121
Update map headers to unmodifiableMap type
tuteng Feb 23, 2022
673818c
Merge branch 'master' into fixed/support-pass-http-auth-status
tuteng Feb 27, 2022
49c1080
Remove redundant final
tuteng Feb 27, 2022
b1def58
Fixed style
tuteng Feb 27, 2022
485e961
Fixed comment
tuteng Feb 28, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,14 @@ default AuthenticationState newAuthState(AuthData authData,
return new OneStageAuthenticationState(authData, remoteAddress, sslSession, this);
}

/**
* Create an http authentication data State use passed in AuthenticationDataSource.
*/
default AuthenticationState newHttpAuthState(HttpServletRequest request)
throws AuthenticationException {
return new OneStageAuthenticationState(request, this);
}

/**
* Validate the authentication for the given credentials with the specified authentication data.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,37 @@ public AuthenticationState newAuthState(AuthData authData, SocketAddress remoteA
}
}

@Override
public AuthenticationState newHttpAuthState(HttpServletRequest request) throws AuthenticationException {
final List<AuthenticationState> states = new ArrayList<>(providers.size());

AuthenticationException authenticationException = null;
try {
applyAuthProcessor(
providers,
provider -> {
AuthenticationState state = provider.newHttpAuthState(request);
states.add(state);
return state;
}
);
} catch (AuthenticationException ae) {
authenticationException = ae;
}
if (states.isEmpty()) {
log.debug("Failed to initialize a new http auth state from {}",
request.getRemoteHost(), authenticationException);
if (authenticationException != null) {
throw authenticationException;
} else {
throw new AuthenticationException(
"Failed to initialize a new http auth state from " + request.getRemoteHost());
}
} else {
return new AuthenticationListState(states);
}
}

@Override
public boolean authenticateHttpRequest(HttpServletRequest request, HttpServletResponse response) throws Exception {
Boolean authenticated = applyAuthProcessor(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.util.List;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.commons.lang3.StringUtils;
import org.apache.pulsar.broker.ServiceConfiguration;
import org.apache.pulsar.broker.authentication.metrics.AuthenticationMetrics;
Expand Down Expand Up @@ -165,6 +166,11 @@ public AuthenticationState newAuthState(AuthData authData, SocketAddress remoteA
return new TokenAuthenticationState(this, authData, remoteAddress, sslSession);
}

@Override
public AuthenticationState newHttpAuthState(HttpServletRequest request) throws AuthenticationException {
return new TokenAuthenticationHttpState(this, request);
}

public static String getToken(AuthenticationDataSource authData) throws AuthenticationException {
if (authData.hasDataFromCommand()) {
// Authenticate Pulsar binary connection
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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).

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;
}

/**
* Here is an explanation of why the null value is returned.
* pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationState.java#L49
Copy link
Contributor

Choose a reason for hiding this comment

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

Lines change in the future, please refer to a method name, using javadoc syntax

*/
@Override
public AuthData authenticate(AuthData authData) throws AuthenticationException {
return null;
}

@Override
public boolean isComplete() {
// The authentication of tokens is always done in one single stage
return true;
}

@Override
public boolean isExpired() {
return expiration < System.currentTimeMillis();
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.apache.commons.lang3.StringUtils;
import org.apache.pulsar.broker.PulsarServerException;
import org.apache.pulsar.broker.ServiceConfiguration;
import org.apache.pulsar.broker.web.AuthenticationFilter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

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

To be compatible with the previous implementation

AuthenticationState authenticationState = providerToUse.newHttpAuthState(request);
Copy link
Member

Choose a reason for hiding this comment

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

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?

authData = authenticationState.getAuthDataSource();
}
try {
return providerToUse.authenticate(authData);
} catch (AuthenticationException e) {
Expand All @@ -109,7 +114,8 @@ public String authenticateHttpRequest(HttpServletRequest request) throws Authent
} else {
for (AuthenticationProvider provider : providers.values()) {
try {
return provider.authenticate(authData);
AuthenticationState authenticationState = provider.newHttpAuthState(request);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary because providers are a list

return provider.authenticate(authenticationState.getAuthDataSource());
} catch (AuthenticationException e) {
if (LOG.isDebugEnabled()) {
LOG.debug("Authentication failed for provider " + provider.getAuthMethodName() + ": "
Expand Down Expand Up @@ -137,6 +143,10 @@ public String authenticateHttpRequest(HttpServletRequest request) throws Authent
}
}

public String authenticateHttpRequest(HttpServletRequest request) throws AuthenticationException {
return authenticateHttpRequest(request, null);
}

public AuthenticationProvider getAuthenticationProvider(String authMethodName) {
return providers.get(authMethodName);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.net.SocketAddress;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;

/**
Expand All @@ -45,6 +46,12 @@ public OneStageAuthenticationState(AuthData authData,
this.authRole = provider.authenticate(authenticationDataSource);
}

public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
throws AuthenticationException {
this.authenticationDataSource = new AuthenticationDataHttps(request);
this.authRole = provider.authenticate(authenticationDataSource);
}

@Override
public String getAuthRole() {
return authRole;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import javax.servlet.http.HttpServletResponse;
import org.apache.pulsar.broker.authentication.AuthenticationDataHttps;
import org.apache.pulsar.broker.authentication.AuthenticationService;
import org.apache.pulsar.broker.authentication.AuthenticationState;
import org.apache.pulsar.common.sasl.SaslConstants;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -44,6 +45,8 @@ public class AuthenticationFilter implements Filter {

public static final String AuthenticatedRoleAttributeName = AuthenticationFilter.class.getName() + "-role";
public static final String AuthenticatedDataAttributeName = AuthenticationFilter.class.getName() + "-data";
public static final String PULSAR_AUTH_METHOD_NAME = "X-Pulsar-Auth-Method-Name";
Copy link
Member Author

Choose a reason for hiding this comment

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

To avoid introducing the pulsar-client-api package, define it again here



public AuthenticationFilter(AuthenticationService authenticationService) {
this.authenticationService = authenticationService;
Expand Down Expand Up @@ -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,
Copy link
Member Author

Choose a reason for hiding this comment

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

Backward compatible

new AuthenticationDataHttps((HttpServletRequest) request));
}
String role;
if (authenticationState != null) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

role = authenticationService.authenticateHttpRequest(
(HttpServletRequest) request, authenticationState.getAuthDataSource());
} else {
role = authenticationService.authenticateHttpRequest((HttpServletRequest) request);
}
request.setAttribute(AuthenticatedRoleAttributeName, role);
request.setAttribute(AuthenticatedDataAttributeName,
new AuthenticationDataHttps((HttpServletRequest) request));

if (LOG.isDebugEnabled()) {
LOG.debug("[{}] Authenticated HTTP request with role {}", request.getRemoteAddr(), role);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
package org.apache.pulsar.broker.authentication;

import static java.nio.charset.StandardCharsets.UTF_8;
import javax.servlet.http.HttpServletRequest;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertTrue;
Expand Down Expand Up @@ -165,6 +168,14 @@ private AuthenticationState newAuthState(String token, String expectedSubject) t
return authState;
}

private AuthenticationState newHttpAuthState(HttpServletRequest request, String expectedSubject) throws Exception {
AuthenticationState authState = authProvider.newHttpAuthState(request);
assertEquals(authState.getAuthRole(), expectedSubject);
assertTrue(authState.isComplete());
assertFalse(authState.isExpired());
return authState;
}

private void verifyAuthStateExpired(AuthenticationState authState, String expectedSubject)
throws Exception {
assertEquals(authState.getAuthRole(), expectedSubject);
Expand All @@ -188,4 +199,38 @@ public void testNewAuthState() throws Exception {

}

@Test
public void testNewHttpAuthState() throws Exception {
HttpServletRequest requestAA = mock(HttpServletRequest.class);
when(requestAA.getRemoteAddr()).thenReturn("127.0.0.1");
when(requestAA.getRemotePort()).thenReturn(8080);
when(requestAA.getHeader("Authorization")).thenReturn("Bearer " + expiringTokenAA);
AuthenticationState authStateAA = newHttpAuthState(requestAA, SUBJECT_A);

HttpServletRequest requestAB = mock(HttpServletRequest.class);
when(requestAB.getRemoteAddr()).thenReturn("127.0.0.1");
when(requestAB.getRemotePort()).thenReturn(8080);
when(requestAB.getHeader("Authorization")).thenReturn("Bearer " + expiringTokenAB);
AuthenticationState authStateAB = newHttpAuthState(requestAB, SUBJECT_B);

HttpServletRequest requestBA = mock(HttpServletRequest.class);
when(requestBA.getRemoteAddr()).thenReturn("127.0.0.1");
when(requestBA.getRemotePort()).thenReturn(8080);
when(requestBA.getHeader("Authorization")).thenReturn("Bearer " + expiringTokenBA);
AuthenticationState authStateBA = newHttpAuthState(requestBA, SUBJECT_A);

HttpServletRequest requestBB = mock(HttpServletRequest.class);
when(requestBB.getRemoteAddr()).thenReturn("127.0.0.1");
when(requestBB.getRemotePort()).thenReturn(8080);
when(requestBB.getHeader("Authorization")).thenReturn("Bearer " + expiringTokenBB);
AuthenticationState authStateBB = newHttpAuthState(requestBB, SUBJECT_B);

Thread.sleep(TimeUnit.SECONDS.toMillis(6));

verifyAuthStateExpired(authStateAA, SUBJECT_A);
verifyAuthStateExpired(authStateAB, SUBJECT_B);
verifyAuthStateExpired(authStateBA, SUBJECT_A);
verifyAuthStateExpired(authStateBB, SUBJECT_B);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
import org.apache.commons.lang3.tuple.Pair;
import org.apache.pulsar.broker.PulsarService;
import org.apache.pulsar.broker.ServiceConfiguration;
import org.apache.pulsar.broker.authentication.AuthenticationDataHttps;
import org.apache.pulsar.broker.authentication.AuthenticationDataSource;
import org.apache.pulsar.broker.authorization.AuthorizationService;
import org.apache.pulsar.broker.namespace.LookupOptions;
Expand Down Expand Up @@ -136,8 +135,8 @@ public String originalPrincipal() {
return httpRequest.getHeader(ORIGINAL_PRINCIPAL_HEADER);
}

public AuthenticationDataHttps clientAuthData() {
return (AuthenticationDataHttps) httpRequest.getAttribute(AuthenticationFilter.AuthenticatedDataAttributeName);
public AuthenticationDataSource clientAuthData() {
Copy link
Member Author

Choose a reason for hiding this comment

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

AuthenticationDataHttps is the return signature of the error, use AuthenticationDataSource instead of it

return (AuthenticationDataSource) httpRequest.getAttribute(AuthenticationFilter.AuthenticatedDataAttributeName);
}

public boolean isRequestHttps() {
Expand Down Expand Up @@ -1149,7 +1148,8 @@ && pulsar().getBrokerService().isAuthorizationEnabled()) {
return FutureUtil.failedFuture(
new RestException(Status.UNAUTHORIZED, "Need to authenticate to perform the request"));
}
AuthenticationDataHttps authData = clientAuthData();

AuthenticationDataSource authData = clientAuthData();
Copy link
Member Author

Choose a reason for hiding this comment

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

AuthenticationDataHttps is the return value of the error, use AuthenticationDataSource instead of it

authData.setSubscription(subscription);
return pulsar().getBrokerService().getAuthorizationService()
.allowTopicOperationAsync(topicName, operation, originalPrincipal(), clientAppId(), authData)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
@InterfaceAudience.LimitedPrivate
@InterfaceStability.Stable
public interface AuthenticationDataProvider extends Serializable {

String PULSAR_AUTH_METHOD_NAME = "X-Pulsar-Auth-Method-Name";
/*
* TLS
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.apache.pulsar.client.api.PulsarClientException;

public class AuthenticationBasic implements Authentication, EncodedAuthenticationParameterSupport {
static final String AUTH_METHOD_NAME = "basic";
private String userId;
private String password;

Expand All @@ -39,7 +40,7 @@ public void close() throws IOException {

@Override
public String getAuthMethodName() {
return "basic";
return AUTH_METHOD_NAME;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public boolean hasDataForHttp() {
public Set<Map.Entry<String, String>> getHttpHeaders() {
Map<String, String> headers = new HashMap<>();
headers.put(HTTP_HEADER_NAME, httpAuthToken);
headers.put(PULSAR_AUTH_METHOD_NAME, AuthenticationBasic.AUTH_METHOD_NAME);
return headers.entrySet();
}

Expand Down
Loading