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

feat: appset scm generators and PR generators should be able to access only secrets related to appset #20309

Merged

Conversation

pasha-codefresh
Copy link
Member

@pasha-codefresh pasha-codefresh commented Oct 9, 2024

Apps-in-any-namespace allows non-admin Argo CD users to manage their own Application manifests via GitOps or kubectl by restricting which AppProjects may be used from a given Applications namespace.

AppSets-in-any-namespace expands that functionality to ApplicationSets. But instead of only restricting AppProjects, it also requires that the admin specify which SCM providers the non-admin users may reference from their ApplicationSets. Setting the --allowed-scm-providers flag on the ApplicationSet controller allows the Argo CD administrator to limit which domains the ApplicationSet controller may connect to using user-specific SCM credentials.

Since non-admin users may set the tokenRef field of an SCM or Pull Request generator, they may cause any Secret stored in the argocd namespace to be sent to the specified SCM provider. The SCM provider allowlist ensures that the destination is trustworthy.

Argo CD's failure to enforce that allow list on Pull Request generators targeting the Bitbucket or Azure DevOps SCMs may allow a user to leak credentials from the argocd namespace. The user would have to control some external domain and be able to read the headers of incoming requests.

Credit and big thanks to @crenshaw-dev for implementing it.

…s only secrets related to appset

Signed-off-by: pashakostohrys <pavel@codefresh.io>
@pasha-codefresh pasha-codefresh requested review from a team as code owners October 9, 2024 10:22
Copy link

bunnyshell bot commented Oct 9, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

Signed-off-by: pashakostohrys <pavel@codefresh.io>
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 56.01%. Comparing base (195de1a) to head (315c5da).
Report is 368 commits behind head on master.

Files with missing lines Patch % Lines
applicationset/generators/pull_request.go 12.50% 7 Missing ⚠️
applicationset/generators/scm_provider.go 33.33% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #20309   +/-   ##
=======================================
  Coverage   56.01%   56.01%           
=======================================
  Files         322      322           
  Lines       44742    44747    +5     
=======================================
+ Hits        25061    25064    +3     
  Misses      17077    17077           
- Partials     2604     2606    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!! Just a minor nit..

docs/user-guide/annotations-and-labels.md Outdated Show resolved Hide resolved
Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pasha-codefresh and others added 2 commits October 15, 2024 13:18
Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com>
Signed-off-by: pasha-codefresh <pavel@codefresh.io>
Signed-off-by: pashakostohrys <pavel@codefresh.io>
@pasha-codefresh
Copy link
Member Author

Can we also add the param in argo-cd/docs/operator-manual /argocd-cmd-params-cm.yaml ?

Added @ishitasequeira , let me know if you have other comments :)

Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pasha-codefresh!! LGTM!!

@ishitasequeira ishitasequeira merged commit a25378f into argoproj:master Oct 15, 2024
27 checks passed
austin5219 pushed a commit to austin5219/argo-cd that referenced this pull request Oct 16, 2024
…s only secrets related to appset (argoproj#20309)

Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>
adriananeci pushed a commit to adriananeci/argo-cd that referenced this pull request Dec 4, 2024
…s only secrets related to appset (argoproj#20309)

Signed-off-by: Adrian Aneci <aneci@adobe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants