From 945fda9f5676ce44b107e4fa5d4950a6697eed4c Mon Sep 17 00:00:00 2001 From: Blake Pettersson Date: Sun, 3 Jul 2022 16:27:42 +0200 Subject: [PATCH] feat: add deny destinations for projects (#9464) This adds the ability to selectively deny destinations, by prefixing either its `namespace` or `server` with a `!`. Closes #9464. Signed-off-by: Blake Pettersson --- docs/user-guide/projects.md | 30 +++ .../application/v1alpha1/app_project_types.go | 43 +++- pkg/apis/application/v1alpha1/types.go | 8 +- pkg/apis/application/v1alpha1/types_test.go | 228 +++++++++++++++++- test/e2e/app_management_test.go | 53 ++++ test/e2e/project_management_test.go | 14 ++ 6 files changed, 363 insertions(+), 13 deletions(-) diff --git a/docs/user-guide/projects.md b/docs/user-guide/projects.md index b1494f549263d..50c43ed98b3e4 100644 --- a/docs/user-guide/projects.md +++ b/docs/user-guide/projects.md @@ -54,6 +54,36 @@ argocd proj add-destination , argocd proj remove-destination , ``` +We can also do negations of destinations (i.e. install anywhere _apart from_). + +```bash +argocd proj add-destination !,! +argocd proj remove-destination !,! +``` + +Declaratively we can do something like this: + +```yaml +spec: + destinations: + # Do not allow any app to be installed in `kube-system` + - namespace: '!kube-system' + server: '*' + # Or any cluster that has a URL of `team1-*` + - namespace: '*' + server: '!https://team1-*' + # Any other namespace or server is fine though. + - namespace: '*' + server: '*' +``` + +A destination is considered valid if the following conditions hold: + +1) _Any_ allow destination rule (i.e. a rule which isn't prefixed with `!`) permits the destination +2) AND *no* deny destination (i.e. a rule which is prefixed with `!`) rejects the destination + +Keep in mind that `!*` is an invalid rule, since it doesn't make any sense to disallow everything. + Permitted destination K8s resource kinds are managed with the commands. Note that namespaced-scoped resources are restricted via a deny list, whereas cluster-scoped resources are restricted via allow list. diff --git a/pkg/apis/application/v1alpha1/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index 562b91a77ae3b..cdf358f348e71 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -154,6 +154,18 @@ func (p *AppProject) ValidateJWTTokenID(roleName string, id string) error { func (p *AppProject) ValidateProject() error { destKeys := make(map[string]bool) for _, dest := range p.Spec.Destinations { + if dest.Name == "!*" { + return status.Errorf(codes.InvalidArgument, "name has an invalid format, '!*'") + } + + if dest.Server == "!*" { + return status.Errorf(codes.InvalidArgument, "server has an invalid format, '!*'") + } + + if dest.Namespace == "!*" { + return status.Errorf(codes.InvalidArgument, "namespace has an invalid format, '!*'") + } + key := fmt.Sprintf("%s/%s", dest.Server, dest.Namespace) if _, ok := destKeys[key]; ok { return status.Errorf(codes.InvalidArgument, "destination '%s' already added", key) @@ -336,7 +348,11 @@ func (proj *AppProject) RemoveFinalizer() { setFinalizer(&proj.ObjectMeta, ResourcesFinalizerName, false) } -func globMatch(pattern string, val string, separators ...rune) bool { +func globMatch(pattern string, val string, allowNegation bool, separators ...rune) bool { + if allowNegation && isDenyDestination(pattern) { + return !glob.Match(pattern[1:], val, separators...) + } + if pattern == "*" { return true } @@ -348,7 +364,7 @@ func (proj AppProject) IsSourcePermitted(src ApplicationSource) bool { srcNormalized := git.NormalizeGitURL(src.RepoURL) for _, repoURL := range proj.Spec.SourceRepos { normalized := git.NormalizeGitURL(repoURL) - if globMatch(normalized, srcNormalized, '/') { + if globMatch(normalized, srcNormalized, false, '/') { return true } } @@ -357,14 +373,27 @@ func (proj AppProject) IsSourcePermitted(src ApplicationSource) bool { // IsDestinationPermitted validates if the provided application's destination is one of the allowed destinations for the project func (proj AppProject) IsDestinationPermitted(dst ApplicationDestination) bool { + anyDestinationMatched := false + noDenyDestinationsMatched := true + for _, item := range proj.Spec.Destinations { - dstNameMatched := dst.Name != "" && globMatch(item.Name, dst.Name) - dstServerMatched := dst.Server != "" && globMatch(item.Server, dst.Server) - if (dstServerMatched || dstNameMatched) && globMatch(item.Namespace, dst.Namespace) { - return true + dstNameMatched := dst.Name != "" && globMatch(item.Name, dst.Name, true) + dstServerMatched := dst.Server != "" && globMatch(item.Server, dst.Server, true) + dstNamespaceMatched := globMatch(item.Namespace, dst.Namespace, true) + + matched := (dstServerMatched || dstNameMatched) && dstNamespaceMatched + if matched { + anyDestinationMatched = true + } else if ((!dstNameMatched && isDenyDestination(item.Name)) || (!dstServerMatched && isDenyDestination(item.Server))) || (!dstNamespaceMatched && isDenyDestination(item.Namespace)) { + noDenyDestinationsMatched = false } } - return false + + return anyDestinationMatched && noDenyDestinationsMatched +} + +func isDenyDestination(pattern string) bool { + return strings.HasPrefix(pattern, "!") } // TODO: document this method diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index e8860128f99c5..92f264d05e678 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -1836,7 +1836,7 @@ func (w *SyncWindows) Matches(app *Application) *SyncWindows { for _, w := range *w { if len(w.Applications) > 0 { for _, a := range w.Applications { - if globMatch(a, app.Name) { + if globMatch(a, app.Name, false) { matchingWindows = append(matchingWindows, w) break } @@ -1845,8 +1845,8 @@ func (w *SyncWindows) Matches(app *Application) *SyncWindows { if len(w.Clusters) > 0 { for _, c := range w.Clusters { dst := app.Spec.Destination - dstNameMatched := dst.Name != "" && globMatch(c, dst.Name) - dstServerMatched := dst.Server != "" && globMatch(c, dst.Server) + dstNameMatched := dst.Name != "" && globMatch(c, dst.Name, false) + dstServerMatched := dst.Server != "" && globMatch(c, dst.Server, false) if dstNameMatched || dstServerMatched { matchingWindows = append(matchingWindows, w) break @@ -1855,7 +1855,7 @@ func (w *SyncWindows) Matches(app *Application) *SyncWindows { } if len(w.Namespaces) > 0 { for _, n := range w.Namespaces { - if globMatch(n, app.Spec.Destination.Namespace) { + if globMatch(n, app.Spec.Destination.Namespace, false) { matchingWindows = append(matchingWindows, w) break } diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index a06facf59debf..655413ef5e8fa 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -141,7 +141,154 @@ func TestAppProject_IsDestinationPermitted(t *testing.T) { Destinations: data.projDest, }, } - assert.Equal(t, proj.IsDestinationPermitted(data.appDest), data.isPermitted) + assert.Equal(t, data.isPermitted, proj.IsDestinationPermitted(data.appDest)) + } +} + +func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { + testData := []struct { + projDest []ApplicationDestination + appDest ApplicationDestination + isPermitted bool + }{{ + projDest: []ApplicationDestination{{ + Server: "!https://kubernetes.default.svc", Namespace: "default", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "default"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "https://kubernetes.default.svc", Namespace: "!default", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "kube-system"}, + isPermitted: true, + }, { + projDest: []ApplicationDestination{{ + Server: "!https://my-cluster", Namespace: "default", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "default"}, + isPermitted: true, + }, { + projDest: []ApplicationDestination{{ + Server: "!https://kubernetes.default.svc", Namespace: "*", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "kube-system"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "!https://*.default.svc", Namespace: "default", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "default"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "!https://team1-*", Namespace: "default", + }}, + appDest: ApplicationDestination{Server: "https://test2-dev-cluster", Namespace: "default"}, + isPermitted: true, + }, { + projDest: []ApplicationDestination{{ + Server: "https://kubernetes.default.svc", Namespace: "!test-*", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "test-foo"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "https://kubernetes.default.svc", Namespace: "!test-*", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "test"}, + isPermitted: true, + }, { + projDest: []ApplicationDestination{{ + Server: "", Namespace: "*", Name: "!test", + }}, + appDest: ApplicationDestination{Name: "test", Namespace: "test"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "", Namespace: "*", Name: "!test2", + }}, + appDest: ApplicationDestination{Name: "test", Namespace: "test"}, + isPermitted: true, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "kube-system", + }, { + Server: "*", Namespace: "!kube-system", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "kube-system"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "*", + }, { + Server: "*", Namespace: "!kube-system", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "kube-system"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "https://kubernetes.default.svc", Namespace: "*", + }, { + Server: "!https://kubernetes.default.svc", Namespace: "*", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "kube-system"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "*", + }, { + Server: "!https://kubernetes.default.svc", Namespace: "kube-system", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "kube-system"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "*", + }, { + Server: "!https://kubernetes.default.svc", Namespace: "kube-system", + }, { + Server: "*", Namespace: "!kube-system", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "kube-system"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "*", + }, { + Server: "!https://kubernetes.default.svc", Namespace: "kube-system", + }, { + Server: "*", Namespace: "!kube-system", + }}, + appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "default"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "*", + }, { + Server: "!https://kubernetes.default.svc", Namespace: "kube-system", + }, { + Server: "*", Namespace: "!kube-system", + }}, + appDest: ApplicationDestination{Server: "https://test-dev-cluster", Namespace: "kube-system"}, + isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "", Namespace: "*", Name: "test", + }, { + Server: "", Namespace: "*", Name: "!test", + }}, + appDest: ApplicationDestination{Name: "test", Namespace: "test"}, + isPermitted: false, + }} + + for _, data := range testData { + proj := AppProject{ + Spec: AppProjectSpec{ + Destinations: data.projDest, + }, + } + assert.Equal(t, data.isPermitted, proj.IsDestinationPermitted(data.appDest)) } } @@ -260,11 +407,88 @@ func TestAppProject_RemoveGroupFromRole(t *testing.T) { func newTestProject() *AppProject { p := AppProject{ ObjectMeta: metav1.ObjectMeta{Name: "my-proj"}, - Spec: AppProjectSpec{Roles: []ProjectRole{{Name: "my-role"}}}, + Spec: AppProjectSpec{Roles: []ProjectRole{{Name: "my-role"}}, Destinations: []ApplicationDestination{{}}}, } return &p } +// TestAppProject_ValidateDestinations tests for an invalid destination +func TestAppProject_ValidateDestinations(t *testing.T) { + p := newTestProject() + err := p.ValidateProject() + assert.NoError(t, err) + badNamespaces := []string{ + "!*", + } + for _, badName := range badNamespaces { + p.Spec.Destinations[0].Namespace = badName + err = p.ValidateProject() + assert.Error(t, err) + } + + goodNamespaces := []string{ + "*", + "some-namespace", + } + for _, goodNamespace := range goodNamespaces { + p.Spec.Destinations[0].Namespace = goodNamespace + err = p.ValidateProject() + assert.NoError(t, err) + } + + badServers := []string{ + "!*", + } + for _, badServer := range badServers { + p.Spec.Destinations[0].Server = badServer + err = p.ValidateProject() + assert.Error(t, err) + } + + goodServers := []string{ + "*", + "some-server", + } + for _, badName := range goodServers { + p.Spec.Destinations[0].Server = badName + err = p.ValidateProject() + assert.NoError(t, err) + } + + badNames := []string{ + "!*", + } + for _, badName := range badNames { + p.Spec.Destinations[0].Name = badName + err = p.ValidateProject() + assert.Error(t, err) + } + + goodNames := []string{ + "*", + "some-name", + } + for _, goodName := range goodNames { + p.Spec.Destinations[0].Name = goodName + err = p.ValidateProject() + assert.NoError(t, err) + } + + validDestination := ApplicationDestination{ + Server: "some-server", + Namespace: "some-namespace", + } + + p.Spec.Destinations[0] = validDestination + err = p.ValidateProject() + assert.NoError(t, err) + + //no duplicates allowed + p.Spec.Destinations = []ApplicationDestination{validDestination, validDestination} + err = p.ValidateProject() + assert.Error(t, err) +} + // TestValidateRoleName tests for an invalid role name func TestAppProject_ValidateRoleName(t *testing.T) { p := newTestProject() diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go index 815f486573619..fa8c1b8f83443 100644 --- a/test/e2e/app_management_test.go +++ b/test/e2e/app_management_test.go @@ -1245,7 +1245,60 @@ func TestPermissionDeniedWithScopedRepo(t *testing.T) { CreateApp(). Then(). Expect(Error("", "is not permitted in project")) +} +func TestPermissionDeniedWithNegatedNamespace(t *testing.T) { + projName := "argo-project" + projectFixture. + Given(t). + Name(projName). + Destination("*,!*test-permission-denied-with-negated-namespace*"). + When(). + Create() + + repoFixture.Given(t, true). + When(). + Path(RepoURL(RepoURLTypeFile)). + Project(projName). + Create() + + GivenWithSameState(t). + Project(projName). + RepoURLType(RepoURLTypeFile). + Path("two-nice-pods"). + When(). + PatchFile("pod-1.yaml", `[{"op": "add", "path": "/metadata/annotations", "value": {"argocd.argoproj.io/sync-options": "Prune=false"}}]`). + IgnoreErrors(). + CreateApp(). + Then(). + Expect(Error("", "is not permitted in project")) +} + +func TestPermissionDeniedWithNegatedServer(t *testing.T) { + projName := "argo-project" + projectFixture. + Given(t). + Name(projName). + Destination("!https://kubernetes.default.svc,*"). + When(). + Create() + + repoFixture.Given(t, true). + When(). + Path(RepoURL(RepoURLTypeFile)). + Project(projName). + Create() + + GivenWithSameState(t). + Project(projName). + RepoURLType(RepoURLTypeFile). + Path("two-nice-pods"). + When(). + PatchFile("pod-1.yaml", `[{"op": "add", "path": "/metadata/annotations", "value": {"argocd.argoproj.io/sync-options": "Prune=false"}}]`). + IgnoreErrors(). + CreateApp(). + Then(). + Expect(Error("", "is not permitted in project")) } // make sure that if we deleted a resource from the app, it is not pruned if annotated with Prune=false diff --git a/test/e2e/project_management_test.go b/test/e2e/project_management_test.go index 83102a5765cc7..d8aa7836da83b 100644 --- a/test/e2e/project_management_test.go +++ b/test/e2e/project_management_test.go @@ -166,6 +166,20 @@ func TestAddProjectDestination(t *testing.T) { assert.Error(t, err) assert.True(t, strings.Contains(err.Error(), "already defined")) + _, err = fixture.RunCli("proj", "add-destination", projectName, + "!*", + "test1", + ) + assert.Error(t, err) + assert.True(t, strings.Contains(err.Error(), "server has an invalid format, '!*'")) + + _, err = fixture.RunCli("proj", "add-destination", projectName, + "https://192.168.99.100:8443", + "!*", + ) + assert.Error(t, err) + assert.True(t, strings.Contains(err.Error(), "namespace has an invalid format, '!*'")) + proj, err := fixture.AppClientset.ArgoprojV1alpha1().AppProjects(fixture.ArgoCDNamespace).Get(context.Background(), projectName, metav1.GetOptions{}) assert.NoError(t, err) assert.Equal(t, projectName, proj.Name)