[KYUUBI #3020] kyuubi ldap add new config property kyuubi.authentication.ldap.bindpw and kyuubi.authentication.ldap.attrs by dev-lpq · Pull Request #3213 · apache/kyuubi

dev-lpq · 2022-08-10T06:19:05Z

Why are the changes needed?

Among the kyuubi ldap configuration parameters, there is currently "kyuubi.authentication.ldap.base.dn=", but there is no place corresponding to the CN password setting. similar to hive
"hive.server2.authentication.ldap.binddn",
"hive.server2.authentication.ldap.bindpw".

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request

…w and kyuubi.authentication.ldap.attrs

dev-lpq · 2022-08-10T06:40:56Z

1.set kyuubi config

2.connect kyuubi successfully

3.Test LDAP InitialLdapContext failed

4.Test LDAP InitialDirContext failed

yaooqinn

LGTM, only minor revisions

yaooqinn · 2022-08-10T07:45:33Z

btw, #3213 (comment) these UTs can be added to the codebase too?

dev-lpq · 2022-08-10T08:18:51Z

btw, #3213 (comment) these UTs can be added to the codebase too?
The ldap UTs code will be added.

…w and kyuubi.authentication.ldap.attrs

dev-lpq · 2022-08-15T06:28:59Z

btw, #3213 (comment) these UTs can be added to the codebase too?
The ldap UTs code will be added.

@yaooqinn Please help check the PR, I have added the LDAP UTs.

yaooqinn · 2022-08-15T08:40:10Z

cc @turboFei can you check this, since LDAP is adopted in your environment

turboFei · 2022-08-16T05:10:50Z

BatchRestApiSuite:
- basic batch rest client
- basic batch rest client with invalid user *** FAILED ***
  "org.apache.http.client.HttpResponseException: status code: 403, reason phrase: <html>
  <head>
  <meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1"/>
  <title>Error 403 LDAP InitialLdapContext search results are empty, LDAP user: user.</title>
  </head>
  <body><h2>HTTP ERROR 403 LDAP InitialLdapContext search results are empty, LDAP user: user.</h2>
  <table>
  <tr><th>URI:</th><td>/api/v1/batches/1</td></tr>
  <tr><th>STATUS:</th><td>403</td></tr>
  <tr><th>MESSAGE:</th><td>LDAP InitialLdapContext search results are empty, LDAP user: user.</td></tr>
  <tr><th>SERVLET:</th><td>org.glassfish.jersey.servlet.ServletContainer-7b1d7f74</td></tr>
  </table>
  <hr/><a href="https://eclipse.org/jetty">Powered by Jetty:// 9.4.48.v20220622</a><hr/>
  
  </body>
  </html>
  " did not contain "Error validating LDAP user: uid=user" (BatchRestApiSuite.scala:75)

could you help fix above UT failure?

turboFei · 2022-08-16T05:11:33Z

AllKyuubiConfiguration:
- Check all kyuubi configs *** FAILED ***
  java.lang.AssertionError: assertion failed: /home/runner/work/incubator-kyuubi/incubator-kyuubi/docs/deployment/settings.md out of date, please update doc with KYUUBI_UPDATE=1 build/mvn clean install -Pflink-provided,spark-provided,hive-provided -DwildcardSuites=org.apache.kyuubi.config.AllKyuubiConfiguration

KYUUBI_UPDATE=1 build/mvn clean install -Pflink-provided,spark-provided,hive-provided -DwildcardSuites=org.apache.kyuubi.config.AllKyuubiConfiguration

turboFei · 2022-08-16T05:33:49Z

+    val bindnPw = conf.get(AUTHENTICATION_LDAP_PASSWORD).get
+    val attrs = conf.get(AUTHENTICATION_LDAP_ATTRIBUTES).toArray

+    env.put(Context.SECURITY_PRINCIPAL, guidKey)


According to https://docs.cloudera.com/cdp-private-cloud-base/7.1.6/using-das/topics/das-user-authentication-ldap.html

LDAP GUID Key: Specify the LDAP attribute name whose values are unique on this LDAP server. For example: uid or CN.

GUID Key is not used for SECURITY_PRINCIPAL.

dev-lpq · 2022-08-16T07:33:30Z

BatchRestApiSuite:
- basic batch rest client
- basic batch rest client with invalid user *** FAILED ***
  "org.apache.http.client.HttpResponseException: status code: 403, reason phrase: <html>
  <head>
  <meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1"/>
  <title>Error 403 LDAP InitialLdapContext search results are empty, LDAP user: user.</title>
  </head>
  <body><h2>HTTP ERROR 403 LDAP InitialLdapContext search results are empty, LDAP user: user.</h2>
  <table>
  <tr><th>URI:</th><td>/api/v1/batches/1</td></tr>
  <tr><th>STATUS:</th><td>403</td></tr>
  <tr><th>MESSAGE:</th><td>LDAP InitialLdapContext search results are empty, LDAP user: user.</td></tr>
  <tr><th>SERVLET:</th><td>org.glassfish.jersey.servlet.ServletContainer-7b1d7f74</td></tr>
  </table>
  <hr/><a href="https://eclipse.org/jetty">Powered by Jetty:// 9.4.48.v20220622</a><hr/>
  
  </body>
  </html>
  " did not contain "Error validating LDAP user: uid=user" (BatchRestApiSuite.scala:75)

could you help fix above UT failure?

fixed

pan3793 · 2022-08-17T03:02:37Z

The change LGTM, please rebase master and resolve conflicts

turboFei · 2022-08-17T03:29:12Z

-        user
-      }
-
-    val guidKey = conf.get(AUTHENTICATION_LDAP_GUIDKEY)


seems AUTHENTICATION_LDAP_GUIDKEY is not used now.

Can we remove it? cc @pan3793

Is it a break change?

it is a break change, we should keep backward compatibility.

@dev-lpq

could you deprecate AUTHENTICATION_LDAP_GUIDKEY?

If the AUTHENTICATION_LDAP_BINDDN is specified, using it first.

Otherwise, fallback to use original logical to get SECURITY_PRINCIPAL?

codecov-commenter · 2022-08-17T08:22:31Z

Codecov Report

Merging #3213 (31817cc) into master (a63587b) will increase coverage by 0.26%.
The diff coverage is 97.33%.

@@             Coverage Diff              @@
##             master    #3213      +/-   ##
============================================
+ Coverage     51.06%   51.33%   +0.26%     
  Complexity       13       13              
============================================
  Files           470      468       -2     
  Lines         26224    26205      -19     
  Branches       3626     3631       +5     
============================================
+ Hits          13392    13452      +60     
+ Misses        11556    11465      -91     
- Partials       1276     1288      +12

Impacted Files	Coverage Δ
...uthentication/LdapAuthenticationProviderImpl.scala	`95.38% <96.15%> (+3.38%)`	⬆️
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala	`97.05% <100.00%> (-0.35%)`	⬇️
...ubi/engine/spark/operation/PlanOnlyStatement.scala	`62.22% <0.00%> (-4.45%)`	⬇️
...rg/apache/kyuubi/ctl/cmd/log/LogBatchCommand.scala	`78.00% <0.00%> (-4.00%)`	⬇️
...ache/kyuubi/operation/KyuubiOperationManager.scala	`80.82% <0.00%> (-1.37%)`	⬇️
...he/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala	`67.50% <0.00%> (-0.63%)`	⬇️
...a/org/apache/kyuubi/service/TFrontendService.scala	`91.17% <0.00%> (-0.30%)`	⬇️
...c/main/java/org/apache/kyuubi/jdbc/hive/Utils.java	`36.78% <0.00%> (-0.11%)`	⬇️
.../java/org/apache/kyuubi/jdbc/KyuubiHiveDriver.java	`4.87% <0.00%> (ø)`
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

turboFei · 2022-08-18T01:28:43Z

Now the only remaining comments is #3213 (comment)

how about fallback to get binddn according to guidKey & baseDn just like before if AUTHENTICATION_LDAP_BINDDN is not specified?

Otherwise, the customers that use the old kyuubi version will meet failure after upgrading kyuubi.

@dev-lpq

turboFei · 2022-08-18T10:19:04Z

thanks for your contribution @dev-lpq , will merge it after UT passed.

turboFei · 2022-08-18T12:29:36Z

thanks @dev-lpq , merged to master

turboFei · 2022-08-18T12:31:07Z

could you bind your email(pengqli@cisco.com) to your github account?

FYI: https://github.com/settings/emails

@dev-lpq

dev-lpq · 2022-08-18T12:46:58Z

could you bind your email(pengqli@cisco.com) to your github account?

FYI: https://github.com/settings/emails

@dev-lpq

@turboFei Added, thanks.

pan3793 · 2022-08-18T14:43:32Z

merged to 1.6 as well

