Skip to content

[SPARK-26349][PYSPARK] Forbid insecure py4j gateways - #23441

Closed
squito wants to merge 7 commits into
apache:masterfrom
squito:SPARK-26349
Closed

[SPARK-26349][PYSPARK] Forbid insecure py4j gateways#23441
squito wants to merge 7 commits into
apache:masterfrom
squito:SPARK-26349

Conversation

@squito

@squitosquito commented Jan 3, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Spark always creates secure py4j connections between java and python,
but it also allows users to pass in their own connection. This ensures
that even passed in connections are secure.

Added test cases verifying the failure with a (mocked) insecure gateway.

This is closely related to SPARK-26019, but this entirely forbids the
insecure connection, rather than creating the "escape-hatch".

@squito
[SPARK-26349][PYSPARK] Forbid insecure py4j gateways
8a74acf
Spark always creates secure py4j connections between java and python,
but it also allows users to pass in their own connection. This ensures
that even passed in connections are secure.
Added test cases verifying the failure without the extra configuration.
For the tests, I added ways to create insecure gateways, but I tried to put in protections to make sure that wouldn't get used incorrectly.
This is closely related to SPARK-26019, but this entirely forbids the
insecure connection, rather than creating the "escape-hatch".
@squito

squito commented Jan 3, 2019

Copy link
Copy Markdown
ContributorAuthor

@HyukjinKwon@BryanCutler@holdenk@tgravescs@vanzin since you all looked at the related #23337, this is the same just without any way to allow the insecure connection

@squito

squito commented Jan 3, 2019

Copy link
Copy Markdown
ContributorAuthor

ps thanks @HyukjinKwon for the test refactoring, so much nicer & faster to run my pyspark tests locally now on the master branch :)

@SparkQA

SparkQA commented Jan 3, 2019

Copy link
Copy Markdown

Test build #100710 has finished for PR 23441 at commit 8a74acf.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

vanzin
vanzin reviewed Jan 3, 2019
View reviewed changes
Comment threadcore/src/main/scala/org/apache/spark/api/python/PythonGatewayServer.scala Outdated
if (sys.env.getOrElse("_PYSPARK_CREATE_INSECURE_GATEWAY", "0") != "1") {
builder.authToken(secret)
} else {
assert(sys.env.getOrElse("SPARK_TESTING", "0") == "1",

@vanzinvanzinJan 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Utils.isTesting?

Comment threadpython/pyspark/tests/test_context.py Outdated
def test_forbid_insecure_gateway(self):
# Fail immediately if you try to create a SparkContext
# with an insecure gateway
gateway = _launch_gateway(insecure=True)

@vanzinvanzinJan 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems to be just for the check you're adding in the SparkContext python class, right?

So instead of this insecure flag, couldn't you just instantiate a dummy insecure gateway? Then the Java and Python code could be a little simpler and avoid the test-only checks.

@squitosquitoJan 4, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh great point, that simplified this code a ton.

@SparkQA

SparkQA commented Jan 4, 2019

Copy link
Copy Markdown

Test build #100711 has finished for PR 23441 at commit ee6343c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Jan 4, 2019

Copy link
Copy Markdown

Test build #100717 has finished for PR 23441 at commit 2d40cba.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Jan 4, 2019

Copy link
Copy Markdown

Test build #100719 has finished for PR 23441 at commit 3d925ac.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Jan 4, 2019

Copy link
Copy Markdown

Test build #100720 has finished for PR 23441 at commit 4edc4b4.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

HyukjinKwon
HyukjinKwon reviewed Jan 4, 2019
View reviewed changes
Comment threadpython/pyspark/tests/test_context.py Outdated
@HyukjinKwon

HyukjinKwon commented Jan 4, 2019

Copy link
Copy Markdown
Member

Looks good otherwise.

@SparkQA

SparkQA commented Jan 4, 2019

Copy link
Copy Markdown

Test build #100738 has finished for PR 23441 at commit 3ce8a98.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

HyukjinKwon commented Jan 7, 2019

Copy link
Copy Markdown
Member

retest this please

HyukjinKwon
HyukjinKwon reviewed Jan 7, 2019
View reviewed changes
Comment threadpython/pyspark/tests/test_context.py Outdated
# with an insecure gateway
parameters = namedtuple('MockGatewayParameters', 'auth_token')(None)
mock_insecure_gateway = namedtuple('MockJavaGateway', 'gateway_parameters')(parameters)
with self.assertRaises(Exception) as context:

@HyukjinKwonHyukjinKwonJan 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would cache ValueError

@SparkQA

SparkQA commented Jan 7, 2019

Copy link
Copy Markdown

Test build #100857 has finished for PR 23441 at commit 3ce8a98.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Jan 7, 2019

Copy link
Copy Markdown

Test build #100905 has finished for PR 23441 at commit 6e4f236.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

BryanCutler
BryanCutler approved these changes Jan 8, 2019
View reviewed changes

@BryanCutlerBryanCutler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@asfgitasfgit closed this in 32515d2Jan 8, 2019
@BryanCutler

BryanCutler commented Jan 8, 2019

Copy link
Copy Markdown
Member

merged to master, thanks @squito !

jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@squitoJackey Lee
[SPARK-26349][PYSPARK] Forbid insecure py4j gateways
e69b5b0
Spark always creates secure py4j connections between java and python,
but it also allows users to pass in their own connection. This ensures
that even passed in connections are secure.
Added test cases verifying the failure with a (mocked) insecure gateway.
This is closely related to SPARK-26019, but this entirely forbids the
insecure connection, rather than creating the "escape-hatch".
Closesapache#23441 from squito/SPARK-26349.
Authored-by: Imran Rashid <irashid@cloudera.com>
Signed-off-by: Bryan Cutler <cutlerb@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vanzinvanzinvanzin left review comments
@BryanCutlerBryanCutlerBryanCutler approved these changes
@HyukjinKwonHyukjinKwonHyukjinKwon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@squito@SparkQA@HyukjinKwon@BryanCutler@vanzin