fix(ssh): use TOFU known_hosts for interactive SSH sessions#653
Conversation
Interactive SSH used StrictHostKeyChecking=no, completely bypassing host key verification. Use holodeck's known_hosts file with accept-new for consistency with the Go SSH TOFU pattern. Audit finding NVIDIA#31 (LOW). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses an audit finding by improving interactive SSH host key verification to follow a TOFU (trust-on-first-use) approach, aligning interactive system ssh behavior with Holodeck’s Go SSH TOFU pattern.
Changes:
- Replaces
StrictHostKeyChecking=nowithStrictHostKeyChecking=accept-newfor interactive system SSH. - Configures interactive system SSH to use a Holodeck-specific
known_hostsfile under the user cache directory.
| cacheBase, _ := os.UserCacheDir() | ||
| knownHostsPath := filepath.Join(cacheBase, "holodeck", "known_hosts") | ||
|
|
There was a problem hiding this comment.
os.UserCacheDir() error is ignored here, and this also bypasses the CLI --cachepath setting (and instances.NewManager default). If UserCacheDir fails or a custom cache path is used, the interactive ssh session will read/write host keys in an unexpected location (or relative to CWD). Consider deriving knownHostsPath from m.cachePath when set (same base as instance cache), and return an error when the cache directory cannot be determined.
| cacheBase, _ := os.UserCacheDir() | |
| knownHostsPath := filepath.Join(cacheBase, "holodeck", "known_hosts") | |
| var knownHostsPath string | |
| if m.cachePath != "" { | |
| // When a custom cache path is configured, keep known_hosts alongside the instance cache. | |
| knownHostsPath = filepath.Join(m.cachePath, "known_hosts") | |
| } else { | |
| cacheBase, err := os.UserCacheDir() | |
| if err != nil { | |
| return fmt.Errorf("failed to determine user cache directory: %w", err) | |
| } | |
| knownHostsPath = filepath.Join(cacheBase, "holodeck", "known_hosts") | |
| } | |
| // Ensure the directory for the known_hosts file exists. | |
| if err := os.MkdirAll(filepath.Dir(knownHostsPath), 0o755); err != nil { | |
| return fmt.Errorf("failed to create known_hosts directory: %w", err) | |
| } |
| knownHostsPath := filepath.Join(cacheBase, "holodeck", "known_hosts") | ||
|
|
||
| args := []string{ | ||
| "-i", keyPath, | ||
| "-o", "StrictHostKeyChecking=no", | ||
| "-o", "UserKnownHostsFile=/dev/null", | ||
| "-o", "StrictHostKeyChecking=accept-new", | ||
| "-o", fmt.Sprintf("UserKnownHostsFile=%s", knownHostsPath), |
There was a problem hiding this comment.
Before calling system ssh, ensure the parent directory for knownHostsPath exists (e.g., os.MkdirAll(filepath.Dir(knownHostsPath), 0700)). Otherwise OpenSSH will fail to create/update the file when the holodeck cache directory hasn't been created yet (notably when using a non-default cache location).
Pull Request Test Coverage Report for Build 21962538360Details
💛 - Coveralls |
Summary
StrictHostKeyChecking=nowithaccept-new+ holodeck's known_hosts fileAudit Finding
cmd/cli/ssh/ssh.goChanges
cmd/cli/ssh/ssh.go: UseUserKnownHostsFilepointing to holodeck cache,StrictHostKeyChecking=accept-newTest plan
gofmt— no formatting issuesgo build— compilesgo test ./pkg/...— all tests pass