-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SPARK-3883 SSL support for HttpServer and Akka #2739
Conversation
jacek-lewandowski
commented
Oct 9, 2014
- Introduced SSLOptions object
- SSLOptions is created by SecurityManager
- SSLOptions configures Akka and Jetty to use SSL
- SSLOptions uses property file which is node-local to set SSL settings
- Provided utility methods to determine the proper Akka protocol for Akka requests and to configure SSL socket factory for URL connections
- Added tests cases for AkkaUtils, FileServer, SSLOptions and SecurityManager
- Introduced SSLOptions object - SSLOptions is created by SecurityManager - SSLOptions configures Akka and Jetty to use SSL - SSLOptions uses property file which is node-local to set SSL settings - Provided utility methods to determine the proper Akka protocol for Akka requests and to configure SSL socket factory for URL connections - Added tests cases for AkkaUtils, FileServer, SSLOptions and SecurityManager
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
#1980 is a PR to add SSL to the web UI, which might benefit from SSLOptions. Do you want to comment on that PR's strategy for configuration, etc? |
I'll go through the discussion and changes in that ticket tomorrow morning, thanks |
@@ -0,0 +1,27 @@ | |||
# | |||
# Licensed to the Apache Software Foundation (ASF) under one or more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an expert at Apache licensing, but other config files in conf/
don't have a license header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll remove it
Hi @jacek-lewandowski, I like this because it's trying to support more than just the Web UI, but I think the configuration handling is sort of confusing and overengineered. Feels like a simpler approach based on "all configuration goes through SparkConf" would be much simpler to handle and maintain (both in the code and by admins deploying Spark). What do you think? |
Thanks for review @vanzin |
btw. @vanzin how to make Jenkins run the tests on this branch? |
Let me see if I can trigger tests for you - otherwise an admin will have to intervene. Also, let me think about the configuration thing some more. To be frank, I'm not really that concerned about the Master/Worker configuration, I think those are not that interesting; I'm way more interested in applications using SSL, since that's when you're passing potentially sensitive data around. |
Jenkins, test this please. |
(Seems like that only works for my own PRs, so this will probably need an admin to trigger tests for you...) |
I suppose that this is because this pr is not against the master branch |
@vanzin This PR doesn't secure data transfers anyway, because Spark uses raw communication to exchange the real data. This is intended to secure mainly control messages, JARS, application settings, like command line arguments and Spark configuration, because they may include passwords to access third party data sources from executors. |
Jenkins, this is ok to test. Jenkins, retest this please. |
QA tests have started for PR 2739 at commit
|
QA tests have finished for PR 2739 at commit
|
Test FAILed. |
There's still sensitive data that may go in control messages; e.g., IIRC broadcasts go through akka, and those may include things like Hadoop job configuration and delegation tokens. Anyway, I know there are more channels that might need securing, but it's ok to treat those separately. Having a common SSL configuration infrastructure is a good first start. |
@JoshRosen will it be retested automatically after commit ? |
QA tests have finished for PR 2739 at commit
|
Test FAILed. |
@vanzin fyi:
|
You may want to add that file to |
QA tests have started for PR 2739 at commit
|
QA tests have finished for PR 2739 at commit
|
Test FAILed. |
Do you mind re-opening this pull request against the |
Please also update the documentation. docs/security.md and the big comment header at the top of SecurityManager.scala |
@jacek-lewandowski do you have any sense of when you'll be able to do this? It would be great to get this into master soon! |
@jacek-lewandowski are you still working on this? If you don't plan to continue working on this I'd like to pick it up. Thanks! |
Back to working on this... I've rebased against branch-1.2 and then I'll rebase against master if you want. |
@jacek-lewandowski please work on top of master. We can work on backporting it to branch-1.2 if there's a strong desire for it, but new features should always be checked into master first. |
@vanzin already did that. Now I'm running tests - is there a new procedure for testing? Or just sbt clean assembly test? I can see one test failure which I can reproduce on master as well. |
I think You'll probably need to open a new PR, I don't think Github allows you to change the target branch. |
@vanzin yeah, thats right |
I still have got one test failing:
It fails on master and on my branch - do you know about it - it looks to me as a broken test case. Can you confirm? |
If you don't believe it's your fault, it will be much easier to help if you create the new PR and an admin triggers a jenkins job to test it. Then we can see whether it's a flaky test or a result of your code. |
Jenkins, test this please. |
QA tests have started for PR 2739 at commit
|
Oh yeah - this is still against 1.1. @jacek-lewandowski can you open a new PR and close this one? |
QA tests have finished for PR 2739 at commit
|
Test FAILed. |
Here is the new PR #3571 |
So - can I close this one? |
Yes please. |