Discourage use of unredacted summary file#1637
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omertuc The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThis PR introduces a security mechanism to prevent accidental emission of unredacted secret keys in summary output. A new Cargo feature flag gates unredacted summary file writing: disabled by default, it panics to prevent secrets from being written; when enabled, it writes the file with restrictive ChangesUnredacted Summary Security Gating
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/logging.rs`:
- Around line 111-113: The binding summary_file in the if let Some(summary_file)
= summary.recert_config.summary_file.clone() is unused when the feature
insecure_dev_only_unredacted_summary is disabled and triggers a compiler
warning; either rename the binding to _summary_file to silence the warning or
move the #[cfg(not(feature = "insecure_dev_only_unredacted_summary"))] to the
entire if-let block so the variable is never bound when that feature is
disabled; update the code around summary.recert_config.summary_file and the
panic site accordingly.
- Around line 115-122: The current code creates the summary file with default
umask then calls std::fs::set_permissions which introduces a TOCTOU window;
replace summary_file.0.create() + set_permissions(...) with creating the file
atomically with the desired mode via std::fs::OpenOptions and
OpenOptionsExt::mode(0o600) (e.g. use
OpenOptions::new().write(true).create(true).truncate(true).mode(0o600).open(path)
and write the YAML with serde_yaml::to_writer), remove the separate
Permissions::from_mode/set_permissions call, and add the necessary use
std::os::unix::fs::OpenOptionsExt import and fix rustfmt issues in the block;
reference the summary_file variable, the create() call being replaced, and
serde_yaml::to_writer for where to write the content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1de362aa-67c2-43cf-9642-6b50531ba65d
📒 Files selected for processing (2)
Cargo.tomlsrc/logging.rs
| #[cfg(feature = "insecure_dev_only_unredacted_summary")] | ||
| { | ||
| let path = summary_file.0.path().to_owned(); | ||
| let file = summary_file.0.create().context("opening summary file for writing")?; | ||
| std::fs::set_permissions(&path, Permissions::from_mode(0o600)) | ||
| .context("setting summary file permissions to 0600")?; | ||
| serde_yaml::to_writer(file, &summary).context("serializing cluster crypto into summary file")?; | ||
| } |
There was a problem hiding this comment.
TOCTOU race: file is briefly world-readable before set_permissions applies.
The file is created with default umask permissions (often 0644), then set_permissions is called afterward. During this window, another process on a shared system could read the unredacted secrets, defeating the purpose of restrictive permissions.
Set the mode atomically at file creation using OpenOptionsExt::mode():
🔒 Proposed fix using atomic permission setting
+use std::fs::OpenOptions;
+use std::os::unix::fs::OpenOptionsExt;
use std::fs::Permissions;
-use std::os::unix::fs::PermissionsExt; #[cfg(feature = "insecure_dev_only_unredacted_summary")]
{
let path = summary_file.0.path().to_owned();
- let file = summary_file.0.create().context("opening summary file for writing")?;
- std::fs::set_permissions(&path, Permissions::from_mode(0o600))
- .context("setting summary file permissions to 0600")?;
+ let file = OpenOptions::new()
+ .write(true)
+ .create(true)
+ .truncate(true)
+ .mode(0o600)
+ .open(&path)
+ .context("creating summary file with 0600 permissions")?;
serde_yaml::to_writer(file, &summary).context("serializing cluster crypto into summary file")?;
}Note: The pipeline also flagged a rustfmt issue in this block that will need to be addressed.
🧰 Tools
🪛 GitHub Actions: Rust / 0_build.txt
[error] 116-118: cargo fmt --check failed in rust-fmt (Makefile:171). Formatting diff detected: 'std::fs::set_permissions(...).context(...)' needs to be split onto separate lines as per rustfmt output.
🪛 GitHub Actions: Rust / build
[error] 116-116: cargo fmt --check failed due to formatting difference in src/logging.rs. std::fs::set_permissions(&path, Permissions::from_mode(0o600)) should be chained with .context(...) without a line break.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/logging.rs` around lines 115 - 122, The current code creates the summary
file with default umask then calls std::fs::set_permissions which introduces a
TOCTOU window; replace summary_file.0.create() + set_permissions(...) with
creating the file atomically with the desired mode via std::fs::OpenOptions and
OpenOptionsExt::mode(0o600) (e.g. use
OpenOptions::new().write(true).create(true).truncate(true).mode(0o600).open(path)
and write the YAML with serde_yaml::to_writer), remove the separate
Permissions::from_mode/set_permissions call, and add the necessary use
std::os::unix::fs::OpenOptionsExt import and fix rustfmt issues in the block;
reference the summary_file variable, the create() call being replaced, and
serde_yaml::to_writer for where to write the content.
Source: Pipeline failures
|
/retest |
omertuc
force-pushed
the
summaryno
branch
2 times, most recently
from
3ae5815 to
63d9175
Compare
The summary file option is too innocently named when considering what it outputs. It's not used in any production contexts. It's only meant for dev. This commit puts it behind a cargo feature flag, and bails if you try to use it without the feature. Also sets the permissions of the summary file to 0600.
|
/override ci/prow/e2e-aws-ovn-single-node-recert-parallel |
|
@omertuc: Overrode contexts on behalf of omertuc: ci/prow/e2e-aws-ovn-single-node-recert-parallel DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
1 participant
See commit message