[fix][broker] Broker incorrectly validates proxy role for http request

[rdhabalia]

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:

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

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository:

[nodece]

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([improve][broker] Require authRole is proxyRole to set originalPrincipal #19455).
• In the HTTP protocol, the broker allows the proxy's principal name and original principal name to be the same( [fix][broker] Allow proxy to pass same role for authRole and originalRole #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.

[rdhabalia]

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.

[Technoboy-]

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 ?

[nodece]

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.

[lhotari]

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?

[michaeljmarshall]

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 #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.

[rdhabalia]

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.

[michaeljmarshall]

I don't think this is a valid change. 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.

We verify both the proxy and the original role permission level on the binary protocol here:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

Lines 468 to 494 in 53df683

	private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, TopicOperation operation,
	AuthenticationDataSource authDataSource, AuthenticationDataSource originalAuthDataSource) {
	if (!service.isAuthorizationEnabled()) {
	return CompletableFuture.completedFuture(true);
	}
	CompletableFuture<Boolean> isProxyAuthorizedFuture;
	if (originalPrincipal != null) {
	isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
	topicName, operation, originalPrincipal,
	originalAuthDataSource != null ? originalAuthDataSource : authDataSource);
	} else {
	isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
	}
	CompletableFuture<Boolean> isAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
	topicName, operation, authRole, authDataSource);
	return isProxyAuthorizedFuture.thenCombine(isAuthorizedFuture, (isProxyAuthorized, isAuthorized) -> {
	if (!isProxyAuthorized) {
	log.warn("OriginalRole {} is not authorized to perform operation {} on topic {}",
	originalPrincipal, operation, topicName);
	}
	if (!isAuthorized) {
	log.warn("Role {} is not authorized to perform operation {} on topic {}",
	authRole, operation, topicName);
	}
	return isProxyAuthorized && isAuthorized;
	});
	}

If you found a way to work around that, I would consider that a vulnerability, and it would be best to discuss the details on the private mailing list to correctly identify the next steps without any accidental/premature exposure.

[michaeljmarshall]

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 #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.

[rdhabalia]

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.

[rdhabalia]

Regarding the change itself, it was introduced in #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.

[rdhabalia]

. We verify both the proxy and the original role permission level on the binary protocol here:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

Lines 468 to 494 in 53df683

	private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, TopicOperation operation,
	AuthenticationDataSource authDataSource, AuthenticationDataSource originalAuthDataSource) {
	if (!service.isAuthorizationEnabled()) {
	return CompletableFuture.completedFuture(true);
	}
	CompletableFuture<Boolean> isProxyAuthorizedFuture;
	if (originalPrincipal != null) {
	isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
	topicName, operation, originalPrincipal,
	originalAuthDataSource != null ? originalAuthDataSource : authDataSource);
	} else {
	isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
	}
	CompletableFuture<Boolean> isAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
	topicName, operation, authRole, authDataSource);
	return isProxyAuthorizedFuture.thenCombine(isAuthorizedFuture, (isProxyAuthorized, isAuthorized) -> {
	if (!isProxyAuthorized) {
	log.warn("OriginalRole {} is not authorized to perform operation {} on topic {}",
	originalPrincipal, operation, topicName);
	}
	if (!isAuthorized) {
	log.warn("Role {} is not authorized to perform operation {} on topic {}",
	authRole, operation, topicName);
	}
	return isProxyAuthorized && isAuthorized;
	});
	}

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

[michaeljmarshall]

. We verify both the proxy and the original role permission level on the binary protocol here:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

Lines 468 to 494 in 53df683

	private CompletableFuture<Boolean> isTopicOperationAllowed(TopicName topicName, TopicOperation operation,
	AuthenticationDataSource authDataSource, AuthenticationDataSource originalAuthDataSource) {
	if (!service.isAuthorizationEnabled()) {
	return CompletableFuture.completedFuture(true);
	}
	CompletableFuture<Boolean> isProxyAuthorizedFuture;
	if (originalPrincipal != null) {
	isProxyAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
	topicName, operation, originalPrincipal,
	originalAuthDataSource != null ? originalAuthDataSource : authDataSource);
	} else {
	isProxyAuthorizedFuture = CompletableFuture.completedFuture(true);
	}
	CompletableFuture<Boolean> isAuthorizedFuture = service.getAuthorizationService().allowTopicOperationAsync(
	topicName, operation, authRole, authDataSource);
	return isProxyAuthorizedFuture.thenCombine(isAuthorizedFuture, (isProxyAuthorized, isAuthorized) -> {
	if (!isProxyAuthorized) {
	log.warn("OriginalRole {} is not authorized to perform operation {} on topic {}",
	originalPrincipal, operation, topicName);
	}
	if (!isAuthorized) {
	log.warn("Role {} is not authorized to perform operation {} on topic {}",
	authRole, operation, topicName);
	}
	return isProxyAuthorized && isAuthorized;
	});
	}

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.

[nodece]

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 operating the resource, 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 operate the resource.

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.