[SPARK-47172][CORE] Add support for AES-GCM for RPC encryption #46515

sweisdb · 2024-05-09T20:33:40Z

What changes were proposed in this pull request?

This change adds AES-GCM as an optional AES cipher mode for RPC encryption. The current default is using AES-CTR without any authentication. That would allow someone on the network to easily modify RPC contents on the wire and impact Spark behavior. See SPARK-47172 for more details.

Why are the changes needed?

The current default is using AES-CTR without any authentication. That would allow someone on the network to easily modify RPC contents on the wire and impact Spark behavior.

Does this PR introduce any user-facing change?

Yes, it adds an additional configuration flag is reflected in the documentation.

How was this patch tested?

Existing unit tests are all ensured to pass. New unit tests are written to explicitly test GCM support and to verify that modifying ciphertext content will cause an exception and fail.

build/sbt "network-common/test:testOnly"
build/sbt "network-common/test:testOnly org.apache.spark.network.crypto.AuthIntegrationSuite"
build/sbt "network-common/test:testOnly org.apache.spark.network.crypto.AuthEngineSuite"

Was this patch authored or co-authored using generative AI tooling?

Nope.

dongjoon-hyun · 2024-05-09T21:02:44Z

cc @mridulm

docs/security.md

common/network-common/src/main/java/org/apache/spark/network/crypto/GcmTransportCipher.java

mridulm · 2024-05-14T02:10:02Z

Took a quick pass through it, sorry for the delay.

+CC @JoshRosen as well.

common/network-common/src/main/java/org/apache/spark/network/crypto/AuthEngine.java

common/network-common/src/main/java/org/apache/spark/network/crypto/CtrTransportCipher.java

mridulm

I did a quick pass, not yet looked through tests and doc.
Thanks for working on this @sweisdb !

common/network-common/src/main/java/org/apache/spark/network/crypto/GcmTransportCipher.java

mridulm

Took a pass through it.
Can you also address pending comments please ? Thanks

common/network-common/src/main/java/org/apache/spark/network/crypto/GcmTransportCipher.java

mridulm

Just a few comments, it is looking pretty close to being done.

+CC @Ngone51, @JoshRosen as well - I would really like more eyes on this given it is a backport candidate for 3.x !

common/network-common/src/main/java/org/apache/spark/network/crypto/GcmTransportCipher.java

common/network-common/src/test/java/org/apache/spark/network/crypto/AuthEngineSuite.java

… into two classes

Ngone51 · 2024-06-11T09:13:36Z

docs/security.md

+    Cipher mode to use. Defaults "AES/CTR/NoPadding" for backward compatibility, which is not authenticated. 
+    Recommended to use "AES/GCM/NoPadding", which is an authenticated encryption mode.
+  </td>
+  <td>4.0.0</td>


@mridulm Can we still add the new feature since 4.0.0-preview1 has been passed?

That was a preview release, I am sure we will have additional new features landing and existing behavior evolving before 4.0 RC gets cut :-)

Note that this specific PR will also get backported to 3.5 and 3.4 - given the security concerns it is mitigating.
So would love to hear your thoughts @Ngone51 - particularly with the intent to derisk the change.

If you need more context on why we need to backport this to 3.5/3.4, @sweisdb could provide you more context via email.

mridulm

Looks good to me, thanks for adding support for this @sweisdb !

I will keep this open for a few days in case there are additional review comments.
+CC @dongjoon-hyun as well, given the plans to backport it to 3.5 and 3.4

mridulm · 2024-06-18T05:09:04Z

@Ngone51, did you get a chance to review this ? (not sure if you synced with @sweisdb offline :-) )
Given this has to go into released branches, I want to make sure we have more eyes on it.

Ngone51 · 2024-06-18T09:53:40Z

common/network-common/src/main/java/org/apache/spark/network/crypto/GcmTransportCipher.java

+                        return transferredThisCall;
+                    }
+                }
+                boolean lastSegment = getReadableBytes() == 0;


If we want to track the segment read, shouldn't it be bytesRead == aesGcmHkdfStreaming.getPlaintextSegmentSize()?

I think that would almost be correct, except sometimes the bytesRead may be segment aligned. You may be on the last segment and bytesRead could be less than or equal to a full segment.

I would use readableBytes() being empty as the clear indication that we just read the last segment worth of data.

I think I misunderstood it. It is last the one segment rather than the previous one segment. Then it looks good to me.

Ngone51

LGTM with two minor comments.

Ngone51 · 2024-06-19T07:26:06Z

common/network-common/src/main/java/org/apache/spark/network/crypto/GcmTransportCipher.java

