[codex] Fix SSH git sync on fresh machines

[gmoigneu]

Summary

Fixes first-run SSH git sync failures for users who have not previously populated known_hosts or loaded keys into ssh-agent.

Root cause

The go-git SSH transport was relying on its default known_hosts callback and SSH agent behavior. On a fresh machine this could fail before clone with:

unable to find any valid known_hosts file

After creating known_hosts, it could still fail when a default private key existed on disk but was not loaded into ssh-agent.

Changes

• Add explicit SSH auth setup for git clone, fetch, and pull.
• Create/use the normal ~/.ssh/known_hosts path, while honoring SSH_KNOWN_HOSTS.
• Accept and persist unknown host keys on first contact, while still rejecting changed host keys.
• Load signers from both ssh-agent and default unencrypted private key files in ~/.ssh.
• Add unit coverage for known_hosts creation, accept-new behavior, changed-key rejection, and default-key auth fallback.

Fixes #239.

Validation

• go test ./internal/core/vcs
• make build
• make test

[Theosakamg]

🤖 QA Automated Review

QA Review — getgaal/gaal PR #240
Title: [codex] Fix SSH git sync on fresh machines
Author / branch: gmoigneu — codex/fix-ssh-first-run-sync
PR state: open (draft)
Labels: none
Reviewers requested: none
Created: 2026-06-27 — Last updated: 2026-07-04

Summary of change

• Adds explicit SSH auth and known_hosts handling for go-git clones/fetches/pulls.
• Ensures ~/.ssh/known_hosts exists (honors SSH_KNOWN_HOSTS).
• Accepts and persists unknown host keys on first contact; still rejects changed host keys.
• Loads signers from ssh-agent and default unencrypted keys from ~/.ssh.
• Adds unit tests covering known_hosts creation, accept-new behavior, changed-key rejection, and default-key auth fallback.
  Files modified (3):
• internal/core/vcs/git.go
• internal/core/vcs/ssh_known_hosts.go (new/changed: core of the feature)
• internal/core/vcs/ssh_known_hosts_test.go (new tests)

Risk classification

• Overall risk: 🟡 MEDIUM

Rationale:

• Scope is small (3 files) and focused on internal VCS/SSH auth behavior.
• Unit tests were added for the new known_hosts/SSH-keys logic which reduces regression risk.
• However this change modifies the SSH trust model by auto-accepting unknown host keys and persists them automatically — a security-sensitive behavioral change with potential operational impact. Because it touches core git clone/update paths used by the system, the change should be reviewed carefully and documented for users/admins. Therefore classified MEDIUM.

Detailed findings

1. High-level correctness and behavior

• The new flow explicitly wires SSH auth into golang go-git operations:
  • Clone uses sshAuthForURL()
  • Update uses sshAuthForRemote()
• Known-host handling:
  • knownHostsCandidates() respects SSH_KNOWN_HOSTS env or falls back to $HOME/.ssh/known_hosts and /etc/ssh/ssh_known_hosts.
  • prepareKnownHosts() ensures the user known_hosts file exists and builds file list for knownhosts.New().
  • acceptNewKnownHostsCallback() wraps knownhosts.New(...) and will append the host key to the configured writePath on first contact. It rejects changed keys (returns KeyError with Want populated).
• SSH auth:
  • sshPublicKeysAuth() attempts to create an agent-backed auth (newSSHAgentAuth) and loads default private keys from ~/.ssh (id_ed25519, id_ecdsa, id_rsa, id_dsa) when agent is unavailable or when agent has no signers.
  • The PublicKeysCallback returned combines file-based signers and agent signers when available.

2. Tests

• internal/core/vcs/ssh_known_hosts_test.go
  • Tests cover:
    • knownHostsCandidates uses HOME when SSH_KNOWN_HOSTS unset
    • honoring SSH_KNOWN_HOSTS
    • prepareKnownHosts creating user file
    • acceptNewKnownHostsCallback appending unknown host and rejecting changed host key
    • sshAuthForURL returning nil for non-ssh URLs and using default username for SSH
    • defaultSSHPrivateKeyPaths uses HOME
    • loadSSHPrivateKeySigners loads a PEM-ed key file
    • sshPublicKeysAuth fallback when agent unavailable
    • sshPublicKeysAuth error when no agent and no default keys
  • Good coverage of the new logic; tests make nice use of newSSHAgentAuth test hook.

3. Code quality / style

