Copilot commented on code in PR #57796:
URL: https://github.com/apache/spark/pull/57796#discussion_r3722241683
##########
core/src/main/scala/org/apache/spark/deploy/master/DriverInfo.scala:
##########
@@ -55,4 +56,25 @@ private[deploy] class DriverInfo(
def withResources(r: Map[String, ResourceInformation]): Unit = _resources = r
def resources: Map[String, ResourceInformation] = _resources
+
+ private[deploy] def redactedCopy(conf: SparkConf): DriverInfo = {
+ val redactedCommand = desc.command.copy(
+ environment = Utils.redact(conf, desc.command.environment.toSeq).toMap,
+ javaOpts = Utils.redactCommandLineArgs(conf, desc.command.javaOpts))
+ val redactedDesc = desc.copy(command = redactedCommand)
+ val copy = new DriverInfo(startTime, id, redactedDesc, submitDate)
+ copy.withResources(_resources)
+ copy
+ }
Review Comment:
Same issue as `ApplicationInfo`: `writeReplace()` serializes a newly
constructed `DriverInfo` that only rehydrates a subset of fields (constructor
args + `_resources`). Any driver state captured in other members of the
original instance will be dropped from the persisted recovery state.
`redactedCopy()` should preserve all state and only redact `desc.command`
(e.g., explicitly copy all mutable fields, or customize serialization to redact
only the sensitive subfields).
##########
core/src/main/scala/org/apache/spark/deploy/master/ApplicationInfo.scala:
##########
@@ -204,4 +205,23 @@ private[spark] class ApplicationInfo(
System.currentTimeMillis() - startTime
}
}
+
+ private[deploy] def redactedCopy(conf: SparkConf): ApplicationInfo = {
+ val redactedCommand = desc.command.copy(
+ environment = Utils.redact(conf, desc.command.environment.toSeq).toMap,
+ javaOpts = Utils.redactCommandLineArgs(conf, desc.command.javaOpts))
+ val redactedDesc = desc.copy(command = redactedCommand)
+ new ApplicationInfo(startTime, id, redactedDesc, submitDate, driver,
defaultCores)
+ }
+
+ @transient private var _conf: SparkConf = _
+
+ private[deploy] def withConf(conf: SparkConf): this.type = {
+ _conf = conf
+ this
+ }
+
+ private def writeReplace(): AnyRef = {
+ if (_conf == null) this else redactedCopy(_conf)
+ }
Review Comment:
⚠️ `writeReplace()` causes the serialized form to be a *different instance*
created by `redactedCopy()`. As written, that new `ApplicationInfo` only
carries constructor fields plus the redacted `desc`, so any mutable/derived
fields that may have been set on the original instance after construction will
not be persisted (and therefore won’t be restored on Master failover). To avoid
losing HA recovery state, `redactedCopy()` should preserve the full original
object state and only redact the sensitive parts (e.g., copy over all mutable
members, or implement custom serialization that writes a redacted
`desc.command` while keeping the rest of the object unchanged).
##########
core/src/main/scala/org/apache/spark/deploy/master/DriverInfo.scala:
##########
@@ -55,4 +56,25 @@ private[deploy] class DriverInfo(
def withResources(r: Map[String, ResourceInformation]): Unit = _resources = r
def resources: Map[String, ResourceInformation] = _resources
+
+ private[deploy] def redactedCopy(conf: SparkConf): DriverInfo = {
+ val redactedCommand = desc.command.copy(
+ environment = Utils.redact(conf, desc.command.environment.toSeq).toMap,
+ javaOpts = Utils.redactCommandLineArgs(conf, desc.command.javaOpts))
+ val redactedDesc = desc.copy(command = redactedCommand)
+ val copy = new DriverInfo(startTime, id, redactedDesc, submitDate)
+ copy.withResources(_resources)
+ copy
+ }
+
+ @transient private var _conf: SparkConf = _
+
+ private[deploy] def withConf(conf: SparkConf): this.type = {
+ _conf = conf
+ this
+ }
+
+ private def writeReplace(): AnyRef = {
+ if (_conf == null) this else redactedCopy(_conf)
+ }
Review Comment:
Same issue as `ApplicationInfo`: `writeReplace()` serializes a newly
constructed `DriverInfo` that only rehydrates a subset of fields (constructor
args + `_resources`). Any driver state captured in other members of the
original instance will be dropped from the persisted recovery state.
`redactedCopy()` should preserve all state and only redact `desc.command`
(e.g., explicitly copy all mutable fields, or customize serialization to redact
only the sensitive subfields).
##########
core/src/main/scala/org/apache/spark/deploy/master/ApplicationInfo.scala:
##########
@@ -204,4 +205,23 @@ private[spark] class ApplicationInfo(
System.currentTimeMillis() - startTime
}
}
+
+ private[deploy] def redactedCopy(conf: SparkConf): ApplicationInfo = {
+ val redactedCommand = desc.command.copy(
+ environment = Utils.redact(conf, desc.command.environment.toSeq).toMap,
+ javaOpts = Utils.redactCommandLineArgs(conf, desc.command.javaOpts))
+ val redactedDesc = desc.copy(command = redactedCommand)
+ new ApplicationInfo(startTime, id, redactedDesc, submitDate, driver,
defaultCores)
+ }
Review Comment:
⚠️ `writeReplace()` causes the serialized form to be a *different instance*
created by `redactedCopy()`. As written, that new `ApplicationInfo` only
carries constructor fields plus the redacted `desc`, so any mutable/derived
fields that may have been set on the original instance after construction will
not be persisted (and therefore won’t be restored on Master failover). To avoid
losing HA recovery state, `redactedCopy()` should preserve the full original
object state and only redact the sensitive parts (e.g., copy over all mutable
members, or implement custom serialization that writes a redacted
`desc.command` while keeping the rest of the object unchanged).
##########
core/src/main/scala/org/apache/spark/deploy/master/ApplicationInfo.scala:
##########
@@ -204,4 +205,23 @@ private[spark] class ApplicationInfo(
System.currentTimeMillis() - startTime
}
}
+
+ private[deploy] def redactedCopy(conf: SparkConf): ApplicationInfo = {
+ val redactedCommand = desc.command.copy(
+ environment = Utils.redact(conf, desc.command.environment.toSeq).toMap,
+ javaOpts = Utils.redactCommandLineArgs(conf, desc.command.javaOpts))
+ val redactedDesc = desc.copy(command = redactedCommand)
+ new ApplicationInfo(startTime, id, redactedDesc, submitDate, driver,
defaultCores)
+ }
+
+ @transient private var _conf: SparkConf = _
+
+ private[deploy] def withConf(conf: SparkConf): this.type = {
+ _conf = conf
+ this
+ }
+
+ private def writeReplace(): AnyRef = {
+ if (_conf == null) this else redactedCopy(_conf)
+ }
Review Comment:
`writeReplace()` silently falls back to serializing `this` (unredacted) when
`_conf` is not set, which means any future caller that forgets
`.withConf(conf)` can reintroduce plaintext secret persistence. Consider making
redaction non-optional by construction (e.g., require the conf/redaction
settings at `ApplicationInfo` creation time, or make persistence codepaths
enforce/attach the conf before serialization and fail fast if missing).
##########
core/src/test/scala/org/apache/spark/deploy/master/PersistenceEngineSuite.scala:
##########
@@ -42,6 +44,55 @@ class PersistenceEngineSuite extends SparkFunSuite {
}
}
+ test("SPARK-58592: FileSystemPersistenceEngine redacts secrets in
ApplicationInfo and " +
+ "DriverInfo") {
+ withTempDir { dir =>
+ val conf = new SparkConf()
+ val serializer = new JavaSerializer(conf)
+ val engine = new FileSystemPersistenceEngine(dir.getAbsolutePath,
serializer)
+ try {
+ val secretEnv = Map("PASSWORD" -> "topsecret", "JAVA_HOME" ->
"/usr/lib/jvm/default")
+ val secretJavaOpts = Seq("-Dspark.executorEnv.TOKEN=env-token",
"-Xmx2g")
+ val cmd = Command("mainClass", List("arg1"), secretEnv, Seq(), Seq(),
secretJavaOpts)
+
+ val appDesc = ApplicationDescription(
+ "name", Some(4), cmd, "appUiUrl", defaultResourceProfile)
+ val appInfo = new ApplicationInfo(
+ 0, "app-1", appDesc, new java.util.Date(0), null,
Int.MaxValue).withConf(conf)
+ engine.addApplication(appInfo)
+
+ val driverDesc = new DriverDescription("hdfs://some.jar", 100, 3,
false, cmd)
+ val driverInfo = new DriverInfo(0, "driver-1", driverDesc, new
java.util.Date(0))
+ .withConf(conf)
+ engine.addDriver(driverInfo)
+
+ // The plaintext secrets must not be present in the bytes actually
written to disk.
+ Seq("app_app-1", "driver_driver-1").foreach { fileName =>
+ val bytes = Files.readAllBytes(Paths.get(dir.getAbsolutePath,
fileName))
+ val contents = new String(bytes,
java.nio.charset.StandardCharsets.ISO_8859_1)
+ assert(!contents.contains("topsecret"))
+ assert(!contents.contains("env-token"))
+ }
Review Comment:
The on-disk byte-scan assertions can become a false positive if the
`FileSystemPersistenceEngine` output is compressed/encoded (secrets won’t
appear as plaintext even without redaction). To make this test reliably
validate redaction, configure the engine to write uncompressed bytes for the
test (e.g., set the compression codec to `none` via `SparkConf` if supported),
or alternatively decode/decompress prior to searching, or rely entirely on a
deserialize-then-assert approach and drop the raw-byte `contains` checks.
##########
core/src/test/scala/org/apache/spark/deploy/master/PersistenceEngineSuite.scala:
##########
@@ -42,6 +44,55 @@ class PersistenceEngineSuite extends SparkFunSuite {
}
}
+ test("SPARK-58592: FileSystemPersistenceEngine redacts secrets in
ApplicationInfo and " +
+ "DriverInfo") {
+ withTempDir { dir =>
+ val conf = new SparkConf()
+ val serializer = new JavaSerializer(conf)
+ val engine = new FileSystemPersistenceEngine(dir.getAbsolutePath,
serializer)
Review Comment:
The on-disk byte-scan assertions can become a false positive if the
`FileSystemPersistenceEngine` output is compressed/encoded (secrets won’t
appear as plaintext even without redaction). To make this test reliably
validate redaction, configure the engine to write uncompressed bytes for the
test (e.g., set the compression codec to `none` via `SparkConf` if supported),
or alternatively decode/decompress prior to searching, or rely entirely on a
deserialize-then-assert approach and drop the raw-byte `contains` checks.
--
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]
