feat: diff against application manifest (#4706)

[crenshaw-dev]

This change allows diffing a live application manifests vs. what the manifests would look like if some arbitrary app manifests were applied.

One major limitation is that the user must have "update" permissions for the target application in order to get this diff. It seems possible that an Argo CD operator might give someone "get" permission on an application with the belief that the user can't do certain potentially-sensitive actions (like change the source's path to reveal contents of another directory, or set a sensitive env var in a plugin).

I understand that one main use case for this command is in CI pipelines to predict the result of a given change. This limitation would force users to set an update-capable token in their CI, even though their CI doesn't really need to update anything.

If this limitation is too problematic, I suggest we tackle the problem from the other direction: instead of allowing users to pass a full, new application spec, we should determine which fields we want to set and enable them one-by-one as we determine their safety (as we did for --revision).

Looking forward to everyone's thoughts!

Closes #4706

[codecov]

Codecov Report

Patch coverage: 6.42% and project coverage change: -0.08 ⚠️

Comparison is base (f6e3139) 47.77% compared to head (fe1aeb8) 47.69%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12813      +/-   ##
==========================================
- Coverage   47.77%   47.69%   -0.08%     
==========================================
  Files         246      246              
  Lines       41985    42073      +88     
==========================================
+ Hits        20057    20066       +9     
- Misses      19929    20008      +79     
  Partials     1999     1999              

Impacted Files	Coverage Δ
cmd/argocd/commands/app.go	21.12% <0.00%> (-0.39%)	⬇️
server/application/application.go	28.84% <0.00%> (-0.69%)	⬇️
reposerver/repository/repository.go	60.56% <35.29%> (+0.06%)	⬆️
cmd/util/app.go	55.57% <50.00%> (ø)

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[MeNsaaH]

@crenshaw-dev, I'm not too sure I'm in line with requiring update permissions to be able to generate diffs. As you've said, it's a bit overkill for CIs that just want to see changes.

If this limitation is too problematic, I suggest we tackle the problem from the other direction: instead of allowing users to pass a full, new application spec, we should determine which fields we want to set and enable them one-by-one as we determine their safety (as we did for --revision).

As for this, I'm not too sure I follow whatyou mean by determining fields to be set and enabling them one-by-one.

[crenshaw-dev]

@MeNsaaH very fair, I don't love it either.

I'm not too sure I follow whatyou mean by determining fields to be set and enabling them one-by-one.

So, here's an example of a problem that I think makes it unsafe to enable diffing with just get access.

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: guestbook
spec:
  source:
    repoURL: https://github.com/argoproj/argocd-example-apps.git
    plugin:
      env:
      - name: POST_PROCESSOR_CMD
        value: "sed 's/:<tag>/:1.2.3/'"

The plugin designer uses an env var to run some arbitrary command after generating manifests. That's safe, as long as users aren't allowed to update manifests.

But if I allow the user to send an arbitrary application.yaml for diffing, they could provide a malicious command in that env var.

So I think spec.source.plugin.env is out of bounds. But my guess is that most users don't want to set that field. They want to set something like spec.source.helm.parameters, which should be reasonably safe.

So instead of accepting --app, we could just accept --set-helm-param.

What do you think? It's possible I'm being too paranoid, but I want to be very careful about code that runs on the repo-server.

[crenshaw-dev]

Alternatively, we could allow-list certain fields. The API server could ignore any out-of-bounds fields and return a warning to the user, "Hey, we ignored the changes you made to field X because it's unsafe to let you set that."

[MeNsaaH]

The plugin designer uses an env var to run some arbitrary command after generating manifests. That's safe, as long as users aren't allowed to update manifests.

Oh, I see now what direction you're coming from. And agree that it might need elevated permissions. I'm however not sure for most people, but for me this is mostly just basic diffs, but yeah I see the vulnerability here.

What do you think? It's possible I'm being too paranoid, but I want to be very careful about code that runs on the repo-server.

I think it's a valid concern.

[MeNsaaH]

Alternatively, we could allow-list certain fields. The API server could ignore any out-of-bounds fields and return a warning to the user, "Hey, we ignored the changes you made to field X because it's unsafe to let you set that."

For better DX, I think we could go with this. And maybe an option in the settings file to show these changes. I'm just curious how argocd will know what changes are safe to display and what changes aren't

[crenshaw-dev]

This would be my initial allow-list proposal:

spec:
  source:
    targetRevision: # could let you poke around git
    helm:
      parameters: # gets env-substituted, so you could get the destination k8s API version and versions of all CRDs on the k8s API - maybe not important?
      releaseName:
      ignoreMissingValueFiles:
      values:
      skipCrds:
      version:
    kustomize:
      version:
      namePrefix:
      nameSuffix:
      commonLabels:
      forceCommonLabels:
      commonAnnotations:
      commonAnnotationsEnvsubst:
      forceCommonAnnotations:
      images:
      namespace:
      replicas:
  destination:
    namespace:

I'm a bit hesitant about targetRevision, because that would let someone look at git repo contents that maybe they're not supposed to see. But it does seem like probably one of the most commonly-used fields for this feature.

So these fields would be blocked:

spec:
  project:
  source:
    repoURL:
    path:
    chart:
    helm:
      passCredentials:
      fileParameters: # would let you poke around git - arbitrary file types
      valueFiles: # would let you poke around git - yaml/json files
    directory:
      recurse: # would let you poke around git
      jsonnet: # idk anything about jsonnet, so assuming it's all unsafe
        extVars:
        tlas:
      exclude: # would let you poke around git - manifest files
      include: # would let you poke around git - manifest files
    plugin: # who knows what plugins do, block it all
      name:
      env:
      parameters:
  destination:
    server:
  # none of the rest is even relevant for manifest generation
  syncPolicy:
    automated:
      prune:
      selfHeal:
      allowEmpty:
    syncOptions:
    managedNamespaceMetadata:
      labels:
      annotations:
    retry:
      limit:
      backoff:
        duration:
        factor:
        maxDuration:
  ignoreDifferences:
  revisionHistoryLimit:
  info:

Do any of those seem particularly problematic to block?

[MeNsaaH]

I see alot of people using helm.valueFiles. I think that maybe a problem. And for plugins, it's a tough one. I personally use a plugin to pass helm manifests back to kustomize. Well, I think having the option to display all the unfiltered diffs as we've discussed could help out for such use cases

[crenshaw-dev]

How do you feel about this more permissive allow-list?

spec:
  source:
    targetRevision:
    path:
    helm:
      parameters:
      releaseName:
      ignoreMissingValueFiles:
      values:
      fileParameters:
      valueFiles:
      skipCrds:
      version:
    kustomize:
      version:
      namePrefix:
      nameSuffix:
      commonLabels:
      forceCommonLabels:
      commonAnnotations:
      commonAnnotationsEnvsubst:
      forceCommonAnnotations:
      images:
      namespace:
      replicas:
    directory:
      recurse:
      exclude:
      include:
    plugin: # who knows what plugins do, block it all
      name:
      env:
      parameters:
  destination:
    namespace:

Deny-list:

spec:
  project:
  source:
    repoURL:
    chart:
    helm:
      passCredentials:
    directory:
      jsonnet: # idk anything about jsonnet, so assuming it's all unsafe
        extVars:
        tlas:
  destination:
    server:
  # none of the rest is even relevant for manifest generation
  syncPolicy:
    automated:
      prune:
      selfHeal:
      allowEmpty:
    syncOptions:
    managedNamespaceMetadata:
      labels:
      annotations:
    retry:
      limit:
      backoff:
        duration:
        factor:
        maxDuration:
  ignoreDifferences:
  revisionHistoryLimit:
  info:

I think in this case we'd want to offer an off-by-default feature flag.

# By enabling this, you allow users with `applications, get` access to use the "Application diff API" (e.g. 
# `argocd app diff --app my-app.yaml`).
# IMPORTANT:
# This effectively gives those users full read access to the applications' source repositories, because they 
# can run manifest generation with modified source fields such as `path` and `helm.fileParameters`.
# If you use Config Management Plugins, this also gives users with `applications, get` access to run 
# manifest generation with modified plugin parameters. Make sure it is safe for these users to pass 
# arbitrary parameters to your plugins before enabling this feature.
server.enable.full.application.diff: "false"

[crenshaw-dev]

We could also do a pre-check for update access before doing sanitization. If someone has update access, we let them change whatever fields they'd be allowed to edit by running update (i.e. we'd need to check the namespace and project fields).

[MeNsaaH]

I'm in for this. Sounds concrete.

[mikebryant]

I'd love to see this feature. My feedback:
We don't use custom plugins, and our repositories are all visible to all engineers internally, which means I think we'd feel completely safe enabling the flag.
I would imagine lots of places to be in a similar position, so I think this set of limitations still gives it broad appeal. Having the flag be off by default makes sense.

This approach still means we need to find the updated apps manifests and run this diff command for each. It would be amazing if we could pass an option to the original app diff command and have it run through this recursively - future enhancement?

(e.g. if I have an Application object pointing at my repo, currently I do an app diff --revision blah to see the first level differences, which includes other Application objects. Having it automatically do this recursively for all of those App objects as well etc would be ace)

[crenshaw-dev]

We don't use custom plugins, and our repositories are all visible to all engineers internally, which means I think we'd feel completely safe enabling the flag.

Makes sense. And I agree, this will probably be the case for most users.

This approach still means we need to find the updated apps manifests and run this diff command for each.

I think for the first implementation, we'd leave that to the user to implement in BASH or similar. But does sound like a useful tool to add once the first iteration is done.

[crenshaw-dev]

Just planned out my 2.8 time, and I don't think I'll be able to squeeze this in. If someone wants to pick up the PR, I'd be happy to help guide the implementation and do a review!

[MeNsaaH]

@crenshaw-dev I'd try and see if I can pick out time this week to work on this.

[crenshaw-dev]

@MeNsaaH just lmk if you need any help!