Access control / Configuration validation

Proof-of-concept (Poc) to reproduce the behavior before the fix: Prerequisites: - A Grafana deployment with provisioning CRDs installed (GitHub repository provisioning). - Access to create Connection and Repository resources (Kubernetes CRDs or Grafana provisioning API). - A GitHub Connection with webhook.disabled set to true. 1) Create a GitHub Connection with webhook disabled kubectl apply -f - << 'YAML' apiVersion: provisioning.grafana.app/v0alpha1 kind: Connection metadata: name: conn-webhook-disabled namespace: default spec: title: Webhook Disabled Connection type: github github: appID: "123456" installationID: "454545" webhook: disabled: true YAML 2) Create a GitHub Repository that references the above connection but does NOT disable repo webhooks kubectl apply -f - << 'YAML' apiVersion: provisioning.grafana.app/v0alpha1 kind: Repository metadata: name: repo-missing-webhook-disabled namespace: default spec: title: Repo Missing Webhook Disabled type: github github: url: https://github.com/org/repo branch: main connection: name: conn-webhook-disabled # note: no spec.webhook.disabled YAML Expected before the fix: - The creation of the Repository would succeed (no validation error) and provisioning would proceed, potentially resulting in webhook handling that does not honor the connection's webhook-disabled state. Expected after the fix: - The API should reject the Repository creation with a validation error on spec.webhook.disabled, indicating that it must be true because the connected resource has webhook.disabled set to true. 3) Validation check (if you have an admission webhook or API that returns detailed errors): - Error path should include field: spec.webhook.disabled - Error message should state that repository webhook must be disabled when the referenced connection has webhook.disabled enabled Notes: - The exact API endpoints or CRD schemas may differ depending on cluster setup. The key is the relationship: repository.spec.connection.name points to a connection whose spec.webhook.disabled is true, and repository.spec.webhook.disabled is not set to true. The fix enforces this dependency in admission validation.

