search

vmware-tanzu / pinniped

Pinniped is the easy, secure way to log in to your Kubernetes clusters.
https://pinniped.dev
Apache License 2.0
566 stars 66 forks source link

Allow override of OIDC discovery information in Supervisor and Concierge #859

Open mayankbh opened 3 years ago

mayankbh commented 3 years ago

Is your feature request related to a problem? Please describe.

Certain identity providers may not fully adhere to the OIDC discovery specification. For instance, with VMware Cloud Services, there are issuer URL mismatches. KubeApps discusses this in a bit of detail (https://kubeapps.com/docs/using-an-OIDC-provider/)

Note: VMware Cloud Services has an issuer URL specific to organizations which is required for the Kubeapps auth proxy configuration above, but if you check the .well-known/openid-configuration you will see that it identifies a different (parent) issuer, https://gaz-preview.csp-vidm-prod.com. It is for this reason that the --insecure-oidc-skip-issuer-verification option is required above. For the same reason, the OIDC id_tokens that are minted specify the parent issuer as well, which is why the Kubernetes API server config above uses that.

This is confirmed from visiting https://console.cloud.vmware.com/csp/gateway/am/api/.well-known/openid-configuration

...
"issuer":"https://gaz.csp-vidm-prod.com"
...

This seems to affect, if I'm reading the code correctly-

hence breaking OIDC discovery and preventing the use of such identity providers.

go-oidc, which Pinniped seems to use directly/indirectly, recently added support for overriding the issuer URL (https://github.com/coreos/go-oidc/commit/916b64feddcecf5c6821f0d207868b03e4fc5b95) but Pinniped does not expose any knobs to plumb that information down into its OIDC related configurations.

Describe the solution you'd like

It would be nice to be able to override the information from the OIDC discovery endpoint (or skip OIDC discovery entirely, as kubeapps seems to do) in cases where we expect such mismatches to occur. I don't think it makes sense to make it a default, of course, purely opt-in.

A strawdog solution (I'm not entirely sure if one would want to override all of the fields in the OIDC discovery endpoint or only a strict subset) -

kind: OIDCIdentityProvider
...
spec:
  client:                                                                                                                                                                                                         
    secretName: my-oidc-identity-provider-client                                                                                                                                                                  
  issuer: https://foo-bar.com/idp # used for discovery
  issuerOverrides: # defaults to empty dict.
    issuerURL: https://actual-issuer.com/ ​
apiVersion: authentication.concierge.pinniped.dev/v1alpha1
kind: JWTAuthenticator
metadata:
  name: csp-jwt-authenticator
spec:
    issuer: https://foo-bar.com/idp
    issuerOverrides:
      issuerURL: https://actual-issuer.com/login

(I'm not sure how painful this would be to implement, since some of this issuer URL verification seems to happen in the APIserver libraries imported by pinniped)

Describe alternatives you've considered

I think it's possible to have Concierge use a WebhookAuthenticator to ignore such issues within a single cluster, but that would lose the benefits of using the Pinniped Supervisor for federation (for instance, when I have several clusters).

Are you considering submitting a PR for this feature?

Not yet - I wanted to file the initial issue to see if this made sense to track as a feature request.

Additional context

Not strictly related, but I found related issues in Azure linked to the go-oidc change above, which I understand to be related to multi-tenant apps in Azure (I could be misunderstanding though, I am not all that familiar with Azure's tenancy model):

enj commented 3 years ago

I am strongly opposed to adding any such insecure flags to Pinniped, just in the same way we do not expose insecure skipping verification of TLS hostnames. It does not matter to me if it is opt-in, I have no plans to give such a choice at all. Providing first class config options for violating the OIDC spec is a non-starter for me.

Anyone wanting this functionality today can simply fork pinniped and carry the minor patch required to make it work.

Long term, I would like to add a VMware Cloud Services specific IDP integration. It would of course be aware of this limitation of the IDP and would provide the user with the specific knobs necessary to make the configuration safe to use.

enj commented 3 years ago

Note also that the comments in the go-oidc code are simply wrong - Azure AD's OIDC integration works perfectly with Pinniped today because they are fully spec compliant. They even provide a nice UI to allow for configuration of their ID tokens.

We should also request that VMware Cloud Services issue at upgrade that fixes this OIDC spec violation.

enj commented 3 years ago

@mayankbh have you tried using https://gaz.csp-vidm-prod.com as the issuer? https://gaz.csp-vidm-prod.com/.well-known/openid-configuration seems to have the correct information (though it may be a different IDP all together).