Prevent mutations by entry cache callers (spiffe#3215)

Recently, we introduced a change wherein the agents supply an output
mask to the server (spiffe#3123) to reduce bandwidth usage.

This exposed a bug in the interactions between the SVID API handler and
the entry cache. The cache currently returns its owned copy of the entry
to callers. This was done for performance reasons.... making a copy of
each entry increases memory pressure in one of the hottest codepaths in
the server.

Due to this behavior however, the SVID handler, when applying the mask
to remove fields from the entries before including them in the response,
was inadvertently stripping off fields from entries within the cache.
This was not only resulting in temporary data loss (e.g. dns names) on
the entries (next cache refresh would restore the fields) but could
easily become a data race, wherein entries could get mutated by multiple
entities at once (since the fields are mutated concurrently without any
sort of lock protection).

This change updates the cache to clone the entries before returning them
to the caller. Although this results in some increase in memory
pressure, it is the cleanest, and most robust approach. If the increase
in memory pressure turns out to be too much, we can explore other
options, though those may come with a large cost in code complexity
(e.g. on-demand cloning of shared data structure). Even if we did
something cute, the GetAuthorizedEntries RPC is by far the most called
RPC in the agent and would need to clone anyway to apply the mask.

Fixes: spiffe#3184

Signed-off-by: Andrew Harding <aharding@vmware.com>

pkg/server/api/entry.go

@@ -44,13 +44,16 @@ func RegistrationEntryToProto(e *common.RegistrationEntry) (*types.Entry, error)
 		return nil, fmt.Errorf("invalid parent ID: %w", err)
 	}
 
-	federatesWith := make([]string, 0, len(e.FederatesWith))
-	for _, trustDomainID := range e.FederatesWith {
-		td, err := spiffeid.TrustDomainFromString(trustDomainID)
-		if err != nil {
-			return nil, fmt.Errorf("invalid federated trust domain: %w", err)
+	var federatesWith []string
+	if len(e.FederatesWith) > 0 {
+		federatesWith = make([]string, 0, len(e.FederatesWith))
+		for _, trustDomainID := range e.FederatesWith {
+			td, err := spiffeid.TrustDomainFromString(trustDomainID)
+			if err != nil {
+				return nil, fmt.Errorf("invalid federated trust domain: %w", err)
+			}
+			federatesWith = append(federatesWith, td.String())
 		}
-		federatesWith = append(federatesWith, td.String())
 	}
 
 	return &types.Entry{

pkg/server/cache/entrycache/fullcache.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/spiffe/go-spiffe/v2/spiffeid"
 	"github.com/spiffe/spire-api-sdk/proto/spire/api/types"
+	"google.golang.org/protobuf/proto"
 )
 
 var (
@@ -175,7 +176,7 @@ func (c *FullEntryCache) GetAuthorizedEntries(agentID spiffeid.ID) []*types.Entr
 	seen := allocSeenSet()
 	defer freeSeenSet(seen)
 
-	return c.getAuthorizedEntries(spiffeIDFromID(agentID), seen)
+	return cloneEntries(c.getAuthorizedEntries(spiffeIDFromID(agentID), seen))
 }
 
 func (c *FullEntryCache) getAuthorizedEntries(id spiffeID, seen map[spiffeID]struct{}) []*types.Entry {
@@ -268,3 +269,14 @@ func isSubset(sub, whole selectorSet) bool {
 	}
 	return true
 }
+
+func cloneEntries(entries []*types.Entry) []*types.Entry {
+	if len(entries) == 0 {
+		return entries
+	}
+	cloned := make([]*types.Entry, 0, len(entries))
+	for _, entry := range entries {
+		cloned = append(cloned, proto.Clone(entry).(*types.Entry))
+	}
+	return cloned
+}

pkg/server/cache/entrycache/fullcache_test.go

@@ -132,6 +132,30 @@ func TestCache(t *testing.T) {
 	assert.Equal(t, expected, actual)
 }
 
+func TestCacheReturnsClonedEntries(t *testing.T) {
+	ds := fakedatastore.New(t)
+
+	expected, err := api.RegistrationEntryToProto(createRegistrationEntry(context.Background(), t, ds, &common.RegistrationEntry{
+		ParentId:  "spiffe://domain.test/node",
+		SpiffeId:  "spiffe://domain.test/workload",
+		Selectors: []*common.Selector{{Type: "T", Value: "V"}},
+		DnsNames:  []string{"dns"},
+	}))
+	require.NoError(t, err)
+
+	cache, err := BuildFromDataStore(context.Background(), ds)
+	require.NoError(t, err)
+
+	actual := cache.GetAuthorizedEntries(spiffeid.RequireFromString("spiffe://domain.test/node"))
+	require.Equal(t, []*types.Entry{expected}, actual)
+
+	// Now mutate the returned entry, refetch, and assert the cache copy was
+	// not altered.
+	actual[0].DnsNames = nil
+	actual = cache.GetAuthorizedEntries(spiffeid.RequireFromString("spiffe://domain.test/node"))
+	require.Equal(t, []*types.Entry{expected}, actual)
+}
+
 func TestFullCacheNodeAliasing(t *testing.T) {
 	ds := fakedatastore.New(t)
 	ctx := context.Background()