• Code is clean, well-structured, and well-documented (comments explain behavior).
• Uses a test hook newSSHAgentAuth to allow injecting a fake agent in tests — good practice.
• Concurrency around callback is protected by a mutex; callback reloads known_hosts after writing — reasonable.
• Error messages are wrapped in fmt.Errorf to provide context.

4. Security considerations (important)

• Auto-accepting unknown host keys and appending them to known_hosts changes the default SSH trust model (from "strict host key checking" to "trust-on-first-use (TOFU) with persistence").
  • This is documented in the PR body, but should be made explicit in user-facing docs and release notes.
  • Consider making this behavior configurable or clearly documenting the security implications for operators.
• Encrypted private keys (passphrase protected) are not supported by loadSSHPrivateKeySigner (ssh.ParsePrivateKey will fail). The code expects unencrypted keys or an available ssh-agent; if users have passphrase-protected private keys only, the behavior is to fail to parse and continue — tests check relevant fallback scenarios. Consider documenting this limitation.

5. Potential issues / edge cases / suggestions

• appendKnownHost opens the file with os.O_APPEND|os.O_WRONLY (no create) relying on ensureKnownHostsFile to have created it. prepareKnownHosts does ensure creation, but be mindful if functions are called in different order in future refactors.
• nonEmptyPathList does not expand tilde (~). If users set SSH_KNOWN_HOSTS to paths using ~, the code will not expand it. Consider documenting or expanding ~.
• encrypted private keys:
  • Currently parsed via ssh.ParsePrivateKey which returns an error for encrypted keys. If user keys are passphrase-protected, they must be loaded into ssh-agent. Consider adding support for prompting (likely not desired in non-interactive contexts) or explicit error messages suggesting ssh-agent.
• knownHostsCandidates uses systemKnownHostsPath (/etc/ssh/ssh_known_hosts) as fallback; on some systems that file might be inaccessible (permissions). prepareKnownHosts filters out files that don't exist or cannot be stat'd — behavior is sensible.
• The code will accept hostnames with ports (callback receives e.g. "example.com:22" and writes that into known_hosts). That is OK but might cause duplicate lines if different ways of referring to host are used; tests check only simple flows.

6. Missing or weak coverage

• There are no integration tests exercising the full clone/fetch/pull flow with the new auth callback. The unit tests cover the core pieces, but not the actual go-git operations when interacting with a real SSH server. Consider adding an integration test (containerized git+ssh server) or at least exercising PlainCloneContext with a local git server in CI if feasible.
• No tests for handling passphrase-protected keys. Add a test ensuring loadSSHPrivateKeySigner fails for encrypted keys with expected error message, or update docs.

7. Compilation / API surface

• The new code uses a testable hook newSSHAgentAuth allowing test injection — good.
• No exported API changes; changes are internal.

Actionable recommendations

1. Security & documentation

   • Document the new TOFU behavior prominently (README, upgrade notes). Explain that unknown host keys are appended automatically; provide instructions to opt out (e.g. set SSH_KNOWN_HOSTS to pre-seeded file).
   • Consider making the automatic acceptance of unknown host keys configurable via environment variable or config flag (so users who want strict checking can opt out).

2. Tests / CI

   • Add integration tests that exercise a full clone or fetch from an SSH git server (containerized or local ephemeral server) to validate behavior end-to-end.
   • Add a unit test or a note covering behavior when default key files are encrypted (ParsePrivateKey error); ensure the error returned to the caller is actionable.

3. Minor code suggestions

   • Optionally expand ~ in SSH_KNOWN_HOSTS paths (or document that ~ is not expanded).
   • Consider improving error messages when both agent is unavailable and keys are passphrase-protected, to explicitly tell user to run ssh-agent or create an unencrypted key (or load key with ssh-add).
   • When appending known_hosts, consider adding a debug/info log that indicates a new host key was accepted (I see a debug slog.Debug in acceptNewKnownHostsCallback, but consider same for appendKnownHost success).

Conclusion

• The patch is focused and small, has good unit coverage for the newly added functionality, and addresses the reported first-run failure modes. The primary concern is the security implication of auto-accepting unknown host keys — this is an intentional behavior in the PR, but it should be documented and possibly made configurable. With documentation and maybe an opt-out, this is a reasonable and useful change.
• Risk: 🟡 MEDIUM due to security semantics and touching core VCS behavior. Once documentation and optional configurability are addressed, the change can be merged from draft.

---

Generated by Gaal QA Agent — Mastra