Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default authZ server with role bindings API and provider integration #56

Merged
merged 40 commits into from
Jan 2, 2024

Conversation

aayustark007-fk
Copy link
Collaborator

@aayustark007-fk aayustark007-fk commented Nov 20, 2023

  • Role Bindings Update API
  • AuthZ for update API for Role Bindings (need help)
  • Calling AuthZ server from Default provider
  • Tests for AuthZ server
  • Tests for integration of provider with server

Copy link

github-actions bot commented Nov 21, 2023

Test Results

173 tests   173 ✅  32s ⏱️
 40 suites    0 💤
 40 files      0 ❌

Results for commit 00fd97c.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2023

Codecov Report

Attention: 110 lines in your changes are missing coverage. Please review.

Comparison is base (d1431ef) 69.63% compared to head (00fd97c) 67.18%.

Files Patch % Lines
...m/flipkart/varadhi/web/v1/authz/AuthZHandlers.java 32.09% 54 Missing and 1 partial ⚠️
...m/flipkart/varadhi/web/routes/RouteDefinition.java 0.00% 17 Missing ⚠️
...lipkart/varadhi/entities/auth/RoleBindingNode.java 0.00% 11 Missing ⚠️
...in/java/com/flipkart/varadhi/VerticleDeployer.java 36.36% 5 Missing and 2 partials ⚠️
...va/com/flipkart/varadhi/services/AuthZService.java 85.10% 4 Missing and 3 partials ⚠️
...ipkart/varadhi/entities/auth/IAMPolicyRequest.java 0.00% 4 Missing ⚠️
.../java/com/flipkart/varadhi/entities/auth/Role.java 0.00% 4 Missing ⚠️
...n/java/com/flipkart/varadhi/utils/LoaderUtils.java 33.33% 3 Missing and 1 partial ⚠️
...art/varadhi/auth/DefaultAuthorizationProvider.java 98.78% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master      #56      +/-   ##
============================================
- Coverage     69.63%   67.18%   -2.45%     
- Complexity      374      424      +50     
============================================
  Files            88       93       +5     
  Lines          1696     1923     +227     
  Branches        108      124      +16     
