Fixed segfault when a user manually updates their group file (#1049)

For example, changing the TLS settings

core/drand_beacon.go

@@ -114,36 +114,33 @@ func (bp *BeaconProcess) Load() (bool, error) {
 		return false, err
 	}
 
-	if bp.group != nil {
-		bp.state.Lock()
-		info := chain.NewChainInfo(bp.group)
-		bp.chainHash = info.Hash()
-		bp.state.Unlock()
+	beaconID := bp.getBeaconID()
+	if bp.group == nil {
+		bp.dkgDone = false
+		metrics.DKGStateChange(metrics.DKGNotStarted, beaconID, false)
+		return false, nil
 	}
 
+	bp.state.Lock()
+	info := chain.NewChainInfo(bp.group)
+	bp.chainHash = info.Hash()
 	checkGroup(bp.log, bp.group)
+	bp.state.Unlock()
+
 	bp.share, err = bp.store.LoadShare()
 	if err != nil {
 		return false, err
 	}
 
-	bp.index = int(bp.group.Find(bp.priv.Public).Index)
+	thisBeacon := bp.group.Find(bp.priv.Public)
+	if thisBeacon == nil {
+		return false, fmt.Errorf("could not restore beacon info for the given identity - this can happen if you updated the group file manually")
+	}
+	bp.index = int(thisBeacon.Index)
 	bp.log = bp.log.Named(fmt.Sprint(bp.index))
 
 	bp.log.Debugw("", "serving", bp.priv.Public.Address())
-
-	if bp.group != nil {
-		beaconID := bp.getBeaconID()
-
-		// TODO: can this ever be true?
-		if bp.dkgDone {
-			metrics.DKGStateChange(metrics.DKGDone, beaconID, false)
-		} else {
-			metrics.DKGStateChange(metrics.DKGNotStarted, beaconID, false)
-		}
-	}
-
-	bp.dkgDone = false
+	metrics.DKGStateChange(metrics.DKGDone, beaconID, false)
 
 	return false, nil
 }

core/drand_test.go

@@ -6,9 +6,13 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path"
+	"strings"
 	"testing"
 	"time"
 
+	"github.com/weaveworks/common/fs"
+
 	"github.com/drand/drand/protobuf/common"
 
 	"github.com/stretchr/testify/assert"
@@ -1167,6 +1171,7 @@ func TestReshareWithoutOldGroupFailsButNoSegfault(t *testing.T) {
 	sch, beaconID := scheme.GetSchemeFromEnv(), test.GetBeaconIDFromEnv()
 
 	dt := NewDrandTestScenario(t, n, thr, p, sch, beaconID)
+	defer dt.Cleanup()
 	_ = dt.RunDKG()
 
 	resharePacket := drand.InitResharePacket{
@@ -1188,3 +1193,51 @@ func TestReshareWithoutOldGroupFailsButNoSegfault(t *testing.T) {
 	_, err := dt.nodes[1].daemon.InitReshare(context.Background(), &resharePacket)
 	assert.EqualError(t, err, "cannot reshare without an old group")
 }
+
+func TestModifyingGroupFileManuallyDoesNotSegfault(t *testing.T) {
+	// set up 3 nodes for a test
+	n := 3
+	thr := key.DefaultThreshold(n)
+	p := 1 * time.Second
+	sch, beaconID := scheme.GetSchemeFromEnv(), test.GetBeaconIDFromEnv()
+
+	dt := NewDrandTestScenario(t, n, thr, p, sch, beaconID)
+	defer dt.Cleanup()
+
+	node := dt.nodes[0]
+	dir := dt.dir
+	priv := node.drand.priv
+
+	// set a persistent keystore, as the normal test ones are ephemeral
+	store := key.NewFileStore(dir, beaconID)
+	node.drand.store = store
+
+	// save the key pair, as this was done ephemerally inside of `NewDrandTestScenario` >.>
+	err := store.SaveKeyPair(priv)
+	require.NoError(t, err)
+
+	// run a DKG so that every node gets a group file and key share
+	_ = dt.RunDKG()
+
+	// stop the node and wait for it
+	node.daemon.Stop(context.Background())
+	<-node.daemon.exitCh
+
+	// modify your entry (well, all of them!) in the group file to change the TLS status
+	groupPath := path.Join(dir, beaconID, key.GroupFolderName, "drand_group.toml")
+
+	// read
+	groupFileReader, err := fs.Open(groupPath)
+	require.NoError(t, err)
+	groupFile, err := io.ReadAll(groupFileReader)
+	require.NoError(t, err)
+	// write
+	err = os.WriteFile(groupPath, []byte(strings.ReplaceAll(string(groupFile), "true", "false")), 0740)
+	require.NoError(t, err)
+
+	// try and reload the beacon from the store
+	// the updated TLS status will fail verification
+	_, err = node.daemon.LoadBeaconFromStore(beaconID, store)
+
+	assert.EqualError(t, err, "could not restore beacon info for the given identity - this can happen if you updated the group file manually")
+}

core/util_test.go

@@ -108,6 +108,7 @@ func BatchNewDrand(t *testing.T, n int, insecure bool, sch scheme.Scheme, beacon
 	tmp := os.TempDir()
 
 	dir, err := os.MkdirTemp(tmp, "drand")
+	dir = path.Join(dir, common.MultiBeaconFolder)
 	assert.NoError(t, err)
 
 	certPaths = make([]string, n)

key/store.go

@@ -50,7 +50,7 @@ type Tomler interface {
 	TOMLValue() interface{}
 }
 
-// fileStore is a Store using filesystem to store informations
+// fileStore is a Store using filesystem to store information
 type fileStore struct {
 	baseFolder     string
 	beaconID       string
@@ -153,7 +153,7 @@ func (f *fileStore) Reset(...ResetOption) error {
 		return fmt.Errorf("drand: err deleting dist. key file: %w", err)
 	}
 	if err := Delete(f.shareFile); err != nil {
-		return fmt.Errorf("drand: errd eleting share file: %w", err)
+		return fmt.Errorf("drand: err deleting share file: %w", err)
 	}
 
 	if err := Delete(f.groupFile); err != nil {