🔒 [security fix] Fix insecure SSH server verification default

[bearice]

This PR fixes a security vulnerability where the SSH connector would silently default to an insecure state (accepting any server key) if the configuration was omitted.

🎯 What: The vulnerability fixed is the insecure default for SSH host key verification.
⚠️ Risk: If left unfixed, attackers could perform Man-In-The-Middle (MITM) attacks on SSH connections established through the proxy.
🛡️ Solution: The fix makes serverKeyVerification a mandatory field in the SSH connector configuration. Users must now explicitly provide a host key fingerprint or explicitly opt-in to insecure behavior. This follows the "Secure by Default" principle.

---

PR created automatically by Jules for task 3122378215122098665 started by @bearice

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[claude]

Code Review — PR #524: Fix insecure SSH server verification default

Reviewing with direct engineering standards. Here's what I found.

---

Issue 1: Breaking Change Without Migration Path

Problem: Removing #[serde(default)] from server_key_verification silently breaks every existing SSH connector configuration that omits this field. Any user upgrading will get a serde parse error at startup — the proxy refuses to start with no graceful degradation.

Impact: This is a hard breaking change. Users running automated deployments will have an outage. The PR description calls this a "security fix" but it trades one security problem for a reliability problem.

Correct approach:

1. Keep #[serde(default)], which defaults to InsecureAcceptAny
2. Emit a tracing::warn! at connect time when using the default — make noise, not silence
3. Gate on an explicit opt_in: true field inside InsecureAcceptAny if you want to force users to be intentional
4. Document the field as required in CONFIG_GUIDE.md

Making a field mandatory mid-release is only acceptable with a major version bump and a migration guide. Neither exists here.

---

Issue 2: The Security Model Is Half-Finished

Problem: Two options are provided: InsecureAcceptAny (obviously wrong) and Fingerprint (brittle in practice). SHA256 fingerprint pinning breaks when a server rotates its host key — routine maintenance at many sites. There is no knownHosts option.

SSH has had ~/.ssh/known_hosts for 30 years. The correct fix adds a third variant:

#[serde(rename = "knownHosts")]
KnownHosts { path: PathBuf },

Without this, the "secure" option is operationally worse than InsecureAcceptAny for any non-trivial deployment. You've replaced one footgun with another.

---

Issue 3: Ok(false) on Fingerprint Mismatch

Location: src/connectors/ssh.rs:226

// current code on mismatch:
Ok(false)

Problem: This relies entirely on russh's internal behavior. If russh's check_server_key contract changes between versions, Ok(false) could silently proceed. The intent is to abort the connection — the code should make that explicit:

Err(russh::Error::NotAuthenticated)

Return an error, not a boolean that a future library version might interpret differently.

---

Issue 4: Default Trait Removal is an API Break

Problem: ServerKeyVerification previously derived Default. The PR removes this. Any code calling ServerKeyVerification::default() will fail to compile after this change. The Default impl for SshConnectorConfig is also removed.

Default on a public type is a public API commitment. If you want to remove it, that requires a deliberate deprecation cycle, not a side-effect of a security fix.

---

Issue 5: New Test File is Redundant and Fragile

Location: tests/ssh_security.rs

