[#5790] auth(chain): Chain authorization properties #5791

xunliu · 2024-12-07T05:59:39Z

What changes were proposed in this pull request?

Add AuthorizationProperties interface
Add ChainAuthorizationProperties class

Why are the changes needed?

Fix: #5790

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Add test

orenccl · 2024-12-07T17:21:55Z

Kindly note that this PR should reference fix:5790, not fix:5779.

xunliu · 2024-12-09T00:45:42Z

@orenccl Thank you for your help check it. Thanks.

core/src/main/java/org/apache/gravitino/connector/WildcardPropertiesMeta.java

core/src/main/java/org/apache/gravitino/connector/AuthorizationPropertiesMeta.java

core/src/main/java/org/apache/gravitino/connector/WildcardPropertiesMeta.java

core/src/main/java/org/apache/gravitino/connector/AuthorizationPropertiesMeta.java

xunliu · 2024-12-09T11:50:08Z

@mchades I fixed all problems, Please help me review again, thanks!

core/src/main/java/org/apache/gravitino/connector/WildcardPropertiesMetadata.java

core/src/main/java/org/apache/gravitino/connector/PropertyEntry.java

core/src/main/java/org/apache/gravitino/authorization/AuthorizationPropertiesMetadata.java

core/src/main/java/org/apache/gravitino/connector/WildcardPropertiesMetadata.java

core/src/main/java/org/apache/gravitino/authorization/AuthorizationPropertiesMetadata.java

core/src/main/java/org/apache/gravitino/connector/WildcardPropertiesMetadata.java

xunliu · 2024-12-10T10:19:57Z

@mchades I fixed all the problems, please help me review it again. Thanks.

core/src/main/java/org/apache/gravitino/connector/WildcardPropertiesMetadata.java

xunliu · 2024-12-11T11:23:12Z

@mchades I fixed all the problems, please help me review it again. Thanks.

xunliu · 2024-12-13T07:43:26Z

@tengqm @mchades @yuqi1129 I am refactoring this PR, Please help me review it, Thanks.

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

...-ranger/src/main/java/org/apache/gravitino/authorization/ranger/AuthorizationProperties.java

...er/src/main/java/org/apache/gravitino/authorization/ranger/ChainAuthorizationProperties.java

...src/test/java/org/apache/gravitino/connector/authorization/mysql/TestMySQLAuthorization.java

...c/test/java/org/apache/gravitino/connector/authorization/ranger/TestRangerAuthorization.java

...-ranger/src/main/java/org/apache/gravitino/authorization/ranger/AuthorizationProperties.java

...er/src/main/java/org/apache/gravitino/authorization/ranger/ChainAuthorizationProperties.java

mchades · 2024-12-16T09:42:54Z

...er/src/main/java/org/apache/gravitino/authorization/ranger/ChainAuthorizationProperties.java

+      }
+    }
+
+    resultProperties.forEach((key, value) -> System.out.println(key + " = " + value));


plz remove the println

jerqi · 2024-12-16T09:12:23Z

...er/src/main/java/org/apache/gravitino/authorization/ranger/ChainAuthorizationProperties.java

+ * "authorization.chain.hive1.ranger.password" = "admin"; <br>
+ * "authorization.chain.hive1.ranger.service.name" = "hiveDev"; <br>
+ * "authorization.chain.hdfs1.provider" = "ranger"; <br>
+ * "authorization.chain.hdfs1.catalog-provider" = "hadoop"; <br>


This isn't reasonable. The plugins of the chain should have the same catalog provider.

jerqi · 2024-12-16T11:35:50Z

...er/src/main/java/org/apache/gravitino/authorization/ranger/ChainAuthorizationProperties.java

@@ -34,28 +34,25 @@
 * Configuration Example: <br>
 * "authorization.chain.plugins" = "hive1,hdfs1" <br>
 * "authorization.chain.hive1.provider" = "ranger"; <br>
- * "authorization.chain.hive1.catalog-provider" = "hive"; <br>
+ * "authorization.chain.hive1.ranger.service.type" = "hive"; <br>


Hadoop SQL is more suitable?

mchades · 2024-12-16T11:49:35Z

...er/src/main/java/org/apache/gravitino/authorization/ranger/ChainAuthorizationProperties.java

+        properties.containsKey(CHAIN_PLUGINS_PROPERTIES_KEY),
+        String.format("%s is required", CHAIN_PLUGINS_PROPERTIES_KEY));
+
+    String[] pluginNames = properties.get(CHAIN_PLUGINS_PROPERTIES_KEY).split(PLUGINS_SPLITTER);


Potential NPE when properties.get(CHAIN_PLUGINS_PROPERTIES_KEY) return null

