Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permit only Project-scoped clusters to be a valid Destination #10220

Closed
blakepettersson opened this issue Aug 5, 2022 · 2 comments · Fixed by #10237
Closed

Permit only Project-scoped clusters to be a valid Destination #10220

blakepettersson opened this issue Aug 5, 2022 · 2 comments · Fixed by #10237
Labels
enhancement New feature or request security Security related

Comments

@blakepettersson
Copy link
Member

blakepettersson commented Aug 5, 2022

Summary

It seems to be the case that project-scoped Applications can be installed onto clusters which are not a part of the same project.

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: "some-app"
spec:
  destination:
    # This destination might not actually be a cluster which belongs to `foo-project`
    server: https://some-k8s-server/
    namespace: "some-ns"
  project: foo-project

Motivation

So far this has not been a problem, but it would be nice to ensure that this situation cannot happen at all. We have a bunch of developers that are slinging Applications left and right, and we'd like to ensure that mistakes (or something more... sinister) cannot happen.

Proposal

In AppProjectSpec we would add an extra attribute, permitOnlyProjectScopedClusters or something similar.

Then in app_project_types.go, we can extend the existing validation we have on IsDestinationPermitted to be something like this:

func (proj AppProject) IsDestinationPermitted(dst ApplicationDestination, projectClusters func(project string) ([]*Cluster, error)) (bool, error) {
        // The existing logic we have for isDestinationPermitted
	destinationMatched := proj.isDestinationPermitted(dst) 
	if destinationMatched && proj.Spec.PermitOnlyProjectScopedClusters {
		clusters, err := projectClusters(proj.Name)
		if err != nil {
			return false, err
		}

		// TODO: If no project clusters are found, return an error instead?
		for _, cluster := range clusters {
			if cluster.Server == dst.Server {
				return true, nil
			}
		}

		return false, nil
	}

	return destinationMatched, nil
}

projectClusters is a function which would wrap ArgoDB.GetProjectClusters, where we'd get all clusters for the given AppProject and then check if that a destination is indeed a part of the given AppProject.

@blakepettersson blakepettersson added the enhancement New feature or request label Aug 5, 2022
@crenshaw-dev crenshaw-dev added the security Security related label Aug 5, 2022
@crenshaw-dev
Copy link
Member

crenshaw-dev commented Aug 5, 2022

Would you be up for writing a PR?

@alexmt and @jannfis do you agree that this restriction would make sense?

@blakepettersson
Copy link
Member Author

Yes, of course 😄

blakepettersson added a commit to blakepettersson/argo-cd that referenced this issue Aug 19, 2022
This commit adds a new flag, `permitOnlyProjectScopedClusters`, which
prevents any application from syncing to clusters which are not a part
of the same project. Fixes argoproj#10220.

Signed-off-by: Blake Pettersson <[email protected]>
crenshaw-dev pushed a commit that referenced this issue Sep 8, 2022
This commit adds a new flag, `permitOnlyProjectScopedClusters`, which
prevents any application from syncing to clusters which are not a part
of the same project. Fixes #10220.

Signed-off-by: Blake Pettersson <[email protected]>

Signed-off-by: Blake Pettersson <[email protected]>
ocraviotto pushed a commit to ocraviotto/argo-cd that referenced this issue Sep 15, 2022
This commit adds a new flag, `permitOnlyProjectScopedClusters`, which
prevents any application from syncing to clusters which are not a part
of the same project. Fixes argoproj#10220.

Signed-off-by: Blake Pettersson <[email protected]>

Signed-off-by: Blake Pettersson <[email protected]>
docs: remove duplicate word in user-management doc (argoproj#10546)

Signed-off-by: Mickaël Canévet <[email protected]>

Signed-off-by: Mickaël Canévet <[email protected]>
fix: hide terminal on the non-pod resource kind (argoproj#9980) (argoproj#10556)

Signed-off-by: ashutosh16 <[email protected]>

Signed-off-by: ashutosh16 <[email protected]>
fix: add more info to  creationtime format (argoproj#10286) (argoproj#10493)

* fix: add more info to  creationtime format

Signed-off-by: Ashutosh <[email protected]>

* lint issue

Signed-off-by: ashutosh16 <[email protected]>

* fix: add more info to creationtime format

Signed-off-by: ashutosh16 <[email protected]>

Signed-off-by: Ashutosh <[email protected]>
Signed-off-by: ashutosh16 <[email protected]>
Signed-off-by: ashutosh16 <[email protected]>
Co-authored-by: Ashutosh <[email protected]>
feat: support multiple sources for application

Signed-off-by: ishitasequeira <[email protected]>

remove debug logging and unwanted code

Signed-off-by: ishitasequeira <[email protected]>

fix lint and unit test errors

Signed-off-by: ishitasequeira <[email protected]>

fix lint and unit test errors

Signed-off-by: ishitasequeira <[email protected]>

Merge branch 'multiple-sources-for-applications' of github.com:ishitasequeira/argo-cd into multiple-sources-for-applications

feat: support multiple sources for application

Signed-off-by: ishitasequeira <[email protected]>

remove debug logging and unwanted code

Signed-off-by: ishitasequeira <[email protected]>

fix lint and unit test errors

Signed-off-by: ishitasequeira <[email protected]>

fix lint and unit test errors

Signed-off-by: ishitasequeira <[email protected]>

fix bug introduced after rebase

Signed-off-by: ishitasequeira <[email protected]>

executed make codegen

Signed-off-by: ishitasequeira <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request security Security related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants