AuthZ: integration and refactoring with core flows #80

aayustark007-fk · 2024-01-09T14:59:42Z

Refactoring to AuthZ flows: changed RoleBinding to IamPolicy
Added test authN mechanism for testing and e2e runs
Added required permissions for all routes
Using ResourceHierarchy handlers for passing resource path
Added roles and permissions in config
removed PermissionsAuthorization
test changes to use auth handlers
enabled authn and authz by default

Add permissions authorization for org and team

…h methods

…nce that can be inferred from the path param

…se the ctx resource heirarchy instead

… unauthorized

aayustark007-fk · 2024-03-01T13:32:17Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

@@ -212,6 +213,7 @@ private void addResourceContextForLeafNode(
                                "/projects/%s/subscriptions/%s".formatted(segments[2], segments[3])
                        )
                ));
+                // TODO: This fails when action is SET/GET IAM policy on topic or sub. Here, we rely on action since we don't know the leaf resource type. But it breaks in case of IAM actions.


need to discuss how to address this

one possiblity is to differentiate set/get policy based on resource type.

have added TOPIC_IAM_POLICY_SET/GET and SUBSCRIPTION_IAM_POLICY_SET/GET

aayustark007-fk · 2024-03-01T13:33:08Z

server/src/main/java/com/flipkart/varadhi/services/IamPolicyService.java

@@ -117,7 +95,13 @@ private boolean isResourceValid(String resourceId, ResourceType resourceType) {
                String varadhiTopicName = String.join(NAME_SEPARATOR, segments[0], segments[1]);
                yield (segments.length == 2) && metaStore.checkTopicExists(varadhiTopicName);
            }
-            case SUBSCRIPTION -> false; //TODO
+            case SUBSCRIPTION -> {


added handling for subscription policy

aayustark007-fk · 2024-03-01T13:33:36Z

server/src/main/java/com/flipkart/varadhi/utils/IamPolicyHelper.java

+    private IamPolicyHelper() {
+    }
+
+    public static final String AUTH_RESOURCE_NAME_SEPARATOR = ":";


capture common utils used by tests, handlers and service

aayustark007-fk · 2024-03-01T13:34:00Z

server/src/main/java/com/flipkart/varadhi/web/AuthnHandler.java

@@ -73,4 +84,24 @@ JWTAuthHandler createJWTHandler(Vertx vertx, AuthenticationOptions.JWTConfig con
            throw new VaradhiException("Failed to Initialise JWT Authentication handler.", e);
        }
    }
