Skip to content

feat: restrict directories for included files in the cluster templates#2608

Open
Unix4ever wants to merge 1 commit intosiderolabs:mainfrom
Unix4ever:restrict-included-files-scope-cluster-templates
Open

feat: restrict directories for included files in the cluster templates#2608
Unix4ever wants to merge 1 commit intosiderolabs:mainfrom
Unix4ever:restrict-included-files-scope-cluster-templates

Conversation

@Unix4ever
Copy link
Copy Markdown
Member

By default only allow to include files from the same directory where the template file lives.
This is to prevent malicious cluster templates that include something like /etc/passwd.
Fixes: #2590

Copilot AI review requested due to automatic review settings March 31, 2026 09:18
@github-project-automation github-project-automation bot moved this to To Do in Planning Mar 31, 2026
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds allow-list based path validation for files referenced by cluster templates to mitigate malicious templates including arbitrary local files (e.g., /etc/passwd).

Changes:

  • Introduces AllowedFilePaths validation options and threads them through template/model validation.
  • Adds ValidateFilePath helper + unit tests, and extends template tests for allowed-path behavior.
  • Adds --allowed-paths omnictl flag and wires it into template sync/diff flows.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
client/pkg/testdata/parent-patch.yaml Adds test fixture patch file located outside template testdata dir.
client/pkg/template/template.go Adds ValidateOptions and passes allow-list to model validation.
client/pkg/template/template_test.go Updates validation calls and adds allow-list validation coverage.
client/pkg/template/operations/validate.go Updates validation call site (currently passes empty options).
client/pkg/template/operations/sync.go Adds AllowedFilePaths to SyncOptions and passes through to validation.
client/pkg/template/operations/status.go Updates validation call site (currently passes empty options).
client/pkg/template/operations/render.go Updates validation call site (currently passes empty options).
client/pkg/template/operations/export.go Updates model list validation signature to accept options.
client/pkg/template/operations/diff.go Adds allowedFilePaths parameter and passes through to validation.
client/pkg/template/operations/delete.go Updates validation call site (currently passes empty options).
client/pkg/template/internal/models/workers.go Threads validation options down to patch validation.
client/pkg/template/internal/models/validate_path.go Implements allow-list directory check for referenced files.
client/pkg/template/internal/models/validate_path_test.go Adds unit tests for file path allow-list behavior.
client/pkg/template/internal/models/patch.go Adds allow-list check before reading patch files during validation.
client/pkg/template/internal/models/models.go Adds ValidateOptions and updates Model interface signature.
client/pkg/template/internal/models/machine.go Threads validation options down to patch validation.
client/pkg/template/internal/models/list.go Threads validation options to all models during list validation.
client/pkg/template/internal/models/kubernetes_manifest.go Adds allow-list check for manifest file paths during validation.
client/pkg/template/internal/models/controlplane.go Threads validation options down to patch validation.
client/pkg/template/internal/models/cluster.go Threads validation options down to patches/manifests validation.
client/pkg/omnictl/cluster/template/template.go Adds --allowed-paths flag and resolves relative paths against template directory.
client/pkg/omnictl/cluster/template/sync.go Sets AllowedFilePaths per processed template file before syncing.
client/pkg/omnictl/cluster/template/diff.go Passes resolved allowed paths into diff operation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

//
// Symlinks in the file path are resolved to prevent bypassing the allowed path restriction
// (e.g., a symlink ./patches/passwd -> /etc/passwd would be caught).
func ValidateFilePath(filePath string, allowedPaths []string) error {
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 if we really want to be strict about this check, we should do it not on validation, but the moment we try to open the file.

When we do validation, e.g. a symlink might be fine, but potentially something might replace the symlink target after the validation.

Also, from security perspective, I think it's better to use https://pkg.go.dev/os#Root as it already has lots of logic to prevent reads outside of the root.

There might be an easier model if we allow a single root (allowedPaths is a single entry), and open any file we need from within that root throughout the whole module.


modelList := buildModelList(clusterModel, controlPlaneMachineSetModel, workerMachineSetModels, machineModels)
if err = modelList.Validate(); err != nil {
if err = modelList.Validate(models.ValidateOptions{}); err != nil {
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 like that we simply use an options struct, no functional args pattern etc.

Copy link
Copy Markdown
Member

@utkuozdemir utkuozdemir left a comment

Choose a reason for hiding this comment

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

LGTM

@github-project-automation github-project-automation bot moved this from In Review to Approved in Planning Apr 9, 2026
By default only allow to include files from the same directory where the
template file lives.
This is to prevent malicious cluster templates that include something
like `/etc/passwd`.
Fixes: siderolabs#2590

Signed-off-by: Artem Chernyshev <artem.chernyshev@talos-systems.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration/e2e-templates Triggers all e2e templates tests for Omni status/ok-to-test

Projects

Status: Approved

Development

Successfully merging this pull request may close these issues.

[feature] limit cluster template access to files outside of the template file directly by default

5 participants