Skip to content

security: use StrictHostKeyChecking=accept-new instead of no - #303

Closed
cluster2600 wants to merge 1 commit into
NVIDIA:mainfrom
cluster2600:fix/ssh-strict-host-key
Closed

security: use StrictHostKeyChecking=accept-new instead of no#303
cluster2600 wants to merge 1 commit into
NVIDIA:mainfrom
cluster2600:fix/ssh-strict-host-key

Conversation

@cluster2600

@cluster2600 cluster2600 commented Mar 18, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Replace StrictHostKeyChecking=no with StrictHostKeyChecking=accept-new in all SSH commands
  • Affects bin/nemoclaw.js (7 instances) and test/e2e/test-full-e2e.sh (1 instance)

Motivation

StrictHostKeyChecking=no disables host key verification entirely, making SSH connections vulnerable to MITM attacks. accept-new is the safe alternative — it accepts first-time connections (same UX as no for new hosts) but verifies known hosts on subsequent connections, catching any key changes.

Test plan

  • Verified all instances replaced across .js and .sh files
  • No functional change for first-time connections
  • Known hosts are now verified on reconnection

Summary by CodeRabbit

  • Chores
    • Updated SSH host key handling to accept new host keys (while keeping verification enabled) instead of disabling checks. This change applies to deployment, file synchronization, environment uploads, setup/service startup, sandbox connections, and in-sandbox inference commands to improve connection security.

@coderabbitai

coderabbitai Bot commented Mar 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: aaacc4fa-877c-4fb3-995d-82f4fdaf59d9

📥 Commits

Reviewing files that changed from the base of the PR and between d65f415 and a548cd3.

📒 Files selected for processing (2)
  • bin/nemoclaw.js
  • test/e2e/test-full-e2e.sh
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/e2e/test-full-e2e.sh
  • bin/nemoclaw.js
📝 Walkthrough

Walkthrough

SSH host key checking policy changed from StrictHostKeyChecking=no to StrictHostKeyChecking=accept-new across deployment scripts and the end-to-end test script to accept new host keys while retaining verification for known hosts.

Changes

Cohort / File(s) Summary
SSH Host Key Policy
bin/nemoclaw.js, test/e2e/test-full-e2e.sh		 Replaced -o StrictHostKeyChecking=no with -o StrictHostKeyChecking=accept-new in all ssh, scp, and rsync -e "ssh ..." invocations used during deploy, file sync, setup, service start, and sandbox connection/test.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 From rigid "no" to "accept-new" I scamper,
New keys welcomed, old ones still hamper—no longer a damper.
I nibble at prompts, hop through the night,
Securely connecting by soft lantern light,
A rabbit's cheer for safer flights ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: replacing StrictHostKeyChecking=no with accept-new across SSH commands for improved security.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@wscurran
wscurran requested a review from drobison00 March 23, 2026 16:42
@drobison00

drobison00 commented Mar 23, 2026

Copy link
Copy Markdown

@cluster2600 Hello! Thank you for contributing this PR. Could you rebase against main to resolve the branch conflicts?

@drobison00 drobison00 self-assigned this Mar 23, 2026
@cluster2600

cluster2600 commented Mar 23, 2026

Copy link
Copy Markdown
Contributor Author

@cluster2600 Hello! Thank you for contributing this PR. Could you rebase against main to resolve the branch conflicts?

Done mate!

@cluster2600
security: use StrictHostKeyChecking=accept-new instead of no
a548cd3 
StrictHostKeyChecking=no disables host key verification entirely,
making SSH connections vulnerable to MITM attacks. Replace with
accept-new which safely accepts first-time connections but verifies
known hosts on subsequent connections.

Signed-off-by: Maxime Grenu <maxime@cluster2600.com>
Signed-off-by: Maxime Grenu <maxime.grenu@gmail.com>
@cluster2600
cluster2600 force-pushed the fix/ssh-strict-host-key branch from d65f415 to a548cd3 Compare March 23, 2026 21:10
@cluster2600

cluster2600 commented Mar 23, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main and resolved the conflicts — kept the qname/runInteractive/try-finally improvements from main and applied the accept-new change on top.

@cv

cv commented Mar 24, 2026

Copy link
Copy Markdown
Collaborator

Closing — fully superseded by #691 (ericksoa), which implements trust-on-first-use: captures the host key via ssh-keyscan after the initial probe, then enforces StrictHostKeyChecking=yes with pinned known_hosts for all subsequent connections. Strictly stronger than accept-new, which still trusts the first connection without pinning.

@cv cv closed this Mar 24, 2026
mafueee pushed a commit to mafueee/NemoClaw that referenced this pull request Mar 28, 2026
@drew
feat(bootstrap): restore per-gateway Docker bridge networks (NVIDIA#303)
4b23a7e 
PR NVIDIA#281 removed the shared openshell-cluster Docker network in favor of
the default bridge. This restores custom bridge networking but makes each
gateway use its own isolated network named openshell-cluster-{name},
matching the existing container/volume naming convention.

Changes:
- Add network_name() to constants.rs for per-gateway network naming
- Add ensure_network() with retry/backoff and force_remove_network()
  parameterized by network name instead of a global constant
- Attach containers to their per-gateway network via network_mode
- Disconnect and remove the network during gateway destroy
- Wire ensure_network() into the deploy flow before ensure_volume()
- Update architecture docs to reflect per-gateway network isolation
@wscurran wscurran added feature PR adds or expands user-visible functionality and removed priority: high labels Jun 3, 2026
@coderabbitai coderabbitai Bot mentioned this pull request Jun 15, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drobison00 drobison00 Awaiting requested review from drobison00

Assignees

@drobison00 drobison00

Labels

feature PR adds or expands user-visible functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cluster2600 @drobison00 @cv @wscurran