Authentication/Authorization - Token reuse across repository URL changes (credential misuse risk)

Proof-of-concept (conceptual) reproduction steps: 1) Precondition: Have a provisioning repository resource with URL set to old URL (e.g., https://github.com/acme/old) and a valid token stored in secure.token (e.g., old-token). 2) Action: Attempt to update the repository URL to a new URL (e.g., https://github.com/acme/new) via the Provisioning API (e.g., Kubernetes CRD patch or Grafana provisioning endpoint) without providing a new token (secure.token omitted or empty). 3) Expected (pre-fix) behavior: The update would succeed and the new repository would carry over the old token (CopySecureValues would copy old token to the new spec), enabling credential reuse for the new URL. 4) Expected (post-fix) behavior: The update is rejected with an error stating that a new token is required when changing the repository URL (as implemented by RequiresNewTokenForURLChange and validated in Validate). 5) Optional verification (after fix): Supply a new token in the payload when updating the URL; the update should succeed and the new token should be associated with the new URL, with no leakage of the old token. Concrete payload example (Kubernetes-like CRD patch): - apiVersion: grafana.grafana.io/v0alpha1 kind: Repository metadata: name: my-repo spec: type: github gitHub: url: https://github.com/acme/new branch: main secure: # omit token to trigger rejection on URL change; or provide a new token instead token: create: "new-token" # or omit to trigger the rejection Or via REST API / PATCH (conceptual): PATCH /api/provisioning/repositories/{name} { "spec": { "gitHub": { "url": "https://github.com/acme/new" } } } Expected result after fix: 400 Bad Request with message containing "a new token is required when changing the repository URL" when token is not provided; 200 OK when a new token is supplied and the URL is changed.

diff --git a/apps/provisioning/pkg/repository/mutator.go b/apps/provisioning/pkg/repository/mutator.go index fbe9712833942..f08553b6b4513 100644 --- a/apps/provisioning/pkg/repository/mutator.go +++ b/apps/provisioning/pkg/repository/mutator.go @@ -83,3 +83,7 @@ func CopySecureValues(new, old *provisioning.Repository) { new.Secure.WebhookSecret = old.Secure.WebhookSecret } } + +func RequiresNewTokenForURLChange(new, old *provisioning.Repository) bool { + return old != nil && new.URL() != old.URL() && new.Secure.Token.IsZero() +} diff --git a/apps/provisioning/pkg/repository/validator.go b/apps/provisioning/pkg/repository/validator.go index d3f6ef2678817..4575d1978279d 100644 --- a/apps/provisioning/pkg/repository/validator.go +++ b/apps/provisioning/pkg/repository/validator.go @@ -255,6 +255,14 @@ func (v *AdmissionValidator) Validate(ctx context.Context, a admission.Attribute // Copy previous values if they exist if a.GetOldObject() != nil { if oldRepo, ok := a.GetOldObject().(*provisioning.Repository); ok { + if a.GetOperation() == admission.Update && RequiresNewTokenForURLChange(r, oldRepo) { + return invalidRepositoryError(a.GetName(), field.ErrorList{ + field.Forbidden( + field.NewPath("secure", "token"), + "a new token is required when changing the repository URL", + ), + }) + } CopySecureValues(r, oldRepo) } } diff --git a/apps/provisioning/pkg/repository/validator_test.go b/apps/provisioning/pkg/repository/validator_test.go index 6e107656aa4f7..4e3527d312632 100644 --- a/apps/provisioning/pkg/repository/validator_test.go +++ b/apps/provisioning/pkg/repository/validator_test.go @@ -744,6 +744,87 @@ func TestAdmissionValidator_CopiesSecureValuesOnUpdate(t *testing.T) { assert.Equal(t, "old-secret", newRepo.Secure.WebhookSecret.Name) } +func TestAdmissionValidator_RequiresNewTokenWhenURLChanges(t *testing.T) { + tests := []struct { + name string + oldURL string + newURL string + newToken common.InlineSecureValue + wantErr bool + wantErrContains string + }{ + { + name: "rejects url change without new token", + oldURL: "https://github.com/grafana/old", + newURL: "https://github.com/grafana/new", + wantErr: true, + wantErrContains: "secure.token", + }, + { + name: "allows url change with new token", + oldURL: "https://github.com/grafana/old", + newURL: "https://github.com/grafana/new", + newToken: common.InlineSecureValue{Create: "new-token"}, + wantErr: false, + }, + { + name: "allows unchanged url without new token", + oldURL: "https://github.com/grafana/repo", + newURL: "https://github.com/grafana/repo", + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockFactory := NewMockFactory(t) + mockFactory.EXPECT().Validate(mock.Anything, mock.Anything).Return(field.ErrorList{}).Maybe() + + validator := NewValidator(false, mockFactory) + admissionValidator := NewAdmissionValidator( + []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder}, + validator, + ) + + oldRepo := newGitHubRepositoryForURLChangeTest(tt.oldURL) + oldRepo.Secure.Token = common.InlineSecureValue{Name: "old-token"} + + newRepo := newGitHubRepositoryForURLChangeTest(tt.newURL) + newRepo.Secure.Token = tt.newToken + + attr := newAdmissionValidatorTestAttributes(newRepo, oldRepo, admission.Update) + + err := admissionValidator.Validate(context.Background(), attr, nil) + if tt.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErrContains) + assert.True(t, newRepo.Secure.Token.IsZero(), "old token should not be copied after rejection") + return + } + + require.NoError(t, err) + }) + } +} + +func newGitHubRepositoryForURLChangeTest(url string) *provisioning.Repository { + return &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Finalizers: []string{CleanFinalizer}, + }, + Spec: provisioning.RepositorySpec{ + Title: "Test Repo", + Type: provisioning.GitHubRepositoryType, + GitHub: &provisioning.GitHubRepositoryConfig{ + URL: url, + Branch: "main", + }, + Sync: provisioning.SyncOptions{IntervalSeconds: 60}, + }, + } +} + // mockValidator implements Validator for testing type mockValidator struct { called bool diff --git a/pkg/registry/apis/provisioning/test.go b/pkg/registry/apis/provisioning/test.go index 856e30f1b8b86..128c5d8bb9dde 100644 --- a/pkg/registry/apis/provisioning/test.go +++ b/pkg/registry/apis/provisioning/test.go @@ -134,6 +134,12 @@ func (s *testConnector) Connect(ctx context.Context, name string, _ runtime.Obje old, _ := s.repoGetter.GetRepository(ctx, name) if old != nil { oldCfg := old.Config() + if repository.RequiresNewTokenForURLChange(&cfg, oldCfg) { + responder.Error(k8serrors.NewBadRequest( + "a new token is required when changing the repository URL", + )) + return + } repository.CopySecureValues(&cfg, oldCfg) // Copying previous finalizers diff --git a/pkg/registry/apis/provisioning/test_test.go b/pkg/registry/apis/provisioning/test_test.go new file mode 100644 index 0000000000000..6e9a94fcd9e75 --- /dev/null +++ b/pkg/registry/apis/provisioning/test_test.go @@ -0,0 +1,192 @@ +package provisioning + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/endpoints/request" + + provisioningv0alpha1 "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/apps/provisioning/pkg/connection" + appcontroller "github.com/grafana/grafana/apps/provisioning/pkg/controller" + "github.com/grafana/grafana/apps/provisioning/pkg/repository" + common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" + provisioningcontroller "github.com/grafana/grafana/pkg/registry/apis/provisioning/controller" +) + +func TestTestConnector_RequiresNewTokenWhenURLChanges(t *testing.T) { + oldRepo := &staticTestRepository{ + cfg: testGitHubRepository("test", "default", "https://github.com/grafana/old"), + } + oldRepo.cfg.Secure.Token = common.InlineSecureValue{Name: "old-token"} + + repoFactory := repository.NewMockFactory(t) + connector := NewTestConnector( + &testConnectorDeps{repo: oldRepo, repoFactory: repoFactory}, + repository.NewTester(), + ) + + responder := &testResponder{} + handler, err := connector.Connect(request.WithNamespace(context.Background(), "default"), "test", nil, responder) + require.NoError(t, err) + + body := `{"spec":{"title":"Test Repo","type":"github","github":{"url":"https://github.com/grafana/new","branch":"main"}}}` + handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(body))) + + require.Error(t, responder.err) + status := responder.err.(apierrors.APIStatus).Status() + assert.Equal(t, int32(http.StatusBadRequest), status.Code) + assert.Contains(t, status.Message, "a new token is required when changing the repository URL") +} + +func TestTestConnector_AllowsURLChangeWithNewToken(t *testing.T) { + oldRepo := &staticTestRepository{ + cfg: testGitHubRepository("test", "default", "https://github.com/grafana/old"), + } + oldRepo.cfg.Secure.Token = common.InlineSecureValue{Name: "old-token"} + + tmpRepo := &staticTestRepository{ + cfg: testGitHubRepository("test", "default", "https://github.com/grafana/new"), + rsp: &provisioningv0alpha1.TestResults{Success: true, Code: http.StatusOK}, + } + + repoFactory := repository.NewMockFactory(t) + repoFactory.EXPECT().Build(mock.Anything, mock.MatchedBy(func(cfg *provisioningv0alpha1.Repository) bool { + return cfg.URL() == "https://github.com/grafana/new" && + cfg.Secure.Token.Create == common.RawSecureValue("new-token") + })).Return(tmpRepo, nil).Once() + + responder := &testResponder{} + connector := NewTestConnector( + &testConnectorDeps{repo: oldRepo, repoFactory: repoFactory}, + repository.NewTester(), + ) + handler, err := connector.Connect(request.WithNamespace(context.Background(), "default"), "test", nil, responder) + require.NoError(t, err) + + body := `{"spec":{"title":"Test Repo","type":"github","github":{"url":"https://github.com/grafana/new","branch":"main"}},"secure":{"token":{"create":"new-token"}}}` + handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(body))) + + require.NoError(t, responder.err) + assert.Equal(t, http.StatusOK, responder.statusCode) + assert.True(t, tmpRepo.testCalled) +} + +func TestTestConnector_AllowsUnchangedURLWithoutNewToken(t *testing.T) { + oldRepo := &staticTestRepository{ + cfg: testGitHubRepository("test", "default", "https://github.com/grafana/repo"), + } + oldRepo.cfg.Secure.Token = common.InlineSecureValue{Name: "old-token"} + + tmpRepo := &staticTestRepository{ + cfg: testGitHubRepository("test", "default", "https://github.com/grafana/repo"), + rsp: &provisioningv0alpha1.TestResults{Success: true, Code: http.StatusOK}, + } + + repoFactory := repository.NewMockFactory(t) + repoFactory.EXPECT().Build(mock.Anything, mock.MatchedBy(func(cfg *provisioningv0alpha1.Repository) bool { + return cfg.URL() == "https://github.com/grafana/repo" && + cfg.Secure.Token.Name == "old-token" + })).Return(tmpRepo, nil).Once() + + responder := &testResponder{} + connector := NewTestConnector( + &testConnectorDeps{repo: oldRepo, repoFactory: repoFactory}, + repository.NewTester(), + ) + handler, err := connector.Connect(request.WithNamespace(context.Background(), "default"), "test", nil, responder) + require.NoError(t, err) + + body := `{"spec":{"title":"Updated Title","type":"github","github":{"url":"https://github.com/grafana/repo","branch":"main"}}}` + handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest(http.MethodPost, "/test", strings.NewReader(body))) + + require.NoError(t, responder.err) + assert.Equal(t, http.StatusOK, responder.statusCode) + assert.True(t, tmpRepo.testCalled) +} + +type testResponder struct { + statusCode int + object runtime.Object + err error +} + +func (r *testResponder) Object(statusCode int, obj runtime.Object) { + r.statusCode = statusCode + r.object = obj +} + +func (r *testResponder) Error(err error) { + r.err = err +} + +type testConnectorDeps struct { + repo repository.Repository + repoFactory repository.Factory +} + +func (d *testConnectorDeps) GetRepository(_ context.Context, _ string) (repository.Repository, error) { + return d.repo, nil +} + +func (d *testConnectorDeps) GetHealthyRepository(_ context.Context, _ string) (repository.Repository, error) { + return d.repo, nil +} + +func (d *testConnectorDeps) GetConnection(_ context.Context, _ string) (connection.Connection, error) { + return nil, nil +} + +func (d *testConnectorDeps) GetStatusPatcher() *appcontroller.RepositoryStatusPatcher { + return nil +} + +func (d *testConnectorDeps) GetHealthChecker() *provisioningcontroller.RepositoryHealthChecker { + return nil +} + +func (d *testConnectorDeps) GetRepoFactory() repository.Factory { + return d.repoFactory +} + +type staticTestRepository struct { + cfg *provisioningv0alpha1.Repository + rsp *provisioningv0alpha1.TestResults + testCalled bool +} + +func (r *staticTestRepository) Config() *provisioningv0alpha1.Repository { + return r.cfg +} + +func (r *staticTestRepository) Test(context.Context) (*provisioningv0alpha1.TestResults, error) { + r.testCalled = true + return r.rsp, nil +} + +func testGitHubRepository(name, namespace, url string) *provisioningv0alpha1.Repository { + return &provisioningv0alpha1.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Finalizers: []string{repository.CleanFinalizer}, + }, + Spec: provisioningv0alpha1.RepositorySpec{ + Title: "Test Repo", + Type: provisioningv0alpha1.GitHubRepositoryType, + GitHub: &provisioningv0alpha1.GitHubRepositoryConfig{ + URL: url, + Branch: "main", + }, + }, + } +} diff --git a/pkg/tests/apis/provisioning/git/url_token_test.go b/pkg/tests/apis/provisioning/git/url_token_test.go new file mode 100644 index 0000000000000..b174dcb636ace --- /dev/null +++ b/pkg/tests/apis/provisioning/git/url_token_test.go @@ -0,0 +1,164 @@ +package git + +import ( + "context" + "encoding/json" + "net/http" + "testing" + + provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" + apicommon "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" + "github.com/grafana/nanogit/gittest" + "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" + + "github.com/grafana/grafana/pkg/tests/apis/provisioning/common" +) + +func TestIntegrationProvisioning_GitRequiresNewTokenWhenRepositoryURLChanges(t *testing.T) { + helper := sharedGitHelper(t) + ctx := context.Background() + + repoName := "git-url-change-test" + helper.CreateGitRepo(t, repoName, map[string][]byte{ + "dashboard.json": common.DashboardJSON(repoName+"-dashboard", "Dashboard", 1), + }, "write") + + t.Run("update rejects url change without a new token", func(t *testing.T) { + require.EventuallyWithT(t, func(collect *assert.CollectT) { + repo, err := helper.Repositories.Resource.Get(ctx, repoName, metav1.GetOptions{}) + require.NoError(collect, err) + updated := common.MustToUnstructured(t, repo) + require.NoError(collect, unstructured.SetNestedField(updated.Object, "https://some-new-url/", "spec", "git", "url")) + unstructured.RemoveNestedField(updated.Object, "secure", "token") + + _, err = helper.Repositories.Resource.Update(ctx, updated, metav1.UpdateOptions{}) + require.Error(collect, err) + require.True(collect, apierrors.IsInvalid(err), "expected invalid repository update, got %v", err) + require.ErrorContains(collect, err, "secure.token") + require.ErrorContains(collect, err, "a new token is required when changing the repository URL") + }, common.WaitTimeoutDefault, common.WaitIntervalDefault) + }) + + t.Run("update allows url change with a new token", func(t *testing.T) { + changedRemote, changedUser := createEmptyGitRepo(t, helper, "git-url-change-new-token-new") + + require.EventuallyWithT(t, func(collect *assert.CollectT) { + repo, err := helper.Repositories.Resource.Get(ctx, repoName, metav1.GetOptions{}) + require.NoError(collect, err) + + repoObj := common.MustFromUnstructured[provisioning.Repository](t, repo) + repoObj.Spec.Git ... [truncated]