You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/11/23 04:26:12 UTC

[GitHub] [incubator-kyuubi] bowenliang123 opened a new pull request, #3838: [Authz] support for RSA signing and verifying `kyuubi.session.user`

bowenliang123 opened a new pull request, #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838

   <!--
   Thanks for sending a pull request!
   
   Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
     2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
     3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
   -->
   
   ### _Why are the changes needed?_
   <!--
   Please clarify why the changes are needed. For instance,
     1. If you add a feature, you can talk about the use case of it.
     2. If you fix a bug, you can clarify why it is a bug.
   -->
   
   - Authz plugin rely on `kyuubi.session.user` local property of spark session to identify end user for authorization checking
   - `kyuubi.session.user` could be easily change in scala or python script
   - use RSA to sign and verify the integrity of`kyuubi.session.user`, by generating sign with private key on server side and verifying sign with public key on Spark engine side.
   
   ### _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](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3838: [KYUUBI #3839] [Authz] WIP: Support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1032057950


##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/SparkOperation.scala:
##########
@@ -73,11 +76,24 @@ abstract class SparkOperation(session: Session)
     spark.conf.getOption(KyuubiConf.OPERATION_SCHEDULER_POOL.key).orElse(
       session.sessionManager.getConf.get(KyuubiConf.OPERATION_SCHEDULER_POOL))
 
