Denial of Service (Availability) due to improper KV key validation for colon in resource names

PoC: Steps:\n1) Deploy Grafana 12.x with Unified Storage enabled.\n2) Create a user-storage resource named grafana-splash-screen:uid in the default namespace. This yields a KV key containing ':' in the Name segment.\n3) Trigger a KV-iteration path (e.g., server restart or a list/resync operation). In versions before 12.4.0, the colon-containing key would be rejected by the KV layer, leading to an error or crash during initialization. After applying the patch, the colon-containing key is accepted and initialization succeeds.\n\nNotes: The test suite added asserts that ':' is accepted by kv.IsValidKey and preserved end-to-end, aligning the KV validation with Grafana's resource-name validators.

diff --git a/pkg/storage/unified/resource/keys_test.go b/pkg/storage/unified/resource/keys_test.go index 60e6920c144aa..f4d4e622350a1 100644 --- a/pkg/storage/unified/resource/keys_test.go +++ b/pkg/storage/unified/resource/keys_test.go @@ -7,6 +7,8 @@ import ( "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/apimachinery/validation" + "github.com/grafana/grafana/pkg/storage/unified/resource/kv" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" ) @@ -266,3 +268,37 @@ func TestVerifyRequestKeyCollection(t *testing.T) { }) } } + +// TestKeyRegexCoversValidationCharSets pins the contract that the unified +// storage KV regex (kv.IsValidKey) accepts every byte the upstream Grafana +// resource-name validators (IsValidGrafanaName / IsValidNamespace / +// IsValidGroup / IsValidResource) accept. +// +// Why: ensure KV regex supports Grafana API regex expressions +// +// The test lives in pkg/storage/unified/resource to avoid dependency between +// validation and kv packages. +func TestKeyRegexCoversValidationCharSets(t *testing.T) { + validators := map[string]func(string) []string{ + "IsValidGrafanaName": validation.IsValidGrafanaName, + "IsValidNamespace": validation.IsValidNamespace, + "IsValidGroup": validation.IsValidGroup, + "IsValidResource": validation.IsValidResource, + } + + // so upstream regex don't reject the probe byte for the wrong reason. + // Only sweep ASCII (0..127) because upstream regex's characters are ASCII. + for b := 0; b < 128; b++ { + // Probe ASCII bytes between a alphanumeric string to pass name validation + sample := "ab" + string(rune(b)) + "cd" + + for name, accepts := range validators { + if errs := accepts(sample); len(errs) > 0 { + continue + } + require.Truef(t, kv.IsValidKey(sample), + "%s accepts rune %q in sample %q, but kv.IsValidKey rejects it", + name, rune(b), sample) + } + } +} diff --git a/pkg/storage/unified/resource/kv/datastoretypes_test.go b/pkg/storage/unified/resource/kv/datastoretypes_test.go index 139ae0f6bcfe8..22a409b33a07f 100644 --- a/pkg/storage/unified/resource/kv/datastoretypes_test.go +++ b/pkg/storage/unified/resource/kv/datastoretypes_test.go @@ -66,6 +66,27 @@ func TestParseDataKeyParts_ResourceVersionMetadata(t *testing.T) { require.Equal(t, "f", dk.Folder) require.Equal(t, []string{"1", "created", "f", "guid-xyz"}, rvParts) }) + + t.Run("userstorage format with colon", func(t *testing.T) { + t.Parallel() + // User-storage names follow the "<service>:<userUID>" convention + // enforced by pkg/registry/apis/userstorage/strategy.go, so the Name + // segment of a KV key for these resources contains ':'. The KV regex + // must accept that, otherwise iterator-side validation rejects every + // freshly-written user-storage key with "received invalid key from + // server" and the resource server cannot init. + parts := strings.Split("userstorage.grafana.app/user-storage/default/grafana-splash-screen:myuser1234abcd/2052318477101322240~created~", "/") + dk, rvParts, err := ParseDataKeyParts(parts) + require.NoError(t, err) + require.Equal(t, "userstorage.grafana.app", dk.Group) + require.Equal(t, "user-storage", dk.Resource) + require.Equal(t, "default", dk.Namespace) + require.Equal(t, "grafana-splash-screen:myuser1234abcd", dk.Name) + require.Equal(t, int64(2052318477101322240), dk.ResourceVersion) + require.Equal(t, DataActionCreated, dk.Action) + require.Equal(t, "", dk.Folder) + require.Equal(t, []string{"2052318477101322240", "created", ""}, rvParts) + }) } func TestParseKeyWithGUID(t *testing.T) { diff --git a/pkg/storage/unified/resource/kv/kv.go b/pkg/storage/unified/resource/kv/kv.go index a820fb480b46b..7a3af9c36328c 100644 --- a/pkg/storage/unified/resource/kv/kv.go +++ b/pkg/storage/unified/resource/kv/kv.go @@ -433,9 +433,9 @@ func PrefixRangeEnd(prefix string) string { var ( // validKeyRegex validates keys used in the unified storage - // Keys can contain alphanumeric characters (both upper and lowercase), '-', '.', '/', and '~' + // Keys can contain alphanumeric characters (both upper and lowercase), ':', '-', '.', '/', and '~' // Any combination of these characters is allowed as long as the key is not empty - validKeyRegex = regexp.MustCompile(`^[a-zA-Z0-9./~_-]+$`) + validKeyRegex = regexp.MustCompile(`^[a-zA-Z0-9.:/~_-]+$`) ) func IsValidKey(key string) bool { diff --git a/pkg/storage/unified/resource/kv/kv_test.go b/pkg/storage/unified/resource/kv/kv_test.go index 1da1dbd3e855a..85afc8e6d4c83 100644 --- a/pkg/storage/unified/resource/kv/kv_test.go +++ b/pkg/storage/unified/resource/kv/kv_test.go @@ -242,6 +242,9 @@ func TestIsValidKey(t *testing.T) { {"underscores", "a_b", true}, {"key with underscores and mixed chars", "4_D6mSh4z", true}, {"complex key with underscores", "ns/group_name/resource-name/Name_123~action-type", true}, + {"colon in name segment", "a:b", true}, + {"user-storage key with colon", + "userstorage.grafana.app/user-storage/default/grafana-splash-screen:myuser1234abcd/2052318477101322240~created~", true}, // invalid keys {"empty key", "", false},