sarutak commented on code in PR #59185:
URL: https://github.com/apache/spark/pull/59185#discussion_r4163297916
##########
core/src/main/scala/org/apache/spark/deploy/security/UserCredentialManager.scala:
##########
@@ -519,6 +527,44 @@ private[spark] object UserCredentialManager extends
Logging {
}
}
+ /**
+ * Returns true if Spark's RPC channel encryption is configured: either
SSL-based RPC encryption
+ * (`spark.ssl.rpc.enabled`), or authentication (`spark.authenticate`)
together with either
+ * AES-based encryption (`spark.network.crypto.enabled`) or the separate
(deprecated) SASL-based
+ * encryption (`spark.authenticate.enableSaslEncryption`).
+ *
+ * This is shared with [[HadoopDelegationTokenManager]]'s
direct-credential-provider check so the
+ * two credential-propagation paths cannot drift apart: that path fails
startup via `require()`,
+ * while OIDC propagation only warns (see [[warnIfRpcEncryptionDisabled]]).
+ */
+ private[security] def isRpcEncryptionEnabled(sparkConf: SparkConf): Boolean
= {
Review Comment:
Agreed on the direction: the predicate shouldn't live in the OIDC-specific
object, and `HadoopDelegationTokenManager` shouldn't have to reach into it. I
moved it to a neutral home: `SecurityManager.isRpcEncryptionEnabled(conf)` (a
`private[spark]` helper on the companion object). Both the OIDC warning and the
direct-credential-provider `require()` now call that, so neither path depends
on the other.
On reusing the existing `isEncryptionEnabled()` / `TransportConf`: I looked
at that first, but they answer a different question and reusing them would be
incorrect here. `SecurityManager.isEncryptionEnabled()`:
- treats SSL RPC as *disabling* AES/SASL (it returns `false` when
`spark.ssl.rpc.enabled=true`, because AES/SASL and SSL RPC are mutually
exclusive transports); and
- does not require `spark.authenticate`.
Its only caller (`SparkEnv`) relies on exactly that, pairing it with
`isSslRpcEnabled()`:
`if (!(securityManager.isEncryptionEnabled() ||
securityManager.isSslRpcEnabled())) { warn }`.
So for the credential paths it gives the wrong answer in two concrete cases:
- `spark.ssl.rpc.enabled=true` alone: `isEncryptionEnabled()` returns
**false**, but the channel *is* encrypted (we'd warn / fail spuriously).
- `spark.network.crypto.enabled=true` with `spark.authenticate=false`:
`isEncryptionEnabled()` returns true, but nothing is actually encrypted (we'd
stay silent). This is exactly the gap the negative tests pin.
Rather than overload `isEncryptionEnabled()` with a second meaning, I added
a separate, clearly-named predicate and documented the distinction in both
scaladocs (and added a cross-reference on `isEncryptionEnabled()` pointing at
the new one). I also added a `SecurityManager.SSL_RPC_ENABLED_CONF` constant as
the single source of truth for `spark.ssl.rpc.enabled` (which has no Scala
`ConfigEntry`) and used it in the predicate, the field, and the remediation.
--
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]