mchades · 2024-12-16T11:52:07Z

...er/src/main/java/org/apache/gravitino/authorization/ranger/ChainAuthorizationProperties.java

+
+    String[] pluginNames = properties.get(CHAIN_PLUGINS_PROPERTIES_KEY).split(PLUGINS_SPLITTER);
+    Preconditions.checkArgument(
+        pluginNames.length > 0,


redundant check since the below check Arrays.asList(pluginNames).contains(pluginName) already includes this

jerqi · 2024-12-17T03:16:31Z

docs/security/authorization-pushdown.md

@@ -24,7 +24,8 @@ In order to use the Ranger Hadoop SQL Plugin, you need to configure the followin
 | `authorization.ranger.auth.type`    | The Apache Ranger authentication type `simple` or `kerberos`.                                                                                        | `simple`      | No       | 0.6.0-incubating |
 | `authorization.ranger.username`     | The Apache Ranger admin web login username (auth type=simple), or kerberos principal(auth type=kerberos), Need have Ranger administrator permission. | (none)        | No       | 0.6.0-incubating |
 | `authorization.ranger.password`     | The Apache Ranger admin web login user password (auth type=simple), or path of the keytab file(auth type=kerberos)                                   | (none)        | No       | 0.6.0-incubating |
-| `authorization.ranger.service.name` | The Apache Ranger service name.                                                                                                                      | (none)        | No       | 0.6.0-incubating |
+| `authorization.ranger.service.type` | The Apache Ranger service type.                                                                                                                      | (none)        | No       | 0.6.0-incubating |


0.8.0-incubating?

...er/src/main/java/org/apache/gravitino/authorization/ranger/ChainAuthorizationProperties.java

jerqi · 2024-12-17T07:10:55Z

...er/src/main/java/org/apache/gravitino/authorization/ranger/ChainAuthorizationProperties.java

+        Arrays.asList(pluginNames).contains(pluginName),
+        String.format("pluginName %s must be one of %s", pluginName, Arrays.toString(pluginNames)));
+
+    String regex = "^authorization\\.chain\\.(" + pluginName + ")\\..*";


I hesitate about regex expression. Usually regex expression will make code hard to understand. Is it neccessary although the regex epression seems simple.

I need to use regular expressions:

Check whether the plugin name meets requirements. For example, the plugin name cannot be included . symbol.

Check that the plugin name must be one of several values specified

Delete plugin name from the chain attribute to generate plugin attribute configuration.
Only the simplest regular expressions are used here, which I don't think is a problem.

Maybe we can get the configuration by configuration prefix like authorization.pluginName.xxxx.

ChainAuthorizationProperties is just a regular expression.

"authorization.chain.plugins" = "hive1,hdfs1" "authorization.chain.*.provider" = "ranger"; "authorization.chain.*.ranger.service.type" = "HadoopSQL"; "authorization.chain.*.ranger.service.name" = "hiveDev"; "authorization.chain.*.ranger.auth.type" = "simple"; "authorization.chain.*.ranger.admin.url" = "http://localhost:6080"; "authorization.chain.*.ranger.username" = "admin"; "authorization.chain.*.ranger.password" = "admin"; 

We can use ChainAuthorizationProperties to configuration multiple plugin properties.:

Configuration Example: "authorization.chain.plugins" = "hive1,hdfs1" "authorization.chain.hive1.provider" = "ranger"; "authorization.chain.hive1.ranger.service.type" = "HadoopSQL"; "authorization.chain.hive1.ranger.service.name" = "hiveDev"; "authorization.chain.hive1.ranger.auth.type" = "simple"; "authorization.chain.hive1.ranger.admin.url" = "http://localhost:6080"; "authorization.chain.hive1.ranger.username" = "admin"; "authorization.chain.hive1.ranger.password" = "admin"; "authorization.chain.hdfs1.provider" = "ranger"; "authorization.chain.hdfs1.ranger.service.type" = "HDFS"; "authorization.chain.hdfs1.ranger.service.name" = "hdfsDev"; "authorization.chain.hdfs1.ranger.auth.type" = "simple"; "authorization.chain.hdfs1.ranger.admin.url" = "http://localhost:6080"; "authorization.chain.hdfs1.ranger.username" = "admin"; "authorization.chain.hdfs1.ranger.password" = "admin";

jerqi

After discussion, I still think prefix match is better than regex expression. But I think we don't need to block the pull request because of this point.

xunliu · 2024-12-17T08:18:58Z

We can strictly check user properties configurations in the chain by using regular.
Checking with strings is less elegant.
Can I reference in TestChainAuthorizationProperties.java Some test cases in Java.

