You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2023/01/18 10:35:06 UTC
[GitHub] [spark] nija-at opened a new pull request, #39641: [SPARK-42106] [Pyspark] Hide parameters when re-printing remote URL in REPL
nija-at opened a new pull request, #39641:
URL: https://github.com/apache/spark/pull/39641
### What changes were proposed in this pull request?
When the `pyspark` REPL is initialized with the `--remote <url>` option,
the URL provided is logged out to stdout as part of the REPL startup.
However, it may have auth tokens specified as query parameters.
This change sanitizes the URL by removing all query parameters when
the URL is logged.
### Why are the changes needed?
Security best practices require not logging auth tokens to standard
output or any destination.
### Does this PR introduce _any_ user-facing change?
Previously -
```sh
$ ./bin/pyspark --remote "sc://foo.com/?x=y"
Python 3.10.8 (main, Nov 24 2022, 08:08:27) [Clang 14.0.6 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
Welcome to
____ __
/ __/__ ___ _____/ /__
_\ \/ _ \/ _ `/ __/ '_/
/__ / .__/\_,_/_/ /_/\_\ version 3.4.0.dev0
/_/
Using Python version 3.10.8 (main, Nov 24 2022 08:08:27)
Client connected to the Spark Connect server at sc://foo.com/?x=y
```
Now -
```sh
$ ./bin/pyspark --remote "sc://foo.com/?x=y"
Python 3.10.8 (main, Nov 24 2022, 08:08:27) [Clang 14.0.6 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
Welcome to
____ __
/ __/__ ___ _____/ /__
_\ \/ _ \/ _ `/ __/ '_/
/__ / .__/\_,_/_/ /_/\_\ version 3.4.0.dev0
/_/
Using Python version 3.10.8 (main, Nov 24 2022 08:08:27)
Client connected to the Spark Connect server at sc://foo.com/
```
### How was this patch tested?
New unit tests added.
--
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
[GitHub] [spark] srowen commented on a diff in pull request #39641: [SPARK-42106] [Pyspark] Hide parameters when printing remote URL in REPL
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #39641:
URL: https://github.com/apache/spark/pull/39641#discussion_r1073628734
##########
python/pyspark/shell.py:
##########
@@ -86,7 +88,7 @@
% (platform.python_version(), platform.python_build()[0], platform.python_build()[1])
)
if is_remote():
- print("Client connected to the Spark Connect server at %s" % (os.environ["SPARK_REMOTE"]))
+ print("Client connected to the Spark Connect server at %s" % urlparse(os.environ["SPARK_REMOTE"]).netloc)
Review Comment:
I think this is fine; I don't know if security info would be in the URL (shouldn't be), and I wonder if some deployments want that info to clarify what's being connected to (like some kind of proxy), but can't think of a definite 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
[GitHub] [spark] nija-at commented on pull request #39641: [SPARK-42106] [Pyspark] Hide parameters when printing remote URL in REPL
Posted by GitBox <gi...@apache.org>.
nija-at commented on PR #39641:
URL: https://github.com/apache/spark/pull/39641#issuecomment-1387143304
Since Spark Connect servers are always hosted at the root and they always use the scheme `sc`, I've simplified the implementation to only print the network location (host and port) to terminal.
cc @grundprinzip
--
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
[GitHub] [spark] HyukjinKwon closed pull request #39641: [SPARK-42106][PYTHON] Hide parameters when printing remote URL in REPL
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #39641: [SPARK-42106][PYTHON] Hide parameters when printing remote URL in REPL
URL: https://github.com/apache/spark/pull/39641
--
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
[GitHub] [spark] grundprinzip commented on a diff in pull request #39641: [SPARK-42106] [Pyspark] Hide parameters when printing remote URL in REPL
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on code in PR #39641:
URL: https://github.com/apache/spark/pull/39641#discussion_r1073387248
##########
python/pyspark/tests/test_url.py:
##########
@@ -0,0 +1,31 @@
+#
+# 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.
+#
+import unittest
+from pyspark.url import sanitize_url
+
+
+class UrlTests(unittest.TestCase):
+ def test_sanitize_url_valid(self):
Review Comment:
Can you have a look at `pyspark/sql/connect/client.py:ChannelBuilder` which has the parsing logic. The remote URLs are always encoded using `sc://....`. Not HTTP.
--
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
[GitHub] [spark] grundprinzip commented on a diff in pull request #39641: [SPARK-42106] [Pyspark] Hide parameters when printing remote URL in REPL
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on code in PR #39641:
URL: https://github.com/apache/spark/pull/39641#discussion_r1073389296
##########
python/pyspark/tests/test_url.py:
##########
@@ -0,0 +1,31 @@
+#
+# 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.
+#
+import unittest
+from pyspark.url import sanitize_url
+
+
+class UrlTests(unittest.TestCase):
+ def test_sanitize_url_valid(self):
Review Comment:
Basically, make sure the scheme tests are testing for `sc://` 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
[GitHub] [spark] HyukjinKwon commented on pull request #39641: [SPARK-42106][PYTHON] Hide parameters when printing remote URL in REPL
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #39641:
URL: https://github.com/apache/spark/pull/39641#issuecomment-1396289402
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: 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
[GitHub] [spark] HyukjinKwon commented on pull request #39641: [SPARK-42106] [Pyspark] Hide parameters when printing remote URL in REPL
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #39641:
URL: https://github.com/apache/spark/pull/39641#issuecomment-1386933263
I'll leave this to @grundprinzip .
--
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