You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "Kimahriman (via GitHub)" <gi...@apache.org> on 2025/02/19 11:54:01 UTC
[PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Kimahriman opened a new pull request, #50006:
URL: https://github.com/apache/spark/pull/50006
<!--
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://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
Adds static token authentication support to Spark Connect, which is used by default for automatically started servers locally.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
To add authentication support to Spark Connect so a connect server isn't started that could be accessible to other users inadvertently.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
The local authentication should be transparent to users, but adds the option for users manually starting connect servers to specify an authentication token.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
Existing UTs
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
No
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962571359
##########
python/pyspark/sql/connect/client/core.py:
##########
@@ -220,7 +220,9 @@ def userId(self) -> Optional[str]:
@property
def token(self) -> Optional[str]:
- return self._params.get(ChannelBuilder.PARAM_TOKEN, None)
+ return self._params.get(
+ ChannelBuilder.PARAM_TOKEN, os.environ.get("SPARK_CONNECT_AUTHENTICATE_TOKEN")
Review Comment:
We should probably don't set this if `os.environ.get("SPARK_CONNECT_AUTHENTICATE_TOKEN")` is `None` because we will not use this when this auth is off.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962294314
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/PreSharedKeyAuthenticationInterceptor.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.spark.sql.connect.service
+
+import io.grpc.{Metadata, ServerCall, ServerCallHandler, ServerInterceptor, Status}
+
+class PreSharedKeyAuthenticationInterceptor(token: String) extends ServerInterceptor {
+
+ override def interceptCall[ReqT, RespT](
+ call: ServerCall[ReqT, RespT],
+ metadata: Metadata,
+ next: ServerCallHandler[ReqT, RespT]): ServerCall.Listener[ReqT] = {
+ val authHeaderValue =
+ metadata.get(Metadata.Key.of("Authorization", Metadata.ASCII_STRING_MARSHALLER))
+
+ if (authHeaderValue == null) {
+ val status = Status.UNAUTHENTICATED.withDescription("No authentication token provided")
+ call.close(status, new Metadata())
+ new ServerCall.Listener[ReqT]() {}
+ } else if (authHeaderValue != s"Bearer $token") {
Review Comment:
Nit: you could pre-compute the expected value.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962450039
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/PreSharedKeyAuthenticationInterceptor.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.spark.sql.connect.service
+
+import io.grpc.{Metadata, ServerCall, ServerCallHandler, ServerInterceptor, Status}
+
+class PreSharedKeyAuthenticationInterceptor(token: String) extends ServerInterceptor {
+
+ override def interceptCall[ReqT, RespT](
+ call: ServerCall[ReqT, RespT],
+ metadata: Metadata,
+ next: ServerCallHandler[ReqT, RespT]): ServerCall.Listener[ReqT] = {
+ val authHeaderValue =
+ metadata.get(Metadata.Key.of("Authorization", Metadata.ASCII_STRING_MARSHALLER))
+
+ if (authHeaderValue == null) {
+ val status = Status.UNAUTHENTICATED.withDescription("No authentication token provided")
+ call.close(status, new Metadata())
+ new ServerCall.Listener[ReqT]() {}
+ } else if (authHeaderValue != s"Bearer $token") {
Review Comment:
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962533307
##########
python/pyspark/sql/connect/session.py:
##########
@@ -1041,11 +1045,17 @@ def _start_connect_server(master: str, opts: Dict[str, Any]) -> None:
init_opts.update(opts)
opts = init_opts
+ token = str(uuid.uuid4())
+
# Configurations to be overwritten
overwrite_conf = opts
overwrite_conf["spark.master"] = master
overwrite_conf["spark.local.connect"] = "1"
+ # When running a local server, always use an ephemeral port
+ overwrite_conf["spark.connect.grpc.binding.port"] = "0"
Review Comment:
Yeah I just had this in branch as I was messing with things, will revert
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962450252
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/PreSharedKeyAuthenticationInterceptor.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.spark.sql.connect.service
+
+import io.grpc.{Metadata, ServerCall, ServerCallHandler, ServerInterceptor, Status}
+
+class PreSharedKeyAuthenticationInterceptor(token: String) extends ServerInterceptor {
+
+ override def interceptCall[ReqT, RespT](
+ call: ServerCall[ReqT, RespT],
+ metadata: Metadata,
+ next: ServerCallHandler[ReqT, RespT]): ServerCall.Listener[ReqT] = {
+ val authHeaderValue =
+ metadata.get(Metadata.Key.of("Authorization", Metadata.ASCII_STRING_MARSHALLER))
Review Comment:
just put it in the class for now
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962485140
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
+ .doc("A pre-shared token that will be used to authenticate clients. This secret must be" +
+ " passed as a bearer token by for clients to connect.")
+ .version("4.0.0")
+ .internal()
+ .stringConf
+ .createOptional
+
+ val CONNECT_AUTHENTICATE_TOKEN_ENV = "SPARK_CONNECT_AUTHENTICATE_TOKEN"
+
+ def getAuthenticateToken: Option[String] = {
+ SparkEnv.get.conf.get(CONNECT_AUTHENTICATE_TOKEN).orElse {
+ sys.env.get(CONNECT_AUTHENTICATE_TOKEN_ENV)
Review Comment:
Let's avoid `sys.env`. This creates a map internally for all environment variables, and it's actually pretty slow. Let's use System.getenv
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962488891
##########
python/pyspark/sql/connect/session.py:
##########
@@ -1041,11 +1045,17 @@ def _start_connect_server(master: str, opts: Dict[str, Any]) -> None:
init_opts.update(opts)
opts = init_opts
+ token = str(uuid.uuid4())
+
# Configurations to be overwritten
overwrite_conf = opts
overwrite_conf["spark.master"] = master
overwrite_conf["spark.local.connect"] = "1"
+ # When running a local server, always use an ephemeral port
+ overwrite_conf["spark.connect.grpc.binding.port"] = "0"
+ overwrite_conf["spark.connect.authenticate.token"] = token
os.environ["SPARK_LOCAL_CONNECT"] = "1"
+ os.environ["SPARK_CONNECT_AUTHENTICATE_TOKEN"] = token
Review Comment:
You're making the same mistake as I did :-). Individual Spark tests does not actually terminate JVM but only stop SparkContext via SparkSession.stop. After that, you create the SparkContext (via SparkSession) to Spark connect server here, and it creates a new token. Then the previously set `SPARK_CONNECT_AUTHENTICATE_TOKEN` will be applied to the Spark Connect server, then you end up with a different token.
To workaround this problem, you should either create the token once (as in https://github.com/apache/spark/pull/49880), or have a variable to reset on `SparkSession.stop`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962533943
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/PreSharedKeyAuthenticationInterceptor.scala:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.spark.sql.connect.service
+
+import io.grpc.{Metadata, ServerCall, ServerCallHandler, ServerInterceptor, Status}
+
+class PreSharedKeyAuthenticationInterceptor(token: String) extends ServerInterceptor {
Review Comment:
What do you mean by abuse it? It's intentionally designed to be used for a non-local case well
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/PreSharedKeyAuthenticationInterceptor.scala:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.spark.sql.connect.service
+
+import io.grpc.{Metadata, ServerCall, ServerCallHandler, ServerInterceptor, Status}
+
+class PreSharedKeyAuthenticationInterceptor(token: String) extends ServerInterceptor {
Review Comment:
What do you mean by abuse it? It's intentionally designed to be used for a non-local case as well
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962485895
##########
python/pyspark/sql/connect/session.py:
##########
@@ -1041,11 +1045,17 @@ def _start_connect_server(master: str, opts: Dict[str, Any]) -> None:
init_opts.update(opts)
opts = init_opts
+ token = str(uuid.uuid4())
+
# Configurations to be overwritten
overwrite_conf = opts
overwrite_conf["spark.master"] = master
overwrite_conf["spark.local.connect"] = "1"
+ # When running a local server, always use an ephemeral port
+ overwrite_conf["spark.connect.grpc.binding.port"] = "0"
Review Comment:
I think we shouldn't mix up with the improvement and this auth change. Let's revert this change.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962573477
##########
python/pyspark/sql/connect/session.py:
##########
@@ -1041,11 +1045,17 @@ def _start_connect_server(master: str, opts: Dict[str, Any]) -> None:
init_opts.update(opts)
opts = init_opts
+ token = str(uuid.uuid4())
+
# Configurations to be overwritten
overwrite_conf = opts
overwrite_conf["spark.master"] = master
overwrite_conf["spark.local.connect"] = "1"
+ # When running a local server, always use an ephemeral port
+ overwrite_conf["spark.connect.grpc.binding.port"] = "0"
+ overwrite_conf["spark.connect.authenticate.token"] = token
os.environ["SPARK_LOCAL_CONNECT"] = "1"
+ os.environ["SPARK_CONNECT_AUTHENTICATE_TOKEN"] = token
Review Comment:
Ah, okie. I saw you are deleting it. One simpler way is just to generate `token` once only for this process, and don't clean it up at all. The token will be generated per process in any event, so I think it could make the things easier. `spark.connect.authenticate.token` is a static config anyway.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962440576
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
Review Comment:
Yeah I wasn't sure, just modeled after `spark.authenticate`, which itself makes sense as a verb, but then there's `spark.authenticate.secret`. Initially I had this as `spark.connect.authenticate.secret`, but then changed it to `token`. I don't have a strong preference. Could go back to `spark.connect.authenticate.secret` for consistency, or change to `spark.connect.authentication.token`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962544598
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
Review Comment:
Okay I see what you mean, I was not expect the conf vars set on the Python `SparkConf` to end up on the command line, I thought it was just passed in memory through py4j. Let me think about that.
There are a lot of use cases to consider here though that I'm trying to encompass, which is why the dual config/env var setup.
- `pyspark --remote local` / `spark-shell --remote local`: For the most part I assume this is for testing purposes. I'm not sure what the use case is for multiple users being on the same machine but they need to prevent others from connecting to their own session. This would be the case where seeing the `ps` output could be thought of as a security hole. Having the authentication at least prevents users on other servers from remotely accessing this connect server
- `pyspark --conf spark.api.mode=connect` / `spark-shell --conf spark.api.mode-connect`: I think this is effectively the same thing as the previous use case
- `spark-submit --deploy-mode client --conf spark.api.mode=connect`: Kinda similar to the previous two. multiple users on the same machine where a job is submitted from, but you don't want them to access your own sessions. I guess if multiple users can remotely start sessions this way on a dedicated server, you could see the `ps` output from the Spark driver. I don't _think_ this method would show the token on the command line, but I would need to verify
- `spark-submit --deploy-mode cluster --conf spark.api.mode=connect`: This is the case I am most worried about from a security perspective. You are launching a driver in a shared compute cluster, so anyone else on that cluster would be able to access your Spark Connect server without any authentication (the reason I brought up the security issue in the beginning). I also don't think this would show the token in the command line, but would need to verify
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
Review Comment:
Okay I see what you mean, I was not expecting the conf vars set on the Python `SparkConf` to end up on the command line, I thought it was just passed in memory through py4j. Let me think about that.
There are a lot of use cases to consider here though that I'm trying to encompass, which is why the dual config/env var setup.
- `pyspark --remote local` / `spark-shell --remote local`: For the most part I assume this is for testing purposes. I'm not sure what the use case is for multiple users being on the same machine but they need to prevent others from connecting to their own session. This would be the case where seeing the `ps` output could be thought of as a security hole. Having the authentication at least prevents users on other servers from remotely accessing this connect server
- `pyspark --conf spark.api.mode=connect` / `spark-shell --conf spark.api.mode-connect`: I think this is effectively the same thing as the previous use case
- `spark-submit --deploy-mode client --conf spark.api.mode=connect`: Kinda similar to the previous two. multiple users on the same machine where a job is submitted from, but you don't want them to access your own sessions. I guess if multiple users can remotely start sessions this way on a dedicated server, you could see the `ps` output from the Spark driver. I don't _think_ this method would show the token on the command line, but I would need to verify
- `spark-submit --deploy-mode cluster --conf spark.api.mode=connect`: This is the case I am most worried about from a security perspective. You are launching a driver in a shared compute cluster, so anyone else on that cluster would be able to access your Spark Connect server without any authentication (the reason I brought up the security issue in the beginning). I also don't think this would show the token in the command line, but would need to verify
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support in Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1963507017
##########
python/pyspark/sql/connect/session.py:
##########
@@ -1041,11 +1045,17 @@ def _start_connect_server(master: str, opts: Dict[str, Any]) -> None:
init_opts.update(opts)
opts = init_opts
+ token = str(uuid.uuid4())
+
# Configurations to be overwritten
overwrite_conf = opts
overwrite_conf["spark.master"] = master
overwrite_conf["spark.local.connect"] = "1"
+ # When running a local server, always use an ephemeral port
+ overwrite_conf["spark.connect.grpc.binding.port"] = "0"
+ overwrite_conf["spark.connect.authenticate.token"] = token
os.environ["SPARK_LOCAL_CONNECT"] = "1"
+ os.environ["SPARK_CONNECT_AUTHENTICATE_TOKEN"] = token
Review Comment:
Okay I changed things around a little bit:
- The unit tests set a static token value via the config
- When starting the local session, we check if the env var is already set and use that if it is, otherwise if the config is set we set the env var to that, otherwise generate a new token and set the env var
- Don't set the config for the auto started server, just let the env var set the token on the server
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support in Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1963504003
##########
python/pyspark/sql/connect/session.py:
##########
@@ -1041,11 +1045,17 @@ def _start_connect_server(master: str, opts: Dict[str, Any]) -> None:
init_opts.update(opts)
opts = init_opts
+ token = str(uuid.uuid4())
+
# Configurations to be overwritten
overwrite_conf = opts
overwrite_conf["spark.master"] = master
overwrite_conf["spark.local.connect"] = "1"
+ # When running a local server, always use an ephemeral port
+ overwrite_conf["spark.connect.grpc.binding.port"] = "0"
Review Comment:
Reverted
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support in Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1963516068
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
Review Comment:
Updated so that the tests specify a constant value via the config, but normal usage would only set the env var for the automatically launched server
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962569687
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
Review Comment:
Okie. I am fine with going ahead with this first, and following up.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962488891
##########
python/pyspark/sql/connect/session.py:
##########
@@ -1041,11 +1045,17 @@ def _start_connect_server(master: str, opts: Dict[str, Any]) -> None:
init_opts.update(opts)
opts = init_opts
+ token = str(uuid.uuid4())
+
# Configurations to be overwritten
overwrite_conf = opts
overwrite_conf["spark.master"] = master
overwrite_conf["spark.local.connect"] = "1"
+ # When running a local server, always use an ephemeral port
+ overwrite_conf["spark.connect.grpc.binding.port"] = "0"
+ overwrite_conf["spark.connect.authenticate.token"] = token
os.environ["SPARK_LOCAL_CONNECT"] = "1"
+ os.environ["SPARK_CONNECT_AUTHENTICATE_TOKEN"] = token
Review Comment:
You're making the same mistake as I did :-). Individual Spark tests does not actually terminate JVM but only stop SparkContext. After that, you create the SparkContext (via SparkSession) to Spark connect server here, and it creates a new token. Then the previously set `SPARK_CONNECT_AUTHENTICATE_TOKEN` will be applied to the Spark Connect server, then you end up with a different token.
To workaround this problem, you should either create the token once (as in https://github.com/apache/spark/pull/49880), or have a variable to reset on `SparkSession.stop`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962495059
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
Review Comment:
> https://github.com/apache/spark/pull/49880#discussion_r1952889051
I also still don't like it to be as a conf. Are we sure that this is not shown in `ps`? Those will be passed to Spark Submit through Py4J server launcher to start Spark Connect server.
This is different with other cases because it will be always down in `ps` command vs other configurations are set in spark.conf file in general.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962536597
##########
python/pyspark/sql/connect/session.py:
##########
@@ -1041,11 +1045,17 @@ def _start_connect_server(master: str, opts: Dict[str, Any]) -> None:
init_opts.update(opts)
opts = init_opts
+ token = str(uuid.uuid4())
+
# Configurations to be overwritten
overwrite_conf = opts
overwrite_conf["spark.master"] = master
overwrite_conf["spark.local.connect"] = "1"
+ # When running a local server, always use an ephemeral port
+ overwrite_conf["spark.connect.grpc.binding.port"] = "0"
+ overwrite_conf["spark.connect.authenticate.token"] = token
os.environ["SPARK_LOCAL_CONNECT"] = "1"
+ os.environ["SPARK_CONNECT_AUTHENTICATE_TOKEN"] = token
Review Comment:
Isn't that what `del os.environ["SPARK_CONNECT_AUTHENTICATE_TOKEN"]` is doing in `SparkSession.stop`?
I did hit issues with tests and incorrect tokens, which is why I had to manually set the token for the foreach batch worker and query listener processes that are started. The env vars we're stomping on each other. I don't think it's from the token not clearing after stopping, but due to thread-based parallelism that is sharing the same environment vars. I need to go through and remind myself where if anywhere the env vars are actually still needed with how things are setup. I think there were a few unit tests that try to create a second session to the same remote server, which I don't know if that's really even a real use case
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #50006:
URL: https://github.com/apache/spark/pull/50006#issuecomment-2676490565
Merged to master and branch-4.0.
Thanks @Kimahriman for taking this over!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #50006: [SPARK-51156][CONNECT] Static token authentication support in Spark Connect
URL: https://github.com/apache/spark/pull/50006
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962301111
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
Review Comment:
Nit: Should we use the verb `Authenticate` or the noun `Authentication`? I'd lean towards the noun, however I am an ESL speaker.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #50006:
URL: https://github.com/apache/spark/pull/50006#issuecomment-2669679061
@Kimahriman for scala you probably need to change `org.apache.spark.sql.connect.client.SparkConnectClient.Configuration.credentials`. If you don't want SSL I guess you can compose `InsecureChannelCredentials` with `CallCredentials`, but I frankly have no idea if that satisfies the requirements, and if it is even allowed by gRPC.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on PR #50006:
URL: https://github.com/apache/spark/pull/50006#issuecomment-2668435031
@HyukjinKwon
Mostly based on your initial PR, main differences being:
- Uses the config system in addition to an env var. The env var is mostly for the internal use and test cases where additional clients connect to the same automatically launched local server. The config can be used by external users creating dedicated servers as well to add authentication support
- Uses the `Authorization` header with the `token` parameter of the gRPC client by adding the ability to use a token with a non-TLS local connection
I haven't done the Scala client yet, I would need to make similar changes allowing a token over a non-TLS local connection as well. Mostly just seeing if you think this is helpful or a good slightly alternative route to go down.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962495059
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
Review Comment:
> https://github.com/apache/spark/pull/49880#discussion_r1952889051
I also still don't like it to be as a conf. Are we sure that this is not shown in `ps`? Those will be passed to Spark Submit through Py4J server launcher to start Spark Connect server.
This is different in other cases because it will be always down in `ps` command vs other configurations are set in spark.conf file in general.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962499580
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
Review Comment:
I am being difficult particularly on this because we're adding this mainly to block the access from an arbitrary user .. but it will be pretty useless if this can be just seen with simple `ps aux`..
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962318360
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
Review Comment:
We use Authenticate everywhere else. Let's be consistent :)...
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support in Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on PR #50006:
URL: https://github.com/apache/spark/pull/50006#issuecomment-2673268575
Yep working on some tests and figuring out the scala client still
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #50006:
URL: https://github.com/apache/spark/pull/50006#issuecomment-2668710962
will take a closer look tmr :-)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962490191
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/PreSharedKeyAuthenticationInterceptor.scala:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.spark.sql.connect.service
+
+import io.grpc.{Metadata, ServerCall, ServerCallHandler, ServerInterceptor, Status}
+
+class PreSharedKeyAuthenticationInterceptor(token: String) extends ServerInterceptor {
Review Comment:
I still don't like it other people can abuse this for other purposes. Can you at least write a comment or sth that we're using this only for local server?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962298751
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/StreamingForeachBatchHelper.scala:
##########
@@ -135,7 +136,10 @@ object StreamingForeachBatchHelper extends Logging {
sessionHolder: SessionHolder): (ForeachBatchFnType, AutoCloseable) = {
val port = SparkConnectService.localPort
- val connectUrl = s"sc://localhost:$port/;user_id=${sessionHolder.userId}"
+ var connectUrl = s"sc://localhost:$port/;user_id=${sessionHolder.userId}"
+ Connect.getAuthenticateToken.foreach { token =>
Review Comment:
Nit: If you like reusing code then factor this into a helper function in `sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala`.
```scala
def authenticationTokenParam: String = getAuthenticateToken.map(token => ";token=" + token).getOrElse("")
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on PR #50006:
URL: https://github.com/apache/spark/pull/50006#issuecomment-2670020464
> Would be great if we can have some tests (can refer #49880 or write your own). Otherwise, let's go with this way, looks good to me too 👍
Yeah I saw you added some tests in your branch, I'll check that out and add some as well
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support in Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on PR #50006:
URL: https://github.com/apache/spark/pull/50006#issuecomment-2675251514
> @Kimahriman for scala you probably need to change `org.apache.spark.sql.connect.client.SparkConnectClient.Configuration.credentials`. If you don't want SSL I guess you can compose `InsecureChannelCredentials` with `CallCredentials`, but I frankly have no idea if that satisfies the requirements, and if it is even allowed by gRPC.
Yep it is not allowed by gRPC. The Java library doesn't have the equivalent `LocalChannelCredentials` as the Python library: https://github.com/grpc/grpc-java/issues/9900
Was able to work around this by simply passing in the metadata through the interceptor if the host is localhost, let me know what you think.
I also had to make some updates around the assertions for tokens and SSL on the client setup, and changed it to act like the Python setup, where TLS is used by default if you have a token set, unless you're talking to localhost, and no checks are made on the options you try to use
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962293535
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/PreSharedKeyAuthenticationInterceptor.scala:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.spark.sql.connect.service
+
+import io.grpc.{Metadata, ServerCall, ServerCallHandler, ServerInterceptor, Status}
+
+class PreSharedKeyAuthenticationInterceptor(token: String) extends ServerInterceptor {
+
+ override def interceptCall[ReqT, RespT](
+ call: ServerCall[ReqT, RespT],
+ metadata: Metadata,
+ next: ServerCallHandler[ReqT, RespT]): ServerCall.Listener[ReqT] = {
+ val authHeaderValue =
+ metadata.get(Metadata.Key.of("Authorization", Metadata.ASCII_STRING_MARSHALLER))
Review Comment:
Nit: Let's not recreate the same key over and over. Either put it in class of a companion object.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "Kimahriman (via GitHub)" <gi...@apache.org>.
Kimahriman commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962546013
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -313,4 +314,21 @@ object Connect {
.internal()
.booleanConf
.createWithDefault(true)
+
+ val CONNECT_AUTHENTICATE_TOKEN =
+ buildStaticConf("spark.connect.authenticate.token")
Review Comment:
The reason the config is designed to be used for more than just the local use case is for something like
- `spark-submit --deploy-mode cluster --conf spark.connect.authenticate.token=<token> --class org.apache.spark.sql.connect.service.SparkConnectServer`: So you can launch a remote Spark session on behalf of a user and then only let that user authenticate to it
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50006:
URL: https://github.com/apache/spark/pull/50006#discussion_r1962571359
##########
python/pyspark/sql/connect/client/core.py:
##########
@@ -220,7 +220,9 @@ def userId(self) -> Optional[str]:
@property
def token(self) -> Optional[str]:
- return self._params.get(ChannelBuilder.PARAM_TOKEN, None)
+ return self._params.get(
+ ChannelBuilder.PARAM_TOKEN, os.environ.get("SPARK_CONNECT_AUTHENTICATE_TOKEN")
Review Comment:
We should probably don't set this if `os.environ.get("SPARK_CONNECT_AUTHENTICATE_TOKEN")` is `None` because we will not use this when this auth is off.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #50006:
URL: https://github.com/apache/spark/pull/50006#issuecomment-2668700184
nice!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support to Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #50006:
URL: https://github.com/apache/spark/pull/50006#issuecomment-2669971265
Would be great if we can have some tests (can refer https://github.com/apache/spark/pull/49880 or write your own). Otherwise, let's go with this way, looks good to me too 👍
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51156][CONNECT] Static token authentication support in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #50006:
URL: https://github.com/apache/spark/pull/50006#issuecomment-2675925891
There's a conflict with my PR merged. I resolved them on my own 👍
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org