You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "hangc0276 (via GitHub)" <gi...@apache.org> on 2023/03/15 07:25:56 UTC
[GitHub] [pulsar] hangc0276 opened a new pull request, #19820: [Branch-2.11] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455)
hangc0276 opened a new pull request, #19820:
URL: https://github.com/apache/pulsar/pull/19820
This reverts commit c7eabc9391361508fc222469479bfea83d5d0c64.
### Motivation
PR #19455 introduced break change and has been cherry-picked to release branch-2.11. It will lead to users upgrading to the new release version authenticate failed.
### Modifications
<!-- Describe the modifications you've done. -->
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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 closed pull request #19820: [Branch-2.11] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455)
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall closed pull request #19820: [Branch-2.11] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455)
URL: https://github.com/apache/pulsar/pull/19820
--
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] Technoboy- closed pull request #19820: [Branch-2.11] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455)
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed pull request #19820: [Branch-2.11] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455)
URL: https://github.com/apache/pulsar/pull/19820
--
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 #19820: [Branch-2.11] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455)
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #19820:
URL: https://github.com/apache/pulsar/pull/19820#issuecomment-1469591275
We need to revert more commits related to 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] codelipenghui commented on pull request #19820: [Branch-2.11] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455)
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #19820:
URL: https://github.com/apache/pulsar/pull/19820#issuecomment-1471264003
We have an acceptable solution for now. So that we don't need to revert this change, just push a fix to the release branches.
https://lists.apache.org/thread/q1k1krfjr8njdf8t3sy1ht4vg67s4x71 the discussion thread.
And @michaeljmarshall message me in Slack, he will open PR to fix it.
Thanks @hangc0276 and @michaeljmarshall
--
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 #19820: [Branch-2.11] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455)
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #19820:
URL: https://github.com/apache/pulsar/pull/19820#issuecomment-1471360578
https://github.com/apache/pulsar/pull/19830 should supersede this PR, so I am going to close this for now.
--
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 #19820: [Branch-2.11] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455)
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19820:
URL: https://github.com/apache/pulsar/pull/19820#discussion_r1137209272
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java:
##########
@@ -292,53 +293,19 @@ public CompletableFuture<Boolean> allowSinkOpsAsync(NamespaceName namespaceName,
return provider.allowSinkOpsAsync(namespaceName, role, authenticationData);
}
- /**
- * Whether the authenticatedPrincipal and the originalPrincipal form a valid pair. This method assumes that
- * authenticatedPrincipal and originalPrincipal can be equal, as long as they are not a proxy role. This use
- * case is relvant for the admin server because of the way the proxy handles authentication. The binary protocol
- * should not use this method.
- * @return true when roles are a valid combination and false when roles are an invalid combination
- */
- public boolean isValidOriginalPrincipal(String authenticatedPrincipal,
- String originalPrincipal,
- AuthenticationDataSource authDataSource) {
- SocketAddress remoteAddress = authDataSource != null ? authDataSource.getPeerAddress() : null;
- return isValidOriginalPrincipal(authenticatedPrincipal, originalPrincipal, remoteAddress, true);
- }
-
- /**
- * Validates that the authenticatedPrincipal and the originalPrincipal are a valid combination.
- * Valid combinations fulfill one of the following two rules:
- * <p>
- * 1. The authenticatedPrincipal is in {@link ServiceConfiguration#getProxyRoles()}, if, and only if,
- * the originalPrincipal is set to a role that is not also in {@link ServiceConfiguration#getProxyRoles()}.
- * <p>
- * 2. The authenticatedPrincipal and the originalPrincipal are the same, but are not a proxyRole, when
- * allowNonProxyPrincipalsToBeEqual is true.
- *
- * @return true when roles are a valid combination and false when roles are an invalid combination
- */
- public boolean isValidOriginalPrincipal(String authenticatedPrincipal,
- String originalPrincipal,
- SocketAddress remoteAddress,
- boolean allowNonProxyPrincipalsToBeEqual) {
- String errorMsg = null;
- if (conf.getProxyRoles().contains(authenticatedPrincipal)) {
+ private static void validateOriginalPrincipal(Set<String> proxyRoles, String authenticatedPrincipal,
+ String originalPrincipal) {
+ if (proxyRoles.contains(authenticatedPrincipal)) {
+ // Request has come from a proxy
if (StringUtils.isBlank(originalPrincipal)) {
- errorMsg = "originalPrincipal must be provided when connecting with a proxy role.";
- } else if (conf.getProxyRoles().contains(originalPrincipal)) {
- errorMsg = "originalPrincipal cannot be a proxy role.";
+ log.warn("Original principal empty in request authenticated as {}", authenticatedPrincipal);
+ throw new RestException(Response.Status.UNAUTHORIZED, "Original principal cannot be empty if the "
+ + "request is via proxy.");
+ }
+ if (proxyRoles.contains(originalPrincipal)) {
+ log.warn("Original principal {} cannot be a proxy role ({})", originalPrincipal, proxyRoles);
+ throw new RestException(Response.Status.UNAUTHORIZED, "Original principal cannot be a proxy role");
}
- } else if (StringUtils.isNotBlank(originalPrincipal)
- && !(allowNonProxyPrincipalsToBeEqual && originalPrincipal.equals(authenticatedPrincipal))) {
- errorMsg = "cannot specify originalPrincipal when connecting without valid proxy role.";
- }
Review Comment:
These are the lines that are under question.
--
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