feat: create general purpose toolbox CLI by khuedoan · Pull Request #1 · khuedoan/cloudlab

khuedoan · 2026-01-17T03:38:50Z

No description provided.

Copilot

Pull request overview

This PR introduces a new general-purpose CLI tool called "toolbox" for managing infrastructure, specifically focused on managing secrets in HashiCorp Vault through SSH connections to Kubernetes clusters. The tool provides SSH tunneling, port-forwarding capabilities, and automated secret generation.

Changes:

Created a new Go CLI tool with Cobra framework for managing Vault secrets
Implemented SSH connection management with port-forwarding to Kubernetes services
Added support for generating random strings, SSH keypairs (ed25519/RSA), and manual secret entry

Reviewed changes

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

Show a summary per file

File	Description
toolbox/go.mod	Defines Go module with dependencies for CLI, Vault API, and SSH libraries
toolbox/go.sum	Dependency checksums for reproducible builds
toolbox/main.go	Main entry point that delegates to the cmd package
toolbox/cmd/root.go	Root CLI command definition with persistent flags for SSH connection
toolbox/cmd/secrets.go	Secrets management command with support for multiple secret types
toolbox/internal/cluster/cluster.go	SSH connection and port-forwarding implementation
toolbox/internal/cluster/client.go	High-level cluster client with Vault integration
toolbox/Makefile	Build and test targets for the toolbox
settings.yaml	Example configuration file demonstrating secret types
flake.nix	Nix flake configuration to build and package the toolbox

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

Copilot · 2026-01-23T16:22:45Z

toolbox/internal/cluster/cluster.go

