You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "hasnain-db (via GitHub)" <gi...@apache.org> on 2023/10/04 18:20:26 UTC
[PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
hasnain-db opened a new pull request, #43220:
URL: https://github.com/apache/spark/pull/43220
### What changes were proposed in this pull request?
This change adds new settings to `TransportConf` which are needed for the RPC SSL functionality to work. Additionally, add some sample configurations which are used by tests in follow up PRs (see https://github.com/apache/spark/pull/42685 for the full context)
### Why are the changes needed?
These changes are needed so that other modules can easily access configurations, and that the sample configurations are easily accessible for tests.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Added a test, then ran:
```
./build/sbt
> project network-common
> testOnly org.apache.spark.network.TransportConfSuite
```
There are more follow up tests coming (see https://github.com/apache/spark/pull/42685)
### Was this patch authored or co-authored using generative AI tooling?
No
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43220:
URL: https://github.com/apache/spark/pull/43220#discussion_r1346794861
##########
common/network-common/src/main/java/org/apache/spark/network/util/TransportConf.java:
##########
@@ -257,6 +258,159 @@ public int sslShuffleChunkSize() {
conf.get("spark.network.ssl.maxEncryptedBlockSize", "64k")));
}
+ /**
+ * Whether Secure (SSL/TLS) RPC (including Block Transfer Service) is enabled
+ */
+ public boolean sslRpcEnabled() {
+ return conf.getBoolean("spark.ssl.rpc.enabled", false);
+ }
Review Comment:
Typically, there are two cases -
a) Use of signed certificates, which can be validated,
b) Use of self-signed certificates.
Given the configs documented here, how are we planning to support these ?
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43220:
URL: https://github.com/apache/spark/pull/43220#issuecomment-1747416093
I am waiting on a few more CI retries to get a clear CI signal, but this should be ready to review (had to rebase to fix python lint issues but otherwise the original build was mostly good)
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43220:
URL: https://github.com/apache/spark/pull/43220#discussion_r1347665837
##########
common/network-common/src/main/java/org/apache/spark/network/util/TransportConf.java:
##########
@@ -257,6 +258,159 @@ public int sslShuffleChunkSize() {
conf.get("spark.network.ssl.maxEncryptedBlockSize", "64k")));
}
+ /**
+ * Whether Secure (SSL/TLS) RPC (including Block Transfer Service) is enabled
+ */
+ public boolean sslRpcEnabled() {
+ return conf.getBoolean("spark.ssl.rpc.enabled", false);
+ }
Review Comment:
updated, PTAL
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43220:
URL: https://github.com/apache/spark/pull/43220#discussion_r1347664722
##########
common/network-common/src/main/java/org/apache/spark/network/util/TransportConf.java:
##########
@@ -257,6 +258,159 @@ public int sslShuffleChunkSize() {
conf.get("spark.network.ssl.maxEncryptedBlockSize", "64k")));
}
+ /**
+ * Whether Secure (SSL/TLS) RPC (including Block Transfer Service) is enabled
+ */
+ public boolean sslRpcEnabled() {
+ return conf.getBoolean("spark.ssl.rpc.enabled", false);
+ }
Review Comment:
You are not missing anything. Thank you for catching this. In my testing, I was using a trust store that accepts the self signed certificate and copied this check to make the branches similar. But in the JDK SSL provider case, we do not always need the trust store as you rightly pointed out, so I'm going to remove the check
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43220:
URL: https://github.com/apache/spark/pull/43220#issuecomment-1749287957
Merged to master.
Thanks for working on this @hasnain-db !
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43220:
URL: https://github.com/apache/spark/pull/43220#discussion_r1349612445
##########
common/network-common/src/test/java/org/apache/spark/network/ssl/SslSampleConfigs.java:
##########
@@ -0,0 +1,235 @@
+/*
+ * 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.
+ */
+package org.apache.spark.network.ssl;
+
+import javax.security.auth.x500.X500Principal;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.math.BigInteger;
+import java.security.GeneralSecurityException;
+import java.security.InvalidKeyException;
+import java.security.Key;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import java.security.SignatureException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateEncodingException;
+import java.security.cert.X509Certificate;
+import java.util.*;
+
+import org.bouncycastle.x509.X509V1CertificateGenerator;
+
+import org.apache.spark.network.util.ConfigProvider;
+import org.apache.spark.network.util.MapConfigProvider;
+
+
+/**
+ *
+ */
Review Comment:
Please supply the comment or remove it.
@hasnain-db You can do the job at https://github.com/apache/spark/pull/43238.
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43220:
URL: https://github.com/apache/spark/pull/43220#issuecomment-1747747602
CI looks green so this should be good to review. cc @JoshRosen @mridulm
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43220:
URL: https://github.com/apache/spark/pull/43220#discussion_r1349615245
##########
common/network-common/src/test/java/org/apache/spark/network/ssl/SslSampleConfigs.java:
##########
@@ -0,0 +1,235 @@
+/*
+ * 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.
+ */
+package org.apache.spark.network.ssl;
+
+import javax.security.auth.x500.X500Principal;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.math.BigInteger;
+import java.security.GeneralSecurityException;
+import java.security.InvalidKeyException;
+import java.security.Key;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import java.security.SignatureException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateEncodingException;
+import java.security.cert.X509Certificate;
+import java.util.*;
+
+import org.bouncycastle.x509.X509V1CertificateGenerator;
+
+import org.apache.spark.network.util.ConfigProvider;
+import org.apache.spark.network.util.MapConfigProvider;
+
+
+/**
+ *
+ */
Review Comment:
I missed this, thanks for flagging it @beliefer !
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43220:
URL: https://github.com/apache/spark/pull/43220#discussion_r1346908774
##########
common/network-common/src/main/java/org/apache/spark/network/util/TransportConf.java:
##########
@@ -257,6 +258,159 @@ public int sslShuffleChunkSize() {
conf.get("spark.network.ssl.maxEncryptedBlockSize", "64k")));
}
+ /**
+ * Whether Secure (SSL/TLS) RPC (including Block Transfer Service) is enabled
+ */
+ public boolean sslRpcEnabled() {
+ return conf.getBoolean("spark.ssl.rpc.enabled", false);
+ }
Review Comment:
Actually, looking at it again, I want to make sure I am not missing something here.
Currently, `sslRpcEnabledAndKeysAreValid` will return `false` when key store is specified, but trust store is not - and so ssl factory will be `null`.
Within ssl factory, we do have code to handle the case of trust store being null resulting in accepting all server certs - but this will never get triggered since factory is null.
What am I missing here ?
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #43220: [SPARK-45408][CORE] Add RPC SSL settings to TransportConf
URL: https://github.com/apache/spark/pull/43220
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #43220:
URL: https://github.com/apache/spark/pull/43220#discussion_r1346877805
##########
common/network-common/src/main/java/org/apache/spark/network/util/TransportConf.java:
##########
@@ -257,6 +258,159 @@ public int sslShuffleChunkSize() {
conf.get("spark.network.ssl.maxEncryptedBlockSize", "64k")));
}
+ /**
+ * Whether Secure (SSL/TLS) RPC (including Block Transfer Service) is enabled
+ */
+ public boolean sslRpcEnabled() {
+ return conf.getBoolean("spark.ssl.rpc.enabled", false);
+ }
Review Comment:
Ok, so we are relying on the fact the if truststore is not specified, it implies self signed cert as we are going to bypass cert check.
That sounds fine to me given this is different from 'typical' usage where client is accepting server's cert and does not know if it will be trusted, or should bypass trust (if cert is trusted, use that - but if it no, should it still be trusted).
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43220:
URL: https://github.com/apache/spark/pull/43220#discussion_r1346818789
##########
common/network-common/src/main/java/org/apache/spark/network/util/TransportConf.java:
##########
@@ -257,6 +258,159 @@ public int sslShuffleChunkSize() {
conf.get("spark.network.ssl.maxEncryptedBlockSize", "64k")));
}
+ /**
+ * Whether Secure (SSL/TLS) RPC (including Block Transfer Service) is enabled
+ */
+ public boolean sslRpcEnabled() {
+ return conf.getBoolean("spark.ssl.rpc.enabled", false);
+ }
Review Comment:
We support both of these.
If you like to use the JDK SSL provider, we require a `keyStore` argument which takes in the key being used for the server, and the `trustStore` is used to verify the peer we talk to.
If the openSSL provider is used, we require the `privateKey` and `certChain` arguments which have similar purposes.
You can look at the `SSLFactory` in the reference PR https://github.com/apache/spark/pull/42685 (I was going to put it up after this PR and another one) to see how it's used.
The tests in the reference PR https://github.com/apache/spark/pull/42685 use self signed certificates and work fine.
Does that answer your question? Happy to clarify as needed
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43220:
URL: https://github.com/apache/spark/pull/43220#discussion_r1354108856
##########
common/network-common/src/test/java/org/apache/spark/network/ssl/SslSampleConfigs.java:
##########
@@ -0,0 +1,235 @@
+/*
+ * 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.
+ */
+package org.apache.spark.network.ssl;
+
+import javax.security.auth.x500.X500Principal;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.math.BigInteger;
+import java.security.GeneralSecurityException;
+import java.security.InvalidKeyException;
+import java.security.Key;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import java.security.SignatureException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateEncodingException;
+import java.security.cert.X509Certificate;
+import java.util.*;
+
+import org.bouncycastle.x509.X509V1CertificateGenerator;
+
+import org.apache.spark.network.util.ConfigProvider;
+import org.apache.spark.network.util.MapConfigProvider;
+
+
+/**
+ *
+ */
Review Comment:
addressing in https://github.com/apache/spark/pull/43249
--
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
Re: [PR] [SPARK-45408][CORE] Add RPC SSL settings to TransportConf [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #43220:
URL: https://github.com/apache/spark/pull/43220#discussion_r1349629425
##########
common/network-common/src/test/java/org/apache/spark/network/ssl/SslSampleConfigs.java:
##########
@@ -0,0 +1,235 @@
+/*
+ * 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.
+ */
+package org.apache.spark.network.ssl;
+
+import javax.security.auth.x500.X500Principal;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.math.BigInteger;
+import java.security.GeneralSecurityException;
+import java.security.InvalidKeyException;
+import java.security.Key;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import java.security.SignatureException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateEncodingException;
+import java.security.cert.X509Certificate;
+import java.util.*;
+
+import org.bouncycastle.x509.X509V1CertificateGenerator;
+
+import org.apache.spark.network.util.ConfigProvider;
+import org.apache.spark.network.util.MapConfigProvider;
+
+
+/**
+ *
+ */
Review Comment:
will do, thanks for catching this!
--
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