+  protected val isSessionUserVerifyEnabled: Boolean = spark.sparkContext.getConf.getBoolean(
+    s"spark.${SESSION_USER_VERIFY_ENABLED.key}",
+    SESSION_USER_VERIFY_ENABLED.defaultVal.get)
+
   protected def withLocalProperties[T](f: => T): T = {
     try {
       spark.sparkContext.setJobGroup(statementId, redactedStatement, forceCancel)
       spark.sparkContext.setLocalProperty(KYUUBI_SESSION_USER_KEY, session.user)
       spark.sparkContext.setLocalProperty(KYUUBI_STATEMENT_ID_KEY, statementId)
+
+      if (isSessionUserVerifyEnabled) {
+        val (publicKey, privateKey) = SignUtils.generateKeyPair
+        val signed = SignUtils.signWithECDSA(session.user, privateKey)
+        val publicKeyStr = Base64.getEncoder.encodeToString(publicKey.getEncoded)
+        spark.sparkContext.setLocalProperty(KYUUBI_SESSION_USER_PUBIC_KEY, publicKeyStr)
+        spark.sparkContext.setLocalProperty(KYUUBI_SESSION_USER_SIGN, signed)

Review Comment:
   It's alright to leave these local properties set, as they are generated and refreshed in every statement execution.
   But it's okay to clear them as well. Changes updated.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1035924805


##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/AuthZUtils.scala:
##########
@@ -74,6 +85,10 @@ private[authz] object AuthZUtils {
   def getAuthzUgi(spark: SparkContext): UserGroupInformation = {
     // kyuubi.session.user is only used by kyuubi
     val user = spark.getLocalProperty("kyuubi.session.user")
+    if (StringUtils.isNotBlank(user)) {

Review Comment:
   OK. If `kyuubi.session.user.sign.enabled` is enabled, force to check session user.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3838: [KYUUBI #3839] [Authz] Support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1032948540


##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecutePython.scala:
##########
@@ -74,6 +74,16 @@ class ExecutePython(
         s"($statementId, $redactedStatement, $forceCancel)")
       setSparkLocalProperties(KYUUBI_SESSION_USER_KEY, session.user)
       setSparkLocalProperties(KYUUBI_STATEMENT_ID_KEY, statementId)
+
+      if (isSessionUserVerifyEnabled) {
+        setSparkLocalProperties(
+          KYUUBI_SESSION_SIGN_PUBLICKEY,
+          session.conf.get(KYUUBI_SESSION_SIGN_PUBLICKEY).orNull)
+        setSparkLocalProperties(
+          KYUUBI_SESSION_USER_SIGN,
+          session.conf.get(KYUUBI_SESSION_USER_SIGN).orNull)

Review Comment:
   Yes, these two configs are set to session via `configOverlay`  in `OpenSession` on server side , when session sign enabled.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1035706099


##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/AuthZUtils.scala:
##########
@@ -175,4 +190,38 @@ private[authz] object AuthZUtils {
   def quote(parts: Seq[String]): String = {
     parts.map(quoteIfNeeded).mkString(".")
   }
+
+  private def verifyKyuubiSessionUser(spark: SparkContext, user: String): Unit = {
+    val isSessionUserVerifyEnabled =
+      spark.getConf.getBoolean(s"spark.kyuubi.session.user.sign.enabled", defaultValue = false)
+    if (isSessionUserVerifyEnabled) {
+      val isVerified = {
+        try {
+          val userPubKeyStr = spark.getLocalProperty("kyuubi.session.sign.publickey")
+          val userSign = spark.getLocalProperty("kyuubi.session.user.sign")
+          verifySignWithECDSA(user, userSign, userPubKeyStr)
+        } catch {
+          case _: Exception =>
+            false
+        }
+      }
+      if (!isVerified) {
+        throw new AccessControlException(s"Permission denied: user [$user] is not verified")
+      }
+    }
+  }
+
+  private def verifySignWithECDSA(
+      plainText: String,
+      signature: String,
+      publicKeyStr: String): Boolean = {

Review Comment:
   publicKeyBase64



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [kyuubi] bowenliang123 commented on pull request #3838: [KYUUBI #3839][Authz] Introduce signature mechanism to protect session variable on engine side

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3838:
URL: https://github.com/apache/kyuubi/pull/3838#issuecomment-1371695911

   This is also applied to pyspark support. https://github.com/apache/kyuubi/issues/3780


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3838: [KYUUBI #3839] [Authz] WIP: Support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1032055137


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiReservedKeys.scala:
##########
@@ -20,6 +20,8 @@ package org.apache.kyuubi.config
 object KyuubiReservedKeys {
   final val KYUUBI_CLIENT_IP_KEY = "kyuubi.client.ipAddress"
   final val KYUUBI_SESSION_USER_KEY = "kyuubi.session.user"
+  final val KYUUBI_SESSION_USER_SIGN = "kyuubi.session.user.sign"
+  final val KYUUBI_SESSION_USER_PUBIC_KEY = "kyuubi.session.user.public.key"

Review Comment:
   fixed.



##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/AuthZUtils.scala:
##########
@@ -74,6 +85,13 @@ private[authz] object AuthZUtils {
   def getAuthzUgi(spark: SparkContext): UserGroupInformation = {
     // kyuubi.session.user is only used by kyuubi
     val user = spark.getLocalProperty("kyuubi.session.user")
+    if (StringUtils.isNotBlank(user)) {
+      if (!verifyKyuubiSessionUser(spark, user)) {
+        throw new AccessControlException(

Review Comment:
   Good point, thx.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1036111765


##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/AuthZUtils.scala:
##########
@@ -175,4 +194,38 @@ private[authz] object AuthZUtils {
   def quote(parts: Seq[String]): String = {
     parts.map(quoteIfNeeded).mkString(".")
   }
+
+  private def verifyKyuubiSessionUser(spark: SparkContext, user: String): Unit = {
+    val isVerified = {
+      try {
+        val userPubKeyBase64 = spark.getLocalProperty(KYUUBI_SESSION_SIGN_PUBLICKEY)
+        val userSign = spark.getLocalProperty(KYUUBI_SESSION_USER_SIGN)
+        if (StringUtils.isAnyBlank(user, userPubKeyBase64, userSign)) {
+          false
+        } else {
+          verifySignWithECDSA(user, userSign, userPubKeyBase64)
+        }
+      } catch {
+        case _: Exception =>
+          false
+      }
+    }
+    if (!isVerified) {
+      throw new AccessControlException(s"Permission denied: user [$user] is not verified")

Review Comment:
   "Permission denied" is not suitable here, it's some kind of "illegal identifier" or "invalid user identifier"



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1035708551


##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecutePython.scala:
##########
@@ -74,6 +74,16 @@ class ExecutePython(
         s"($statementId, $redactedStatement, $forceCancel)")
       setSparkLocalProperties(KYUUBI_SESSION_USER_KEY, session.user)
       setSparkLocalProperties(KYUUBI_STATEMENT_ID_KEY, statementId)
+
+      if (isSessionUserVerifyEnabled) {
+        setSparkLocalProperties(
+          KYUUBI_SESSION_SIGN_PUBLICKEY,
+          session.conf.get(KYUUBI_SESSION_SIGN_PUBLICKEY).orNull)
+        setSparkLocalProperties(
+          KYUUBI_SESSION_USER_SIGN,
+          session.conf.get(KYUUBI_SESSION_USER_SIGN).orNull)

Review Comment:
   my point is should we fail fast instead of `.orNull`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] cfmcgrady commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
cfmcgrady commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1036889944


##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/SparkOperation.scala:
##########
@@ -193,6 +208,25 @@ abstract class SparkOperation(session: Session)
     // TODO: (fchen) make all operation support arrow
     getClass.getCanonicalName == classOf[ExecuteStatement].getCanonicalName
   }
+
+  protected def setSessionUserSign(): Unit = {
+    (
+      session.conf.get(KYUUBI_SESSION_SIGN_PUBLICKEY),
+      session.conf.get(KYUUBI_SESSION_USER_SIGN)) match {
+      case (Some(pubKey), Some(userSign)) =>
+        setSparkLocalProperty(KYUUBI_SESSION_SIGN_PUBLICKEY, pubKey)

Review Comment:
   yep, It should work, I misread before



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3838: [KYUUBI #3839] [Authz] WIP: Support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1031935223


##########
docs/deployment/settings.md:
##########
@@ -497,6 +497,7 @@ kyuubi.session.idle.timeout|PT6H|session idle timeout, it will be closed when it
 kyuubi.session.local.dir.allow.list||The local dir list that are allowed to access by the kyuubi session application. User might set some parameters such as `spark.files` and it will upload some local files when launching the kyuubi engine, if the local dir allow list is defined, kyuubi will check whether the path to upload is in the allow list. Note that, if it is empty, there is no limitation for that and please use absolute path list.|seq|1.6.0
 kyuubi.session.name|&lt;undefined&gt;|A human readable name of session and we use empty string by default. This name will be recorded in event. Note that, we only apply this value from session conf.|string|1.4.0
 kyuubi.session.timeout|PT6H|(deprecated)session timeout, it will be closed when it's not accessed for this duration|duration|1.0.0
+kyuubi.session.user.verify.enabled|true|Whether to verify the integrity of session user name in Spark engine within Authz plugin.|boolean|1.7.0

Review Comment:
   OK, changed the config name to `kyuubi.session.user.sign.enabled`.
   
   As for the rest configs suggested, they are not good to put in configs this time maybe.
   - algorithm , 
           - not suitable config in one option, itis related to different details including the KeyPair generation algorithm (like "EC"), and the signing algorithm (like "SHA256withECDSA")
           - used by both the server and engine side plugin Authz
   - public.key / private.key
           - set to spark conf , may be show on Spark UI, even being redacted
           - if set to spark conf via `SparkProcBuilder`, they will be easily leaking to using scripts for regenearting fake  session user which passing the use sign verifying in Authz.
    ```
   """
   privatekey = spark.context.getconf.get("privatekey")
   
   newsign = sign(fakeuser, privatekey)
   spark.context.setLocalProp(fakeuser)
   
   # invalid access with fake session user
   df = spark.sql("select * from confidential_table")
    ```



##########
docs/deployment/settings.md:
##########
@@ -497,6 +497,7 @@ kyuubi.session.idle.timeout|PT6H|session idle timeout, it will be closed when it
 kyuubi.session.local.dir.allow.list||The local dir list that are allowed to access by the kyuubi session application. User might set some parameters such as `spark.files` and it will upload some local files when launching the kyuubi engine, if the local dir allow list is defined, kyuubi will check whether the path to upload is in the allow list. Note that, if it is empty, there is no limitation for that and please use absolute path list.|seq|1.6.0
 kyuubi.session.name|&lt;undefined&gt;|A human readable name of session and we use empty string by default. This name will be recorded in event. Note that, we only apply this value from session conf.|string|1.4.0
 kyuubi.session.timeout|PT6H|(deprecated)session timeout, it will be closed when it's not accessed for this duration|duration|1.0.0
+kyuubi.session.user.verify.enabled|true|Whether to verify the integrity of session user name in Spark engine within Authz plugin.|boolean|1.7.0

Review Comment:
   OK, changed the config name to `kyuubi.session.user.sign.enabled`.
   
   As for the rest configs suggested, they are not good to put in configs this time maybe.
   - algorithm , 
           - not suitable config in one option, itis related to different details including the KeyPair generation algorithm (like "EC"), and the signing algorithm (like "SHA256withECDSA")
           - used by both the server and engine side plugin Authz
   - public.key / private.key
           - set to spark conf , may be show on Spark UI, even being redacted
           - if set to spark conf via `SparkProcBuilder`, they will be easily leaking to using scripts for regenearting fake  session user which passing the use sign verifying in Authz.
    ```
   privatekey = spark.context.getconf.get("privatekey")
   
   newsign = sign(fakeuser, privatekey)
   spark.context.setLocalProp(fakeuser)
   
   # invalid access with fake session user
   df = spark.sql("select * from confidential_table")
    ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1036119615


##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/SparkOperation.scala:
##########
@@ -73,11 +74,25 @@ abstract class SparkOperation(session: Session)
     spark.conf.getOption(KyuubiConf.OPERATION_SCHEDULER_POOL.key).orElse(
       session.sessionManager.getConf.get(KyuubiConf.OPERATION_SCHEDULER_POOL))
 
+  protected val isSessionUserSignEnabled: Boolean = spark.sparkContext.getConf.getBoolean(
+    s"spark.${SESSION_USER_SIGN_ENABLED.key}",
+    SESSION_USER_SIGN_ENABLED.defaultVal.get)
+
   protected def withLocalProperties[T](f: => T): T = {
     try {
       spark.sparkContext.setJobGroup(statementId, redactedStatement, forceCancel)
       spark.sparkContext.setLocalProperty(KYUUBI_SESSION_USER_KEY, session.user)
       spark.sparkContext.setLocalProperty(KYUUBI_STATEMENT_ID_KEY, statementId)
+
+      if (isSessionUserSignEnabled) {
+        spark.sparkContext.setLocalProperty(
+          KYUUBI_SESSION_SIGN_PUBLICKEY,
+          session.conf(KYUUBI_SESSION_SIGN_PUBLICKEY))
+        spark.sparkContext.setLocalProperty(
+          KYUUBI_SESSION_USER_SIGN,
+          session.conf(KYUUBI_SESSION_USER_SIGN))
+      }

Review Comment:
   ```suggestion
         spark.sparkContext.setLocalProperty(
           KYUUBI_SESSION_SIGN_PUBLICKEY,
           session.conf(KYUUBI_SESSION_SIGN_PUBLICKEY).orNull)
         spark.sparkContext.setLocalProperty(
           KYUUBI_SESSION_USER_SIGN,
           session.conf(KYUUBI_SESSION_USER_SIGN).orNull)
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3838: [KYUUBI #3839] [Authz] WIP: Support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#issuecomment-1329369052

   With the session user `bowen2`, querying with pyspark.
   In Authz plugin on engine side, the session user `bowen2` are verified with public key generated on server side and checked for table privileges with session user.
   
   - session user verified, no access privileges to the table:
   <img width="1093" alt="image" src="https://user-images.githubusercontent.com/1935105/204326485-f2c05917-d5d6-416e-be79-eae0540ae800.png">
   
   - session user disguised as `superadmin` directly in SparkContext in user code (multi-line scripts for single execution), session user `superadmin`  is not verified:
   <img width="1073" alt="image" src="https://user-images.githubusercontent.com/1935105/204327264-89dc3d87-e19b-467f-9036-a5ebd06debae.png">
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3838: [KYUUBI #3839] [Authz] Support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1032913326


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignUtils.scala:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kyuubi.util
+
+import java.nio.charset.StandardCharsets
+import java.security.{KeyPair, KeyPairGenerator, PrivateKey, SecureRandom, Signature}
+import java.security.spec.ECGenParameterSpec
+import java.util.Base64
+
+object SignUtils {
+
+  private lazy val ecKeyPairGenerator = {
+    val g = KeyPairGenerator.getInstance("EC")
+    g.initialize(new ECGenParameterSpec("secp256k1"), new SecureRandom())
+    g
+  }
+
+  def generateKeyPair(algorithm: String = "EC"): KeyPair = {

Review Comment:
   It's fine to return `(PublicKey, PrivateKey)`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3838: [KYUUBI #3839] [Authz] WIP: Support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#issuecomment-1326922447

   > Could you add some ut？ Shall we redact the key for en/decryption？
   
   OK, let me mark this as WIP. Will add some ut soon.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1035706972


##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/AuthZUtils.scala:
##########
@@ -175,4 +190,38 @@ private[authz] object AuthZUtils {
   def quote(parts: Seq[String]): String = {
     parts.map(quoteIfNeeded).mkString(".")
   }
+
+  private def verifyKyuubiSessionUser(spark: SparkContext, user: String): Unit = {
+    val isSessionUserVerifyEnabled =
+      spark.getConf.getBoolean(s"spark.kyuubi.session.user.sign.enabled", defaultValue = false)
+    if (isSessionUserVerifyEnabled) {
+      val isVerified = {
+        try {
+          val userPubKeyStr = spark.getLocalProperty("kyuubi.session.sign.publickey")
+          val userSign = spark.getLocalProperty("kyuubi.session.user.sign")
+          verifySignWithECDSA(user, userSign, userPubKeyStr)

Review Comment:
   `userSign` and `userPubKeyStr` may be null



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3838: [KYUUBI #3839][Authz] Introduce signature mechanism to protect session variable on engine side

Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#issuecomment-1334799424

   Thanks @bowenliang123 great work! Merged to master


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #3838: [KYUUBI #3839] [Authz] support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#issuecomment-1324723149

   # [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3838](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5272e5d) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/6bf4e492bf00e1a68809e4ff3f1206870df42bad?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6bf4e49) will **decrease** coverage by `0.05%`.
   > The diff coverage is `52.63%`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3838      +/-   ##
   ============================================
   - Coverage     52.97%   52.92%   -0.06%     
     Complexity       13       13              
   ============================================
     Files           497      498       +1     
     Lines         28022    28061      +39     
     Branches       3863     3869       +6     
   ============================================
   + Hits          14845    14851       +6     
   - Misses        11781    11805      +24     
   - Partials       1396     1405       +9     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...he/kyuubi/plugin/spark/authz/util/AuthZUtils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3V0aWwvQXV0aFpVdGlscy5zY2FsYQ==) | `42.64% <0.00%> (-12.07%)` | :arrow_down: |
   | [.../org/apache/kyuubi/config/KyuubiReservedKeys.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpUmVzZXJ2ZWRLZXlzLnNjYWxh) | `0.00% <ø> (ø)` | |
   | [.../kyuubi/engine/spark/operation/ExecutePython.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vRXhlY3V0ZVB5dGhvbi5zY2FsYQ==) | `89.86% <80.00%> (-0.35%)` | :arrow_down: |
   | [...kyuubi/engine/spark/operation/SparkOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vU3BhcmtPcGVyYXRpb24uc2NhbGE=) | `82.55% <80.00%> (-0.16%)` | :arrow_down: |
   | [.../main/scala/org/apache/kyuubi/util/SignUtils.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS91dGlsL1NpZ25VdGlscy5zY2FsYQ==) | `92.30% <92.30%> (ø)` | |
   | [.../kyuubi/server/mysql/constant/MySQLErrorCode.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbXlzcWwvY29uc3RhbnQvTXlTUUxFcnJvckNvZGUuc2NhbGE=) | `13.84% <0.00%> (-6.16%)` | :arrow_down: |
   | [...uubi/engine/spark/events/SparkOperationEvent.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9ldmVudHMvU3BhcmtPcGVyYXRpb25FdmVudC5zY2FsYQ==) | `88.88% <0.00%> (-5.56%)` | :arrow_down: |
   | [...ache/kyuubi/server/mysql/MySQLCommandHandler.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbXlzcWwvTXlTUUxDb21tYW5kSGFuZGxlci5zY2FsYQ==) | `76.84% <0.00%> (-4.22%)` | :arrow_down: |
   | [...ache/kyuubi/server/mysql/MySQLGenericPackets.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbXlzcWwvTXlTUUxHZW5lcmljUGFja2V0cy5zY2FsYQ==) | `76.59% <0.00%> (-2.13%)` | :arrow_down: |
   | [...apache/kyuubi/session/KyuubiBatchSessionImpl.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0t5dXViaUJhdGNoU2Vzc2lvbkltcGwuc2NhbGE=) | `90.66% <0.00%> (-1.23%)` | :arrow_down: |
   | ... and [6 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/3838/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3838: [KYUUBI #3839] [Authz] support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#issuecomment-1324555451

   Linked to issue #3839 .


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on pull request #3838: [KYUUBI #3839] [Authz] Support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#issuecomment-1328225327

   Overall this LGTM, could you please add test cases on server side? I think it should include
   - positive case
   - sign enabled can not disable by session configuration
   - unmatched user and sign
   - sign enabled but no sign in configuration


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3838: [KYUUBI #3839] [Authz] WIP: Support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1033737563


##########
kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignUtils.scala:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kyuubi.util
+
+import java.nio.charset.StandardCharsets
+import java.security.{KeyPair, KeyPairGenerator, PrivateKey, SecureRandom, Signature}
+import java.security.spec.ECGenParameterSpec
+import java.util.Base64
+
+object SignUtils {
+
+  private lazy val ecKeyPairGenerator = {
+    val g = KeyPairGenerator.getInstance("EC")
+    g.initialize(new ECGenParameterSpec("secp256k1"), new SecureRandom())
+    g
+  }
+
+  def generateKeyPair(algorithm: String = "EC"): KeyPair = {

Review Comment:
   OK, it's better.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1035924079


##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/AuthZUtils.scala:
##########
@@ -175,4 +190,38 @@ private[authz] object AuthZUtils {
   def quote(parts: Seq[String]): String = {
     parts.map(quoteIfNeeded).mkString(".")
   }
+
+  private def verifyKyuubiSessionUser(spark: SparkContext, user: String): Unit = {
+    val isSessionUserVerifyEnabled =
+      spark.getConf.getBoolean(s"spark.kyuubi.session.user.sign.enabled", defaultValue = false)
+    if (isSessionUserVerifyEnabled) {
+      val isVerified = {
+        try {
+          val userPubKeyStr = spark.getLocalProperty("kyuubi.session.sign.publickey")
+          val userSign = spark.getLocalProperty("kyuubi.session.user.sign")
+          verifySignWithECDSA(user, userSign, userPubKeyStr)

Review Comment:
   Yes, let's check them not null.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 closed pull request #3838: [KYUUBI #3839][Authz] Introduce signature mechanism to protect session variable on engine side

Posted by GitBox <gi...@apache.org>.
pan3793 closed pull request #3838: [KYUUBI #3839][Authz] Introduce signature mechanism to protect session variable on engine side
URL: https://github.com/apache/incubator-kyuubi/pull/3838


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] cfmcgrady commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
cfmcgrady commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1036834694


##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/SparkOperation.scala:
##########
@@ -193,6 +208,25 @@ abstract class SparkOperation(session: Session)
     // TODO: (fchen) make all operation support arrow
     getClass.getCanonicalName == classOf[ExecuteStatement].getCanonicalName
   }
+
+  protected def setSessionUserSign(): Unit = {
+    (
+      session.conf.get(KYUUBI_SESSION_SIGN_PUBLICKEY),
+      session.conf.get(KYUUBI_SESSION_USER_SIGN)) match {
+      case (Some(pubKey), Some(userSign)) =>
+        setSparkLocalProperty(KYUUBI_SESSION_SIGN_PUBLICKEY, pubKey)

Review Comment:
   >  Server -> Engine: Server generates session signing on session user when opening Kyuubi Session to Engine
   
   Since each session user has its own `KYUUBI_SESSION_USER_SIGN`, but all the user sessions within one application share one `SparkContext`, this means the verification mechanism only works for CONNECTION/USER mode?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3838: [KYUUBI #3839][Authz] Introduce signature mechanism to protect session variable on engine side

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#issuecomment-1335275838

   > Thanks @bowenliang123 great work! Merged to master
   
   Many thanks for the support, help, and discussion from all the reviewers!
   Hope this PR will bring general security improvement in current and future expected scenarios, especially for dynamic scripting like Scala, Python and etc, which is greatly in demand for dynamic flexibility while privileges are guarded at the front line. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] turboFei commented on pull request #3838: [KYUUBI #3839] [Authz] support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
turboFei commented on PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#issuecomment-1326894410

   Could you add some ut？
   Shall we redact the key for en/decryption？


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1036120286


##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/SparkOperation.scala:
##########
@@ -89,6 +104,10 @@ abstract class SparkOperation(session: Session)
       spark.sparkContext.setLocalProperty(SPARK_SCHEDULER_POOL_KEY, null)
       spark.sparkContext.setLocalProperty(KYUUBI_SESSION_USER_KEY, null)
       spark.sparkContext.setLocalProperty(KYUUBI_STATEMENT_ID_KEY, null)
+      if (isSessionUserSignEnabled) {
+        spark.sparkContext.setLocalProperty(KYUUBI_SESSION_SIGN_PUBLICKEY, null)
+        spark.sparkContext.setLocalProperty(KYUUBI_SESSION_USER_SIGN, null)
+      }

Review Comment:
   ```suggestion
         spark.sparkContext.setLocalProperty(KYUUBI_SESSION_SIGN_PUBLICKEY, null)
         spark.sparkContext.setLocalProperty(KYUUBI_SESSION_USER_SIGN, null)
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3838: [KYUUBI #3839] [Authz] support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#issuecomment-1324564107

   cc @yaooqinn @turboFei @pan3793 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on pull request #3838: [KYUUBI #3839] [Authz] support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#issuecomment-1324653130

   also cc @cfmcgrady 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3838: [KYUUBI #3839] [Authz] Support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1032912458


##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecutePython.scala:
##########
@@ -74,6 +74,16 @@ class ExecutePython(
         s"($statementId, $redactedStatement, $forceCancel)")
       setSparkLocalProperties(KYUUBI_SESSION_USER_KEY, session.user)
       setSparkLocalProperties(KYUUBI_STATEMENT_ID_KEY, statementId)
+
+      if (isSessionUserVerifyEnabled) {
+        setSparkLocalProperties(
+          KYUUBI_SESSION_SIGN_PUBLICKEY,
+          session.conf.get(KYUUBI_SESSION_SIGN_PUBLICKEY).orNull)
+        setSparkLocalProperties(
+          KYUUBI_SESSION_USER_SIGN,
+          session.conf.get(KYUUBI_SESSION_USER_SIGN).orNull)

Review Comment:
   If `isSessionUserVerifyEnabled`, `KYUUBI_SESSION_SIGN_PUBLICKEY` `KYUUBI_SESSION_USER_SIGN` must be present?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] bowenliang123 commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1036881380


##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/SparkOperation.scala:
##########
@@ -193,6 +208,25 @@ abstract class SparkOperation(session: Session)
     // TODO: (fchen) make all operation support arrow
     getClass.getCanonicalName == classOf[ExecuteStatement].getCanonicalName
   }
+
+  protected def setSessionUserSign(): Unit = {
+    (
+      session.conf.get(KYUUBI_SESSION_SIGN_PUBLICKEY),
+      session.conf.get(KYUUBI_SESSION_USER_SIGN)) match {
+      case (Some(pubKey), Some(userSign)) =>
+        setSparkLocalProperty(KYUUBI_SESSION_SIGN_PUBLICKEY, pubKey)

Review Comment:
   It should work for SERVER share level as well.
   1. setting properties to SparkContext is to put value in a ThreadLocal prop holders, not sharing with other threads.
   2. `runInternal` is running  the code in synchronized, which means  `setSessionUserSign` and `claeaSessionUserSign` properly set/clear the properties in the same thread  right before/after the code excuted



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3838: [KYUUBI #3839] [Feature] [Authz] Support session signing keypair and verifying session user on engine side

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1035704448


##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/AuthZUtils.scala:
##########
@@ -74,6 +85,10 @@ private[authz] object AuthZUtils {
   def getAuthzUgi(spark: SparkContext): UserGroupInformation = {
     // kyuubi.session.user is only used by kyuubi
     val user = spark.getLocalProperty("kyuubi.session.user")
+    if (StringUtils.isNotBlank(user)) {

Review Comment:
   if `kyuubi.session.user.sign.enabled` is true and `kyuubi.session.user` is absent, we should fail w/ `AccessControlException`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a diff in pull request #3838: [KYUUBI #3839] [Authz] support signing and verifying `kyuubi.session.user`

Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #3838:
URL: https://github.com/apache/incubator-kyuubi/pull/3838#discussion_r1031774248


##########
docs/deployment/settings.md:
##########
@@ -497,6 +497,7 @@ kyuubi.session.idle.timeout|PT6H|session idle timeout, it will be closed when it
 kyuubi.session.local.dir.allow.list||The local dir list that are allowed to access by the kyuubi session application. User might set some parameters such as `spark.files` and it will upload some local files when launching the kyuubi engine, if the local dir allow list is defined, kyuubi will check whether the path to upload is in the allow list. Note that, if it is empty, there is no limitation for that and please use absolute path list.|seq|1.6.0
 kyuubi.session.name|&lt;undefined&gt;|A human readable name of session and we use empty string by default. This name will be recorded in event. Note that, we only apply this value from session conf.|string|1.4.0
 kyuubi.session.timeout|PT6H|(deprecated)session timeout, it will be closed when it's not accessed for this duration|duration|1.0.0
+kyuubi.session.user.verify.enabled|true|Whether to verify the integrity of session user name in Spark engine within Authz plugin.|boolean|1.7.0

Review Comment:
   How about
   ```
   kyuubi.session.user.sign.enabled
   // it could be used for other cases in the future
   kyuubi.session.sign.algorithm
   kyuubi.session.sign.public.key
   kyuubi.session.sign.private.key
   ...
   ```
   ?



##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/SparkOperation.scala:
##########
@@ -73,11 +76,24 @@ abstract class SparkOperation(session: Session)
     spark.conf.getOption(KyuubiConf.OPERATION_SCHEDULER_POOL.key).orElse(
       session.sessionManager.getConf.get(KyuubiConf.OPERATION_SCHEDULER_POOL))
 
+  protected val isSessionUserVerifyEnabled: Boolean = spark.sparkContext.getConf.getBoolean(
+    s"spark.${SESSION_USER_VERIFY_ENABLED.key}",
+    SESSION_USER_VERIFY_ENABLED.defaultVal.get)
+
   protected def withLocalProperties[T](f: => T): T = {
     try {
       spark.sparkContext.setJobGroup(statementId, redactedStatement, forceCancel)
       spark.sparkContext.setLocalProperty(KYUUBI_SESSION_USER_KEY, session.user)
       spark.sparkContext.setLocalProperty(KYUUBI_STATEMENT_ID_KEY, statementId)
+
+      if (isSessionUserVerifyEnabled) {
+        val (publicKey, privateKey) = SignUtils.generateKeyPair
+        val signed = SignUtils.signWithECDSA(session.user, privateKey)
+        val publicKeyStr = Base64.getEncoder.encodeToString(publicKey.getEncoded)
+        spark.sparkContext.setLocalProperty(KYUUBI_SESSION_USER_PUBIC_KEY, publicKeyStr)
+        spark.sparkContext.setLocalProperty(KYUUBI_SESSION_USER_SIGN, signed)

Review Comment:
   seems you forget to clear the local properties



##########
kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiReservedKeys.scala:
##########
@@ -20,6 +20,8 @@ package org.apache.kyuubi.config
 object KyuubiReservedKeys {
   final val KYUUBI_CLIENT_IP_KEY = "kyuubi.client.ipAddress"
   final val KYUUBI_SESSION_USER_KEY = "kyuubi.session.user"
+  final val KYUUBI_SESSION_USER_SIGN = "kyuubi.session.user.sign"
+  final val KYUUBI_SESSION_USER_PUBIC_KEY = "kyuubi.session.user.public.key"

Review Comment:
   typo: PUBIC



##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecutePython.scala:
##########
@@ -70,10 +72,20 @@ class ExecutePython(
       worker.runCode(s"spark.sparkContext.setLocalProperty('$key', '$value')")
     }
     try {
+

Review Comment:
   nit: unnecessary new line



##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/util/AuthZUtils.scala:
##########
@@ -74,6 +85,13 @@ private[authz] object AuthZUtils {
   def getAuthzUgi(spark: SparkContext): UserGroupInformation = {
     // kyuubi.session.user is only used by kyuubi
     val user = spark.getLocalProperty("kyuubi.session.user")
+    if (StringUtils.isNotBlank(user)) {
+      if (!verifyKyuubiSessionUser(spark, user)) {
+        throw new AccessControlException(

Review Comment:
   we can move the `throw new AccessControlException` into `verifyKyuubiSessionUser` to make the caller side clearer.



##########
docs/deployment/settings.md:
##########
@@ -497,6 +497,7 @@ kyuubi.session.idle.timeout|PT6H|session idle timeout, it will be closed when it
 kyuubi.session.local.dir.allow.list||The local dir list that are allowed to access by the kyuubi session application. User might set some parameters such as `spark.files` and it will upload some local files when launching the kyuubi engine, if the local dir allow list is defined, kyuubi will check whether the path to upload is in the allow list. Note that, if it is empty, there is no limitation for that and please use absolute path list.|seq|1.6.0
 kyuubi.session.name|&lt;undefined&gt;|A human readable name of session and we use empty string by default. This name will be recorded in event. Note that, we only apply this value from session conf.|string|1.4.0
 kyuubi.session.timeout|PT6H|(deprecated)session timeout, it will be closed when it's not accessed for this duration|duration|1.0.0
+kyuubi.session.user.verify.enabled|true|Whether to verify the integrity of session user name in Spark engine within Authz plugin.|boolean|1.7.0

Review Comment:
   And it's not limited to authz mode, please make the description more generic, the authz use case could be referred to as an "example" 



##########
kyuubi-common/src/main/scala/org/apache/kyuubi/util/SignUtils.scala:
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kyuubi.util
+
+import java.nio.charset.StandardCharsets
+import java.security.{KeyPairGenerator, PrivateKey, PublicKey, SecureRandom, Signature}
+import java.security.spec.ECGenParameterSpec
+import java.util.Base64
+
+object SignUtils {
+
+  private lazy val ecKeyPairGenerator = {
+    val g = KeyPairGenerator.getInstance("EC")
+    g.initialize(new ECGenParameterSpec("secp256k1"), new SecureRandom())

Review Comment:
   tip, using `Random` w/ a random seed generates indeterminate results, it's secure and should be used for production, but makes the test not repeatable, better to allow setting the seed in UT.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org