You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/06/30 12:40:59 UTC
[GitHub] [pulsar] freeznet opened a new pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
freeznet opened a new pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172
### Motivation
https://github.com/apache/pulsar-client-go/issues/554 reported an issue and we found out an inconsistency issue between http lookup service and binary lookup service. It appears that http lookup service requires auth check for `GetTopicsOfNamespace` but not with binary lookup service.
This PR adds auth check to `GetTopicsOfNamespace` with binary lookup service.
### Verifying this change
- [ ] Make sure that the change passes the CI 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] sijie commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-879639270
@eolivelli please review this PR again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-883018587
@eolivelli Can you review this again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-899734952
@hangc0276 - I believe this commit should be cherry picked to `branch-2.7` as well.
--
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] eolivelli merged pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172
--
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] sijie commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-883018587
@eolivelli Can you review this again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] freeznet commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
freeznet commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-874470277
@eolivelli changes have been made according to your comment, PTAL, 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] freeznet commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
freeznet commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-871873729
@Anonymitaet thanks for asking, and I dont think this fix relates to any doc yet.
--
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] sijie commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-883018587
@eolivelli Can you review this again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] freeznet commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
freeznet commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-874443977
--
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] hangc0276 commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-893308652
@eolivelli Would you please help review again? 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] freeznet commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
freeznet commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-874443977
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 change in pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#discussion_r680224433
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1722,30 +1723,97 @@ public void readEntryFailed(ManagedLedgerException exception, Object ctx) {
});
}
+ private CompletableFuture<Boolean> isNamespaceOperationAllowed(NamespaceName namespaceName,
Review comment:
It seems like this method and the `isTopicOperationAllowed` method share a lot of core logic with just a couple places that would branch for looking up `allowNamespaceOperationAsync` vs `allowNamespaceOperationAsync`. It'd be good to consolidate this logic, if possible.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1722,30 +1723,97 @@ public void readEntryFailed(ManagedLedgerException exception, Object ctx) {
});
}
+ private CompletableFuture<Boolean> isNamespaceOperationAllowed(NamespaceName namespaceName,
+ NamespaceOperation operation) {
+ CompletableFuture<Boolean> isProxyAuthorizedFuture;
+ CompletableFuture<Boolean> isAuthorizedFuture;
+ if (service.isAuthorizationEnabled()) {
+ if (originalPrincipal != null) {
+ isProxyAuthorizedFuture = service.getAuthorizationService().allowNamespaceOperationAsync(
+ namespaceName, operation, originalPrincipal, getAuthenticationData());
+ } else {
+ isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
+ }
+ isAuthorizedFuture = service.getAuthorizationService().allowNamespaceOperationAsync(
+ namespaceName, operation, authRole, authenticationData);
+ } else {
+ isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
+ isAuthorizedFuture = CompletableFuture.completedFuture(true);
+ }
+ return isProxyAuthorizedFuture.thenCombine(isAuthorizedFuture, (isProxyAuthorized, isAuthorized) -> {
+ if (!isProxyAuthorized) {
+ log.warn("OriginalRole {} is not authorized to perform operation {} on namespace {}",
+ originalPrincipal, operation, namespaceName);
+ }
+ if (!isAuthorized) {
+ log.warn("Role {} is not authorized to perform operation {} on namespace {}",
+ authRole, operation, namespaceName);
+ }
+ return isProxyAuthorized && isAuthorized;
+ });
+ }
+
@Override
protected void handleGetTopicsOfNamespace(CommandGetTopicsOfNamespace commandGetTopicsOfNamespace) {
final long requestId = commandGetTopicsOfNamespace.getRequestId();
final String namespace = commandGetTopicsOfNamespace.getNamespace();
final CommandGetTopicsOfNamespace.Mode mode = commandGetTopicsOfNamespace.getMode();
final NamespaceName namespaceName = NamespaceName.get(namespace);
- getBrokerService().pulsar().getNamespaceService().getListOfTopics(namespaceName, mode)
- .thenAccept(topics -> {
- if (log.isDebugEnabled()) {
- log.debug("[{}] Received CommandGetTopicsOfNamespace for namespace [//{}] by {}, size:{}",
- remoteAddress, namespace, requestId, topics.size());
- }
- commandSender.sendGetTopicsOfNamespaceResponse(topics, requestId);
- })
- .exceptionally(ex -> {
- log.warn("[{}] Error GetTopicsOfNamespace for namespace [//{}] by {}",
- remoteAddress, namespace, requestId);
- commandSender.sendErrorResponse(requestId,
- BrokerServiceException.getClientErrorCode(new ServerMetadataException(ex)),
- ex.getMessage());
-
- return null;
- });
+ final Semaphore lookupSemaphore = service.getLookupRequestSemaphore();
Review comment:
I'm seeing some code duplication in this file where we call `service.getLookupRequestSemaphore()`. It could be good to come back later and consolidate the 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] hangc0276 commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-893308652
@eolivelli Would you please help review again? 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] hangc0276 commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-900718522
> @hangc0276 - I believe this commit should be cherry picked to `branch-2.7` as well.
@michaeljmarshall Thanks for your remind, i have add release-2.7.4 label, this PR will be cherry-picked to branch-2.7.
--
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] eolivelli commented on a change in pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#discussion_r662804510
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1722,30 +1723,97 @@ public void readEntryFailed(ManagedLedgerException exception, Object ctx) {
});
}
+ private CompletableFuture<Boolean> isNamespaceOperationAllowed(NamespaceName namespaceName,
+ NamespaceOperation operation) {
+ CompletableFuture<Boolean> isProxyAuthorizedFuture;
+ CompletableFuture<Boolean> isAuthorizedFuture;
+ if (service.isAuthorizationEnabled()) {
+ if (originalPrincipal != null) {
+ isProxyAuthorizedFuture = service.getAuthorizationService().allowNamespaceOperationAsync(
+ namespaceName, operation, originalPrincipal, getAuthenticationData());
+ } else {
+ isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
+ }
+ isAuthorizedFuture = service.getAuthorizationService().allowNamespaceOperationAsync(
+ namespaceName, operation, authRole, authenticationData);
+ } else {
+ isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
+ isAuthorizedFuture = CompletableFuture.completedFuture(true);
+ }
+ return isProxyAuthorizedFuture.thenCombine(isAuthorizedFuture, (isProxyAuthorized, isAuthorized) -> {
+ if (!isProxyAuthorized) {
+ log.warn("OriginalRole {} is not authorized to perform operation {} on namespace {}",
+ originalPrincipal, operation, namespaceName);
+ }
+ if (!isAuthorized) {
+ log.warn("Role {} is not authorized to perform operation {} on namespace {}",
+ authRole, operation, namespaceName);
+ }
+ return isProxyAuthorized && isAuthorized;
+ });
+ }
+
@Override
protected void handleGetTopicsOfNamespace(CommandGetTopicsOfNamespace commandGetTopicsOfNamespace) {
final long requestId = commandGetTopicsOfNamespace.getRequestId();
final String namespace = commandGetTopicsOfNamespace.getNamespace();
final CommandGetTopicsOfNamespace.Mode mode = commandGetTopicsOfNamespace.getMode();
final NamespaceName namespaceName = NamespaceName.get(namespace);
- getBrokerService().pulsar().getNamespaceService().getListOfTopics(namespaceName, mode)
- .thenAccept(topics -> {
- if (log.isDebugEnabled()) {
- log.debug("[{}] Received CommandGetTopicsOfNamespace for namespace [//{}] by {}, size:{}",
- remoteAddress, namespace, requestId, topics.size());
- }
- commandSender.sendGetTopicsOfNamespaceResponse(topics, requestId);
- })
- .exceptionally(ex -> {
- log.warn("[{}] Error GetTopicsOfNamespace for namespace [//{}] by {}",
- remoteAddress, namespace, requestId);
- commandSender.sendErrorResponse(requestId,
- BrokerServiceException.getClientErrorCode(new ServerMetadataException(ex)),
- ex.getMessage());
-
- return null;
- });
+ final Semaphore lookupSemaphore = service.getLookupRequestSemaphore();
Review comment:
this change is not listed in the description, can you please add 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] hangc0276 commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-900718522
> @hangc0276 - I believe this commit should be cherry picked to `branch-2.7` as well.
@michaeljmarshall Thanks for your remind, i have add release-2.7.4 label, this PR will be cherry-picked to branch-2.7.
--
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] freeznet commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
freeznet commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-871840829
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #11172: [broker] fix `GetTopicsOfNamespace` with binary lookup service not check auth
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #11172:
URL: https://github.com/apache/pulsar/pull/11172#issuecomment-871872980
@freeznet does this affect docs?
--
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