Auth Module for Connector Services #571

amboz · 2024-03-05T03:01:57Z

This PR introduces the following:

An authorization interface for connector services to authorize requests based on MetacatRequestContext and return an AuthorizationStatus. DefaultAuthorization can be overridden as needed with custom auth logic
ValidatingConnector{Catalog/Database/Partition/Table}Service classes (e.g. ValidatingConnectorTableService) which subsume the existing throttling functionality and also include the new auth functionality
ConnectorFactoryDecorator decorates connector services with validating connector services based on whether rate limiting and/or auth is enabled. Enablement of auth is based on the connector.authorization-exempted config (ref).
Tests to validate: (1) decoration and delegation for ConnectorFactoryManager, (2) auth and rate limiting application in validating connector services (example), and (3) ConnectorManager functionality under all possible combinations of auth and/or rate limiting enablement.

insyncoss · 2024-03-08T21:25:56Z

...main/java/com/netflix/metacat/common/server/connectors/AuthEnabledConnectorTableService.java

+                                             final QualifiedName name,
+                                             final String filter,
+                                             final Integer limit) {
+        authorize(MetacatContextManager.getContext(), name);


Now that you're adding a new AuthEnabledConnector services in addition to existing ThrottlingConnector ones there is an opportunity for some refactor and improvement. Instead of overriding each method similar to what the throttling connector already does, there could be a parent class that does these overrides and calls an abstract check and both auth and throttling provide their own implementation of this.

That being the case, this update could also be done in a later change.

addressed all comments in a refactor to combine auth and throttling into one validating class 😄

insyncoss · 2024-03-08T21:30:13Z

...main/java/com/netflix/metacat/common/server/connectors/AuthEnabledConnectorTableService.java

+        delegate.rename(context, oldName, newName);
+    }
+
+    private void authorize(final MetacatRequestContext context, final QualifiedName resource) {


Adding to the previous comment, using such a framework and extending it would make it so you wouldn't need to have basically the same method and logic duplicated three times across classes which is not a great pattern for code structure or maintainability.

insyncoss · 2024-03-08T22:00:04Z

...n/java/com/netflix/metacat/common/server/connectors/AuthEnabledConnectorDatabaseService.java

+ */
+@Slf4j
+@RequiredArgsConstructor
+public class AuthEnabledConnectorDatabaseService implements ConnectorDatabaseService {


There is a ConnectorBaseService that the different types extend so having certain generalizable functionality that is needed across different types there is also an option if you see such an opportunity though up to you.

stevie9868 · 2024-03-13T19:44:45Z

...ain/java/com/netflix/metacat/common/server/connectors/ValidatingConnectorCatalogService.java


    @Override
    public void create(final ConnectorRequestContext context, final CatalogInfo resource) {
-        checkThrottling(MetacatContextManager.getContext().getRequestName(), resource.getName());
+        if (rateLimiterEnabled) {


nit: I feel like this part of the code (two if statements) can be refactored across Catalog/Database/Partition/Table Service.

Option 1: Just have a simple Util function that covers these 2 if statements, which can be reused across different services/functions.

Option 2: Have a Validator Interface, and for now, we can have two classes (say one for Throttling and another one for Authorization) implements this interface. And each Service can contain a list of validator to run before triggering the delegate call. And in the future, if we want to add more validation logic, we can just add a new Validator.

wdyt?
If we have tight deadline, we can also do refactoring after.

yeah i like this idea; the only consideration is that the decision to throttle is actually at the service-method granularity atm. because checkThrottling requires a resource (QualifiedName), throttling is not checked (even if enabled) for certain methods (example). this was existing logic before this PR.

@insyncoss and i discussed removing this special casing and making the resource nullable so that checkThrottling can be called indiscriminately across methods in a service. it would require null checking in checkThrottling and this would be necessary whether we go with Option 1 or Option 2.

Option 2 is probably better; there is also some validation in RequestGateway elsewhere in the repo that could be pulled into this later on if we go with Option 2.

for Option 2, would you suggest having configs for each connector-service within the connector config, e.g. connector.rate-limiter-exempted-catalog-service or do you think there's a better place for them?

For option2, maybe I miss something, but I feel like we don't need to have a config for each service.

I was thinking we still keep the two original config extempted setting (one for throttling and one for authorization), and have just 1 Validator interface and 2 Implementations (one for throttling and another for Authorization).

The interface can have one function say validate, which takes in two parameters one for MetacatRequestContext, and another for QualifiedName

For the implementation for either ThrottlingValidator and AuthorizationValidator, each should one class variable say enabled, and during the initialization of the Validator, it should read the exempted value and then set the value to true or false accordingly. ThrottlingValidator should also creates the RateLimiter and AuthorizationValidator should creates the Authorization through bean initialization. And the implementation for validate function should be if its enabled, then trigger the corresponding validation.

Then for each service (table/partition/catalog/database), it should have a list of Validator. I see most of the functions check both Throttling and Authorization, so we can have a generic util static function that run all validators. And as you said for some functions where not all validators should run, we can handle them separately.

Looks like this will be a lot of code changes, and I am ok we keep the current version first. We can have others chime in to share their opinions as well.

i see what you mean-- this is cleaner and more extensible. we will need to be careful to include the following:

special handling for specific methods that do not check throttling even when enabled at the connector level

ensure we are explicit about the order in which different validations occur. for example, always check throttling first, then auth

(less important) retain the ability to switch the order of each validation at the method level. this is not needed now but could be as we add more to it

will wait for others opinions before making the change

Here's a suggestion which iterates over the above suggestions:

As Yingjian suggested, we should have a Validator interface with two implementations—ThrottlingValidator and AuthorizationValidator—that perform validations.

Have a ValidationRegistry class that contains all the validators registered with it. It contains a method run that takes in a list of validations to be run. This will be used to execute the validations in the order or exact order they are to be run.

Implement a ValidatingConnectorBase which implements a generic ConnectorBaseService. It takes the validation registry and a delegate and implements all the methods. Each method would first perform the validation via registry.

Similarly, implement all the specific implementations for table, partition etc; with a similar logic.

stevie9868 · 2024-03-14T15:42:00Z

...vy/com/netflix/metacat/common/server/connectors/ValidatingConnectorCatalogServiceSpec.groovy

+    }
+
+    List<List> parametrizedCases() {
+        return [


nit: would better to have a comment on what these 4 columns represent

stevie9868 · 2024-03-14T16:10:18Z

.../com/netflix/metacat/common/server/connectors/ValidatingConnectorPartitionServiceSpec.groovy

+        }
+
+        then:
+        if (rateLimiterEnabled || !rateLimiterEnabled) {


nit: the if statement might be redundant

stevie9868 · 2024-03-14T16:13:07Z

...oovy/com/netflix/metacat/common/server/connectors/ValidatingConnectorTableServiceSpec.groovy

+        }
+
+        then:
+        if (rateLimiterEnabled || !rateLimiterEnabled) {


nit: this if statement might be redundant

stevie9868 · 2024-03-14T16:19:08Z

Looks like previously, there are two places where we don't checkThrottling

ThrottlingConnectorPartitionService getPartitionNames
and
ThrottlingConnectorTableService.java getTableNames

Do we know why these two calls can be exempted from throttling check?

amboz · 2024-03-14T17:56:13Z

Looks like previously, there are two places where we don't checkThrottling

ThrottlingConnectorPartitionService getPartitionNames and ThrottlingConnectorTableService.java getTableNames

Do we know why these two calls can be exempted from throttling check?

@stevie9868 my undersanding these methods dont have a qualified resource associated with them, just prefix searches, and rate limiting is currently only on a per-resource basis

akashdw · 2024-03-14T17:47:57Z

...r/src/main/java/com/netflix/metacat/common/server/api/authorization/AuthorizationStatus.java

+@Builder
+@Getter
+@EqualsAndHashCode
+public class AuthorizationStatus {


This could be an enum and details could be decoupled from the status.

akashdw · 2024-03-14T17:54:48Z

...er/src/main/java/com/netflix/metacat/common/server/connectors/ConnectorFactoryDecorator.java

+        }
+
+        return !Boolean.parseBoolean(
+            connectorContext.getConfiguration().getOrDefault("connector.authorization-exempted", "false")


nit: static constant for "connector.authorization-exempted"

Additionally, there appears to be a discrepancy between the default values when the configuration is null and when the connector. authorization-exempted property is not set. Ideally, these defaults should be identical.

authorization-exempted -> authorization-isEnabled ?

akashdw · 2024-03-14T18:07:54Z

...er/src/main/java/com/netflix/metacat/common/server/connectors/ConnectorFactoryDecorator.java

+                rateLimiter,
+                rateLimiterEnabled,
+                authorization,
+                authorizationEnabled


The authorization object should have the capability to determine its own enabled status, rather than having it flagged externally.

akashdw · 2024-03-14T18:08:34Z

...er/src/main/java/com/netflix/metacat/common/server/connectors/ConnectorFactoryDecorator.java

+                rateLimiter,
+                rateLimiterEnabled,
+                authorization,
+                authorizationEnabled


The authorization object should have the capability to determine its own enabled status, rather than having it flagged externally.

akashdw · 2024-03-14T18:08:41Z

...er/src/main/java/com/netflix/metacat/common/server/connectors/ConnectorFactoryDecorator.java

+                rateLimiter,
+                rateLimiterEnabled,
+                authorization,
+                authorizationEnabled


The authorization object should have the capability to determine its own enabled status, rather than having it flagged externally.

akashdw · 2024-03-14T18:11:22Z

...ain/java/com/netflix/metacat/common/server/connectors/ValidatingConnectorCatalogService.java

+        if (rateLimiterEnabled) {
+            checkThrottling(MetacatContextManager.getContext().getRequestName(), name);
+        }
+        if (authorizationEnabled) {


The responsibility for this and similar if blocks should lie with the Authorization implementation, rather than the caller performing the checks.

akashdw · 2024-03-14T18:13:23Z

...-server/src/main/java/com/netflix/metacat/common/server/api/authorization/Authorization.java

+     * @param context The request context.
+     * @return True if the request is authorized and False if unauthorized.
+     */
+    default AuthorizationStatus isAuthorized(@NonNull MetacatRequestContext context) {


The interface could be more explicit about its expectations from the MetacatRequestContext, instead of passing down the entire context.

Amanda Bozigian added 3 commits March 2, 2024 18:20

add auth skeleton

851075d

add connector factory/delegation tests

0f213af

auth enabled connector service tests

7176306

insyncoss reviewed Mar 8, 2024

View reviewed changes

Amanda Bozigian added 3 commits March 10, 2024 20:06

refactor to one validating parent class

9876142

lint

11fd869

update test name

51236ee

amboz changed the title ~~[WIP] Abozigian/auth module~~ Auth Module for Connector Services Mar 13, 2024

stevie9868 reviewed Mar 13, 2024

View reviewed changes

stevie9868 reviewed Mar 14, 2024

View reviewed changes

akashdw suggested changes Mar 14, 2024

View reviewed changes

amboz closed this Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auth Module for Connector Services #571

Auth Module for Connector Services #571

amboz commented Mar 5, 2024 •

edited

Loading

insyncoss Mar 8, 2024

insyncoss Mar 8, 2024

amboz Mar 13, 2024

insyncoss Mar 8, 2024

insyncoss Mar 8, 2024

stevie9868 Mar 13, 2024 •

edited

Loading

amboz Mar 13, 2024 •

edited

Loading

stevie9868 Mar 14, 2024 •

edited

Loading

amboz Mar 14, 2024

snehalchennuru Mar 14, 2024

stevie9868 Mar 14, 2024

stevie9868 Mar 14, 2024 •

edited

Loading

stevie9868 Mar 14, 2024 •

edited

Loading

stevie9868 commented Mar 14, 2024

amboz commented Mar 14, 2024

akashdw Mar 14, 2024

akashdw Mar 14, 2024

akashdw Mar 14, 2024

akashdw Mar 14, 2024

akashdw Mar 14, 2024

akashdw Mar 14, 2024

akashdw Mar 14, 2024

Auth Module for Connector Services #571

Auth Module for Connector Services #571

Conversation

amboz commented Mar 5, 2024 • edited Loading

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

stevie9868 Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

amboz Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

stevie9868 Mar 14, 2024 • edited Loading

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

stevie9868 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

stevie9868 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

stevie9868 commented Mar 14, 2024

amboz commented Mar 14, 2024

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

amboz commented Mar 5, 2024 •

edited

Loading

stevie9868 Mar 13, 2024 •

edited

Loading

amboz Mar 13, 2024 •

edited

Loading

stevie9868 Mar 14, 2024 •

edited

Loading

stevie9868 Mar 14, 2024 •

edited

Loading

stevie9868 Mar 14, 2024 •

edited

Loading