From 73db56af5284a634692ef1d78dc1e700f1889e7a Mon Sep 17 00:00:00 2001 From: Tom DNetto Date: Tue, 4 Oct 2022 10:51:45 -0700 Subject: [PATCH] ipn/ipnlocal: filter peers with bad signatures when tka is enabled Signed-off-by: Tom DNetto --- envknob/envknob.go | 5 +++ ipn/ipnlocal/local.go | 3 ++ ipn/ipnlocal/network-lock.go | 33 +++++++++++++++++ ipn/ipnlocal/network-lock_test.go | 60 +++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+) diff --git a/envknob/envknob.go b/envknob/envknob.go index f8ff392bc..969cf7eac 100644 --- a/envknob/envknob.go +++ b/envknob/envknob.go @@ -277,6 +277,11 @@ func SSHPolicyFile() string { return String("TS_DEBUG_SSH_POLICY_FILE") } // SSHIgnoreTailnetPolicy is whether to ignore the Tailnet SSH policy for development. func SSHIgnoreTailnetPolicy() bool { return Bool("TS_DEBUG_SSH_IGNORE_TAILNET_POLICY") } + +// TKASkipSignatureCheck is whether to skip node-key signature checking for development. +func TKASkipSignatureCheck() bool { return Bool("TS_UNSAFE_SKIP_NKS_VERIFICATION") } + + // NoLogsNoSupport reports whether the client's opted out of log uploads and // technical support. func NoLogsNoSupport() bool { diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 51bbf32b8..12c4ca537 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -778,6 +778,9 @@ func (b *LocalBackend) setClientStatus(st controlclient.Status) { if err := b.tkaSyncIfNeededLocked(st.NetMap); err != nil { b.logf("[v1] TKA sync error: %v", err) } + if !envknob.TKASkipSignatureCheck() { + b.tkaFilterNetmapLocked(st.NetMap) + } if b.findExitNodeIDLocked(st.NetMap) { prefsChanged = true } diff --git a/ipn/ipnlocal/network-lock.go b/ipn/ipnlocal/network-lock.go index 1f1cc7ecb..6dcb92ba7 100644 --- a/ipn/ipnlocal/network-lock.go +++ b/ipn/ipnlocal/network-lock.go @@ -39,6 +39,39 @@ type tkaState struct { storage *tka.FS } +// tkaFilterNetmapLocked checks the signatures on each node key, dropping +// nodes from the netmap who's signature does not verify. +func (b *LocalBackend) tkaFilterNetmapLocked(nm *netmap.NetworkMap) { + if !envknob.UseWIPCode() { + return // Feature-flag till network-lock is in Alpha. + } + if b.tka == nil { + return // TKA not enabled. + } + + toDelete := make(map[int]struct{}, len(nm.Peers)) + for i, p := range nm.Peers { + 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{}{} + } 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{}{} + } + } + } + + // 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) + } + } + nm.Peers = peers +} + // tkaSyncIfNeededLocked examines TKA info reported from the control plane, // performing the steps necessary to synchronize local tka state. // diff --git a/ipn/ipnlocal/network-lock_test.go b/ipn/ipnlocal/network-lock_test.go index 8f151f654..3933d2e33 100644 --- a/ipn/ipnlocal/network-lock_test.go +++ b/ipn/ipnlocal/network-lock_test.go @@ -15,7 +15,9 @@ import ( "path/filepath" "testing" + "github.com/google/go-cmp/cmp" "tailscale.com/control/controlclient" + "tailscale.com/envknob" "tailscale.com/hostinfo" "tailscale.com/ipn" "tailscale.com/tailcfg" @@ -484,3 +486,61 @@ func TestTKASync(t *testing.T) { }) } } + +func TestTKAFilterNetmap(t *testing.T) { + envknob.Setenv("TAILSCALE_USE_WIP_CODE", "1") + + nlPriv := key.NewNLPrivate() + nlKey := tka.Key{Kind: tka.Key25519, Public: nlPriv.Public().Verifier(), Votes: 2} + storage := &tka.Mem{} + authority, _, err := tka.Create(storage, tka.State{ + Keys: []tka.Key{nlKey}, + DisablementSecrets: [][]byte{bytes.Repeat([]byte{0xa5}, 32)}, + }, nlPriv) + if err != nil { + t.Fatalf("tka.Create() failed: %v", err) + } + + n1, n2, n3, n4, n5 := key.NewNode(), key.NewNode(), key.NewNode(), key.NewNode(), key.NewNode() + n1GoodSig, err := signNodeKey(tailcfg.TKASignInfo{NodePublic: n1.Public()}, nlPriv) + if err != nil { + t.Fatal(err) + } + n4Sig, err := signNodeKey(tailcfg.TKASignInfo{NodePublic: n4.Public()}, nlPriv) + if err != nil { + t.Fatal(err) + } + n4Sig.Signature[3] = 42 // mess up the signature + n4Sig.Signature[4] = 42 // mess up the signature + n5GoodSig, err := signNodeKey(tailcfg.TKASignInfo{NodePublic: n5.Public()}, nlPriv) + if err != nil { + t.Fatal(err) + } + + nm := netmap.NetworkMap{ + Peers: []*tailcfg.Node{ + {ID: 1, Key: n1.Public(), KeySignature: n1GoodSig.Serialize()}, + {ID: 2, Key: n2.Public(), KeySignature: nil}, // missing sig + {ID: 3, Key: n3.Public(), KeySignature: n1GoodSig.Serialize()}, // someone elses sig + {ID: 4, Key: n4.Public(), KeySignature: n4Sig.Serialize()}, // messed-up signature + {ID: 5, Key: n5.Public(), KeySignature: n5GoodSig.Serialize()}, + }, + } + + b := &LocalBackend{ + logf: t.Logf, + tka: &tkaState{authority: authority}, + } + b.tkaFilterNetmapLocked(&nm) + + want := []*tailcfg.Node{ + {ID: 1, Key: n1.Public(), KeySignature: n1GoodSig.Serialize()}, + {ID: 5, Key: n5.Public(), KeySignature: n5GoodSig.Serialize()}, + } + nodePubComparer := cmp.Comparer(func(x, y key.NodePublic) bool { + return x.Raw32() == y.Raw32() + }) + if diff := cmp.Diff(nm.Peers, want, nodePubComparer); diff != "" { + t.Errorf("filtered netmap differs (-want, +got):\n%s", diff) + } +}