diff --git a/handshake.go b/handshake.go index 1c51d78..014427b 100644 --- a/handshake.go +++ b/handshake.go @@ -42,6 +42,18 @@ type HandshakeMsg struct { } // TrustRecord holds information about a trusted peer. +// +// PublicKey is the peer key the record was established against, and is +// what scopes the record: node IDs are recycled (reaped and re-claimed +// by a different identity) and peers rotate keys, so a record only +// describes the same counterparty for as long as the key behind the +// node ID is unchanged. Checks that have the peer's current key in +// scope go through IsTrustedWithKey, which compares the two. +// +// An empty PublicKey means "unbound": records restored from a snapshot +// written before keys were recorded, and relay-established records +// whose backfillPeerKey has not landed yet, both look like this. Those +// stay trusted on node ID alone and adopt the first key that turns up. type TrustRecord struct { NodeID uint32 PublicKey string // base64 Ed25519 pubkey @@ -747,11 +759,15 @@ func (hm *Manager) handleRequest(stream coreapi.Stream, msg *HandshakeMsg, regis hm.mu.Lock() defer hm.mu.Unlock() - // Already trusted? + // Already trusted? Only if the record still binds the key this + // request carries — otherwise it is dropped here and the request + // falls through to the normal approval paths below. if _, ok := hm.trusted[peerNodeID]; ok { - slog.Debug("node already trusted", "peer_node_id", peerNodeID) - hm.sendAcceptLocked(peerNodeID) - return + if hm.reconcileTrustBindingLocked(peerNodeID, msg.PublicKey) { + slog.Debug("node already trusted", "peer_node_id", peerNodeID) + hm.sendAcceptLocked(peerNodeID) + return + } } // Check if we have an outgoing request to this peer (mutual handshake) @@ -909,6 +925,22 @@ func (hm *Manager) handleAccept(msg *HandshakeMsg) { delete(hm.revoked, peerNodeID) } + // An accept that carries a different key than the record we hold + // comes from a different key holder behind the same node ID. Drop + // the record instead of rebinding it — re-trusting is an explicit + // handshake, not a side effect of an inbound accept. + if existing, ok := hm.trusted[peerNodeID]; ok { + bound := existing.PublicKey + if !hm.reconcileTrustBindingLocked(peerNodeID, msg.PublicKey) { + delete(hm.outgoing, peerNodeID) + return + } + // Preserve an already-bound key when the accept omits one. + if msg.PublicKey == "" { + msg.PublicKey = bound + } + } + delete(hm.outgoing, peerNodeID) hm.markTrustedLocked(peerNodeID, &TrustRecord{ NodeID: peerNodeID, @@ -1185,6 +1217,11 @@ func (hm *Manager) processRelayedApproval(fromNodeID uint32) { // backfillPeerKey fetches a peer's public key from the registry and updates // the trust record. Called asynchronously after relay-based trust establishment, // where the P2P public key exchange didn't happen. +// +// It doubles as the binding check for relay-established records: an +// unbound record adopts the registry key, and a record bound to some +// other key is dropped, since the node ID now resolves to a different +// identity than the one trust was granted to. func (hm *Manager) backfillPeerKey(peerNodeID uint32) { reg := hm.rt.Registry() if reg == nil { @@ -1201,12 +1238,30 @@ func (hm *Manager) backfillPeerKey(peerNodeID uint32) { } hm.mu.Lock() - defer hm.mu.Unlock() - if rec, ok := hm.trusted[peerNodeID]; ok && rec.PublicKey == "" { + rec, ok := hm.trusted[peerNodeID] + if !ok { + hm.mu.Unlock() + return + } + if rec.PublicKey == "" { rec.PublicKey = pubKeyB64 hm.markDirty() + hm.mu.Unlock() slog.Debug("backfilled peer public key", "peer_node_id", peerNodeID) + return + } + if subtle.ConstantTimeCompare([]byte(rec.PublicKey), []byte(pubKeyB64)) == 1 { + hm.mu.Unlock() + return } + delete(hm.trusted, peerNodeID) + hm.markDirty() + hm.mu.Unlock() + + slog.Warn("dropping trust record: registry key differs from bound key", "peer_node_id", peerNodeID) + hm.rt.PublishEvent("trust.changed", map[string]interface{}{ + "peer_node_id": peerNodeID, "state": "revoked", "reason": "key_changed", + }) } // ProcessRelayedRejection handles a handshake rejection received via registry relay. @@ -1394,6 +1449,11 @@ func (hm *Manager) handleRevokeMsg(msg *HandshakeMsg) { } // IsTrusted returns whether a peer has been approved. +// +// This answers the node-ID-only question, for call sites that have no +// peer key in scope (outbound dials, for instance). Callers that do +// know the key the peer is currently presenting should use +// IsTrustedWithKey instead so the record's binding is enforced. func (hm *Manager) IsTrusted(nodeID uint32) bool { hm.mu.RLock() defer hm.mu.RUnlock() @@ -1401,6 +1461,114 @@ func (hm *Manager) IsTrusted(nodeID uint32) bool { return ok } +// IsTrustedWithKey returns whether a peer has been approved, given the +// base64 Ed25519 key that peer currently presents (same encoding as +// TrustRecord.PublicKey — crypto.EncodePublicKey). +// +// A record bound to a different key does not describe the peer behind +// this node ID any more, so it is dropped and the answer is false; the +// peer has to handshake again under its new key. +// +// Both unknowns degrade to IsTrusted rather than to a denial, so +// upgrades and older peers keep their trust: +// +// - currentKeyB64 empty — no key resolved at the call site. +// - record unbound — pre-binding snapshot or a not-yet-backfilled +// relay record. It adopts currentKeyB64 here, so the binding takes +// effect from the first check that resolves a key. +// +// The common case (bound key, matching) is served entirely under the +// read lock; only adopting and dropping take the write lock. +func (hm *Manager) IsTrustedWithKey(nodeID uint32, currentKeyB64 string) bool { + hm.mu.RLock() + rec, ok := hm.trusted[nodeID] + var bound string + if ok { + bound = rec.PublicKey + } + hm.mu.RUnlock() + + if !ok { + return false + } + if currentKeyB64 == "" { + return true + } + if bound == "" { + hm.adoptPeerKey(nodeID, currentKeyB64) + return true + } + if subtle.ConstantTimeCompare([]byte(bound), []byte(currentKeyB64)) == 1 { + return true + } + hm.dropRebound(nodeID, bound) + return false +} + +// adoptPeerKey binds keyB64 to an existing, still-unbound trust record. +// No-op when the record has since gone away or been bound elsewhere. +func (hm *Manager) adoptPeerKey(nodeID uint32, keyB64 string) { + hm.mu.Lock() + rec, ok := hm.trusted[nodeID] + if !ok || rec.PublicKey != "" { + hm.mu.Unlock() + return + } + rec.PublicKey = keyB64 + hm.markDirty() + hm.mu.Unlock() + slog.Debug("bound peer public key to trust record", "peer_node_id", nodeID) +} + +// dropRebound removes a trust record whose bound key no longer matches +// the key the peer presents. staleKey is re-checked under the write lock +// so a record re-established in the meantime is left alone. +func (hm *Manager) dropRebound(nodeID uint32, staleKey string) { + hm.mu.Lock() + rec, ok := hm.trusted[nodeID] + if !ok || rec.PublicKey != staleKey { + hm.mu.Unlock() + return + } + delete(hm.trusted, nodeID) + hm.markDirty() + hm.mu.Unlock() + + slog.Warn("dropping trust record: peer public key changed", "peer_node_id", nodeID) + hm.rt.PublishEvent("trust.changed", map[string]interface{}{ + "peer_node_id": nodeID, "state": "revoked", "reason": "key_changed", + }) +} + +// reconcileTrustBindingLocked resolves an existing trust record against +// the key a peer is presenting on an inbound handshake message, and +// reports whether the record still stands. Same rules as +// IsTrustedWithKey; separate because these call sites already hold +// hm.mu write-locked. Caller MUST hold hm.mu. +func (hm *Manager) reconcileTrustBindingLocked(nodeID uint32, currentKeyB64 string) bool { + rec, ok := hm.trusted[nodeID] + if !ok { + return false + } + if currentKeyB64 == "" || rec.PublicKey == "" { + if rec.PublicKey == "" && currentKeyB64 != "" { + rec.PublicKey = currentKeyB64 + hm.markDirty() + } + return true + } + if subtle.ConstantTimeCompare([]byte(rec.PublicKey), []byte(currentKeyB64)) == 1 { + return true + } + delete(hm.trusted, nodeID) + hm.markDirty() + slog.Warn("dropping trust record: peer public key changed", "peer_node_id", nodeID) + hm.rt.PublishEvent("trust.changed", map[string]interface{}{ + "peer_node_id": nodeID, "state": "revoked", "reason": "key_changed", + }) + return false +} + // TrustedPeers returns all trusted peers. func (hm *Manager) TrustedPeers() []TrustRecord { hm.mu.RLock() diff --git a/zz_attack_replay_pubkey_panic_test.go b/zz_attack_replay_pubkey_panic_test.go new file mode 100644 index 0000000..fef9547 --- /dev/null +++ b/zz_attack_replay_pubkey_panic_test.go @@ -0,0 +1,234 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package handshake + +import ( + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "fmt" + "strings" + "testing" + "time" + + "github.com/pilot-protocol/common/crypto" +) + +// Adversarial replay for the remote-panic finding: a handshake message +// whose public_key is well-formed base64 but decodes to something other +// than 32 bytes used to reach ed25519.Verify, which panics on a +// wrong-size key. A panic inside the handshake listener is a remote DoS +// against any node reachable on the handshake port. +// +// The fix is two-layer: crypto.Verify (common) length-guards before +// calling into ed25519, and handleConnection carries a recover() so any +// future panic inside the dispatch drops one connection rather than the +// process. Both layers are exercised here. + +// malformedKeySizes covers every interesting wrong length: empty-ish, +// truncated, off-by-one either side of the real key size, signature- +// sized (a plausible copy/paste confusion), and oversized. +var malformedKeySizes = []int{1, 2, 15, 16, 31, 33, 63, 64, 65, 128, 1024, 4096} + +func randomB64(t *testing.T, n int) string { + t.Helper() + b := make([]byte, n) + if _, err := rand.Read(b); err != nil { + t.Fatalf("rand: %v", err) + } + return base64.StdEncoding.EncodeToString(b) +} + +// TestAttackReplay_CryptoVerifyRejectsWrongLengthKeys pins the common +// v0.5.9 guard directly: crypto.Verify must return false — never panic — +// for any key length other than ed25519.PublicKeySize. +func TestAttackReplay_CryptoVerifyRejectsWrongLengthKeys(t *testing.T) { + t.Parallel() + + real, err := crypto.GenerateIdentity() + if err != nil { + t.Fatalf("generate identity: %v", err) + } + msg := []byte("handshake:1:2") + sig := ed25519.Sign(real.PrivateKey, msg) + + for _, n := range append([]int{0}, malformedKeySizes...) { + t.Run(fmt.Sprintf("keylen=%d", n), func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("ATTACK SUCCEEDED: crypto.Verify panicked on a %d-byte key: %v", n, r) + } + }() + key := make([]byte, n) + if _, err := rand.Read(key); err != nil { + t.Fatalf("rand: %v", err) + } + if crypto.Verify(key, msg, sig) { + t.Fatalf("ATTACK SUCCEEDED: crypto.Verify accepted a %d-byte key", n) + } + }) + } +} + +// TestAttackReplay_MalformedPubKeyOverWire drives the full production +// path — attacker JSON bytes into handleConnection — for every handshake +// message type and every wrong key length. The assertion is twofold: the +// process must survive (no panic escapes), and nothing may end up +// trusted. +func TestAttackReplay_MalformedPubKeyOverWire(t *testing.T) { + t.Parallel() + + types := []string{HandshakeRequest, HandshakeAccept, HandshakeReject, HandshakeRevoke} + + nodeID := uint32(60000) + for _, typ := range types { + for _, n := range malformedKeySizes { + typ, n := typ, n + nodeID++ + victim := nodeID + t.Run(fmt.Sprintf("%s/keylen=%d", typ, n), func(t *testing.T) { + hm, _ := newBoundHM(t, 2000) + defer func() { + if r := recover(); r != nil { + t.Fatalf("ATTACK SUCCEEDED: handshake panicked on %s with a %d-byte public_key: %v", typ, n, r) + } + }() + + payload := attackPayload(t, &HandshakeMsg{ + Type: typ, + NodeID: victim, + PublicKey: randomB64(t, n), + Signature: randomB64(t, ed25519.SignatureSize), + Timestamp: time.Now().Unix(), + }) + hm.handleConnection(newWireStream(payload, victim)) + + assertNotTrusted(t, hm, victim, fmt.Sprintf("%s with a %d-byte public_key", typ, n)) + }) + } + } +} + +// TestAttackReplay_MalformedSignatureOverWire is the sibling case: a +// correctly-sized key paired with a wrong-length signature. ed25519 +// tolerates this without panicking, but the message must still be +// rejected rather than sliding into trust. +func TestAttackReplay_MalformedSignatureOverWire(t *testing.T) { + t.Parallel() + + id, err := crypto.GenerateIdentity() + if err != nil { + t.Fatalf("generate identity: %v", err) + } + + nodeID := uint32(61000) + for _, n := range []int{0, 1, 32, 63, 65, 128, 4096} { + n := n + nodeID++ + victim := nodeID + t.Run(fmt.Sprintf("siglen=%d", n), func(t *testing.T) { + hm, _ := newBoundHM(t, 2001) + defer func() { + if r := recover(); r != nil { + t.Fatalf("ATTACK SUCCEEDED: handshake panicked on a %d-byte signature: %v", n, r) + } + }() + + payload := attackPayload(t, &HandshakeMsg{ + Type: HandshakeAccept, + NodeID: victim, + PublicKey: crypto.EncodePublicKey(id.PublicKey), + Signature: randomB64(t, n), + Timestamp: time.Now().Unix(), + }) + hm.handleConnection(newWireStream(payload, victim)) + + assertNotTrusted(t, hm, victim, fmt.Sprintf("accept with a %d-byte signature", n)) + }) + } +} + +// TestAttackReplay_HostilePubKeyEncodings covers the parser layer under +// the length guard: non-base64 bytes, padding abuse, embedded NULs, and +// an oversized field. None of these may panic or trust anything. +func TestAttackReplay_HostilePubKeyEncodings(t *testing.T) { + t.Parallel() + + keys := map[string]string{ + "not-base64": "!!!!not base64 at all!!!!", + "bad-padding": "AAAA=AAA", + "embedded-nul": "AAAA\x00AAAA", + "whitespace": " \n\t ", + "huge": strings.Repeat("A", 60000), + "unicode": "🔑🔑🔑🔑", + "only-padding": "====", + "valid-b64-odd": base64.StdEncoding.EncodeToString([]byte("short")), + "url-safe-alpha": "-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-A", + } + + nodeID := uint32(62000) + for name, key := range keys { + name, key := name, key + nodeID++ + victim := nodeID + t.Run(name, func(t *testing.T) { + hm, _ := newBoundHM(t, 2002) + defer func() { + if r := recover(); r != nil { + t.Fatalf("ATTACK SUCCEEDED: handshake panicked on public_key %q: %v", name, r) + } + }() + + payload := attackPayload(t, &HandshakeMsg{ + Type: HandshakeRequest, + NodeID: victim, + PublicKey: key, + Signature: randomB64(t, ed25519.SignatureSize), + Timestamp: time.Now().Unix(), + }) + hm.handleConnection(newWireStream(payload, victim)) + + assertNotTrusted(t, hm, victim, "hostile public_key encoding "+name) + }) + } +} + +// TestAttackReplay_TruncatedAndGarbageWire feeds bytes that are not a +// handshake message at all straight into the connection handler — the +// outermost layer an unauthenticated remote can reach. +func TestAttackReplay_TruncatedAndGarbageWire(t *testing.T) { + t.Parallel() + + payloads := [][]byte{ + nil, + {}, + []byte("{"), + []byte(`{"type":`), + []byte(`{"type":"handshake_accept","node_id":`), + []byte(`{"type":"handshake_accept","node_id":99999999999999999999}`), + []byte(`[]`), + []byte(`null`), + []byte(`"handshake_accept"`), + []byte{0x00, 0xff, 0xfe, 0x01, 0x02}, + } + + for i, p := range payloads { + i, p := i, p + t.Run(fmt.Sprintf("payload=%d", i), func(t *testing.T) { + hm, _ := newBoundHM(t, 2003) + defer func() { + if r := recover(); r != nil { + t.Fatalf("ATTACK SUCCEEDED: handshake panicked on payload %d: %v", i, r) + } + }() + hm.handleConnection(newWireStream(p, 66666)) + + hm.mu.RLock() + n := len(hm.trusted) + hm.mu.RUnlock() + if n != 0 { + t.Fatalf("ATTACK SUCCEEDED: garbage payload %d injected %d trust records", i, n) + } + }) + } +} diff --git a/zz_attack_replay_trust_injection_test.go b/zz_attack_replay_trust_injection_test.go new file mode 100644 index 0000000..ccfd844 --- /dev/null +++ b/zz_attack_replay_trust_injection_test.go @@ -0,0 +1,325 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package handshake + +import ( + "crypto/ed25519" + "encoding/base64" + "encoding/json" + "fmt" + "io" + "testing" + "time" + + "github.com/pilot-protocol/common/coreapi" + "github.com/pilot-protocol/common/crypto" +) + +// Adversarial replay harness for the pentest findings against the +// handshake trust surface (PPA-001 / PPA-002 family). These tests drive +// the real production paths — handleConnection parses attacker-supplied +// JSON bytes off a stream exactly as the listener does — so they fail +// loudly if the accept/revoke identity binding at processMessage is ever +// relaxed again. +// +// Threat model: the attacker owns a node on the overlay and can open a +// handshake stream to the victim. The stream's authenticated identity is +// the attacker's node ID (that is what the tunnel proved). Everything +// inside the JSON body is attacker-chosen. + +// wireStream is a coreapi.Stream that replays a fixed attacker payload +// and reports an attacker-controlled authenticated remote node ID. It is +// the closest fake to what handleConnection sees in production: a real +// stream whose RemoteAddr().Node was established by the transport, not +// by the message body. +type wireStream struct { + data []byte + pos int + addr coreapi.Addr + writes [][]byte + closed bool + readDone bool +} + +func newWireStream(payload []byte, remoteNode uint32) *wireStream { + return &wireStream{data: payload, addr: coreapi.Addr{Node: remoteNode}} +} + +func (s *wireStream) Read(p []byte) (int, error) { + if s.pos >= len(s.data) { + s.readDone = true + return 0, io.EOF + } + n := copy(p, s.data[s.pos:]) + s.pos += n + return n, nil +} + +func (s *wireStream) Write(p []byte) (int, error) { + cp := make([]byte, len(p)) + copy(cp, p) + s.writes = append(s.writes, cp) + return len(p), nil +} + +func (s *wireStream) Close() error { s.closed = true; return nil } +func (s *wireStream) LocalAddr() coreapi.Addr { return coreapi.Addr{} } +func (s *wireStream) LocalPort() uint16 { return 0 } +func (s *wireStream) RemoteAddr() coreapi.Addr { return s.addr } +func (s *wireStream) RemotePort() uint16 { return 0 } +func (s *wireStream) SetDeadline(time.Time) error { return nil } +func (s *wireStream) SetReadDeadline(time.Time) error { return nil } +func (s *wireStream) SetWriteDeadline(time.Time) error { return nil } + +// attackPayload marshals a handshake message to the exact bytes an +// attacker would put on the wire. +func attackPayload(t *testing.T, msg *HandshakeMsg) []byte { + t.Helper() + b, err := json.Marshal(msg) + if err != nil { + t.Fatalf("marshal attack payload: %v", err) + } + return b +} + +func assertNotTrusted(t *testing.T, hm *Manager, nodeID uint32, what string) { + t.Helper() + hm.mu.RLock() + rec, trusted := hm.trusted[nodeID] + hm.mu.RUnlock() + if trusted { + t.Fatalf("ATTACK SUCCEEDED (%s): node %d is trusted, record=%+v", what, nodeID, rec) + } +} + +func assertTrusted(t *testing.T, hm *Manager, nodeID uint32, what string) { + t.Helper() + hm.mu.RLock() + _, trusted := hm.trusted[nodeID] + hm.mu.RUnlock() + if !trusted { + t.Fatalf("ATTACK SUCCEEDED (%s): node %d lost its trust record", what, nodeID) + } +} + +// TestAttackReplay_BareAcceptOverWire replays the original pentest +// payload byte-for-byte through handleConnection: a handshake_accept +// naming the victim node ID, with no public key and no signature, sent +// from an attacker-authenticated stream. This is the exact message that +// used to inject trust for an arbitrary node ID. +func TestAttackReplay_BareAcceptOverWire(t *testing.T) { + t.Parallel() + hm, _ := newBoundHM(t, 1000) + + const victim = uint32(31337) + const attacker = uint32(66613) + + payload := attackPayload(t, &HandshakeMsg{ + Type: HandshakeAccept, + NodeID: victim, + PublicKey: "", + Signature: "", + Timestamp: time.Now().Unix(), + }) + + hm.handleConnection(newWireStream(payload, attacker)) + + assertNotTrusted(t, hm, victim, "bare empty-pubkey accept over the wire") + assertNotTrusted(t, hm, attacker, "bare empty-pubkey accept over the wire (attacker self-trust)") +} + +// TestAttackReplay_BareAcceptSprayOverWire replays the same bare accept +// against a whole range of victim node IDs from one attacker stream — +// the "harvest the network" form of the attack. Distinct node IDs each +// produce a distinct replay-set hash, so every message reaches the +// identity check rather than being swallowed by replay suppression. +func TestAttackReplay_BareAcceptSprayOverWire(t *testing.T) { + t.Parallel() + hm, _ := newBoundHM(t, 1001) + + const attacker = uint32(66614) + for victim := uint32(20000); victim < 20064; victim++ { + payload := attackPayload(t, &HandshakeMsg{ + Type: HandshakeAccept, + NodeID: victim, + Timestamp: time.Now().Unix(), + }) + hm.handleConnection(newWireStream(payload, attacker)) + } + + hm.mu.RLock() + n := len(hm.trusted) + hm.mu.RUnlock() + if n != 0 { + t.Fatalf("ATTACK SUCCEEDED (bare accept spray): %d trust records injected", n) + } +} + +// TestAttackReplay_BareRevokeOverWire is the destructive half of +// PPA-001: an unauthenticated handshake_revoke naming a victim must not +// strip an existing trust record (a trust-teardown DoS against any peer +// pair on the network). +func TestAttackReplay_BareRevokeOverWire(t *testing.T) { + t.Parallel() + hm, rt := newBoundHM(t, 1002) + + const victim = uint32(4242) + const attacker = uint32(66615) + + hm.mu.Lock() + hm.trusted[victim] = &TrustRecord{NodeID: victim, ApprovedAt: time.Now()} + hm.mu.Unlock() + + payload := attackPayload(t, &HandshakeMsg{ + Type: HandshakeRevoke, + NodeID: victim, + Reason: "trust revoked", + Timestamp: time.Now().Unix(), + }) + + hm.handleConnection(newWireStream(payload, attacker)) + + assertTrusted(t, hm, victim, "bare revoke over the wire") + + // The revoke handler also tears the tunnel down; a rejected revoke + // must not have reached that side effect either. + rt.removedPeersMu.Lock() + removed := append([]uint32(nil), rt.removedPeers...) + rt.removedPeersMu.Unlock() + for _, id := range removed { + if id == victim { + t.Fatalf("ATTACK SUCCEEDED (bare revoke): tunnel to victim %d was torn down", victim) + } + } +} + +// TestAttackReplay_SignedAcceptWithAttackerKey is the upgraded form of +// the attack: rather than a bare message, the attacker mints their own +// ed25519 identity and produces a *cryptographically valid* signature +// over the challenge for the victim's node ID. With no registry record +// to contradict it, signature verification passes — so the only thing +// standing between the attacker and injected trust is the +// accept/revoke-to-transport-identity binding. +func TestAttackReplay_SignedAcceptWithAttackerKey(t *testing.T) { + t.Parallel() + const self = uint32(1003) + hm, _ := newBoundHM(t, self) + + const victim = uint32(777) + const attacker = uint32(66616) + + id, err := crypto.GenerateIdentity() + if err != nil { + t.Fatalf("generate attacker identity: %v", err) + } + challenge := fmt.Sprintf("handshake:%d:%d", victim, self) + sig := ed25519.Sign(id.PrivateKey, []byte(challenge)) + + payload := attackPayload(t, &HandshakeMsg{ + Type: HandshakeAccept, + NodeID: victim, + PublicKey: crypto.EncodePublicKey(id.PublicKey), + Signature: base64.StdEncoding.EncodeToString(sig), + Timestamp: time.Now().Unix(), + }) + + hm.handleConnection(newWireStream(payload, attacker)) + + assertNotTrusted(t, hm, victim, "self-signed accept impersonating a victim node ID") +} + +// TestAttackReplay_SignedRevokeWithAttackerKey is the same escalation +// applied to revoke: a valid signature over an attacker-owned key must +// not let them tear down someone else's trust record. +func TestAttackReplay_SignedRevokeWithAttackerKey(t *testing.T) { + t.Parallel() + const self = uint32(1004) + hm, _ := newBoundHM(t, self) + + const victim = uint32(778) + const attacker = uint32(66617) + + hm.mu.Lock() + hm.trusted[victim] = &TrustRecord{NodeID: victim, ApprovedAt: time.Now()} + hm.mu.Unlock() + + id, err := crypto.GenerateIdentity() + if err != nil { + t.Fatalf("generate attacker identity: %v", err) + } + challenge := fmt.Sprintf("handshake:%d:%d", victim, self) + sig := ed25519.Sign(id.PrivateKey, []byte(challenge)) + + payload := attackPayload(t, &HandshakeMsg{ + Type: HandshakeRevoke, + NodeID: victim, + PublicKey: crypto.EncodePublicKey(id.PublicKey), + Signature: base64.StdEncoding.EncodeToString(sig), + Timestamp: time.Now().Unix(), + }) + + hm.handleConnection(newWireStream(payload, attacker)) + + assertTrusted(t, hm, victim, "self-signed revoke impersonating a victim node ID") +} + +// TestAttackReplay_AcceptWithNoAuthenticatedTransport covers the +// registry-relay / no-stream shape: an accept that arrives with no +// authenticated transport underneath it has nothing to bind the claimed +// node ID to, so it must be dropped rather than defaulting to trust. +func TestAttackReplay_AcceptWithNoAuthenticatedTransport(t *testing.T) { + t.Parallel() + hm, _ := newBoundHM(t, 1005) + + for _, typ := range []string{HandshakeAccept, HandshakeRevoke} { + hm.processMessage(nil, &HandshakeMsg{ + Type: typ, + NodeID: 999, + Timestamp: time.Now().Unix(), + }) + } + assertNotTrusted(t, hm, 999, "accept with nil stream") +} + +// TestAttackReplay_RelayedApprovalWithoutOutgoing is the second pentest +// path: rather than a direct stream, the attacker gets the registry to +// relay an approval for a peer the victim never handshaked with. Without +// a matching outgoing request the approval has no precondition to +// satisfy and must be dropped (PPA-002). +func TestAttackReplay_RelayedApprovalWithoutOutgoing(t *testing.T) { + t.Parallel() + hm := newTestHM(t, "") + t.Cleanup(hm.Stop) + + for peer := uint32(50000); peer < 50032; peer++ { + hm.ProcessRelayedApproval(peer) + } + + hm.mu.RLock() + n := len(hm.trusted) + hm.mu.RUnlock() + if n != 0 { + t.Fatalf("ATTACK SUCCEEDED (relayed approval without outgoing): %d trust records injected", n) + } +} + +// TestAttackReplay_RelayedApprovalDuringRevokeCooldown checks the +// stale-approval replay: after a local revoke, an approval still sitting +// in the registry inbox (or replayed by an attacker) must not resurrect +// the trust record during the cooldown window, even when an outgoing +// request exists. +func TestAttackReplay_RelayedApprovalDuringRevokeCooldown(t *testing.T) { + t.Parallel() + hm := newTestHM(t, "") + t.Cleanup(hm.Stop) + + const peer = uint32(8181) + hm.mu.Lock() + hm.outgoing[peer] = time.Now() + hm.revoked[peer] = time.Now().Add(5 * time.Minute) + hm.mu.Unlock() + + hm.ProcessRelayedApproval(peer) + + assertNotTrusted(t, hm, peer, "relayed approval replayed inside the revoke cooldown") +} diff --git a/zz_fuzz_process_message_test.go b/zz_fuzz_process_message_test.go new file mode 100644 index 0000000..75dfc91 --- /dev/null +++ b/zz_fuzz_process_message_test.go @@ -0,0 +1,173 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package handshake + +import ( + "crypto/ed25519" + "encoding/base64" + "encoding/json" + "fmt" + "testing" + "time" + + "github.com/pilot-protocol/common/crypto" +) + +// Port 444 accepts one JSON message per connection from any node that can +// reach it — no prior trust required, which is the whole point of a +// handshake. Everything in HandshakeMsg is attacker-chosen, including the +// base64 public_key and signature that feed the Ed25519 verify. + +// fuzzHM builds a Manager over a fake runtime with a registry whose Lookup +// returns three deliberately different shapes, so a fuzzed node_id can land +// on each branch of the registry-binding check: +// +// node 1 — a well-formed registered key (pubkey-match branch) +// node 2 — a registered key that is valid base64 but the wrong length +// node 3 — a registered key that is not valid base64 at all +// +// Nodes 2 and 3 are the interesting ones: verifyKey is taken from the +// registry response and handed to the base64 decoder and then to the +// signature verify. +func fuzzHM(tb testing.TB) (*Manager, ed25519.PublicKey) { + tb.Helper() + rt := newFakeRuntime() + id, err := crypto.GenerateIdentity() + if err != nil { + tb.Fatalf("identity: %v", err) + } + rt.identity = id + rt.nodeID = 4242 + + good := crypto.EncodePublicKey(id.PublicKey) + rt.registry.setLookup(1, map[string]interface{}{"public_key": good}) + rt.registry.setLookup(2, map[string]interface{}{ + "public_key": base64.StdEncoding.EncodeToString(make([]byte, 7)), + }) + rt.registry.setLookup(3, map[string]interface{}{"public_key": "!!!not-base64!!!"}) + + hm := NewManager(rt) + tb.Cleanup(hm.Stop) + return hm, id.PublicKey +} + +// FuzzHandshakeConnection drives raw connection bytes through the exact path +// a remote peer reaches: bounded read, json.Unmarshal into HandshakeMsg, then +// processMessage. Seeds cover each message type plus the malformed-base64 and +// wrong-length key shapes explicitly. +func FuzzHandshakeConnection(f *testing.F) { + hm, pub := fuzzHM(f) + + add := func(v map[string]interface{}) { + b, err := json.Marshal(v) + if err == nil { + f.Add(b) + } + } + + now := time.Now().Unix() + goodKey := crypto.EncodePublicKey(pub) + + for _, msgType := range []string{ + HandshakeRequest, HandshakeAccept, HandshakeReject, HandshakeRevoke, "", "bogus", + } { + add(map[string]interface{}{ + "type": msgType, "node_id": 1, "public_key": goodKey, + "signature": base64.StdEncoding.EncodeToString(make([]byte, ed25519.SignatureSize)), + "timestamp": now, + }) + } + + // Wrong-length key material on both the key and the signature — the + // exact shape that panics an unguarded ed25519.Verify. + for _, n := range []int{0, 1, 7, 31, 33, 63, 64, 65, 128} { + add(map[string]interface{}{ + "type": HandshakeRequest, "node_id": 9, + "public_key": base64.StdEncoding.EncodeToString(make([]byte, n)), + "signature": base64.StdEncoding.EncodeToString(make([]byte, n)), + "timestamp": now, + }) + } + + // Registry-supplied keys that are malformed (nodes 2 and 3). + for _, nodeID := range []int{1, 2, 3} { + add(map[string]interface{}{ + "type": HandshakeRequest, "node_id": nodeID, "public_key": goodKey, + "signature": base64.StdEncoding.EncodeToString(make([]byte, ed25519.SignatureSize)), + "timestamp": now, + }) + } + + // Non-base64 key/signature, missing fields, extreme timestamps. + add(map[string]interface{}{"type": HandshakeRequest, "node_id": 1, "public_key": "@@@@", "signature": "@@@@", "timestamp": now}) + add(map[string]interface{}{"type": HandshakeRequest, "node_id": 1, "timestamp": now}) + add(map[string]interface{}{"type": HandshakeAccept, "node_id": 1, "public_key": goodKey, "signature": "", "timestamp": now}) + add(map[string]interface{}{"type": HandshakeRequest, "node_id": 1, "public_key": goodKey, "timestamp": int64(1) << 62}) + add(map[string]interface{}{"type": HandshakeRequest, "node_id": 1, "public_key": goodKey, "timestamp": -(int64(1) << 62)}) + add(map[string]interface{}{ + "type": HandshakeRequest, "node_id": 1, "public_key": goodKey, + "justification": string(make([]byte, 4096)), "timestamp": now, + }) + + // Structural junk the JSON decoder must reject rather than crash on. + f.Add([]byte("")) + f.Add([]byte("{")) + f.Add([]byte("null")) + f.Add([]byte("[]")) + f.Add([]byte(`{"node_id":99999999999999999999}`)) + f.Add([]byte(`{"type":123}`)) + + f.Fuzz(func(t *testing.T, data []byte) { + if len(data) > 64*1024 { + data = data[:64*1024] + } + // handleConnection has its own recover; call it so the bounded-read + // and decode framing are covered, then assert no panic escaped by + // re-running the decoded message through processMessage directly. + hm.handleConnection(newMockStreamData(data)) + + var msg HandshakeMsg + if err := json.Unmarshal(data, &msg); err != nil { + return + } + // Fresh timestamp on half the iterations so the age gate does not + // short-circuit every input before the signature path runs. + msg.Timestamp = time.Now().Unix() + hm.processMessage(newMockStreamData(nil), &msg) + }) +} + +// FuzzHandshakeProcessMessage builds the message struct from fuzzer-chosen +// fields instead of going through JSON, so the fuzzer spends its budget on +// the crypto/validation branches rather than on rediscovering JSON syntax. +func FuzzHandshakeProcessMessage(f *testing.F) { + hm, pub := fuzzHM(f) + goodKey := crypto.EncodePublicKey(pub) + + f.Add(HandshakeRequest, uint32(1), goodKey, "") + f.Add(HandshakeRequest, uint32(2), goodKey, "AAAA") + f.Add(HandshakeRequest, uint32(3), goodKey, "AAAA") + f.Add(HandshakeAccept, uint32(1), "", "") + f.Add(HandshakeRevoke, uint32(1), goodKey, "@@") + f.Add("", uint32(0), "", "") + f.Add(HandshakeReject, uint32(0xFFFFFFFF), base64.StdEncoding.EncodeToString(make([]byte, 31)), + base64.StdEncoding.EncodeToString(make([]byte, 63))) + + f.Fuzz(func(t *testing.T, msgType string, nodeID uint32, pubKeyB64, sigB64 string) { + if len(pubKeyB64) > 8192 || len(sigB64) > 8192 || len(msgType) > 1024 { + return + } + msg := &HandshakeMsg{ + Type: msgType, + NodeID: nodeID, + PublicKey: pubKeyB64, + Signature: sigB64, + Timestamp: time.Now().Unix(), + // Vary the justification so the replay-set hash differs per + // iteration; otherwise every input after the first is dropped + // by the replay guard. + Justification: fmt.Sprintf("%d", time.Now().UnixNano()), + } + hm.processMessage(newMockStreamData(nil), msg) + }) +} diff --git a/zz_handshake_accept_request_test.go b/zz_handshake_accept_request_test.go index 5094c6b..15f0ee5 100644 --- a/zz_handshake_accept_request_test.go +++ b/zz_handshake_accept_request_test.go @@ -71,16 +71,19 @@ func TestHandleRequestAlreadyTrustedSendsAcceptWithoutDuplicating(t *testing.T) msg := &HandshakeMsg{ Type: HandshakeRequest, NodeID: 42, - PublicKey: "new-key-should-be-ignored", + PublicKey: "original-key", Timestamp: time.Now().Unix(), } hm.handleRequest(nil, msg, false) // conn is unused in body - // PublicKey should NOT have been overwritten (handleRequest for trusted peers - // is a no-op beyond emitting sendAcceptLocked). + // The record is left as-is (handleRequest for a trusted peer whose + // key still matches is a no-op beyond emitting sendAcceptLocked). hm.mu.RLock() rec := hm.trusted[42] hm.mu.RUnlock() + if rec == nil { + t.Fatal("trusted[42] dropped for a request carrying the bound key") + } if rec.PublicKey != "original-key" { t.Fatalf("trusted[42].PublicKey mutated: got %q, want 'original-key'", rec.PublicKey) } @@ -89,6 +92,73 @@ func TestHandleRequestAlreadyTrustedSendsAcceptWithoutDuplicating(t *testing.T) } } +// A request for an already-trusted node ID that carries a different key +// is a different key holder behind a recycled node ID. The record is +// dropped and the request falls through to the normal approval flow. +func TestHandleRequestTrustedNodeWithDifferentKeyDropsRecord(t *testing.T) { + t.Parallel() + hm, _ := hsTestManager(t, false) + defer waitForGoRPCDrain() + + hm.trusted[42] = &TrustRecord{ + NodeID: 42, + PublicKey: "original-key", + ApprovedAt: time.Now().Add(-1 * time.Hour), + } + + msg := &HandshakeMsg{ + Type: HandshakeRequest, + NodeID: 42, + PublicKey: "reclaimed-key", + Timestamp: time.Now().Unix(), + } + hm.handleRequest(nil, msg, false) + + hm.mu.RLock() + _, stillTrusted := hm.trusted[42] + _, nowPending := hm.pending[42] + hm.mu.RUnlock() + + if stillTrusted { + t.Fatal("trusted[42] survived a request carrying a different public key") + } + if !nowPending { + t.Fatal("request from the new key holder should be queued for approval") + } +} + +// A record persisted before keys were bound (or one whose registry +// backfill never landed) keeps working, and adopts the first key it +// sees. +func TestHandleRequestUnboundLegacyRecordAdoptsKey(t *testing.T) { + t.Parallel() + hm, _ := hsTestManager(t, false) + defer waitForGoRPCDrain() + + hm.trusted[42] = &TrustRecord{ + NodeID: 42, + ApprovedAt: time.Now().Add(-1 * time.Hour), + } + + msg := &HandshakeMsg{ + Type: HandshakeRequest, + NodeID: 42, + PublicKey: "first-seen-key", + Timestamp: time.Now().Unix(), + } + hm.handleRequest(nil, msg, false) + + hm.mu.RLock() + rec := hm.trusted[42] + hm.mu.RUnlock() + if rec == nil { + t.Fatal("legacy unbound record was dropped — upgrades must not break existing trust") + } + if rec.PublicKey != "first-seen-key" { + t.Fatalf("legacy record did not adopt the presented key: got %q", rec.PublicKey) + } +} + // --- handleRequest: mutual auto-approve (outgoing[peer]=true) --- func TestHandleRequestMutualAutoApprovesAndMarksMutual(t *testing.T) { diff --git a/zz_trust_key_binding_test.go b/zz_trust_key_binding_test.go new file mode 100644 index 0000000..22e884f --- /dev/null +++ b/zz_trust_key_binding_test.go @@ -0,0 +1,262 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package handshake + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +const ( + bindKeyA = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAE=" + bindKeyB = "QkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkJCQkI=" +) + +func seedTrusted(hm *Manager, nodeID uint32, key string) { + hm.mu.Lock() + hm.trusted[nodeID] = &TrustRecord{ + NodeID: nodeID, + PublicKey: key, + ApprovedAt: time.Now().Add(-time.Hour), + } + hm.mu.Unlock() +} + +func trustRecordOf(hm *Manager, nodeID uint32) *TrustRecord { + hm.mu.RLock() + defer hm.mu.RUnlock() + return hm.trusted[nodeID] +} + +// Trust granted against key A stays valid for key A. +func TestIsTrustedWithKeySameKeyStaysTrusted(t *testing.T) { + t.Parallel() + hm, _ := newFakeHM(t) + + seedTrusted(hm, 42, bindKeyA) + + if !hm.IsTrustedWithKey(42, bindKeyA) { + t.Fatal("peer presenting the bound key should stay trusted") + } + if rec := trustRecordOf(hm, 42); rec == nil || rec.PublicKey != bindKeyA { + t.Fatalf("record mutated on a matching check: %+v", rec) + } +} + +// The same node ID presenting a different key is not trusted, and the +// stale record is dropped rather than left to match again later. +func TestIsTrustedWithKeyDifferentKeyNotTrusted(t *testing.T) { + t.Parallel() + hm, _ := newFakeHM(t) + + seedTrusted(hm, 42, bindKeyA) + + if hm.IsTrustedWithKey(42, bindKeyB) { + t.Fatal("peer presenting a different key must not be trusted") + } + if rec := trustRecordOf(hm, 42); rec != nil { + t.Fatal("stale trust record should have been dropped") + } + // And it stays dropped for the original key holder too. + if hm.IsTrustedWithKey(42, bindKeyA) { + t.Fatal("dropped record should not resurrect for the original key") + } +} + +// Legacy records — persisted before keys were bound — keep working, so +// an upgrade does not untrust anyone. +func TestIsTrustedWithKeyLegacyUnboundRecordStaysTrusted(t *testing.T) { + t.Parallel() + hm, _ := newFakeHM(t) + + seedTrusted(hm, 42, "") + + if !hm.IsTrustedWithKey(42, bindKeyA) { + t.Fatal("legacy unbound record must remain trusted") + } + if rec := trustRecordOf(hm, 42); rec == nil || rec.PublicKey != bindKeyA { + t.Fatalf("legacy record should have adopted the presented key: %+v", rec) + } + // Having adopted key A, it now rejects a different key. + if hm.IsTrustedWithKey(42, bindKeyB) { + t.Fatal("record should be bound after adoption") + } +} + +// A call site with no key in scope degrades to the node-ID-only answer +// rather than denying. +func TestIsTrustedWithKeyEmptyKeyFallsBackToNodeID(t *testing.T) { + t.Parallel() + hm, _ := newFakeHM(t) + + seedTrusted(hm, 42, bindKeyA) + + if !hm.IsTrustedWithKey(42, "") { + t.Fatal("empty presented key should fall back to node-ID-only trust") + } + if hm.IsTrustedWithKey(43, "") { + t.Fatal("untrusted node must stay untrusted regardless of key") + } + if rec := trustRecordOf(hm, 42); rec.PublicKey != bindKeyA { + t.Fatalf("binding must not be cleared by a keyless check, got %q", rec.PublicKey) + } +} + +// IsTrusted keeps its node-ID-only contract for callers with no key. +func TestIsTrustedUnchangedForNodeIDOnlyCallers(t *testing.T) { + t.Parallel() + hm, _ := newFakeHM(t) + + seedTrusted(hm, 42, bindKeyA) + + if !hm.IsTrusted(42) { + t.Fatal("IsTrusted should still answer on node ID alone") + } + if hm.IsTrusted(43) { + t.Fatal("IsTrusted should be false for an unknown node") + } +} + +// A node ID reaped from the registry and re-claimed by a different +// identity does not inherit the previous holder's trust: backfill sees +// the registry key diverge from the bound key and drops the record. +func TestBackfillPeerKeyReclaimedNodeIDLosesTrust(t *testing.T) { + t.Parallel() + hm, rt := newFakeHM(t) + rt.registry.setLookup(42, map[string]interface{}{"public_key": bindKeyB}) + + seedTrusted(hm, 42, bindKeyA) + hm.backfillPeerKey(42) + + if rec := trustRecordOf(hm, 42); rec != nil { + t.Fatalf("trust must not survive the node ID being re-claimed by another key: %+v", rec) + } +} + +// The same path binds a relay-established record once the registry +// answers, and leaves a matching one alone. +func TestBackfillPeerKeyBindsThenValidates(t *testing.T) { + t.Parallel() + hm, rt := newFakeHM(t) + rt.registry.setLookup(42, map[string]interface{}{"public_key": bindKeyA}) + + seedTrusted(hm, 42, "") + hm.backfillPeerKey(42) + + if rec := trustRecordOf(hm, 42); rec == nil || rec.PublicKey != bindKeyA { + t.Fatalf("relay record should have been bound to the registry key: %+v", rec) + } + + hm.backfillPeerKey(42) + if rec := trustRecordOf(hm, 42); rec == nil || rec.PublicKey != bindKeyA { + t.Fatalf("a matching backfill must be a no-op: %+v", rec) + } +} + +// An accept carrying a different key than the record we hold does not +// silently rebind it. +func TestHandleAcceptDifferentKeyDropsRecord(t *testing.T) { + t.Parallel() + hm, _ := newFakeHM(t) + + seedTrusted(hm, 42, bindKeyA) + hm.mu.Lock() + hm.outgoing[42] = time.Now() + hm.mu.Unlock() + + hm.handleAccept(&HandshakeMsg{ + Type: HandshakeAccept, + NodeID: 42, + PublicKey: bindKeyB, + Timestamp: time.Now().Unix(), + }) + + if rec := trustRecordOf(hm, 42); rec != nil { + t.Fatalf("accept from a different key holder must not keep or rebind the record: %+v", rec) + } +} + +// An accept that omits the key leaves an existing binding intact rather +// than clearing it. +func TestHandleAcceptWithoutKeyPreservesBinding(t *testing.T) { + t.Parallel() + hm, _ := newFakeHM(t) + + seedTrusted(hm, 42, bindKeyA) + + hm.handleAccept(&HandshakeMsg{ + Type: HandshakeAccept, + NodeID: 42, + Timestamp: time.Now().Unix(), + }) + + rec := trustRecordOf(hm, 42) + if rec == nil { + t.Fatal("keyless accept should not drop the record") + } + if rec.PublicKey != bindKeyA { + t.Fatalf("binding cleared by a keyless accept, got %q", rec.PublicKey) + } +} + +// Snapshots written before public keys were recorded load as unbound +// records, and unbound records still count as trusted. +func TestLoadTrustSnapshotWithoutPublicKeyStaysTrusted(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + path := filepath.Join(dir, "trust.json") + legacy := `{"trusted":[{"node_id":42,"approved_at":"` + + time.Now().Add(-time.Hour).Format(time.RFC3339) + `","mutual":true}]}` + if err := os.WriteFile(path, []byte(legacy), 0o600); err != nil { + t.Fatalf("write legacy snapshot: %v", err) + } + + // NewManager derives storePath from the identity path's directory + // and loads the snapshot during construction. + hm := newTestHM(t, path) + t.Cleanup(hm.Stop) + + rec := trustRecordOf(hm, 42) + if rec == nil { + t.Fatal("legacy snapshot entry did not load") + } + if rec.PublicKey != "" { + t.Fatalf("expected an unbound record, got key %q", rec.PublicKey) + } + // Checked with no key in scope, so nothing is adopted and no + // background save is triggered into the temp dir. + if !hm.IsTrusted(42) || !hm.IsTrustedWithKey(42, "") { + t.Fatal("legacy snapshot entry must remain trusted after upgrade") + } +} + +// A bound key survives a save/load round trip, so the binding is not +// re-learned from scratch on every restart. +func TestSaveLoadTrustPreservesBoundKey(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + path := filepath.Join(dir, "trust.json") + + hm := newTestHM(t, path) + t.Cleanup(hm.Stop) + seedTrusted(hm, 42, bindKeyA) + hm.saveTrust() // synchronous; no drain goroutine involved + + hm2 := newTestHM(t, path) + t.Cleanup(hm2.Stop) + + rec := trustRecordOf(hm2, 42) + if rec == nil { + t.Fatal("record missing after round trip") + } + if rec.PublicKey != bindKeyA { + t.Fatalf("bound key not persisted: got %q, want %q", rec.PublicKey, bindKeyA) + } + if !hm2.IsTrustedWithKey(42, bindKeyA) { + t.Fatal("reloaded record should still accept the bound key") + } +} diff --git a/zz_trustedagents_test.go b/zz_trustedagents_test.go index 47fba5b..29cc71c 100644 --- a/zz_trustedagents_test.go +++ b/zz_trustedagents_test.go @@ -177,7 +177,7 @@ func TestHandleRequestTrustedAgentAlreadyTrustedNoRewrite(t *testing.T) { msg := &HandshakeMsg{ Type: HandshakeRequest, NodeID: trustedNodeID, - PublicKey: "new-pubkey-must-be-ignored", + PublicKey: originalKey, Timestamp: time.Now().Unix(), } hm.handleRequest(nil, msg, true) @@ -186,6 +186,9 @@ func TestHandleRequestTrustedAgentAlreadyTrustedNoRewrite(t *testing.T) { rec := hm.trusted[trustedNodeID] hm.mu.RUnlock() + if rec == nil { + t.Fatal("trust record dropped for a request carrying the bound key") + } if rec.PublicKey != originalKey { t.Fatalf("PublicKey overwritten: got %q, want %q", rec.PublicKey, originalKey) } @@ -193,3 +196,46 @@ func TestHandleRequestTrustedAgentAlreadyTrustedNoRewrite(t *testing.T) { t.Fatalf("ApprovedAt overwritten: got %v, want %v", rec.ApprovedAt, originalAt) } } + +// A trusted-agents node ID that turns up under a different key does not +// carry the old record forward: it is dropped, and the allowlist + +// registry binding then re-grants trust bound to the new key. The +// re-grant is what makes the entry an allowlist; what must not happen +// is the old record surviving with a stale key. +func TestHandleRequestTrustedAgentDifferentKeyRebinds(t *testing.T) { + setTestTrustedAgents(t, []testAgent{ + {Hostname: "list-agents", NodeID: trustedNodeID}, + }) + + hm, _ := hsTestManager(t, false) + defer waitForGoRPCDrain() + + originalAt := time.Now().Add(-1 * time.Hour) + hm.trusted[trustedNodeID] = &TrustRecord{ + NodeID: trustedNodeID, + PublicKey: "original-pubkey", + ApprovedAt: originalAt, + } + + msg := &HandshakeMsg{ + Type: HandshakeRequest, + NodeID: trustedNodeID, + PublicKey: "reclaimed-pubkey", + Timestamp: time.Now().Unix(), + } + hm.handleRequest(nil, msg, true) + + hm.mu.RLock() + rec := hm.trusted[trustedNodeID] + hm.mu.RUnlock() + + if rec == nil { + t.Fatal("allowlisted node should be re-granted trust under its new key") + } + if rec.PublicKey != "reclaimed-pubkey" { + t.Fatalf("stale key survived: got %q, want %q", rec.PublicKey, "reclaimed-pubkey") + } + if !rec.ApprovedAt.After(originalAt) { + t.Fatalf("ApprovedAt not refreshed: got %v, want after %v", rec.ApprovedAt, originalAt) + } +}