[#4698] feat(auth-ranger): Extended Ranger authorization by rules #4744

xunliu · 2024-08-28T14:12:07Z

What changes were proposed in this pull request?

Added interface RangerPrivilegesMappingProvider, we can use it to map Gravitino privileges to the Ranger privileges.
Added abstract class RangerAuthorizationPlugin, we can use it to extend another Ranger authorization plugin.

Why are the changes needed?

Fix: #4698

Does this PR introduce any user-facing change?

NA

How was this patch tested?

CI Passed.

xunliu · 2024-08-28T14:12:49Z

@lw-yang Please help me review this PR, thanks.

lw-yang · 2024-08-30T01:25:19Z

LGTM.

authorizations/authorization-ranger/build.gradle.kts

...ons/authorization-ranger/src/test/resources/authorization-defs/authorization-hive.properties

...tion-ranger/src/main/java/org/apache/gravitino/authorization/ranger/AuthorizationConfig.java

xunliu · 2024-08-30T05:25:45Z

@yuqi1129 I fixed all comments. Please help me review again, Thanks!

yuqi1129 · 2024-08-30T06:33:29Z

...thorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java

+   * Initial Ranger policy search key defines. <br>
+   * Initial precise filter key defines. <br>
+   */
+  private void initAuthorizationConfig(AuthorizationConfig authorizationConfig) {


Should we provide default values as before, it's too complicated to set the mapping for most users and only those users that have special requirement will configure this value.

Good idea, I improved it, Please help me review this PR, again.

yuqi1129 · 2024-09-02T02:50:52Z

LGTM, @jerqi @jerryshao Would you like to take a look?

jerryshao · 2024-09-04T22:06:44Z

Sorry for late response, I will take a look at this today.

jerryshao · 2024-09-05T17:06:10Z

I'm a little confused about the changes here made, can you describe more clearly about the scenarios and how users will use this feature? It's really not easy to understand.

xunliu · 2024-09-06T01:23:05Z

I'm a little confused about the changes here made, can you describe more clearly about the scenarios and how users will use this feature? It's really not easy to understand.

In Gravitino, we specify that each Gravitino Privilege maps multiple Privileges on the underlying system(Ranger), but there are some users whose environments have different privilege mappings than ours, so we allow users to configure their own to meet their own personalized needs, in addition to using our default Privilege mappings.

lw-yang · 2024-09-06T01:39:07Z

@jerryshao This feature allows for custom configuration of the mapping relationship between Gravitino Privileges and Ranger Privileges. If the default mapping relationship in Gravitino does not meet our scenario, we can customize it.

For example, by default, Gravitino's MODIFY_TABLE maps to Ranger's RangerHivePrivilege.UPDATE, RangerHivePrivilege.ALTER, RangerHivePrivilege.WRITE. In our scenario, we need to add RangerHivePrivilege.DROP, so we can customize this mapping relationship.

jerryshao · 2024-09-09T17:25:33Z

...tion-ranger/src/main/java/org/apache/gravitino/authorization/ranger/AuthorizationConfig.java

+import org.apache.gravitino.config.ConfigEntry;
+
+public class AuthorizationConfig extends Config {
+  public static final ConfigEntry<List<String>> AUTHORIZATION_OWNER_PRIVILEGES =


Are these generic authorization configs or ranger authorization only configs?

These are generic authorization configs.

If these are generic configs, they should be put into the code module, not Ranger module. Also I'm curious how other plugins like JDBC, IAM can leverage these configurations later on. From my understanding, they're quite Ranger specific.

jerryshao · 2024-09-09T17:31:46Z

...tion-ranger/src/main/java/org/apache/gravitino/authorization/ranger/AuthorizationConfig.java

+    AuthorizationConfig authorizationConfig = new AuthorizationConfig();
+    boolean testEnv = System.getenv("GRAVITINO_TEST") != null;
+    String propertyFilePath =
+        String.format("authorization-defs/authorization-%s.properties", providerName);


Maybe we can directly put this file into the conf, not so necessary to add another "authorization-defs" folder, WDYT?

OK, I moved it to distribution/package/authorizations/ranger/conf/authorization-hive.properties.

jerryshao · 2024-09-09T17:37:33Z

...rization-ranger/src/main/resources/authorization-defs/authorization-hive.properties.template

+# Format: authorization.privilege.mapping.<GRAVITINO-PRIVILEGE> = <RANGER-PRIVILEGE>,<RANGER-PRIVILEGE>,...
+# gravitino.authorization.privilege.mapping.CREATE_SCHEMA = create
+# gravitino.authorization.privilege.mapping.CREATE_TABLE = create
+# gravitino.authorization.privilege.mapping.SELECT_TABLE = read,select


Do we handle the scenarios like having prefix and trailing whitespace in between policies for the conf value? like:

gravitino.authorization.privilege.mapping.CREATE_TABLE = create, aaa, xxx, aaa

Yes, I have test case to cover these scenarios.
authorizations/authorization-ranger/src/test/resources/authorization-defs/authorization-hive-nonstandard.properties

jerryshao · 2024-09-09T17:45:35Z

docs/security/authorization-pushdown.md

+
+Gravitino privileges defined in the `api/src/main/java/org/apache/gravitino/authorization/Privilege.java`
+Format: authorization.privilege.mapping.<GRAVITINO-PRIVILEGE> = <RANGER-PRIVILEGE>,<RANGER-PRIVILEGE>,...
+:::


I think the document here is not enough. You should clearly describe how to configure, what is the meaning, and give an example here.

Also, do we only support 4 privileges for mapping, how do we support more privileges to map easily?

I think the document here is not enough. You should clearly describe how to configure, what is the meaning, and give an example here.

OK, I added example in the docs.

Also, do we only support 4 privileges for mapping, how do we support more privileges to map easily?

No, We can support all Gravitino privileges to configurations.
Because I use

Privilege.Name gravitinoPrivilege = Privilege.Name.valueOf(key.trim().toUpperCase());

These codes can convert any String to a Gravitino Privilege enum value.

xunliu · 2024-09-10T12:15:51Z

...tion-ranger/src/main/java/org/apache/gravitino/authorization/ranger/AuthorizationConfig.java

+import org.apache.gravitino.config.ConfigEntry;
+
+public class AuthorizationConfig extends Config {
+  public static final ConfigEntry<List<String>> AUTHORIZATION_OWNER_PRIVILEGES =


These are generic authorization configs.

xunliu · 2024-09-10T12:46:09Z

...rization-ranger/src/main/resources/authorization-defs/authorization-hive.properties.template

+# Format: authorization.privilege.mapping.<GRAVITINO-PRIVILEGE> = <RANGER-PRIVILEGE>,<RANGER-PRIVILEGE>,...
+# gravitino.authorization.privilege.mapping.CREATE_SCHEMA = create
+# gravitino.authorization.privilege.mapping.CREATE_TABLE = create
+# gravitino.authorization.privilege.mapping.SELECT_TABLE = read,select


Yes, I have test case to cover these scenarios.
authorizations/authorization-ranger/src/test/resources/authorization-defs/authorization-hive-nonstandard.properties

xunliu · 2024-09-10T13:00:28Z

...tion-ranger/src/main/java/org/apache/gravitino/authorization/ranger/AuthorizationConfig.java

+    AuthorizationConfig authorizationConfig = new AuthorizationConfig();
+    boolean testEnv = System.getenv("GRAVITINO_TEST") != null;
+    String propertyFilePath =
+        String.format("authorization-defs/authorization-%s.properties", providerName);


OK, I moved it to distribution/package/authorizations/ranger/conf/authorization-hive.properties.

xunliu · 2024-09-10T13:06:37Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+                Privilege.Name gravitinoPrivilege =
+                    Privilege.Name.valueOf(key.trim().toUpperCase());


hi @jerryshao
These codes can convert any String to a Gravitino Privilege enum value.

xunliu · 2024-09-10T13:07:03Z

docs/security/authorization-pushdown.md

+
+Gravitino privileges defined in the `api/src/main/java/org/apache/gravitino/authorization/Privilege.java`
+Format: authorization.privilege.mapping.<GRAVITINO-PRIVILEGE> = <RANGER-PRIVILEGE>,<RANGER-PRIVILEGE>,...
+:::


I think the document here is not enough. You should clearly describe how to configure, what is the meaning, and give an example here.

OK, I added example in the docs.

Also, do we only support 4 privileges for mapping, how do we support more privileges to map easily?

No, We can support all Gravitino privileges to configurations.
Because I use

Privilege.Name gravitinoPrivilege = Privilege.Name.valueOf(key.trim().toUpperCase());

These codes can convert any String to a Gravitino Privilege enum value.

xunliu · 2024-09-10T23:51:23Z

@jerryshao I updated this PR, Please help me review it again, thanks.

jerqi · 2024-09-12T03:01:28Z

This is just a discussion instead of review suggestion.
In my opinion,

What's the difference between users and developers.
Users can only change the configurations. Developers can use the interface to custom their need.

This feature should be used by developers instead of users. So I prefer extracting an interface PrivilegeMappingProviderPlugin to let other developer extend other mapping relation.

Configuration has less restriction than the interface. The interface can help us define better behaviour.

This is just my cent, not a strong suggestion.

xunliu · 2024-09-13T03:43:32Z

@jerryshao I removed the use properties file to overwrite the default authorization configuration.
Please help me review it again, thanks!

jerryshao · 2024-09-13T23:33:26Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationConfig.java

+import org.apache.gravitino.authorization.Privilege;
+
+/**
+ * Ranger authorization use this configuration to mapping Gravitino privilege to the Ranger


I need to think a bit on this, my initial feeling is that This RangerAuthorizationConfig may not be easy for people to use.

Most of the interfaces like initializePrivilegesMappingConfig() is hard to people to understand from the definition. People doesn't know what should be implemented, what is the input, output and side-effect of this interface. Without knowing this, it is hard for the developers to extend or implement this interface.

It's OK for internal use. But if we want to expose to the developers to extend, seems the definition is not self-explanatory. The concept of xxxConfig makes people hard to understand without context.

Maybe we can change class name fromRangerAuthorizationConfig to PrivilegeMappingRules?
and change the function name from

initializePrivilegesMappingConfig() -> setPrivilegesMappingRule()

initializeOwnerPrivilegesConfig() -> setOwnerPrivilegesRule()

initializePolicyResourceDefinesConfig() -> setPolicyResourceDefinesRule()

What do you think?

Typically, it is hard to design an interface for like "initializeXXX" or "setXXX", because it is hard for developers to know how to correctly implement a code about "initializeXXX" or "setXXX". Instead, I would suggest like:

interface RangerPrivilegesMappingProvider { Map<GravitinoPrivilege, Set<RangerPrivileges>> privilegesMappingRule() Map<GravitinoPrivilege, Set<RangerPrivileges>> ownerMappingRule() ... }

Just for your reference, please take a look, thanks.

OK, I accept your suggestion.

jerryshao · 2024-09-13T23:59:16Z

...rization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerPrivilege.java

+public interface RangerPrivilege {
+  String toString();
+
+  boolean equals(String value);


the interface name equals is conflicted with Java's built-in equals method name, also for toString, it is better to have another name.

OK, change equals() to isEquals().

jerryshao · 2024-09-14T00:03:13Z

...rization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerPrivilege.java

+    REPLADMIN("repladmin"),
+    SERVICEADMIN("serviceadmin");
+
+    private final String string; // Access a type in the Ranger policy item


It is better to have a different name for this class variable, not similar to the reserved word.

…figuration

xunliu · 2024-09-18T07:27:50Z

@jerryshao I accept your suggestion and modify the code, please help me review again, thanks.

jerryshao · 2024-09-18T09:16:10Z

...src/main/java/org/apache/gravitino/authorization/ranger/RangerPrivilegesMappingProvider.java

+  void ownerMappingRule();
+
+  /** Set the policy resource defines rule. */
+  void policyResourceDefinesRule();


I still think that you should have the return value for the method you defined here. Otherwise, it is hard for others to implement a such interface.

jerryshao · 2024-09-18T13:39:04Z

...r/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHivePlugin.java

+    return ImmutableList.of(
+        PolicyResource.DATABASE.getName(),
+        PolicyResource.TABLE.getName(),
+        PolicyResource.COLUMN.getName());


Seems like all these methods return immutable map or list, if so, I would suggest make them singletons, so that we don't have to create maps or lists for each call.

Changed to singleton.

jerryshao · 2024-09-18T13:40:19Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+  private Map<Privilege.Name, Set<RangerPrivilege>> privilegesMapping;
+  /** The owner privileges, the owner can do anything on the metadata object configuration */
+  private Set<RangerPrivilege> ownerPrivileges;
+  /** The Ranger policy resource defines configuration. */
+  private List<String> policyResourceDefines;


Can you please make these variables final?

Also for other class variables.

jerryshao · 2024-09-18T13:41:23Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+    // Initialize privilegesMapping and ownerPrivileges
+    ownerPrivileges = ownerMappingRule();
+    privilegesMapping = privilegesMappingRule();
+    policyResourceDefines = policyResourceDefinesRule();


Also please change the style to this.xxx = xxx for class variable assignment.

jerryshao · 2024-09-18T13:43:13Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+  }
+
+  public final List<String> getPolicyResourceDefines() {
+    return policyResourceDefines;


Also do you still need these methods? I think you can directly call privilegesMappingRule, ownerMappingRule, etc.

jerryshao · 2024-09-18T13:47:48Z

...rization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerPrivilege.java

+public interface RangerPrivilege {
+  String getName();
+
+  boolean isEquals(String value);


"isEquals" is not correct for grammar, can you please change to equalsTo(xxxx)?

jerryshao · 2024-09-18T13:56:24Z

...st-common/src/test/java/org/apache/gravitino/integration/test/container/RangerContainer.java

@@ -38,7 +38,7 @@ public class RangerContainer extends BaseContainer {
  public static final String DEFAULT_IMAGE = System.getenv("GRAVITINO_CI_RANGER_DOCKER_IMAGE");
  public static final String HOST_NAME = "gravitino-ci-ranger";
  public static final int RANGER_SERVER_PORT = 6080;
-  public RangerClient rangerClient;
+  public RangerClientExtend rangerClient;


Can you please change this class name "RangerClientExtend" to "RangerExtendedClient" or "RangerClientExtension", the name "xxxExtend" is a verb not a noun.

Changed to RangerClientExtension.

jerryshao · 2024-09-18T13:59:45Z

@xunliu , can you please also update the PR title and description to reflect what you did currently.

xunliu

@jerryshao I refactoring the code, Please help me review again.

xunliu · 2024-09-18T16:14:23Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+  private Map<Privilege.Name, Set<RangerPrivilege>> privilegesMapping;
+  /** The owner privileges, the owner can do anything on the metadata object configuration */
+  private Set<RangerPrivilege> ownerPrivileges;
+  /** The Ranger policy resource defines configuration. */
+  private List<String> policyResourceDefines;


xunliu · 2024-09-18T16:14:29Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+    // Initialize privilegesMapping and ownerPrivileges
+    ownerPrivileges = ownerMappingRule();
+    privilegesMapping = privilegesMappingRule();
+    policyResourceDefines = policyResourceDefinesRule();


xunliu · 2024-09-18T16:14:37Z

...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java

+  }
+
+  public final List<String> getPolicyResourceDefines() {
+    return policyResourceDefines;


xunliu · 2024-09-18T16:14:43Z

...rization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerPrivilege.java

+public interface RangerPrivilege {
+  String getName();
+
+  boolean isEquals(String value);


xunliu · 2024-09-18T16:14:56Z

...st-common/src/test/java/org/apache/gravitino/integration/test/container/RangerContainer.java

@@ -38,7 +38,7 @@ public class RangerContainer extends BaseContainer {
  public static final String DEFAULT_IMAGE = System.getenv("GRAVITINO_CI_RANGER_DOCKER_IMAGE");
  public static final String HOST_NAME = "gravitino-ci-ranger";
  public static final int RANGER_SERVER_PORT = 6080;
-  public RangerClient rangerClient;
+  public RangerClientExtend rangerClient;


Changed to RangerClientExtension.

xunliu · 2024-09-18T16:15:20Z

...r/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHivePlugin.java

+    return ImmutableList.of(
+        PolicyResource.DATABASE.getName(),
+        PolicyResource.TABLE.getName(),
+        PolicyResource.COLUMN.getName());


Changed to singleton.

xunliu requested a review from yuqi1129 August 28, 2024 14:12

xunliu self-assigned this Aug 28, 2024

xunliu force-pushed the issue-4698 branch from 3d0d3b6 to a8ef65c Compare August 29, 2024 03:25

xunliu changed the title ~~[#4698] feat(core): Extended Apache Ranger authorization by configuration~~ [#4698] feat(auth-ranger): Extended Ranger authorization by configuration Aug 29, 2024

xunliu force-pushed the issue-4698 branch from a8ef65c to ed8e81d Compare August 29, 2024 05:40

yuqi1129 reviewed Aug 30, 2024

View reviewed changes

jerryshao reviewed Sep 9, 2024

View reviewed changes

xunliu force-pushed the issue-4698 branch from 3155ecf to 156e57f Compare September 10, 2024 13:18

xunliu commented Sep 10, 2024

View reviewed changes

jerryshao reviewed Sep 14, 2024

View reviewed changes

xunliu added 8 commits September 18, 2024 11:54

[apache#4698] feat(core): Extended Apache Ranger authorization by con…

9fe3aa2

…figuration

remove .template suffix

fefc415

fix

9af7222

override default configure

dc0b6f0

move config file

87d52af

remove configuration

aa99c5c

update docs

acefec2

rollback gradlew script change

1542d74

xunliu force-pushed the issue-4698 branch 3 times, most recently from 449d564 to be618e8 Compare September 18, 2024 05:54

rename interface name

37a5048

xunliu force-pushed the issue-4698 branch from be618e8 to 37a5048 Compare September 18, 2024 06:22

jerryshao reviewed Sep 18, 2024

View reviewed changes

add return type

ee217a3

jerryshao reviewed Sep 18, 2024

View reviewed changes

xunliu changed the title ~~[#4698] feat(auth-ranger): Extended Ranger authorization by configuration~~ [#4698] feat(auth-ranger): Extended Ranger authorization by rules Sep 18, 2024

singleton

86731ca

xunliu force-pushed the issue-4698 branch from cc8e655 to 86731ca Compare September 18, 2024 15:14

xunliu commented Sep 18, 2024

View reviewed changes

jerryshao approved these changes Sep 19, 2024

View reviewed changes

jerryshao merged commit aa988e1 into apache:main Sep 19, 2024
26 checks passed

		Privilege.Name gravitinoPrivilege =
		Privilege.Name.valueOf(key.trim().toUpperCase());

[#4698] feat(auth-ranger): Extended Ranger authorization by rules #4744

[#4698] feat(auth-ranger): Extended Ranger authorization by rules #4744

Conversation

xunliu commented Aug 28, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

xunliu commented Aug 28, 2024

lw-yang commented Aug 30, 2024

xunliu commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuqi1129 commented Sep 2, 2024

jerryshao commented Sep 4, 2024

jerryshao commented Sep 5, 2024

xunliu commented Sep 6, 2024 • edited Loading

lw-yang commented Sep 6, 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

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

xunliu commented Sep 10, 2024

jerqi commented Sep 12, 2024

xunliu commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu Sep 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu commented Sep 18, 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

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

jerryshao commented Sep 18, 2024

xunliu 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

xunliu commented Aug 28, 2024 •

edited

Loading

xunliu commented Sep 6, 2024 •

edited

Loading

xunliu Sep 14, 2024 •

edited

Loading