From 781f79408d497525e8d8ad45c276d1ad2c67b916 Mon Sep 17 00:00:00 2001 From: Anton Tolchanov Date: Thu, 27 Jun 2024 12:54:17 +0100 Subject: [PATCH] ipn/ipnlocal: allow multiple signature chains from the same SigCredential Detection of duplicate Network Lock signature chains added in 01847e0123dee3b7a6f9645155da69270f01155e failed to account for chains originating with a SigCredential signature, which is used for wrapped auth keys. This results in erroneous removal of signatures that originate from the same re-usable auth key. This change ensures that multiple nodes created by the same re-usable auth key are not getting filtered out by the network lock. Updates tailscale/corp#19764 Signed-off-by: Anton Tolchanov --- ipn/ipnlocal/network-lock.go | 14 +++++++-- ipn/ipnlocal/network-lock_test.go | 50 +++++++++++++++++++------------ tka/sig.go | 6 ++-- tka/sig_test.go | 29 ++++++++++++++---- 4 files changed, 69 insertions(+), 30 deletions(-) diff --git a/ipn/ipnlocal/network-lock.go b/ipn/ipnlocal/network-lock.go index fff54231e..943e652e0 100644 --- a/ipn/ipnlocal/network-lock.go +++ b/ipn/ipnlocal/network-lock.go @@ -142,8 +142,9 @@ func (b *LocalBackend) tkaFilterNetmapLocked(nm *netmap.NetworkMap) { // - for each SigRotation signature, all previous node keys referenced by the // nested signatures are marked as obsolete. // - if there are multiple SigRotation signatures tracing back to the same -// wrapping pubkey (e.g. if a node is cloned with all its keys), we keep -// just one of them, marking the others as obsolete. +// wrapping pubkey of the initial SigDirect signature (e.g. if a node is +// cloned with all its keys), we keep just one of them, marking the others as +// obsolete. type rotationTracker struct { // obsolete is the set of node keys that are obsolete due to key rotation. // users of rotationTracker should use the obsoleteKeys method for complete results. @@ -165,6 +166,13 @@ type sigRotationDetails struct { func (r *rotationTracker) addRotationDetails(np key.NodePublic, d *tka.RotationDetails) { r.obsolete.Make() r.obsolete.AddSlice(d.PrevNodeKeys) + if d.InitialSig.SigKind != tka.SigDirect { + // Only enforce uniqueness of chains originating from a SigDirect + // signature. Chains that begin with a SigCredential can legitimately + // start from the same wrapping pubkey when multiple nodes join the + // network using the same reusable auth key. + return + } rd := sigRotationDetails{ np: np, numPrevKeys: len(d.PrevNodeKeys), @@ -172,7 +180,7 @@ func (r *rotationTracker) addRotationDetails(np key.NodePublic, d *tka.RotationD if r.byWrappingKey == nil { r.byWrappingKey = make(map[string][]sigRotationDetails) } - wp := string(d.WrappingPubkey) + wp := string(d.InitialSig.WrappingPubkey) r.byWrappingKey[wp] = append(r.byWrappingKey[wp], rd) } diff --git a/ipn/ipnlocal/network-lock_test.go b/ipn/ipnlocal/network-lock_test.go index 16e713c7e..c3576dfb0 100644 --- a/ipn/ipnlocal/network-lock_test.go +++ b/ipn/ipnlocal/network-lock_test.go @@ -609,7 +609,9 @@ func TestTKAFilterNetmap(t *testing.T) { t.Fatal(err) } - n6, n6Sig := nodeFromAuthKey(preauth) + // Two nodes created using the same auth key, both should be valid. + n60, n60Sig := nodeFromAuthKey(preauth) + n61, n61Sig := nodeFromAuthKey(preauth) nm := &netmap.NetworkMap{ Peers: nodeViews([]*tailcfg.Node{ @@ -619,7 +621,8 @@ func TestTKAFilterNetmap(t *testing.T) { {ID: 4, Key: n4.Public(), KeySignature: n4Sig.Serialize()}, // messed-up signature {ID: 50, Key: n5.Public(), KeySignature: n5InitialSig.Serialize()}, // rotated {ID: 51, Key: n5Rotated.Public(), KeySignature: n5RotatedSig}, - {ID: 6, Key: n6.Public(), KeySignature: n6Sig}, + {ID: 60, Key: n60.Public(), KeySignature: n60Sig}, + {ID: 61, Key: n61.Public(), KeySignature: n61Sig}, }), } @@ -628,7 +631,8 @@ func TestTKAFilterNetmap(t *testing.T) { want := nodeViews([]*tailcfg.Node{ {ID: 1, Key: n1.Public(), KeySignature: n1GoodSig.Serialize()}, {ID: 51, Key: n5Rotated.Public(), KeySignature: n5RotatedSig}, - {ID: 6, Key: n6.Public(), KeySignature: n6Sig}, + {ID: 60, Key: n60.Public(), KeySignature: n60Sig}, + {ID: 61, Key: n61.Public(), KeySignature: n61Sig}, }) nodePubComparer := cmp.Comparer(func(x, y key.NodePublic) bool { return x.Raw32() == y.Raw32() @@ -1206,6 +1210,14 @@ func TestRotationTracker(t *testing.T) { raw32 := [32]byte{idx} return key.NodePublicFromRaw32(go4mem.B(raw32[:])) } + + rd := func(initialKind tka.SigKind, wrappingKey []byte, prevKeys ...key.NodePublic) *tka.RotationDetails { + return &tka.RotationDetails{ + InitialSig: &tka.NodeKeySignature{SigKind: initialKind, WrappingPubkey: wrappingKey}, + PrevNodeKeys: prevKeys, + } + } + n1, n2, n3, n4, n5 := newNK(1), newNK(2), newNK(3), newNK(4), newNK(5) pk1, pk2, pk3 := []byte{1}, []byte{2}, []byte{3} @@ -1225,46 +1237,46 @@ func TestRotationTracker(t *testing.T) { { name: "single_prev_key", addDetails: []addDetails{ - {np: n1, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n2}, WrappingPubkey: pk1}}, + {np: n1, details: rd(tka.SigDirect, pk1, n2)}, }, want: set.SetOf([]key.NodePublic{n2}), }, { name: "several_prev_keys", addDetails: []addDetails{ - {np: n1, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n2}, WrappingPubkey: pk1}}, - {np: n3, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n4}, WrappingPubkey: pk2}}, - {np: n2, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n3, n4}, WrappingPubkey: pk1}}, + {np: n1, details: rd(tka.SigDirect, pk1, n2)}, + {np: n3, details: rd(tka.SigDirect, pk2, n4)}, + {np: n2, details: rd(tka.SigDirect, pk1, n3, n4)}, }, want: set.SetOf([]key.NodePublic{n2, n3, n4}), }, { name: "several_per_pubkey_latest_wins", addDetails: []addDetails{ - {np: n2, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n1}, WrappingPubkey: pk3}}, - {np: n3, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n1, n2}, WrappingPubkey: pk3}}, - {np: n4, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n1, n2, n3}, WrappingPubkey: pk3}}, - {np: n5, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n4}, WrappingPubkey: pk3}}, + {np: n2, details: rd(tka.SigDirect, pk3, n1)}, + {np: n3, details: rd(tka.SigDirect, pk3, n1, n2)}, + {np: n4, details: rd(tka.SigDirect, pk3, n1, n2, n3)}, + {np: n5, details: rd(tka.SigDirect, pk3, n4)}, }, want: set.SetOf([]key.NodePublic{n1, n2, n3, n4}), }, { name: "several_per_pubkey_same_chain_length_all_rejected", addDetails: []addDetails{ - {np: n2, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n1}, WrappingPubkey: pk3}}, - {np: n3, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n1, n2}, WrappingPubkey: pk3}}, - {np: n4, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n1, n2}, WrappingPubkey: pk3}}, - {np: n5, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n1, n2}, WrappingPubkey: pk3}}, + {np: n2, details: rd(tka.SigDirect, pk3, n1)}, + {np: n3, details: rd(tka.SigDirect, pk3, n1, n2)}, + {np: n4, details: rd(tka.SigDirect, pk3, n1, n2)}, + {np: n5, details: rd(tka.SigDirect, pk3, n1, n2)}, }, want: set.SetOf([]key.NodePublic{n1, n2, n3, n4, n5}), }, { name: "several_per_pubkey_longest_wins", addDetails: []addDetails{ - {np: n2, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n1}, WrappingPubkey: pk3}}, - {np: n3, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n1, n2}, WrappingPubkey: pk3}}, - {np: n4, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n1, n2}, WrappingPubkey: pk3}}, - {np: n5, details: &tka.RotationDetails{PrevNodeKeys: []key.NodePublic{n1, n2, n3}, WrappingPubkey: pk3}}, + {np: n2, details: rd(tka.SigDirect, pk3, n1)}, + {np: n3, details: rd(tka.SigDirect, pk3, n1, n2)}, + {np: n4, details: rd(tka.SigDirect, pk3, n1, n2)}, + {np: n5, details: rd(tka.SigDirect, pk3, n1, n2, n3)}, }, want: set.SetOf([]key.NodePublic{n1, n2, n3, n4}), }, diff --git a/tka/sig.go b/tka/sig.go index 4fdeb6a02..6c68a588e 100644 --- a/tka/sig.go +++ b/tka/sig.go @@ -313,9 +313,9 @@ type RotationDetails struct { // PrevNodeKeys is a list of node keys which have been rotated out. PrevNodeKeys []key.NodePublic - // WrappingPubkey is the public key which has been authorized to sign + // InitialSig is the first signature in the chain which led to // this rotating signature. - WrappingPubkey []byte + InitialSig *NodeKeySignature } // rotationDetails returns the RotationDetails for a SigRotation signature. @@ -339,7 +339,7 @@ func (s *NodeKeySignature) rotationDetails() (*RotationDetails, error) { } nested = nested.Nested } - sri.WrappingPubkey = nested.WrappingPubkey + sri.InitialSig = nested return sri, nil } diff --git a/tka/sig_test.go b/tka/sig_test.go index 42a6fd7f7..d857eaf55 100644 --- a/tka/sig_test.go +++ b/tka/sig_test.go @@ -356,7 +356,11 @@ func TestNodeKeySignatureRotationDetails(t *testing.T) { return sig }, want: &RotationDetails{ - WrappingPubkey: cPub, + InitialSig: &NodeKeySignature{ + SigKind: SigCredential, + KeyID: pub, + WrappingPubkey: cPub, + }, }, }, { @@ -382,8 +386,13 @@ func TestNodeKeySignatureRotationDetails(t *testing.T) { return sig }, want: &RotationDetails{ - WrappingPubkey: cPub, - PrevNodeKeys: []key.NodePublic{n1.Public()}, + InitialSig: &NodeKeySignature{ + SigKind: SigDirect, + Pubkey: n1pub, + KeyID: pub, + WrappingPubkey: cPub, + }, + PrevNodeKeys: []key.NodePublic{n1.Public()}, }, }, { @@ -418,13 +427,23 @@ func TestNodeKeySignatureRotationDetails(t *testing.T) { return sig }, want: &RotationDetails{ - WrappingPubkey: cPub, - PrevNodeKeys: []key.NodePublic{n2.Public(), n1.Public()}, + InitialSig: &NodeKeySignature{ + SigKind: SigDirect, + Pubkey: n1pub, + KeyID: pub, + WrappingPubkey: cPub, + }, + PrevNodeKeys: []key.NodePublic{n2.Public(), n1.Public()}, }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.want != nil { + initialHash := tt.want.InitialSig.SigHash() + tt.want.InitialSig.Signature = ed25519.Sign(priv, initialHash[:]) + } + sig := tt.sigFn() if err := sig.verifySignature(tt.nodeKey, k); err != nil { t.Fatalf("verifySignature(node) failed: %v", err)