HyukjinKwon commented on code in PR #59185:
URL: https://github.com/apache/spark/pull/59185#discussion_r4156230924
##########
core/src/main/scala/org/apache/spark/deploy/security/UserCredentialManager.scala:
##########
@@ -519,6 +527,45 @@ private[spark] object UserCredentialManager extends
Logging {
}
}
+ /**
+ * Returns true if Spark's RPC channel encryption is configured, using the
same criteria as
+ * [[HadoopDelegationTokenManager]]'s direct-credential-provider check:
either SSL-based RPC
+ * encryption (`spark.ssl.rpc.enabled`), or AES-based encryption enabled
together with
Review Comment:
**Nit (P3):** Only `spark.network.crypto.enabled` is the AES-based mode.
`spark.authenticate.enableSaslEncryption` is the separate (deprecated)
SASL-based encryption described in docs/security.md. Something like "or
AES-based (`spark.network.crypto.enabled`) or SASL-based
(`spark.authenticate.enableSaslEncryption`) encryption together with
authentication (`spark.authenticate`)" would describe it accurately.
**Verification:**
- **Inspection:** The comment names AES-based and SASL-based encryption
separately, consistent with docs/security.md.
##########
core/src/main/scala/org/apache/spark/deploy/security/UserCredentialManager.scala:
##########
@@ -519,6 +527,45 @@ private[spark] object UserCredentialManager extends
Logging {
}
}
+ /**
+ * Returns true if Spark's RPC channel encryption is configured, using the
same criteria as
+ * [[HadoopDelegationTokenManager]]'s direct-credential-provider check:
either SSL-based RPC
+ * encryption (`spark.ssl.rpc.enabled`), or AES-based encryption enabled
together with
+ * authentication (`spark.authenticate` plus one of
`spark.network.crypto.enabled` /
+ * `spark.authenticate.enableSaslEncryption`).
+ *
+ * NOTE: This condition is intentionally kept identical to the
`hasEncryption` check in
+ * `HadoopDelegationTokenManager` (the Kerberos direct-credential-provider
path). If the set of
+ * config keys that constitute "RPC encryption is enabled" changes, update
both places together.
+ */
+ private[security] def isRpcEncryptionEnabled(sparkConf: SparkConf): Boolean
= {
+ sparkConf.getBoolean("spark.ssl.rpc.enabled", false) ||
Review Comment:
**Nit (P3):** This expression duplicates `hasEncryption` in
`HadoopDelegationTokenManager` (lines 82-84). The NOTE asks maintainers to keep
the two in sync, but nothing enforces it, and the tests only exercise this
copy. Both classes are in `org.apache.spark.deploy.security`, so
`HadoopDelegationTokenManager` could call this `private[security]` helper
directly (or both could use one shared helper). That would make the NOTE
unnecessary and keep the OIDC warning and the direct-credential-provider
`require()` from drifting apart.
**Verification:**
- **Regression:** The direct-credential-provider path still fails startup
without RPC encryption and succeeds with each accepted encryption configuration.
- **Inspection:** No second copy of the encryption predicate remains in
deploy.security.
##########
core/src/test/scala/org/apache/spark/deploy/security/UserCredentialManagerSuite.scala:
##########
@@ -858,4 +859,78 @@ class UserCredentialManagerSuite extends SparkFunSuite {
AnotherFakeCredentialProvider.throwOnProperties = false
}
}
+
+ // ========== RPC encryption warning ==========
+
+ private val encryptionWarningFragment = "RPC channel encryption is not
configured"
+
+ test("isRpcEncryptionEnabled: false when nothing is configured") {
+ assert(!UserCredentialManager.isRpcEncryptionEnabled(new
SparkConf(loadDefaults = false)))
+ }
+
+ test("isRpcEncryptionEnabled: true with SSL RPC encryption") {
+ val conf = new SparkConf(loadDefaults =
false).set("spark.ssl.rpc.enabled", "true")
+ assert(UserCredentialManager.isRpcEncryptionEnabled(conf))
+ }
+
+ test("isRpcEncryptionEnabled: true with authentication + network crypto") {
+ val conf = new SparkConf(loadDefaults = false)
+ .set(NETWORK_AUTH_ENABLED, true)
+ .set(NETWORK_CRYPTO_ENABLED, true)
+ assert(UserCredentialManager.isRpcEncryptionEnabled(conf))
+ }
+
+ test("isRpcEncryptionEnabled: true with authentication + SASL encryption") {
+ val conf = new SparkConf(loadDefaults = false)
+ .set(NETWORK_AUTH_ENABLED, true)
+ .set(SASL_ENCRYPTION_ENABLED, true)
+ assert(UserCredentialManager.isRpcEncryptionEnabled(conf))
+ }
+
+ test("isRpcEncryptionEnabled: false when authentication is enabled without
encryption") {
Review Comment:
**Nit (P3):** None of these cases enables `spark.network.crypto.enabled` or
`spark.authenticate.enableSaslEncryption` without `spark.authenticate`. If
someone dropped the `NETWORK_AUTH_ENABLED &&` conjunct from
`isRpcEncryptionEnabled`, all of these tests would still pass, and the warning
would no longer fire for e.g. `spark.network.crypto.enabled=true` alone, which
does not actually encrypt anything. Could you add a case asserting
`isRpcEncryptionEnabled` is false for crypto (and/or SASL) enabled with auth
off?
**Verification:**
- **Regression:** Encryption flags enabled without authentication are
reported as not encrypted.
--
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]