Problem: The three tests in tests/ssh_security.rs duplicate tests already in the module (#[cfg(test)] block). Worse, they call serde_yaml_ng::from_str::<SshConnectorConfig>() directly, bypassing the actual production code path (from_value() in src/connectors/ssh.rs:86). These tests don't test what happens in production — they test serde in isolation.

Delete this file. Fix the existing module tests instead.

---

Issue 6: Existing Tests Updated With Wrong Intent

Problem: Tests named test_ssh_connector_config_password and test_ssh_connector_config_custom_port now have serverKeyVerification: type: insecureAcceptAny added. These tests were validating that password auth and custom ports parse correctly. They no longer test what their names say they test. The test names are now misleading.

---

Improvement Roadmap

Critical (Fix Now)

• src/connectors/ssh.rs:26 — Restore #[serde(default)] on server_key_verification. Do not break existing configs.
• src/connectors/ssh.rs:226 — Change Ok(false) to Err(russh::Error::NotAuthenticated) on fingerprint mismatch.
• Do not merge: removing the Default impl is an API break with no justification.

Important (Next)

• src/connectors/ssh.rs — Add KnownHosts { path: PathBuf } variant to ServerKeyVerification. Without it, the only "secure" option is operationally impractical.
• src/connectors/ssh.rs:209 — Emit tracing::warn! when InsecureAcceptAny is loaded from config (not just at connect time). Make the insecure choice visible at startup.
• CONFIG_GUIDE.md — Document serverKeyVerification as strongly recommended and explain the security implications of each variant.

Nice to Have (Later)

• Delete tests/ssh_security.rs — it's redundant dead weight that tests serde, not the proxy.
• Rename existing module tests to accurately reflect what they test after the field additions.

[claude]

Code Review: PR #524 — SSH Server Verification Default

The intent here is correct: defaulting to InsecureAcceptAny was genuinely dangerous. Removing that default is the right call. But the execution has several problems that need to be addressed.

---

1. Breaking Change With No Migration Path

Problem: Removing #[serde(default)] from server_key_verification turns a configuration omission (previously silently "working") into a hard parse failure. Every existing deployment that omits serverKeyVerification will fail to start after upgrading. There is no deprecation period, no clear error message pointing users to the fix, and no mention of this in any documentation.

Impact: Real users hit a runtime panic on startup with a cryptic serde error. This is not "secure by default" — it's "breaks on upgrade by default."

Concrete solution:

• Add a clear, actionable error message in from_value() when server_key_verification is missing, e.g.: "SSH connector 'name': serverKeyVerification is required. Omitting it previously defaulted to insecureAcceptAny. See docs for migration."
• Update CONFIG_GUIDE.md with a migration note.
• The serde error message assertion in tests/ssh_security.rs (assert!(err.contains("missing field \serverKeyVerification`"))`) is checking an internal serde implementation detail. If the crate ever changes its error formatting, this test silently rots.

---

2. Fingerprint Mismatch Returns Ok(false), Not an Error

Problem: In src/connectors/ssh.rs:207, a fingerprint mismatch returns Ok(false) rather than Err(...):

ServerKeyVerification::Fingerprint { fingerprint: expected } => {
    // ...
    if fingerprint.to_string() == *expected {
        Ok(true)
    } else {
        tracing::error!("SSH server key fingerprint mismatch: ...");
        Ok(false)  // <--- THIS
    }
}

Impact: Whether the russh library treats Ok(false) as "connection rejected" or "connection allowed" depends entirely on russh's internal interpretation of the Handler::check_server_key return value. If russh treats Ok(false) as "key rejected but not fatal" and proceeds anyway, this is a security hole that the PR was supposed to fix. An active MITM attack gets a log line and a handshake that may proceed. Err(russh::Error::...) is unambiguous. Look at what Ok(false) actually does in the russh call chain before shipping this.

---

3. Test Bloat: Duplicate Tests, Wrong Code Path

Problem 1: test_ssh_connector_config_with_fingerprint_verification and test_ssh_connector_config_with_additional_fingerprint_verification in src/connectors/ssh.rs:335 and :355 are identical tests with different names. One of them must be deleted. Having two tests that verify the exact same behavior is not "more coverage" — it's noise that makes the test suite harder to read and maintain.

Problem 2: The new integration tests in tests/ssh_security.rs bypass the actual runtime code path. They call serde_yaml_ng::from_str::<SshConnectorConfig> directly, whereas the actual parsing at runtime goes through from_value(&serde_yaml_ng::Value) in src/connectors/ssh.rs:67. These are not equivalent. from_value does a clone and a second deserialization pass. A test that doesn't exercise the real code path is worse than no test — it gives false confidence.

Concrete solution: Delete the duplicate test. Rewrite tests/ssh_security.rs to call from_value() directly, matching the actual runtime path.

---

4. test_ssh_connector_default_config Enshrines the Insecure Behavior

Problem: src/connectors/ssh.rs:375:

fn test_ssh_connector_default_config() {
    let config = SshConnectorConfig {
        server_key_verification: ServerKeyVerification::InsecureAcceptAny,
        // ...
    };
    assert!(matches!(config.server_key_verification, ServerKeyVerification::InsecureAcceptAny));
}

This test constructs a struct with InsecureAcceptAny and then asserts it's InsecureAcceptAny. It tests nothing meaningful. Worse, its name (test_ssh_connector_default_config) implies InsecureAcceptAny is still the "default." This is confusing given the entire purpose of the PR.

---

Improvement Roadmap

Critical (Fix Now)

• src/connectors/ssh.rs:207 — Audit whether Ok(false) from check_server_key causes russh to reject or continue the connection. If russh does not hard-abort on Ok(false), change this to Err(russh::Error::UnknownKey) or equivalent.
• src/connectors/ssh.rs / CONFIG_GUIDE.md — Document the breaking change. Add a meaningful error message in from_value() when the field is missing, pointing users to the migration path.

Important (Next)

• tests/ssh_security.rs — Rewrite tests to go through from_value() instead of serde_yaml_ng::from_str to match the actual runtime code path.
• src/connectors/ssh.rs:335 or :355 — Delete one of the two identical fingerprint verification tests.
• src/connectors/ssh.rs:375 — Delete or rename test_ssh_connector_default_config; the test is meaningless and misleadingly named.

Nice to Have (Later)

• src/connectors/ssh.rs:191 — Escalate the log level for InsecureAcceptAny from warn to error. A misconfigured production proxy should be impossible to miss in logs.
• tests/ssh_security.rs:17 — Remove the err.contains("missing field \serverKeyVerification`")` assertion; it tests a serde implementation detail, not behavior.

[claude]

Code Review — PR #524: 🔒 Fix insecure SSH server verification default

Verdict: Do Not Merge As-Is — The security intent is correct. The implementation silently breaks every existing SSH connector deployment on upgrade.

---

Problem 1: Silent Breaking Change with a Terrible Error Message

Removing #[serde(default)] from server_key_verification means any config file that doesn't include serverKeyVerification now fails to parse. The user gets:

missing field `serverKeyVerification`

That's a serde deserialization error buried in whatever error propagation you have. Users upgrading will see their proxy fail to start with a cryptic message. There's no indication of why the field is now required, what values are valid, or what the old behavior was.

This is not "secure by default." This is "fail by default" with a bad error message.

---

Problem 2: The Break is Real

Any user who has:

connectors:
  - type: ssh
    name: my-tunnel
    server: bastion.example.com
    username: proxy
    auth:
      type: privateKey
      path: /etc/proxy/id_ed25519

...will find their proxy completely broken after upgrading to this version. No warning, no migration, no grace period. Just a parse error at startup.

The previous behavior was insecure (accept any key) but functional. This change prioritizes correctness over operability in a way that will guarantee people either:

1. Never upgrade (because their configs break)
2. Add type: insecureAcceptAny to get it working again — the exact insecure default you were trying to eliminate

You haven't made the system more secure. You've annoyed your users into re-enabling insecure mode.

---

Problem 3: The Default Removal Creates a Papercut

Deleting impl Default for SshConnectorConfig is a breaking change to the Rust API surface. Any code constructing SshConnectorConfig via struct literal now must specify server_key_verification explicitly. The tests in this PR do exactly that — they all hardcode InsecureAcceptAny, which proves the tests aren't actually testing the new behavior (mandatory field). They're just patching the compilation failure.

---

Problem 4: Test in Wrong Place

tests/ssh_security.rs as a top-level integration test file for what is fundamentally a unit-level serialization test is organizational bloat. These belong in src/connectors/ssh.rs in the existing mod tests block. The module already has a tests::test_ssh_connector_config_password test. Add alongside it.

---

The Correct Approach

Option A: Proper migration with validation feedback (Recommended)

Add a custom validator in config loading that checks for missing serverKeyVerification and emits a clear, actionable error:

// In from_value() or wherever configs are loaded:
if config.server_key_verification is missing {
    return Err(anyhow!(
        "SSH connector '{}' requires explicit 'serverKeyVerification'. \
         The insecure default has been removed. \
         Add 'serverKeyVerification: {{type: insecureAcceptAny}}' to preserve \
         the old behavior, or configure a fingerprint for security.",
        config.name
    ));
}

Option B: Keep the default, add a startup warning

The existing code already does tracing::warn!("SSH server key verification disabled - connection is insecure!") at runtime. That's actually fine. Log it loudly at startup too:

// In SshConnector::new() or from_value():
if matches!(config.server_key_verification, ServerKeyVerification::InsecureAcceptAny) {
    tracing::warn!(
        "SSH connector '{}' has insecure server key verification! \
         Configure 'serverKeyVerification' with a fingerprint to fix this.",
        config.name
    );
}

Then in the next major version, make it mandatory. Breaking changes belong in major version bumps with changelogs.

---

Improvement Roadmap

Critical (Fix Now)

• src/connectors/ssh.rs:26 — Do not make server_key_verification mandatory without a migration path; either restore #[serde(default)] or add a custom deserializer with a clear error message
• Add explicit changelog/migration note if this is intended to be a breaking change in a major version

Important (Next)

• Move tests/ssh_security.rs tests into src/connectors/ssh.rs's existing mod tests block — they're unit tests, not integration tests
• Remove the duplicate test in ssh.rs: test_ssh_connector_config_with_additional_fingerprint_verification is identical to test_ssh_connector_config_with_fingerprint_verification

Nice to Have (Later)

• Add a third ServerKeyVerification variant: KnownHosts { path: PathBuf } — fingerprint-only is not production-usable for most operators. The comment "if users need known_hosts, they should use OpenSSH" in the test is punting on a legitimate use case.

[claude]

Code Review — Linus Torvalds Style

PR #524: Fix insecure SSH server verification default

The security diagnosis is correct. Having InsecureAcceptAny as a silent default violates the most basic principle of secure system design: the dangerous path should require explicit intent. A MITM on an SSH tunnel proxy defeats the entire purpose of having a proxy.

The fix — removing #[serde(default)] and #[derive(Default)] — forces operators to be explicit. Good.

---

Problem 1: This Is a Breaking Change With No Migration Path

Removing #[serde(default)] means every existing deployment without an explicit serverKeyVerification field will fail to start after this update. Silently. With a serde deserialization error that most operators won't immediately understand.

The error users will see:

missing field `serverKeyVerification`

This is a configuration-breaking change. The operator running a production system does not know this PR exists. Their proxy just stops starting after an update with a cryptic error.

What should happen instead:

1. A CHANGELOG or migration notice must document this change prominently
2. The error message from from_value() should be enriched to say why the field is now required:

   failed to parse SSH connector config: missing field `serverKeyVerification`
   
   Note: serverKeyVerification is now required. To restore previous behavior explicitly:
     serverKeyVerification:
       type: insecureAcceptAny
   

   This is trivially achievable with .with_context() on the deserialization call.

---

Problem 2: The Test at tests/ssh_security.rs:13 Is Testing Serde, Not Security

fn test_missing_verification_fails() {
    // ...
    assert!(err.contains("missing field `serverKeyVerification`"), ...);
}

This test verifies that serde emits a specific error string. That's a fragile test of library behavior, not your behavior. Serde's error messages are not part of its public API and can change between versions. More importantly, testing that "config without the field fails to deserialize" is already covered by the invariant you just created — it's circular.

The security property you actually care about is: does check_server_key return false (or error) when no fingerprint matches? Write that test. The deserialization test is noise.

---

Problem 3: InsecureAcceptAny Still Has No Warning at Config Load Time

The warn at connection time (src/connectors/ssh.rs:210) is good but insufficient:

ServerKeyVerification::InsecureAcceptAny => {
    tracing::warn!("SSH server key verification disabled - connection is insecure!");

This fires per-connection. A heavily loaded proxy will spam the log with warnings that operators learn to ignore. The security warning belongs at config validation time, once, loudly. Add it to the SshConnector::new() or from_value() path.

---

Problem 4: There Is Still No KnownHosts / TOFU Mode

After this PR, operators have exactly two choices:

1. Accept any key blindly (insecureAcceptAny) — inherently insecure
2. Hard-code a fingerprint — operationally painful (key rotations break deployments)

Real SSH implementations use Trust On First Use with a known_hosts file. This is not a reason to reject the PR, but it means the "security fix" label is overstated. You've made the insecure option explicit; you have not made the secure option practical.

---

Test Coverage Gap

The existing tests in src/connectors/ssh.rs still have cases that test the struct literal directly with InsecureAcceptAny hardcoded but don't test the fingerprint verification logic end-to-end. The check_server_key function at line 204 is the critical path — it has no unit test.

---

Improvement Roadmap

Critical (Fix Now)

• src/connectors/ssh.rs:87-90 — Enrich the deserialization error with a human-readable migration hint explaining why serverKeyVerification is now required
• Add CHANGELOG entry / migration notes for this breaking change before any release

Important (Next)

• src/connectors/ssh.rs:204-230 — Add a unit test for check_server_key that verifies fingerprint mismatch returns false and fingerprint match returns true
• src/connectors/ssh.rs:81 or from_value() — Emit a config-load-time tracing::warn! when InsecureAcceptAny is configured, not just per-connection
• Delete tests/ssh_security.rs:13-27 (test_missing_verification_fails) — it tests serde internals, not your security invariant

Nice to Have (Later)

• Implement a knownHosts verification mode with a file path for persistent TOFU key storage — until then, the "secure" option is impractical for any dynamic infrastructure

[claude]

test comment

[claude]

Code Review: PR #524 — Fix Insecure SSH Server Verification Default

Short verdict: intent is correct, implementation has a hard breaking change with no migration path, and error propagation on fingerprint mismatch is wrong.

---

Issue 1 — This is a Breaking Change, Not a "Fix"

Problem Statement
Removing #[serde(default)] from server_key_verification means any existing YAML config that omits serverKeyVerification will now fail at startup with a deserialization error, rather than silently using the insecure default.

The PR title says "security fix." It is actually an incompatible API change. Every existing SSH connector config in production must be updated before deploying this change. If operators auto-deploy without reading changelogs, their proxy stops working entirely — which is arguably a worse outcome than the insecure default it was replacing.

Impact

• Any deployed config without serverKeyVerification field silently breaks at startup
• No warning, no helpful error message — just missing field 'serverKeyVerification'
• Silent operational failure is not a security improvement

Concrete Solution
Pick one of two honest approaches:

1. True secure-by-default: Add a third verification variant that refuses to connect and prints a runtime error with actionable instructions:

pub enum ServerKeyVerification {
    Fingerprint { fingerprint: String },
    InsecureAcceptAny,
    #[default]
    NotConfigured,  // runtime connect error: serverKeyVerification must be configured
}

This preserves backward compatibility (existing configs still parse), but fails at connect time with a clear message rather than silently succeeding.

2. Accept the break, document it explicitly: If you insist on making the field required, add a BREAKING CHANGE note, list migration steps, and bump the major version. Do not sneak this in as a patch.

---

Issue 2 — Fingerprint Mismatch Returns Ok(false) Instead of Err(...)

Problem Statement
In check_server_key after the PR:

tracing::error!(
    "SSH server key fingerprint mismatch: expected {}, got {}",
    expected,
    fingerprint
);
Ok(false)

The russh API treats Ok(false) as "reject and disconnect." The error returned to the caller of connect() is a generic russh connection error — not the fingerprint mismatch message. In a proxy routing context, the caller gets an opaque "SSH connection failed" with no indication why, making it nearly impossible to diagnose misconfigured fingerprints without enabling tracing at the right log level.

Impact

• Operator configures a stale fingerprint after server key rotation
• Proxy fails to connect with an opaque error
• No indication in the returned Result that it was a key mismatch

Concrete Solution
Return an Err so the mismatch propagates through the call stack:

Err(russh::Error::IO(std::io::Error::new(
    std::io::ErrorKind::ConnectionRefused,
    format!("SSH server key fingerprint mismatch: expected {expected}, got {fingerprint}"),
)))

---

Issue 3 — No TOFU or known_hosts Support

Problem Statement
The only options are "accept any key" or "pin one specific fingerprint." A comment in the new test file says:

// If users need known_hosts verification, they should use OpenSSH directly

This is a design cop-out. If the SSH connector cannot support standard host verification, document that scope limitation explicitly in the config guide — not in a comment inside a test file nobody reads.

Concrete Solution
Either add a KnownHosts { path: PathBuf } variant to ServerKeyVerification, or add a note to CONFIG_GUIDE.md explicitly stating the SSH connector is scoped to static single-server scenarios where fingerprint-pinning is acceptable.

---

Issue 4 — Test Redundancy

Problem Statement
test_ssh_connector_config_with_fingerprint_verification and test_ssh_connector_config_with_additional_fingerprint_verification test the identical scenario with the same fingerprint value SHA256:abcdef123456789. The new tests/ssh_security.rs adds a third fingerprint test. Three tests, same code path, zero coverage for error propagation at the connect() call site when a key mismatch occurs.

Concrete Solution
Delete the duplicate. Write a test that verifies the returned Result contains actionable error text on a fingerprint mismatch.

---

Improvement Roadmap

Critical (Fix Now)

• src/connectors/ssh.rs: Add a NotConfigured default variant OR explicitly document this as a breaking change with migration steps — the current approach silently breaks existing deployments
• src/connectors/ssh.rs in check_server_key: Return Err(...) on fingerprint mismatch so the error propagates back to the caller, not just to the log

Important (Next)

• Add KnownHosts { path: PathBuf } variant to ServerKeyVerification — fingerprint-only is insufficient for any production deployment with key rotation
• Write a connect-level test that verifies the Result returned on key mismatch contains a meaningful error string

Nice to Have (Later)

• Remove test_ssh_connector_config_with_additional_fingerprint_verification from src/connectors/ssh.rs — it is a duplicate of the fingerprint test directly above it