diff --git a/apps/provisioning/pkg/repository/github/connection_webhook_validator.go b/apps/provisioning/pkg/repository/github/connection_webhook_validator.go new file mode 100644 index 0000000000000..94f2e120200ce --- /dev/null +++ b/apps/provisioning/pkg/repository/github/connection_webhook_validator.go @@ -0,0 +1,62 @@ +package github + +import ( + "context" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/endpoints/request" + + provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/apps/provisioning/pkg/repository" + "github.com/grafana/grafana/pkg/apimachinery/identity" +) + +type connectionWebhookValidator struct { + connGetter repository.ConnectionSpecGetter +} + +// NewConnectionWebhookValidator returns a Validator that rejects a GitHub repository +// which references a webhook-disabled connection but does not itself set spec.webhook.disabled: true. +func NewConnectionWebhookValidator(getter repository.ConnectionSpecGetter) repository.Validator { + return &connectionWebhookValidator{connGetter: getter} +} + +func (v *connectionWebhookValidator) Validate(ctx context.Context, cfg *provisioning.Repository) field.ErrorList { + if cfg.Spec.Type != provisioning.GitHubRepositoryType { + return nil + } + + if cfg.Spec.Connection == nil || cfg.Spec.Connection.Name == "" { + return nil + } + + if cfg.Spec.Webhook != nil && cfg.Spec.Webhook.Disabled { + return nil + } + + var err error + ctx, _, err = identity.WithProvisioningIdentity(ctx, cfg.Namespace) + if err != nil { + return field.ErrorList{field.InternalError(field.NewPath("spec", "connection", "name"), err)} + } + ctx = request.WithNamespace(ctx, cfg.Namespace) + + conn, err := v.connGetter.GetConnectionSpec(ctx, cfg.Spec.Connection.Name) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return field.ErrorList{field.InternalError(field.NewPath("spec", "connection", "name"), err)} + } + + if conn.Spec.Webhook != nil && conn.Spec.Webhook.Disabled { + return field.ErrorList{field.Invalid( + field.NewPath("spec", "webhook", "disabled"), + false, + "must be true because the referenced connection has webhook.disabled set to true", + )} + } + + return nil +} diff --git a/apps/provisioning/pkg/repository/github/connection_webhook_validator_test.go b/apps/provisioning/pkg/repository/github/connection_webhook_validator_test.go new file mode 100644 index 0000000000000..cd31abb0ca049 --- /dev/null +++ b/apps/provisioning/pkg/repository/github/connection_webhook_validator_test.go @@ -0,0 +1,126 @@ +package github_test + +import ( + "context" + "errors" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/apps/provisioning/pkg/repository/github" +) + +type stubConnGetter struct { + conn *provisioning.Connection + err error +} + +func (s *stubConnGetter) GetConnectionSpec(_ context.Context, _ string) (*provisioning.Connection, error) { + return s.conn, s.err +} + +func connWithWebhookDisabled(disabled bool) *provisioning.Connection { + return &provisioning.Connection{ + Spec: provisioning.ConnectionSpec{ + Webhook: &provisioning.ConnectionWebhookConfig{ + Disabled: disabled, + }, + }, + } +} + +func ghRepo(ns, connName string, webhookDisabled bool) *provisioning.Repository { + r := &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: ns, + }, + Spec: provisioning.RepositorySpec{ + Type: provisioning.GitHubRepositoryType, + GitHub: &provisioning.GitHubRepositoryConfig{ + URL: "https://github.com/org/repo", + }, + }, + } + if connName != "" { + r.Spec.Connection = &provisioning.ConnectionInfo{Name: connName} + } + if webhookDisabled { + r.Spec.Webhook = &provisioning.WebhookConfig{Disabled: true} + } + return r +} + +func TestConnectionWebhookValidator(t *testing.T) { + tests := []struct { + name string + repo *provisioning.Repository + getter *stubConnGetter + wantErrPath string + }{ + { + name: "non-github repo is skipped", + repo: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{Type: provisioning.LocalRepositoryType}, + }, + getter: &stubConnGetter{}, + }, + { + name: "no connection name is skipped", + repo: ghRepo("default", "", false), + getter: &stubConnGetter{}, + }, + { + name: "repo webhook already disabled, no conflict", + repo: ghRepo("default", "my-conn", true), + getter: &stubConnGetter{conn: connWithWebhookDisabled(true)}, + }, + { + name: "connection webhook not disabled, no error", + repo: ghRepo("default", "my-conn", false), + getter: &stubConnGetter{conn: connWithWebhookDisabled(false)}, + }, + { + name: "connection not found, no error", + repo: ghRepo("default", "my-conn", false), + getter: &stubConnGetter{err: apierrors.NewNotFound(schema.GroupResource{Resource: "connections"}, "my-conn")}, + }, + { + name: "connection webhook disabled but repo is not, error returned", + repo: ghRepo("default", "my-conn", false), + getter: &stubConnGetter{conn: connWithWebhookDisabled(true)}, + wantErrPath: "spec.webhook.disabled", + }, + { + name: "connection webhook disabled, repo also disabled, no error", + repo: ghRepo("default", "my-conn", true), + getter: &stubConnGetter{conn: connWithWebhookDisabled(true)}, + }, + { + name: "getter returns unexpected error, internal error returned", + repo: ghRepo("default", "my-conn", false), + getter: &stubConnGetter{err: errors.New("storage error")}, + wantErrPath: "spec.connection.name", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + v := github.NewConnectionWebhookValidator(tc.getter) + errs := v.Validate(context.Background(), tc.repo) + + if tc.wantErrPath == "" { + require.Empty(t, errs) + } else { + require.NotEmpty(t, errs) + assert.Equal(t, tc.wantErrPath, errs[0].Field) + } + }) + } +} diff --git a/apps/provisioning/pkg/repository/lister.go b/apps/provisioning/pkg/repository/lister.go index 5f969cd6b9d83..969ded3f1f51a 100644 --- a/apps/provisioning/pkg/repository/lister.go +++ b/apps/provisioning/pkg/repository/lister.go @@ -24,6 +24,11 @@ type RepositoryByConnectionLister interface { ListByConnection(ctx context.Context, connectionName string) ([]provisioning.Repository, error) } +// ConnectionSpecGetter fetches a raw Connection spec by name within the current namespace. +type ConnectionSpecGetter interface { + GetConnectionSpec(ctx context.Context, name string) (*provisioning.Connection, error) +} + // storageListerBackend is an interface for listing repositories from storage. // This is typically implemented by grafanarest.Storage. type storageListerBackend interface { diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 7113a10c70036..4db3863e5e442 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -38,6 +38,7 @@ import ( "github.com/grafana/grafana/apps/provisioning/pkg/loki" "github.com/grafana/grafana/apps/provisioning/pkg/quotas" "github.com/grafana/grafana/apps/provisioning/pkg/repository" + repogithub "github.com/grafana/grafana/apps/provisioning/pkg/repository/github" "github.com/grafana/grafana/pkg/apimachinery/identity" apiutils "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/apiserver/auditing" @@ -739,7 +740,8 @@ func (b *APIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.APIGroupI b.repoValidator = repository.NewValidator(b.allowImageRendering, b.repoFactory) existingReposValidator := repository.NewVerifyAgainstExistingRepositoriesValidator(b.repoLister, b.quotaGetter) - repoAdmissionValidator := repository.NewAdmissionValidator(b.allowedTargets, b.repoValidator, existingReposValidator) + connWebhookValidator := repogithub.NewConnectionWebhookValidator(b) + repoAdmissionValidator := repository.NewAdmissionValidator(b.allowedTargets, b.repoValidator, existingReposValidator, connWebhookValidator) b.admissionHandler.RegisterMutator(provisioning.RepositoryResourceInfo.GetName(), repository.NewAdmissionMutator(b.repoFactory, b.minSyncInterval)) b.admissionHandler.RegisterValidator(provisioning.RepositoryResourceInfo.GetName(), repoAdmissionValidator) // Connection mutator and validator @@ -1590,6 +1592,18 @@ func (b *APIBuilder) GetConnection(ctx context.Context, name string) (connection return b.asConnection(ctx, obj, nil) } +func (b *APIBuilder) GetConnectionSpec(ctx context.Context, name string) (*provisioning.Connection, error) { + obj, err := b.connectionStore.Get(ctx, name, &metav1.GetOptions{}) + if err != nil { + return nil, err + } + c, ok := obj.(*provisioning.Connection) + if !ok { + return nil, fmt.Errorf("unexpected connection type %T", obj) + } + return c, nil +} + func (b *APIBuilder) GetRepoFactory() repository.Factory { return b.repoFactory } diff --git a/pkg/tests/apis/provisioning/repository/webhook_connection_validation_test.go b/pkg/tests/apis/provisioning/repository/webhook_connection_validation_test.go new file mode 100644 index 0000000000000..41bdfb65305f5 --- /dev/null +++ b/pkg/tests/apis/provisioning/repository/webhook_connection_validation_test.go @@ -0,0 +1,116 @@ +package repository + +import ( + "context" + "encoding/base64" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +// TestIntegrationProvisioning_RepositoryWebhookConnectionValidation checks that +// creating a GitHub repository is rejected at admission when the referenced +// connection has spec.webhook.disabled: true but the repository does not. +func TestIntegrationProvisioning_RepositoryWebhookConnectionValidation(t *testing.T) { + helper := sharedHelper(t) + ctx := context.Background() + privateKeyBase64 := base64.StdEncoding.EncodeToString([]byte(testPrivateKeyPEM)) + + connection := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "provisioning.grafana.app/v0alpha1", + "kind": "Connection", + "metadata": map[string]any{ + "name": "conn-webhook-disabled", + "namespace": "default", + }, + "spec": map[string]any{ + "title": "Webhook Disabled Connection", + "type": "github", + "github": map[string]any{ + "appID": "123456", + "installationID": "454545", + }, + "webhook": map[string]any{ + "disabled": true, + }, + }, + "secure": map[string]any{ + "privateKey": map[string]any{ + "create": privateKeyBase64, + }, + }, + }} + + _, err := helper.CreateGithubConnection(t, ctx, connection) + require.NoError(t, err, "failed to create connection") + + t.Run("repository without webhook disabled is rejected", func(t *testing.T) { + repo := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "provisioning.grafana.app/v0alpha1", + "kind": "Repository", + "metadata": map[string]any{ + "name": "repo-missing-webhook-disabled", + "namespace": "default", + }, + "spec": map[string]any{ + "title": "Repo Missing Webhook Disabled", + "type": "github", + "sync": map[string]any{ + "enabled": false, + "target": "folder", + }, + "github": map[string]any{ + "url": "https://github.com/some/url", + "branch": "main", + }, + "connection": map[string]any{ + "name": "conn-webhook-disabled", + }, + }, + }} + + _, err := helper.Repositories.Resource.Create(ctx, repo, metav1.CreateOptions{FieldValidation: "Strict"}) + require.Error(t, err) + + var statusErr *apierrors.StatusError + require.ErrorAs(t, err, &statusErr) + assert.Equal(t, metav1.StatusReasonInvalid, statusErr.ErrStatus.Reason) + assert.Contains(t, statusErr.ErrStatus.Message, "spec.webhook.disabled") + }) + + t.Run("repository with webhook disabled is accepted", func(t *testing.T) { + repo := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "provisioning.grafana.app/v0alpha1", + "kind": "Repository", + "metadata": map[string]any{ + "name": "repo-with-webhook-disabled", + "namespace": "default", + }, + "spec": map[string]any{ + "title": "Repo With Webhook Disabled", + "type": "github", + "sync": map[string]any{ + "enabled": false, + "target": "folder", + }, + "github": map[string]any{ + "url": "https://github.com/some/url", + "branch": "main", + }, + "connection": map[string]any{ + "name": "conn-webhook-disabled", + }, + "webhook": map[string]any{ + "disabled": true, + }, + }, + }} + + _, err := helper.Repositories.Resource.Create(ctx, repo, metav1.CreateOptions{FieldValidation: "Strict"}) + require.NoError(t, err, "repository with webhook disabled should be accepted when connection also has webhook disabled") + }) +}