From d23cd046dea58fa2b8a7460b114dde49797ebd81 Mon Sep 17 00:00:00 2001 From: Teodor Date: Mon, 22 Jun 2026 14:40:32 +0300 Subject: [PATCH] Add optional Ed25519 pubkey pinning to trusted-agents (H4) Each agent entry may carry an optional base64 public_key that pins the node_id to a specific Ed25519 key. IsTrustedWithKey enforces the pin with a constant-time compare when present and falls back to node_id-only trust when absent, so the change is backward-compatible with every entry shipped today. IsTrusted is unchanged for key-less callers. Closes audit finding H4 (node_id-only trust lets a takeover of any trusted node_id inherit auto-approve). Inbound auto-accept enforcement needs upstream wiring; documented as a TODO at the Service call site. --- CHANGELOG.md | 11 +++ README.md | 30 ++++++- data.go | 116 +++++++++++++++++++++++--- service.go | 26 ++++++ service_disabled.go | 4 + zz_pubkey_pin_test.go | 185 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 357 insertions(+), 15 deletions(-) create mode 100644 zz_pubkey_pin_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e1da867..e74cf30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ All notable changes to this project are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added + +- Optional per-entry `public_key` (base64 Ed25519) pinning a trusted + `node_id` to a specific key. `IsTrustedWithKey(nodeID, pubKey)` + enforces the pin with a constant-time compare when present, and falls + back to `node_id`-only trust when absent (backward-compatible — every + shipped entry is unpinned). `IsTrusted(nodeID)` is unchanged. + Addresses audit finding H4. + ## [v0.1.0] Initial release. diff --git a/README.md b/README.md index ef75a21..ac2c1d9 100644 --- a/README.md +++ b/README.md @@ -18,20 +18,46 @@ import "github.com/pilot-protocol/trustedagents" ## Usage ```go -// Lookup: +// Lookup (node_id only): name, ok := trustedagents.IsTrusted(nodeID) _ = name _ = ok +// Lookup with pubkey pin enforcement (preferred when the +// authenticated peer key is in scope, e.g. inbound handshake): +name, ok = trustedagents.IsTrustedWithKey(nodeID, peerPubKey) + // Daemon registration: rt.Register(trustedagents.NewService()) ``` +## Optional pubkey pinning + +Each entry may carry an optional `public_key` (base64 std-encoded +Ed25519) that pins the `node_id` to a specific key: + +```json +{ "hostname": "list-agents", "node_id": 14161, "public_key": "BASE64_ED25519_PUBKEY" } +``` + +`IsTrustedWithKey(nodeID, peerPubKey)` enforces the binding: if an entry +has a `public_key`, the authenticated peer key must match it +(constant-time compare) or the peer is **not** trusted. Entries without a +`public_key` — every entry shipped today — fall back to `node_id`-only +trust, so adding pins is fully backward-compatible. `IsTrusted(nodeID)` +is unchanged and still answers the key-less question for callers with no +peer key in scope. + +This closes audit finding **H4**: without a pin, taking over any trusted +`node_id` (or a registry mapping a trusted `node_id` to an attacker key) +inherits full auto-approve trust. Enforcement at the inbound auto-accept +path requires upstream wiring — see the TODO on `Service.IsTrustedWithKey`. + ## Layout | File | What it does | |---|---| -| `data.go` | Embedded JSON list. `Load`, `All`, `IsTrusted(nodeID) → (description, ok)`, `SetForTest`. | +| `data.go` | Embedded JSON list. `Load`, `All`, `IsTrusted(nodeID) → (name, ok)`, `IsTrustedWithKey(nodeID, pubKey) → (name, ok)`, `SetForTest`. | | `runtime.go` | `Run(ctx)` — periodic fetcher over HTTPS to `raw.githubusercontent.com`. | | `service.go` | `*Service` — `coreapi.Service` adapter (`Name/Order/Start/Stop` + `IsTrusted`). Build tag `!no_trustedagents`. | | `service_disabled.go` | Stub `*Service` when build tag `no_trustedagents` is set. | diff --git a/data.go b/data.go index 578487b..2ecedd4 100644 --- a/data.go +++ b/data.go @@ -18,8 +18,9 @@ package trustedagents import ( "crypto/ed25519" - "encoding/base64" + "crypto/subtle" _ "embed" + "encoding/base64" "encoding/json" "fmt" "log/slog" @@ -30,10 +31,20 @@ import ( // Hostname and Address are kept for logs and `pilotctl trusted list`. // Other JSON fields in the source file (tier, description, ...) are // silently ignored on unmarshal — we don't care about them at runtime. +// +// PublicKey is OPTIONAL: a base64 (std encoding) Ed25519 public key +// pinning the node_id to a specific key. When present, the inbound +// auto-accept path MUST verify the authenticated peer's key equals it +// (see IsTrustedWithKey). When absent — as for every entry shipped +// today — trust falls back to node_id alone, preserving current +// behavior. Pinning closes audit finding H4: without it, taking over a +// trusted node_id (or a registry that maps a trusted node_id to an +// attacker key) inherits full auto-approve trust. type Agent struct { - Hostname string `json:"hostname"` - Address string `json:"address"` - NodeID uint32 `json:"node_id"` + Hostname string `json:"hostname"` + Address string `json:"address"` + NodeID uint32 `json:"node_id"` + PublicKey string `json:"public_key,omitempty"` } //go:embed trusted-agents.json @@ -48,19 +59,45 @@ func EmbeddedJSON() []byte { return out } +// entry is the in-memory form of a trusted agent: the display name plus +// the optional decoded Ed25519 pin. pubKey is nil when the source entry +// had no public_key (the unpinned, node_id-only case). +type entry struct { + name string + pubKey ed25519.PublicKey // nil == unpinned +} + var ( mu sync.RWMutex - byNode map[uint32]string // node_id -> name + byNode map[uint32]entry // node_id -> entry all []Agent ) +// decodePin parses an Agent.PublicKey field into an ed25519.PublicKey. +// Empty string → (nil, nil): unpinned, not an error. A non-empty value +// that is not valid base64 or not 32 bytes is an error so a malformed +// pin fails the whole Load rather than silently degrading to unpinned. +func decodePin(b64 string) (ed25519.PublicKey, error) { + if b64 == "" { + return nil, nil + } + raw, err := base64.StdEncoding.DecodeString(b64) + if err != nil { + return nil, fmt.Errorf("public_key: bad base64: %w", err) + } + if len(raw) != ed25519.PublicKeySize { + return nil, fmt.Errorf("public_key: want %d bytes, got %d", ed25519.PublicKeySize, len(raw)) + } + return ed25519.PublicKey(raw), nil +} + func init() { if err := Load(embeddedJSON); err != nil { // CI guards this via TestEmbeddedListLoads; if it ever fires in // production, an empty list (zero auto-accepts) is the safe default. slog.Error("trustedagents: embedded list malformed", "err", err) mu.Lock() - byNode = map[uint32]string{} + byNode = map[uint32]entry{} mu.Unlock() } } @@ -68,18 +105,63 @@ func init() { // IsTrusted reports whether nodeID is in the trusted-agents list. The // caller MUST verify the (node_id, public_key) binding at the registry // before acting on a true result — this package only checks the list. +// +// IsTrusted does NOT consult the optional per-entry pubkey pin: it +// answers the node_id-only question for callers that have no +// authenticated key in scope. Callers that DO have the authenticated +// peer key (e.g. the inbound handshake auto-accept path) MUST prefer +// IsTrustedWithKey so a present pin is enforced. func IsTrusted(nodeID uint32) (string, bool) { mu.RLock() defer mu.RUnlock() - name, ok := byNode[nodeID] - return name, ok + e, ok := byNode[nodeID] + if !ok { + return "", false + } + return e.name, true +} + +// IsTrustedWithKey reports whether nodeID is trusted given the +// authenticated peer's Ed25519 public key. +// +// - nodeID not in the list → ("", false) +// - entry HAS a pinned PublicKey → trusted ONLY if pubKey equals +// the pin (constant-time compare). A mismatch — or an empty/short +// pubKey when a pin is required — is ("", false). +// - entry has NO pin (every entry today) → trusted by node_id alone, +// preserving IsTrusted's behavior. The unpinned match is logged at +// debug so finding-H4 exposure is observable until pins are added. +// +// Pass the AUTHENTICATED key (the one the peer proved possession of in +// the handshake), never an unverified claim — otherwise the pin adds +// nothing. +func IsTrustedWithKey(nodeID uint32, pubKey []byte) (string, bool) { + mu.RLock() + defer mu.RUnlock() + e, ok := byNode[nodeID] + if !ok { + return "", false + } + if e.pubKey == nil { + // Unpinned: backward-compatible node_id-only trust. + slog.Debug("trustedagents: trusting unpinned entry by node_id only", + "node_id", nodeID, "agent", e.name) + return e.name, true + } + // Pinned: require an exact, constant-time key match. + if subtle.ConstantTimeCompare(e.pubKey, pubKey) != 1 { + slog.Warn("trustedagents: pubkey pin mismatch — refusing auto-accept", + "node_id", nodeID, "agent", e.name) + return "", false + } + return e.name, true } // SetForTest replaces the active list with agents and returns a restore // function that reloads the embedded list. Test-only — never call from // production code. func SetForTest(agents []Agent) (restore func()) { - idx := make(map[uint32]string, len(agents)) + idx := make(map[uint32]entry, len(agents)) for _, a := range agents { if a.NodeID == 0 { continue @@ -90,7 +172,11 @@ func SetForTest(agents []Agent) (restore func()) { if other, exists := idx[a.NodeID]; exists { panic(fmt.Sprintf("SetForTest: duplicate node_id %d (hostnames %q and %q)", a.NodeID, other, a.Hostname)) } - idx[a.NodeID] = a.Hostname + pin, err := decodePin(a.PublicKey) + if err != nil { + panic(fmt.Sprintf("SetForTest: node_id %d (%q): %v", a.NodeID, a.Hostname, err)) + } + idx[a.NodeID] = entry{name: a.Hostname, pubKey: pin} } mu.Lock() prevByNode, prevAll := byNode, all @@ -197,7 +283,7 @@ func Load(raw []byte) error { if err := json.Unmarshal(raw, &doc); err != nil { return err } - idx := make(map[uint32]string, len(doc.Agents)) + idx := make(map[uint32]entry, len(doc.Agents)) for _, a := range doc.Agents { if a.NodeID == 0 { continue // 0 is reserved / would silently match unset fields @@ -206,9 +292,13 @@ func Load(raw []byte) error { continue // empty hostname: missing required field — drop } if other, exists := idx[a.NodeID]; exists { - return fmt.Errorf("duplicate node_id %d in trusted-agents list: %q and %q", a.NodeID, other, a.Hostname) + return fmt.Errorf("duplicate node_id %d in trusted-agents list: %q and %q", a.NodeID, other.name, a.Hostname) + } + pin, err := decodePin(a.PublicKey) + if err != nil { + return fmt.Errorf("node_id %d (%q): %w", a.NodeID, a.Hostname, err) } - idx[a.NodeID] = a.Hostname + idx[a.NodeID] = entry{name: a.Hostname, pubKey: pin} } mu.Lock() byNode = idx diff --git a/service.go b/service.go index 0045609..d88fde8 100644 --- a/service.go +++ b/service.go @@ -67,3 +67,29 @@ func (s *Service) Stop(ctx context.Context) error { func (s *Service) IsTrusted(nodeID uint32) (string, bool) { return IsTrusted(nodeID) } + +// IsTrustedWithKey is the pubkey-pinned trust gate (audit finding H4). +// It enforces a per-entry Ed25519 pin when one is present and otherwise +// falls back to node_id-only trust. Delegates to the package-global +// allowlist. +// +// TODO(H4 wiring): the inbound auto-accept call site lives in +// protocol/plugins/handshake (handshake.go ~L639), where it calls +// hm.rt.IsTrusted(peerNodeID) via the coreapi.TrustChecker interface. +// The authenticated peer key IS in scope there as msg.PublicKey (it is +// already stored into the TrustRecord). To actually ENFORCE pinning, +// two upstream changes are needed, in this order: +// +// 1. common/coreapi.TrustChecker: add +// IsTrustedWithKey(nodeID uint32, pubKey []byte) (string, bool). +// 2. protocol/plugins/handshake: replace the auto-accept +// hm.rt.IsTrusted(peerNodeID) call with +// hm.rt.IsTrustedWithKey(peerNodeID, msg.PublicKey). +// +// Until those land, this method is callable directly but the daemon's +// handshake path still routes through IsTrusted, so pins are stored and +// validated on Load but not yet enforced at auto-accept. That is safe: +// every shipped entry is unpinned, so behavior is unchanged. +func (s *Service) IsTrustedWithKey(nodeID uint32, pubKey []byte) (string, bool) { + return IsTrustedWithKey(nodeID, pubKey) +} diff --git a/service_disabled.go b/service_disabled.go index e76eb7b..5caf365 100644 --- a/service_disabled.go +++ b/service_disabled.go @@ -33,3 +33,7 @@ func (s *Service) Stop(_ context.Context) error { return nil } // IsTrusted always reports the node as untrusted when the plugin is // disabled. Callers should treat this as a fail-closed default. func (s *Service) IsTrusted(_ uint32) (string, bool) { return "", false } + +// IsTrustedWithKey mirrors IsTrusted's fail-closed behavior: with the +// plugin disabled every peer is untrusted regardless of key. +func (s *Service) IsTrustedWithKey(_ uint32, _ []byte) (string, bool) { return "", false } diff --git a/zz_pubkey_pin_test.go b/zz_pubkey_pin_test.go new file mode 100644 index 0000000..74c1ddc --- /dev/null +++ b/zz_pubkey_pin_test.go @@ -0,0 +1,185 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package trustedagents + +import ( + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "testing" +) + +// newPin generates an Ed25519 keypair and returns the public key both as +// raw bytes (for IsTrustedWithKey) and base64 (for the Agent.PublicKey +// field), matching the std-encoding the loader expects. +func newPin(t *testing.T) (raw ed25519.PublicKey, b64 string) { + t.Helper() + pub, _, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("GenerateKey: %v", err) + } + return pub, base64.StdEncoding.EncodeToString(pub) +} + +// TestIsTrustedWithKey_PinnedCorrectKey: a pinned entry trusts the peer +// when the authenticated key matches the pin. +func TestIsTrustedWithKey_PinnedCorrectKey(t *testing.T) { + raw, b64 := newPin(t) + restore := SetForTest([]Agent{ + {Hostname: "pinned-agent", NodeID: 100, PublicKey: b64}, + }) + t.Cleanup(restore) + + name, ok := IsTrustedWithKey(100, raw) + if !ok || name != "pinned-agent" { + t.Fatalf("IsTrustedWithKey(100, correct) = (%q,%v), want (pinned-agent,true)", name, ok) + } +} + +// TestIsTrustedWithKey_PinnedWrongKey: a pinned entry refuses a peer +// presenting a different key, even though the node_id matches. +func TestIsTrustedWithKey_PinnedWrongKey(t *testing.T) { + _, b64 := newPin(t) + wrong, _ := newPin(t) + restore := SetForTest([]Agent{ + {Hostname: "pinned-agent", NodeID: 100, PublicKey: b64}, + }) + t.Cleanup(restore) + + if name, ok := IsTrustedWithKey(100, wrong); ok { + t.Fatalf("IsTrustedWithKey(100, wrong-key) = (%q,true), want untrusted", name) + } +} + +// TestIsTrustedWithKey_PinnedEmptyKey: a pinned entry refuses an empty +// or short presented key — a missing authenticated key cannot satisfy a +// pin. +func TestIsTrustedWithKey_PinnedEmptyKey(t *testing.T) { + _, b64 := newPin(t) + restore := SetForTest([]Agent{ + {Hostname: "pinned-agent", NodeID: 100, PublicKey: b64}, + }) + t.Cleanup(restore) + + if _, ok := IsTrustedWithKey(100, nil); ok { + t.Fatal("IsTrustedWithKey(100, nil) must not trust a pinned entry") + } + if _, ok := IsTrustedWithKey(100, []byte{1, 2, 3}); ok { + t.Fatal("IsTrustedWithKey(100, short) must not trust a pinned entry") + } +} + +// TestIsTrustedWithKey_UnpinnedBackwardCompat: an entry with no +// PublicKey (the state of every entry shipped today) is trusted by +// node_id alone, regardless of the presented key. This preserves +// current production behavior. +func TestIsTrustedWithKey_UnpinnedBackwardCompat(t *testing.T) { + restore := SetForTest([]Agent{ + {Hostname: "legacy-agent", NodeID: 200}, // no PublicKey + }) + t.Cleanup(restore) + + // Any key — including a random one — is accepted because the entry + // is unpinned. + some, _ := newPin(t) + if name, ok := IsTrustedWithKey(200, some); !ok || name != "legacy-agent" { + t.Fatalf("IsTrustedWithKey(200, anykey) = (%q,%v), want (legacy-agent,true)", name, ok) + } + // Even a nil key is fine for an unpinned entry. + if name, ok := IsTrustedWithKey(200, nil); !ok || name != "legacy-agent" { + t.Fatalf("IsTrustedWithKey(200, nil) = (%q,%v), want (legacy-agent,true)", name, ok) + } +} + +// TestIsTrustedWithKey_UnknownNode: a node_id not on the list is never +// trusted, with or without a key. +func TestIsTrustedWithKey_UnknownNode(t *testing.T) { + restore := SetForTest([]Agent{ + {Hostname: "known", NodeID: 200}, + }) + t.Cleanup(restore) + + some, _ := newPin(t) + if _, ok := IsTrustedWithKey(999, some); ok { + t.Fatal("IsTrustedWithKey(999, ...) must not trust an unknown node") + } +} + +// TestIsTrusted_UnchangedByPin: the existing key-less IsTrusted keeps +// working for both pinned and unpinned entries — it answers the +// node_id-only question and does not consult the pin. +func TestIsTrusted_UnchangedByPin(t *testing.T) { + _, b64 := newPin(t) + restore := SetForTest([]Agent{ + {Hostname: "pinned-agent", NodeID: 100, PublicKey: b64}, + {Hostname: "legacy-agent", NodeID: 200}, + }) + t.Cleanup(restore) + + if name, ok := IsTrusted(100); !ok || name != "pinned-agent" { + t.Fatalf("IsTrusted(100) = (%q,%v), want (pinned-agent,true)", name, ok) + } + if name, ok := IsTrusted(200); !ok || name != "legacy-agent" { + t.Fatalf("IsTrusted(200) = (%q,%v), want (legacy-agent,true)", name, ok) + } + if _, ok := IsTrusted(999); ok { + t.Fatal("IsTrusted(999): unknown node must not match") + } +} + +// TestLoad_PinnedEntry: a public_key in the JSON round-trips through +// Load into an enforced pin. +func TestLoad_PinnedEntry(t *testing.T) { + restore := SetForTest(nil) + t.Cleanup(restore) + + raw, b64 := newPin(t) + doc := []byte(`{"agents":[{"hostname":"x","node_id":1,"public_key":"` + b64 + `"}]}`) + if err := Load(doc); err != nil { + t.Fatalf("Load: %v", err) + } + if _, ok := IsTrustedWithKey(1, raw); !ok { + t.Fatal("loaded pinned entry: correct key should be trusted") + } + wrong, _ := newPin(t) + if _, ok := IsTrustedWithKey(1, wrong); ok { + t.Fatal("loaded pinned entry: wrong key must not be trusted") + } +} + +// TestLoad_BadPinRejected: a malformed public_key fails the whole Load +// rather than silently degrading the entry to unpinned (which would +// re-open finding H4 for that agent). +func TestLoad_BadPinRejected(t *testing.T) { + restore := SetForTest(nil) + t.Cleanup(restore) + + // Not valid base64. + if err := Load([]byte(`{"agents":[{"hostname":"x","node_id":1,"public_key":"!!!not-base64!!!"}]}`)); err == nil { + t.Error("Load must reject a non-base64 public_key") + } + // Valid base64 but wrong length for an Ed25519 key. + short := base64.StdEncoding.EncodeToString([]byte("too-short")) + if err := Load([]byte(`{"agents":[{"hostname":"x","node_id":1,"public_key":"` + short + `"}]}`)); err == nil { + t.Error("Load must reject a wrong-length public_key") + } +} + +// TestService_IsTrustedWithKey_Delegates exercises the Service adapter +// method end-to-end. +func TestService_IsTrustedWithKey_Delegates(t *testing.T) { + raw, b64 := newPin(t) + restore := SetForTest([]Agent{ + {Hostname: "svc-agent", NodeID: 100, PublicKey: b64}, + }) + t.Cleanup(restore) + + s := NewService() + if name, ok := s.IsTrustedWithKey(100, raw); !ok || name != "svc-agent" { + t.Fatalf("Service.IsTrustedWithKey(100, correct) = (%q,%v), want (svc-agent,true)", name, ok) + } + wrong, _ := newPin(t) + if _, ok := s.IsTrustedWithKey(100, wrong); ok { + t.Fatal("Service.IsTrustedWithKey(100, wrong) must be untrusted") + } +}