fix: Phase 1 — Security, concurrency & provisioner resilience + E2E tests#623
Merged
ArangoGutierrez merged 1 commit intoFeb 9, 2026
Conversation
There was a problem hiding this comment.
Pull request overview
Phase 1 hardening pass focused on safer template execution, more resilient provisioning behavior, improved logger concurrency behavior, and adding a new E2E test suite for AWS-backed environments.
Changes:
- Added Ginkgo/Gomega E2E coverage for AWS lifecycle + provisioning (containerd/kubeadm and docker/NVIDIA runtime scenarios).
- Added template input validation helpers + tests to reduce shell injection risk; improved retry behavior in provisioning templates.
- Improved error wrapping (
%w) broadly; updated provisioner streaming to avoid goroutine leaks / fatal exits.
Reviewed changes
Copilot reviewed 24 out of 639 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e_test.go | Adds Ginkgo suite bootstrap + shared env initialization for E2E. |
| tests/aws_test.go | Implements AWS E2E table tests that create, provision, and clean up environments. |
| tests/common/common.go | Adds helper for per-test unique IDs to avoid name collisions. |
| tests/data/test_aws.yml | Adds E2E AWS env spec for containerd + kubeadm scenario. |
| tests/data/test_aws_docker.yml | Adds E2E AWS env spec for docker + NVIDIA toolkit scenario. |
| pkg/provisioner/templates/validate.go | Introduces input validators (versions/arch/host/etc.) for safer template interpolation. |
| pkg/provisioner/templates/validate_test.go | Adds unit tests to validate injection-resistant behavior. |
| pkg/provisioner/templates/kubernetes.go | Validates Kubernetes template inputs and wraps template execution errors. |
| pkg/provisioner/templates/docker.go | Adds retry for apt update; validates Docker template inputs; wraps errors. |
| pkg/provisioner/templates/crio.go | Adds version validation and wraps template execution errors. |
| pkg/provisioner/templates/containerd.go | Improves apt retry usage; supports “latest” containerd behavior; minor template hardening. |
| pkg/provisioner/templates/container-toolkit.go | Fixes template execution flow; adds runtime validation; adds retry-install behavior. |
| pkg/provisioner/templates/common.go | Makes package install more resilient (retries, dpkg repair, apt refresh). |
| pkg/provisioner/templates/nv-driver.go | Improves NVIDIA driver install resilience with arch detection + fallback chain. |
| pkg/provisioner/provisioner.go | Improves SSH session output streaming and error wrapping; avoids fatal exit in goroutine. |
| internal/logger/logger.go | Adds context-aware loading indicator and prevents double-close panic in Exit. |
| internal/logger/logger_test.go | Adds unit tests for logger edge cases + goroutine exit behaviors. |
| pkg/provider/aws/create.go | Switches many provider errors to %w wrapping for better error chains. |
| pkg/provider/aws/dryrun.go | Wraps dry-run errors with %w. |
| pkg/provider/aws/delete.go | Increases termination waiter timeout. |
| cmd/cli/create/create.go | Wraps returned errors with %w for better higher-level error inspection. |
| cmd/cli/dryrun/dryrun.go | Wraps SSH key read/parse errors with %w. |
| cmd/action/ci/entrypoint.go | Wraps errors with %w for CI action entrypoint reliability. |
| go.mod | Updates Go version and adds Ginkgo/Gomega dependencies for E2E. |
Comments suppressed due to low confidence (1)
pkg/provisioner/templates/common.go:30
- Setting
APT::Get::AllowUnauthenticated 1globally weakens package authenticity checks for all subsequent installs, which is a significant security regression (especially in a PR focused on hardening). Prefer removing this entirely and fixing the underlying key/repo setup; if it must exist for a narrow case, scope it to a single command and immediately undo it afterwards.
echo "APT::Get::AllowUnauthenticated 1;" | sudo tee /etc/apt/apt.conf.d/99allow-unauthenticated
Comment on lines
+36
to
+41
| func TestMain(t *testing.T) { | ||
| suiteName := "E2E Holodeck" | ||
|
|
||
| RegisterFailHandler(Fail) | ||
| RunSpecs(t, suiteName) | ||
| } |
There was a problem hiding this comment.
TestMain is a special name in Go testing when the signature is func TestMain(m *testing.M). Here it's a normal test function, which can be confusing and makes it harder to add a real TestMain later. Consider renaming this to something like TestE2E / TestE2ESuite.
Comment on lines
+115
to
+126
| defer func() { | ||
| if p.Client != nil { | ||
| session, err := p.Client.NewSession() | ||
| if err == nil { | ||
| session.Close() | ||
| if err := p.Client.Close(); err != nil { | ||
| Expect(err).NotTo(HaveOccurred(), "Failed to close ssh client") | ||
| } | ||
| } | ||
| p.Client = nil | ||
| } | ||
| }() |
There was a problem hiding this comment.
If p.Client.NewSession() fails, the SSH client is never closed, leaking the underlying connection. Also, creating a session just to close the client is unnecessary—closing the client should be unconditional when non-nil.
Comment on lines
214
to
222
| localFile, err := os.Open(env.Spec.Kubernetes.KindConfig) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to open local file %s: %v", env.Spec.Kubernetes.KindConfig, err) | ||
| return fmt.Errorf("failed to open local file %s: %w", env.Spec.Kubernetes.KindConfig, err) | ||
| } | ||
|
|
||
| // copy local file to remote file | ||
| if _, err := io.Copy(remoteFile, localFile); err != nil { | ||
| return fmt.Errorf("failed to copy local file %s to remote file %s: %v", env.Spec.Kubernetes.KindConfig, remoteFilePath, err) | ||
| return fmt.Errorf("failed to copy local file %s to remote file %s: %w", env.Spec.Kubernetes.KindConfig, remoteFilePath, err) | ||
| } |
There was a problem hiding this comment.
localFile is never closed, which can leak file descriptors. Add a defer localFile.Close() after the successful os.Open(...).
| containerRuntimePattern = regexp.MustCompile(`^(docker|containerd|crio|cri-o)$`) | ||
|
|
||
| // filePathPattern matches safe file paths (no shell metacharacters) | ||
| filePathPattern = regexp.MustCompile(`^[a-zA-Z0-9_./$~-]+$`) |
There was a problem hiding this comment.
The comment says "no shell metacharacters", but the pattern allows $, which is a shell metacharacter and enables variable expansion. Either (a) remove $ from the allowed set, or (b) ensure every use of the path in templates is safely single-quoted/escaped and update the comment to match the intended threat model.
Suggested change
| filePathPattern = regexp.MustCompile(`^[a-zA-Z0-9_./$~-]+$`) | |
| filePathPattern = regexp.MustCompile(`^[a-zA-Z0-9_./~-]+$`) |
Comment on lines
+225
to
+245
| ticker := time.After(330 * time.Millisecond) | ||
| i := 0 | ||
| spinners := []string{"|", "/", "-", "\\"} | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| fmt.Print("\r\033[2K") | ||
| printMessage(green, checkmark, message) | ||
| return | ||
| case <-failCh: | ||
| fmt.Print("\r\033[2K") | ||
| printMessage(redText, redXEmoji, message) | ||
| return | ||
| case <-ticker: | ||
| i++ | ||
| fmt.Printf("\r%s\t%s", spinners[i], message) | ||
| if i >= len(spinners)-1 { | ||
| i = 0 | ||
| } | ||
| ticker = time.After(330 * time.Millisecond) |
There was a problem hiding this comment.
This re-allocates a new time.After(...) timer on every tick, which is unnecessary overhead and can increase GC pressure during long-running operations. Use a time.NewTicker(...) and defer ticker.Stop(). Also consider writing to l.Out (or routing through a writer-aware helper) instead of fmt.Print/Printf to keep logger output consistent/testable.
Suggested change
| ticker := time.After(330 * time.Millisecond) | |
| i := 0 | |
| spinners := []string{"|", "/", "-", "\\"} | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| fmt.Print("\r\033[2K") | |
| printMessage(green, checkmark, message) | |
| return | |
| case <-failCh: | |
| fmt.Print("\r\033[2K") | |
| printMessage(redText, redXEmoji, message) | |
| return | |
| case <-ticker: | |
| i++ | |
| fmt.Printf("\r%s\t%s", spinners[i], message) | |
| if i >= len(spinners)-1 { | |
| i = 0 | |
| } | |
| ticker = time.After(330 * time.Millisecond) | |
| ticker := time.NewTicker(330 * time.Millisecond) | |
| defer ticker.Stop() | |
| i := 0 | |
| spinners := []string{"|", "/", "-", "\\"} | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| fmt.Fprint(outFile, "\r\033[2K") | |
| printMessage(green, checkmark, message) | |
| return | |
| case <-failCh: | |
| fmt.Fprint(outFile, "\r\033[2K") | |
| printMessage(redText, redXEmoji, message) | |
| return | |
| case <-ticker.C: | |
| i++ | |
| fmt.Fprintf(outFile, "\r%s\t%s", spinners[i], message) | |
| if i >= len(spinners)-1 { | |
| i = 0 | |
| } |
Comment on lines
+240
to
+244
| i++ | ||
| fmt.Printf("\r%s\t%s", spinners[i], message) | ||
| if i >= len(spinners)-1 { | ||
| i = 0 | ||
| } |
There was a problem hiding this comment.
Spinner indexing skips spinners[0] on the first tick because i is incremented before indexing. Adjust the increment/mod logic so the sequence starts at index 0 and never risks out-of-range if this logic changes later.
Suggested change
| i++ | |
| fmt.Printf("\r%s\t%s", spinners[i], message) | |
| if i >= len(spinners)-1 { | |
| i = 0 | |
| } | |
| fmt.Printf("\r%s\t%s", spinners[i%len(spinners)], message) | |
| i++ |
Comment on lines
68
to
71
| }, 10*time.Minute, waiterOptions...); err != nil { | ||
| a.fail() | ||
| return fmt.Errorf("error waiting for instance to be terminated: %v", err) | ||
| } |
There was a problem hiding this comment.
This return path still uses %v instead of %w, which breaks error unwrapping and is inconsistent with the rest of the PR’s error-wrapping changes. Prefer %w here.
Comment on lines
+22
to
+31
| "log" | ||
| ) | ||
|
|
||
| func GenerateUID() string { | ||
| b := make([]byte, 4) | ||
| _, err := rand.Read(b) | ||
| if err != nil { | ||
| log.Fatalf("failed to generate secure random UID: %v", err) | ||
| } | ||
| return hex.EncodeToString(b) |
There was a problem hiding this comment.
A 4-byte random value has a non-trivial collision risk across many E2E runs (especially in parallel). Consider increasing to 16 bytes (or at least 8) and returning (string, error) instead of calling log.Fatalf, so the caller/test can decide how to fail.
Suggested change
| "log" | |
| ) | |
| func GenerateUID() string { | |
| b := make([]byte, 4) | |
| _, err := rand.Read(b) | |
| if err != nil { | |
| log.Fatalf("failed to generate secure random UID: %v", err) | |
| } | |
| return hex.EncodeToString(b) | |
| "fmt" | |
| ) | |
| func GenerateUID() (string, error) { | |
| b := make([]byte, 16) | |
| if _, err := rand.Read(b); err != nil { | |
| return "", fmt.Errorf("failed to generate secure random UID: %w", err) | |
| } | |
| return hex.EncodeToString(b), nil |
| privateKey: /home/runner/.cache/key | ||
| instance: | ||
| type: g4dn.xlarge | ||
| region: us-west-1 |
There was a problem hiding this comment.
Using 0.0.0.0/0 for ingress in E2E test configs exposes SSH (and any opened ports) to the internet during tests. Prefer restricting ingress to the runner’s public IP/CIDR (for example, injected via CI), or at least documenting that this file should only be used in tightly scoped test accounts with short-lived credentials.
Suggested change
| region: us-west-1 | |
| region: us-west-1 | |
| # NOTE: 0.0.0.0/0 is used here only for E2E testing. | |
| # This configuration must only be used in tightly scoped test accounts | |
| # with short-lived, non-production credentials and isolated resources. | |
| # For CI, prefer restricting ingress to the runner's public IP/CIDR | |
| # injected via the pipeline instead of exposing ports to the internet. |
ArangoGutierrez
force-pushed
the
fix/phase1-security-concurrency
branch
from
February 9, 2026 13:28
6c72936 to
8a7fefa
Compare
ArangoGutierrez
enabled auto-merge (squash)
February 9, 2026 13:29
Pull Request Test Coverage Report for Build 21830773231Details
💛 - Coveralls |
ArangoGutierrez
force-pushed
the
fix/phase1-security-concurrency
branch
from
February 9, 2026 13:39
8a7fefa to
045ad1d
Compare
…ping - Add template input validation to prevent command injection via shell metacharacters in version strings, git URLs, and user-supplied fields - Replace fmt.Errorf %v with %w across non-vendor packages for proper error chain propagation enabling errors.Is/errors.As usage Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
force-pushed
the
fix/phase1-security-concurrency
branch
from
February 9, 2026 15:17
045ad1d to
fba9ce3
Compare
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 10, 2026
…ping (NVIDIA#623) - Add template input validation to prevent command injection via shell metacharacters in version strings, git URLs, and user-supplied fields - Replace fmt.Errorf %v with %w across non-vendor packages for proper error chain propagation enabling errors.Is/errors.As usage Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 10, 2026
…ping (NVIDIA#623) - Add template input validation to prevent command injection via shell metacharacters in version strings, git URLs, and user-supplied fields - Replace fmt.Errorf %v with %w across non-vendor packages for proper error chain propagation enabling errors.Is/errors.As usage Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 10, 2026
…ping (NVIDIA#623) - Add template input validation to prevent command injection via shell metacharacters in version strings, git URLs, and user-supplied fields - Replace fmt.Errorf %v with %w across non-vendor packages for proper error chain propagation enabling errors.Is/errors.As usage Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 12, 2026
…ping (NVIDIA#623) - Add template input validation to prevent command injection via shell metacharacters in version strings, git URLs, and user-supplied fields - Replace fmt.Errorf %v with %w across non-vendor packages for proper error chain propagation enabling errors.Is/errors.As usage Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 13, 2026
…ping (NVIDIA#623) - Add template input validation to prevent command injection via shell metacharacters in version strings, git URLs, and user-supplied fields - Replace fmt.Errorf %v with %w across non-vendor packages for proper error chain propagation enabling errors.Is/errors.As usage Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
ArangoGutierrez
added a commit
to ArangoGutierrez/holodeck
that referenced
this pull request
Feb 13, 2026
…ping (NVIDIA#623) - Add template input validation to prevent command injection via shell metacharacters in version strings, git URLs, and user-supplied fields - Replace fmt.Errorf %v with %w across non-vendor packages for proper error chain propagation enabling errors.Is/errors.As usage Re-implemented against current upstream/main. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 of the 360-degree codebase evaluation. Addresses security hardening, concurrency improvements, and provisioner resilience.
Security & Concurrency Fixes
%vwith%wacross error returns for proper error chain propagationProvisioner Resilience
E2E Test Suite (NEW)
Test Plan
go vet ./...passes-s -S(DCO + SSH)Made with Cursor