fix(security): validate node labels and IPs before shell interpolation #656

ArangoGutierrez · 2026-02-12T20:28:20Z

Summary

Add Kubernetes label pattern validation to reject shell metacharacters in label keys/values
Validate PrivateIP with net.ParseIP before interpolation into grep commands
Prevents command injection via crafted YAML config

Audit Findings

Fix initial release #17 (MEDIUM): Label values interpolated into kubectl commands without sanitization
Fix initial release #18 (MEDIUM): PrivateIP interpolated into grep commands without validation

Changes

api/holodeck/v1alpha1/validation.go: Add validateLabels function with k8s label pattern
pkg/provisioner/cluster.go: Add net.ParseIP validation before shell interpolation

Test plan

gofmt — no formatting issues
go build — compiles
go test ./pkg/... — all tests pass

User-controlled label keys/values from YAML config were directly interpolated into kubectl commands via fmt.Sprintf, enabling command injection. Add label validation against Kubernetes label pattern. Also validate PrivateIP with net.ParseIP before grep interpolation. Audit findings NVIDIA#17 (MEDIUM), NVIDIA#18 (MEDIUM). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>

coveralls · 2026-02-12T20:31:58Z

Pull Request Test Coverage Report for Build 21963082665

Details

0 of 5 (0.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.03%) to 47.474%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/provisioner/cluster.go	0	5	0.0%

Totals
Change from base Build 21955389842:	-0.03%
Covered Lines:	2500
Relevant Lines:	5266

💛 - Coveralls

Copilot

Pull request overview

