From b63e3fd3aebb73fa1d0b6c1f97334660e18a2a25 Mon Sep 17 00:00:00 2001 From: maicon Date: Fri, 19 Sep 2025 09:37:17 -0300 Subject: [PATCH] Unistore: Add validation for resource names (#110990) * Unistore: Add validation for resource names Signed-off-by: Maicon Costa --------- Signed-off-by: Maicon Costa --- .../storage/testing/watcher_tests.go | 8 +- pkg/storage/unified/resource/bulk.go | 12 ++ pkg/storage/unified/resource/keys.go | 12 ++ pkg/storage/unified/resource/keys_test.go | 115 ++++++++++ pkg/storage/unified/resource/server.go | 4 + pkg/storage/unified/resource/server_test.go | 197 ++++++++++++++++++ pkg/storage/unified/resource/validation.go | 20 ++ 7 files changed, 364 insertions(+), 4 deletions(-) diff --git a/pkg/apiserver/storage/testing/watcher_tests.go b/pkg/apiserver/storage/testing/watcher_tests.go index 89ea37d4b95..371a3d612f9 100644 --- a/pkg/apiserver/storage/testing/watcher_tests.go +++ b/pkg/apiserver/storage/testing/watcher_tests.go @@ -684,7 +684,7 @@ func RunTestClusterScopedWatch(ctx context.Context, t *testing.T, store storage. currentObjs := map[string]*example.Pod{} for _, watchTest := range tt.watchTests { out := &example.Pod{} - key := "pods/" + watchTest.obj.Name + key := "pods/ns-1/" + watchTest.obj.Name err := store.GuaranteedUpdate(ctx, key, out, true, nil, storage.SimpleUpdate( func(runtime.Object) (runtime.Object, error) { obj := watchTest.obj.DeepCopy() @@ -1607,15 +1607,15 @@ func clusterScopedNodeNameAttrFunc(obj runtime.Object) (labels.Set, fields.Set, } func basePod(podName string) *example.Pod { - return baseNamespacedPod(podName, "") + return baseNamespacedPod(podName, "ns-1") } func basePodUpdated(podName string) *example.Pod { - return baseNamespacedPodUpdated(podName, "") + return baseNamespacedPodUpdated(podName, "ns-1") } func basePodAssigned(podName, nodeName string) *example.Pod { - return baseNamespacedPodAssigned(podName, "", nodeName) + return baseNamespacedPodAssigned(podName, "ns-1", nodeName) } func baseNamespacedPod(podName, namespace string) *example.Pod { diff --git a/pkg/storage/unified/resource/bulk.go b/pkg/storage/unified/resource/bulk.go index 90298c0da66..a0720efa6de 100644 --- a/pkg/storage/unified/resource/bulk.go +++ b/pkg/storage/unified/resource/bulk.go @@ -170,6 +170,18 @@ func (s *server) BulkProcess(stream resourcepb.BulkStore_BulkProcessServer) erro }) } + // Verify all request keys are valid + for _, k := range settings.Collection { + if r := verifyRequestKey(k); r != nil { + return sendAndClose(&resourcepb.BulkResponse{ + Error: &resourcepb.ErrorResult{ + Message: fmt.Sprintf("invalid request key: %s", r.Message), + Code: http.StatusBadRequest, + }, + }) + } + } + if settings.RebuildCollection { for _, k := range settings.Collection { // Can we delete the whole collection diff --git a/pkg/storage/unified/resource/keys.go b/pkg/storage/unified/resource/keys.go index 0af18d6c048..204246f3497 100644 --- a/pkg/storage/unified/resource/keys.go +++ b/pkg/storage/unified/resource/keys.go @@ -17,6 +17,18 @@ func verifyRequestKey(key *resourcepb.ResourceKey) *resourcepb.ErrorResult { if key.Resource == "" { return NewBadRequestError("request key is missing resource") } + if err := validateName(key.Name); err != nil { + return NewBadRequestError(fmt.Sprintf("name '%s' is invalid: '%s'", key.Name, err)) + } + if err := validateQualifiedName(key.Namespace); err != nil { + return NewBadRequestError(fmt.Sprintf("namespace '%s' is invalid: '%s'", key.Namespace, err)) + } + if err := validateQualifiedName(key.Group); err != nil { + return NewBadRequestError(fmt.Sprintf("group '%s' is invalid: '%s'", key.Group, err)) + } + if err := validateQualifiedName(key.Resource); err != nil { + return NewBadRequestError(fmt.Sprintf("resource '%s' is invalid: '%s'", key.Resource, err)) + } return nil } diff --git a/pkg/storage/unified/resource/keys_test.go b/pkg/storage/unified/resource/keys_test.go index d14c03fcc95..f86f1224313 100644 --- a/pkg/storage/unified/resource/keys_test.go +++ b/pkg/storage/unified/resource/keys_test.go @@ -1,6 +1,8 @@ package resource import ( + "net/http" + "strings" "testing" "github.com/stretchr/testify/require" @@ -66,3 +68,116 @@ func TestSearchIDKeys(t *testing.T) { } } } + +func TestVerifyRequestKey(t *testing.T) { + validGroup := "group.grafana.app" + validResource := "resource" + validNamespace := "default" + validName := "fdgsv37qslr0ga" + validLegacyUID := "f8cc010c-ee72-4681-89d2-d46e1bd47d33" + + invalidGroup := "group.~~~~~grafana.app" + invalidResource := "##resource" + invalidNamespace := "(((((default" + invalidName := " " // only spaces + + namespaceTooLong := strings.Repeat("a", MaxQualifiedNameLength+1) + nameTooLong := strings.Repeat("a", 300) + + tests := []struct { + name string + input *resourcepb.ResourceKey + expectedCode int32 + }{ + { + name: "no error when all fields are set and valid", + input: &resourcepb.ResourceKey{ + Namespace: validNamespace, + Group: validGroup, + Resource: validResource, + Name: validName, + }, + }, + { + name: "invalid namespace returns error", + input: &resourcepb.ResourceKey{ + Namespace: invalidNamespace, + Group: validGroup, + Resource: validResource, + Name: validName, + }, + expectedCode: http.StatusBadRequest, + }, + { + name: "invalid group returns error", + input: &resourcepb.ResourceKey{ + Namespace: validNamespace, + Group: invalidGroup, + Resource: validResource, + Name: validName, + }, + expectedCode: http.StatusBadRequest, + }, + { + name: "invalid resource returns error", + input: &resourcepb.ResourceKey{ + Namespace: validNamespace, + Group: validGroup, + Resource: invalidResource, + Name: validName, + }, + expectedCode: http.StatusBadRequest, + }, + { + name: "invalid name returns error", + input: &resourcepb.ResourceKey{ + Namespace: validNamespace, + Group: validGroup, + Resource: validResource, + Name: invalidName, + }, + expectedCode: http.StatusBadRequest, + }, + { + name: "valid legacy UID returns no error", + input: &resourcepb.ResourceKey{ + Namespace: validNamespace, + Group: validGroup, + Resource: validResource, + Name: validLegacyUID, + }, + }, + { + name: "namespace too long returns error", + input: &resourcepb.ResourceKey{ + Namespace: namespaceTooLong, + Group: validGroup, + Resource: validResource, + Name: validName, + }, + expectedCode: http.StatusBadRequest, + }, + { + name: "name too long returns error", + input: &resourcepb.ResourceKey{ + Namespace: namespaceTooLong, + Group: validGroup, + Resource: validResource, + Name: nameTooLong, + }, + expectedCode: http.StatusBadRequest, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := verifyRequestKey(test.input) + if test.expectedCode == 0 { + require.Nil(t, err) + return + } + + require.Equal(t, test.expectedCode, err.Code) + }) + } +} diff --git a/pkg/storage/unified/resource/server.go b/pkg/storage/unified/resource/server.go index 35987a418b8..55fe947fa56 100644 --- a/pkg/storage/unified/resource/server.go +++ b/pkg/storage/unified/resource/server.go @@ -626,6 +626,10 @@ func (s *server) Create(ctx context.Context, req *resourcepb.CreateRequest) (*re ctx, span := s.tracer.Start(ctx, "storage_server.Create") defer span.End() + if r := verifyRequestKey(req.Key); r != nil { + return nil, fmt.Errorf("invalid request key: %s", r.Message) + } + rsp := &resourcepb.CreateResponse{} user, ok := claims.AuthInfoFrom(ctx) if !ok || user == nil { diff --git a/pkg/storage/unified/resource/server_test.go b/pkg/storage/unified/resource/server_test.go index 98bc928da63..150ee328af1 100644 --- a/pkg/storage/unified/resource/server_test.go +++ b/pkg/storage/unified/resource/server_test.go @@ -7,6 +7,7 @@ import ( "log/slog" "net/http" "os" + "strings" "sync" "testing" "time" @@ -195,6 +196,202 @@ func TestSimpleServer(t *testing.T) { require.Len(t, all.Items, 0) // empty }) + t.Run("playlist FAIL CRUD paths due to invalid key", func(t *testing.T) { + raw := []byte(`{ + "apiVersion": "playlist.grafana.app/v0alpha1", + "kind": "Playlist", + "metadata": { + "name": "fdgsv37#qslr0ga", + "uid": "xyz", + "namespace": "default", + "annotations": { + "grafana.app/repoName": "elsewhere", + "grafana.app/repoPath": "path/to/item", + "grafana.app/repoTimestamp": "2024-02-02T00:00:00Z" + } + }, + "spec": { + "title": "hello", + "interval": "5m", + "items": [ + { + "type": "dashboard_by_uid", + "value": "vmie2cmWz" + } + ] + } + }`) + + // invalid group + key := &resourcepb.ResourceKey{ + Group: "playlist.grafana.app###", + Resource: "rrrr", // can be anything :( + Namespace: "default", + Name: "fdgsv37qslr0ga", + } + + created, err := server.Create(ctx, &resourcepb.CreateRequest{ + Value: raw, + Key: key, + }) + require.Error(t, err) + require.Nil(t, created) + + // invalid resource + key = &resourcepb.ResourceKey{ + Group: "playlist.grafana.app", + Resource: "rrrr###", // can be anything :( + Namespace: "default", + Name: "fdgsv37qslr0ga", + } + + created, err = server.Create(ctx, &resourcepb.CreateRequest{ + Value: raw, + Key: key, + }) + require.Error(t, err) + require.Nil(t, created) + + // invalid namespace + key = &resourcepb.ResourceKey{ + Group: "playlist.grafana.app", + Resource: "rrrr", // can be anything :( + Namespace: "default###", + Name: "fdgsv37qslr0ga", + } + + created, err = server.Create(ctx, &resourcepb.CreateRequest{ + Value: raw, + Key: key, + }) + require.Error(t, err) + require.Nil(t, created) + + // invalid name + key = &resourcepb.ResourceKey{ + Group: "playlist.grafana.app", + Resource: "rrrr", // can be anything :( + Namespace: "default", + Name: "fdgsv37qslr0g###", + } + + created, err = server.Create(ctx, &resourcepb.CreateRequest{ + Value: raw, + Key: key, + }) + require.Error(t, err) + require.Nil(t, created) + + // legacy name - valid + key = &resourcepb.ResourceKey{ + Group: "playlist.grafana.app", + Resource: "rrrr", // can be anything :( + Namespace: "default", + Name: "2c7e5361-7360-4d2a-ae45-5e79bba458d6", + } + + created, err = server.Create(ctx, &resourcepb.CreateRequest{ + Value: raw, + Key: key, + }) + + require.NoError(t, err) + require.NotNil(t, created) + + // legacy name - also valid + key = &resourcepb.ResourceKey{ + Group: "playlist.grafana.app", + Resource: "rrrr", // can be anything :( + Namespace: "default", + Name: "IvIsO_YGz", + } + + created, err = server.Create(ctx, &resourcepb.CreateRequest{ + Value: raw, + Key: key, + }) + + require.NoError(t, err) + require.NotNil(t, created) + + // legacy name - also valid + key = &resourcepb.ResourceKey{ + Group: "playlist.grafana.app", + Resource: "rrrr", // can be anything :( + Namespace: "default", + Name: "_IvIsOYGz", + } + + created, err = server.Create(ctx, &resourcepb.CreateRequest{ + Value: raw, + Key: key, + }) + + require.NoError(t, err) + require.NotNil(t, created) + + invalidQualifiedNames := []string{ + "", // empty + strings.Repeat("1", MaxQualifiedNameLength+1), // too long + " ", // only spaces + "f8cc010c.ee72.4681;89d2+d46e1bd47d33", // invalid chars + } + + // group + for _, invalidGroup := range invalidQualifiedNames { + key = &resourcepb.ResourceKey{ + Group: invalidGroup, + Resource: "rrrr", // can be anything :( + Namespace: "default", + Name: "_IvIsOYGz", + } + + created, err = server.Create(ctx, &resourcepb.CreateRequest{ + Value: raw, + Key: key, + }) + + require.Error(t, err) + require.Nil(t, created) + } + + // resource + for _, invalidResource := range invalidQualifiedNames { + key = &resourcepb.ResourceKey{ + Group: "playlist.grafana.app", + Resource: invalidResource, + Namespace: "default", + Name: "_IvIsOYGz", + } + + created, err = server.Create(ctx, &resourcepb.CreateRequest{ + Value: raw, + Key: key, + }) + + require.Error(t, err) + require.Nil(t, created) + } + + // namespace + for _, invalidNamespace := range invalidQualifiedNames { + key = &resourcepb.ResourceKey{ + Group: "playlist.grafana.app", + Resource: "rrrr", // can be anything :( + Namespace: invalidNamespace, + Name: "_IvIsOYGz", + } + + created, err = server.Create(ctx, &resourcepb.CreateRequest{ + Value: raw, + Key: key, + }) + + require.Error(t, err) + require.Nil(t, created) + } + }) + t.Run("playlist update optimistic concurrency check", func(t *testing.T) { raw := []byte(`{ "apiVersion": "playlist.grafana.app/v0alpha1", diff --git a/pkg/storage/unified/resource/validation.go b/pkg/storage/unified/resource/validation.go index 2c1b5cf3740..c3d3ee5bd45 100644 --- a/pkg/storage/unified/resource/validation.go +++ b/pkg/storage/unified/resource/validation.go @@ -1,11 +1,15 @@ package resource import ( + "fmt" "regexp" "github.com/grafana/grafana/pkg/storage/unified/resourcepb" + "k8s.io/apimachinery/pkg/util/validation" ) +const MaxQualifiedNameLength = 40 + var validNameCharPattern = `a-zA-Z0-9:\-\_\.` var validNamePattern = regexp.MustCompile(`^[` + validNameCharPattern + `]*$`).MatchString @@ -24,3 +28,19 @@ func validateName(name string) *resourcepb.ErrorResult { // so we will be slightly more lenient than standard k8s return nil } + +func validateQualifiedName(value string) *resourcepb.ErrorResult { + if len(value) == 0 { + return NewBadRequestError("value is too short") + } + if len(value) > MaxQualifiedNameLength { + return NewBadRequestError("value is too long") + } + + err := validation.IsQualifiedName(value) + if len(err) > 0 { + return NewBadRequestError(fmt.Sprintf("name is not a valid qualified name: %+v", err)) + } + + return nil +}