Add pluggable authorization mechanism #1200

rdhabalia · 2018-02-07T21:57:13Z

Motivation

As discussed in #1072, some times, we require to add pluggable authorization in broker, proxy, websocket.

Modifications

Add pluggable authorization mechanism. also made existing authorization as default provider.

Result

User can plugin custom authorization framework. It will not impact existing authorization's functionality and performance.

I will add documentation in separate PR once, we merge this change.

merlimat · 2018-02-07T23:33:20Z

...roker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java

+public interface AuthorizationProvider extends Closeable {
+
+    /**
+     * Provide a unique authorization name that can be passed by a client to get authorized. 


Can you specify a bit more, with examples, on this?

merlimat · 2018-02-07T23:34:43Z

...roker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java

+     * @param role
+     *            the app id used to send messages to the destination.
+     */
+    public CompletableFuture<Boolean> canProduceAsync(DestinationName destination, String role);


I think we should maybe have some more informations available to the Authz implementation, such as the client credentials, which may be used in addition to "role" itself.

yes, that's correct, we should try to give as many info as possible as we may not have liberty to change interface later on. I will fix it.

merlimat · 2018-02-07T23:36:27Z

...roker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java

+     */
+    public CompletableFuture<Boolean> canLookupAsync(DestinationName destination, String role);
+
+}


Should we also use this interface to grant the permissions?

Eg: I might use a custom backend for authorization, but the same grant-permission could still work by writing through this interface.

yes, that makes sense. I will add it.

merlimat · 2018-02-07T23:37:39Z

...broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java

+        if (provider != null) {
+            return provider.canProduceAsync(destination, role);
+        }
+        CompletableFuture<Boolean> result = new CompletableFuture<>();


We have a FutureUtil.failedFuture(...) utility

merlimat · 2018-02-07T23:39:01Z

conf/proxy.conf

@@ -49,6 +49,9 @@ authenticationProviders=
 # Enforce authorization
 authorizationEnabled=false

+# Authorization provider name list, which is comma separated list of class names
+authorizationProviders=org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider


What is the behavior when multiple backends are configured? at-least-once has to say "yes"? It might get tricky to define well, when there are multiple source of truth that we're following.

if authMethod is not provided by client then AuthorizationService iterates over all the providers and look for one "yes".

However, right now, if client doesn't provide authMethodName then it I have kept default authMethod in protoAPI which makes broker to do authorization against default-existing-provider to avoid iteration over all available providers as all old client don't send authMethod.

I think it might not be always desirable to have the client choose which authorization backend to use. Do you have a concrete use case example to have multiple auth backends? I would actually just vote to have a single backend impl allowed. :)

I don't have a strong opinion for having multiple providers but it can be useful when one wants to manage multiple authorization-systems for time being when they are migrating from one authorization provider to another. I agree that it's not desirable that client chooses the auth-method. I will remove support of multiple providers.

Ok, it is always easier to add it if needed, then remove it later

merlimat · 2018-02-07T23:40:01Z

pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthorizationProducerConsumerTest.java

+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+
+import jersey.repackaged.com.google.common.collect.Maps;


rdhabalia · 2018-02-08T05:04:23Z

@merlimat addressed all the comments

merlimat

👍

fix: move FutureUtils to common and fix import add grantpermission api take default auth method pass authData to authorization provider keep single authorization provider

) ### Motivation The `AuthenticationDataSource` variable was added to the `ProxyConnection` class in #1200. It is no longer needed. Note that we store this variable in the `ServerCnx` because it is used for authorization. Because we do not do authorization in the proxy, we don't need it. ### Modifications * Remove unused variable ### Verifying this change This is a trivial change. ### Does this pull request potentially affect one of the following parts: The only conceivable way this is a breaking change is if a third party implementation implemented their library so that `authState.getAuthDataSource()` has a side effect. ### Documentation - [x] `doc-not-needed`  No docs needed.

…che#19278) ### Motivation The `AuthenticationDataSource` variable was added to the `ProxyConnection` class in apache#1200. It is no longer needed. Note that we store this variable in the `ServerCnx` because it is used for authorization. Because we do not do authorization in the proxy, we don't need it. ### Modifications * Remove unused variable ### Verifying this change This is a trivial change. ### Does this pull request potentially affect one of the following parts: The only conceivable way this is a breaking change is if a third party implementation implemented their library so that `authState.getAuthDataSource()` has a side effect. ### Documentation - [x] `doc-not-needed`  No docs needed. (cherry picked from commit a9b6519)

rdhabalia added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages type/feature The PR added a new feature or issue requested a new feature labels Feb 7, 2018

rdhabalia self-assigned this Feb 7, 2018

rdhabalia added this to the 1.22.0-incubating milestone Feb 7, 2018

rdhabalia changed the title ~~Add pluggable authorization service~~ Add pluggable authorization mechanism Feb 7, 2018

merlimat reviewed Feb 7, 2018

View reviewed changes

rdhabalia force-pushed the plug_auth branch 3 times, most recently from 577d307 to 0a97ee9 Compare February 8, 2018 05:03

rdhabalia force-pushed the plug_auth branch 4 times, most recently from 285c544 to 168e5c5 Compare February 11, 2018 00:29

merlimat approved these changes Feb 11, 2018

View reviewed changes

Add pluggable authorization service

c9645fe

fix: move FutureUtils to common and fix import add grantpermission api take default auth method pass authData to authorization provider keep single authorization provider

rdhabalia force-pushed the plug_auth branch from 168e5c5 to c9645fe Compare February 11, 2018 00:45

rdhabalia and others added 2 commits February 11, 2018 22:55

fix rebase change

bafe724

Merge branch 'master' into plug_auth

83bbb0d

merlimat merged commit fb1c61d into apache:master Feb 12, 2018

merlimat mentioned this pull request Feb 13, 2018

Regenerated the PulsarApi from proto with latest protoc #1227

Merged

sijie mentioned this pull request Apr 25, 2018

Support pluggable authorization mechansim #1072

Closed

michaeljmarshall mentioned this pull request Jan 18, 2023

[cleanup][proxy] Remove unused AuthenticationDataSource variable #19278

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pluggable authorization mechanism #1200

Add pluggable authorization mechanism #1200

rdhabalia commented Feb 7, 2018 •

edited

Loading

merlimat Feb 7, 2018

merlimat Feb 7, 2018

rdhabalia Feb 8, 2018

merlimat Feb 7, 2018

rdhabalia Feb 8, 2018

merlimat Feb 7, 2018

merlimat Feb 7, 2018

rdhabalia Feb 8, 2018

merlimat Feb 9, 2018

rdhabalia Feb 9, 2018

merlimat Feb 9, 2018

merlimat Feb 7, 2018

rdhabalia commented Feb 8, 2018

merlimat left a comment

Add pluggable authorization mechanism #1200

Add pluggable authorization mechanism #1200

Conversation

rdhabalia commented Feb 7, 2018 • edited Loading

Motivation

Modifications

Result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdhabalia commented Feb 8, 2018

merlimat left a comment

Choose a reason for hiding this comment

rdhabalia commented Feb 7, 2018 •

edited

Loading