From a0e603a3eaf822cb0c41f292ad5845ee54df0ccf Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Wed, 18 Jan 2023 00:41:59 -0600 Subject: [PATCH 01/12] [cleanup][broker] Validate originalPrincipal earlier in ServerCnx --- .../pulsar/broker/service/ServerCnx.java | 84 ++++++------------- 1 file changed, 26 insertions(+), 58 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index 89ce9d71dd7d7..d4f91a297a2d2 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -395,17 +395,32 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E ctx.close(); } - /* - * If authentication and authorization is enabled(and not sasl) and - * if the authRole is one of proxyRoles we want to enforce - * - the originalPrincipal is given while connecting - * - originalPrincipal is not blank - * - originalPrincipal is not a proxy principal + /** + * When transitioning from Connecting to Connected, this class validates that the {@link #authRole} and the + * {@link #originalPrincipal} are a valid combination. Valid combinations fulfill the following rule: + *

+ * The {@link #authRole} is in {@link #proxyRoles}, if, and only if, the {@link #originalPrincipal} is set to a role + * that is not also in {@link #proxyRoles}. */ - private boolean invalidOriginalPrincipal(String originalPrincipal) { - return (service.isAuthenticationEnabled() && service.isAuthorizationEnabled() - && proxyRoles.contains(authRole) && (StringUtils.isBlank(originalPrincipal) - || proxyRoles.contains(originalPrincipal))); + private void validateRoleAndOriginalPrincipal() { + String errorMsg = null; + if (proxyRoles.contains(authRole)) { + if (StringUtils.isBlank(originalPrincipal)) { + errorMsg = "originalPrincipal must be provided when connecting with a proxy role."; + } else if (proxyRoles.contains(originalPrincipal)) { + errorMsg = "originalPrincipal cannot be a proxy role."; + } + } else if (StringUtils.isNotBlank(originalPrincipal)) { + errorMsg = "cannot specify originalPrincipal when connecting without valid proxy role."; + } + if (errorMsg != null) { + service.getPulsarStats().recordConnectionCreateFail(); + log.warn("[{}] Illegal combination of role [{}] and originalPrincipal {}: {}", remoteAddress, authRole, + originalPrincipal, errorMsg); + // Provide generic error message to prevent leaking information about proxy roles + writeAndFlush(Commands.newError(-1, ServerError.AuthorizationError, "Unable to authenticate")); + close(); + } } // //// @@ -487,14 +502,6 @@ protected void handleLookup(CommandLookupTopic lookup) { final Semaphore lookupSemaphore = service.getLookupRequestSemaphore(); if (lookupSemaphore.tryAcquire()) { - if (invalidOriginalPrincipal(originalPrincipal)) { - final String msg = "Valid Proxy Client role should be provided for lookup "; - log.warn("[{}] {} with role {} and proxyClientAuthRole {} on topic {}", remoteAddress, msg, authRole, - originalPrincipal, topicName); - writeAndFlush(newLookupErrorResponse(ServerError.AuthorizationError, msg, requestId)); - lookupSemaphore.release(); - return; - } isTopicOperationAllowed(topicName, TopicOperation.LOOKUP, authenticationData, originalAuthData).thenApply( isAuthorized -> { if (isAuthorized) { @@ -568,14 +575,6 @@ protected void handlePartitionMetadataRequest(CommandPartitionedTopicMetadata pa final Semaphore lookupSemaphore = service.getLookupRequestSemaphore(); if (lookupSemaphore.tryAcquire()) { - if (invalidOriginalPrincipal(originalPrincipal)) { - final String msg = "Valid Proxy Client role should be provided for getPartitionMetadataRequest "; - log.warn("[{}] {} with role {} and proxyClientAuthRole {} on topic {}", remoteAddress, msg, authRole, - originalPrincipal, topicName); - commandSender.sendPartitionMetadataResponse(ServerError.AuthorizationError, msg, requestId); - lookupSemaphore.release(); - return; - } isTopicOperationAllowed(topicName, TopicOperation.LOOKUP, authenticationData, originalAuthData).thenApply( isAuthorized -> { if (isAuthorized) { @@ -965,6 +964,7 @@ protected void handleConnect(CommandConnect connect) { remoteAddress, originalPrincipal); } } + validateRoleAndOriginalPrincipal(); } catch (Exception e) { service.getPulsarStats().recordConnectionCreateFail(); logAuthException(remoteAddress, "connect", getPrincipal(), Optional.empty(), e); @@ -1019,14 +1019,6 @@ protected void handleSubscribe(final CommandSubscribe subscribe) { remoteAddress, authRole, originalPrincipal); } - if (invalidOriginalPrincipal(originalPrincipal)) { - final String msg = "Valid Proxy Client role should be provided while subscribing "; - log.warn("[{}] {} with role {} and proxyClientAuthRole {} on topic {}", remoteAddress, msg, authRole, - originalPrincipal, topicName); - commandSender.sendErrorResponse(requestId, ServerError.AuthorizationError, msg); - return; - } - final String subscriptionName = subscribe.getSubscription(); final SubType subType = subscribe.getSubType(); final String consumerName = subscribe.hasConsumerName() ? subscribe.getConsumerName() : ""; @@ -1272,14 +1264,6 @@ protected void handleProducer(final CommandProducer cmdProducer) { return; } - if (invalidOriginalPrincipal(originalPrincipal)) { - final String msg = "Valid Proxy Client role should be provided while creating producer "; - log.warn("[{}] {} with role {} and proxyClientAuthRole {} on topic {}", remoteAddress, msg, authRole, - originalPrincipal, topicName); - commandSender.sendErrorResponse(requestId, ServerError.AuthorizationError, msg); - return; - } - CompletableFuture isAuthorizedFuture = isTopicOperationAllowed( topicName, TopicOperation.PRODUCE, authenticationData, originalAuthData ); @@ -2120,14 +2104,6 @@ protected void handleGetTopicsOfNamespace(CommandGetTopicsOfNamespace commandGet final Semaphore lookupSemaphore = service.getLookupRequestSemaphore(); if (lookupSemaphore.tryAcquire()) { - if (invalidOriginalPrincipal(originalPrincipal)) { - final String msg = "Valid Proxy Client role should be provided for getTopicsOfNamespaceRequest "; - log.warn("[{}] {} with role {} and proxyClientAuthRole {} on namespace {}", remoteAddress, msg, - authRole, originalPrincipal, namespaceName); - commandSender.sendErrorResponse(requestId, ServerError.AuthorizationError, msg); - lookupSemaphore.release(); - return; - } isNamespaceOperationAllowed(namespaceName, NamespaceOperation.GET_TOPICS).thenApply(isAuthorized -> { if (isAuthorized) { getBrokerService().pulsar().getNamespaceService().getListOfTopics(namespaceName, mode) @@ -2686,14 +2662,6 @@ protected void handleCommandWatchTopicList(CommandWatchTopicList commandWatchTop final Semaphore lookupSemaphore = service.getLookupRequestSemaphore(); if (lookupSemaphore.tryAcquire()) { - if (invalidOriginalPrincipal(originalPrincipal)) { - final String msg = "Valid Proxy Client role should be provided for watchTopicListRequest "; - log.warn("[{}] {} with role {} and proxyClientAuthRole {} on namespace {}", remoteAddress, msg, - authRole, originalPrincipal, namespaceName); - commandSender.sendErrorResponse(watcherId, ServerError.AuthorizationError, msg); - lookupSemaphore.release(); - return; - } isNamespaceOperationAllowed(namespaceName, NamespaceOperation.GET_TOPICS).thenApply(isAuthorized -> { if (isAuthorized) { topicListService.handleWatchTopicList(namespaceName, watcherId, requestId, topicsPattern, From 214e1ecb8ab6547351efdb210fc1e4b93fa02c0b Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Wed, 18 Jan 2023 11:06:57 -0600 Subject: [PATCH 02/12] Only validate roles when authorization is enabled --- .../main/java/org/apache/pulsar/broker/service/ServerCnx.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index d4f91a297a2d2..9c2429feb98a4 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -964,7 +964,9 @@ protected void handleConnect(CommandConnect connect) { remoteAddress, originalPrincipal); } } - validateRoleAndOriginalPrincipal(); + if (service.isAuthorizationEnabled()) { + validateRoleAndOriginalPrincipal(); + } } catch (Exception e) { service.getPulsarStats().recordConnectionCreateFail(); logAuthException(remoteAddress, "connect", getPrincipal(), Optional.empty(), e); From c571a4b7b550a9636dc53c455189797f830bc2c0 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Wed, 18 Jan 2023 11:10:58 -0600 Subject: [PATCH 03/12] Create method for duplicated code --- .../apache/pulsar/broker/service/ServerCnx.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index 9c2429feb98a4..a45c46f019dcd 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -414,12 +414,9 @@ private void validateRoleAndOriginalPrincipal() { errorMsg = "cannot specify originalPrincipal when connecting without valid proxy role."; } if (errorMsg != null) { - service.getPulsarStats().recordConnectionCreateFail(); log.warn("[{}] Illegal combination of role [{}] and originalPrincipal {}: {}", remoteAddress, authRole, originalPrincipal, errorMsg); - // Provide generic error message to prevent leaking information about proxy roles - writeAndFlush(Commands.newError(-1, ServerError.AuthorizationError, "Unable to authenticate")); - close(); + closeWithAuthenticationException(); } } @@ -968,14 +965,18 @@ protected void handleConnect(CommandConnect connect) { validateRoleAndOriginalPrincipal(); } } catch (Exception e) { - service.getPulsarStats().recordConnectionCreateFail(); logAuthException(remoteAddress, "connect", getPrincipal(), Optional.empty(), e); - String msg = "Unable to authenticate"; - writeAndFlush(Commands.newError(-1, ServerError.AuthenticationError, msg)); - close(); + closeWithAuthenticationException(); } } + private void closeWithAuthenticationException() { + service.getPulsarStats().recordConnectionCreateFail(); + String msg = "Unable to authenticate"; + writeAndFlush(Commands.newError(-1, ServerError.AuthenticationError, msg)); + close(); + } + @Override protected void handleAuthResponse(CommandAuthResponse authResponse) { checkArgument(authResponse.hasResponse()); From 6dc119fc629f230625170c46ee4e4bc4d2dd50d8 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Wed, 18 Jan 2023 11:36:05 -0600 Subject: [PATCH 04/12] Improve method name --- .../java/org/apache/pulsar/broker/service/ServerCnx.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index a45c46f019dcd..3addf4154da89 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -416,7 +416,7 @@ private void validateRoleAndOriginalPrincipal() { if (errorMsg != null) { log.warn("[{}] Illegal combination of role [{}] and originalPrincipal {}: {}", remoteAddress, authRole, originalPrincipal, errorMsg); - closeWithAuthenticationException(); + closeForAuthenticationError(); } } @@ -966,11 +966,11 @@ protected void handleConnect(CommandConnect connect) { } } catch (Exception e) { logAuthException(remoteAddress, "connect", getPrincipal(), Optional.empty(), e); - closeWithAuthenticationException(); + closeForAuthenticationError(); } } - private void closeWithAuthenticationException() { + private void closeForAuthenticationError() { service.getPulsarStats().recordConnectionCreateFail(); String msg = "Unable to authenticate"; writeAndFlush(Commands.newError(-1, ServerError.AuthenticationError, msg)); From 6d67bdf28af42e1242d111e6d3a4e7e0df1a6b67 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Wed, 18 Jan 2023 12:28:31 -0600 Subject: [PATCH 05/12] Fix log format --- .../main/java/org/apache/pulsar/broker/service/ServerCnx.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index 3addf4154da89..701813ba857e6 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -414,7 +414,7 @@ private void validateRoleAndOriginalPrincipal() { errorMsg = "cannot specify originalPrincipal when connecting without valid proxy role."; } if (errorMsg != null) { - log.warn("[{}] Illegal combination of role [{}] and originalPrincipal {}: {}", remoteAddress, authRole, + log.warn("[{}] Illegal combination of role [{}] and originalPrincipal [{}]: {}", remoteAddress, authRole, originalPrincipal, errorMsg); closeForAuthenticationError(); } From ebe5411b4437501bcd637bff6d8ebf16c95a2ee1 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Wed, 18 Jan 2023 13:22:25 -0600 Subject: [PATCH 06/12] Add test to cover all invalid role combinations --- .../pulsar/broker/service/ServerCnx.java | 1 + .../pulsar/broker/service/ServerCnxTest.java | 78 +++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index 701813ba857e6..910592488d46c 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -971,6 +971,7 @@ protected void handleConnect(CommandConnect connect) { } private void closeForAuthenticationError() { + state = State.Failed; service.getPulsarStats().recordConnectionCreateFail(); String msg = "Unable to authenticate"; writeAndFlush(Commands.newError(-1, ServerError.AuthenticationError, msg)); diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java index 1a98822340fc3..2489a7784f12e 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java @@ -435,6 +435,55 @@ public void testConnectCommandWithAuthenticationPositive() throws Exception { channel.finish(); } + @Test(timeOut = 30000) + public void testConnectCommandWithInvalidRoleCombinations() throws Exception { + AuthenticationService authenticationService = mock(AuthenticationService.class); + AuthenticationProvider authenticationProvider = new BasicCommandAuthenticationProvider(); + String authMethodName = authenticationProvider.getAuthMethodName(); + + when(brokerService.getAuthenticationService()).thenReturn(authenticationService); + when(authenticationService.getAuthenticationProvider(authMethodName)).thenReturn(authenticationProvider); + svcConfig.setAuthenticationEnabled(true); + svcConfig.setAuthorizationEnabled(true); + svcConfig.setProxyRoles(Collections.singleton("proxy")); + + resetChannel(); + assertTrue(channel.isActive()); + assertEquals(serverCnx.getState(), State.Start); + + // Invalid combinations where authData is proxy role + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy", "proxy"); + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy", ""); + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy", null); + // Invalid combinations where original principal is set to a proxy role + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "", "proxy"); + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, null, "proxy"); + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "client", "proxy"); + // Invalid combinations where the original principal is set to a non-proxy role + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "", "some_user"); + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, null, "some_user"); + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "client", "some_user"); + } + + private void verifyAuthRoleAndOriginalPrincipalBehavior(String authMethodName, String authData, + String originalPrincipal) throws Exception { + resetChannel(); + assertTrue(channel.isActive()); + assertEquals(serverCnx.getState(), State.Start); + + ByteBuf clientCommand = Commands.newConnect(authMethodName, authData, 1,null, + null, originalPrincipal, null, null); + channel.writeInbound(clientCommand); + + Object response1 = getResponse(); + assertTrue(response1 instanceof CommandConnected); + Object response2 = getResponse(); + assertTrue(response2 instanceof CommandError); + assertEquals(((CommandError) response2).getMessage(), "Unable to authenticate"); + assertEquals(serverCnx.getState(), State.Failed); + channel.finish(); + } + @Test(timeOut = 30000) public void testConnectCommandWithAuthenticationNegative() throws Exception { AuthenticationService authenticationService = mock(AuthenticationService.class); @@ -2600,4 +2649,33 @@ public void sendEndTxnOnSubscriptionFailed() throws Exception { channel.finish(); } + /** + * Used to verify certain ServerCnx handling of roles, so the authentication component is essentially bypassed. + */ + public static class BasicCommandAuthenticationProvider implements AuthenticationProvider { + + @Override + public void close() throws IOException { + } + + @Override + public void initialize(ServiceConfiguration config) throws IOException { + } + + @Override + public String getAuthMethodName() { + return "BasicAuthentication"; + } + + @Override + public String authenticate(AuthenticationDataSource authData) { + if (authData.hasDataFromCommand()) { + // Because we're only using this to verify the ServerCnx, there is no need to verify any other + // kind of data. + return authData.getCommandData(); + } + return null; + } + } + } From 964ec39a204f863346711ff72ee2867d768fc817 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Sun, 5 Feb 2023 00:41:31 -0600 Subject: [PATCH 07/12] Fix implementation in light of changes to master --- .../pulsar/broker/service/ServerCnx.java | 24 ++++--- .../pulsar/broker/service/ServerCnxTest.java | 62 ++++--------------- 2 files changed, 26 insertions(+), 60 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index d96b49201439a..abfb71eb39830 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -403,8 +403,9 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E *

* The {@link #authRole} is in {@link #proxyRoles}, if, and only if, the {@link #originalPrincipal} is set to a role * that is not also in {@link #proxyRoles}. + * @return true when roles are valid and false when roles are invalid */ - private void validateRoleAndOriginalPrincipal() { + private boolean isValidRoleAndOriginalPrincipal() { String errorMsg = null; if (proxyRoles.contains(authRole)) { if (StringUtils.isBlank(originalPrincipal)) { @@ -418,7 +419,9 @@ private void validateRoleAndOriginalPrincipal() { if (errorMsg != null) { log.warn("[{}] Illegal combination of role [{}] and originalPrincipal [{}]: {}", remoteAddress, authRole, originalPrincipal, errorMsg); - closeForAuthenticationError(); + return false; + } else { + return true; } } @@ -689,6 +692,15 @@ ByteBuf createConsumerStatsResponse(Consumer consumer, long requestId) { // complete the connect and sent newConnected command private void completeConnect(int clientProtoVersion, String clientVersion) { + if (service.isAuthorizationEnabled()) { + if (!isValidRoleAndOriginalPrincipal()) { + state = State.Failed; + service.getPulsarStats().recordConnectionCreateFail(); + final ByteBuf msg = Commands.newError(-1, ServerError.AuthorizationError, "Invalid roles."); + NettyChannelUtil.writeAndFlushWithClosePromise(ctx, msg); + return; + } + } writeAndFlush(Commands.newConnected(clientProtoVersion, maxMessageSize, enableSubscriptionPatternEvaluation)); state = State.Connected; service.getPulsarStats().recordConnectionCreateSuccess(); @@ -1024,14 +1036,6 @@ protected void handleConnect(CommandConnect connect) { } } - private void closeForAuthenticationError() { - state = State.Failed; - service.getPulsarStats().recordConnectionCreateFail(); - String msg = "Unable to authenticate"; - writeAndFlush(Commands.newError(-1, ServerError.AuthenticationError, msg)); - close(); - } - @Override protected void handleAuthResponse(CommandAuthResponse authResponse) { checkArgument(authResponse.hasResponse()); diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java index d8b4a31357d8e..6be8e915900d3 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java @@ -479,31 +479,24 @@ public void testConnectCommandWithPassingOriginalPrincipal() throws Exception { @Test(timeOut = 30000) public void testConnectCommandWithInvalidRoleCombinations() throws Exception { AuthenticationService authenticationService = mock(AuthenticationService.class); - AuthenticationProvider authenticationProvider = new BasicCommandAuthenticationProvider(); + AuthenticationProvider authenticationProvider = new MockAuthenticationProvider(); String authMethodName = authenticationProvider.getAuthMethodName(); when(brokerService.getAuthenticationService()).thenReturn(authenticationService); when(authenticationService.getAuthenticationProvider(authMethodName)).thenReturn(authenticationProvider); svcConfig.setAuthenticationEnabled(true); + svcConfig.setAuthenticateOriginalAuthData(false); svcConfig.setAuthorizationEnabled(true); - svcConfig.setProxyRoles(Collections.singleton("proxy")); - - resetChannel(); - assertTrue(channel.isActive()); - assertEquals(serverCnx.getState(), State.Start); + svcConfig.setProxyRoles(Collections.singleton("pass.proxy")); // Invalid combinations where authData is proxy role - verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy", "proxy"); - verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy", ""); - verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy", null); - // Invalid combinations where original principal is set to a proxy role - verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "", "proxy"); - verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, null, "proxy"); - verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "client", "proxy"); + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "pass.proxy", "pass.proxy"); + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "pass.proxy", ""); + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "pass.proxy", null); + // Invalid combinations where original principal is set to a pass.proxy role + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "pass.client", "pass.proxy"); // Invalid combinations where the original principal is set to a non-proxy role - verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "", "some_user"); - verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, null, "some_user"); - verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "client", "some_user"); + verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "pass.client1", "pass.client"); } private void verifyAuthRoleAndOriginalPrincipalBehavior(String authMethodName, String authData, @@ -516,11 +509,9 @@ private void verifyAuthRoleAndOriginalPrincipalBehavior(String authMethodName, S null, originalPrincipal, null, null); channel.writeInbound(clientCommand); - Object response1 = getResponse(); - assertTrue(response1 instanceof CommandConnected); - Object response2 = getResponse(); - assertTrue(response2 instanceof CommandError); - assertEquals(((CommandError) response2).getMessage(), "Unable to authenticate"); + Object response = getResponse(); + assertTrue(response instanceof CommandError); + assertEquals(((CommandError) response).getError(), ServerError.AuthorizationError); assertEquals(serverCnx.getState(), State.Failed); channel.finish(); } @@ -3108,33 +3099,4 @@ public void sendEndTxnOnSubscriptionFailed() throws Exception { channel.finish(); } - /** - * Used to verify certain ServerCnx handling of roles, so the authentication component is essentially bypassed. - */ - public static class BasicCommandAuthenticationProvider implements AuthenticationProvider { - - @Override - public void close() throws IOException { - } - - @Override - public void initialize(ServiceConfiguration config) throws IOException { - } - - @Override - public String getAuthMethodName() { - return "BasicAuthentication"; - } - - @Override - public String authenticate(AuthenticationDataSource authData) { - if (authData.hasDataFromCommand()) { - // Because we're only using this to verify the ServerCnx, there is no need to verify any other - // kind of data. - return authData.getCommandData(); - } - return null; - } - } - } From 57ebb9348b8efc6c613f957dcb63c429bab45e30 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Sun, 5 Feb 2023 00:46:28 -0600 Subject: [PATCH 08/12] Only verify when both authn and authz enabled --- .../main/java/org/apache/pulsar/broker/service/ServerCnx.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index abfb71eb39830..543c48b34a17a 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -692,7 +692,7 @@ ByteBuf createConsumerStatsResponse(Consumer consumer, long requestId) { // complete the connect and sent newConnected command private void completeConnect(int clientProtoVersion, String clientVersion) { - if (service.isAuthorizationEnabled()) { + if (service.isAuthenticationEnabled() && service.isAuthorizationEnabled()) { if (!isValidRoleAndOriginalPrincipal()) { state = State.Failed; service.getPulsarStats().recordConnectionCreateFail(); From 10b526ed84435cd1ead49dd5c13ff0d6d55c9de5 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Sun, 5 Feb 2023 00:52:33 -0600 Subject: [PATCH 09/12] Make the logic in this PR equivalent to current logic --- .../org/apache/pulsar/broker/service/ServerCnx.java | 11 ++++------- .../apache/pulsar/broker/service/ServerCnxTest.java | 4 ---- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index 543c48b34a17a..4c4dfc9d8d86d 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -398,11 +398,10 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E } /** - * When transitioning from Connecting to Connected, this class validates that the {@link #authRole} and the - * {@link #originalPrincipal} are a valid combination. Valid combinations fulfill the following rule: - *

- * The {@link #authRole} is in {@link #proxyRoles}, if, and only if, the {@link #originalPrincipal} is set to a role - * that is not also in {@link #proxyRoles}. + * When transitioning from Connecting to Connected, this method validates that if the authRole is one of proxyRoles, + * - the originalPrincipal is given while connecting + * - originalPrincipal is not blank + * - originalPrincipal is not a proxy principal * @return true when roles are valid and false when roles are invalid */ private boolean isValidRoleAndOriginalPrincipal() { @@ -413,8 +412,6 @@ private boolean isValidRoleAndOriginalPrincipal() { } else if (proxyRoles.contains(originalPrincipal)) { errorMsg = "originalPrincipal cannot be a proxy role."; } - } else if (StringUtils.isNotBlank(originalPrincipal)) { - errorMsg = "cannot specify originalPrincipal when connecting without valid proxy role."; } if (errorMsg != null) { log.warn("[{}] Illegal combination of role [{}] and originalPrincipal [{}]: {}", remoteAddress, authRole, diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java index 6be8e915900d3..7e17743a8d442 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java @@ -493,10 +493,6 @@ public void testConnectCommandWithInvalidRoleCombinations() throws Exception { verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "pass.proxy", "pass.proxy"); verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "pass.proxy", ""); verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "pass.proxy", null); - // Invalid combinations where original principal is set to a pass.proxy role - verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "pass.client", "pass.proxy"); - // Invalid combinations where the original principal is set to a non-proxy role - verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "pass.client1", "pass.client"); } private void verifyAuthRoleAndOriginalPrincipalBehavior(String authMethodName, String authData, From 73ac9bd36f59b09521aee860082938a231c55796 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Sun, 5 Feb 2023 01:00:26 -0600 Subject: [PATCH 10/12] Add back unnecessarily removed whitespace --- .../main/java/org/apache/pulsar/broker/service/ServerCnx.java | 1 + 1 file changed, 1 insertion(+) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index 4c4dfc9d8d86d..48b9712c0ca64 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -1027,6 +1027,7 @@ protected void handleConnect(CommandConnect connect) { remoteAddress, originalPrincipal); } } + doAuthentication(clientData, false, clientProtocolVersion, clientVersion); } catch (Exception e) { authenticationFailed(e); From 5245739e94a3979042bd7776fc7c538096e566f4 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Sun, 5 Feb 2023 01:14:43 -0600 Subject: [PATCH 11/12] Fix Javadoc for checkstyle --- .../java/org/apache/pulsar/broker/service/ServerCnx.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index 48b9712c0ca64..6b891c52d9049 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -398,10 +398,11 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E } /** - * When transitioning from Connecting to Connected, this method validates that if the authRole is one of proxyRoles, + * When transitioning from Connecting to Connected, this method validates the roles. + * Tf the authRole is one of proxyRoles, the following must be true: * - the originalPrincipal is given while connecting * - originalPrincipal is not blank - * - originalPrincipal is not a proxy principal + * - originalPrincipal is not a proxy principal. * @return true when roles are valid and false when roles are invalid */ private boolean isValidRoleAndOriginalPrincipal() { From 284b64596fc0486346b960476fac144796271155 Mon Sep 17 00:00:00 2001 From: Michael Marshall Date: Sun, 5 Feb 2023 01:15:08 -0600 Subject: [PATCH 12/12] Fix typo --- .../main/java/org/apache/pulsar/broker/service/ServerCnx.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java index 6b891c52d9049..988f7d34e9916 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java @@ -399,7 +399,7 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E /** * When transitioning from Connecting to Connected, this method validates the roles. - * Tf the authRole is one of proxyRoles, the following must be true: + * If the authRole is one of proxyRoles, the following must be true: * - the originalPrincipal is given while connecting * - originalPrincipal is not blank * - originalPrincipal is not a proxy principal.