Introduce RBAC based approach to pod logs

[reggie-k]

Summary

Need to disable the access to pod logs in the UI / cli for some users

Motivation

Security considerations. Operators of the deployments are sometimes not allowed to see the pod logs, which possibly contain sensitive business data.

Proposal

RBAC based approach to pod logs, API enforced.
Something like the following project policy may be appropriate:
"- p, proj:my-project:log-viewer, log, get, my-project/*, allow"

From 18.11 Meeting notes:

• All are in favor of adding new RBAC (sub)resource for logs
• This would be a breaking change; we can mitigate with flag that allows log reads by default
  Migration requires work for those who use project RBAC extensively
  Transitionary release that doesn’t break logs by default?

Also discussed here:
#7686

[89leon]

a must !

[Hillelmed]

ASAP ! :)

[matankila]

i am struggling to see logs, and my clients are too..

[igalg]

ASAP!!!!!!!!!

[reggie-k]

So we are going for the RBAC approach, treating the "logs" as a sub-resource.
I thought of a default behavior of showing logs as "allowed", and only those customers, who would want to hide the logs, will set it as "deny".
Does this help mitigating the breaking change issue?
@jannfis

[jannfis]

By default, we deny all (sub-) resources and they would have to be explicitly allowed. When we change the behavior to default allow only to this particular resource, imho this would be utterly confusing.

In yesterday's community meeting, we discussed having a global flag to disable the RBAC enforcement on logs resource, so that people can set it when upgrading and don't have to care about RBAC rules yet. If they have put the appropriate RBAC rules in place, they can remove the global flag again and RBAC would be enforced on the logs resource. This flag would probably only available for one intermediate release, before making logs RBAC mandatory.

The problem that we had identified is, that you can't come up with the RBAC rules for a new (sub-) resource before the upgrade, because there is some validation of the rules. So when this feature gets released in version N, you can't put the appropriate rules in place in release N-1. And since people possibly have a lot of RBAC rules especially attached to AppProjects, this may have severe consequences on some orgs.

I guess we didn't go into details how this flag would look like (e.g. an environment variable, a command line switch, or similar), so this is something we'd still have to figure out.

/cc @alexmt @jessesuen

[jannfis]

I also think that the title of this issue and the details should be adapted to reflect that this is not a UI-only change and that we're gonna introduce a new RBAC resource and enforcement in the API.

[reggie-k]

For the intermediate release then, shall I set a param named server.rbac.project.log.enforce with a "false" value in argocd-cmd-params-cm.yaml?

The motivation for such naming convention is to clearly denote resource nesting, in favor of possible new future (sub)-resources.

Should the param be also supported as an env on the server deployment for the sake of those who use the old configuration method?

Another thought, most of the existing resources support CRUD actions, while only "get" action is relevant for log resource. Shall I treat that in some dedicated way?

[jannfis]

For the intermediate release then, shall I set a param named server.rbac.project.log.enforce with a "false" value in argocd-cmd-params-cm.yaml?

I think server.rbac.log.enforce would be more appropriate, since it should apply to non-project RBAC as well (at enforcement time, we don't differentiate between a project scoped RBAC or a global one).

Should the param be also supported as an env on the server deployment for the sake of those who use the old configuration method?

The way parameters in argocd-cmd-params-cm.yaml work is, that they are actually set as environment variables for the respective workloads. So, introducing a new parameter to the ConfigMap makes the env var implicit :) Here is an example:

argo-cd/manifests/base/server/argocd-server-deployment.yaml

Line 25 in 475bea5

- name: ARGOCD_SERVER_INSECURE

Another thought, most of the existing resources support CRUD actions, while only "get" action is relevant for log resource. Shall I treat that in some dedicated way?

I think we don't need to handle that specially. The enforcement methods in the API will query whether a given action (i.e. get) is allowed in the RBAC policy, and allow or deny access according to the result. I think we may want to document that only get is actually used for log (sub-) resources and it should be fine.

[reggie-k]

As for the intermediate release - what would be the path we follow?

A:

• A default "false" hard coded value for the log rbac enf
• Release doc stating that in order to enable the rbac enforcement the param as "true" in the cm + relevant policies must be defined
• Full transparency for those who do not require this

B:

• No hardcoded default value for the log rbac enf
• Release doc stating that in order to enable the rbac enforcement the param as "true" in the cm + relevant policies must be defined
• A change in the argocd-cmd-params-cm.yaml , that is part of the installation manifests , to contain the param with a "false" value / Release dos stating that the param must be defined in the cm otherwise logs will not be visible
• Not transparent and requires a change in the installation yamls during upgrade for everyone

Any other alternatives?

[reggie-k]

Regarding the parameter for the intermediate release:

• As agreed, It will be "false" by default

• If the parameter for some customer stays with the default value of "false" ,and he writes a policy allowing/denying getting logs, the behavior might not be consistent (for example, for a deny policy, "can-i" might return "no" but an attempt to get the logs might succeed if no special handling is in place)

• For a "false" value, what should be the expected behavior if an RBAC policy allowing/denying getting logs is defined by user (either globally or on a project)? (An edge case of mis-usage, but might still need to be taken care of)

1. Policy considered invalid (as if the "logs" RBAC subresource didn't exist at all)
2. Policy considered valid - if the effect is "allow" and invalid only if the effect is "deny"

Or this is not something to worry about and covering it in the docs should be enough?

/cc @alexmt @jessesuen @jannfis

[todaywasawesome]

Discussed in contributor experience meeting, the consensus was to just add a notice in the docs and release notes that the UI will throw this error when enabled. The feature is essentially complete from the backend standpoint so we feel it's worth moving forward.

@reggie-k Can you add a note to docs and release notes?

[crenshaw-dev]

https://github.com/argoproj/argo-cd/blob/master/docs/operator-manual/upgrading/2.3-2.4.md is the specific file I was thinking of probably needing a note. :-)

[reggie-k]

https://github.com/argoproj/argo-cd/blob/master/docs/operator-manual/upgrading/2.3-2.4.md is the specific file I was thinking of probably needing a note. :-)

Thanks, in order to change the release notes to also contain that note, do I edit the CHANGELOG.md directly?

[crenshaw-dev]

I've wondered the same. @alexmt how can we make sure something makes it into the release notes?

[reggie-k]

Here is the relevant PR:

docs: logs RBAC known UI issue section #9479

[alexmt]

I believe this was implemented