Skip to content

fix: Write secret files without following symlinks - #775

Open
lfrancke wants to merge 2 commits into
mainfrom
fix/write-secret-files-nofollow
Open

lfrancke wants to merge 2 commits into
mainfrom
fix/write-secret-files-nofollow

Conversation

@lfrancke

@lfrancke lfrancke commented Nov 10, 2026 •

Copy link
Copy Markdown
Member

Description

This is a followup to #773 and #770.
There are two places where we write files now: the marker file and the secret.
In the last PRs we secured the marker file against "symlink" attacks. The problem is that the pod can write to its own volume so it may have replaced any part of the path with a symlink.
This PR also secures the secret writing part by combining them into one write function.

This introduces a new direct dependecy on rustix but we already had an indirect dependency on it before. The alternative that i've used in the past would be the cap-std library/crate but that's a whole new thing.

Reviewer

  • Code contains useful comments
  • Code contains useful logging statements
  • Changelog updated
  • Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

  • Proper release label has been added

Processes in the Pod can write to the volume, because the kubelet applies
the fsGroup with group write permissions, so any entry in it may have been
replaced since the last publish. Secret files and their directories are now
opened relative to their parent without following symlinks, and data is only
written into regular files.
@lfrancke
lfrancke force-pushed the fix/write-secret-files-nofollow branch from 932fea6 to d469897 Compare November 10, 2026 21:34
@lfrancke
lfrancke marked this pull request as ready for review November 10, 2026 21:41
@lfrancke lfrancke moved this to Development: Waiting for Review in Stackable Engineering Nov 10, 2026
@Techassi
Techassi self-requested a review November 11, 2026 07:59
};
use serde::{Deserialize, de::IntoDeserializer};
use snafu::{ResultExt, Snafu, ensure};
use snafu::{OptionExt, ResultExt, Snafu, ensure};

@NickLarsenNZ NickLarsenNZ Nov 11, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it was already done that way for ResultExt, but it's prefarable to show that we only need the trait methods:

Suggested change
use snafu::{OptionExt, ResultExt, Snafu, ensure};
use snafu::{OptionExt as _, ResultExt as _, Snafu, ensure};

.await
.context(publish_error::CreateFileSnafu { path })?;
Ok(())
write_secret_file(target_path, Path::new(PROVISIONED_MARKER_FILE_NAME), &[])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a const (might need to be PathBuf).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realise it wasn't before, so no big deal

assert_eq!(contents, b"key");
}

#[test]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think tests using std::os::unix should technically be gated. But not a big deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Development: Waiting for Review

Development

Successfully merging this pull request may close these issues.

2 participants