[v18] Fixes possible collisions when hashing immutable labels

lib/scopes/joining/token.go

@@ -19,6 +19,7 @@ package joining
 import (
 	"cmp"
 	"crypto/sha256"
+	"encoding/binary"
 	"encoding/hex"
 	"slices"
 	"time"
@@ -480,7 +481,13 @@ func HashImmutableLabels(labels *joiningv1.ImmutableLabels) string {
 	}
 
 	hash := sha256.New()
-	if sshLabels := labels.GetSsh(); sshLabels != nil {
+	var bytesWrittenToHash int
+	writeHash := func(p []byte) {
+		n, _ := hash.Write(p)
+		bytesWrittenToHash += n
+	}
+
+	if sshLabels := labels.GetSsh(); len(sshLabels) > 0 {
 		sorted := make([]struct{ key, value string }, 0, len(sshLabels))
 		for k, v := range sshLabels {
 			sorted = append(sorted, struct{ key, value string }{k, v})
@@ -489,12 +496,35 @@ func HashImmutableLabels(labels *joiningv1.ImmutableLabels) string {
 			return cmp.Compare(a.key, b.key)
 		})
 
+		// first we write the service type so that the following labels do not collide with identical labels
+		// from other services e.g. app labels or database labels
+		writeHash([]byte("ssh"))
+
+		// Each map entry is added to the hash as 4 components:
+		// 1. The length of the key
+		// 2. The value of the key
+		// 3. The length of the value
+		// 4. The value itself
+		// This combination prevents collisions between:
+		// - single labels (e.g. aaa=bbb and aaab=bb)
+		// - splitting labels (e.g. aaa=bbbcccddd and aaa=bbb,ccc=ddd)
+		// ...because in both cases the lengths of the keys/values must change to create different labels from
+		// the same set of characters.
 		for _, v := range sorted {
-			_, _ = hash.Write([]byte(v.key))
-			_, _ = hash.Write([]byte(v.value))
+			buf := [8]byte{}
+			binary.BigEndian.PutUint64(buf[:], uint64(len(v.key)))
+			writeHash(buf[:])
+			writeHash([]byte(v.key))
+			binary.BigEndian.PutUint64(buf[:], uint64(len(v.value)))
+			writeHash(buf[:])
+			writeHash([]byte(v.value))
 		}
 	}
 
+	if bytesWrittenToHash == 0 {
+		return ""
+	}
+
 	return hex.EncodeToString(hash.Sum(nil))
 }
 

lib/scopes/joining/token_test.go

@@ -18,6 +18,7 @@ package joining_test
 
 import (
 	"fmt"
+	"maps"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -419,15 +420,158 @@ func TestImmutableLabelHashing(t *testing.T) {
 
 	// assert that the same labels match with their hash
 	initialHash := joining.HashImmutableLabels(labels)
-	require.True(t, joining.VerifyImmutableLabelsHash(labels, initialHash))
+	require.True(t, joining.VerifyImmutableLabelsHash(proto.CloneOf(labels), initialHash))
 
 	// assert that changing a label value fails the hash check
 	labels.Ssh["hello"] = "other"
-	require.False(t, joining.VerifyImmutableLabelsHash(labels, initialHash))
+	require.False(t, joining.VerifyImmutableLabelsHash(proto.CloneOf(labels), initialHash))
 
 	// assert that adding a label fails the hash check
 	labels.Ssh["three"] = "3"
-	require.False(t, joining.VerifyImmutableLabelsHash(labels, initialHash))
+	require.False(t, joining.VerifyImmutableLabelsHash(proto.CloneOf(labels), initialHash))
+}
+
+func TestImmutableLabelHashCollision(t *testing.T) {
+	// Assert labels that could feasibly result in the same set of strings in the same order do not collide
+	// unless they're the exact same keys and values. Represented as a slice of test cases to make it easier
+	// to extend once immutable labels are made up of more than SSH labels.
+	cases := []struct {
+		name    string
+		labelsA *joiningv1.ImmutableLabels
+		labelsB *joiningv1.ImmutableLabels
+	}{
+		{
+			// guards against map entries being naively concatenated as they're hashed. e.g.
+			// aaa=bbbcccddd should not collide with aaa=bbb,ccc=ddd
+			name: "split label concatenation",
+			labelsA: &joiningv1.ImmutableLabels{
+				Ssh: map[string]string{
+					"aaa": "bbbcccddd",
+				},
+			},
+
+			labelsB: &joiningv1.ImmutableLabels{
+				Ssh: map[string]string{
+					"aaa": "bbb",
+					"ccc": "ddd",
+				},
+			},
+		},
+		{
+			// guards against single entries being naively concatenated as they're hashed. e.g.
+			// aaa=bbb should not collide with aaab=bb
+			name: "single label concatenation",
+			labelsA: &joiningv1.ImmutableLabels{
+				Ssh: map[string]string{
+					"aaa": "bbb",
+				},
+			},
+
+			labelsB: &joiningv1.ImmutableLabels{
+				Ssh: map[string]string{
+					"aaab": "bb",
+				},
+			},
+		},
+		// TODO (eriktate): add test case for identical labels applied to different service types once immutable
+		// labels support more than SSH
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			hashA := joining.HashImmutableLabels(c.labelsA)
+			require.False(t, joining.VerifyImmutableLabelsHash(c.labelsB, hashA))
+		})
+	}
+}
+
+// TestImmutableLabelHashGolden tests the immutable labels hashing implementation against a set of known-good hashes
+// to help guard against regressions.
+func TestImmutableLabelHashGolden(t *testing.T) {
+	cases := []struct {
+		name   string
+		labels *joiningv1.ImmutableLabels
+		hash   string
+	}{
+		{
+			name: "single ssh label",
+			labels: &joiningv1.ImmutableLabels{
+				Ssh: map[string]string{
+					"aaa": "bbb",
+				},
+			},
+			hash: "5dd8fad69587f17535a4dea3ab41400914c3fbecd1972d4e194b1c18c0f4c4ff",
+		},
+		{
+			name: "multiple ssh labels",
+			labels: &joiningv1.ImmutableLabels{
+				Ssh: map[string]string{
+					"aaa": "bbb",
+					"ccc": "ddd",
+					"eee": "fff",
+				},
+			},
+			hash: "b4757712bb94a422f835ca983e9ab3a9ce9925617496e9eeea676fb65b28f2b9",
+		},
+		{
+			name: "empty labels",
+			labels: &joiningv1.ImmutableLabels{
+				Ssh: map[string]string{},
+			},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			hash := joining.HashImmutableLabels(c.labels)
+			// assert both VerifyImmutableLabelsHash and a regular equality check just in case
+			// the VerifyImmutableLabelsHash implementation drifts
+			assert.True(t, joining.VerifyImmutableLabelsHash(c.labels, hash))
+			assert.Equal(t, c.hash, hash)
+		})
+	}
+}
+
+func FuzzImmutableLabelHash(f *testing.F) {
+	f.Add("hello", "world", "foo", "bar", "baz", "qux", true)   // base case
+	f.Add("aaa", "bbbcccddd", "aaa", "bbb", "ccc", "ddd", true) // split label concatenation
+	f.Add("aaa", "bbb", "aaab", "bb", "", "", false)            // single label concatenation
+
+	f.Fuzz(func(t *testing.T, key1, value1, key2, value2, key3, value3 string, multiLabel bool) {
+		labelsA := &joiningv1.ImmutableLabels{
+			Ssh: map[string]string{
+				key1: value1,
+			},
+		}
+		labelsB := &joiningv1.ImmutableLabels{
+			Ssh: map[string]string{
+				key2: value2,
+			},
+		}
+		// assign a second label only if multiLabel is true
+		if multiLabel {
+			labelsB.Ssh[key3] = value3
+		}
+
+		// assert we can generate hashes for both labels without panicking
+		hashA := joining.HashImmutableLabels(labelsA)
+		require.NotEmpty(t, hashA)
+		hashB := joining.HashImmutableLabels(labelsB)
+		require.NotEmpty(t, hashB)
+
+		// assert that hashes are verified against their own labels
+		assert.True(t, joining.VerifyImmutableLabelsHash(proto.CloneOf(labelsA), hashA))
+		assert.True(t, joining.VerifyImmutableLabelsHash(proto.CloneOf(labelsB), hashB))
+
+		// assert that the same labels always result in the same hash and different labels always result in different hashes
+		assertFn := assert.False
+		if maps.Equal(labelsA.Ssh, labelsB.Ssh) {
+			assertFn = assert.True
+		}
+
+		assertFn(t, joining.VerifyImmutableLabelsHash(proto.CloneOf(labelsA), hashB))
+		assertFn(t, joining.VerifyImmutableLabelsHash(proto.CloneOf(labelsB), hashA))
+	})
 }
 
 func TestValidateTokenUpdate(t *testing.T) {