fix(security): known-hosts Put fails closed on concurrent first-use key conflict (#668)

* fix(security): known-hosts Put fails closed on concurrent first-use key conflict

internal/knownhosts.Store.Put discarded the command tag, so a conflicting-key
INSERT that updated 0 rows returned nil. On a concurrent first-use race (two
first-ever dials to the same hostname — scheduler + manual scan, or scan +
liveness), the first connection records key A; a second arriving with a
DIFFERENT key B (an attacker's MITM key on one path) hits ON CONFLICT, the
`WHERE public_key = EXCLUDED.public_key` predicate is false, 0 rows update, and
Put returned nil — so the TOFU callback accepted the unverified key B and
presented the real credential to the attacker. Steady state was already safe
(later dials Get-hit A and reject B); the gap was strictly the concurrent
first-use window.

Fix: check tag.RowsAffected(); 0 rows means a different key is already stored, so
return ssh.ErrHostKeyMismatch and let the dial fail closed. Same key still
refreshes last_seen (1 row) and stays idempotent.

Regression test: Put A, then Put B for the same host returns ErrHostKeyMismatch
and leaves A stored. Found by the rc.13 security review.

* docs: use standard 'Fixed' category for the Unreleased docs entry

The docs-accuracy entry (#667) used a '### Docs' heading under [Unreleased],
which TestChangelog_UnreleasedCategoriesAreStandard rejects (only Added/Changed/
Deprecated/Removed/Fixed/Security are allowed there). Renamed to '### Fixed'.
Released sections may keep their '### Docs' heading; the test only gates
[Unreleased].

Author: remyluslosius

CHANGELOG.md

@@ -10,7 +10,7 @@ Versioning: [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
 
-### Docs
+### Fixed
 - Operator-guide truthfulness + accuracy pass (`docs/guides/`): verified every
   documented `openwatch` CLI subcommand, REST endpoint, file path, env var, and
   systemd unit against the binary, `api/openapi.yaml`, and `packaging/`. Fixed

internal/knownhosts/store.go

@@ -14,6 +14,8 @@ import (
 	"time"
 
 	"github.com/jackc/pgx/v5/pgxpool"
+
+	owssh "github.com/Hanalyx/openwatch/internal/ssh"
 )
 
 // Store implements ssh.KnownHostsStore against the ssh_known_hosts table.
@@ -47,11 +49,25 @@ func (s *Store) Get(hostname string) ([]byte, bool) {
 func (s *Store) Put(hostname string, marshalled []byte) error {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
-	_, err := s.pool.Exec(ctx,
+	tag, err := s.pool.Exec(ctx,
 		`INSERT INTO ssh_known_hosts (hostname, public_key)
 		 VALUES ($1, $2)
 		 ON CONFLICT (hostname) DO UPDATE SET last_seen = now()
 		 WHERE ssh_known_hosts.public_key = EXCLUDED.public_key`,
 		hostname, marshalled)
-	return err
+	if err != nil {
+		return err
+	}
+	// Zero rows affected means a row already existed for this hostname with a
+	// DIFFERENT key: a concurrent first-use race lost to another connection
+	// that recorded a different key (or a changed key slipped past Get). The
+	// INSERT conflicted and the UPDATE's key-equality predicate was false, so
+	// nothing was written. Fail closed — never let Put report success for a key
+	// that does not match what is now stored, which would otherwise let the
+	// TOFU callback accept an unverified (possibly MITM) host key on this
+	// connection.
+	if tag.RowsAffected() == 0 {
+		return owssh.ErrHostKeyMismatch
+	}
+	return nil
 }

internal/knownhosts/store_test.go

@@ -5,9 +5,11 @@
 package knownhosts
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/Hanalyx/openwatch/internal/db/dbtest"
+	owssh "github.com/Hanalyx/openwatch/internal/ssh"
 )
 
 // @ac AC-22
@@ -49,3 +51,32 @@ func TestStore_DurableTOFU(t *testing.T) {
 		}
 	})
 }
+
+// @ac AC-22
+// Regression (known-hosts TOCTOU): when a row already exists for the hostname
+// with a DIFFERENT key — the concurrent first-use race where another connection
+// recorded a different key first — Put MUST fail closed with ErrHostKeyMismatch.
+// Before the fix the conflicting UPDATE touched 0 rows but Exec returned nil, so
+// Put reported success and the TOFU callback accepted an unverified key.
+func TestStore_Put_ConflictingKeyFailsClosed(t *testing.T) {
+	t.Run("system-ssh-connectivity/AC-22", func(t *testing.T) {
+		pool := dbtest.Pool(t)
+		s := NewStore(pool)
+
+		// First connection records key A.
+		if err := s.Put("host-x", []byte("first-key-A")); err != nil {
+			t.Fatalf("Put A: %v", err)
+		}
+		// A racing first-use connection presents a DIFFERENT key B for the same
+		// host. Put must NOT report success.
+		err := s.Put("host-x", []byte("racing-different-key-B"))
+		if !errors.Is(err, owssh.ErrHostKeyMismatch) {
+			t.Fatalf("Put with conflicting key = %v, want ErrHostKeyMismatch", err)
+		}
+		// The originally-stored key still wins; the conflicting key was rejected.
+		got, ok := s.Get("host-x")
+		if !ok || string(got) != "first-key-A" {
+			t.Fatalf("stored key = (%q, %v), want first-key-A unchanged", got, ok)
+		}
+	})
+}