…ion.ldap.bindpw and kyuubi.authentication.ldap.attrs ### _Why are the changes needed?_ Among the kyuubi ldap configuration parameters, there is currently "kyuubi.authentication.ldap.base.dn=", but there is no place corresponding to the CN password setting. similar to hive "hive.server2.authentication.ldap.binddn", "hive.server2.authentication.ldap.bindpw". ### _How was this patch tested?_ - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [x] Add screenshots for manual tests if appropriate - [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #3213 from dev-lpq/branch-ldap-1.6. Closes #3020 31817cc [pengqli] upgrade ldap to support ldap before 1.6.0 3109403 [pengqli] upgrade ldap to support ldap before 1.6.0 f62e4c9 [pengqli] upgrade ldap to support ldap before 1.6.0 e03e036 [pengqli] add AUTHENTICATION_LDAP_GUIDKEY deprecated 6f680df [pengqli] add ldap config AUTHENTICATION_LDAP_BINDDN 19f8e6c [pengqli] add ldap config AUTHENTICATION_LDAP_BINDDN 07df56b [pengqli] upgrade LdapAuthenticationProviderImpl 1a48f41 [pengqli] upgrade settings.md 8e9b572 [pengqli] upgrade ldap ldapBindpw 5793a81 [pengqli] upgrade ldap ldapBindpw 822d4c3 [pengqli] upgrade docs/deployment/settings.md c2b2041 [pengqli] upgrade ldap WithLdapServer UTs 5d7a301 [pengqli] add AUTHENTICATION_LDAP_PASSWORD into serverOnlyConfEntries 10b42b1 [pengqli] upgrade AuthenticationProviderFactorySuite UTs 490bfae [pengqli] kyuubi ldap add new config property kyuubi.authentication.ldap.bindnpw and kyuubi.authentication.ldap.attrs 5e07125 [pengqli] upgrafe config kyuubi.authentication.ldap.attrs 4e771ec [pengqli] kyuubi ldap add new config property kyuubi.authentication.ldap.bindnpw and kyuubi.authentication.ldap.attrs Authored-by: pengqli <pengqli@cisco.com> Signed-off-by: Fei Wang <fwang12@ebay.com>

pan3793 · 2022-08-24T04:21:37Z

@dev-lpq @turboFei after a detailed look and comparing to the hive codebase, I think the current implementation is not mature. As 1.6 is going to RC, I prefer to revert this change in branch-1.6 and leave it on master to get more time to improve it.

pan3793 · 2022-09-08T08:33:01Z

During the review period of #3457, I found that some changes in this PR are confusing, I'm going to revert this on master and rework it in #3457

dev-lpq · 2022-09-08T09:11:16Z

@pan3793 Thanks, what is confusing? I will research the #3457, and submit a version that better matches hive ldap mode.

pan3793 · 2022-09-08T09:24:54Z

The following logic is not correct I think

val mail = if (!hasDomain(user) && domain.nonEmpty) (user + "@" + domain.get) else user
...
nameEnuResults = ctx.search(baseDn, s"(mail=$mail)", sc)

I can not fully understand what's purpose of kyuubi.authentication.ldap.attrs by reading the description "Specifies part of the search as an attribute returned by LDAP. For example: mail,name." and code. BTW, I don't find a similar conf key in Hive.

I don't have much experience w/ LDAP, and I would like to provide similar configurations w/ Hive unless we have strong reasons that we can give a better solution.

pan3793 · 2022-09-08T09:29:56Z

Basically, the principle is to give users the same(or better) configuration keys w/ Hive.

For implementation, unboundid-ldapsdk claims to provide better API than JDK, I think it is valued to have a try, another option is referring to Hive code.

dev-lpq · 2022-09-08T10:39:12Z

Thank you for your answer, the current way to verify ldap:
1.access to ldap datasource
kyuubi.authentication.ldap.bindpw & kyuubi.authentication.ldap.binddn
2. query target user info
nameEnuResults = ctx.search(baseDn, s"(mail=$mail)", sc)
3. check the account and password according to kyuubi.authentication.ldap.attrs.

I will research unboundid-ldapsdk and hive LDAP , and provide a better solution.

pan3793 · 2022-09-08T13:25:45Z

@dev-lpq, thanks for your passion and patience

kyuubi ldap add new config property kyuubi.authentication.ldap.bindnp…

4e771ec

…w and kyuubi.authentication.ldap.attrs

github-actions bot added kind:documentation Documentation is a feature! module:common labels Aug 10, 2022

yaooqinn reviewed Aug 10, 2022

View reviewed changes

Comment thread kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala

yaooqinn reviewed Aug 10, 2022

View reviewed changes

Comment thread kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala Outdated

yaooqinn approved these changes Aug 10, 2022

View reviewed changes

dev-lpq added 2 commits August 15, 2022 11:20

