[fix][authentication] Store the original auth when using anonymous role #19390

nodece · 2023-02-02T07:52:42Z

Motivation

When anonymousUserRole is configured, and auth method is none, the broker only stores the authRole, ignores store the original authentication data and role forwarded by the proxy, see

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

Lines 898 to 904 in b3432f4

    
           if (authenticationProvider == null) { 
        
               authRole = getBrokerService().getAuthenticationService().getAnonymousUserRole() 
        
                   .orElseThrow(() -> 
        
                       new AuthenticationException("No anonymous role, and no authentication provider configured")); 
        
               completeConnect(clientProtocolVersion, clientVersion, enableSubscriptionPatternEvaluation); 
        
               return; 
        
           }

We have to store the forwarded authentication data and role, the data from CommandConnect#getOriginalAuthData, and the role from CommandConnect#getOriginalPrincipal.

Modifications

Add checkAndStoreOriginalAuthDataForwardedByProxy method to check and store the original authentication
When using anonymous role, we also check and store the original authentication

Verifying this change

Make sure that the change passes the CI checks.

Added ProxyAnonymousRoleTest test.

Documentation

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

codelipenghui · 2023-02-03T06:49:59Z

pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAnonymousRoleTest.java

+import org.testng.annotations.Test;
+
+@Slf4j
+public class ProxyAnonymousRoleTest extends ProducerConsumerBase {


Please add a test group.

codelipenghui · 2023-02-03T06:58:04Z

pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAnonymousRoleTest.java

+        conf.getProperties().setProperty("tokenSecretKey", "data:;base64," + Base64.getEncoder().encodeToString(SECRET_KEY.getEncoded()));
+
+        Set<String> superUserRoles = new HashSet<>();
+        superUserRoles.add(ANONYMOUS_ROLE);


Is it required?

This means the proxy is a superuser. Without this config, the proxy has no permission to communicate.

codelipenghui · 2023-02-03T07:04:57Z

pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAnonymousRoleTest.java

+    private void createAdminClient() throws Exception {
+        admin = spy(PulsarAdmin.builder().serviceHttpUrl(brokerUrl.toString())
+                .authentication(AuthenticationFactory.token(ADMIN_TOKEN)).build());
+    }


The admin client will not work with proxy for this case?

Any address will do.

michaeljmarshall

I don't think we should let the proxy authenticate using an anonymous role. I started work in #19270 to propose that we make stricter requirements so that when a connection is started by a proxy, the authRole must be a proxy role (we know because the command has the originalPrincipal and maybe the originalAuthData). That work is only a draft because of some edge cases that will be resolved by #19409.

I propose that we consider a proxy connecting as the anonymous role as a misconfiguration.

codelipenghui · 2023-02-06T01:54:12Z

I don't think we should let the proxy authenticate using an anonymous role. I started work in #19270 to propose that we make stricter requirements so that when a connection is started by a proxy, the authRole must be a proxy role (we know because the command has the originalPrincipal and maybe the originalAuthData). That work is only a draft because of some edge cases that will be resolved by #19409.

I propose that we consider a proxy connecting as the anonymous role as a misconfiguration.

Sound good to me.

nodece · 2023-02-06T03:25:01Z

I don't think we should let the proxy authenticate using an anonymous role.

@michaeljmarshall I also agree with this. But I think there is a bug here, when the proxy is an anonymous user, we don't care about storing the original role and original authentication data, so this PR is used to fix this.

#19270 is validating the auth role and proxy role to avoid the auth role as the proxy role, the two PR have different goal.

pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAnonymousRoleTest.java

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

michaeljmarshall

I'd like to discuss taking a different direction. I think we should fail the connection if the proxy connects without a valid authMethod. My proposal is in the comments.

michaeljmarshall · 2023-02-07T20:54:54Z

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

@@ -971,6 +973,7 @@ protected void handleConnect(CommandConnect connect) {
                authRole = getBrokerService().getAuthenticationService().getAnonymousUserRole()


Why not just add the following before the call to get the anonymous role? I think this would correctly handle the issue of a proxy authenticating as the anonymous role.

if (connect.hasOriginalPrincipal() || connect.hasOriginalAuthData()) { throw new AuthenticationException("Proxy cannot authenticate as the anonymous role."); }

This means that the proxy cannot use the anonymous role to connect to the broker, I don't think this is correct.

Usually, the proxy always enable the authentication feature, but we also should consider a case that the proxy doesn't enable the authentication feature.

This means that the proxy cannot use the anonymous role to connect to the broker

Yes, this is the design I am proposing we move towards. That is why I said this earlier in this PR:

I propose that we consider a proxy connecting as the anonymous role as a misconfiguration.

Usually, the proxy always enable the authentication feature, but we also should consider a case that the proxy doesn't enable the authentication feature.

In this case, the broker wouldn't enable authentication either. In the admin API, we only look for the original principal when the authRole is in the proxyRoles set. Given that the proxy has to have at least as much permission as the originalPrincipal, I do not see what the point of authentication would be if the anonymous role is also a proxy role.

Your idea is very true, we also recommend enabling authentication anywhere.

We have a case in which we don't want to check the proxy authentication in the broker, only check the original authentication. Would you have any idea?

Right now, we set up anonymousUserRole, but we cannot get the original authentication, because of the broker jumped to completeConnect method, there is misses storing the original authentication.

We have a case in which we don't want to check the proxy authentication in the broker, only check the original authentication. Would you have any idea?

That context helps a lot, thanks! Is it possible to add a configuration to the proxy so that it forwards the original authentication data as the authData part of the Connect command? The broker wouldn't necessarily know the connection is from the proxy, but I think this should be fine.

I think this discussion probably relates to #19332. I have been thinking that our authentication state integration between proxy and broker is very complicated and doesn't appear to be working in all cases (see also #19291).

It seems like we could always drop the proxy auth data in cases where the authentication data can be forwarded.

Sounds good, we need to improve the proxy codebase to find a workaround, this is going to take some time.

michaeljmarshall · 2023-02-07T20:57:41Z

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

                authRole = getBrokerService().getAuthenticationService().getAnonymousUserRole()
                    .orElseThrow(() ->
                        new AuthenticationException("No anonymous role, and no authentication provider configured"));
+                checkAndStoreOriginalAuthDataForwardedByProxy(connect);


My concern with this solution is that it lets a proxy connect as the anonymous role, and I don't think that will work correctly. Note that we do not use the anonymousRole for the originalPrincipal, so it seems to me that the anonymousRole is only meant for direct broker access.

michaeljmarshall · 2023-02-07T20:59:58Z

pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAnonymousRoleTest.java

+    }
+
+    @Test
+    public void testAuthorizationWithProxyAsAnonymousRoleWithOriginalRole() throws Exception {


If we do go with my proposed restriction, it might be helpful to use the ServerCnxTest class to validate how the Connect command behaves. If you look through the commit history on that file, you'll see that I've added several authentication related tests that allow for low level manipulation of the protocol without much need for mocking.

github-actions · 2023-03-11T01:56:50Z

The pr had no activity for 30 days, mark with Stale label.

nodece requested a review from michaeljmarshall February 2, 2023 07:53

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 2, 2023

nodece self-assigned this Feb 2, 2023

nodece added area/authn and removed doc-not-needed Your PR changes do not impact docs labels Feb 2, 2023

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 2, 2023

nodece requested review from Technoboy-, codelipenghui, eolivelli, mattisonchao, merlimat and tuteng February 3, 2023 02:42

codelipenghui reviewed Feb 3, 2023

View reviewed changes

michaeljmarshall reviewed Feb 3, 2023

View reviewed changes

eolivelli reviewed Feb 6, 2023

View reviewed changes

pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAnonymousRoleTest.java Outdated Show resolved Hide resolved

nodece changed the title ~~[fix][authentication] Check the original auth when using anonymous role~~ [fix][authentication] Store the original auth when using anonymous role Feb 7, 2023

[fix][authentication] Store the original auth when using anonymous role

057703a

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

nodece force-pushed the anonymous-role-and-original-auth branch from 45eb7e9 to 057703a Compare February 7, 2023 07:38

nodece requested review from codelipenghui, eolivelli and michaeljmarshall February 7, 2023 07:39

michaeljmarshall reviewed Feb 7, 2023

View reviewed changes

github-actions bot added the Stale label Mar 11, 2023

nodece closed this Jul 7, 2023

	if (authenticationProvider == null) {
	authRole = getBrokerService().getAuthenticationService().getAnonymousUserRole()
	.orElseThrow(() ->
	new AuthenticationException("No anonymous role, and no authentication provider configured"));
	completeConnect(clientProtocolVersion, clientVersion, enableSubscriptionPatternEvaluation);
	return;
	}

		@@ -971,6 +973,7 @@ protected void handleConnect(CommandConnect connect) {
		authRole = getBrokerService().getAuthenticationService().getAnonymousUserRole()

[fix][authentication] Store the original auth when using anonymous role #19390

[fix][authentication] Store the original auth when using anonymous role #19390

Uh oh!

Conversation

nodece commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codelipenghui commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodece commented Feb 6, 2023

Uh oh!

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodece Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nodece commented Feb 2, 2023 •

edited

Loading

michaeljmarshall left a comment •

edited

Loading

codelipenghui commented Feb 6, 2023 •

edited

Loading

nodece Feb 8, 2023 •

edited

Loading