[fix][broker] Only validate superuser access if authz enabled

[michaeljmarshall]

Motivation

In #19455, I added a requirement that only the proxy role could supply an original principal. That check is only supposed to apply when the broker has authorization enabled. However, in one case, that was not the case. This PR does a check and returns early when authorization is not enabled in the broker.

See #19830 (comment) for additional motivation.

Modifications

• Update the PulsarWebResource#validateSuperUserAccessAsync to only validate when authentication and authorization are enabled in the configuration.

Verifying this change

This is a trivial change. It'd be good to add tests, but I didn't include them here because this is a somewhat urgent fix. There was one test that broke because of this change, so there is at least some existing coverage.

Documentation

• doc-not-needed

Matching PR in forked repository

PR in forked repository: michaeljmarshall#39

[poorbarcode]

Since there will not have a scenario like this disabled authentication & enabled authorization, so it will work.

LGTM

[michaeljmarshall]

Since there will not have a scenario like this disabled authentication & enabled authorization, so it will work.

Correct. The broker actually fails on startup if that is the configuration:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

Lines 695 to 698 in 6621fd3

	if (config.isAuthorizationEnabled() && !config.isAuthenticationEnabled()) {
	throw new IllegalStateException("Invalid broker configuration. Authentication must be enabled with "
	+ "authenticationEnabled=true when authorization is enabled with authorizationEnabled=true.");
	}

[codelipenghui]

/pulsarbot run-failure-checks

[codelipenghui]

@michaeljmarshall Is it possible to add a test to make we will not be impacted in the future?

[poorbarcode]

/pulsarbot run-failure-checks

[michaeljmarshall]

Looks like there are some legitimate test failures. I fixed the first. The second is activeBrokerParse added by #9191. It looks like that test enables authorization, but it doesn't look configured. I think we can remove those config lines, but I'm not certain.

[liangyepianzhou]

/pulsarbot run-failure-checks

[michaeljmarshall]

The second is activeBrokerParse added by #9191. It looks like that test enables authorization, but it doesn't look configured. I think we can remove those config lines, but I'm not certain.

I decided to remove the configuration to enable authorization. It was not actually used in the previous tests and it was forcing the failure here. If we want to test authorization in that test, we need to enable authentication as well.

[Technoboy-]

/pulsarbot run-failure-checks

[michaeljmarshall]

There are more, legitimate test failures. This one is next testNamespacesApiRedirects. Interestingly, it looks like that test might have a hint as to why the activeBrokerParse was enabling authorization:

pulsar/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/NamespacesTest.java

Lines 706 to 707 in 3a069ed

	// Trick to force redirection
	conf.setAuthorizationEnabled(true);

When running the activeBrokerParse locally, it is flaky, and it appears that it passes on master when there are several 307 redirects, but fails on my branch without any of those redirects.

[michaeljmarshall]

This is the part of the code that has the "hack" described in the above comment:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

Lines 512 to 519 in 3b76dd4

	if (isValidCluster(pulsar, cluster)
	// this code should only happen with a v1 namespace format prop/cluster/namespaces
	|| pulsar.getConfiguration().getClusterName().equals(cluster)) {
	clusterDataFuture.complete(null);
	return clusterDataFuture;
	}
	// redirect to the cluster requested
	pulsar.getPulsarResources().getClusterResources().getClusterAsync(cluster)

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

Lines 536 to 547 in 3b76dd4

	static boolean isValidCluster(PulsarService pulsarService, String cluster) {// If the cluster name is
	// cluster == null or "global", don't validate the
	// cluster ownership. Cluster will be null in v2 naming.
	// The validation will be done by checking the namespace configuration
	if (cluster == null || Constants.GLOBAL_CLUSTER.equals(cluster)) {
	return true;
	}
	if (!pulsarService.getConfiguration().isAuthorizationEnabled()) {
	// Without authorization, any cluster name should be valid and accepted by the broker
	return true;
	}

[michaeljmarshall]

After discovering the hack, I noticed that the "easiest" solution is to add the check for authentication back in. I don't have time to dig into the (surprising) coupling of authorization and geo replication, so I created an issue #20023.

[michaeljmarshall]

@michaeljmarshall Is it possible to add a test to make we will not be impacted in the future?

@codelipenghui - given that it made a test fail, I think we're "covered" because I updated the test. I think the more ideal solution will be to refactor this logic so that it is in the AuthorizationService.

[codecov-commenter]

Codecov Report

Merging #19989 (04f9e98) into master (68c10ee) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master   #19989      +/-   ##
============================================
- Coverage     72.89%   72.82%   -0.08%     
+ Complexity    31619    31594      -25     
============================================
  Files          1861     1861              
  Lines        137356   137383      +27     
  Branches      15117    15123       +6     
============================================
- Hits         100131   100054      -77     
- Misses        29271    29371     +100     
- Partials       7954     7958       +4     

Flag	Coverage Δ
inttests	24.38% <0.00%> (-0.06%)	⬇️
systests	24.95% <0.00%> (-0.09%)	⬇️
unittests	72.13% <100.00%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...rg/apache/pulsar/broker/web/PulsarWebResource.java	64.34% <100.00%> (+0.39%)	⬆️

... and 102 files with indirect coverage changes

[michaeljmarshall]

I'll take care of cherry picking this later tonight. There are some merge conflicts to resolve.

[michaeljmarshall]

Cherry picking complete.