You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "wangyum (via GitHub)" <gi...@apache.org> on 2026/08/05 15:40:50 UTC
[PR] [SPARK-58592][CORE] Redact secrets in persisted Master recovery state [spark]
wangyum opened a new pull request, #57796:
URL: https://github.com/apache/spark/pull/57796
### What changes were proposed in this pull request?
`ApplicationInfo` and `DriverInfo` are tagged with the active `SparkConf` at the two
places `Master` constructs them (`createApplication`/`createDriver`), and a
`writeReplace()` hook is added to both classes so that, whenever an instance is
actually Java-serialized (i.e. by `PersistenceEngine` when persisting Master HA
recovery state to ZooKeeper, the filesystem, or RocksDB), a redacted copy is
substituted in place of the original. Redaction reuses the existing `Utils.redact`
/ `Utils.redactCommandLineArgs` helpers already used elsewhere (e.g.
`JsonProtocol.writeApplicationDescription`) to scrub the app/driver `Command`'s
environment variables and `-D` Java options.
The in-memory objects used by the running `Master` are untouched; only the bytes
that get written to the recovery store are redacted.
### Why are the changes needed?
`PersistenceEngine` implementations persist raw `ApplicationInfo`/`DriverInfo`
objects via plain Java serialization for Standalone Master HA recovery. If an
application or driver's launch command contains secrets in its environment
variables or Java options (e.g. passwords, tokens), those secrets are currently
written in plaintext to ZooKeeper znodes or the on-disk recovery directory,
where they can persist indefinitely and be read by anyone with access to that
store.
### Does this PR introduce _any_ user-facing change?
No. The Master UI/REST JSON output and RPC responses are unchanged; the fix only
affects the bytes written to the HA recovery store (ZooKeeper/filesystem/RocksDB),
which are not directly user-visible.
### How was this patch tested?
Added tests:
- `JsonProtocolSuite`: `SPARK-58592: redactedCopy redacts secrets in ApplicationInfo
and DriverInfo` verifies the redaction helper directly; `SPARK-58592: writeReplace
redacts secrets when Java-serialized for persistence` round-trips tagged instances
through a real `JavaSerializer` and asserts secrets are redacted post-deserialize
while non-secret fields survive.
- `PersistenceEngineSuite`: `SPARK-58592: FileSystemPersistenceEngine redacts secrets
in ApplicationInfo and DriverInfo` persists a secret-bearing `ApplicationInfo`/
`DriverInfo` through a real `FileSystemPersistenceEngine`, reads the raw bytes
written to disk to assert the plaintext secret is absent, and reads the objects
back via `PersistenceEngine.read` to confirm redacted values and intact non-secret
fields.
Ran:
```
build/sbt "core/testOnly org.apache.spark.deploy.JsonProtocolSuite org.apache.spark.deploy.master.PersistenceEngineSuite"
```
All 22 tests pass.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-sonnet-5)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58592][CORE] Redact secrets in Standalone Master RequestMasterState RPC [spark]
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on code in PR #57796:
URL: https://github.com/apache/spark/pull/57796#discussion_r3726536227
##########
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:
May be persistence handling for sensitive fields:
- When spark.authenticate=true: consider encrypting the persisted data instead of redacting it.
- When spark.authenticate=false: no change to existing behavior.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58592][CORE] Redact secrets in Standalone Master RequestMasterState RPC [spark]
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on code in PR #57796:
URL: https://github.com/apache/spark/pull/57796#discussion_r3726088010
##########
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:
Thank you @uros-b, I reduced the scope to RPC only and this only affects Spark Standalone mode. ApplicationInfo/DriverInfo are Standalone/Master-specific classes; YARN and Kubernetes do not use them or have an equivalent RPC.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58592][CORE] Redact secrets in persisted Master recovery state [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #57796:
URL: https://github.com/apache/spark/pull/57796#discussion_r3724662831
##########
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:
Functional regression in HA recovery for supervised drivers and executors. After a master crash and recovery, the objects deserialized from disk are already-redacted copies: desc.command.environment and desc.command.javaOpts contain Utils.REDACTION_REPLACEMENT_TEXT in place of real secrets.
The recovery path in Master.completeRecovery() calls relaunchDriver(d) for every supervised driver whose worker is gone, and relaunchDriver calls createDriver(driver.desc); where driver.desc is the recovered, permanently-redacted DriverDescription. The resulting DriverInfo (even with withConf(conf) attached) has a redacted command, so the worker receives LaunchDriver with REDACTION_REPLACEMENT_TEXT in env vars. Any driver that requires env-var secrets (e.g. HADOOP_CREDSTORE_PASSWORD, AWS_ACCESS_KEY) will fail to authenticate after master-crash recovery. The same applies to executor re-launches: when a recovered application needs new executors, launchExecutor sends exec.application.desc (the redacted ApplicationDescription) to the worker, which builds the executor process with the redacted environment. The PersistenceEngineSuite test explicitly asserts recoveredApp.desc.command.environment("PASSWORD") == Utils.REDACTION_REPLACEMENT_TEXT, confirming the regression.
The fix should store an out-of-band, separate redacted copy for persistence (e.g., serialize a lightweight ApplicationDescription/DriverDescription snapshot with redacted fields) rather than having the live deserialized object carry permanently-redacted state.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58592][CORE] Redact secrets in Standalone Master RequestMasterState RPC [spark]
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on PR #57796:
URL: https://github.com/apache/spark/pull/57796#issuecomment-5830441124
Thank you @uros-b. I have fixed the lint issues
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58592][CORE] Redact secrets in Standalone Master RequestMasterState RPC [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #57796:
URL: https://github.com/apache/spark/pull/57796#issuecomment-5802440713
There are some lint issues here - @wangyum could you please take a look? If these are not related to your changes, please just try to re-trigger and/or rebase / merge latest master, and this should make the CI green here
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58592][CORE] Redact secrets in persisted Master recovery state [spark]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58592][CORE] Redact secrets in Standalone Master RequestMasterState RPC [spark]
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on code in PR #57796:
URL: https://github.com/apache/spark/pull/57796#discussion_r3726088010
##########
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:
Thank you @uros-b, I reduced the scope to RPC only, and this only affects Spark Standalone mode. ApplicationInfo/DriverInfo are Standalone/Master-specific classes; YARN and Kubernetes do not use them or have an equivalent RPC.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58592][CORE] Redact secrets in Standalone Master RequestMasterState RPC [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #57796:
URL: https://github.com/apache/spark/pull/57796#issuecomment-5768382535
I reviewed this and the core design checks out. For the record, I verified the assumptions it rests on:
- All in-tree `RequestMasterState` callers (`MasterPage`, `ApplicationPage`, `MasterWebUI`, and every test) go through `master.self.askSync`, and `NettyRpcEnv.askAbortable` takes the `dispatcher.postLocalMessage` path when `remoteAddr == address`, so no serialization happens and the Master UI is unaffected.
- `WorkerInfo.executors` / `WorkerInfo.drivers` are `@transient`, so there is no un-redacted `ApplicationInfo` reachable through `ExecutorDesc.application` in the serialized graph. The `workers` array is not a bypass.
- `ObjectOutputStream` does not re-invoke `writeReplace` when the replacement has the same class, and `_conf` is `@transient` on the replacement anyway, so there is no recursion.
- `defaultCores` was already a field before this PR (the `getOrElse` lambda in `requestedCores` captures it -- `javap` shows `private final int defaultCores`), so referencing it from `redactedCopy` does not change `ApplicationInfo`'s default `serialVersionUID`. Master recovery from `ZooKeeperPersistenceEngine` / `FileSystemPersistenceEngine` stays compatible. Worth stating explicitly, since a SUID change there would have silently deleted persisted apps/drivers on upgrade (`deserializeFromFile` catches and `zk.delete()`s).
Three findings, all minor:
### 1. `javaOpts` redaction is never actually asserted (`MasterSuite.scala` L282, L291, L319, L327)
```scala
assert(!redactedApp.desc.command.javaOpts.contains("env-token"))
```
`Seq[String].contains` is element equality, and no element ever equals `"env-token"` -- it is `-Dspark.executorEnv.TOKEN=env-token` before redaction and `-Dspark.executorEnv.TOKEN=*********(redacted)` after. The assertion is vacuously true in both states.
Concretely: drop `javaOpts = Utils.redactCommandLineArgs(conf, javaOpts)` from `Command.redactedCopy` and all three new tests still pass. The `-Xmx2g` assertions and the `PASSWORD` env assertions are unaffected, and only the negative test (`without withConf`) compares the full `-Dspark.executorEnv.TOKEN=env-token` string. So half of what `redactedCopy` does is currently uncovered.
Suggested:
```scala
assert(redactedApp.desc.command.javaOpts.contains(
s"-Dspark.executorEnv.TOKEN=${Utils.REDACTION_REPLACEMENT_TEXT}"))
```
### 2. `Map.contains` checks keys, not values (`MasterSuite.scala` L279, L288, L316, L323)
```scala
assert(!redactedApp.desc.command.environment.contains("topsecret"))
```
`scala.collection.Map.contains` is a key lookup, and no key is ever `"topsecret"`, so this holds even if `Utils.redact` were removed entirely. The real coverage is the next line (`environment("PASSWORD") == Utils.REDACTION_REPLACEMENT_TEXT`), which is correct -- these four lines just read as if they were checking values when they are not. `!environment.values.exists(_.contains("topsecret"))` if a value-level check is wanted, otherwise they can go.
### 3. `ApplicationInfo.redactedCopy` drops `_retryCount`, contradicting its own comment
```scala
// Any future non-transient field added to ApplicationInfo must be copied here
// to avoid silently dropping it from the serialized (redacted) form.
private[deploy] def redactedCopy(conf: SparkConf): ApplicationInfo = {
```
`private var _retryCount = 0` has no `@transient`, so it is already part of the serialized form (`javap`: `private int _retryCount;`), and the new instance leaves it at 0. An app that has been retried N times (`Master.scala`, `log"failed ${MDC(LogKeys.NUM_RETRY, appInfo.retryCount)} times"`) goes over the wire as 0.
The practical impact is nil today -- no in-tree consumer reads it off the wire -- but the invariant the comment states is not actually held by the code as written, which will mislead whoever adds the next field. Either copy it (`copy.resetRetryCount()` is not enough; it needs a setter or a constructor param) or narrow the comment to say which fields are intentionally not carried over.
### Non-blocking notes
- `withConf` is opt-in / fail-open: a future `MasterStateResponse` reply site that forgets it leaks silently. There is only one construction site today (`Master.scala`), so this is not a defect, but it is worth a thought vs. making redaction the default and having the persistence path opt out.
- The comment in the third test ("The persistence/recovery path relies on this contract to preserve original secrets on disk") is a bit off: `PersistenceEngine` serializes `ApplicationInfo` / `DriverInfo` directly, never `MasterStateResponse`, so it does not depend on this contract. The PR description gets this right; just the test comment overstates it.
- The same class of leak still exists on the Worker side: `RequestWorkerState` -> `WorkerStateResponse` carries `ExecutorRunner.appDesc` and `DriverRunner.driverDesc`, both with an unredacted `Command`. Out of scope here, but a natural follow-up now that `Command.redactedCopy` exists.
Line lengths and ASCII are clean on the changed files.
Reviewed with Claude Opus 5.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58592][CORE] Redact secrets in Standalone Master RequestMasterState RPC [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #57796:
URL: https://github.com/apache/spark/pull/57796#issuecomment-5768314058
To @wangyum , I already approved your PR and I thought you were going to merge this already.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58592][CORE] Redact secrets in Standalone Master RequestMasterState RPC [spark]
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on PR #57796:
URL: https://github.com/apache/spark/pull/57796#issuecomment-5269415365
ping @dongjoon-hyun
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org