xunliu requested a review from mchades December 7, 2024 05:59

xunliu self-assigned this Dec 7, 2024

xunliu changed the title ~~[#5779] feat(core): Wildcard properties meta~~ [#5790] feat(core): Wildcard properties meta Dec 9, 2024

tengqm reviewed Dec 9, 2024

View reviewed changes

core/src/main/java/org/apache/gravitino/connector/WildcardPropertiesMeta.java Outdated Show resolved Hide resolved

mchades reviewed Dec 9, 2024

View reviewed changes

xunliu force-pushed the issue-5790 branch from 9ef2d81 to b2a8dc0 Compare December 9, 2024 10:51

mchades reviewed Dec 9, 2024

View reviewed changes

core/src/main/java/org/apache/gravitino/connector/WildcardPropertiesMetadata.java Outdated Show resolved Hide resolved

mchades reviewed Dec 10, 2024

View reviewed changes

mchades reviewed Dec 11, 2024

View reviewed changes

core/src/main/java/org/apache/gravitino/connector/WildcardPropertiesMetadata.java Outdated Show resolved Hide resolved

mchades reviewed Dec 11, 2024

View reviewed changes

core/src/main/java/org/apache/gravitino/connector/WildcardPropertiesMetadata.java Outdated Show resolved Hide resolved

[apache#5790] auth(chain): Chain authorization properties

1132b3c

xunliu force-pushed the issue-5790 branch from 1179f8a to 1132b3c Compare December 13, 2024 06:34

xunliu changed the title ~~[#5790] feat(core): Wildcard properties meta~~ [#5790] auth(chain): Chain authorization properties Dec 13, 2024

fix CI

bee7f3a

xunliu requested a review from diqiu50 December 13, 2024 07:42

xunliu requested review from yuqi1129 and removed request for diqiu50 December 13, 2024 07:49

mchades reviewed Dec 16, 2024

View reviewed changes

xunliu added 3 commits December 16, 2024 14:12

improve

25b2626

fix CI

07ccc08

fix

885a949

mchades reviewed Dec 16, 2024

View reviewed changes

jerqi reviewed Dec 16, 2024

View reviewed changes

add ranger.service.type property

49981c9

fix CI

8468ff1

jerqi reviewed Dec 16, 2024

View reviewed changes

mchades reviewed Dec 16, 2024

View reviewed changes

xunliu added 2 commits December 17, 2024 11:08

add ranger service type

49924fd

improve

5063e6c

jerqi reviewed Dec 17, 2024

View reviewed changes

doc

44beab2

jerqi reviewed Dec 17, 2024

View reviewed changes

...er/src/main/java/org/apache/gravitino/authorization/ranger/ChainAuthorizationProperties.java Show resolved Hide resolved

jerqi reviewed Dec 17, 2024

View reviewed changes

jerqi approved these changes Dec 17, 2024

View reviewed changes

xunliu merged commit 983ce4b into apache:main Dec 17, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5790] auth(chain): Chain authorization properties #5791

[#5790] auth(chain): Chain authorization properties #5791

xunliu commented Dec 7, 2024 •

edited

Loading

orenccl commented Dec 7, 2024

xunliu commented Dec 9, 2024

xunliu commented Dec 9, 2024

xunliu commented Dec 10, 2024

xunliu commented Dec 11, 2024

xunliu commented Dec 13, 2024

mchades Dec 16, 2024

xunliu Dec 17, 2024

jerqi Dec 16, 2024

xunliu Dec 17, 2024

jerqi Dec 16, 2024

xunliu Dec 17, 2024

mchades Dec 16, 2024

xunliu Dec 17, 2024

mchades Dec 16, 2024

xunliu Dec 17, 2024

jerqi Dec 17, 2024

xunliu Dec 17, 2024

jerqi Dec 17, 2024

xunliu Dec 17, 2024

jerqi Dec 17, 2024

xunliu Dec 17, 2024 •

edited

Loading

jerqi left a comment

xunliu commented Dec 17, 2024

[#5790] auth(chain): Chain authorization properties #5791

[#5790] auth(chain): Chain authorization properties #5791

Conversation

xunliu commented Dec 7, 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?

orenccl commented Dec 7, 2024

xunliu commented Dec 9, 2024

xunliu commented Dec 9, 2024

xunliu commented Dec 10, 2024

xunliu commented Dec 11, 2024

xunliu commented Dec 13, 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

xunliu Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

jerqi left a comment

Choose a reason for hiding this comment

xunliu commented Dec 17, 2024

xunliu commented Dec 7, 2024 •

edited

Loading

xunliu Dec 17, 2024 •

edited

Loading