[#1185] feat(server-common): Server supports Kerberos authentication #1614

qqqttt123 · 2024-01-19T08:03:57Z

What changes were proposed in this pull request?

Server supports Kerberos authentication. I referred to the implementation of Hadoop.
I will submit another pr about the client.

Why are the changes needed?

Fix: #1185.

Does this PR introduce any user-facing change?

Added the document.

How was this patch tested?

Add a new UT.

mchades · 2024-01-22T03:25:14Z

openapi.yaml should also be modified

qqqttt123 · 2024-01-22T03:50:41Z

openapi.yaml should also be modified

Added.

qqqttt123 · 2024-01-24T10:46:10Z

@mchades @jerryshao @yuqi1129 @diqiu50 @FANNG1 @Clearvive Could you help me review this pr?

qqqttt123 · 2024-01-24T12:30:45Z

server-common/src/main/java/com/datastrato/gravitino/server/auth/KerberosAuthenticator.java

+        String challenge = AuthConstants.AUTHORIZATION_NEGOTIATE_HEADER + authenticateToken;
+        throw new UnauthorizedException("GssContext isn't established", challenge);
+      }
+      // Usually principal names are in the form 'user/instance@REALM' or 'user@REALM'.


I simplify the logic of convertion from principal to user.

qqqttt123 · 2024-01-24T12:31:48Z

server-common/src/main/java/com/datastrato/gravitino/server/auth/KerberosAuthenticator.java

