defenseunicorns / uds-core

A FOSS secure runtime platform for mission-critical capabilities
https://uds.defenseunicorns.com
GNU Affero General Public License v3.0
48 stars 20 forks source link

Keycloak client mappers configuration #305

Closed ericwyles closed 1 month ago

ericwyles commented 7 months ago

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

When using SSO Secret Templating (#263) to make a new client in keycloak, we need a way to be able to configure mappers in the new client. Mattermost requires the following mappers to be present in order to work with keycloak masquerading as gitlab:

image

Describe the solution you'd like

Given I have an application configured to integrate with a UDSPackage CRD
When I add a new secret template field to the sso section with mappers specified in the template
And I apply that CRD to my cluster
The mappers are created in the client in keycloak in my cluster.

Additional context

I ran onto this integrating mattermost with keycloak and solved it by creating the mappers manually

image

ericwyles commented 7 months ago

@mjnagel -- There is an error in the screenshot for the mapper configuration. In the first mapper the user attribute needs to be 'mattermostid' (all lower case). Without mixed case, mattermost fails to create the user after initial authentication.

mjnagel commented 7 months ago

@rjferguson21 @UnicornChance I think we might be able to bake these mappers into the realm somehow so that we don't have to expose full mapper configuration in the Package spec? That would be my preference to start with, if possible - adding full mapper config sounds like a lot of extra complexity to expose. I'm not sure how often mappers might be a need though - if its a common requirement then maybe we should expose this. From Big Bang history in the past...Mattermost is the only place I recall mappers being used?

ericwyles commented 7 months ago

Further data point on this, I'm just getting started integrating SonarQube and looking at these instructions and it looks like it will require some mappers as well.

EDIT: There are more options on the sonarqube side, I'm going to sync with the team and see if it's already determined which way to go. If we go with SAML it requires the mappers.

ericwyles commented 2 months ago

@rjferguson21 Following up here based on our discussion in slack. I created this draft PR just to show an example and kick off discussion.

https://github.com/defenseunicorns/uds-core/pull/621

This just adds the ability to specify keycloak protocolMappers in the sso object.

I tested that with mattermost. Before that change mattermost uds-package looks like this:

apiVersion: uds.dev/v1alpha1
kind: Package
metadata:
  name: mattermost
  namespace: {{ .Release.Namespace }}
spec:
  {{- if .Values.sso.enabled }}
  sso:
    - name: Mattermost Login
      clientId: uds-swf-mattermost
      redirectUris:
        - "https://{{ .Values.subdomain }}.{{ .Values.domain }}/*"
      defaultClientScopes:
        - "openid"
        - "mapper-oidc-username-username"
        - "mapper-oidc-mattermostid-id"
        - "mapper-oidc-email-email"

      secretName: {{ .Values.sso.secretName }}
      secretTemplate:
            ....

And with the change it can be like this (and no mappers required in the realm):

apiVersion: uds.dev/v1alpha1
kind: Package
metadata:
  name: mattermost
  namespace: {{ .Release.Namespace }}
spec:
  {{- if .Values.sso.enabled }}
  sso:
    - name: Mattermost Login
      clientId: uds-swf-mattermost
      redirectUris:
        - "https://{{ .Values.subdomain }}.{{ .Values.domain }}/*"
      defaultClientScopes:
        - "openid"

      protocolMappers:
        - name: username
          protocol: "openid-connect"
          protocolMapper: "oidc-usermodel-property-mapper"
          config:
            user.attribute: "username"
            claim.name: "username"
            userinfo.token.claim: "true"

        - name: email
          protocol: "openid-connect"
          protocolMapper: "oidc-usermodel-property-mapper"
          config:
            user.attribute: "email"
            claim.name: "email"
            userinfo.token.claim: "true"

        - name: mattermostid
          protocol: "openid-connect"
          protocolMapper: "oidc-usermodel-attribute-mapper"
          config:
            user.attribute: "mattermostid"
            claim.name: "id"
            jsonType.label: "long"
            userinfo.token.claim: "true"

      secretName: {{ .Values.sso.secretName }}
      secretTemplate:
                    ...

In my opinion it's also easier to understand if you are a keycloak admin looking in the console, because it groups everything under the client and doesn't introduce the extra indirection through the client scopes

image

An obvious downside is the configuration section of the protocol mapper is pretty loose (key/value pairs) with no documentation, so you have to know what you are doing to configure one properly. In the example above I configured multiple different ones to show cases where different values have to be specified where we don't want defaults, but the set of possible configuration key/value pairs really depends on the specifics and type of each mapper. It's the same stuff that ends up in the realm.json just that anything where you can use defaults isn't necessary here in the yaml.

ericwyles commented 2 months ago

I also did test that this updates mappers if you redeploy the package which is nice. It puts the mapper updates on the same lifecycle as the rest of the package which is really nice for package creators.

mjnagel commented 1 month ago

@rjferguson21 I think this can be closed out now with the merge of the protocol mappers PR?