upgrafe config kyuubi.authentication.ldap.attrs

5e07125

kyuubi ldap add new config property kyuubi.authentication.ldap.bindnp…

490bfae

…w and kyuubi.authentication.ldap.attrs

yaooqinn reviewed Aug 15, 2022

View reviewed changes

Comment thread kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala

upgrade AuthenticationProviderFactorySuite UTs

10b42b1

turboFei reviewed Aug 15, 2022

View reviewed changes

Comment thread kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala

dev-lpq added 2 commits August 15, 2022 17:17

add AUTHENTICATION_LDAP_PASSWORD into serverOnlyConfEntries

5d7a301

upgrade ldap WithLdapServer UTs

c2b2041

github-actions bot added the module:server label Aug 16, 2022

turboFei reviewed Aug 16, 2022

View reviewed changes

Comment thread ...src/main/scala/org/apache/kyuubi/service/authentication/LdapAuthenticationProviderImpl.scala Outdated

turboFei reviewed Aug 16, 2022

View reviewed changes

Comment thread ...src/main/scala/org/apache/kyuubi/service/authentication/LdapAuthenticationProviderImpl.scala Outdated

upgrade docs/deployment/settings.md

822d4c3

turboFei reviewed Aug 16, 2022

View reviewed changes

Comment thread kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/WithLdapServer.scala Outdated

turboFei reviewed Aug 16, 2022

View reviewed changes

dev-lpq added 2 commits August 16, 2022 15:01

upgrade ldap ldapBindpw

5793a81

upgrade ldap ldapBindpw

8e9b572

add ldap config AUTHENTICATION_LDAP_BINDDN

19f8e6c

pan3793 approved these changes Aug 17, 2022

View reviewed changes

turboFei reviewed Aug 17, 2022

View reviewed changes

add ldap config AUTHENTICATION_LDAP_BINDDN

6f680df

turboFei reviewed Aug 17, 2022

View reviewed changes

Comment thread kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala

add AUTHENTICATION_LDAP_GUIDKEY deprecated

e03e036

dev-lpq added 3 commits August 18, 2022 15:26

upgrade ldap to support ldap before 1.6.0

f62e4c9

upgrade ldap to support ldap before 1.6.0

3109403

upgrade ldap to support ldap before 1.6.0

31817cc

turboFei changed the title ~~[KYUUBI #3020] kyuubi ldap add new config property kyuubi.authentication.ldap.bindnpw and kyuubi.authentication.ldap.attrs~~ [KYUUBI #3020] kyuubi ldap add new config property kyuubi.authentication.ldap.bindpw and kyuubi.authentication.ldap.attrs Aug 18, 2022

turboFei approved these changes Aug 18, 2022

View reviewed changes

turboFei closed this in 5d88c7b Aug 18, 2022

pan3793 added this to the v1.6.0 milestone Aug 18, 2022

Conversation

dev-lpq commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are the changes needed?

How was this patch tested?

Uh oh!

dev-lpq commented Aug 10, 2022

Uh oh!

Uh oh!

Uh oh!

yaooqinn left a comment

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Aug 10, 2022

Uh oh!

dev-lpq commented Aug 10, 2022

Uh oh!

dev-lpq commented Aug 15, 2022

Uh oh!

Uh oh!

yaooqinn commented Aug 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

turboFei commented Aug 16, 2022

Uh oh!

turboFei commented Aug 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

turboFei Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

dev-lpq commented Aug 16, 2022

Uh oh!

pan3793 commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

turboFei Aug 17, 2022

Choose a reason for hiding this comment

Uh oh!

pan3793 Aug 17, 2022

Choose a reason for hiding this comment

Uh oh!

turboFei Aug 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

turboFei commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

turboFei commented Aug 18, 2022

Uh oh!

turboFei commented Aug 18, 2022

Uh oh!

turboFei commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dev-lpq commented Aug 18, 2022

Uh oh!

pan3793 commented Aug 18, 2022

Uh oh!

pan3793 commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan3793 commented Sep 8, 2022

Uh oh!

dev-lpq commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan3793 commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dev-lpq commented Aug 10, 2022 •

edited

Loading

pan3793 commented Aug 17, 2022 •

edited

Loading

codecov-commenter commented Aug 17, 2022 •

edited

Loading

turboFei commented Aug 18, 2022 •

edited

Loading

turboFei commented Aug 18, 2022 •

edited

Loading

pan3793 commented Aug 24, 2022 •

edited

Loading

dev-lpq commented Sep 8, 2022 •

edited

Loading

pan3793 commented Sep 8, 2022 •

edited

Loading