+  @Override
+  public void initialize(Config config) throws RuntimeException {
+    try {
+      String principal = config.get(KerberosConfig.PRINCIPAL);


We don't support * principal.

Please add this to the comment.

qqqttt123 · 2024-01-24T12:32:08Z

server-common/src/main/java/com/datastrato/gravitino/server/auth/KerberosAuthenticator.java

+      if (!keytabFile.exists()) {
+        throw new IllegalArgumentException("Keytab doesn't exist: " + keytab);
+      }
+


We remove the validation of rule names.

jerryshao · 2024-01-24T15:12:35Z

docs/security.md

-| `gravitino.authenticator.oauth.tokenPath`         | The path for token of the default OAuth server.                             | (none)            | Yes if use `oauth` as the authenticator | 0.3.0         |
+| Configuration item                                | Description                                                                 | Default value     | Required                                   | Since version |
+|---------------------------------------------------|-----------------------------------------------------------------------------|-------------------|--------------------------------------------|---------------|
+| `gravitino.authenticator`                         | The authenticator which Gravitino uses, setting as `simple` or `oauth`.     | `simple`          | No                                         | 0.3.0         |


Do you need to update this?

jerryshao · 2024-01-24T15:14:49Z

server-common/src/main/java/com/datastrato/gravitino/server/auth/AuthenticationFilter.java

+        for (String challenge : ue.getChallenges()) {
+          resp.setHeader(AuthConstants.HTTP_CHALLENGE_HEADER, challenge);
+        }
+      }


Please add more comment here about the reason of this change.

jerryshao · 2024-01-24T15:16:29Z

server-common/src/main/java/com/datastrato/gravitino/server/auth/KerberosAuthenticator.java

+  @Override
+  public void initialize(Config config) throws RuntimeException {
+    try {
+      String principal = config.get(KerberosConfig.PRINCIPAL);


Please add this to the comment.

jerryshao · 2024-01-25T03:01:06Z

build.gradle.kts

@@ -78,7 +78,8 @@ project.extra["extraJvmArgs"] = if (extra["jdkVersion"] in listOf("8", "11")) {
    "--add-opens", "java.base/sun.nio.ch=ALL-UNNAMED",
    "--add-opens", "java.base/sun.nio.cs=ALL-UNNAMED",
    "--add-opens", "java.base/sun.security.action=ALL-UNNAMED",
-    "--add-opens", "java.base/sun.util.calendar=ALL-UNNAMED"
+    "--add-opens", "java.base/sun.util.calendar=ALL-UNNAMED",
+    "--add-opens", "java.security.jgss/sun.security.krb5=ALL-UNNAMED"


I think you should also update the start script under "bin".

jerryshao · 2024-01-25T03:10:48Z

common/src/main/java/com/datastrato/gravitino/auth/KerberosUtils.java

+// Referred from Apache Hadoop KerberosTestUtils.java
+// hadoop-common-project/hadoop-auth/src/test/java/org/apache/hadoop/security/\
+// authentication/KerberosTestUtils.java
+public class KerberosUtils {


Still the same thing here, please point out which part did you modify for what purpose.

jerryshao · 2024-01-25T03:13:34Z

common/src/main/java/com/datastrato/gravitino/auth/KerberosUtils.java

+      Subject subject = new Subject(false, principals, new HashSet<>(), new HashSet<>());
+      loginContext =
+          new LoginContext("", subject, null, new KerberosConfiguration(principal, keyTabFile));
+      loginContext.login();


Do we have to login everytime, is it a typical implementation?

Split this into two methods.

jerryshao · 2024-01-25T03:19:31Z

server-common/src/main/java/com/datastrato/gravitino/server/auth/KerberosAuthenticator.java

+      String principal = config.get(KerberosConfig.PRINCIPAL);
+      String keytab = config.get(KerberosConfig.KEYTAB);
+      File keytabFile = new File(keytab);
+      if (!keytabFile.exists()) {


You should also check the permission file.

jerryshao · 2024-01-25T03:22:22Z

server-common/src/main/java/com/datastrato/gravitino/server/auth/KerberosAuthenticator.java

+          new PrivilegedExceptionAction<Principal>() {
+            @Override
+            public Principal run() throws Exception {
+              return validClientToken(serverPrincipal, clientToken);


validateClientToken?

jerryshao · 2024-01-25T03:24:38Z

server-common/src/main/java/com/datastrato/gravitino/server/auth/KerberosUtil.java

+
+// Referred from Apache Hadoop KerberosUtil.java
+// hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/\
+// security/authentication/util/KerberosUtil.java


Also here, please point out which part did you modify for what purpose?

jerryshao · 2024-01-25T09:43:17Z

common/src/main/java/com/datastrato/gravitino/auth/KerberosUtils.java

+      getNumericOidInstance("1.2.840.113554.1.2.2.1");
+
+  // numeric oids will never generate a GSSException for a malformed oid.
+  // use to initialize statics.


Capitalize the first letter.

jerryshao · 2024-01-25T09:43:35Z

common/src/main/java/com/datastrato/gravitino/auth/KerberosUtils.java

+    }
+  }
+
+  public static LoginContext login(String principal, String keyTabFile) throws LoginException {


Do you need a logout method?

I will call LoginContext.logout

jerryshao · 2024-01-25T09:54:16Z

server-common/src/main/java/com/datastrato/gravitino/server/auth/KerberosAuthenticator.java

+    }
+  }
+
+  private Principal validateClientToken(String serverPrincipal, byte[] clientToken)


This method doesn't only validate the token, but also return the user principal if the check is passed. I would suggest use a better method name.

How about retrievePrincipalFromToken?

Heng Qin added 6 commits January 17, 2024 14:50

kerberos

7da6d1b

fix

2875d79

fix

96d054d

fix

de6629c

fix

e2ba106

fix

d528823

qqqttt123 marked this pull request as draft January 19, 2024 08:04

Heng Qin added 10 commits January 19, 2024 16:19

fix

e6e93f2

Merge remote-tracking branch 'upstream/main' into ISSUE-1185

f8c8918

fix

f8c4bc8

fix

d255848

fix

a53cf2e

Merge remote-tracking branch 'upstream/main' into ISSUE-1185

701e558

fix

f25fa23

fix

88f3c68

fix

d82cc41

fix

83a98de

qqqttt123 marked this pull request as ready for review January 19, 2024 10:57

qqqttt123 requested review from jerryshao, FANNG1, xunliu, diqiu50, yuqi1129, mchades and Clearvive and removed request for jerryshao January 19, 2024 10:57

address comments

af1205d

Heng Qin added 2 commits January 22, 2024 15:24

fix

3bb5d4d

fix

c0b133f

qqqttt123 added this to the Gravitino 0.4.0 milestone Jan 24, 2024

qqqttt123 self-assigned this Jan 24, 2024

Heng Qin added 5 commits January 24, 2024 19:50

Merge remote-tracking branch 'upstream/main' into ISSUE-1185

f3ba81e

Modify licence.bin

0423104

Add license

82c43cc

fix

e9a859b

fix

0c7699f

qqqttt123 commented Jan 24, 2024

View reviewed changes

Heng Qin added 2 commits January 24, 2024 20:41

optimize ut

4fa5522

fix

34aa6f1

jerryshao reviewed Jan 25, 2024

View reviewed changes

Heng Qin added 9 commits January 25, 2024 11:29

Add proper blank

0339177

address comments

3738aa5

fix

9f59254

fix

14b7fa7

Merge remote-tracking branch 'upstream/main' into ISSUE-1185

d4a252d

fix

7c39424

fix

62c782f

Merge remote-tracking branch 'upstream/main' into ISSUE-1185

247ce36

fix

ada86ac

qqqttt123 requested a review from jerryshao January 25, 2024 09:55

jerryshao reviewed Jan 25, 2024

View reviewed changes

address comments

e7aa618

qqqttt123 requested a review from jerryshao January 25, 2024 11:10

jerryshao approved these changes Jan 25, 2024

View reviewed changes

jerryshao merged commit 173dd07 into apache:main Jan 25, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1185] feat(server-common): Server supports Kerberos authentication #1614

[#1185] feat(server-common): Server supports Kerberos authentication #1614

qqqttt123 commented Jan 19, 2024 •

edited

Loading

mchades commented Jan 22, 2024

qqqttt123 commented Jan 22, 2024 •

edited

Loading

qqqttt123 commented Jan 24, 2024

qqqttt123 Jan 24, 2024 •

edited

Loading

qqqttt123 Jan 24, 2024

jerryshao Jan 24, 2024

qqqttt123 Jan 25, 2024

qqqttt123 Jan 24, 2024

jerryshao Jan 24, 2024

qqqttt123 Jan 25, 2024

jerryshao Jan 24, 2024

qqqttt123 Jan 25, 2024

jerryshao Jan 24, 2024

jerryshao Jan 25, 2024

qqqttt123 Jan 25, 2024

jerryshao Jan 25, 2024

qqqttt123 Jan 25, 2024

jerryshao Jan 25, 2024

qqqttt123 Jan 25, 2024

jerryshao Jan 25, 2024

qqqttt123 Jan 25, 2024

jerryshao Jan 25, 2024

qqqttt123 Jan 25, 2024

jerryshao Jan 25, 2024

qqqttt123 Jan 25, 2024

jerryshao Jan 25, 2024

qqqttt123 Jan 25, 2024

jerryshao Jan 25, 2024

qqqttt123 Jan 25, 2024

jerryshao Jan 25, 2024

qqqttt123 Jan 25, 2024

[#1185] feat(server-common): Server supports Kerberos authentication #1614

[#1185] feat(server-common): Server supports Kerberos authentication #1614

Conversation

qqqttt123 commented Jan 19, 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?

mchades commented Jan 22, 2024

qqqttt123 commented Jan 22, 2024 • edited Loading

qqqttt123 commented Jan 24, 2024

qqqttt123 Jan 24, 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

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

qqqttt123 commented Jan 19, 2024 •

edited

Loading

qqqttt123 commented Jan 22, 2024 •

edited

Loading

qqqttt123 Jan 24, 2024 •

edited

Loading