From e55ae5316902f7dc2628ba0839b6ac9e0cfa36c9 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 2 Nov 2022 13:13:26 -0700 Subject: [PATCH] tailcfg: add Node.UnsignedPeerAPIOnly to let server mark node as peerapi-only capver 48 Change-Id: I20b2fa81d61ef8cc8a84e5f2afeefb68832bd904 Signed-off-by: Brad Fitzpatrick --- health/health.go | 7 +++ ipn/ipnlocal/local.go | 47 +++++++++++++++ ipn/ipnlocal/local_test.go | 107 +++++++++++++++++++++++++++++++++ ipn/ipnlocal/network-lock.go | 25 +++++--- tailcfg/tailcfg.go | 12 +++- tailcfg/tailcfg_clone.go | 1 + tailcfg/tailcfg_test.go | 1 + tailcfg/tailcfg_view.go | 2 + util/deephash/deephash_test.go | 2 +- 9 files changed, 193 insertions(+), 11 deletions(-) diff --git a/health/health.go b/health/health.go index a41a28385..109d7b79e 100644 --- a/health/health.go +++ b/health/health.go @@ -72,6 +72,10 @@ const ( // the Windows network adapter's "category" (public, private, domain). // If it's unhealthy, the Windows firewall rules won't match. SysNetworkCategory = Subsystem("network-category") + + // SysValidUnsignedNodes is a health check area for recording problems + // with the unsigned nodes that the coordination server sent. + SysValidUnsignedNodes = Subsystem("valid-unsigned-nodes") ) type watchHandle byte @@ -99,6 +103,9 @@ func RegisterWatcher(cb func(key Subsystem, err error)) (unregister func()) { } } +// SetValidUnsignedNodes sets the state of the map response validation. +func SetValidUnsignedNodes(err error) { set(SysValidUnsignedNodes, err) } + // SetRouterHealth sets the state of the wgengine/router.Router. func SetRouterHealth(err error) { set(SysRouter, err) } diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 3a32d79a0..894075324 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1303,6 +1303,14 @@ func (b *LocalBackend) updateFilterLocked(netMap *netmap.NetworkMap, prefs ipn.P localNetsB.AddPrefix(p) } packetFilter = netMap.PacketFilter + + if packetFilterPermitsUnlockedNodes(netMap.Peers, packetFilter) { + err := errors.New("server sent invalid packet filter permitting traffic to unlocked nodes; rejecting all packets for safety") + health.SetValidUnsignedNodes(err) + packetFilter = nil + } else { + health.SetValidUnsignedNodes(nil) + } } if prefs.Valid() { ar := prefs.AdvertiseRoutes() @@ -1375,6 +1383,45 @@ func (b *LocalBackend) updateFilterLocked(netMap *netmap.NetworkMap, prefs ipn.P } } +// packetFilterPermitsUnlockedNodes reports any peer in peers with the +// UnsignedPeerAPIOnly bool set true has any of its allowed IPs in the packet +// filter. +// +// If this reports true, the packet filter is invalid (the server is either broken +// or malicious) and should be ignored for safety. +func packetFilterPermitsUnlockedNodes(peers []*tailcfg.Node, packetFilter []filter.Match) bool { + var b netipx.IPSetBuilder + var numUnlocked int + for _, p := range peers { + if !p.UnsignedPeerAPIOnly { + continue + } + numUnlocked++ + for _, a := range p.AllowedIPs { // not only addresses! + b.AddPrefix(a) + } + } + if numUnlocked == 0 { + return false + } + s, err := b.IPSet() + if err != nil { + // Shouldn't happen, but if it does, fail closed. + return true + } + for _, m := range packetFilter { + for _, r := range m.Srcs { + if !s.OverlapsPrefix(r) { + continue + } + if len(m.Dsts) != 0 { + return true + } + } + } + return false +} + func (b *LocalBackend) setFilter(f *filter.Filter) { b.filterAtomic.Store(f) b.e.SetFilter(f) diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index f38a96c3b..303efb70a 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -22,6 +22,7 @@ import ( "tailscale.com/types/logger" "tailscale.com/types/netmap" "tailscale.com/wgengine" + "tailscale.com/wgengine/filter" "tailscale.com/wgengine/wgcfg" ) @@ -638,3 +639,109 @@ func TestInternalAndExternalInterfaces(t *testing.T) { }) } } + +func TestPacketFilterPermitsUnlockedNodes(t *testing.T) { + tests := []struct { + name string + peers []*tailcfg.Node + filter []filter.Match + want bool + }{ + { + name: "empty", + want: false, + }, + { + name: "no-unsigned", + peers: []*tailcfg.Node{ + {ID: 1}, + }, + want: false, + }, + { + name: "unsigned-good", + peers: []*tailcfg.Node{ + {ID: 1, UnsignedPeerAPIOnly: true}, + }, + want: false, + }, + { + name: "unsigned-bad", + peers: []*tailcfg.Node{ + { + ID: 1, + UnsignedPeerAPIOnly: true, + AllowedIPs: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.0/32"), + }, + }, + }, + filter: []filter.Match{ + { + Srcs: []netip.Prefix{netip.MustParsePrefix("100.64.0.0/32")}, + Dsts: []filter.NetPortRange{ + { + Net: netip.MustParsePrefix("100.99.0.0/32"), + }, + }, + }, + }, + want: true, + }, + { + name: "unsigned-bad-src-is-superset", + peers: []*tailcfg.Node{ + { + ID: 1, + UnsignedPeerAPIOnly: true, + AllowedIPs: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.0/32"), + }, + }, + }, + filter: []filter.Match{ + { + Srcs: []netip.Prefix{netip.MustParsePrefix("100.64.0.0/24")}, + Dsts: []filter.NetPortRange{ + { + Net: netip.MustParsePrefix("100.99.0.0/32"), + }, + }, + }, + }, + want: true, + }, + { + name: "unsigned-okay-because-no-dsts", + peers: []*tailcfg.Node{ + { + ID: 1, + UnsignedPeerAPIOnly: true, + AllowedIPs: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.0/32"), + }, + }, + }, + filter: []filter.Match{ + { + Srcs: []netip.Prefix{netip.MustParsePrefix("100.64.0.0/32")}, + Caps: []filter.CapMatch{ + { + Dst: netip.MustParsePrefix("100.99.0.0/32"), + Cap: "foo", + }, + }, + }, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := packetFilterPermitsUnlockedNodes(tt.peers, tt.filter); got != tt.want { + t.Errorf("got %v, want %v", got, tt.want) + } + }) + } + +} diff --git a/ipn/ipnlocal/network-lock.go b/ipn/ipnlocal/network-lock.go index a4a53b6eb..2956310f1 100644 --- a/ipn/ipnlocal/network-lock.go +++ b/ipn/ipnlocal/network-lock.go @@ -23,6 +23,7 @@ import ( "tailscale.com/types/key" "tailscale.com/types/netmap" "tailscale.com/types/tkatype" + "tailscale.com/util/mak" ) // TODO(tom): RPC retry/backoff was broken and has been removed. Fix? @@ -38,7 +39,7 @@ type tkaState struct { } // tkaFilterNetmapLocked checks the signatures on each node key, dropping -// nodes from the netmap who's signature does not verify. +// nodes from the netmap whose signature does not verify. // // b.mu must be held. func (b *LocalBackend) tkaFilterNetmapLocked(nm *netmap.NetworkMap) { @@ -49,27 +50,33 @@ func (b *LocalBackend) tkaFilterNetmapLocked(nm *netmap.NetworkMap) { return // TKA not enabled. } - toDelete := make(map[int]struct{}, len(nm.Peers)) + var toDelete map[int]bool // peer index => true for i, p := range nm.Peers { + if p.UnsignedPeerAPIOnly { + // Not subject to TKA. + continue + } if len(p.KeySignature) == 0 { b.logf("Network lock is dropping peer %v(%v) due to missing signature", p.ID, p.StableID) - toDelete[i] = struct{}{} + mak.Set(&toDelete, i, true) } else { if err := b.tka.authority.NodeKeyAuthorized(p.Key, p.KeySignature); err != nil { b.logf("Network lock is dropping peer %v(%v) due to failed signature check: %v", p.ID, p.StableID, err) - toDelete[i] = struct{}{} + mak.Set(&toDelete, i, true) } } } // nm.Peers is ordered, so deletion must be order-preserving. - peers := make([]*tailcfg.Node, 0, len(nm.Peers)) - for i, p := range nm.Peers { - if _, delete := toDelete[i]; !delete { - peers = append(peers, p) + if len(toDelete) > 0 { + peers := make([]*tailcfg.Node, 0, len(nm.Peers)) + for i, p := range nm.Peers { + if !toDelete[i] { + peers = append(peers, p) + } } + nm.Peers = peers } - nm.Peers = peers } // tkaSyncIfNeeded examines TKA info reported from the control plane, diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 8545207ae..b26f2d1bc 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -84,7 +84,8 @@ type CapabilityVersion int // - 45: 2022-09-26: c2n /debug/{goroutines,prefs,metrics} // - 46: 2022-10-04: c2n /debug/component-logging // - 47: 2022-10-11: Register{Request,Response}.NodeKeySignature -const CurrentCapabilityVersion CapabilityVersion = 47 +// - 48: 2022-11-02: Node.UnsignedPeerAPIOnly +const CurrentCapabilityVersion CapabilityVersion = 48 type StableID string @@ -231,6 +232,14 @@ type Node struct { // "https://tailscale.com/cap/file-sharing" Capabilities []string `json:",omitempty"` + // UnsignedPeerAPIOnly means that this node is not signed nor subject to TKA + // restrictions. However, in exchange for that privilege, it does not get + // network access. It can only access this node's peerapi, which may not let + // it do anything. It is the tailscaled client's job to double-check the + // MapResponse's PacketFilter to verify that its AllowedIPs will not be + // accepted by the packet filter. + UnsignedPeerAPIOnly bool `json:",omitempty"` + // The following three computed fields hold the various names that can // be used for this node in UIs. They are populated from controlclient // (not from control) by calling node.InitDisplayNames. These can be @@ -1552,6 +1561,7 @@ func (n *Node) Equal(n2 *Node) bool { n.Name == n2.Name && n.User == n2.User && n.Sharer == n2.Sharer && + n.UnsignedPeerAPIOnly == n2.UnsignedPeerAPIOnly && n.Key == n2.Key && n.KeyExpiry.Equal(n2.KeyExpiry) && bytes.Equal(n.KeySignature, n2.KeySignature) && diff --git a/tailcfg/tailcfg_clone.go b/tailcfg/tailcfg_clone.go index 62b9c96df..19ceb35b4 100644 --- a/tailcfg/tailcfg_clone.go +++ b/tailcfg/tailcfg_clone.go @@ -92,6 +92,7 @@ var _NodeCloneNeedsRegeneration = Node(struct { KeepAlive bool MachineAuthorized bool Capabilities []string + UnsignedPeerAPIOnly bool ComputedName string computedHostIfDifferent string ComputedNameWithHost string diff --git a/tailcfg/tailcfg_test.go b/tailcfg/tailcfg_test.go index a7dcb93c9..4a709d0fc 100644 --- a/tailcfg/tailcfg_test.go +++ b/tailcfg/tailcfg_test.go @@ -331,6 +331,7 @@ func TestNodeEqual(t *testing.T) { "Created", "Tags", "PrimaryRoutes", "LastSeen", "Online", "KeepAlive", "MachineAuthorized", "Capabilities", + "UnsignedPeerAPIOnly", "ComputedName", "computedHostIfDifferent", "ComputedNameWithHost", "DataPlaneAuditLogID", } diff --git a/tailcfg/tailcfg_view.go b/tailcfg/tailcfg_view.go index 730e11042..373de31d5 100644 --- a/tailcfg/tailcfg_view.go +++ b/tailcfg/tailcfg_view.go @@ -171,6 +171,7 @@ func (v NodeView) Online() *bool { func (v NodeView) KeepAlive() bool { return v.ж.KeepAlive } func (v NodeView) MachineAuthorized() bool { return v.ж.MachineAuthorized } func (v NodeView) Capabilities() views.Slice[string] { return views.SliceOf(v.ж.Capabilities) } +func (v NodeView) UnsignedPeerAPIOnly() bool { return v.ж.UnsignedPeerAPIOnly } func (v NodeView) ComputedName() string { return v.ж.ComputedName } func (v NodeView) ComputedNameWithHost() string { return v.ж.ComputedNameWithHost } func (v NodeView) DataPlaneAuditLogID() string { return v.ж.DataPlaneAuditLogID } @@ -201,6 +202,7 @@ var _NodeViewNeedsRegeneration = Node(struct { KeepAlive bool MachineAuthorized bool Capabilities []string + UnsignedPeerAPIOnly bool ComputedName string computedHostIfDifferent string ComputedNameWithHost string diff --git a/util/deephash/deephash_test.go b/util/deephash/deephash_test.go index c085d28b9..c864f829f 100644 --- a/util/deephash/deephash_test.go +++ b/util/deephash/deephash_test.go @@ -575,7 +575,7 @@ func TestGetTypeHasher(t *testing.T) { { name: "tailcfg.Node", val: &tailcfg.Node{}, - out: "\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\tn\x88\xf1\xff\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\tn\x88\xf1\xff\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", + out: "\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\tn\x88\xf1\xff\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\tn\x88\xf1\xff\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", }, } for _, tt := range tests {