andygrove opened a new pull request, #2537: URL: https://github.com/apache/datafusion-ballista/pull/2537
# Which issue does this PR close? Closes #2535. # Rationale for this change `SET s3.secret_access_key` and `SET s3.session_token` values were written to the logs in plain text at `debug` and `trace` level, on the client, the scheduler and the executors. The issue lists the six log sites. # What changes are included in this PR? - **`ballista_core::utils::redact_config_value`** returns a config value as it may appear in a log line: unchanged, or `<redacted>` when the key contains `secret`, `password`, `token`, `credential`, `api_key` or `private_key` (ignoring case). Session config travels as plain key/value pairs, including extensions Ballista knows nothing about, so the key name is all there is to go on. This is the same approach as Spark's `spark.redaction.regex`. - **Generic config logging** (`to_key_value_pairs`, `update_from_key_value_pair_mut`) logs through that helper, so non-secret values stay visible. - **Executor task launch** logs task props through `redact_key_value_pairs`. - **`S3RegistryConfiguration`** has a manual `Debug` that redacts the secret key and session token, so the `get_store` trace line and `S3Options`'s own `Debug` are safe. The access key id, region and endpoint are still shown. - **`S3Options::set`** no longer logs values at all, for known or unknown keys. An unknown key may be a misspelled secret, and the generic trace line already shows the redacted value. Only log output changes. The values still travel in the session config as before. Two things to know when reviewing: - The key-name match is a heuristic. A secret under a key that doesn't look like one (for example a misspelled `s3.sesion_tokn` on a process without `S3Options`) is still logged at `trace` by the generic config logging. - #2536 changes the same `derive` line on `S3RegistryConfiguration`, so whichever of the two merges second needs a small rebase. # How are these changes tested? - `ballista/core/tests/secret_logging.rs` installs a capturing logger at `trace` and checks that applying config, sending config, a rejected setting, an unknown `s3.*` key and `get_store` log no secret, that `S3Options`'s `Debug` output is redacted, and that non-secret values are still logged. It is a separate test binary because the logger is process-wide. Six of the seven tests failed before the fix. - Unit tests in `utils.rs` cover which keys are redacted and `redact_key_value_pairs`. - The executor's task launch line is covered through the helper only, not by a test that drives the execution loop. # Are there any user-facing changes? Log output at `debug` and `trace` no longer contains secret values. `ballista_core::utils` gains `REDACTED`, `redact_config_value` and `redact_key_value_pairs`. No breaking changes. -- 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]
