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 2022/06/23 16:38:17 UTC
[GitHub] [pulsar] nodece opened a new pull request, #16201: [fix][broker] Fix passing incorrect authentication data
nodece opened a new pull request, #16201:
URL: https://github.com/apache/pulsar/pull/16201
Signed-off-by: Zixuan Liu <no...@gmail.com>
### Motivation
#16065 fixes the race condition issue, but introduces a new issue, when we use the proxy to request the broker to do lookup operation, the broker uses the original principal and client authentication data to authorize, it is incorrect, we need to use original principal with original authentication data, not client authentication data.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
Added proxy test.
### Documentation
- [x] `doc-not-needed`
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1165655723
These tests need to verify the `originalPrincipal`, we have to forward the client authentication data to the broker by proxy.
```
/**
* <pre>
* It verifies jwt + Authentication + Authorization (client -> proxy -> broker).
*
* 1. client connects to proxy over jwt and pass auth-data
* 2. proxy authenticate client and retrieve client-role
* and send it to broker as originalPrincipal over jwt
* 3. client creates producer/consumer via proxy
* 4. broker authorize producer/consumer create request using originalPrincipal
*
* </pre>
*/
@Test
public void testProxyAuthorization() throws Exception {
```
This is my test result:
<img width="1304" alt="image" src="https://user-images.githubusercontent.com/16235121/175561293-535e3c02-fdf2-43c4-a185-43a8d31ed952.png">
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r906267103
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -382,19 +382,24 @@ private boolean invalidOriginalPrincipal(String originalPrincipal) {
// ////
private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, TopicOperation operation,
- AuthenticationDataSource authData) {
+ AuthenticationDataSource authDataSource, AuthenticationDataSource originalAuthDataSource) {
if (!service.isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(true);
}
CompletableFuture<Boolean> isProxyAuthorizedFuture;
- if (originalPrincipal != null) {
- isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
- topicName, operation, originalPrincipal, authData);
+ if (service.getPulsar().getConfiguration().isAuthenticateOriginalAuthData()) {
Review Comment:
There looks to have breaking change.
In old version, when `authenticateOriginalAuthData() == false`, we checked the original permission, this is incorrect behavior.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r908032084
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -512,7 +517,7 @@ protected void handlePartitionMetadataRequest(CommandPartitionedTopicMetadata pa
lookupSemaphore.release();
return;
}
- isTopicOperationAllowed(topicName, TopicOperation.LOOKUP, getAuthenticationData()).thenApply(
+ isTopicOperationAllowed(topicName, TopicOperation.LOOKUP, authenticationData, originalAuthData).thenApply(
Review Comment:
What's the difference with `isTopicOperationAllowed(topicName, TopicOperation.LOOKUP, getAuthenticationData(), null)`? (The 4th `null` argument is useless)
- If `originalAuthData` is not null, it will be used in `isTopicOperationAllowed`.
- Otherwise, `authenticationData` will be used in `isTopicOperationAllowed`.
But let's see `getAuthentcationData`:
```java
public AuthenticationDataSource getAuthenticationData() {
return originalAuthData != null ? originalAuthData : authenticationData;
}
```
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r908019720
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -185,7 +185,7 @@ public class ServerCnx extends PulsarHandler implements TransportCnx {
private String clientVersion = null;
private int nonPersistentPendingMessages = 0;
private final int maxNonPersistentPendingMessages;
- private String originalPrincipal = null;
+ String originalPrincipal = null;
Review Comment:
It's better to add a @VisibleForTesting annotation here. And we should only expose the getter, not the setter.
```java
@VisibleForTesting
String getOriginalPrincipal() {
return originalPrincipal;
}
```
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -411,9 +412,13 @@ private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName,
private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, String subscriptionName,
TopicOperation operation) {
if (service.isAuthorizationEnabled()) {
- AuthenticationDataSource authData =
- new AuthenticationDataSubscription(getAuthenticationData(), subscriptionName);
- return isTopicOperationAllowed(topicName, operation, authData);
+ AuthenticationDataSource authDataSource =
+ new AuthenticationDataSubscription(authenticationData, subscriptionName);
+ AuthenticationDataSource originalAuthDataSource = null;
+ if (originalAuthData != null) {
+ originalAuthDataSource = new AuthenticationDataSubscription(originalAuthData, subscriptionName);
+ }
+ return isTopicOperationAllowed(topicName, operation, authDataSource, originalAuthDataSource);
Review Comment:
I don't get why should it be so complicated? For the modified code,
- if `originalAuthData` is not `null`, `originalAuthData` will be used to construct the `AuthenticationDataSubscription` object as the 4th argument that will be used in `isTopicOperationAllowed`.
- Otherwise, the `authenticationData` will be used to construct the `AuthenticationDataSubscription` object (`authDataSource`) as the 3rd argument that will be used in `isTopicOperationAllowed`.
I think it's equivalent to
```java
return isTopicOperationAllowed(topicName, operation,
new AuthenticationDataSubscription(getAuthenticationData(), subscriptionName), null);
```
After applying my change, `testVerifyOriginalPrincipalWithAuthDataForwardedFromProxy` will fail, but that's because you assume `allowTopicOperationAsync` will accept a non-null 4th argument.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -167,7 +167,7 @@ public class ServerCnx extends PulsarHandler implements TransportCnx {
private State state;
private volatile boolean isActive = true;
String authRole = null;
- private volatile AuthenticationDataSource authenticationData;
+ volatile AuthenticationDataSource authenticationData;
Review Comment:
It's better to add a `@VisibleForTesting` annotation here.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1165685889
Your suggestion is great, I'll make a PR to do that.
--
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] codelipenghui commented on pull request #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1197614241
@nodece Could you please open a PR for branch-2.8
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1202114949
> @nodece Could you please open a PR for branch-2.8
See #16840.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r906267103
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -382,19 +382,24 @@ private boolean invalidOriginalPrincipal(String originalPrincipal) {
// ////
private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, TopicOperation operation,
- AuthenticationDataSource authData) {
+ AuthenticationDataSource authDataSource, AuthenticationDataSource originalAuthDataSource) {
if (!service.isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(true);
}
CompletableFuture<Boolean> isProxyAuthorizedFuture;
- if (originalPrincipal != null) {
- isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
- topicName, operation, originalPrincipal, authData);
+ if (service.getPulsar().getConfiguration().isAuthenticateOriginalAuthData()) {
Review Comment:
There looks to have breaking change.
In old version, when `authenticateOriginalAuthData() == false`, we checked the original permission, this is incorrect behavior.
There should be a new issue, so should I open a new PR to fix this?
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1166802405
@BewareMyPower Thank you for pointing out this problem, I made a wrong design in the code that made the test fail.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r908028138
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -411,9 +412,13 @@ private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName,
private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, String subscriptionName,
TopicOperation operation) {
if (service.isAuthorizationEnabled()) {
- AuthenticationDataSource authData =
- new AuthenticationDataSubscription(getAuthenticationData(), subscriptionName);
- return isTopicOperationAllowed(topicName, operation, authData);
+ AuthenticationDataSource authDataSource =
+ new AuthenticationDataSubscription(authenticationData, subscriptionName);
+ AuthenticationDataSource originalAuthDataSource = null;
+ if (originalAuthData != null) {
+ originalAuthDataSource = new AuthenticationDataSubscription(originalAuthData, subscriptionName);
+ }
+ return isTopicOperationAllowed(topicName, operation, authDataSource, originalAuthDataSource);
Review Comment:
I don't get why should it be so complicated? For the modified code,
- if `originalAuthData` is not `null`, `originalAuthData` will be used to construct the `AuthenticationDataSubscription` object **as the 4th argument** that will be used in `isTopicOperationAllowed`.
- Otherwise, the `authenticationData` will be used to construct the `AuthenticationDataSubscription` object (`authDataSource`) as the 3rd argument that will be used in `isTopicOperationAllowed`.
I think it's equivalent to
```java
return isTopicOperationAllowed(topicName, operation,
new AuthenticationDataSubscription(getAuthenticationData(), subscriptionName), null);
```
- if `originalAuthData` is not `null`, `getAuthenticationData()` will return `originalAuthData` that will be used to construct the `AuthenticationDataSubscription` object as **the 3rd argument** that will be used in `isTopicOperationAllowed`.
- Otherwise, `getAuthenticationData()` will return `authenticationData` that will be used to construct the `AuthenticationDataSubscription` object (`authDataSource`) as the 3rd argument that will be used in `isTopicOperationAllowed`.
After applying my change, `testVerifyOriginalPrincipalWithAuthDataForwardedFromProxy` will fail, but that's because you assume `allowTopicOperationAsync` will accept a non-null 4th argument.
--
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 pull request #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1165669660
I found the cause. It's because when I ran the tests, I have reverted your changes for `ServerCnx`.
Therefore, you must add another test to verify that when `authenticaionEnabled` and `forwardAuthorizationCredentials` are false, the authorization would fail. Otherwise it could be possible to introduce a regression in future because the tests would still pass if I reverted the changes for `ServerCnx` in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 pull request #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1165686859
I'm still confused about why https://github.com/apache/pulsar/pull/16065 introduced the bug? I just reverted e6b12c64 and run the `ProxyWithJwtAuthorizationTest`,`ProxyWithJwtAuthorizationTest` passed. i.e. it's not a regression.
But this PR makes `ProxyWithJwtAuthorizationTest` require the following configs:
```properties
conf.setAuthenticateOriginalAuthData(true);
proxyConfig.setForwardAuthorizationCredentials(true);
```
It looks like a breaking change on these configs.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r906267103
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -382,19 +382,24 @@ private boolean invalidOriginalPrincipal(String originalPrincipal) {
// ////
private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, TopicOperation operation,
- AuthenticationDataSource authData) {
+ AuthenticationDataSource authDataSource, AuthenticationDataSource originalAuthDataSource) {
if (!service.isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(true);
}
CompletableFuture<Boolean> isProxyAuthorizedFuture;
- if (originalPrincipal != null) {
- isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
- topicName, operation, originalPrincipal, authData);
+ if (service.getPulsar().getConfiguration().isAuthenticateOriginalAuthData()) {
Review Comment:
There looks to have breaking change.
When `authenticateOriginalAuthData() == false`, we shouldn't check the original permission, which is client role, not proxy role.
This is incorrect behavior.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r906267103
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -382,19 +382,24 @@ private boolean invalidOriginalPrincipal(String originalPrincipal) {
// ////
private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, TopicOperation operation,
- AuthenticationDataSource authData) {
+ AuthenticationDataSource authDataSource, AuthenticationDataSource originalAuthDataSource) {
if (!service.isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(true);
}
CompletableFuture<Boolean> isProxyAuthorizedFuture;
- if (originalPrincipal != null) {
- isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
- topicName, operation, originalPrincipal, authData);
+ if (service.getPulsar().getConfiguration().isAuthenticateOriginalAuthData()) {
Review Comment:
There looks to have breaking change.
When `authenticateOriginalAuthData() == false`, we shouldn't check the original permission, which is client role, not proxy role.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1165799030
> I'm still confused about why #16065 introduced the bug?
This bug is triggered only when the proxy is used, you can see this PR uses the `getAuthenticationData()` to get authentication data, when the proxy is used, it'll often use the proxy authentication data.
```
public AuthenticationDataSource getAuthenticationData() {
return originalAuthData != null ? originalAuthData : authenticationData;
}
```
The following is our test log:
```
2022-06-23T07:19:15,242+0000 [pulsar-io-4-1] DEBUG AuthorizationProviderOAuth - subject: client-role, scopes: [access], authzRoles: [
]
2022-06-23T07:19:15,242+0000 [pulsar-io-4-1] DEBUG org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider - Verify if role proxy-role is allowed to LOOKUP to topic per
```
The subject doesn't equal the role, it is incorrect.
I just reverted [e6b12c6](https://github.com/apache/pulsar/commit/e6b12c64b043903eb5ff2dc5186fe8030f157cfc) and run the `ProxyWithJwtAuthorizationTest`,`ProxyWithJwtAuthorizationTest` passed. i.e. it's not a regression.
>
> But this PR makes `ProxyWithJwtAuthorizationTest` require the following configs:
>
> ```ini
> conf.setAuthenticateOriginalAuthData(true);
> proxyConfig.setForwardAuthorizationCredentials(true);
> ```
>
> It looks like a breaking change on these configs.
It was an accident, although the intermediate link of verification is incorrect, the test can still pass.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r906839582
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -382,19 +382,24 @@ private boolean invalidOriginalPrincipal(String originalPrincipal) {
// ////
private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, TopicOperation operation,
- AuthenticationDataSource authData) {
+ AuthenticationDataSource authDataSource, AuthenticationDataSource originalAuthDataSource) {
if (!service.isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(true);
}
CompletableFuture<Boolean> isProxyAuthorizedFuture;
- if (originalPrincipal != null) {
- isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
- topicName, operation, originalPrincipal, authData);
+ if (service.getPulsar().getConfiguration().isAuthenticateOriginalAuthData()) {
Review Comment:
Revert to old code.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1165475705
> The proxy tests still succeed without the modifications on `ServerCnx`
You can try this step:
1. Clone this PR
2. Open `org.apache.pulsar.proxy.server.ProxyWithJwtAuthorizationTest`
3. Comment out the `proxyConfig.setForwardAuthorizationCredentials(true);` and `conf.setAuthenticateOriginalAuthData(true);`
4. Run `org.apache.pulsar.proxy.server.ProxyWithJwtAuthorizationTest`
`org.apache.pulsar.proxy.server.ProxyWithJwtAuthorizationTest#testProxyAuthorization` and `org.apache.pulsar.proxy.server.ProxyWithJwtAuthorizationTest#testProxyAuthorizationWithPrefixSubscriptionAuthMode` will fail.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r906267103
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -382,19 +382,24 @@ private boolean invalidOriginalPrincipal(String originalPrincipal) {
// ////
private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, TopicOperation operation,
- AuthenticationDataSource authData) {
+ AuthenticationDataSource authDataSource, AuthenticationDataSource originalAuthDataSource) {
if (!service.isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(true);
}
CompletableFuture<Boolean> isProxyAuthorizedFuture;
- if (originalPrincipal != null) {
- isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
- topicName, operation, originalPrincipal, authData);
+ if (service.getPulsar().getConfiguration().isAuthenticateOriginalAuthData()) {
Review Comment:
There looks to have breaking change.
When `authenticateOriginalAuthData() == false`, we shouldn't check the original permission, this is incorrect behavior.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1169453222
> Could this please be picked into 2.8, 2.9 and 2.10?
@codelipenghui Do you have this planning?
--
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 pull request #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1165485627
> Comment out the proxyConfig.setForwardAuthorizationCredentials(true); and conf.setAuthenticateOriginalAuthData(true);
So why did you add these two configs to tests?
<img width="1095" alt="image" src="https://user-images.githubusercontent.com/18204803/175526076-a23d33dd-2ec9-47cb-87f4-07bac6f66b1c.png">
In addition, it still succeeded.
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r908103534
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxAuthorizationTest.java:
##########
@@ -0,0 +1,434 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pulsar.broker.service;
+
+import static org.apache.pulsar.broker.BrokerTestUtil.spyWithClassAndConstructorArgs;
+import static org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.createMockBookKeeper;
+import static org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.createMockZooKeeper;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
+import com.google.common.collect.Sets;
+import io.jsonwebtoken.Jwts;
+import io.jsonwebtoken.SignatureAlgorithm;
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelPipeline;
+import io.netty.channel.EventLoopGroup;
+import io.netty.channel.nio.NioEventLoopGroup;
+import javax.crypto.SecretKey;
+import java.net.InetSocketAddress;
+import java.net.SocketAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+import java.util.Collections;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.concurrent.CompletableFuture;
+import org.apache.bookkeeper.common.util.OrderedExecutor;
+import org.apache.bookkeeper.mledger.ManagedLedgerFactory;
+import org.apache.pulsar.broker.PulsarService;
+import org.apache.pulsar.broker.ServiceConfiguration;
+import org.apache.pulsar.broker.authentication.AuthenticationDataSource;
+import org.apache.pulsar.broker.authentication.AuthenticationDataSubscription;
+import org.apache.pulsar.broker.authentication.AuthenticationProviderToken;
+import org.apache.pulsar.broker.authentication.utils.AuthTokenUtils;
+import org.apache.pulsar.broker.authorization.AuthorizationService;
+import org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider;
+import org.apache.pulsar.broker.intercept.BrokerInterceptor;
+import org.apache.pulsar.broker.resources.NamespaceResources;
+import org.apache.pulsar.broker.resources.PulsarResources;
+import org.apache.pulsar.broker.resources.TenantResources;
+import org.apache.pulsar.broker.service.schema.DefaultSchemaRegistryService;
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.impl.auth.AuthenticationToken;
+import org.apache.pulsar.common.api.proto.CommandConnect;
+import org.apache.pulsar.common.api.proto.CommandLookupTopic;
+import org.apache.pulsar.common.api.proto.CommandProducer;
+import org.apache.pulsar.common.api.proto.CommandSubscribe;
+import org.apache.pulsar.common.naming.TopicName;
+import org.apache.pulsar.common.policies.data.TenantInfo;
+import org.apache.pulsar.common.policies.data.TopicOperation;
+import org.apache.pulsar.metadata.api.MetadataStore;
+import org.apache.pulsar.metadata.impl.ZKMetadataStore;
+import org.apache.zookeeper.ZooKeeper;
+import org.mockito.ArgumentMatcher;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+@Test(groups = "broker")
+public class ServerCnxAuthorizationTest {
+ private final SecretKey SECRET_KEY = AuthTokenUtils.createSecretKey(SignatureAlgorithm.HS256);
+ private final String CLIENT_PRINCIPAL = "client";
+ private final String PROXY_PRINCIPAL = "proxy";
+ private final String CLIENT_TOKEN = Jwts.builder().setSubject(CLIENT_PRINCIPAL).signWith(SECRET_KEY).compact();
+ private final String PROXY_TOKEN = Jwts.builder().setSubject(PROXY_PRINCIPAL).signWith(SECRET_KEY).compact();
+
+ private PulsarService pulsar;
+ private PulsarResources pulsarResources;
+ private BrokerService brokerService;
+ private ServiceConfiguration svcConfig;
+
+ @BeforeMethod(alwaysRun = true)
+ public void beforeMethod() throws Exception {
+ EventLoopGroup eventLoopGroup = new NioEventLoopGroup();
+ svcConfig = spy(ServiceConfiguration.class);
+ svcConfig.setKeepAliveIntervalSeconds(0);
+ svcConfig.setBrokerShutdownTimeoutMs(0L);
+ svcConfig.setLoadBalancerOverrideBrokerNicSpeedGbps(Optional.of(1.0d));
+ svcConfig.setClusterName("pulsar-cluster");
+ svcConfig.setSuperUserRoles(Collections.singleton(PROXY_PRINCIPAL));
+ svcConfig.setAuthenticationEnabled(true);
+ svcConfig.setAuthenticationProviders(Sets.newHashSet(AuthenticationProviderToken.class.getName()));
+ svcConfig.setAuthorizationEnabled(true);
+ svcConfig.setAuthorizationProvider(PulsarAuthorizationProvider.class.getName());
+ Properties properties = new Properties();
+ properties.setProperty("tokenSecretKey", "data:;base64,"
+ + Base64.getEncoder().encodeToString(SECRET_KEY.getEncoded()));
+ svcConfig.setProperties(properties);
+
+ pulsar = spyWithClassAndConstructorArgs(PulsarService.class, svcConfig);
+ doReturn(new DefaultSchemaRegistryService()).when(pulsar).getSchemaRegistryService();
+
+ doReturn(svcConfig).when(pulsar).getConfiguration();
+ doReturn(mock(PulsarResources.class)).when(pulsar).getPulsarResources();
+
+ ManagedLedgerFactory mlFactoryMock = mock(ManagedLedgerFactory.class);
+ doReturn(mlFactoryMock).when(pulsar).getManagedLedgerFactory();
+
+ ZooKeeper mockZk = createMockZooKeeper();
+ OrderedExecutor executor = OrderedExecutor.newBuilder().numThreads(1).build();
+ doReturn(createMockBookKeeper(executor))
+ .when(pulsar).getBookKeeperClient();
+
+ MetadataStore store = new ZKMetadataStore(mockZk);
+
+ doReturn(store).when(pulsar).getLocalMetadataStore();
+ doReturn(store).when(pulsar).getConfigurationMetadataStore();
+
+ pulsarResources = spyWithClassAndConstructorArgs(PulsarResources.class, store, store);
+ doReturn(pulsarResources).when(pulsar).getPulsarResources();
+ NamespaceResources namespaceResources =
+ spyWithClassAndConstructorArgs(NamespaceResources.class, store, store, 30);
+ doReturn(namespaceResources).when(pulsarResources).getNamespaceResources();
+
+ TenantResources tenantResources = spyWithClassAndConstructorArgs(TenantResources.class, store, 30);
+ doReturn(tenantResources).when(pulsarResources).getTenantResources();
+
+ doReturn(CompletableFuture.completedFuture(Optional.of(TenantInfo.builder().build()))).when(tenantResources)
+ .getTenantAsync("public");
+
+ brokerService = spyWithClassAndConstructorArgs(BrokerService.class, pulsar, eventLoopGroup);
+ BrokerInterceptor interceptor = mock(BrokerInterceptor.class);
+ doReturn(interceptor).when(brokerService).getInterceptor();
+ doReturn(brokerService).when(pulsar).getBrokerService();
+ doReturn(executor).when(pulsar).getOrderedExecutor();
+ }
+
+ @Test
+ public void testVerifyOriginalPrincipalWithAuthDataForwardedFromProxy() throws Exception {
+ doReturn(true).when(svcConfig).isAuthenticateOriginalAuthData();
+
+ ServerCnx serverCnx = spyWithClassAndConstructorArgs(ServerCnx.class, pulsar);
+ ChannelHandlerContext channelHandlerContext = mock(ChannelHandlerContext.class);
+ Channel channel = mock(Channel.class);
+ ChannelPipeline channelPipeline = mock(ChannelPipeline.class);
+ doReturn(channelPipeline).when(channel).pipeline();
+ doReturn(null).when(channelPipeline).get(PulsarChannelInitializer.TLS_HANDLER);
+
+ SocketAddress socketAddress = new InetSocketAddress(0);
+ doReturn(socketAddress).when(channel).remoteAddress();
+ doReturn(channel).when(channelHandlerContext).channel();
+ channelHandlerContext.channel().remoteAddress();
+ serverCnx.channelActive(channelHandlerContext);
+
+ // connect
+ AuthenticationToken clientAuthenticationToken = new AuthenticationToken(CLIENT_TOKEN);
+ AuthenticationToken proxyAuthenticationToken = new AuthenticationToken(PROXY_TOKEN);
+ CommandConnect connect = new CommandConnect();
+ connect.setAuthMethodName(proxyAuthenticationToken.getAuthMethodName());
+ connect.setAuthData(proxyAuthenticationToken.getAuthData().getCommandData().getBytes(StandardCharsets.UTF_8));
+ connect.setClientVersion("test");
+ connect.setProtocolVersion(1);
+ connect.setOriginalPrincipal(CLIENT_PRINCIPAL);
+ connect.setOriginalAuthData(clientAuthenticationToken.getAuthData().getCommandData());
+ connect.setOriginalAuthMethod(clientAuthenticationToken.getAuthMethodName());
+
+ serverCnx.handleConnect(connect);
+ assertEquals(serverCnx.getOriginalAuthData().getCommandData(),
+ clientAuthenticationToken.getAuthData().getCommandData());
+ assertEquals(serverCnx.getOriginalAuthState().getAuthRole(), CLIENT_PRINCIPAL);
+ assertEquals(serverCnx.getOriginalPrincipal(), CLIENT_PRINCIPAL);
+ assertEquals(serverCnx.getAuthData().getCommandData(),
+ proxyAuthenticationToken.getAuthData().getCommandData());
+ assertEquals(serverCnx.getAuthRole(), PROXY_PRINCIPAL);
+ assertEquals(serverCnx.getAuthState().getAuthRole(), PROXY_PRINCIPAL);
+
+ AuthorizationService authorizationService =
+ spyWithClassAndConstructorArgs(AuthorizationService.class, svcConfig, pulsarResources);
+ doReturn(authorizationService).when(brokerService).getAuthorizationService();
+
+ // lookup
+ CommandLookupTopic commandLookupTopic = new CommandLookupTopic();
+ TopicName topicName = TopicName.get("persistent://public/default/test-topic");
+ commandLookupTopic.setTopic(topicName.toString());
+ commandLookupTopic.setRequestId(1);
+ serverCnx.handleLookup(commandLookupTopic);
+ verify(authorizationService, times(1)).allowTopicOperationAsync(topicName, TopicOperation.LOOKUP,
+ CLIENT_PRINCIPAL,
+ serverCnx.getOriginalAuthData());
+ verify(authorizationService, times(1)).allowTopicOperationAsync(topicName, TopicOperation.LOOKUP,
+ PROXY_PRINCIPAL,
+ serverCnx.getAuthData());
+
+ // producer
+ CommandProducer commandProducer = new CommandProducer();
+ commandProducer.setRequestId(1);
+ commandProducer.setProducerId(1);
+ commandProducer.setProducerName("test-producer");
+ commandProducer.setTopic(topicName.toString());
+ serverCnx.handleProducer(commandProducer);
+ verify(authorizationService, times(1)).allowTopicOperationAsync(topicName, TopicOperation.PRODUCE,
+ CLIENT_PRINCIPAL,
+ serverCnx.getOriginalAuthData());
+ verify(authorizationService, times(1)).allowTopicOperationAsync(topicName, TopicOperation.LOOKUP,
+ PROXY_PRINCIPAL,
+ serverCnx.getAuthData());
+
+ // consumer
+ CommandSubscribe commandSubscribe = new CommandSubscribe();
+ commandSubscribe.setTopic(topicName.toString());
+ commandSubscribe.setRequestId(1);
+ commandSubscribe.setConsumerId(1);
+ final String subscriptionName = "test-subscribe";
+ commandSubscribe.setSubscription("test-subscribe");
+ commandSubscribe.setSubType(CommandSubscribe.SubType.Shared);
+ serverCnx.handleSubscribe(commandSubscribe);
+
+ verify(authorizationService, times(1)).allowTopicOperationAsync(
+ eq(topicName), eq(TopicOperation.CONSUME),
+ eq(CLIENT_PRINCIPAL), argThat(arg -> {
+ assertTrue(arg instanceof AuthenticationDataSubscription);
+ try {
+ assertEquals(arg.getCommandData(), clientAuthenticationToken.getAuthData().getCommandData());
+ } catch (PulsarClientException e) {
+ fail(e.getMessage());
+ }
+ assertEquals(arg.getSubscription(), subscriptionName);
+ return true;
+ }));
+ verify(authorizationService, times(1)).allowTopicOperationAsync(
+ eq(topicName), eq(TopicOperation.CONSUME),
+ eq(PROXY_PRINCIPAL), argThat(arg -> {
+ assertTrue(arg instanceof AuthenticationDataSubscription);
+ try {
+ assertEquals(arg.getCommandData(), proxyAuthenticationToken.getAuthData().getCommandData());
+ } catch (PulsarClientException e) {
+ fail(e.getMessage());
+ }
+ assertEquals(arg.getSubscription(), subscriptionName);
+ return true;
+ }));
+ }
+
+ @Test
+ public void testVerifyOriginalPrincipalWithoutAuthDataForwardedFromProxy() throws Exception {
+ doReturn(false).when(svcConfig).isAuthenticateOriginalAuthData();
+
+ ServerCnx serverCnx = spyWithClassAndConstructorArgs(ServerCnx.class, pulsar);
+ ChannelHandlerContext channelHandlerContext = mock(ChannelHandlerContext.class);
+ Channel channel = mock(Channel.class);
+ ChannelPipeline channelPipeline = mock(ChannelPipeline.class);
+ doReturn(channelPipeline).when(channel).pipeline();
+ doReturn(null).when(channelPipeline).get(PulsarChannelInitializer.TLS_HANDLER);
+
+ SocketAddress socketAddress = new InetSocketAddress(0);
+ doReturn(socketAddress).when(channel).remoteAddress();
+ doReturn(channel).when(channelHandlerContext).channel();
+ channelHandlerContext.channel().remoteAddress();
+ serverCnx.channelActive(channelHandlerContext);
+
+ // connect
+ AuthenticationToken clientAuthenticationToken = new AuthenticationToken(CLIENT_TOKEN);
Review Comment:
```suggestion
```
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r908035655
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -512,7 +517,7 @@ protected void handlePartitionMetadataRequest(CommandPartitionedTopicMetadata pa
lookupSemaphore.release();
return;
}
- isTopicOperationAllowed(topicName, TopicOperation.LOOKUP, getAuthenticationData()).thenApply(
+ isTopicOperationAllowed(topicName, TopicOperation.LOOKUP, authenticationData, originalAuthData).thenApply(
Review Comment:
Sorry, I got it now. Resolve it.
--
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] codelipenghui commented on pull request #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1168372838
@merlimat Please help review 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] nodece commented on pull request #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1168271172
@BewareMyPower Great idea, we should distinguish between the `isProxyAuthorizedFuture` and `isAuthorized`.
--
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] codelipenghui merged pull request #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #16201:
URL: https://github.com/apache/pulsar/pull/16201
--
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 #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#discussion_r908180905
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxAuthorizationTest.java:
##########
@@ -0,0 +1,434 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pulsar.broker.service;
+
+import static org.apache.pulsar.broker.BrokerTestUtil.spyWithClassAndConstructorArgs;
+import static org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.createMockBookKeeper;
+import static org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.createMockZooKeeper;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
+import com.google.common.collect.Sets;
+import io.jsonwebtoken.Jwts;
+import io.jsonwebtoken.SignatureAlgorithm;
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelPipeline;
+import io.netty.channel.EventLoopGroup;
+import io.netty.channel.nio.NioEventLoopGroup;
+import javax.crypto.SecretKey;
+import java.net.InetSocketAddress;
+import java.net.SocketAddress;
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+import java.util.Collections;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.concurrent.CompletableFuture;
+import org.apache.bookkeeper.common.util.OrderedExecutor;
+import org.apache.bookkeeper.mledger.ManagedLedgerFactory;
+import org.apache.pulsar.broker.PulsarService;
+import org.apache.pulsar.broker.ServiceConfiguration;
+import org.apache.pulsar.broker.authentication.AuthenticationDataSource;
+import org.apache.pulsar.broker.authentication.AuthenticationDataSubscription;
+import org.apache.pulsar.broker.authentication.AuthenticationProviderToken;
+import org.apache.pulsar.broker.authentication.utils.AuthTokenUtils;
+import org.apache.pulsar.broker.authorization.AuthorizationService;
+import org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider;
+import org.apache.pulsar.broker.intercept.BrokerInterceptor;
+import org.apache.pulsar.broker.resources.NamespaceResources;
+import org.apache.pulsar.broker.resources.PulsarResources;
+import org.apache.pulsar.broker.resources.TenantResources;
+import org.apache.pulsar.broker.service.schema.DefaultSchemaRegistryService;
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.client.impl.auth.AuthenticationToken;
+import org.apache.pulsar.common.api.proto.CommandConnect;
+import org.apache.pulsar.common.api.proto.CommandLookupTopic;
+import org.apache.pulsar.common.api.proto.CommandProducer;
+import org.apache.pulsar.common.api.proto.CommandSubscribe;
+import org.apache.pulsar.common.naming.TopicName;
+import org.apache.pulsar.common.policies.data.TenantInfo;
+import org.apache.pulsar.common.policies.data.TopicOperation;
+import org.apache.pulsar.metadata.api.MetadataStore;
+import org.apache.pulsar.metadata.impl.ZKMetadataStore;
+import org.apache.zookeeper.ZooKeeper;
+import org.mockito.ArgumentMatcher;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+@Test(groups = "broker")
+public class ServerCnxAuthorizationTest {
+ private final SecretKey SECRET_KEY = AuthTokenUtils.createSecretKey(SignatureAlgorithm.HS256);
+ private final String CLIENT_PRINCIPAL = "client";
+ private final String PROXY_PRINCIPAL = "proxy";
+ private final String CLIENT_TOKEN = Jwts.builder().setSubject(CLIENT_PRINCIPAL).signWith(SECRET_KEY).compact();
+ private final String PROXY_TOKEN = Jwts.builder().setSubject(PROXY_PRINCIPAL).signWith(SECRET_KEY).compact();
+
+ private PulsarService pulsar;
+ private PulsarResources pulsarResources;
+ private BrokerService brokerService;
+ private ServiceConfiguration svcConfig;
+
+ @BeforeMethod(alwaysRun = true)
+ public void beforeMethod() throws Exception {
+ EventLoopGroup eventLoopGroup = new NioEventLoopGroup();
+ svcConfig = spy(ServiceConfiguration.class);
+ svcConfig.setKeepAliveIntervalSeconds(0);
+ svcConfig.setBrokerShutdownTimeoutMs(0L);
+ svcConfig.setLoadBalancerOverrideBrokerNicSpeedGbps(Optional.of(1.0d));
+ svcConfig.setClusterName("pulsar-cluster");
+ svcConfig.setSuperUserRoles(Collections.singleton(PROXY_PRINCIPAL));
+ svcConfig.setAuthenticationEnabled(true);
+ svcConfig.setAuthenticationProviders(Sets.newHashSet(AuthenticationProviderToken.class.getName()));
+ svcConfig.setAuthorizationEnabled(true);
+ svcConfig.setAuthorizationProvider(PulsarAuthorizationProvider.class.getName());
+ Properties properties = new Properties();
+ properties.setProperty("tokenSecretKey", "data:;base64,"
+ + Base64.getEncoder().encodeToString(SECRET_KEY.getEncoded()));
+ svcConfig.setProperties(properties);
+
+ pulsar = spyWithClassAndConstructorArgs(PulsarService.class, svcConfig);
+ doReturn(new DefaultSchemaRegistryService()).when(pulsar).getSchemaRegistryService();
+
+ doReturn(svcConfig).when(pulsar).getConfiguration();
+ doReturn(mock(PulsarResources.class)).when(pulsar).getPulsarResources();
+
+ ManagedLedgerFactory mlFactoryMock = mock(ManagedLedgerFactory.class);
+ doReturn(mlFactoryMock).when(pulsar).getManagedLedgerFactory();
+
+ ZooKeeper mockZk = createMockZooKeeper();
+ OrderedExecutor executor = OrderedExecutor.newBuilder().numThreads(1).build();
+ doReturn(createMockBookKeeper(executor))
+ .when(pulsar).getBookKeeperClient();
+
+ MetadataStore store = new ZKMetadataStore(mockZk);
+
+ doReturn(store).when(pulsar).getLocalMetadataStore();
+ doReturn(store).when(pulsar).getConfigurationMetadataStore();
+
+ pulsarResources = spyWithClassAndConstructorArgs(PulsarResources.class, store, store);
+ doReturn(pulsarResources).when(pulsar).getPulsarResources();
+ NamespaceResources namespaceResources =
+ spyWithClassAndConstructorArgs(NamespaceResources.class, store, store, 30);
+ doReturn(namespaceResources).when(pulsarResources).getNamespaceResources();
+
+ TenantResources tenantResources = spyWithClassAndConstructorArgs(TenantResources.class, store, 30);
+ doReturn(tenantResources).when(pulsarResources).getTenantResources();
+
+ doReturn(CompletableFuture.completedFuture(Optional.of(TenantInfo.builder().build()))).when(tenantResources)
+ .getTenantAsync("public");
+
+ brokerService = spyWithClassAndConstructorArgs(BrokerService.class, pulsar, eventLoopGroup);
+ BrokerInterceptor interceptor = mock(BrokerInterceptor.class);
+ doReturn(interceptor).when(brokerService).getInterceptor();
+ doReturn(brokerService).when(pulsar).getBrokerService();
+ doReturn(executor).when(pulsar).getOrderedExecutor();
+ }
+
+ @Test
+ public void testVerifyOriginalPrincipalWithAuthDataForwardedFromProxy() throws Exception {
+ doReturn(true).when(svcConfig).isAuthenticateOriginalAuthData();
+
+ ServerCnx serverCnx = spyWithClassAndConstructorArgs(ServerCnx.class, pulsar);
+ ChannelHandlerContext channelHandlerContext = mock(ChannelHandlerContext.class);
+ Channel channel = mock(Channel.class);
+ ChannelPipeline channelPipeline = mock(ChannelPipeline.class);
+ doReturn(channelPipeline).when(channel).pipeline();
+ doReturn(null).when(channelPipeline).get(PulsarChannelInitializer.TLS_HANDLER);
+
+ SocketAddress socketAddress = new InetSocketAddress(0);
+ doReturn(socketAddress).when(channel).remoteAddress();
+ doReturn(channel).when(channelHandlerContext).channel();
+ channelHandlerContext.channel().remoteAddress();
+ serverCnx.channelActive(channelHandlerContext);
+
+ // connect
+ AuthenticationToken clientAuthenticationToken = new AuthenticationToken(CLIENT_TOKEN);
+ AuthenticationToken proxyAuthenticationToken = new AuthenticationToken(PROXY_TOKEN);
+ CommandConnect connect = new CommandConnect();
+ connect.setAuthMethodName(proxyAuthenticationToken.getAuthMethodName());
+ connect.setAuthData(proxyAuthenticationToken.getAuthData().getCommandData().getBytes(StandardCharsets.UTF_8));
+ connect.setClientVersion("test");
+ connect.setProtocolVersion(1);
+ connect.setOriginalPrincipal(CLIENT_PRINCIPAL);
+ connect.setOriginalAuthData(clientAuthenticationToken.getAuthData().getCommandData());
+ connect.setOriginalAuthMethod(clientAuthenticationToken.getAuthMethodName());
+
+ serverCnx.handleConnect(connect);
+ assertEquals(serverCnx.getOriginalAuthData().getCommandData(),
+ clientAuthenticationToken.getAuthData().getCommandData());
+ assertEquals(serverCnx.getOriginalAuthState().getAuthRole(), CLIENT_PRINCIPAL);
+ assertEquals(serverCnx.getOriginalPrincipal(), CLIENT_PRINCIPAL);
+ assertEquals(serverCnx.getAuthData().getCommandData(),
+ proxyAuthenticationToken.getAuthData().getCommandData());
+ assertEquals(serverCnx.getAuthRole(), PROXY_PRINCIPAL);
+ assertEquals(serverCnx.getAuthState().getAuthRole(), PROXY_PRINCIPAL);
+
+ AuthorizationService authorizationService =
+ spyWithClassAndConstructorArgs(AuthorizationService.class, svcConfig, pulsarResources);
+ doReturn(authorizationService).when(brokerService).getAuthorizationService();
+
+ // lookup
+ CommandLookupTopic commandLookupTopic = new CommandLookupTopic();
+ TopicName topicName = TopicName.get("persistent://public/default/test-topic");
+ commandLookupTopic.setTopic(topicName.toString());
+ commandLookupTopic.setRequestId(1);
+ serverCnx.handleLookup(commandLookupTopic);
+ verify(authorizationService, times(1)).allowTopicOperationAsync(topicName, TopicOperation.LOOKUP,
+ CLIENT_PRINCIPAL,
+ serverCnx.getOriginalAuthData());
+ verify(authorizationService, times(1)).allowTopicOperationAsync(topicName, TopicOperation.LOOKUP,
+ PROXY_PRINCIPAL,
+ serverCnx.getAuthData());
+
+ // producer
+ CommandProducer commandProducer = new CommandProducer();
+ commandProducer.setRequestId(1);
+ commandProducer.setProducerId(1);
+ commandProducer.setProducerName("test-producer");
+ commandProducer.setTopic(topicName.toString());
+ serverCnx.handleProducer(commandProducer);
+ verify(authorizationService, times(1)).allowTopicOperationAsync(topicName, TopicOperation.PRODUCE,
+ CLIENT_PRINCIPAL,
+ serverCnx.getOriginalAuthData());
+ verify(authorizationService, times(1)).allowTopicOperationAsync(topicName, TopicOperation.LOOKUP,
+ PROXY_PRINCIPAL,
+ serverCnx.getAuthData());
+
+ // consumer
+ CommandSubscribe commandSubscribe = new CommandSubscribe();
+ commandSubscribe.setTopic(topicName.toString());
+ commandSubscribe.setRequestId(1);
+ commandSubscribe.setConsumerId(1);
+ final String subscriptionName = "test-subscribe";
+ commandSubscribe.setSubscription("test-subscribe");
+ commandSubscribe.setSubType(CommandSubscribe.SubType.Shared);
+ serverCnx.handleSubscribe(commandSubscribe);
+
+ verify(authorizationService, times(1)).allowTopicOperationAsync(
+ eq(topicName), eq(TopicOperation.CONSUME),
+ eq(CLIENT_PRINCIPAL), argThat(arg -> {
+ assertTrue(arg instanceof AuthenticationDataSubscription);
+ try {
+ assertEquals(arg.getCommandData(), clientAuthenticationToken.getAuthData().getCommandData());
+ } catch (PulsarClientException e) {
+ fail(e.getMessage());
+ }
+ assertEquals(arg.getSubscription(), subscriptionName);
+ return true;
+ }));
+ verify(authorizationService, times(1)).allowTopicOperationAsync(
+ eq(topicName), eq(TopicOperation.CONSUME),
+ eq(PROXY_PRINCIPAL), argThat(arg -> {
+ assertTrue(arg instanceof AuthenticationDataSubscription);
+ try {
+ assertEquals(arg.getCommandData(), proxyAuthenticationToken.getAuthData().getCommandData());
+ } catch (PulsarClientException e) {
+ fail(e.getMessage());
+ }
+ assertEquals(arg.getSubscription(), subscriptionName);
+ return true;
+ }));
+ }
+
+ @Test
+ public void testVerifyOriginalPrincipalWithoutAuthDataForwardedFromProxy() throws Exception {
+ doReturn(false).when(svcConfig).isAuthenticateOriginalAuthData();
+
+ ServerCnx serverCnx = spyWithClassAndConstructorArgs(ServerCnx.class, pulsar);
+ ChannelHandlerContext channelHandlerContext = mock(ChannelHandlerContext.class);
+ Channel channel = mock(Channel.class);
+ ChannelPipeline channelPipeline = mock(ChannelPipeline.class);
+ doReturn(channelPipeline).when(channel).pipeline();
+ doReturn(null).when(channelPipeline).get(PulsarChannelInitializer.TLS_HANDLER);
+
+ SocketAddress socketAddress = new InetSocketAddress(0);
+ doReturn(socketAddress).when(channel).remoteAddress();
+ doReturn(channel).when(channelHandlerContext).channel();
+ channelHandlerContext.channel().remoteAddress();
+ serverCnx.channelActive(channelHandlerContext);
+
+ // connect
+ AuthenticationToken clientAuthenticationToken = new AuthenticationToken(CLIENT_TOKEN);
Review Comment:
Done.
--
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 pull request #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1168249833
Actually I think the logic is not clear. The key point is that both `originalAuthData` and `authenticationData` might be used in `isProxyAuthorizedFuture`, but `authenticationData` is always used in `isAuthorizedFuture`.
What do you think of my following code?
```java
private CompletableFuture<Boolean> isTopicOperationAllowedByProxy(TopicName topicName, TopicOperation operation,
String subscription) {
if (originalPrincipal != null) {
final AuthenticationDataSource authenticationDataSource = (subscription == null)
? getAuthenticationData()
: new AuthenticationDataSubscription(getAuthenticationData(), subscription);
return service.getAuthorizationService().allowTopicOperationAsync(
topicName, operation, originalPrincipal, authenticationDataSource);
} else {
return CompletableFuture.completedFuture(true);
}
}
private CompletableFuture<Boolean> isTopicOperationAllowedByBroker(TopicName topicName, TopicOperation operation,
String subscription) {
final AuthenticationDataSource authenticationDataSource = (subscription == null)
? getAuthenticationData()
: new AuthenticationDataSubscription(authenticationData, subscription);
return service.getAuthorizationService()
.allowTopicOperationAsync(topicName, operation, authRole, authenticationDataSource);
}
private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, TopicOperation operation,
String subscription) {
if (!service.isAuthorizationEnabled()) {
return CompletableFuture.completedFuture(true);
}
return isTopicOperationAllowedByProxy(topicName, operation, subscription)
.thenCombine(isTopicOperationAllowedByBroker(topicName, operation, subscription),
(isProxyAuthorized, isAuthorized) -> {
/* ... */
return isProxyAuthorized && isAuthorized;
});
}
```
--
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] utahkay commented on pull request #16201: [fix][broker] Fix passing incorrect authentication data
Posted by GitBox <gi...@apache.org>.
utahkay commented on PR #16201:
URL: https://github.com/apache/pulsar/pull/16201#issuecomment-1169165819
Could this please be picked into 2.8, 2.9 and 2.10?
--
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