This PR aims to address command injection vulnerabilities (audit findings #17 and #18) by adding validation for Kubernetes labels and node IP addresses before they are interpolated into shell commands. However, the implementation has critical gaps that prevent it from actually protecting against the vulnerabilities it attempts to fix.

Changes:

Add validateLabels function with regex pattern to validate Kubernetes label keys and values
Add IP address validation using net.ParseIP before interpolation into grep commands
Integrate validation calls into ClusterSpec.Validate() method

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
api/holodeck/v1alpha1/validation.go	Adds validateLabels function with regex pattern and integrates it into ClusterSpec.Validate() to check control-plane and worker labels
pkg/provisioner/cluster.go	Adds net.ParseIP validation for node PrivateIP addresses before they are used in grep commands to identify nodes

Copilot · 2026-02-12T20:34:26Z

api/holodeck/v1alpha1/validation.go

+func validateLabels(labels map[string]string) error {
+	for k, v := range labels {
+		if !k8sLabelPattern.MatchString(k) {
+			return fmt.Errorf("invalid label key %q: contains disallowed characters", k)
+		}
+		if v != "" && !k8sLabelPattern.MatchString(v) {
+			return fmt.Errorf("invalid label value %q for key %q: contains disallowed characters", v, k)
+		}
+	}
+	return nil
+}


Test coverage is missing for the new label validation security feature. The existing test file has comprehensive tests for ClusterSpec validation, but no tests verify that malicious label keys/values (containing shell metacharacters like backticks, semicolons, pipes, or dollar signs) are properly rejected. Given this is a security fix for command injection, test cases demonstrating rejection of dangerous inputs are critical.

Copilot · 2026-02-12T20:34:26Z

api/holodeck/v1alpha1/validation.go

+	// Validate labels for shell-injection safety
+	if err := validateLabels(c.ControlPlane.Labels); err != nil {
+		return fmt.Errorf("control-plane labels: %w", err)
+	}
+	if c.Workers != nil {
+		if err := validateLabels(c.Workers.Labels); err != nil {
+			return fmt.Errorf("worker labels: %w", err)
+		}
+	}


The validation function is defined but never called anywhere in the codebase. This means the label validation will not actually prevent command injection attacks. The ClusterSpec.Validate() method needs to be called when loading the Environment YAML configuration, before the cluster provisioning begins. Without this, malicious label values can still be interpolated into shell commands at lines 484 and 520 of pkg/provisioner/cluster.go, leading to command injection.

Copilot · 2026-02-12T20:34:27Z

pkg/provisioner/cluster.go


+	// Validate all node IPs before interpolating into shell commands
+	for _, node := range nodes {
+		if net.ParseIP(node.PrivateIP) == nil {


While net.ParseIP correctly validates IP addresses, IPv6 addresses contain colons which can interfere with the grep command on lines 479 and 512. For example, an IPv6 address like "2001:db8::1" would be interpolated as grep '2001:db8::1' which could produce unexpected grep behavior. Consider using grep -F (fixed string matching) instead of plain grep to ensure the IP is treated as a literal string, or use a more robust node identification method like matching against the node's hostname or a unique identifier.

Suggested change

if net.ParseIP(node.PrivateIP) == nil {

ip := net.ParseIP(node.PrivateIP)

// Restrict to IPv4 addresses to avoid issues when interpolating into shell/grep

if ip == nil || ip.To4() == nil {

Copilot · 2026-02-12T20:34:27Z

pkg/provisioner/cluster.go

+	// Validate all node IPs before interpolating into shell commands
+	for _, node := range nodes {
+		if net.ParseIP(node.PrivateIP) == nil {
+			return fmt.Errorf("invalid private IP for node %s: %q", node.Name, node.PrivateIP)
+		}
+	}


Test coverage is missing for the new IP validation security feature. No tests verify that invalid IP addresses (like those containing shell metacharacters or malformed IPs) are properly rejected before being interpolated into grep commands. Add test cases for configureNodes that verify rejection of invalid IPs such as empty strings, malformed addresses, and strings containing shell special characters.

Copilot · 2026-02-12T20:34:27Z

api/holodeck/v1alpha1/validation.go

+		if !k8sLabelPattern.MatchString(k) {
+			return fmt.Errorf("invalid label key %q: contains disallowed characters", k)
+		}
+		if v != "" && !k8sLabelPattern.MatchString(v) {


Empty label values are allowed by the validation (line 31 checks v != ""), but they're interpolated directly into kubectl commands without proper handling. When a label has an empty value, the command becomes kubectl label node NAME key= which is valid kubectl syntax. However, if the validation regex has issues (as noted in other comments), an attacker could potentially use an empty string key with a crafted value to bypass validation. Ensure the validation is robust enough that empty values cannot be exploited.

Suggested change

if v != "" && !k8sLabelPattern.MatchString(v) {

if v == "" {

return fmt.Errorf("invalid label value for key %q: value must not be empty", k)

}

if !k8sLabelPattern.MatchString(v) {

Copilot · 2026-02-12T20:34:28Z

api/holodeck/v1alpha1/validation.go

+	"regexp"
 )

+var k8sLabelPattern = regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9._\-/]*[a-zA-Z0-9])?$`)
+
+func validateLabels(labels map[string]string) error {
+	for k, v := range labels {
+		if !k8sLabelPattern.MatchString(k) {
+			return fmt.Errorf("invalid label key %q: contains disallowed characters", k)
+		}
+		if v != "" && !k8sLabelPattern.MatchString(v) {
+			return fmt.Errorf("invalid label value %q for key %q: contains disallowed characters", v, k)
+		}


The label validation regex is incomplete and doesn't fully comply with Kubernetes label requirements. Key issues:

Missing length validation: label names must be at most 63 characters, and prefixes (before the '/') must be at most 253 characters

The regex allows multiple '/' characters anywhere in the string, but Kubernetes only allows a single '/' separator between an optional prefix and the name

Label keys with prefixes should have the prefix part (before '/') follow DNS subdomain rules (lowercase only, dots and hyphens allowed)

Empty label keys should be rejected (the regex currently allows them)

Consider implementing proper validation that:

Splits on '/' to validate prefix and name separately

Validates prefix as a DNS subdomain (lowercase alphanumeric, dots, hyphens, max 253 chars)

Validates name part (alphanumeric start/end, max 63 chars)

Validates values similarly to names (max 63 chars, can be empty)

Suggested change

"regexp"

)

var k8sLabelPattern = regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9._\-/]*[a-zA-Z0-9])?$`)

func validateLabels(labels map[string]string) error {

for k, v := range labels {

if !k8sLabelPattern.MatchString(k) {

return fmt.Errorf("invalid label key %q: contains disallowed characters", k)

}

if v != "" && !k8sLabelPattern.MatchString(v) {

return fmt.Errorf("invalid label value %q for key %q: contains disallowed characters", v, k)

}

"strings"

"k8s.io/apimachinery/pkg/util/validation"

)

func validateLabels(labels map[string]string) error {

for k, v := range labels {

if k == "" {

return fmt.Errorf("invalid label key: key must not be empty")

}

if errs := validation.IsQualifiedName(k); len(errs) > 0 {

return fmt.Errorf("invalid label key %q: %s", k, strings.Join(errs, "; "))

}

if v == "" {

// Empty label values are allowed by Kubernetes.

continue

}

if errs := validation.IsValidLabelValue(v); len(errs) > 0 {

return fmt.Errorf("invalid label value %q for key %q: %s", v, k, strings.Join(errs, "; "))

}

Copilot · 2026-02-12T20:34:28Z

api/holodeck/v1alpha1/validation.go

+var k8sLabelPattern = regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9._\-/]*[a-zA-Z0-9])?$`)
+
+func validateLabels(labels map[string]string) error {
+	for k, v := range labels {


The validation should reject empty label keys explicitly. An empty string would match the regex pattern due to the '?' quantifier making the entire capture group optional, which could lead to kubectl commands with malformed label syntax.

Suggested change

for k, v := range labels {

for k, v := range labels {

if k == "" {

return fmt.Errorf("invalid label key: key must not be empty")

}

Copilot AI review requested due to automatic review settings February 12, 2026 20:28

Copilot started reviewing on behalf of ArangoGutierrez February 12, 2026 20:28 View session

Copilot AI reviewed Feb 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(security): validate node labels and IPs before shell interpolation #656

fix(security): validate node labels and IPs before shell interpolation #656

Uh oh!

ArangoGutierrez commented Feb 12, 2026

Uh oh!

coveralls commented Feb 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Copilot AI Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-		if net.ParseIP(node.PrivateIP) == nil {
+		ip := net.ParseIP(node.PrivateIP)
+		// Restrict to IPv4 addresses to avoid issues when interpolating into shell/grep
+		if ip == nil || ip.To4() == nil {

-		if v != "" && !k8sLabelPattern.MatchString(v) {
+		if v == "" {
+			return fmt.Errorf("invalid label value for key %q: value must not be empty", k)
+		}
+		if !k8sLabelPattern.MatchString(v) {

fix(security): validate node labels and IPs before shell interpolation #656

Are you sure you want to change the base?

fix(security): validate node labels and IPs before shell interpolation #656

Uh oh!

Conversation

ArangoGutierrez commented Feb 12, 2026

Summary

Audit Findings

Changes

Test plan

Uh oh!

coveralls commented Feb 12, 2026

Pull Request Test Coverage Report for Build 21963082665

Details

💛 - Coveralls

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants