[grafana] feat: fail when users store secrets in plaintext

[DerekTBrown]

What does this PR do?

• There are numerous properties within grafana.ini that should be secret.
• Users might try to provide these properties by setting them in helm values. This would result in secrets being leaked, since the contents of grafana.ini is stored as a configmap.
• This PR adds assertions that users do not provide explicit secrets into the grafana.ini configmap.

Alternates considered:

• We could "pluck" these values out of the configmap and store them in a secret. We could then use envFrom or some other mechanism to inject them into grafana.ini. However, this might lead to confusion on the part of users, who may expect all the values to be put into the grafana.ini verbatim.

[CLAassistant]

All committers have signed the CLA.

[zanhsieh]

@DerekTBrown
Thanks for the contribution. Can you bump the version in Chart.yaml and sign DCO too?

[Xtigyro]

@DerekTBrown Please resolve the conflicts.

[z0rc]

You probably wanted to link https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#override-configuration-with-environment-variables instead of https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#variable-expansion

[joeblubaugh]

Isn't this likely to break a number of grafana deployments in the wild? Is there an ability to disable this check?

[goraxe]

Just a point of etiquette for merging changes that change default behavior please bump the major version of the chart (ie indicate a breaking change). I have renovate running here which automerges minor and patch level. Major get manually approved after reading the changelog.

There is an opt out switch introduced by the change but it defaults to on

Some docs might also help as it looks like the current README was not updated and no test cases were included. My initial attempt was to switch the keys in the generic oauth section to the file params however this failed as the path was still the same so this leaves the awkward question of what is the 'correct' way to pass in the variable expansion through the grafana.ini values map.

I had to flip the switch to off to have the chart render in argocd with supplying the values through a secret and parameter expansion.

[afreyermuth98]

To agree with @goraxe , we for example need the env variables names to do so.
For example I achieved to migrate my database.password to the env variable named GF_DATABASE_PASSWORD but I don't have any idea / documentation on the auth.google.client_id|client_secret for example
And yeah i'm putting them as file secret ref as it's recommended on the documentation and it's "recognized" as sensitive even if it's not :

grafana.ini
  auth.google:
    client_id: $__file{/etc/secrets/auth_google/client_id}
    client_secret: $__file{/etc/secrets/auth_google/client_secret}

[Moglum]

The correct syntax of the env variables is described here: https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#override-configuration-with-environment-variables

so in your case

  auth.google:
    client_id: $__file{/etc/secrets/auth_google/client_id}
    client_secret: $__file{/etc/secrets/auth_google/client_secret}

can be provided as the following env variables

GF_AUTH_GOOGLE_CLIENT_ID: foo
GF_AUTH_GOOGLE_CLIENT_SECRET: bar

for example via the envRenderSecret key in Grafana Helm chart values -

helm-charts/charts/grafana/values.yaml

Line 486 in ec449bc

envRenderSecret: {}

[afreyermuth98]

Thank you !
Just a question about that, what is the point to make the data sensitive inside grafana.ini and not in env, it's just shifting the problem no ?

[Moglum]

I have no idea. I'm only here because our gitops pipelines started to fail for no reason. We had a static non-sensitive string in there, similar to what you shared client_secret: $__file{/etc/secrets/auth_generic_oauth_secret/client_secret} and the mounted/referenced Secret was already handled with Sealed Secrets.

So, I've yet to the see point behind this breaking change.

[YvesZelros]

This PR will allow secret reference.

[cwrau]

This was a breaking change and therefor should've been a major release, as this breaks existing configuration.