+                decrypterInit = true;
+                ciphertextRead += aesGcmHkdfStreaming.getHeaderLength();
+                if (expectedLength == ciphertextRead) {
+                    // If the expected length is just the header, the ciphertext is 0 length.


Just for my curiosity: does this really happen in the real world?

I can't tell you whether there will never be a 0-length message sent in Spark. We probably want to be able to support that case if it ever happens upstream rather than throw an error here, right?

Ngone51 · 2024-06-19T07:29:12Z

common/network-common/src/main/java/org/apache/spark/network/crypto/GcmTransportCipher.java

+                            nettyBufReadableBytes,
+                            ciphertextBuffer.remaining());
+                    int expectedRemaining = (int) (expectedLength - ciphertextRead);
+                    int bytesToRead = Integer.min(readableBytes, expectedRemaining);


nit: Correct me if I'm wrong. It looks like it is an invalid case if readableBytes > expectedRemaining here. So we can actually fail eariler before reading the buffer.

mridulm · 2024-06-21T07:34:31Z

@Ngone51 , can you merge it to master/branch-3.5 and branch-3.4 once you are done with the reviews ?
I might be afk, and dont want to block on this PR getting committed. Thanks !

Ngone51 · 2024-06-21T07:37:16Z

@mridulm Sure, will do.

Ngone51 · 2024-06-21T13:10:58Z

Thanks, merged to master!

There are conflicts with branch-3.5/branch-3.4. @sweisdb Could you open the separate PRs for 3.5/3.4? Thanks!