============================================
+ Hits           1181     1292     +111     
- Misses          478      586     +108     
- Partials         37       45       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dependencies {
implementation(project(':common'))
components.java.withVariantsFromConfiguration(configurations.testFixturesApiElements) {
skip()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required to prevent publishing test-fixtures artefacts

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be moved to publish conventions at common place ? also add above comment to the code itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when I add this to conventions, the build starts failing as it is not able to recognise the testFixturesApiElements configurations


public interface AuthorizationProvider {
Future<Boolean> init(AuthorizationOptions authorizationOptions);
Future<Boolean> init(Vertx vertx, AuthorizationOptions authorizationOptions);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required for webclient

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use Vertx.vertx() but can that cause issues?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about using the same vertx instance here because as - it will be tuned for server usage, how using it would outside server impact it, lifecycle of it would be controlled by server, if dependents start storing it's reference will that be any issue ?, possible to avoid having vertx reference in the interfaces.

@@ -1,5 +1,5 @@
plugins {
id 'com.flipkart.varadhi.java-published-library-conventions'
id 'com.flipkart.varadhi.java-library-conventions'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authz does not depend on common anymore, so no need to publish this


public void deployVerticle(
Vertx vertx,
ServerConfiguration configuration
) {
if (configuration.isDefaultAuthorizationServerEnabled()) {
deployDefaultAuthZVerticle(vertx, configuration);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deploying as a separate verticle will not cause issues with roundrobin

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have tested locally

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will create two independent http servers with their own routers. Any advantage of this pattern over combining routes in single router/server.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mainly thinking from pov of isolation
if not needed then we can have it as part of single router

@aayustark007-fk aayustark007-fk changed the title <wip> Default authZ server with roles API Default authZ server with role bindings API and provider integration Nov 22, 2023
dependencies {
implementation(project(':common'))
components.java.withVariantsFromConfiguration(configurations.testFixturesApiElements) {
skip()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be moved to publish conventions at common place ? also add above comment to the code itself.


public interface AuthorizationProvider {
Future<Boolean> init(AuthorizationOptions authorizationOptions);
Future<Boolean> init(Vertx vertx, AuthorizationOptions authorizationOptions);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about using the same vertx instance here because as - it will be tuned for server usage, how using it would outside server impact it, lifecycle of it would be controlled by server, if dependents start storing it's reference will that be any issue ?, possible to avoid having vertx reference in the interfaces.

@@ -15,6 +14,8 @@ publishing {

pom {
name = "${project.name}"
description = "Common AuthZ Provider interface for integrating with Varadhi"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is common convention file, it should avoid module specific references ?

@@ -6,6 +6,7 @@ plugins {
dependencies {
implementation(project(':common'))
implementation(project(':entities'))
implementation(project(':authz'))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think core shouldn't depend on authz module/project.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya. no module will depend on it in gradle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is due to the usage of ResourceType enum which belongs to authz module

@@ -1,5 +1,6 @@
package com.flipkart.varadhi.spi.db;

import com.flipkart.varadhi.auth.RoleBindingNode;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be discussed:
in my view we should keep authz entities/interfaces separate from the core varadhi entities/interfaces and contain them with in authz module as these are optional components ?
Also in terms of module structure core/common/entities should not take dependency on authz, but authz can take a dependency on them as needed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. separate interface.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried separating this out but it will lead to introduction of zookeeper dependencies in VerticleDeployer.
Otherwise, we will have to follow the metastore provider pattern for AuthZ too which will involve quite a lot of changes.

import java.util.HashMap;
import java.util.List;

public class DefaultAuthZService {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AuthzService or iamService or something equivalent ?

return node;
}

public List<RoleBindingNode> getAllRoleBindingNodes() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed ? I mean any use cases which needs all role binding nodes ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is mainly for debug purposes

return metaStore.getRoleBindingNodes();
}

public RoleBindingNode getRoleBindingNode(String resourceId) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would iampolicy better instead of RoleBindingNode ?

}
RoleBindingNode existingNode = metaStore.getRoleBindingNode(resourceId);
if (existingNode.getResourceType() != resourceType) {
throw new InvalidOperationForResourceException(String.format(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invalidArgument ?


@Test
public void testNotInit() {
Assertions.assertThrows(InvalidConfigException.class, () ->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be invalidConfig for not initialized ?

@@ -3,6 +3,7 @@ plugins {
}
dependencies {
implementation(project(":common"))
implementation(project(":authz"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is due to dependency on ResourceType which is used in RoleBindingNode entity

public RoleBindingNode(
@JsonProperty("resourceId") String resourceId,
@JsonProperty("resourceType") ResourceType resourceType,
@JsonProperty("subjectToRolesMapping")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not name it rolesAssignment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this to indicate that the map keys are subject names, otherwise there can be confusion whether its subject to roles mapping or vice versa

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a documentation should be able to clear that up.
"rolesAssignment" is more natural in phrasing. "subjectToRolesMapping" is explicit in how we are representing it. I usually prefer the former.
Just my opinion.

common/src/main/java/com/flipkart/varadhi/Constants.java Outdated Show resolved Hide resolved
Comment on lines 62 to 74
List<RoleBindingNode> getRoleBindingNodes();

RoleBindingNode findRoleBindingNode(String resourceIdWithType);

RoleBindingNode getRoleBindingNode(ResourceType resourceType, String resourceId);

void createRoleBindingNode(RoleBindingNode node);

boolean checkRoleBindingNodeExists(ResourceType resourceType, String resourceId);

int updateRoleBindingNode(RoleBindingNode node);

void deleteRoleBindingNode(ResourceType resourceType, String resourceId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets keep the roleBindings methods in a separate interface.
RoleBindingMetaStore.java?

And have the VaradhiMetaStore implement both MetaStore & RoleBindingMetaStore.

Now, any metastore provider that wants to be able to support roleBinding storage as well, can add all interfaces in its implementation.

And our server module can just check, if the authz default is to be used, then metaStore class must be an instance of RoleBindingMetaStore.class as well, otherwise that is a hard failure, and server wont come up.


void createRoleBindingNode(RoleBindingNode node);

boolean checkRoleBindingNodeExists(ResourceType resourceType, String resourceId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
boolean checkRoleBindingNodeExists(ResourceType resourceType, String resourceId);
boolean isRoleBindingPresent(ResourceType resourceType, String resourceId);

Also, does "node" word have a significance? What if we just say roleBinding(s)?

@@ -1,5 +1,7 @@
package com.flipkart.varadhi.entities;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
package com.flipkart.varadhi.entities;
package com.flipkart.varadhi.authz;

Or move it to entities module itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

public Handler<RoutingContext> setIAMPolicyHandler(ResourceType resourceType) {
return (routingContext) -> {
String resourceId = routingContext.getResourceIdFromPath(resourceType);
IAMPolicyRequest policyForSubject = routingContext.body().asPojo(IAMPolicyRequest.class);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IAMPolicyRequest looks like can go to entities module.
entities.auth package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -98,5 +102,19 @@ public static <T> T getApiResponse(RoutingContext ctx) {
public static void todo(RoutingContext context) {
throw new NotImplementedException("Not Implemented.");
}

public static String getResourceIdFromPath(RoutingContext ctx, ResourceType resourceType) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as suggested elsewhere, this method can be moved to AuthzHandler only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -28,6 +28,7 @@ public class ServerConfiguration {

private AuthorizationOptions authorization;

private boolean authorizationServerEnabled;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make it implicit.
Can server be disabled, if I configure DefaultAuthorizationProvider as the authzProvider?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

server goes in with the default provider, without that there is no way to set the role assignments

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we will have to do the reverse, if default auth provider is enabled, server is enabled by default

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we will have to do the reverse, if default auth provider is enabled, server is enabled by default

Ya I meant this only. If this is the case, then we should go implicit. One less config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@gauravAshok
Copy link
Collaborator

The authz handlers are marked authenticated, but they dont have authz checks. Based on your description, anything pending there?

@aayustark007-fk
Copy link
Collaborator Author

The authz handlers are marked authenticated, but they dont have authz checks. Based on your description, anything pending there?

I have to add new resource type IAM_POLICY so that I can process authorization for set/get IAM policy api. This I plan to do in a follow-up PR

@gauravAshok
Copy link
Collaborator

So, what is pending in this PR?

@aayustark007-fk
Copy link
Collaborator Author

So, what is pending in this PR?

I have pushed all the requested changes, this PR is ready to be merged.
One remaining point is on enabling permissions authorization on authZ handlers but I rather keep it as a separate PR as it will require me to introduce new elements to ResourceType and ResourceAction enums and their handling might warrant further discussion.

Copy link
Collaborator Author

@aayustark007-fk aayustark007-fk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to do these changes as part of separate PR

@@ -0,0 +1,327 @@
package com.flipkart.varadhi;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add integration with existing org, team, etc crud flows (as other PR)
all varadhi apis are authorized if enabled

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to allow authn to work either go the token way or impl default/fake authn provider to take identity from headers being passed.

Copy link
Collaborator

@kmrdhruv kmrdhruv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,
As discussed, some pending refactoring will be taken as part of next PR.

@@ -51,6 +54,7 @@ public class VerticleDeployer {
private final OrgHandlers orgHandlers;
private final TeamHandlers teamHandlers;
private final ProjectHandlers projectHandlers;
private final Supplier<AuthZHandlers> authZHandlersSupplier;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wrap in Supplier ? Any specific uses being looked at ? Also handlers implement RouteProvider, which is already a supplier.

this.configuration =
YamlLoader.loadConfig(
authorizationOptions.getConfigFile(), DefaultAuthorizationConfiguration.class);
this.webClient = WebClient.create(Vertx.vertx()); //CachingWebClient.create(WebClient.create(Vertx.vertx()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's implement Cache independently in a separate task. Cache needed here should have better control in terms of how entries are cached/evicted, metrics being emitted. WebClient cache is unlikely to fit here.

.anyMatch(pair -> isAuthorizedInternal(userContext.getSubject(), action, pair.getValue()));
return Future.succeededFuture(result);
List<Future<Boolean>> futures =
leafToRootResourceIds.stream().filter(pair -> StringUtils.isNotBlank(pair.getValue()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pair.getValue() will not be empty ? Filter on isNotBlank() needed on stream ?

)
.as(BodyCodec.json(RoleBindingNode.class))
.send()
.compose(response -> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Since this is colocated with server, can we avoid webclient and instead directly use metastore. Was this ruled out ?
  2. RoleBindingNode, could be large. Here data specific to a user is needed. Is it intended to store the entire node policy ?

if (node == null || node.subjectToRolesMapping == null) {
return Future.failedFuture("No roles on resource for subject" + subject);
}
log.info(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log.debug might be better, info would be excessive here as this will be for each produce call.

return createRoleBindingNode(resourceId, resourceType);
}
RoleBindingNode existingNode = metaStore.getRoleBindingNode(resourceType, resourceId);
if (existingNode.getResourceType() != resourceType) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given resource type & existence validation happens prior, is this expected ? if yes, when ?

};
}

private void checkValidRoles(RoleBindingNode node) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's implement this, this would prevent users to store random strings as roles.

return node;
}

public List<RoleBindingNode> getAllRoleBindingNodes() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be heavy API as it would return all role bindings across the entire hierarchy and might be an issue w/o pagination etc (which might be tough to implement on top of zk) ? If it is needed for testing, lets move it to test code itself.

@@ -11,6 +11,8 @@ public static class PathParams {
public static final String REQUEST_PATH_PARAM_TEAM = "team";
public static final String REQUEST_PATH_PARAM_PROJECT = "project";
public static final String REQUEST_PATH_PARAM_TOPIC = "topic";
public static final String REQUEST_PATH_PARAM_RESOURCE = "resource";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the new API path implemented, are these still needed ?

import lombok.experimental.StandardException;

@StandardException
public class NotInitializedException extends VaradhiException {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though not related to this PR, you want to modify other init providers as well to use this instead of InvalidStateException ?

@aayustark007-fk aayustark007-fk merged commit 52bae47 into master Jan 2, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants