Unistore: Add validation for resource names (#110990)

* Unistore: Add validation for resource names

Signed-off-by: Maicon Costa <maiconscosta@gmail.com>

---------

Signed-off-by: Maicon Costa <maiconscosta@gmail.com>

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 {

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

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
 }
 

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)
+		})
+	}
+}

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 {

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",

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
+}