+func waitForService(ctx context.Context, addr string) error {
+	client := &http.Client{Timeout: 2 * time.Second}
+	url := "http://" + addr
+
+	for {
+		select {
+		case <-ctx.Done():
+			return ctx.Err()
+		default:
+		}
+
+		resp, err := client.Get(url)
+		if err == nil {
+			resp.Body.Close()
+			return nil
+		}
+
+		// Also try TCP connect for non-HTTP services
+		conn, err := net.DialTimeout("tcp", addr, 2*time.Second)
+		if err == nil {
+			conn.Close()
+			return nil
+		}
+
+		time.Sleep(healthCheckInterval)
+	}


The waitForService function can loop indefinitely if the service never becomes available and the context is never cancelled. Consider adding a maximum timeout or retry limit to prevent indefinite blocking.

Copilot · 2026-01-23T16:22:45Z

toolbox/internal/cluster/cluster.go

+		_ = t.session.Signal(ssh.SIGTERM)
+		t.session.Close()


The session.Close() error is not captured or returned. While this is acceptable in cleanup code where you want to continue closing other resources, consider using errors.Join to accumulate errors from both Signal and Close for better error reporting.

Suggested change

_ = t.session.Signal(ssh.SIGTERM)

t.session.Close()

if err := t.session.Signal(ssh.SIGTERM); err != nil {

errs = append(errs, fmt.Errorf("signal session: %w", err))

}

if err := t.session.Close(); err != nil {

errs = append(errs, fmt.Errorf("close session: %w", err))

}

Copilot · 2026-01-23T16:22:45Z

toolbox/cmd/secrets.go

+		algorithm := e.settings.Algorithm
+		if algorithm == "" {
+			algorithm = "ed25519"
+		}


The default SSH key algorithm is "ed25519" with a fallback to "rsa" using 4096 bits. While these are secure choices, consider documenting these defaults in the settings.yaml example or in code comments to help users understand what algorithms are available and recommended.

Copilot · 2026-01-23T16:22:46Z

toolbox/internal/cluster/cluster.go

+		if err != nil {
+			if errors.Is(err, net.ErrClosed) {
+				return
+			}
+			continue


Errors during tunnel connection handling are silently ignored. When Accept() fails (other than net.ErrClosed), the error is discarded with 'continue'. This makes it difficult to diagnose connection issues. Consider logging these errors for operational visibility.

Copilot · 2026-01-23T16:22:46Z

toolbox/internal/cluster/client.go

+func getVaultToken(conn *Connector) (string, error) {
+	cmd := fmt.Sprintf(
+		`kubectl get secret vault-unseal-keys -n %s -o template='{{ index .data "vault-root" }}'`,
+		vaultNamespace,
+	)
+
+	output, err := conn.RunCommand(cmd)
+	if err != nil {
+		return "", err
+	}
+
+	token, err := base64.StdEncoding.DecodeString(strings.TrimSpace(string(output)))
+	if err != nil {
+		return "", fmt.Errorf("decode token: %w", err)
+	}
+
+	return string(token), nil


The Vault root token is retrieved from a Kubernetes secret and used directly. This is a highly sensitive operation. Consider documenting that this command should only be used in trusted environments, and that proper RBAC should be enforced to prevent unauthorized access to the vault-unseal-keys secret.

Copilot · 2026-01-23T16:22:50Z

toolbox/internal/cluster/cluster.go

+
+		resp, err := client.Get(url)
+		if err == nil {
+			resp.Body.Close()


HTTP response bodies are closed immediately without reading the body. While this is correct for health checks, you should defer the Close call and check its error to ensure proper resource cleanup, even though in practice the error is unlikely to matter here.

Suggested change

resp.Body.Close()

if resp.Body != nil {

defer func() {

if cerr := resp.Body.Close(); cerr != nil {

// ignore close error for health check

}

}()

}

Copilot · 2026-01-23T16:22:50Z

toolbox/cmd/secrets.go

+	randomBytes := make([]byte, length)
+
+	if _, err := rand.Read(randomBytes); err != nil {
+		return "", err
+	}
+
+	for i, b := range randomBytes {
+		result[i] = charset[int(b)%len(charset)]


The generateRandomString function uses modulo bias when selecting characters from the charset. Using int(b)%len(charset) introduces a slight bias toward certain characters. For cryptographic purposes, consider using a rejection sampling approach or crypto/rand's Read with proper bounds checking to ensure uniform distribution.

Suggested change

randomBytes := make([]byte, length)

if _, err := rand.Read(randomBytes); err != nil {

return "", err

}

for i, b := range randomBytes {

result[i] = charset[int(b)%len(charset)]

charsetLen := len(charset)

if charsetLen == 0 {

return "", fmt.Errorf("charset is empty")

}

// Use rejection sampling to avoid modulo bias when mapping random bytes to charset indices.

maxMultiple := 256 / charsetLen * charsetLen

for i := 0; i < length; i++ {

for {

var b [1]byte

if _, err := rand.Read(b[:]); err != nil {

return "", err

}

if int(b[0]) >= maxMultiple {

// Discard values that would introduce modulo bias.

continue

}

result[i] = charset[int(b[0])%charsetLen]

break

}

Copilot · 2026-01-23T16:22:51Z

toolbox/internal/cluster/cluster.go

+	done := make(chan struct{}, 2)
+
+	go func() {
+		io.Copy(remoteConn, localConn)
+		done <- struct{}{}
+	}()
+
+	go func() {
+		io.Copy(localConn, remoteConn)
+		done <- struct{}{}
+	}()
+
+	<-done
+}


The two io.Copy goroutines can potentially leak if one completes while the other is blocked. After receiving from the done channel once, the function returns immediately, potentially leaving the other goroutine running. Consider using a sync.WaitGroup or closing both connections to ensure both goroutines terminate.

Copilot · 2026-01-23T16:22:51Z

toolbox/internal/cluster/cluster.go

+		Auth: []ssh.AuthMethod{
+			ssh.PublicKeys(signer),
+		},
+		HostKeyCallback: ssh.InsecureIgnoreHostKey(),


Using ssh.InsecureIgnoreHostKey() disables host key verification, making the connection vulnerable to man-in-the-middle attacks. Consider implementing proper host key verification using ssh.FixedHostKey() with a known host key, or at least document why this security compromise is acceptable for this use case.

Copilot · 2026-01-23T16:22:51Z

toolbox/internal/cluster/cluster.go

+	go func() {
+		io.Copy(remoteConn, localConn)
+		done <- struct{}{}
+	}()
+
+	go func() {
+		io.Copy(localConn, remoteConn)
+		done <- struct{}{}
+	}()


Errors from io.Copy operations are ignored. If data copying between local and remote connections fails, these errors are silently discarded. This can hide network issues or connection problems. Consider logging these errors for better operational visibility and debugging.

khuedoan added 5 commits January 14, 2026 09:20

feat: create toolbox CLI

aa1da52

refactor(toolbox): split cluster connection logic

aeb2c54

refactor(toolbox): restructure commands

d2fecbb

refactor(toolbox): new contract for SSH key generation

74eb623

refactor(toolbox): rename spec flag to settings

49b1124

khuedoan requested a review from Copilot January 23, 2026 16:14

Copilot started reviewing on behalf of khuedoan January 23, 2026 16:15 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: create general purpose toolbox CLI#1

feat: create general purpose toolbox CLI#1
khuedoan wants to merge 5 commits intomasterfrom
toolbox

khuedoan commented Jan 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Comments

-		_ = t.session.Signal(ssh.SIGTERM)
-		t.session.Close()
+		if err := t.session.Signal(ssh.SIGTERM); err != nil {
+			errs = append(errs, fmt.Errorf("signal session: %w", err))
+		}
+		if err := t.session.Close(); err != nil {
+			errs = append(errs, fmt.Errorf("close session: %w", err))
+		}

-			resp.Body.Close()
+			if resp.Body != nil {
+				defer func() {
+					if cerr := resp.Body.Close(); cerr != nil {
+						// ignore close error for health check
+					}
+				}()
+			}

-	randomBytes := make([]byte, length)
-	if _, err := rand.Read(randomBytes); err != nil {
-		return "", err
-	}
-	for i, b := range randomBytes {
-		result[i] = charset[int(b)%len(charset)]
+	charsetLen := len(charset)
+	if charsetLen == 0 {
+		return "", fmt.Errorf("charset is empty")
+	}
+	// Use rejection sampling to avoid modulo bias when mapping random bytes to charset indices.
+	maxMultiple := 256 / charsetLen * charsetLen
+	for i := 0; i < length; i++ {
+		for {
+			var b [1]byte
+			if _, err := rand.Read(b[:]); err != nil {
+				return "", err
+			}
+			if int(b[0]) >= maxMultiple {
+				// Discard values that would introduce modulo bias.
+				continue
+			}
+			result[i] = charset[int(b[0])%charsetLen]
+			break
+		}

Uh oh!

Conversation

khuedoan commented Jan 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Comments