mintel / dex-k8s-authenticator

A Kubernetes Dex Client Authenticator
MIT License
371 stars 146 forks source link

ClientID is used as cluster name value in templates #63

Closed legal90 closed 6 years ago

legal90 commented 6 years ago

Hi!

First - thank yo so much for this project and especially for helm charts which you provided for both of dex and dex-k8s-authenticator ! 👍

But when I looked on the page with kubectl commands rendered after the successful authentication, I've noticed that in some places it uses .ClientID instead of .ClusterName (when the latter looks more reasonable). For example:

https://github.com/mintel/dex-k8s-authenticator/blob/ab246db44aa89df1d9c8f98dc2e2fdb6b4c797e3/templates/linux-mac-common.html#L80-L82 and https://github.com/mintel/dex-k8s-authenticator/blob/ab246db44aa89df1d9c8f98dc2e2fdb6b4c797e3/templates/linux-mac-common.html#L90

Actual Behavior

That is how it looks in my case:

# dex.yaml  - values file for the dex chart
# <...>

config: |-
  staticClients:
  - id: dex-k8s-authenticator
    name: my-k8s-cluster.com
    secret: "my-secret"
    redirectURIs:
    - https://login.my-k8s-cluster.com/callback/my-k8s-cluster.com

# <...>
# dex-k8s-authenticator.yaml  - values file for the dex-k8s-authenticator chart
# <...>

dexK8sAuthenticator:
  clusters:
  - name: my-k8s-cluster.com
    issuer: https://dex.my-k8s-cluster.com
    k8s_master_uri: https://my-k8s-cluster.com
    client_id: dex-k8s-authenticator
    client_secret: my-secret
    redirect_uri: https://login.my-k8s-cluster.com/callback/my-k8s-cluster.com

# <...>

And that's how commands are rendered on the page:

kubectl config set-cluster dex-k8s-authenticator \
    --certificate-authority=${HOME}/.kube/certs/my-k8s-cluster.com/k8s-ca.crt \
    --server=https://my-k8s-cluster.com

kubectl config set-credentials legal90-dex-k8s-authenticator \
    --auth-provider=oidc \
    --auth-provider-arg=idp-issuer-url=https://dex.my-k8s-cluster.com \
    --auth-provider-arg=client-id=dex-k8s-authenticator \
    --auth-provider-arg=client-secret=my-secret \
<...>

kubectl config set-context legal90-dex-k8s-authenticator \
    --cluster=dex-k8s-authenticator \
    --user=legal90-dex-k8s-authenticator

kubectl config use-context legal90-dex-k8s-authenticator

As we can see, there is dex-k8s-authenticator instead of my-k8s-cluster.com, the cluster name set in the config. I guess that will cause actual problems on the multi-cluster installations, when a single dex-k8s-authenticator instance is used to talk with multiple dex installations - each per cluster. In that case the client_id could be the same (theoretically).

Expected Behavior

I would expect the cluster name being an actual cluster identifier in my kubeconfig:

kubectl config set-cluster my-k8s-cluster.com \
    --certificate-authority=${HOME}/.kube/certs/my-k8s-cluster.com/k8s-ca.crt \
    --server=https://my-k8s-cluster.com

kubectl config set-credentials legal90-my-k8s-cluster.com \
    --auth-provider=oidc \
    --auth-provider-arg=idp-issuer-url=https://dex.my-k8s-cluster.com \
    --auth-provider-arg=client-id=dex-k8s-authenticator \
    --auth-provider-arg=client-secret=my-secret \
<...>

kubectl config set-context legal90-my-k8s-cluster.com \
    --cluster=my-k8s-cluster.com \
    --user=legal90-my-k8s-cluster.com

kubectl config use-context legal90-my-k8s-cluster.com

I can send a PR without any problem, but first I just want to make sure - is that a valid point, or I missed something?

Thank you!

nabadger commented 6 years ago

@legal90 This is an oversight on my part, as we've always used different client_id values (which are essentially the cluster-name for us, so we've not hit this issue).

As you say, using .ClusterName would avoid any possible conflicts.

If you could submit a PR that would be great 👍

nabadger commented 6 years ago

Thanks again 👍