Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ArangoGutierrez wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions api/holodeck/v1alpha1/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,23 @@ package v1alpha1

import (
"fmt"
"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 {
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
if !k8sLabelPattern.MatchString(k) {
return fmt.Errorf("invalid label key %q: contains disallowed characters", k)
}
if v != "" && !k8sLabelPattern.MatchString(v) {
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
return fmt.Errorf("invalid label value %q for key %q: contains disallowed characters", v, k)
}
Comment on lines +21 to +33
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

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

  1. Missing length validation: label names must be at most 63 characters, and prefixes (before the '/') must be at most 253 characters
  2. The regex allows multiple '/' characters anywhere in the string, but Kubernetes only allows a single '/' separator between an optional prefix and the name
  3. Label keys with prefixes should have the prefix part (before '/') follow DNS subdomain rules (lowercase only, dots and hyphens allowed)
  4. 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 uses AI. Check for mistakes.
}
return nil
}
Comment on lines +26 to +36
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

// Validate validates the ClusterSpec configuration.
func (c *ClusterSpec) Validate() error {
if c == nil {
Expand All @@ -43,6 +58,16 @@ func (c *ClusterSpec) Validate() error {
}
}

// 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)
}
}
Comment on lines +61 to +69
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

// Validate HA configuration
if c.HighAvailability != nil {
if err := c.HighAvailability.Validate(c.ControlPlane.Count); err != nil {
Expand Down
8 changes: 8 additions & 0 deletions pkg/provisioner/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"bytes"
"fmt"
"io"
"net"
"os"
"strings"
"sync"
Expand Down Expand Up @@ -437,6 +438,13 @@ func (cp *ClusterProvisioner) configureNodes(firstCP NodeInfo, nodes []NodeInfo)
}
defer provisioner.Client.Close() // nolint: errcheck

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

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
return fmt.Errorf("invalid private IP for node %s: %q", node.Name, node.PrivateIP)
}
}
Comment on lines +441 to +446
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

// Build the node configuration script
// Note: Use sudo -E to preserve KUBECONFIG environment variable, or use --kubeconfig flag
var script strings.Builder
Expand Down
Loading