You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by turp1twin <gi...@git.apache.org> on 2015/11/02 16:46:26 UTC
[GitHub] spark pull request: Spark-6373 Add SSL/TLS for the Netty based Blo...
GitHub user turp1twin opened a pull request:
https://github.com/apache/spark/pull/9416
Spark-6373 Add SSL/TLS for the Netty based BlockTransferService
Sorry if this pull request is premature, but I have received very little feedback, so I am going ahead and creating it. I am still open to comments/feedback and can continue to make changes if necessary. Here are some comments about my implementation...
*Configuration:*
I added a new SSLOptions member variable to SecurityManager.scala, specifically for configuring SSL for the Block Transfer Service:
{code:title=SecurityManager.scala|linenumbers=false|language=scala}
val btsSSLOptions = SSLOptions.parse(sparkConf, "spark.ssl.bts", Some(defaultSSLOptions))
{code}
I expanded the SSLOptions case class to capture additional SSL related parameters:
{code:title=SecurityManager.scala|linenumbers=false|language=scala}
private[spark] case class SSLOptions(
enabled: Boolean = false,
keyStore: Option[File] = None,
keyStorePassword: Option[String] = None,
privateKey: Option[File] = None,
keyPassword: Option[String] = None,
certChain: Option[File] = None,
trustStore: Option[File] = None,
trustStorePassword: Option[String] = None,
trustStoreReloadingEnabled: Boolean = false,
trustStoreReloadInterval: Int = 10000,
openSslEnabled: Boolean = false,
protocol: Option[String] = None,
enabledAlgorithms: Set[String] = Set.empty)
{code}
I added the ability to provide a standard java keystore and truststore, as was possible with the existing file server and akka SSL configurations available in SecurityManager.scala. When using a keystore/truststore I also added the ability to enable truststore reloading (hadoop encrypted shuffle allows for this). In addition, I added the ability to specify an X.509 certificate chain in PEM format and a PKCS#8 private key file in PEM format. If all four parameters are provided (keyStore, trustStore, privateKey, certChain) then the privateKey and certChain parameters will be used.
In TransportConf.java I added two addition configuration parameters:
{code:title=TransportConf.java|linenumbers=false|language=java}
public int sslShuffleChunkSize() {
return conf.getInt("spark.shuffle.io.ssl.chunkSize", 60 * 1024);
}
public boolean sslShuffleEnabled() {
return conf.getBoolean("spark.ssl.bts.enabled", false);
}
{code}
For the _"spark.shuffle.io.ssl.chunkSize"_ config param I set the default to the same size used in Hadoop's encrypted shuffle implementation.
*Implementation:*
For this implementation, I opted to disrupt as little code as possible, meaning I wanted to avoid any major refactoring... Basically the TransportContext class handles the SSL setup internally based on settings in the passed TransportConf. This way none of the method signatures (i.e., createServer, etc) had to change. I opted to not use the TransportClientBootstrap/TransportServerBootstrap interfaces as they were not a good fit. Basically the TransportClientBootstrap is called to late as the client Netty pipeline for SSL needs to be setup earlier in the connection process. The TransportServerBootstrap could have been used, but IMO, it would have been a bit hacky as the doBootstrap method takes an RpcHandler and returns one, which in the case of SSL bootstrapping is not needed. Also, only using the TransportServerBootstrap and not the TransportClientBootstrap would have made its usage seem inconsistent.
Anyways, these are just some initial comments about the implementation. Definitely looking for feedback... If someone has a better alternative I am all for it, just wanted to get something working with minimal invasive changes to the codebase... This is a pretty important feature for my company as we are in the healthcare space and are require HIPAA compliance (data encrypted at rest and in transit). Thanks!
Jeff
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/turp1twin/spark SPARK-6373
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/9416.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #9416
----
commit fd2980ab8cc1fc5b4626bb7a0d1e94128ca3874d
Author: turp1twin <tu...@gmail.com>
Date: 2015-10-31T20:26:14Z
Merged ssl-shuffle-latest
commit a7f915aecea4492d9a41b7310eb465cb32d7ef14
Author: turp1twin <tu...@gmail.com>
Date: 2015-11-01T20:50:09Z
Added new SSL Netty Shuffle test and SSL YarnShuffleService test, cleaned up merge issue in TransportServer.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Spark-6373 Add SSL/TLS for the Netty based Blo...
Posted by turp1twin <gi...@git.apache.org>.
Github user turp1twin commented on the pull request:
https://github.com/apache/spark/pull/9416#issuecomment-199968093
Hey Rusty,
I was keeping this PR in sync with master but was getting no feedback at all from anyone. So I just assumed it was a "dead" PR. I can take the time and get it back in sync with Master if there is an interest in getting it accepted (even if I have to make changes after some peer review). Thoughts?
Jeff
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Spark-6373 Add SSL/TLS for the Netty based Blo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9416#issuecomment-153058457
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Spark-6373 Add SSL/TLS for the Netty based Blo...
Posted by rustyconover <gi...@git.apache.org>.
Github user rustyconover commented on the pull request:
https://github.com/apache/spark/pull/9416#issuecomment-199959738
What is the status on this PR? I tried to cherry pick into 1.6.0 but had many reset connections.
Thanks,
Rusty
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Spark-6373 Add SSL/TLS for the Netty based Blo...
Posted by turp1twin <gi...@git.apache.org>.
Github user turp1twin closed the pull request at:
https://github.com/apache/spark/pull/9416
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org