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 2023/01/20 06:26:49 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #19295: [feat][broker] OneStageAuth State: move authn out of constructor
michaeljmarshall opened a new pull request, #19295:
URL: https://github.com/apache/pulsar/pull/19295
PIP: #12105
### Motivation
In order to implement PIP 97, we must replace `authenticate` calls with `authenticateAsync`. This PR makes those changes for `OneStageAuthenticationState`. It also moves the call to authenticate the `AuthData` out of the constructor and into the `authenticateAsync` method.
One assumption I make in this PR, is that there will not be concurrent calls to the same `AuthenticationState#authenticateAsync` method. That seems like a reasonable assumption because Pulsar uses this state object in the netty eventloop handlers, but please let me know if you disagree.
### Modifications
* Move the `authenticate` call out of the constructor so that authentication is only done on the first pass through the `authenticateAsync` method.
* Make `authenticate` backwards compatible for any plugins that are using it.
### Verifying this change
New tests are added to verify the new and the old code.
### Does this pull request potentially affect one of the following parts:
This could break 3rd party plugins in the broker if they were relying on authentication to happen in the constructor. In order to make those implementations fail fast, this PR includes a change to throw an exception when the `getAuthRole` is called without first calling `authenticateAsync` or `authenticate`. That makes these changes semi-backwards compatible.
### Documentation
- [x] `doc-not-needed`
### Matching PR in forked repository
PR in forked repository: https://github.com/michaeljmarshall/pulsar/pull/17
--
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
[GitHub] [pulsar] michaeljmarshall commented on pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#issuecomment-1447189101
I forgot why I tagged this commit for a commit to all of the release branches. Those commits reference the fact that I added a call to authenticate the `authData`. I did not change the constructors in those 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#issuecomment-1398849932
@lhotari - yes, that proxy test is failing because of my changes. Pushing up a fix now.
Note that the fix includes a new call to `authenticate` in the `ServerCnx`. I think this is reasonable, but it does slightly change the scope of 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1141256918
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify {@link AuthData}
+ */
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);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before calling getAuthRole");
+ }
Review Comment:
> but the issue is that custom AuthenticationProvider implementations that override the current newAuthState method likely don't want to use the OneStageAuthenticationState, and that would lead to unexpected behavior.
Could you show an example of a possible unexpected behavior? IMO, the existing authentication provider only overrides the previous `newAuthState` method still has the previous behavior and is not affected by the new `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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1128937338
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify {@link AuthData}
+ */
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);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before calling getAuthRole");
+ }
Review Comment:
Sorry I missed the fact that you have removed these `cherry-picked` labels.
Regarding this change, I think you should add another constructor to `OneStageAuthenticationState`. Currently, the first argument `authData` is actually not used. Maybe you didn't remove it because it's public. (However, you removed the exception signature) But the semantic changed and it looked weird now.
To keep the compatibility, it's better to mark the constructor as deprecated first and keep it the semantic as "authenticate synchronously". To use the asynchronous semantic, you should add another constructor to do that.
```java
public OneStageAuthenticationState(SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) {
this.provider = provider;
this.remoteAddress = remoteAddress;
this.sslSession = sslSession;
}
@Deprecated
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this(remoteAddress, sslSession, provider);
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), StandardCharsets.UTF_8), remoteAddress, sslSession);
this.authRole = provider.authenticate(this.authenticationDataSource);
}
```
> KoP relied on this implementation detail
I'd rather use "semantics" instead of the "implementation detail". Take `Producer#send` as example:
- Semantics: the send is synchronous
- Implementation details: the exception message representation
If you made a change that makes `send` asynchronous, it should be treated as a breaking change. But if you just modified the exception message, for example, adding a common prefix to the exception message, it would be okay.
--
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
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1142833276
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify {@link AuthData}
+ */
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);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before calling getAuthRole");
+ }
Review Comment:
I got it. The key point is that Pulsar will prefer the new overload but the existing implementations might still implement the old overload. It seems that there is no better way to solve it perfectly.
--
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
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1084406263
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -61,13 +80,47 @@ public AuthenticationDataSource getAuthDataSource() {
return authenticationDataSource;
}
+ /**
+ * Warning: this method is not intended to be called concurrently.
+ */
+ @Override
+ public CompletableFuture<AuthData> authenticateAsync(AuthData authData) {
+ if (authRole != null) {
+ // Authentication is already completed
+ return CompletableFuture.completedFuture(null);
+ }
+ this.authenticationDataSource = new AuthenticationDataCommand(
Review Comment:
> single-stage authentication doesn't refresh authentication data
Yes, that is what I mean. However, I don't agree that it means we need to create a new `AuthenticationState`.
My understanding of the the protocol is that the `AuthenticationState` method `isExpired` returns `true`, Pulsar will then call `refreshAuthentication()` to get the bytes to send to the client. The client responds with `AuthData`, and Pulsar passes the response to `authenticate(AuthData)`. Since `isExpired` is always false for `OneStateAuthenticationState`, there is no expectation of getting an `AuthChallenge` back. The single stage authentication is truly one stage.
--
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
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1127079783
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify {@link AuthData}
+ */
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);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before calling getAuthRole");
+ }
Review Comment:
@BewareMyPower - I agree that we shouldn't cherry pick this line. I don't think I did though, taking a quick look at one of the linked commits. I primarily cherry picked the code that calls `authenticate` on the original auth data. Are you seeing this error in the kop logs?
--
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
[GitHub] [pulsar] nodece commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1083305247
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -61,13 +80,47 @@ public AuthenticationDataSource getAuthDataSource() {
return authenticationDataSource;
}
+ /**
+ * Warning: this method is not intended to be called concurrently.
+ */
+ @Override
+ public CompletableFuture<AuthData> authenticateAsync(AuthData authData) {
+ if (authRole != null) {
+ // Authentication is already completed
+ return CompletableFuture.completedFuture(null);
+ }
+ this.authenticationDataSource = new AuthenticationDataCommand(
Review Comment:
Your means that a single-stage authentication doesn't refresh authentication data, if so, when having `AuthChallenge`, we must new an `AuthenticationState`.
--
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
[GitHub] [pulsar] lhotari merged pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #19295:
URL: https://github.com/apache/pulsar/pull/19295
--
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
[GitHub] [pulsar] michaeljmarshall commented on pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#issuecomment-1400898865
Looks like the error was transient. I am not sure what would have led to that error.
@lhotari - 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#issuecomment-1400850207
Looks like `PersistentTopicStreamingDispatcherTest` is failing in CI, but passes on my machine. Looking into that.
```
Error: Tests run: 45, Failures: 1, Errors: 0, Skipped: 7, Time elapsed: 83.573 s <<< FAILURE! - in org.apache.pulsar.broker.service.persistent.PersistentTopicStreamingDispatcherTest
Error: setup(org.apache.pulsar.broker.service.persistent.PersistentTopicStreamingDispatcherTest) Time elapsed: 0.238 s <<< FAILURE!
org.mockito.exceptions.base.MockitoException: Unable to create mock instance of type 'ServerCnx'
at org.apache.pulsar.broker.BrokerTestUtil.spyWithClassAndConstructorArgsRecordingInvocations(BrokerTestUtil.java:61)
at org.apache.pulsar.broker.service.PersistentTopicTest.setup(PersistentTopicTest.java:233)
at org.apache.pulsar.broker.service.persistent.PersistentTopicStreamingDispatcherTest.setup(PersistentTopicStreamingDispatcherTest.java:34)
at jdk.internal.reflect.GeneratedMethodAccessor406.invoke(Unknown Source)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:568)
at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:139)
at org.testng.internal.invokers.MethodInvocationHelper.invokeMethodConsideringTimeout(MethodInvocationHelper.java:69)
at org.testng.internal.invokers.ConfigInvoker.invokeConfigurationMethod(ConfigInvoker.java:361)
at org.testng.internal.invokers.ConfigInvoker.invokeConfigurations(ConfigInvoker.java:296)
at org.testng.internal.invokers.TestInvoker.runConfigMethods(TestInvoker.java:823)
at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:590)
at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:221)
at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:969)
at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:194)
at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:148)
at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at org.testng.TestRunner.privateRun(TestRunner.java:829)
at org.testng.TestRunner.run(TestRunner.java:602)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:437)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:431)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:391)
at org.testng.SuiteRunner.run(SuiteRunner.java:330)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:95)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1256)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1176)
at org.testng.TestNG.runSuites(TestNG.java:1099)
at org.testng.TestNG.run(TestNG.java:1067)
at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:135)
at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeSingleClass(TestNGDirectoryTestSuite.java:112)
at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeLazy(TestNGDirectoryTestSuite.java:123)
at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:90)
at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:146)
at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
Caused by: org.mockito.creation.instance.InstantiationException:
Unable to create instance of 'ServerCnx'.
Please ensure the target class has a constructor that matches these argument types: [org.apache.pulsar.broker.PulsarService] and executes cleanly.
... 40 more
Caused by: java.lang.reflect.InvocationTargetException
at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:198)
at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:161)
at org.mockito.internal.util.reflection.ModuleMemberAccessor.newInstance(ModuleMemberAccessor.java:42)
at org.mockito.internal.creation.instance.ConstructorInstantiator.invokeConstructor(ConstructorInstantiator.java:70)
at org.mockito.internal.creation.instance.ConstructorInstantiator.withParams(ConstructorInstantiator.java:53)
at org.mockito.internal.creation.instance.ConstructorInstantiator.newInstance(ConstructorInstantiator.java:39)
at org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.doCreateMock(InlineDelegateByteBuddyMockMaker.java:360)
at org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.createMock(InlineDelegateByteBuddyMockMaker.java:330)
at org.mockito.internal.creation.bytebuddy.InlineByteBuddyMockMaker.createMock(InlineByteBuddyMockMaker.java:58)
at org.mockito.internal.util.MockUtil.createMock(MockUtil.java:53)
at org.mockito.internal.MockitoCore.mock(MockitoCore.java:84)
at org.mockito.Mockito.mock(Mockito.java:1964)
... 40 more
Caused by: java.lang.ClassCastException: class org.apache.pulsar.broker.service.BrokerService cannot be cast to class org.apache.pulsar.broker.resources.PulsarResources (org.apache.pulsar.broker.service.BrokerService and org.apache.pulsar.broker.resources.PulsarResources are in unnamed module of loader 'app')
at org.apache.pulsar.broker.PulsarService.getPulsarResources(PulsarService.java:267)
at org.apache.pulsar.broker.service.TopicListService.<init>(TopicListService.java:103)
at org.apache.pulsar.broker.service.ServerCnx.<init>(ServerCnx.java:297)
at org.apache.pulsar.broker.service.ServerCnx.<init>(ServerCnx.java:255)
at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
at org.mockito.internal.util.reflection.InstrumentationMemberAccessor$Dispatcher$ByteBuddy$zWEvVzeS.invokeWithArguments(Unknown Source)
at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.lambda$newInstance$0(InstrumentationMemberAccessor.java:191)
at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:188)
... 51 more
```
--
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
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1141038277
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify {@link AuthData}
+ */
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);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before calling getAuthRole");
+ }
Review Comment:
@BewareMyPower - I finally got a chance to come back to this, and now I remember that I did try to use a similar solution that you proposed while writing this PR, but I found it didn't actually work as you propose. There are a few details to show the issue:
First, the `OneStageAuthenticationState` is not used directly by the broker or by plugins. Instead, the broker and plugins use `AuthenticationProvider#newAuthState` to create the `AuthenticationState` object. Here is the code:
https://github.com/apache/pulsar/blob/fb7f14ceb04d612e456b2e5a834385ae3a97f68f/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java#L96-L104
As such, changing the constructor in the way you propose is unlikely to help users that built their own extensions. For example, KoP does the following:
https://github.com/streamnative/kop/blob/c9f66e4d24608dea6c7f4badde1d288b7dc1f74d/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/SchemaRegistryManager.java#L105-L106
Users must rely on the `AuthenticationProvider#newAuthState` in order to make the `AuthenticationProvider` pluggable.
When I noticed this detail while writing this PR, I thought about adding a new method to the `AuthenticationProvider` that would not take the `AuthData` object, in the same way that you proposed for the new `OneStageAuthenticationState` constructor. However, the main issue is how to default that method in the interface. We could use the following definition:
```java
default AuthenticationState newAuthState(SocketAddress remoteAddress, SSLSession sslSession)
throws AuthenticationException {
return new OneStageAuthenticationState(remoteAddress, sslSession, this);
}
```
This would be analogous to the current `newAuthState` method, but the issue is that custom `AuthenticationProvider` implementations that override the current `newAuthState` method likely don't want to use the `OneStageAuthenticationState`, and that would lead to unexpected behavior.
Another option could be:
```java
default AuthenticationState newAuthState(SocketAddress remoteAddress, SSLSession sslSession)
throws AuthenticationException {
return newAuthState(null, remoteAddress, sslSession);
}
```
However, that would require breaking the current implementation in the way you're concerned about. (For what it's worth, this option might be the best long term solution for cleaning up the interface if we want to remove the `authData` that is passed. The main point is that it doesn't take care of your concern of not breaking user code.)
Another option:
```java
AuthenticationState newAuthState(SocketAddress remoteAddress, SSLSession sslSession);
```
This unimplemented option has two problems. First, it will break user code if they do not implement the method. Since this whole exercise is an effort to try to prevent users from needing to change any code, that isn't a good option. Second, it removes the `OneStageAuthenticationState` default already present in the interface, which seems problematic.
In my view, the primary challenge is that the `AuthenticationProvider` interface has a brittle default behavior.
Given the above, do you have any ideas on how to make this better for users building their own broker extensions?
--
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
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1142515206
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify {@link AuthData}
+ */
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);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before calling getAuthRole");
+ }
Review Comment:
> Could you show an example of a possible unexpected behavior? IMO, the existing authentication provider only overrides the previous `newAuthState` method still has the previous behavior and is not affected by the new `newAuthState` method.
The challenge comes from the fact that Apache Pulsar will transition to using the new method to build the state object and if that method is not overridden by the custom `AuthenticationProvider` implementation, the `OneStageAuthenticationState` will be used. That will lead to unexpected behavior.
A good example is the `AuthenticationProviderToken` class. That one would "work" with the `OneStageAuthenticationState` with the key difference being the `isExpired` value from `OneStageAuthenticationState` always returns `false` while the `TokenAuthenticationState` returns `true` when the token's `exp` claim indicates the token has expired.
--
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
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1128937338
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify {@link AuthData}
+ */
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);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before calling getAuthRole");
+ }
Review Comment:
Sorry I missed the fact that you have removed these `cherry-picked` labels.
Regarding this change, I think you should add another constructor to `OneStageAuthenticationState`. Currently, the first argument `authData` is actually not used. Maybe you didn't remove it because it's public. (However, you removed the exception signature) But the semantic changed and it looked weird now.
To keep the compatibility, it's better to mark the constructor as deprecated first and keep it the semantic as "authenticate synchronously". To use the asynchronous semantic, you should add another constructor to do that.
```java
public OneStageAuthenticationState(SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) {
this.provider = provider;
this.remoteAddress = remoteAddress;
this.sslSession = sslSession;
}
@Deprecated
public OneStageAuthenticationState(AuthData authData,
SocketAddress remoteAddress,
SSLSession sslSession,
AuthenticationProvider provider) throws AuthenticationException {
this(remoteAddress, sslSession, provider);
this.authenticationDataSource = new AuthenticationDataCommand(
new String(authData.getBytes(), StandardCharsets.UTF_8), remoteAddress, sslSession);
this.authRole = provider.authenticate(this.authenticationDataSource);
}
```
> KoP relied on this implementation detail
I'd rather use "semantics" instead of the "implementation detail". Take `Producer#send` as example:
- Semantics: the send is synchronous
- Implementation details: the exception message representation, etc.
If you made a change that makes `send` asynchronous, it should be treated as a breaking change. But if you just modified the exception message, for example, adding a common prefix to the exception message, it would be okay.
--
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
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1127138450
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify {@link AuthData}
+ */
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);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before calling getAuthRole");
+ }
Review Comment:
I understand now. That KoP test references pulsar 3.0.0, so it must be from a recent build of master. First, I'll note that I didn't cherry pick the behavior change to old release branches. I cherry-picked the extra authentication call for original auth data, which was code introduced in this PR. Second, I think this change is necessary for enabling PIP 97. I first documented the contract here: https://github.com/apache/pulsar/pull/19283. The primary issue is that creating the `AuthState` should not trigger authenticating the `AuthData`. KoP relied on this implementation detail, and that is why the tests failed. Given that the only impacted parties are plugin developers, I think a PIP, documented release notes, a major version bump, and a fail fast behavior (the object throws an exception), make this change acceptable for 3.0.0. Let me know what you think, thanks.
--
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
[GitHub] [pulsar] nodece commented on pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#issuecomment-1398022448
/pulsarbot rerun-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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1082207478
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -61,13 +80,47 @@ public AuthenticationDataSource getAuthDataSource() {
return authenticationDataSource;
}
+ /**
+ * Warning: this method is not intended to be called concurrently.
+ */
+ @Override
+ public CompletableFuture<AuthData> authenticateAsync(AuthData authData) {
+ if (authRole != null) {
+ // Authentication is already completed
+ return CompletableFuture.completedFuture(null);
+ }
+ this.authenticationDataSource = new AuthenticationDataCommand(
Review Comment:
I have one question:
When having `AuthChallenge`, do we need to renew `AuthenticationState`?
--
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
[GitHub] [pulsar] lhotari commented on pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#issuecomment-1400938065
> Looks like the error was transient. I am not sure what would have led to that error.
>
> @lhotari - PTAL
That's a known issue, https://github.com/apache/pulsar/issues/13620#issuecomment-1396815606 . Mockito is not thread safe.
--
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
[GitHub] [pulsar] nodece commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1082207478
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -61,13 +80,47 @@ public AuthenticationDataSource getAuthDataSource() {
return authenticationDataSource;
}
+ /**
+ * Warning: this method is not intended to be called concurrently.
+ */
+ @Override
+ public CompletableFuture<AuthData> authenticateAsync(AuthData authData) {
+ if (authRole != null) {
+ // Authentication is already completed
+ return CompletableFuture.completedFuture(null);
+ }
+ this.authenticationDataSource = new AuthenticationDataCommand(
Review Comment:
I have one question:
When having `AuthChallenge`, do we need to renew `AuthenticationState`?
It looks like so.
--
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
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1082976985
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -61,13 +80,47 @@ public AuthenticationDataSource getAuthDataSource() {
return authenticationDataSource;
}
+ /**
+ * Warning: this method is not intended to be called concurrently.
+ */
+ @Override
+ public CompletableFuture<AuthData> authenticateAsync(AuthData authData) {
+ if (authRole != null) {
+ // Authentication is already completed
+ return CompletableFuture.completedFuture(null);
+ }
+ this.authenticationDataSource = new AuthenticationDataCommand(
Review Comment:
Since this is a class for single stage authentication, there are no `AuthChallenges`. The primary reason to set this here instead of the constructor is that we set it with the `authData` that we authenticate. Note that this class always has `isExpired` return `false`, which means there are never any `AuthChallenges` generated.
If this were a multi-stage auth, this solution would not work.
--
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
[GitHub] [pulsar] nodece commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1086218698
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -61,13 +80,47 @@ public AuthenticationDataSource getAuthDataSource() {
return authenticationDataSource;
}
+ /**
+ * Warning: this method is not intended to be called concurrently.
+ */
+ @Override
+ public CompletableFuture<AuthData> authenticateAsync(AuthData authData) {
+ if (authRole != null) {
+ // Authentication is already completed
+ return CompletableFuture.completedFuture(null);
+ }
+ this.authenticationDataSource = new AuthenticationDataCommand(
Review Comment:
Sounds good, I see.
--
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
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1126503217
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify {@link AuthData}
+ */
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);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before calling getAuthRole");
+ }
Review Comment:
This PR looks like a breaking change. In KoP, this method is used to get the role from the token. After upgrading the dependency, many authorization related tests failed. See https://github.com/streamnative/kop/actions/runs/4343910383/jobs/7586547352
Not sure if it should be reverted in master branch, but it should not be cherry-picked to release branches. @michaeljmarshall
/cc @Demogorgon314 @nodece @coderzc
--
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
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1130222459
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify {@link AuthData}
+ */
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);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before calling getAuthRole");
+ }
Review Comment:
@BewareMyPower - thanks for the context and the suggestion.
> I think you should add another constructor to `OneStageAuthenticationState`.
Thanks for the suggestion, I agree this is the best way to move forward without breaking any other applications when 3.0.0 is released. I also agree that it makes the API clearer because the unused `AuthData` won't be passed in. I shoul be able to get a PR submitted within 24 hours or so.
> (However, you removed the exception signature)
I didn't realize this would cause an issue. I'll add it back when I fix the constructor. Out of curiosity, what is the consequence of breaking source compatibility by removing the exception from the method signature?
--
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
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #19295: [feat][broker] OneStageAuth State: move authn out of constructor
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #19295:
URL: https://github.com/apache/pulsar/pull/19295#discussion_r1130341227
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/OneStageAuthenticationState.java:
##########
@@ -20,39 +20,60 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.net.SocketAddress;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
import javax.naming.AuthenticationException;
import javax.net.ssl.SSLSession;
import javax.servlet.http.HttpServletRequest;
import org.apache.pulsar.common.api.AuthData;
/**
- * Interface for authentication state.
- *
- * It tell broker whether the authentication is completed or not,
- * if completed, what is the AuthRole is.
+ * A class to track single stage authentication. This class assumes that:
+ * 1. {@link #authenticateAsync(AuthData)} is called once and when the {@link CompletableFuture} completes,
+ * authentication is complete.
+ * 2. Authentication does not expire, so {@link #isExpired()} always returns false.
+ * <p>
+ * See {@link AuthenticationState} for Pulsar's contract on how this interface is used by Pulsar.
*/
public class OneStageAuthenticationState implements AuthenticationState {
- private final AuthenticationDataSource authenticationDataSource;
- private final String authRole;
+ private AuthenticationDataSource authenticationDataSource;
+ private final SocketAddress remoteAddress;
+ private final SSLSession sslSession;
+ private final AuthenticationProvider provider;
+ private volatile String authRole;
+
+ /**
+ * Constructor for a {@link OneStageAuthenticationState} where there is no authentication performed during
+ * initialization.
+ * @param remoteAddress - remoteAddress associated with the {@link AuthenticationState}
+ * @param sslSession - sslSession associated with the {@link AuthenticationState}
+ * @param provider - {@link AuthenticationProvider} to use to verify {@link AuthData}
+ */
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);
+ AuthenticationProvider provider) {
+ this.provider = provider;
+ this.remoteAddress = remoteAddress;
+ this.sslSession = sslSession;
}
- public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider)
- throws AuthenticationException {
+ public OneStageAuthenticationState(HttpServletRequest request, AuthenticationProvider provider) {
+ // Must initialize this here for backwards compatibility with http authentication
this.authenticationDataSource = new AuthenticationDataHttps(request);
- this.authRole = provider.authenticate(authenticationDataSource);
+ this.provider = provider;
+ // These are not used when invoking this constructor.
+ this.remoteAddress = null;
+ this.sslSession = null;
}
@Override
- public String getAuthRole() {
+ public String getAuthRole() throws AuthenticationException {
+ if (authRole == null) {
+ throw new AuthenticationException("Must authenticate before calling getAuthRole");
+ }
Review Comment:
> Out of curiosity, what is the consequence of breaking source compatibility by removing the exception from the method signature?
No. I just noticed this point when I reviewed again. After looking for some documents and testing the ABI compatibility locally, it seems that removing the exception signature does not affect the ABI compatibility.
--
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