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

Admin user (in Dex) is not admin on Grafana #2653

Closed
gdemonet opened this issue Jul 1, 2020 · 4 comments
Closed

Admin user (in Dex) is not admin on Grafana #2653

gdemonet opened this issue Jul 1, 2020 · 4 comments
Assignees
Labels
kind:bug Something isn't working topic:authentication Anything related to user authentication topic:monitoring Everything related to monitoring of services in a running cluster

Comments

@gdemonet
Copy link
Contributor

gdemonet commented Jul 1, 2020

Component: grafana

What happened:

When logging into Grafana, with the default [email protected] static user in Dex, we get read-only access.

What was expected:

This user should have super-admin access.

Steps to reproduce: (affects all 2.5+ versions)

Resolution proposal (optional):

Not clear: see https://grafana.com/docs/grafana/latest/auth/generic-oauth/#role-mapping

@gdemonet gdemonet added kind:bug Something isn't working topic:monitoring Everything related to monitoring of services in a running cluster topic:authentication Anything related to user authentication labels Jul 1, 2020
@Ebaneck
Copy link
Contributor

Ebaneck commented Jul 13, 2020

It seems like the Dex static user does not support groups and from upstream discussions, Dex will not support this feature for static users. Connector based settings like LDAP, SAML and others seem to support this.

2020-07-10T09:10:47.776792798Z stderr F time="2020-07-10T09:10:47Z" level=info msg="login successful: connector \"local\", username=\"admin\", preferred_username=\"\", email=\"[email protected]\", groups=[]"

This means that we cannot use the role-mapping feature from Grafana since the user info endpoint has no entries for groups.

Alternatively, we could fix this by using the below hack. We should, however, remove the below settings when we implement role-mapping with non-static users

diff --git a/charts/prometheus-operator.yaml b/charts/prometheus-operator.yaml
index e9072631..14c2d24d 100644
--- a/charts/prometheus-operator.yaml
+++ b/charts/prometheus-operator.yaml
@@ -164,6 +164,8 @@ grafana:
       auth_url: '__escape__(https://{{ grains.metalk8s.control_plane_ip }}:8443/oidc/auth)'
       token_url:  '__escape__(https://{{ grains.metalk8s.control_plane_ip }}:8443/oidc/token)'
       api_url: '__escape__(https://{{ grains.metalk8s.control_plane_ip }}:8443/oidc/userinfo)'
+    users:
+      auto_assign_org_role: "Admin"
 

@gdemonet
Copy link
Contributor Author

Oh no we can't deploy Grafana with this "hack". Instead, we could use role-mapping to map this special username (no need to rely on groups) to the Admin role.

@Ebaneck
Copy link
Contributor

Ebaneck commented Jul 31, 2020

Reading the Grafana role mapping docs, here is a fix proposal

role_attribute_path: contains(email, '[email protected]') && 'Admin'

@Ebaneck Ebaneck assigned Ebaneck and unassigned Ebaneck Jul 31, 2020
@Ebaneck
Copy link
Contributor

Ebaneck commented Jul 31, 2020

Instead of simply hard-coding this value like proposed above, we could indeed obtain the value from a read service configuration.

Why?
We expose the OIDC admin user via a metalk8s-dex-config Service Config(SC) which is editable by users. Meaning some users might edit the SC and the changes will not be evaluated on Grafana side since it will continue to use the hard-coded value.

Now if we read from an SC, then the above issue is solved is partly solved, since users will only need to restart the grafana deployments.

Given how we render charts per namespace, simply re-rendering the Prometheus-operator charts with --service-config dex metalk8s-dex-config will not work out great.

  • We need to add a new arg to the render script e.g (--external-service-config chart_name SC_name, namespace)
  • Then fix the render to include the values passed.

We ideally should end with something like this in the Prometheus-operator charts

{% import_yaml 'metalk8s/addons/dex/config/dex.yaml' as dex_defaults with context %}
{% import_yaml 'metalk8s/addons/prometheus-operator/config/grafana.yaml' as grafana_defaults with context %}
{% import_yaml 'metalk8s/addons/prometheus-operator/config/prometheus.yaml' as prometheus_defaults with context %}
{% import_yaml 'metalk8s/addons/prometheus-operator/config/alertmanager.yaml' as alertmanager_defaults with context %}

{%- set grafana = salt.metalk8s_service_configuration.get_service_conf('metalk8s-monitoring', 'metalk8s-grafana-config', grafana_defaults) %}
{%- set prometheus = salt.metalk8s_service_configuration.get_service_conf('metalk8s-monitoring', 'metalk8s-prometheus-config', prometheus_defaults) %}
{%- set alertmanager = salt.metalk8s_service_configuration.get_service_conf('metalk8s-monitoring', 'metalk8s-alertmanager-config', alertmanager_defaults) %}
{%- set dex = salt.metalk8s_service_configuration.get_service_conf('metalk8s-auth', 'metalk8s-dex-config', dex_defaults) %}

With value looking like:

role_attribute_path = contains(email, '{% endraw -%}{{ dex.spec.localuserstore.userlist[0]['email'] }}{%- raw %}') && 'Admin'

@Ebaneck Ebaneck self-assigned this Aug 17, 2020
Ebaneck added a commit that referenced this issue Aug 17, 2020
This chart is rendered using:
```
./charts/render.py prometheus-operator --namespace metalk8s-monitoring \
  charts/prometheus-operator.yaml charts/prometheus-operator/ \
  --service-config grafana metalk8s-grafana-config metalk8s/addons/prometheus-operator/config/grafana.yaml metalk8s-monitoring \
  --service-config prometheus metalk8s-prometheus-config metalk8s/addons/prometheus-operator/config/prometheus.yaml metalk8s-monitoring \
  --service-config alertmanager metalk8s-alertmanager-config metalk8s/addons/prometheus-operator/config/alertmanager.yaml metalk8s-monitoring \
  --service-config dex metalk8s-dex-config metalk8s/addons/dex/config/dex.yaml metalk8s-auth \
  > salt/metalk8s/addons/prometheus-operator/deployed/chart.sls
```

Closes: #2653
Ebaneck added a commit that referenced this issue Aug 17, 2020
This chart is rendered using:
```
./charts/render.py prometheus-operator --namespace metalk8s-monitoring \
  charts/prometheus-operator.yaml charts/prometheus-operator/ \
  --service-config grafana metalk8s-grafana-config metalk8s/addons/prometheus-operator/config/grafana.yaml metalk8s-monitoring \
  --service-config prometheus metalk8s-prometheus-config metalk8s/addons/prometheus-operator/config/prometheus.yaml metalk8s-monitoring \
  --service-config alertmanager metalk8s-alertmanager-config metalk8s/addons/prometheus-operator/config/alertmanager.yaml metalk8s-monitoring \
  --service-config dex metalk8s-dex-config metalk8s/addons/dex/config/dex.yaml metalk8s-auth \
  > salt/metalk8s/addons/prometheus-operator/deployed/chart.sls
```

Closes: #2653
@bert-e bert-e closed this as completed in 4b06ee5 Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug Something isn't working topic:authentication Anything related to user authentication topic:monitoring Everything related to monitoring of services in a running cluster
Projects
None yet
Development

No branches or pull requests

2 participants