You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "rdhabalia (via GitHub)" <gi...@apache.org> on 2024/07/16 07:02:59 UTC
[PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
rdhabalia opened a new pull request, #23036:
URL: https://github.com/apache/pulsar/pull/23036
### Motivation
When Proxy and appropriate Proxy authentication is introduced in the Broker, the Proxy first authenticates the request coming from the client and retrieves its principal name, and passes it to the broker as an original-principal name along with its certificates so, the broker can authenticate the proxy with the proxy's certs and broker can also get an original-principal name in the same Connect request. Broker performs the authorization on the original-principal name and validates that proxy role exists into `ServiceConfigiration::proxyRoles`. With this auth model, the client can manage its principal-name into authorization policies and still can access broker API via proxy without any extra policy management and it works seamlessly.
However, the above auth model was implemented correctly for binary protocol but it was broken in HTTP authentication which is used for HTTP admin requests. In case of HTTP reverse proxy authentication, Broker performs authorization for both proxy's principal name and original principal name and both principal names must be present in namespace authorization policy. It means if client wants to access Pulsar-Broker via proxy then proxy's role must be added into namespace policies and if proxy server's certs are compromised then client data can be in risk. This behavior is incorrect, insecure and not compatible with existing binary authentication.
Broker should perform policy-authorization on original principal and proxy authorization should be done via proxy-roles provided into broker's configurations.
### Modifications
This PR fixes the security risk of http-proxy authentication and makes it compatible with binary protocol.
### 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 -->
- [ ] `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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "rdhabalia (via GitHub)" <gi...@apache.org>.
rdhabalia commented on PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#issuecomment-2234143702
>. We verify both the proxy and the original role permission level on the binary protocol here:
https://github.com/apache/pulsar/blob/53df683b0f78f5f7c12f87e6fbb4d73637ca5bd5/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L468-L494
No, that is incorrect as well. you can check the code and you will see that
`isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync` performs authorization on `originalPrincipal` and ProxyRole gets validated by checking if it's in proxyRole here
https://github.com/apache/pulsar/blob/branch-2.10/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L667
--
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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#discussion_r1681486689
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java:
##########
@@ -787,12 +787,7 @@ public CompletableFuture<Boolean> allowTopicOperationAsync(TopicName topicName,
return CompletableFuture.completedFuture(false);
}
if (isProxyRole(role)) {
- CompletableFuture<Boolean> isRoleAuthorizedFuture = allowTopicOperationAsync(
- topicName, operation, role, authData);
- CompletableFuture<Boolean> isOriginalAuthorizedFuture = allowTopicOperationAsync(
- topicName, operation, originalRole, authData);
- return isRoleAuthorizedFuture.thenCombine(isOriginalAuthorizedFuture,
- (isRoleAuthorized, isOriginalAuthorized) -> isRoleAuthorized && isOriginalAuthorized);
+ return allowTopicOperationAsync(topicName, operation, originalRole, authData);
Review Comment:
It's not clear to me why this PR only proposes changing this block of code. All 9 times we use the `isProxyRole` method in the `AuthorizationService` class, we validate that both the proxy's role and the `originalRole` have permission to perform a given action. @rdhabalia - why would we only modify this one?
Regarding the change itself, it was introduced in https://github.com/apache/pulsar/pull/7788. I remember reading somewhere that the idea was you could have dedicated proxy's for a given set of clients and then you could choose more granular permissions on a per proxy basis.
If you want the proxy to be a super user and want to functionally skip the permission check, just add it to the super user list.
--
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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#issuecomment-2235256804
> How does it matter? `ServiceConfiguration::proxyRoles` is a list of proxy roles and broker can have multiple proxy roles to allow access.
**`ServiceConfiguration::proxyRoles` checks if the authentication principal is a proxy role.** If you want to allow the proxy's principal to do anything, you can add the proxy's principal to `ServiceConfiguration::superUserRoles`.
Proxy cases:
- Proxy case1 - one proxy
Usually, we add the proxy's principal to the super user list and proxy role list. When do authorization, the broker will check whether both the proxy's principal(superuser) and the original principal have been authorized.
- Proxy case2 - multiple proxy
Use multiple proxy to do fine-grained permission control. Assume, there are `proxy-a` and `proxy-b` proxies, and which has been added to the proxy role list, and not exits in super user list. We need to grant the permission for the `proxy-a` and `proxy-b`, otherwise, it is not allowed to pass.
Your PR breaks the case2, and introduces the security risk:
> If I understand it correctly, it implicitly elevates all proxy roles to super user privilege level. Users can currently configure proxy roles to lesser privileges, so that would present a problem for those users.
--
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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "rdhabalia (via GitHub)" <gi...@apache.org>.
rdhabalia commented on PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#issuecomment-2232460755
> When using a proxy, the broker must check the proxy's principal name and original principal name, regardless of the protocol you are using.
Of course, yes but proxy role must not be part of the namespace policy authorization and client doesn't have to explicitly grant permission to proxy-role. Broker checks proxy role by checking authenticating proxy request and proxy's principal name must be part of `ServiceConfiguration::proxyRoles`. Binary Protocol does it correctly because that's we have introduced when it first implemented proxy and authentication. but later on someone implemented this model incorrectly for HTTP requests.
--
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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#discussion_r1680716989
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java:
##########
@@ -787,12 +787,7 @@ public CompletableFuture<Boolean> allowTopicOperationAsync(TopicName topicName,
return CompletableFuture.completedFuture(false);
}
if (isProxyRole(role)) {
- CompletableFuture<Boolean> isRoleAuthorizedFuture = allowTopicOperationAsync(
- topicName, operation, role, authData);
- CompletableFuture<Boolean> isOriginalAuthorizedFuture = allowTopicOperationAsync(
- topicName, operation, originalRole, authData);
- return isRoleAuthorizedFuture.thenCombine(isOriginalAuthorizedFuture,
- (isRoleAuthorized, isOriginalAuthorized) -> isRoleAuthorized && isOriginalAuthorized);
+ return allowTopicOperationAsync(topicName, operation, originalRole, authData);
Review Comment:
I think that the original change was made by @michaeljmarshall together with other changes with the intention to fix a CVE, possibly https://pulsar.apache.org/security/CVE-2023-30429/.
@michaeljmarshall do you have a chance to review?
--
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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#issuecomment-2232622292
> > When using a proxy, the broker must check the proxy's principal name and original principal name, regardless of the protocol you are using.
>
> Of course, yes but proxy role must not be part of the namespace policy authorization and client doesn't have to explicitly grant permission to proxy-role. Broker checks proxy role by checking authenticating proxy request and proxy's principal name must be part of `ServiceConfiguration::proxyRoles`. Binary Protocol does it correctly because that's we have introduced when it first implemented proxy and authentication. but later on someone implemented this model incorrectly for HTTP requests.
agree, could you help add a test for 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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#issuecomment-2232445203
> Broker performs authorization for both proxy's principal name and original principal name and both principal names must be present in namespace authorization policy.
When using a proxy, the broker must check the proxy's principal name and original principal name, regardless of the protocol you are using.
> It means if client wants to access Pulsar-Broker via proxy then proxy's role must be added into namespace policies and if proxy server's certs are compromised then client data can be in risk. This behavior is incorrect, insecure and not compatible with existing binary authentication.
Do you mean that the client uses the proxy's principal name?
- In the binary protocol, the broker doesn't allow the proxy's principal name and original principal name to be the same(https://github.com/apache/pulsar/pull/19455).
- In the HTTP protocol, the broker allows the proxy's principal name and original principal name to be the same( https://github.com/apache/pulsar/pull/19557).
I forgot why I approved to #19557, which maybe introduces the security risk on http-proxy authorization.
This PR changes the authorization logic, right now you only check the original principal name, and the proxy's principal name was ignored.
--
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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "rdhabalia (via GitHub)" <gi...@apache.org>.
rdhabalia commented on PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#issuecomment-2234107215
> Regarding the change itself, it was introduced in https://github.com/apache/pulsar/pull/7788.
I don't see any issue with this PR because super-user can also go via proxy and that should be taken care by above 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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#issuecomment-2234204877
> > . We verify both the proxy and the original role permission level on the binary protocol here:
> > https://github.com/apache/pulsar/blob/53df683b0f78f5f7c12f87e6fbb4d73637ca5bd5/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L468-L494
>
> No, that is incorrect as well. you can check the code and you will see that `isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync` performs authorization on `originalPrincipal` and ProxyRole gets validated by checking if it's in proxyRole here
>
> https://github.com/apache/pulsar/blob/branch-2.10/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L667
The names of the variables appear inverted, but the logic confirms that both the `originalPrincipal` and the `authRole` have permission to perform the action. The futures are chained together and the call is only authorized if both futures return `true`.
--
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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "rdhabalia (via GitHub)" <gi...@apache.org>.
rdhabalia commented on PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#issuecomment-2233828758
> the pulsar manager uses multiple proxies to control access, for example: proxy1 can produce, but not consume, and proxy2 can consume, but not produce.
How does it matter? `ServiceConfiguration::proxyRoles` is a list of proxy roles and broker can have multiple proxy roles to allow access.
--
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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "rdhabalia (via GitHub)" <gi...@apache.org>.
rdhabalia commented on PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#issuecomment-2234067526
> If you want the proxy to be a super user and want to functionally skip the permission check, just add it to the super user list.
No, that is incorrect, we are not proposing proxy-roles to be super-user. Proxy role has to be authenticated at Broker first and also bring the original principal role which must pass the authorization. Super user role doesn't have to authenticate client and doesn't have to pass original principal as well.
Here, fundamental thing is broken where client has to explicitly authorize proxy role to the namespace policy which is not secured, transparent to users and also not compatible behavior with existing binary protocol.
HTTP can't have different behavior where client has to grant permission to proxy roles as well.
Proxy is optional and must be abstract to users and users don't have to know it explicitily.
--
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
Re: [PR] [fix][broker] Broker incorrectly validates proxy role for http request [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #23036:
URL: https://github.com/apache/pulsar/pull/23036#issuecomment-2232664915
Sounds good, but if someone uses the proxy to control access, this will break the user behavior.
```
user1->proxy1-
\
topic-1
/
user2->proxy2-
```
the pulsar manager uses multiple proxies to control access, for example: proxy1 can produce, but not consume, and proxy2 can consume, but not produce.
--
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