This change adds AES-GCM as an optional AES cipher mode for RPC encryption. The current default is using AES-CTR without any authentication. That would allow someone on the network to easily modify RPC contents on the wire and impact Spark behavior. See [SPARK-47172](https://issues.apache.org/jira/browse/SPARK-47172) for more details. The current default is using AES-CTR without any authentication. That would allow someone on the network to easily modify RPC contents on the wire and impact Spark behavior. Yes, it adds an additional configuration flag is reflected in the documentation. Existing unit tests are all ensured to pass. New unit tests are written to explicitly test GCM support and to verify that modifying ciphertext content will cause an exception and fail. `build/sbt "network-common/test:testOnly"` `build/sbt "network-common/test:testOnly org.apache.spark.network.crypto.AuthIntegrationSuite"` `build/sbt "network-common/test:testOnly org.apache.spark.network.crypto.AuthEngineSuite"` Nope. Closes apache#46515 from sweisdb/SPARK-47172. Authored-by: Steve Weis <[email protected]> Signed-off-by: Yi Wu <[email protected]>

sweisdb · 2024-06-21T18:33:11Z

@Ngone51 I created two backport pull requests here:
#47060
#47061

This change adds AES-GCM as an optional AES cipher mode for RPC encryption. The current default is using AES-CTR without any authentication. That would allow someone on the network to easily modify RPC contents on the wire and impact Spark behavior. See [SPARK-47172](https://issues.apache.org/jira/browse/SPARK-47172) for more details. The current default is using AES-CTR without any authentication. That would allow someone on the network to easily modify RPC contents on the wire and impact Spark behavior. Yes, it adds an additional configuration flag is reflected in the documentation. Existing unit tests are all ensured to pass. New unit tests are written to explicitly test GCM support and to verify that modifying ciphertext content will cause an exception and fail. `build/sbt "network-common/test:testOnly"` `build/sbt "network-common/test:testOnly org.apache.spark.network.crypto.AuthIntegrationSuite"` `build/sbt "network-common/test:testOnly org.apache.spark.network.crypto.AuthEngineSuite"` Nope. Closes apache#46515 from sweisdb/SPARK-47172. Authored-by: Steve Weis <[email protected]> Signed-off-by: Yi Wu <[email protected]>

LuciferYang · 2024-07-22T07:58:30Z

common/network-common/src/main/java/org/apache/spark/network/crypto/CtrTransportCipher.java

+   * This method is for testing purposes only.
+   */
+  @VisibleForTesting
+  public String getKeyId() throws GeneralSecurityException {


Based on the method comments, this is a method for testing purposes only. Why is it defined as public?

### What changes were proposed in this pull request? This change adds AES-GCM as an optional AES cipher mode for RPC encryption. The current default is using AES-CTR without any authentication. That would allow someone on the network to easily modify RPC contents on the wire and impact Spark behavior. See [SPARK-47172](https://issues.apache.org/jira/browse/SPARK-47172) for more details. ### Why are the changes needed? The current default is using AES-CTR without any authentication. That would allow someone on the network to easily modify RPC contents on the wire and impact Spark behavior. ### Does this PR introduce _any_ user-facing change? Yes, it adds an additional configuration flag is reflected in the documentation. ### How was this patch tested? Existing unit tests are all ensured to pass. New unit tests are written to explicitly test GCM support and to verify that modifying ciphertext content will cause an exception and fail. `build/sbt "network-common/test:testOnly"` `build/sbt "network-common/test:testOnly org.apache.spark.network.crypto.AuthIntegrationSuite"` `build/sbt "network-common/test:testOnly org.apache.spark.network.crypto.AuthEngineSuite"` ### Was this patch authored or co-authored using generative AI tooling? Nope. Closes apache#46515 from sweisdb/SPARK-47172. Authored-by: Steve Weis <[email protected]> Signed-off-by: Yi Wu <[email protected]>

[SPARK-47172] Add support for AES-GCM for RPC encryption

0bfe017

github-actions bot added DOCS CORE labels May 9, 2024

dongjoon-hyun changed the title ~~[SPARK-47172] Add support for AES-GCM for RPC encryption~~ [SPARK-47172][CORE] Add support for AES-GCM for RPC encryption May 9, 2024

mridulm reviewed May 14, 2024

View reviewed changes

hasnain-db reviewed May 14, 2024

View reviewed changes

common/network-common/src/main/java/org/apache/spark/network/crypto/AuthEngine.java Outdated Show resolved Hide resolved

common/network-common/src/main/java/org/apache/spark/network/crypto/CtrTransportCipher.java Show resolved Hide resolved

sweisdb added 2 commits May 14, 2024 21:59

[SPARK-47172] Updating GcmTransportCipher to use buffered streaming

b9baf22

Adding missing import

71cd10d

mridulm reviewed May 22, 2024

View reviewed changes

common/network-common/src/main/java/org/apache/spark/network/crypto/GcmTransportCipher.java Show resolved Hide resolved

[SPARK-47172] Addressing reviewer comments

5f8cd7f

sweisdb force-pushed the SPARK-47172 branch from 38ac927 to 81a3ca0 Compare June 4, 2024 04:26

[SPARK-47172] Addressing reviewer comments

81a3ca0

mridulm reviewed Jun 5, 2024

View reviewed changes

[SPARK-47172] Addressing reviewer comments

5bc503a

sweisdb force-pushed the SPARK-47172 branch from e9fd3ff to 5bc503a Compare June 7, 2024 23:22

[SPARK-47172] Moving ByteBufferWriteableChannel to its own class

4ceeee3

mridulm reviewed Jun 8, 2024

View reviewed changes

[SPARK-47172] Addressing comments & Refactoring AuthEngine test suite…

4df0367

… into two classes

Ngone51 reviewed Jun 11, 2024

View reviewed changes

mridulm approved these changes Jun 12, 2024

View reviewed changes

Ngone51 reviewed Jun 18, 2024

View reviewed changes

Ngone51 approved these changes Jun 19, 2024

View reviewed changes

Ngone51 closed this in f0563ef Jun 21, 2024

LuciferYang reviewed Jul 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-47172][CORE] Add support for AES-GCM for RPC encryption #46515

[SPARK-47172][CORE] Add support for AES-GCM for RPC encryption #46515

sweisdb commented May 9, 2024

dongjoon-hyun commented May 9, 2024

mridulm commented May 14, 2024

mridulm left a comment •

edited

Loading

mridulm left a comment

mridulm left a comment

Ngone51 Jun 11, 2024 •

edited

Loading

mridulm Jun 12, 2024

mridulm left a comment

mridulm commented Jun 18, 2024

Ngone51 Jun 18, 2024

sweisdb Jun 18, 2024

Ngone51 Jun 19, 2024 •

edited

Loading

Ngone51 left a comment

Ngone51 Jun 19, 2024

sweisdb Jun 20, 2024

Ngone51 Jun 19, 2024

mridulm commented Jun 21, 2024

Ngone51 commented Jun 21, 2024

Ngone51 commented Jun 21, 2024

sweisdb commented Jun 21, 2024

LuciferYang Jul 22, 2024

[SPARK-47172][CORE] Add support for AES-GCM for RPC encryption #46515

[SPARK-47172][CORE] Add support for AES-GCM for RPC encryption #46515

Conversation

sweisdb commented May 9, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

dongjoon-hyun commented May 9, 2024

mridulm commented May 14, 2024

mridulm left a comment • edited Loading

Choose a reason for hiding this comment

mridulm left a comment

Choose a reason for hiding this comment

mridulm left a comment

Choose a reason for hiding this comment

Ngone51 Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

mridulm Jun 12, 2024

Choose a reason for hiding this comment

mridulm left a comment

Choose a reason for hiding this comment

mridulm commented Jun 18, 2024

Ngone51 Jun 18, 2024

Choose a reason for hiding this comment

sweisdb Jun 18, 2024

Choose a reason for hiding this comment

Ngone51 Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Ngone51 left a comment

Choose a reason for hiding this comment

Ngone51 Jun 19, 2024

Choose a reason for hiding this comment

sweisdb Jun 20, 2024

Choose a reason for hiding this comment

Ngone51 Jun 19, 2024

Choose a reason for hiding this comment

mridulm commented Jun 21, 2024

Ngone51 commented Jun 21, 2024

Ngone51 commented Jun 21, 2024

sweisdb commented Jun 21, 2024

LuciferYang Jul 22, 2024

Choose a reason for hiding this comment

mridulm left a comment •

edited

Loading

Ngone51 Jun 11, 2024 •

edited

Loading

Ngone51 Jun 19, 2024 •

edited

Loading