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

Aggressively check RBAC when Application request is fully-parameterized #13393

Closed
crenshaw-dev opened this issue May 1, 2023 · 3 comments · Fixed by #13394
Closed

Aggressively check RBAC when Application request is fully-parameterized #13393

crenshaw-dev opened this issue May 1, 2023 · 3 comments · Fixed by #13394
Labels
enhancement New feature or request

Comments

@crenshaw-dev
Copy link
Member

Summary

GHSA-2q5c-qw9c-fmvq introduced a breaking API change. If an app does not exist, the API always returns a 403.

We can be a bit smarter than that. If the user provides the app name and the project name, we should aggressively perform an RBAC check. If they pass the RBAC check, and the app does not exist, we return a 404. This allows the user to determine the non-existence of the app without calling the can-i API.

However, this changes API behavior for users who send the app name and an incorrect project name, when they do not have access to the given app under the given project. Currently, if a user passes a correct name but an incorrect project name, the API "auto-corrects" the project name to the one actually existing on the application. After this change, we'll reject their request based on the given project name and ignore the actual project name.

Motivation

Intuit has a user who wants to know for certain whether an app exists without making an extra API call to the can-i endpoint. Right now, 403 could mean "the app doesn't exist" or "your token is expired." Since our user knows the app's project name, they can send both the app and project name on their request. Since the request is fully parameterized (with respect to the RBAC check) we can safely determine that they have access and tell them whether the app exists.

@crenshaw-dev crenshaw-dev added the enhancement New feature or request label May 1, 2023
@crenshaw-dev
Copy link
Member Author

This would mitigate the problems described in #13000

@alv91
Copy link

alv91 commented May 25, 2023

Hello, any update on this issue? :)

crenshaw-dev added a commit to crenshaw-dev/argo-cd that referenced this issue May 30, 2023
…ied (argoproj#13393)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

simplify, respond 404 on project specified but doesn't match, always fetch app

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

handle project updates

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
crenshaw-dev added a commit to crenshaw-dev/argo-cd that referenced this issue May 30, 2023
…ied (argoproj#13393)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

simplify, respond 404 on project specified but doesn't match, always fetch app

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

handle project updates

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@crenshaw-dev
Copy link
Member Author

@alv91 I'm going to push the PR forward, but it's not my top priority at the moment. Next ~2 weeks, I should pick it up.

crenshaw-dev added a commit that referenced this issue Jul 19, 2023
…ied (#13393) (#13394)

* fix(api): return 404 when the app is not found if a project is specified (#13393)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

simplify, respond 404 on project specified but doesn't match, always fetch app

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

handle project updates

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* handle new endpoint, fix bad merge

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* docs

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
gcp-cherry-pick-bot bot pushed a commit that referenced this issue Jul 19, 2023
…ied (#13393) (#13394)

* fix(api): return 404 when the app is not found if a project is specified (#13393)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

simplify, respond 404 on project specified but doesn't match, always fetch app

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

handle project updates

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* handle new endpoint, fix bad merge

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* docs

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
crenshaw-dev added a commit that referenced this issue Jul 19, 2023
…ied (#13393) (#13394) (#14600)

* fix(api): return 404 when the app is not found if a project is specified (#13393)



simplify, respond 404 on project specified but doesn't match, always fetch app



handle project updates



* handle new endpoint, fix bad merge



* docs



---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
yyzxw pushed a commit to yyzxw/argo-cd that referenced this issue Aug 9, 2023
…ied (argoproj#13393) (argoproj#13394)

* fix(api): return 404 when the app is not found if a project is specified (argoproj#13393)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

simplify, respond 404 on project specified but doesn't match, always fetch app

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

handle project updates

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* handle new endpoint, fix bad merge

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* docs

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
tesla59 pushed a commit to tesla59/argo-cd that referenced this issue Dec 16, 2023
…ied (argoproj#13393) (argoproj#13394)

* fix(api): return 404 when the app is not found if a project is specified (argoproj#13393)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

simplify, respond 404 on project specified but doesn't match, always fetch app

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

handle project updates

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* handle new endpoint, fix bad merge

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* docs

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants