Skip to content

Commit

Permalink
client: fix corner cases in secret declaration (#124)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
creachadair authored Oct 25, 2024
1 parent e6eb936 commit 14d2640
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 2 deletions.
13 changes: 11 additions & 2 deletions client/setec/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"log"
"math/rand"
"os"
"slices"
"sync"
"time"

Expand Down Expand Up @@ -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
}
26 changes: 26 additions & 0 deletions client/setec/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 14d2640

Please sign in to comment.