tka: add attack-scenario unit tests, defensive checks, resolve TODOs

Signed-off-by: Tom DNetto <tom@tailscale.com>

tka/aum.go

@@ -7,7 +7,6 @@ package tka
 import (
 	"bytes"
 	"crypto/ed25519"
-	"encoding/binary"
 	"errors"
 	"fmt"
 
@@ -122,13 +121,6 @@ type AUM struct {
 	Signatures []Signature `cbor:"23,keyasint,omitempty"`
 }
 
-// Upper bound on checkpoint elements, chosen arbitrarily. Intended to
-// cap out insanely large AUMs.
-const (
-	maxDisablementSecrets = 32
-	maxKeys               = 512
-)
-
 // StaticValidate returns a nil error if the AUM is well-formed.
 func (a *AUM) StaticValidate() error {
 	if a.Key != nil {
@@ -146,34 +138,9 @@ func (a *AUM) StaticValidate() error {
 	}
 
 	if a.State != nil {
-		if a.State.LastAUMHash != nil {
+		if err := a.State.staticValidateCheckpoint(); err != nil {
-			return errors.New("checkpoint state cannot specify a parent AUM")
+			return fmt.Errorf("checkpoint state: %v", err)
-		}
-		if len(a.State.DisablementSecrets) == 0 {
-			return errors.New("at least one disablement secret required")
 		}
-		if numDS := len(a.State.DisablementSecrets); numDS > maxDisablementSecrets {
-			return fmt.Errorf("too many disablement secrets (%d, max %d)", numDS, maxDisablementSecrets)
-		}
-		for i, ds := range a.State.DisablementSecrets {
-			if len(ds) != disablementLength {
-				return fmt.Errorf("disablement[%d]: invalid length (got %d, want %d)", i, len(ds), disablementLength)
-			}
-		}
-		// TODO(tom): Check for duplicate disablement secrets.
-
-		if len(a.State.Keys) == 0 {
-			return errors.New("at least one key is required")
-		}
-		if numKeys := len(a.State.Keys); numKeys > maxKeys {
-			return fmt.Errorf("too many keys (%d, max %d)", numKeys, maxKeys)
-		}
-		for i, k := range a.State.Keys {
-			if err := k.StaticValidate(); err != nil {
-				return fmt.Errorf("key[%d]: %v", i, err)
-			}
-		}
-		// TODO(tom): Check for duplicate keys.
 	}
 
 	switch a.MessageKind {
@@ -213,7 +180,7 @@ func (a *AUM) StaticValidate() error {
 			return errors.New("DisableNL AUMs must specify a disablement secret")
 		}
 		if a.KeyID != nil || a.State != nil || a.Key != nil || a.Votes != nil || a.Meta != nil {
-			return errors.New("DisableNL AUMs may only a disablement secret")
+			return errors.New("DisableNL AUMs may only specify a disablement secret")
 		}
 	}
 
@@ -304,18 +271,17 @@ func (a *AUM) Weight(state State) uint {
 	// signatures with the same key do not result in 2x
 	// the weight.
 	//
-	// We use the first 8 bytes as the key for this map,
+	// Despite the wire encoding being []byte, all KeyIDs are
-	// because KeyIDs are either a blake2s hash or
+	// 32 bytes. As such, we use that as the key for the map,
-	// the 25519 public key, both of which approximate
+	// because map keys cannot be slices.
-	// random distribution.
+	seenKeys := make(map[[32]byte]struct{}, 6)
-	seenKeys := make(map[uint64]struct{}, 6)
 	for _, sig := range a.Signatures {
-		if len(sig.KeyID) < 8 {
+		if len(sig.KeyID) != 32 {
-			// Invalid, don't count it
+			panic("unexpected: keyIDs are 32 bytes")
-			continue
 		}
 
-		keyID := binary.LittleEndian.Uint64(sig.KeyID)
+		var keyID [32]byte
+		copy(keyID[:], sig.KeyID)
 
 		key, err := state.GetKey(sig.KeyID)
 		if err != nil {

tka/scenario_test.go

@@ -113,28 +113,35 @@ outer:
 	return strings.Join(out, ",")
 }
 
-func (s *scenarioTest) syncBetween(n1, n2 *scenarioNode) {
+func (s *scenarioTest) syncBetween(n1, n2 *scenarioNode) error {
 	o1, err := n1.A.SyncOffer()
 	if err != nil {
-		s.t.Fatal(err)
+		return err
 	}
 	o2, err := n2.A.SyncOffer()
 	if err != nil {
-		s.t.Fatal(err)
+		return err
 	}
 
 	aumsFrom1, err := n1.A.MissingAUMs(o2)
 	if err != nil {
-		s.t.Fatal(err)
+		return err
 	}
 	aumsFrom2, err := n2.A.MissingAUMs(o1)
 	if err != nil {
-		s.t.Fatal(err)
+		return err
 	}
 	if err := n2.A.Inform(aumsFrom1); err != nil {
-		s.t.Fatal(err)
+		return err
 	}
 	if err := n1.A.Inform(aumsFrom2); err != nil {
+		return err
+	}
+	return nil
+}
+
+func (s *scenarioTest) testSyncsBetween(n1, n2 *scenarioNode) {
+	if err := s.syncBetween(n1, n2); err != nil {
 		s.t.Fatal(err)
 	}
 }
@@ -201,7 +208,7 @@ func TestScenarioHelpers(t *testing.T) {
 		t.Errorf("chained AUM was not signed: %v", err)
 	}
 
-	s.syncBetween(control, n)
+	s.testSyncsBetween(control, n)
 	s.checkHaveConsensus(control, n)
 }
 
@@ -217,13 +224,13 @@ func TestNormalPropergation(t *testing.T) {
 		"L2": newTestchain(t, `L3 -> L4`),
 	})
 	// Can control haz the updates?
-	s.syncBetween(control, n1)
+	s.testSyncsBetween(control, n1)
 	s.checkHaveConsensus(control, n1)
 
 	// A new node came online, can the new node learn everything
 	// just via control?
 	n2 := s.mkNode("n2")
-	s.syncBetween(control, n2)
+	s.testSyncsBetween(control, n2)
 	s.checkHaveConsensus(control, n2)
 
 	// So by virtue of syncing with control n2 should be at the same
@@ -252,7 +259,7 @@ func TestForkingPropergation(t *testing.T) {
 		"L2": newTestchain(t, `L3 -> L4`),
 	})
 	// Can control haz the updates?
-	s.syncBetween(control, n1)
+	s.testSyncsBetween(control, n1)
 	s.checkHaveConsensus(control, n1)
 
 	// Ooooo what about a forking update?
@@ -264,11 +271,11 @@ func TestForkingPropergation(t *testing.T) {
 			optKey("key2", key, priv),
 			optTemplate("removeKey1", AUM{MessageKind: AUMRemoveKey, KeyID: s.defaultKey.ID()})),
 	})
-	s.syncBetween(control, n2)
+	s.testSyncsBetween(control, n2)
 	s.checkHaveConsensus(control, n2)
 
 	// No wozzles propergating from n2->CTRL, what about CTRL->n1?
-	s.syncBetween(control, n1)
+	s.testSyncsBetween(control, n1)
 	s.checkHaveConsensus(n1, n2)
 
 	if _, err := n1.A.state.GetKey(s.defaultKey.ID()); err != ErrNoSuchKey {
@@ -278,3 +285,104 @@ func TestForkingPropergation(t *testing.T) {
 		t.Errorf("key2 was not trusted: %v", err)
 	}
 }
+
+func TestInvalidAUMPropergationRejected(t *testing.T) {
+	s := testScenario(t, `
+        G -> L1 -> L2
+        G.template = genesis
+    `)
+	control := s.mkNode("control")
+
+	// Construct an invalid L4 AUM, and manually apply it to n1,
+	// resulting in a corrupted Authority.
+	n1 := s.mkNodeWithForks("n1", true, map[string]*testChain{
+		"L2": newTestchain(t, `L3`),
+	})
+	l3 := n1.AUMs["L3"]
+	l3H := l3.Hash()
+	l4 := AUM{MessageKind: AUMAddKey, PrevAUMHash: l3H[:]}
+	l4.sign25519(s.defaultPriv)
+	l4H := l4.Hash()
+	n1.A.storage.CommitVerifiedAUMs([]AUM{l4})
+	n1.A.state.LastAUMHash = &l4H
+
+	// Does control nope out with syncing?
+	if err := s.syncBetween(control, n1); err == nil {
+		t.Error("sync with invalid AUM was successful")
+	}
+
+	// Control should not have accepted ANY of the updates, even
+	// though L3 was well-formed.
+	l2 := control.AUMs["L2"]
+	l2H := l2.Hash()
+	if control.A.Head() != l2H {
+		t.Errorf("head was %x, expected %x", control.A.Head(), l2H)
+	}
+}
+
+func TestUnsignedAUMPropergationRejected(t *testing.T) {
+	s := testScenario(t, `
+        G -> L1 -> L2
+        G.template = genesis
+    `)
+	control := s.mkNode("control")
+
+	// Construct an unsigned L4 AUM, and manually apply it to n1,
+	// resulting in a corrupted Authority.
+	n1 := s.mkNodeWithForks("n1", true, map[string]*testChain{
+		"L2": newTestchain(t, `L3`),
+	})
+	l3 := n1.AUMs["L3"]
+	l3H := l3.Hash()
+	l4 := AUM{MessageKind: AUMNoOp, PrevAUMHash: l3H[:]}
+	l4H := l4.Hash()
+	n1.A.storage.CommitVerifiedAUMs([]AUM{l4})
+	n1.A.state.LastAUMHash = &l4H
+
+	// Does control nope out with syncing?
+	if err := s.syncBetween(control, n1); err == nil || err.Error() != "update 1 invalid: unsigned AUM" {
+		t.Errorf("sync with unsigned AUM was successful (err = %v)", err)
+	}
+
+	// Control should not have accepted ANY of the updates, even
+	// though L3 was well-formed.
+	l2 := control.AUMs["L2"]
+	l2H := l2.Hash()
+	if control.A.Head() != l2H {
+		t.Errorf("head was %x, expected %x", control.A.Head(), l2H)
+	}
+}
+
+func TestBadSigAUMPropergationRejected(t *testing.T) {
+	s := testScenario(t, `
+        G -> L1 -> L2
+        G.template = genesis
+    `)
+	control := s.mkNode("control")
+
+	// Construct a otherwise-valid L4 AUM but mess up the signature.
+	n1 := s.mkNodeWithForks("n1", true, map[string]*testChain{
+		"L2": newTestchain(t, `L3`),
+	})
+	l3 := n1.AUMs["L3"]
+	l3H := l3.Hash()
+	l4 := AUM{MessageKind: AUMNoOp, PrevAUMHash: l3H[:]}
+	l4.sign25519(s.defaultPriv)
+	l4.Signatures[0].Signature[3] = 42
+	l4H := l4.Hash()
+	n1.A.storage.CommitVerifiedAUMs([]AUM{l4})
+	n1.A.state.LastAUMHash = &l4H
+
+	// Does control nope out with syncing?
+	if err := s.syncBetween(control, n1); err == nil || err.Error() != "update 1 invalid: signature 0: invalid signature" {
+		t.Errorf("sync with unsigned AUM was successful (err = %v)", err)
+	}
+
+	// Control should not have accepted ANY of the updates, even
+	// though L3 was well-formed.
+	l2 := control.AUMs["L2"]
+	l2H := l2.Hash()
+	if control.A.Head() != l2H {
+		t.Errorf("head was %x, expected %x", control.A.Head(), l2H)
+	}
+}

tka/state.go

@@ -147,6 +147,9 @@ func (s State) applyVerifiedAUM(update AUM) (State, error) {
 		return update.State.cloneForUpdate(&update), nil
 
 	case AUMAddKey:
+		if update.Key == nil {
+			return State{}, errors.New("no key to add provided")
+		}
 		if _, err := s.GetKey(update.Key.ID()); err == nil {
 			return State{}, errors.New("key already exists")
 		}
@@ -202,3 +205,58 @@ func (s State) applyVerifiedAUM(update AUM) (State, error) {
 		return State{}, fmt.Errorf("unhandled message: %v", update.MessageKind)
 	}
 }
+
+// Upper bound on checkpoint elements, chosen arbitrarily. Intended to
+// cap out insanely large AUMs.
+const (
+	maxDisablementSecrets = 32
+	maxKeys               = 512
+)
+
+// staticValidateCheckpoint validates that the state is well-formed for
+// inclusion in a checkpoint AUM.
+func (s *State) staticValidateCheckpoint() error {
+	if s.LastAUMHash != nil {
+		return errors.New("cannot specify a parent AUM")
+	}
+	if len(s.DisablementSecrets) == 0 {
+		return errors.New("at least one disablement secret required")
+	}
+	if numDS := len(s.DisablementSecrets); numDS > maxDisablementSecrets {
+		return fmt.Errorf("too many disablement secrets (%d, max %d)", numDS, maxDisablementSecrets)
+	}
+	for i, ds := range s.DisablementSecrets {
+		if len(ds) != disablementLength {
+			return fmt.Errorf("disablement[%d]: invalid length (got %d, want %d)", i, len(ds), disablementLength)
+		}
+		for j, ds2 := range s.DisablementSecrets {
+			if i == j {
+				continue
+			}
+			if bytes.Equal(ds, ds2) {
+				return fmt.Errorf("disablement[%d]: duplicates disablement[%d]", i, j)
+			}
+		}
+	}
+
+	if len(s.Keys) == 0 {
+		return errors.New("at least one key is required")
+	}
+	if numKeys := len(s.Keys); numKeys > maxKeys {
+		return fmt.Errorf("too many keys (%d, max %d)", numKeys, maxKeys)
+	}
+	for i, k := range s.Keys {
+		if err := k.StaticValidate(); err != nil {
+			return fmt.Errorf("key[%d]: %v", i, err)
+		}
+		for j, k2 := range s.Keys {
+			if i == j {
+				continue
+			}
+			if bytes.Equal(k.ID(), k2.ID()) {
+				return fmt.Errorf("key[%d]: duplicates key[%d]", i, j)
+			}
+		}
+	}
+	return nil
+}