fix(templates): validate feature gates, track branches, and endpoint host#647
Conversation
…host These fields are interpolated into shell scripts but were not covered by ValidateTemplateInputs(). Add featureGatePattern for K8sFeatureGates, reuse gitRefPattern for Latest.Track branches (K8s + CTK), and add hostnamePattern for K8sEndpointHost. Audit findings NVIDIA#16 (MEDIUM), NVIDIA#32 (LOW), NVIDIA#33 (LOW). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens Holodeck’s provisioner template input validation to prevent shell command injection in user-controlled fields that are interpolated into generated shell scripts.
Changes:
- Add
featureGatePatternvalidation forK8sFeatureGatesentries (FeatureName=true|false). - Validate
Latest.Trackbranch names (Kubernetes + NVIDIA Container Toolkit) using the existinggitRefPattern. - Add
hostnamePatternvalidation forK8sEndpointHostand expand unit/integration coverage around these validations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/provisioner/templates/validate.go | Adds new regex patterns and enforces validation for feature gates, track branches, and endpoint host. |
| pkg/provisioner/templates/validate_test.go | Adds unit tests for new regexes and integration tests ensuring ValidateTemplateInputs rejects injection attempts. |
| accept := []string{"host.example.com", "192.168.1.1", "k8s-api:6443", "my-host"} | ||
| reject := []string{ | ||
| "host.com; rm -rf /", | ||
| "$(curl evil)", | ||
| "host && bad", | ||
| "host`id`", | ||
| "; echo pwned", |
There was a problem hiding this comment.
TestHostnamePattern treats k8s-api:6443 as a valid K8sEndpointHost, but the kubeadm templates append :6443 to the value, so accepting host:port can lead to malformed endpoints (...:6443:6443). Adjust the accepted cases to match the intended contract (host-only) or update the implementation to support optional ports consistently.
| accept := []string{"host.example.com", "192.168.1.1", "k8s-api:6443", "my-host"} | |
| reject := []string{ | |
| "host.com; rm -rf /", | |
| "$(curl evil)", | |
| "host && bad", | |
| "host`id`", | |
| "; echo pwned", | |
| accept := []string{"host.example.com", "192.168.1.1", "my-host"} | |
| reject := []string{ | |
| "host.com; rm -rf /", | |
| "$(curl evil)", | |
| "host && bad", | |
| "host`id`", | |
| "; echo pwned", | |
| "k8s-api:6443", |
| @@ -114,6 +120,32 @@ func ValidateTemplateInputs(env v1alpha1.Environment) error { | |||
| } | |||
| } | |||
|
|
|||
| // Validate track branches (same rules as git refs) | |||
| if env.Spec.Kubernetes.Latest != nil && env.Spec.Kubernetes.Latest.Track != "" { | |||
| if !gitRefPattern.MatchString(env.Spec.Kubernetes.Latest.Track) { | |||
| return fmt.Errorf("invalid kubernetes latest track branch: %q contains disallowed characters", env.Spec.Kubernetes.Latest.Track) | |||
| } | |||
| } | |||
| if env.Spec.NVIDIAContainerToolkit.Latest != nil && env.Spec.NVIDIAContainerToolkit.Latest.Track != "" { | |||
| if !gitRefPattern.MatchString(env.Spec.NVIDIAContainerToolkit.Latest.Track) { | |||
| return fmt.Errorf("invalid nvidia container toolkit latest track branch: %q contains disallowed characters", env.Spec.NVIDIAContainerToolkit.Latest.Track) | |||
| } | |||
| } | |||
|
|
|||
| // Validate feature gates | |||
| for _, gate := range env.Spec.Kubernetes.K8sFeatureGates { | |||
| if !featureGatePattern.MatchString(gate) { | |||
| return fmt.Errorf("invalid kubernetes feature gate: %q must match FeatureName=true|false", gate) | |||
| } | |||
| } | |||
|
|
|||
| // Validate endpoint host | |||
| if env.Spec.Kubernetes.K8sEndpointHost != "" { | |||
| if !hostnamePattern.MatchString(env.Spec.Kubernetes.K8sEndpointHost) { | |||
| return fmt.Errorf("invalid kubernetes endpoint host: %q contains disallowed characters", env.Spec.Kubernetes.K8sEndpointHost) | |||
| } | |||
There was a problem hiding this comment.
hostnamePattern currently allows : and the hostname tests accept values like k8s-api:6443, but the Kubernetes templates append :6443 when using K8sEndpointHost (e.g., --control-plane-endpoint="${K8S_ENDPOINT_HOST}:6443" and controlPlaneEndpoint: "{{.ControlPlaneEndpoint}}:6443"). This combination can produce invalid endpoints like k8s-api:6443:6443 if a user includes a port. Consider either tightening validation to reject host:port (no : allowed), or updating the templates/templating data model to treat this field as host[:port] and only append the default port when none is provided.
Pull Request Test Coverage Report for Build 21961704208Details
💛 - Coveralls |
Summary
featureGatePatternregex to validateK8sFeatureGatesentries matchFeatureName=true|falsegitRefPatternto validateLatest.Trackbranches for both Kubernetes and NVIDIA Container ToolkithostnamePatternregex to validateK8sEndpointHostagainst shell injectionThese fields are interpolated into shell scripts generated by the provisioner templates but were not previously covered by
ValidateTemplateInputs(), allowing potential command injection via shell metacharacters.Audit Findings
Test Plan
featureGatePattern(accepts valid gates, rejects injection)hostnamePattern(accepts valid hosts/IPs, rejects injection)ValidateTemplateInputsrejects shell injection in feature gatesValidateTemplateInputsrejects shell injection in K8s track branchValidateTemplateInputsrejects shell injection in CTK track branchValidateTemplateInputsrejects shell injection in endpoint hostgofmt,golangci-lint,go build,go mod tidy && go mod verifyall pass