[feat][broker] PIP 97: Implement for ServerCnx

[michaeljmarshall]

PIP: #12105
Fixes: #19311

Motivation

Implement asynchronous authentication for the ServerCnx class.

Modifications

• Update authentication of primary auth data to use the authenticateAsync method instead of authenticate. The callbacks are all handled on the context's event loop to ensure thread safety.
• Update the order in which the ServerCnx validates proxied authentication data. The issue is described here [Broker] ServerCnx should not respond with Success until verifying originalAuthData #19311. Now, we authenticate the proxy's auth data, then the client's auth data, and then reply with the Connected command.
• Add failure scenario to the ServerCnx so that it correctly fails authentication when a proxy's original auth method triggers multistaged authentication.
• Clean up error handling. This results in some slightly different error messages to clients when authentication fails. There is no real contract here, so I think this is a safe change.

Verifying this change

Several new tests are added and some existing tests are updated.

Does this pull request potentially affect one of the following parts:

• Send an error to the proxy if the client's auth data triggers an auth challenge.
• Change certain protocol string messages when authentication fails.

Documentation

• doc-not-needed

Matching PR in forked repository

PR in forked repository: michaeljmarshall#23

[michaeljmarshall]

Looks like PersistentTopicTest#testGetReplicationClusters is failing. It hasn't been reported as a flaky test, but it seems unrelated to my changes. I'll look closer tomorrow.

[lhotari]

Good work @michaeljmarshall

[lhotari]

@michaeljmarshall is this a real failure? https://github.com/apache/pulsar/actions/runs/4085441211/jobs/7043646144#step:11:1102

  Error:  Tests run: 14, Failures: 1, Errors: 0, Skipped: 13, Time elapsed: 0.872 s <<< FAILURE! - in org.apache.pulsar.broker.service.ServerCnxAuthorizationTest
  Error:  testVerifyAuthRoleAndAuthDataFromDirectConnectionBroker(org.apache.pulsar.broker.service.ServerCnxAuthorizationTest)  Time elapsed: 0.063 s  <<< FAILURE!
  java.lang.NullPointerException: Cannot invoke "io.netty.channel.ChannelFuture.addListener(io.netty.util.concurrent.GenericFutureListener)" because the return value of "io.netty.channel.ChannelOutboundInvoker.writeAndFlush(Object)" is null
  	at org.apache.pulsar.common.util.netty.NettyChannelUtil.writeAndFlushWithClosePromise(NettyChannelUtil.java:60)
  	at org.apache.pulsar.broker.service.ServerCnx.authenticationFailed(ServerCnx.java:838)
  	at org.apache.pulsar.broker.service.ServerCnx.handleConnect(ServerCnx.java:1027)
  	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
  	at java.base/java.lang.Thread.run(Thread.java:833)
  

[lhotari]

@michaeljmarshall The test class ServerCnxAuthorizationTest is doing extensive mocking of Netty classes, which is a bad solution. The ServerCnxTest has a better solution based on a real class, EmbeddedChannel . Article: https://www.baeldung.com/testing-netty-embedded-channel

pulsar/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java

Lines 1771 to 1786 in 165ee96

	protected void resetChannel() throws Exception {
	int MaxMessageSize = 5 * 1024 * 1024;
	if (channel != null && channel.isActive()) {
	serverCnx.close();
	channel.close().get();
	}
	serverCnx = new ServerCnx(pulsar);
	serverCnx.setAuthRole("");
	channel = new EmbeddedChannel(new LengthFieldBasedFrameDecoder(
	MaxMessageSize,
	0,
	4,
	0,
	4),
	(ChannelHandler) serverCnx);
	}

Instead of adding more mocking to ServerCnxAuthorizationTest, I think it's better to refactor it to use EmbeddedChannel.

[michaeljmarshall]

@lhotari - I completely agree that we should just move the test to the ServerCnxTest. I am working on that now.

[michaeljmarshall]

I can't seem to run the TestPulsarSQLAuth suite locally. I see errors related to missing certificates. Instead of troubleshooting, I am pushed changes that should get the tests into a better spot.

[michaeljmarshall]

Looks like those changes made the SQL integration tests pass. I am pushing one more commit to add a test that explicitly covers the bug that made the SQL tests fail 539ad03.

[lhotari]

I can't seem to run the TestPulsarSQLAuth suite locally. I see errors related to missing certificates. Instead of troubleshooting, I am pushed changes that should get the tests into a better spot.

The missing cert issue gets fixed by running the maven build (for core-modules) once. It copies the certs to the expected location somewhere.