+
+    AuthenticationHandler createTestHandler() {


test mechanism impl

aayustark007-fk · 2024-03-01T13:35:04Z

server/src/main/java/com/flipkart/varadhi/web/AuthorizationHandlerBuilder.java

-    public AuthorizationHandler build(PermissionAuthorization requiredAuthorization) {
-        return new AuthorizationHandler(requiredAuthorization);
+    public AuthorizationHandler build(ResourceAction authorizationOnResource) {
+        return new AuthorizationHandler(authorizationOnResource);


since resource hierarchy is providing complete path, PermissionsAuthorization is not needed and replaced with action only

server/src/main/java/com/flipkart/varadhi/web/AuthorizationHandlerBuilder.java

aayustark007-fk · 2024-03-01T13:48:49Z

server/src/testE2E/java/com/flipkart/varadhi/E2EBase.java

@@ -263,27 +265,31 @@ static <T> Response makeHttpPostRequest(String targetUrl, T entityToCreate) {
        return getClient()
                .target(targetUrl)
                .request(MediaType.APPLICATION_JSON_TYPE)
+                .header(AUTHN_TEST_HEADER, SUPER_USER)


we need this now to pass superuser info for authn.
without this tests will fail due to authentication failure

aayustark007-fk

comments

kmrdhruv · 2024-03-03T11:23:19Z

server/src/main/java/com/flipkart/varadhi/auth/AuthenticationOptions.java

@@ -32,7 +32,8 @@ public <T> T asConfig(Class<T> configClass) {
    }

    public enum Mechanism {
-        jwt
+        jwt,
+        test


Any specific reason to use X-Auth-Token header ?

should we call it user-header instead of test ?

kmrdhruv · 2024-03-03T11:29:00Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

@@ -102,6 +102,9 @@ private List<Pair<ResourceType, ResourceContext>> generateResourceContextHierarc
            return List.of();
        }

+        // trim leading and trailing slashes


What is the expected here and in which case it is getting incorrect ?
thinking if we can ensure correct input/parameter here, instead of doing trim ?

Resource Path as a leading / which causes blank segment at index 0.
For some cases, there also comes a trailing / in case of create topic or create sub.
Apart from that it's just a measure to ensure input is sanitised.

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

kmrdhruv · 2024-03-03T12:25:15Z

server/src/main/java/com/flipkart/varadhi/services/IamPolicyService.java

-
-    public RoleBindingNode findRoleBindingNode(ResourceType resourceType, String resourceId) {
-        return roleBindingMetaStore.getRoleBindingNode(resourceType, resourceId);
+    public List<IamPolicyRecord> getAll() {


Is getAll() and getIamPolicyRecords() needed for testing purpose alone ? If yes, possible to avoid it ? (getIamPolicyRecords() is part of interface and hence trying to ensure if it is needed)

removed them

kmrdhruv · 2024-03-03T12:26:54Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

@@ -212,6 +213,7 @@ private void addResourceContextForLeafNode(
                                "/projects/%s/subscriptions/%s".formatted(segments[2], segments[3])
                        )
                ));
+                // TODO: This fails when action is SET/GET IAM policy on topic or sub. Here, we rely on action since we don't know the leaf resource type. But it breaks in case of IAM actions.


one possiblity is to differentiate set/get policy based on resource type.

server/src/main/java/com/flipkart/varadhi/web/AuthorizationHandlerBuilder.java

kmrdhruv · 2024-03-03T15:32:25Z

server/src/main/java/com/flipkart/varadhi/web/AuthorizationHandlerBuilder.java

@@ -35,24 +33,25 @@ public AuthorizationHandlerBuilder(List<String> superUsers, AuthorizationProvide
        this.provider = Objects.requireNonNull(provider, "Authorization Provider is null");
    }

-    public AuthorizationHandler build(PermissionAuthorization requiredAuthorization) {
-        return new AuthorizationHandler(requiredAuthorization);
+    public AuthorizationHandler build(ResourceAction authorizationOnResource) {


Is Builder still needed or we can just use AuthzHandler i.e. merge builder and handler ?

we still need to pass the provider and super users list, which would require adding extra args to the constructor

kmrdhruv · 2024-03-03T15:41:44Z

server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java

                                .build(this::getHierarchy, this::get),
                        RouteDefinition
                                .post("CreateSubscription", "")
                                .hasBody()
                                .bodyParser(this::setSubscription)
-                                .authorize(SUBSCRIPTION_CREATE, "{project}")
+                                .authorize(SUBSCRIPTION_CREATE)


lets also check CONSUME permission on topic as well (in the API as of now ?)

this will require larger changes either to pass AuthorizationProvider to SubscriptionHandlers or to make changes to provider contract to handle multiple actions.

it will be taken independently ?

yes, I'll raise it as a separate PR

server/src/main/resources/authorizationConfig.yml

Signed-off-by: aayustark007-fk <aayush.gupta@flipkart.com>

kmrdhruv · 2024-03-21T11:14:34Z

common/src/main/java/com/flipkart/varadhi/Constants.java

@@ -4,6 +4,7 @@ public class Constants {
    public static final int RANDOM_PARTITION_KEY_LENGTH = 5;
    public static final String CONTEXT_KEY_BODY = "varadhi.body";
    public static final String CONTEXT_KEY_RESOURCE_HIERARCHY = "varadhi.resourceHierarchy";
+    public static final String USER_ID_HEADER = "X-User-Id";


Any specific reason for using "X-" as prefix ?
x_ is header which Varadhi recognizes and treats them in specific way.
also header needs to be in lower case.

replaced X- with x_

entities/src/main/java/com/flipkart/varadhi/entities/auth/IamPolicyRecord.java

entities/src/main/java/com/flipkart/varadhi/entities/auth/ResourceAction.java

kmrdhruv · 2024-03-21T11:55:27Z

server/src/main/java/com/flipkart/varadhi/auth/DefaultAuthorizationProvider.java

+        try {
+            IamPolicyRecord node =
+                    getAuthZService().getIamPolicy(resourceContext.resourceType(), resourceContext.resourceId());
+            if (node == null) {


Are both possible i.e return value null or ResourceNotFoundException ?

added null check for safety
underlying implementation may or may not return null

let's handle only ResourceNotFoundException instead of handling both. Also add a test to ensure getAuthZService().getIamPolicy() throws exception instead of returning null.

removed null check
test is already added to IamPolicyServiceTest to check for the required case

server/src/main/java/com/flipkart/varadhi/services/IamPolicyService.java

kmrdhruv · 2024-03-21T18:23:29Z

server/src/main/java/com/flipkart/varadhi/web/v1/admin/SubscriptionHandlers.java

                                .build(this::getHierarchy, this::get),
                        RouteDefinition
                                .post("CreateSubscription", "")
                                .hasBody()
                                .bodyParser(this::setSubscription)
-                                .authorize(SUBSCRIPTION_CREATE, "{project}")
+                                .authorize(SUBSCRIPTION_CREATE)


it will be taken independently ?

server/src/main/java/com/flipkart/varadhi/web/v1/authz/IamPolicyHandlers.java

kmrdhruv · 2024-03-21T18:34:38Z

server/src/main/java/com/flipkart/varadhi/web/v1/authz/IamPolicyHandlers.java

+        );
+    }
+
+    private ResourceHierarchy getHierarchy(RoutingContext ctx, boolean hasBody) {


getHierarchy() in case of subscription needed ?

hierarchy will always be needed for top-down roles resolution (eg: org-admin trying to update a subscription)

correct. getHierarchy() needs to handle the case of subscription as well, currently it doesn't.

server/src/main/java/com/flipkart/varadhi/web/v1/authz/IamPolicyHandlers.java

kmrdhruv · 2024-03-22T04:50:18Z

entities/src/main/java/com/flipkart/varadhi/entities/auth/ResourceAction.java

+
+    IAM_POLICY_GET(ResourceType.IAM_POLICY, "get"),
+    IAM_POLICY_SET(ResourceType.IAM_POLICY, "set"),
+    IAM_POLICY_DELETE(ResourceType.IAM_POLICY, "delete");


I am not sure of this being common for org/team/project. But its fine for time being, we can change it later if needed.

org/team/project have hierarchal ordering while for the topic/sub/queue they occupy the same level in the resource hierarchy

this is the reason why we can use common resource action for the former (can infer based on path) while for the latter we need more context since we cannot know what type the leaf id is.

…check and throw the same again

aayustark007-fk

done

aayustark007-fk · 2024-03-27T06:47:11Z

common/src/main/java/com/flipkart/varadhi/Constants.java

@@ -4,6 +4,7 @@ public class Constants {
    public static final int RANDOM_PARTITION_KEY_LENGTH = 5;
    public static final String CONTEXT_KEY_BODY = "varadhi.body";
    public static final String CONTEXT_KEY_RESOURCE_HIERARCHY = "varadhi.resourceHierarchy";
+    public static final String USER_ID_HEADER = "x_user_id";


add todo: this user is only for testing and x_ convention may cause it to be sent to the destination during consume

… return null

kmrdhruv

LGTM

kmrdhruv · 2024-03-27T06:23:43Z

server/src/main/java/com/flipkart/varadhi/web/v1/authz/IamPolicyHandlers.java

+        );
+    }
+
+    private ResourceHierarchy getHierarchy(RoutingContext ctx, boolean hasBody) {


correct. getHierarchy() needs to handle the case of subscription as well, currently it doesn't.

server/src/main/java/com/flipkart/varadhi/web/v1/authz/IamPolicyHandlers.java

aayustark007-fk added 3 commits February 18, 2024 15:45

address prev PR comments

937e967

add changes to remove default authz handlers

c7ea279

rename RoleBindingNode -> IAMPolicyRecord

6692fe9

aayustark007-fk force-pushed the authz-2 branch from 4926cfd to e5e7ab4 Compare February 18, 2024 10:24

fix tests

d8cf28d

aayustark007-fk force-pushed the authz-2 branch from e5e7ab4 to d8cf28d Compare February 18, 2024 10:31

aayustark007-fk and others added 9 commits February 18, 2024 16:05

rename zk path ROLE_BINDING to IAM_POLICY

61c7a33

ensure authz for org handlers

ddf7aec

ensure authz for team handlers

16b9d45

ensure authz for set/get iam policy for org and team nodes

212caee

Merge pull request #108 from flipkart-incubator/authz-integ

2078212

Add permissions authorization for org and team

iam policy handling subscriptions

e89ab68

major refactoring, IAMPolicyResponse changes and added helper for aut…

add035b

…h methods

renaming to iam

8ecb196

Merge remote-tracking branch 'origin/master' into authz-2

0ef205f

aayustark007-fk changed the title ~~<wip> AuthZ: address prev PR comments~~ AuthZ: integration and refactoring with core flows Feb 28, 2024

aayustark007-fk added 14 commits February 28, 2024 10:38

authentication handler 1st priority

5fd63b5

remove resource type and resource id info from iam policy response si…

6674f30

…nce that can be inferred from the path param

replace env with resource heirarchy to the path and attributes

0f9c66c

remove usage of permissions authorization, pass resource action and u…

102af77

…se the ctx resource heirarchy instead

delete permission authorization

e81ef74

simplify iam handlers get paths

831333a

prevent exposing internal errors when policy not found, rather return…

2c3a436

… unauthorized

do not end the context on policy delete

58a0870

refactor e2e http methods to account for authn enabled

480005b

Merge remote-tracking branch 'origin/master' into authz-2

0fe4114

fix docker config due to which authz config was not being found

9fe306d

fix docker config for renaming zkConfig path in authz config

d33b18a

fix super user for running e2e tests

e3f220f

auth test fix

89a0ae9

aayustark007-fk commented Mar 1, 2024

View reviewed changes

server/src/main/java/com/flipkart/varadhi/web/AuthorizationHandlerBuilder.java Show resolved Hide resolved

aayustark007-fk commented Mar 1, 2024

View reviewed changes

kmrdhruv requested changes Mar 3, 2024

View reviewed changes

aayustark007-fk and others added 7 commits March 14, 2024 19:40

change test authn to user header

66b25c7

catch only the resource not found exception in get iam policy

61c2a2f

add explicit iam policy actions for topic and subscription

db9de9c

address PR comments

60ce37a

Signed-off-by: aayustark007-fk <aayush.gupta@flipkart.com>

Merge branch 'master' into authz-2

896d1f6

fix tests

6f55654

Merge branch 'master' into authz-2

6fd613e

kmrdhruv requested changes Mar 22, 2024

View reviewed changes

aayustark007-fk added 6 commits March 25, 2024 21:08

change X-User-Id to x_user_id

9e47f47

remove authResourceId from iam policy record, using name instead

be49382

rename actions for core entity iam policy ResourceAction

4bbd41a

rename node to iamPolicyRecord/record

0276d87

metastore delete will throw resource not found, no need to do exists …

ca0b7b3

…check and throw the same again

add null handling for resource hierarchy in authz builder

0e72dab

aayustark007-fk commented Mar 25, 2024

View reviewed changes

aayustark007-fk commented Mar 27, 2024

View reviewed changes

aayustark007-fk added 3 commits March 27, 2024 12:28

add subscription handling for get hierarchy in iam handlers

fcbe8c2

remove null check from getIamPolicy since we have ensured it will not…

5fb6521

… return null

fix tests

45b4a49

kmrdhruv approved these changes Mar 27, 2024

View reviewed changes

aayustark007-fk merged commit 5abbd15 into master Mar 27, 2024
2 checks passed

AuthZ: integration and refactoring with core flows #80

AuthZ: integration and refactoring with core flows #80

Conversation

aayustark007-fk commented Jan 9, 2024 • edited

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

aayustark007-fk left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aayustark007-fk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmrdhruv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aayustark007-fk commented Jan 9, 2024 •

edited