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

Fix for GHSA-2q5c-qw9c-fmvq means it is not possible to determine the non-existence of an Application, even with correct permissions. #13000

Closed
3 tasks done
milesarmstrong opened this issue Mar 24, 2023 · 7 comments
Labels
bug Something isn't working

Comments

@milesarmstrong
Copy link
Contributor

milesarmstrong commented Mar 24, 2023

Describe the bug
The fix for GHSA-2q5c-qw9c-fmvq means it is no longer possible to determine the non-existence of an Application.

We upgraded our deployment of the argo-cd Helm chart from v5.24.4 to v5.27.3 this morning to address the Github RSA SSH key rotation.

As per the changelog this means argo-cd was upgraded from version v2.6.4 to v2.6.7.

We use the argo-cd API to interact with Applications, in particular using the Golang library we do something like (incomplete sample):

selector := fmt.Sprintf("app=%s", service)

appQuery := application.ApplicationQuery{
  Name:     &previewName,
  Projects: []string{"preview"},
  Selector: &selector,
}

argoApp, err := argo.client.Get(ctx, &appQuery)

switch status.Code(err) {
case codes.OK:
  previewExists = true
  // Do something

case codes.NotFound:
  // Do a different thing

default:
  return err
}

With the fix introduced in 3a28c8a, the API began returning codes.PermissionDenied instead of codes.NotFound. This breaking change wasn't noted in release notes.

The role that issues the token used for this call has the following permissions, i.e. it does have permission to get any application in the preview project:

policies:
- p, proj:preview:preview-branch-deploys, applications, delete, preview/an-application, deny
- p, proj:preview:preview-branch-deploys, applications, sync, preview/an-application, deny
- p, proj:preview:preview-branch-deploys, applications, override, preview/an-application, deny
- p, proj:preview:preview-branch-deploys, applications, delete, preview/a-different-application, deny
- p, proj:preview:preview-branch-deploys, applications, sync, preview/a-different-application, deny
- p, proj:preview:preview-branch-deploys, applications, override, preview/a-different-application, deny
- p, proj:preview:preview-branch-deploys, applications, *, preview/*, allow

To Reproduce

With a token that has permissions to get any Application from a project, try to fetch a non-existent Application. You will receive a PermissionDenied error. It doesn't seem possible to distinguish between a genuine permissions error, and a non-existent Application.

Expected behavior

When using a token that has appropriate permissions, it should be possible to confirm whether an Application exists or not.

Version

➜  argocd version --grpc-web --server *****
argocd: v2.6.7+5bcd846
  BuildDate: 2023-03-23T15:17:30Z
  GitCommit: 5bcd846fa16e4b19d8f477de7da50ec0aef320e5
  GitTreeState: clean
  GoVersion: go1.18.10
  Compiler: gc
  Platform: darwin/amd64
argocd-server: v2.6.7+5bcd846

Logs

(some fields removed/redacted)

{
  "jsonPayload": {
    "span.kind": "server",
    "msg": "received unary call /application.ApplicationService/Get",
    "grpc.service": "application.ApplicationService",
    "grpc.request.content": {
      "projects": [
        "preview"
      ],
      "name": "****",
      "selector": "app=****"
    },
    "level": "info",
    "system": "grpc",
    "grpc.start_time": "2023-03-24T14:51:50Z",
    "grpc.method": "Get"
  },
  "severity": "INFO"
}

{
  "jsonPayload": {
    "system": "grpc",
    "grpc.time_ms": 11.646,
    "level": "warning",
    "grpc.start_time": "2023-03-24T14:51:50Z",
    "msg": "finished unary call with code PermissionDenied",
    "grpc.method": "Get",
    "grpc.service": "application.ApplicationService",
    "span.kind": "server",
    "error": "rpc error: code = PermissionDenied desc = permission denied",
    "grpc.code": "PermissionDenied"
  },
  "severity": "WARNING"
}

{
  "jsonPayload": {
    "namespace": "argocd",
    "msg": "application does not exist",
    "application": "****",
    "level": "warning"
  },
  "severity": "WARNING"
}

Checklist:

  • I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
  • I've included steps to reproduce the bug.
  • I've pasted the output of argocd version.
@milesarmstrong milesarmstrong added the bug Something isn't working label Mar 24, 2023
@crenshaw-dev
Copy link
Member

crenshaw-dev commented Mar 24, 2023

This breaking change wasn't noted in release notes.

That's my bad, I will write up notes about this being a breaking change.

When using a token that has appropriate permissions, it should be possible to confirm whether an Application exists or not.

It seems simple enough to see applications, get, */* and say, "well it can get all apps, so there's no risk in confirming that an app does not exist."

However, the code which decides whether to return "does not exist" or "unauthorized" is pretty far away from the code which parses the policies. The Casbin library kind of knows that the policy grants full access. But it doesn't surface that information to Argo CD.

I think the proper way to fix the API is a different breaking change: require the application request to be fully-parameterized. In other words, the API request must include the application name, namespace, and project (all the pieces which are used in determining access). This lets us answer the question "would you have access to that app" without first going and getting the app to get the remaining parameters (e.g. the app's project name). We considered this change a while back and rejected it in favor of returning Unauthorized. I think that in Argo CD 3.0 we should opt for requiring fully-parameterized requests.

@crenshaw-dev
Copy link
Member

One workaround would be to list all applications and then check the list for the presence of the desired application.

@milesarmstrong
Copy link
Contributor Author

Thanks, I appreciate the explanation!

We could also provide the namespace in our call, but I'm assuming that providing all three parameters in the request won't currently result in a NotFound rather than PermissionDenied?

@crenshaw-dev
Copy link
Member

@milesarmstrong that's correct.... I'm thinking about whether we could provide that as an option without breaking existing functionality much. I think the way GET works now is that if you provide, say, a bogus "project," we just ignore that and roll with the one in the actual app spec. If we started respecting the bogus project, we'd break those requests (but I'd consider that a relatively small number of requests).

@crenshaw-dev
Copy link
Member

Added some warnings to the release notes: https://github.com/argoproj/argo-cd/releases/tag/v2.6.7

I'll also add them to the upgrade notes.

I'm a bit hand-wavy about exactly what API calls might be affected, because it was a sweeping change across the whole Applications service. I'll try to find out if I can be more precise.

@mcanevet
Copy link
Contributor

terraform-provider-argocd looks impacted by that. /cc @oboukili

onematchfox added a commit to onematchfox/terraform-provider-argocd that referenced this issue Mar 28, 2023
Works around breaking change made to `Get` in order to resolve GHSA-2q5c-qw9c-fmvq. 

See also argoproj/argo-cd#13000
onematchfox added a commit to onematchfox/terraform-provider-argocd that referenced this issue Mar 28, 2023
Works around breaking change made to `Get` in order to resolve GHSA-2q5c-qw9c-fmvq. 

See also argoproj/argo-cd#13000
onematchfox added a commit to onematchfox/terraform-provider-argocd that referenced this issue Mar 29, 2023
Works around breaking change made to `Get` in order to resolve GHSA-2q5c-qw9c-fmvq. 

See also argoproj/argo-cd#13000
@nitishkrishna
Copy link

nitishkrishna commented Apr 14, 2023

Can confirm that the exact same issue happens when checking for the existence of a cluster

argocd cluster get https://<addr> --auth-token <token>
FATA[0000] rpc error: code = PermissionDenied desc = permission denied

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants