From 5e179c77718fcc4b25ab980fd207b18333a8aa40 Mon Sep 17 00:00:00 2001 From: Tom DNetto Date: Mon, 12 Sep 2022 16:07:35 -0700 Subject: [PATCH] tka: implement machinery for node-key denylist Signed-off-by: Tom DNetto --- tka/aum.go | 40 +++++++++++++++++++++++++++++----- tka/state.go | 55 +++++++++++++++++++++++++++++++++++++++++++++++ tka/state_test.go | 42 ++++++++++++++++++++++++++++++++++++ tka/tka.go | 12 +++++++++++ 4 files changed, 144 insertions(+), 5 deletions(-) diff --git a/tka/aum.go b/tka/aum.go index 9c2daac6d..222fcbb06 100644 --- a/tka/aum.go +++ b/tka/aum.go @@ -13,6 +13,7 @@ "github.com/fxamacker/cbor/v2" "golang.org/x/crypto/blake2s" + "tailscale.com/types/key" "tailscale.com/types/tkatype" ) @@ -55,7 +56,7 @@ func (h AUMHash) MarshalText() ([]byte, error) { // // Only the Key optional field may be set. AUMAddKey - // A RemoveKey AUM describes hte removal of a key trusted by TKA. + // A RemoveKey AUM describes the removal of a key trusted by TKA. // // Only the KeyID optional field may be set. AUMRemoveKey @@ -70,6 +71,16 @@ func (h AUMHash) MarshalText() ([]byte, error) { // // Only the State optional field may be set. AUMCheckpoint + // A AddDenylistNodeKey AUM describes the addition of a node key to + // the denylist. + // + // Only the NodeKey optional field may be set. + AUMAddDenylistNodeKey + // A RemoveDenylistNodeKey AUM describes the removal of a node key from + // the denylist. + // + // Only the NodeKey optional field may be set. + AUMRemoveDenylistNodeKey ) func (k AUMKind) String() string { @@ -86,6 +97,10 @@ func (k AUMKind) String() string { return "checkpoint" case AUMUpdateKey: return "update-key" + case AUMAddDenylistNodeKey: + return "add-nodekey-denylist" + case AUMRemoveDenylistNodeKey: + return "remove-nodekey-denylist" default: return fmt.Sprintf("AUM?<%d>", int(k)) } @@ -129,6 +144,13 @@ type AUM struct { Votes *uint `cbor:"6,keyasint,omitempty"` Meta map[string]string `cbor:"7,keyasint,omitempty"` + // NodeKey describes the node-key being added or removed from the denylist. + // This field is used for AUMAddDenylistNodeKey & AUMRemoveDenylistNodeKey + // AUMs. + // + // NodeKey is the MarshalBinary representation of a key.NodePublic. + NodeKey []byte `cbor:"8,keyasint,omitempty"` + // Signatures lists the signatures over this AUM. // CBOR key 23 is the last key which can be encoded as a single byte. Signatures []tkatype.Signature `cbor:"23,keyasint,omitempty"` @@ -161,14 +183,14 @@ func (a *AUM) StaticValidate() error { if a.Key == nil { return errors.New("AddKey AUMs must contain a key") } - if a.KeyID != nil || a.State != nil || a.Votes != nil || a.Meta != nil { + if a.KeyID != nil || a.State != nil || a.Votes != nil || a.Meta != nil || a.NodeKey != nil { return errors.New("AddKey AUMs may only specify a Key") } case AUMRemoveKey: if len(a.KeyID) == 0 { return errors.New("RemoveKey AUMs must specify a key ID") } - if a.Key != nil || a.State != nil || a.Votes != nil || a.Meta != nil { + if a.Key != nil || a.State != nil || a.Votes != nil || a.Meta != nil || a.NodeKey != nil { return errors.New("RemoveKey AUMs may only specify a KeyID") } case AUMUpdateKey: @@ -178,16 +200,24 @@ func (a *AUM) StaticValidate() error { if a.Meta == nil && a.Votes == nil { return errors.New("UpdateKey AUMs must contain an update to votes or key metadata") } - if a.Key != nil || a.State != nil { + if a.Key != nil || a.State != nil || a.NodeKey != nil { return errors.New("UpdateKey AUMs may only specify KeyID, Votes, and Meta") } case AUMCheckpoint: if a.State == nil { return errors.New("Checkpoint AUMs must specify the state") } - if a.KeyID != nil || a.Key != nil || a.Votes != nil || a.Meta != nil { + if a.KeyID != nil || a.Key != nil || a.Votes != nil || a.Meta != nil || a.NodeKey != nil { return errors.New("Checkpoint AUMs may only specify State") } + case AUMAddDenylistNodeKey, AUMRemoveDenylistNodeKey: + if a.Key != nil || a.State != nil || a.Votes != nil || a.Meta != nil || a.KeyID != nil { + return errors.New("AUMs manipulating the denylist may only specify a NodeKey") + } + var nodeKey key.NodePublic + if err := nodeKey.UnmarshalBinary(a.NodeKey); err != nil { + return fmt.Errorf("NodeKey invalid: %v", err) + } case AUMNoOp: default: diff --git a/tka/state.go b/tka/state.go index 6bf55f2fb..2b63fd439 100644 --- a/tka/state.go +++ b/tka/state.go @@ -36,6 +36,13 @@ type State struct { // Keys are the public keys currently trusted by the TKA. Keys []Key `cbor:"3,keyasint"` + + // BannedNodekeys is a denylist of node-keys. Any signatures over + // these keys are not considered authorized. + // + // Each entry is encoded as the MarshalBinary representation of the + // underlying key.NodePublic. + BannedNodekeys [][]byte `cbor:"4,keyasint,omitempty"` } // GetKey returns the trusted key with the specified KeyID. @@ -77,6 +84,13 @@ func (s State) Clone() State { } } + if s.BannedNodekeys != nil { + out.BannedNodekeys = make([][]byte, len(s.BannedNodekeys)) + for i := range s.BannedNodekeys { + out.BannedNodekeys[i] = make([]byte, len(s.BannedNodekeys[i])) + copy(out.BannedNodekeys[i], s.BannedNodekeys[i]) + } + } return out } @@ -193,6 +207,32 @@ func (s State) applyVerifiedAUM(update AUM) (State, error) { out.Keys = append(out.Keys[:idx], out.Keys[idx+1:]...) return out, nil + case AUMAddDenylistNodeKey: + for i := range s.BannedNodekeys { + if bytes.Equal(update.NodeKey, s.BannedNodekeys[i]) { + return State{}, errors.New("entry already exists") + } + } + out := s.cloneForUpdate(&update) + // Validity of node-key already checked by StaticValidate in aumVerify(). + out.BannedNodekeys = append(out.BannedNodekeys, update.NodeKey) + return out, nil + + case AUMRemoveDenylistNodeKey: + idx := -1 + for i := range s.BannedNodekeys { + if bytes.Equal(update.NodeKey, s.BannedNodekeys[i]) { + idx = i + break + } + } + if idx < 0 { + return State{}, errors.New("no such entry") + } + out := s.cloneForUpdate(&update) + out.BannedNodekeys = append(out.BannedNodekeys[:idx], out.BannedNodekeys[idx+1:]...) + return out, nil + default: // TODO(tom): Instead of erroring, update lastHash and // continue (to preserve future compatibility). @@ -205,6 +245,7 @@ func (s State) applyVerifiedAUM(update AUM) (State, error) { const ( maxDisablementSecrets = 32 maxKeys = 512 + maxDenylistEntries = 64 ) // staticValidateCheckpoint validates that the state is well-formed for @@ -252,5 +293,19 @@ func (s *State) staticValidateCheckpoint() error { } } } + + if numDenys := len(s.BannedNodekeys); numDenys > maxDenylistEntries { + return fmt.Errorf("too many node-key denylist entries (%d, max %d)", numDenys, maxDenylistEntries) + } + for i, nk := range s.BannedNodekeys { + for j, nk2 := range s.BannedNodekeys { + if i == j { + continue + } + if bytes.Equal(nk, nk2) { + return fmt.Errorf("node-key entry %d duplicates entry %d", i, j) + } + } + } return nil } diff --git a/tka/state_test.go b/tka/state_test.go index 1ccddf38f..aeb721fb8 100644 --- a/tka/state_test.go +++ b/tka/state_test.go @@ -53,6 +53,15 @@ func TestCloneState(t *testing.T) { }, }, }, + { + "BannedNodekeys", + State{ + BannedNodekeys: [][]byte{ + {1, 2, 3, 4}, + {5, 6, 7, 8}, + }, + }, + }, } for _, tc := range tcs { @@ -153,6 +162,26 @@ func TestApplyUpdatesChain(t *testing.T) { LastAUMHash: hashFromHex("57343671da5eea3cfb502954e976e8028bffd3540b50a043b2a65a8d8d8217d0"), }, }, + { + "AddDenyNodekey", + []AUM{{MessageKind: AUMAddDenylistNodeKey, NodeKey: []byte{1, 2, 3, 4}}}, + State{}, + State{ + BannedNodekeys: [][]byte{{1, 2, 3, 4}}, + LastAUMHash: hashFromHex("e6d5db2b73e6ad69ae6b852f9a1c163c17bc98d3acf257ef9616e3a36c7368f2"), + }, + }, + { + "RemoveDenyNodekey", + []AUM{{MessageKind: AUMRemoveDenylistNodeKey, NodeKey: []byte{1, 2, 3, 4}, PrevAUMHash: fromHex("e6d5db2b73e6ad69ae6b852f9a1c163c17bc98d3acf257ef9616e3a36c7368f2")}}, + State{ + BannedNodekeys: [][]byte{{1, 2, 3, 4}}, + LastAUMHash: hashFromHex("e6d5db2b73e6ad69ae6b852f9a1c163c17bc98d3acf257ef9616e3a36c7368f2"), + }, + State{ + LastAUMHash: hashFromHex("beea03557a839a51de8822ffbe0819035ec85a1766c301ac2cd992e2b79b5068"), + }, + }, } for _, tc := range tcs { @@ -166,6 +195,7 @@ func TestApplyUpdatesChain(t *testing.T) { t.Fatalf("Apply message[%d] failed: %v", i, err) } // t.Logf("update[%d] end-state = %+v", i, state) + // t.Logf("state.LastAUMHash = %x", state.LastAUMHash[:]) updateHash := tc.Updates[i].Hash() if got, want := *state.LastAUMHash, updateHash[:]; !bytes.Equal(got[:], want) { @@ -223,6 +253,18 @@ func TestApplyUpdateErrors(t *testing.T) { }, errors.New("parent AUMHash mismatch"), }, + { + "AddDenyNodekey exists", + []AUM{{MessageKind: AUMAddDenylistNodeKey, NodeKey: []byte{1, 2, 3, 4}}}, + State{BannedNodekeys: [][]byte{{1, 2, 3, 4}}}, + errors.New("entry already exists"), + }, + { + "RemoveDenyNodekey notfound", + []AUM{{MessageKind: AUMRemoveDenylistNodeKey, NodeKey: []byte{1, 2, 3, 4}}}, + State{}, + errors.New("no such entry"), + }, } for _, tc := range tcs { diff --git a/tka/tka.go b/tka/tka.go index d87a0f50d..ed7bbae90 100644 --- a/tka/tka.go +++ b/tka/tka.go @@ -677,6 +677,18 @@ func (a *Authority) NodeKeyAuthorized(nodeKey key.NodePublic, nodeKeySignature t return errors.New("credential signatures cannot authorize nodes on their own") } + if len(a.state.BannedNodekeys) > 0 { + nodeBytes, err := nodeKey.MarshalBinary() + if err != nil { + return fmt.Errorf("marshalling pubkey: %v", err) + } + for _, bannedNode := range a.state.BannedNodekeys { + if bytes.Equal(bannedNode, nodeBytes) { + return errors.New("pubkey is on the denylist") + } + } + } + key, err := a.state.GetKey(decoded.KeyID) if err != nil { return fmt.Errorf("key: %v", err)