From 270b80eb10bb333896a7329e2d2d4c0451daf5af Mon Sep 17 00:00:00 2001
From: "M. J. Fromberger" <fromberger@tailscale.com>
Date: Fri, 27 Sep 2024 17:41:35 -0700
Subject: [PATCH] client: fix corner cases in secret declaration

1. Don't allow an empty secret name to be declared.  While in principle this
   could be fine, it is weird and not worth supporting.

2. Deduplicate secret names before initialization. I originally thought this
   was not needed, but because of the way we merge cached results with the
   initialization step, duplicates can "poison" the initialization state.
---
 client/setec/store.go      | 13 +++++++++++--
 client/setec/store_test.go | 26 ++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/client/setec/store.go b/client/setec/store.go
index c7e5bee..beb28dd 100644
--- a/client/setec/store.go
+++ b/client/setec/store.go
@@ -14,6 +14,7 @@ import (
 	"log"
 	"math/rand"
 	"os"
+	"slices"
 	"sync"
 	"time"
 
@@ -855,10 +856,18 @@ func (c StoreConfig) secretNames() ([]string, []*Fields, error) {
 			return nil, nil, fmt.Errorf("parse struct fields: %w", err)
 		}
 
-		// We don't need to deduplicate here, the constructor merges all the
-		// names into a map.
 		sec = append(sec, fs.Secrets()...)
 		svs = append(svs, fs)
 	}
+	// Sort and compact (deduplicate) secret names.
+	// Although the constructor puts them into a map, duplicates can "poison"
+	// the initialization, so remove them up front.
+	slices.Sort(sec)
+	sec = slices.Compact(sec)
+	for _, name := range sec {
+		if name == "" {
+			return nil, nil, errors.New("empty secret name not allowed")
+		}
+	}
 	return sec, svs, nil
 }
diff --git a/client/setec/store_test.go b/client/setec/store_test.go
index bc76a14..c75fed7 100644
--- a/client/setec/store_test.go
+++ b/client/setec/store_test.go
@@ -115,6 +115,32 @@ func TestStore(t *testing.T) {
 			t.Errorf("Lookup(bravo): got %q, want error", s.Get())
 		}
 	})
+
+	t.Run("NewStore_emptyName", func(t *testing.T) {
+		st, err := setec.NewStore(ctx, setec.StoreConfig{
+			Client:      cli,
+			Secrets:     []string{""},
+			Logf:        logger.Discard,
+			AllowLookup: true,
+		})
+		if err == nil {
+			t.Fatalf("NewStore(empty): got %+v, want error", st)
+		}
+	})
+
+	t.Run("NewStore_dupName", func(t *testing.T) {
+		// Regression: A duplicate name can poison the initialization map, so we
+		// need to deduplicate as part of collection.
+		st, err := setec.NewStore(ctx, setec.StoreConfig{
+			Client:  cli,
+			Secrets: []string{"alpha", "alpha"},
+			Logf:    logger.Discard,
+		})
+		if err != nil {
+			t.Fatalf("NewStore(dup): unexpected error: %v", err)
+		}
+		t.Logf("NewStore: got secret %q", st.Secret("alpha").GetString())
+	})
 }
 
 func TestCachedStore(t *testing.T) {