Skip to content

Conversation

@turp1twin
Copy link

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

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@rustyconover
Copy link

What is the status on this PR? I tried to cherry pick into 1.6.0 but had many reset connections.

Thanks,

Rusty

@turp1twin
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants