mridulm commented on code in PR #45425:
URL: https://github.com/apache/spark/pull/45425#discussion_r1525236753
##########
common/network-common/src/main/java/org/apache/spark/network/crypto/AuthEngine.java:
##########
@@ -213,7 +220,7 @@ private TransportCipher generateTransportCipher(
transcript, // Passing this as the HKDF salt
OUTPUT_IV_INFO, // This is the HKDF info field used to differentiate
IV values
AES_GCM_KEY_SIZE_BYTES);
- SecretKeySpec sessionKey = new SecretKeySpec(sharedSecret, "AES");
+ SecretKeySpec sessionKey = new SecretKeySpec(derivedKey, "AES");
Review Comment:
Let us flag guard this change - given we want to backport to 3.x patch
releases as well as giving users migration opportunities. This flag can be
flipped to default in a subsequent minor version.
##########
common/network-common/src/main/java/org/apache/spark/network/crypto/README.md:
##########
@@ -1,6 +1,20 @@
-Forward Secure Auth Protocol
+Forward Secure Auth Protocol v1.1
==============================================
+Deprecation Notice
+------------------
+This is a bespoke key exchange protocol that was implemented before Spark
supported TLS (aka SSL) for RPC
+calls. It is recommended that Spark users upgrade to using TLS for RPC calls
between Spark processes. This protocol
+will be deprecated and removed in the long-term.
+
+See
+the [Spark security
documentation](https://github.com/apache/spark/blob/master/docs/security.md#ssl-encryption)
for
+more information on how to configure TLS.
Review Comment:
I am not sure if we should announce deprecation given TLS is going to come
out only in 4.0 ... we can recommend users to move to TLS though.
Would like feedback from others on this though ... @dongjoon-hyun, @srowen,
@JoshRosen
##########
common/network-common/src/main/java/org/apache/spark/network/crypto/README.md:
##########
@@ -99,3 +114,13 @@ sessions. It would, however, allow impersonation of future
sessions.
In the event of a pre-shared key compromise, messages would still be
confidential from a passive
observer. Only active adversaries spoofing a session would be able to recover
plaintext.
+Security Changes & Compatibility
+-------------
+
+The original version of this protocol, retroactively called v1.0, did not
apply an HKDF to `sharedSecret` and was
+directly using the encoded X coordinate as key material. This is atypical and
standard practice is to pass that shared
+coordinate through an HKDF. The current version, v1.1, adds this additional
HKDF to
+derive `derivedKey`.
Review Comment:
Essentially, this is backwardly incompatible with 3.x on the wire - making
adoption of 4.x in secure env all-or-nothing
Given this, I am leaning towards flag guarding the change.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
