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

Keycloak client mappers configuration #305

Closed
ericwyles opened this issue Mar 27, 2024 · 6 comments · Fixed by defenseunicorns/uds-identity-config#57
Closed

Keycloak client mappers configuration #305

ericwyles opened this issue Mar 27, 2024 · 6 comments · Fixed by defenseunicorns/uds-identity-config#57
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@ericwyles
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

mjnagel commented Mar 28, 2024

@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
Copy link
Contributor Author

ericwyles commented Apr 1, 2024

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 ericwyles self-assigned this Apr 17, 2024
rjferguson21 added a commit that referenced this issue Apr 19, 2024
…operator (#328)

## Description

Add support for saml protocol and attributes and protocolMappers support
for keycloak clients.

## Related Issue
Relates to #326 
Relates to #305

## Type of change

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Other (security config, docs update, etc)

## Checklist before merging

- [x] Test, docs, adr added or updated as needed
- [x] [Contributor Guide
Steps](https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md)(https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md#submitting-a-pull-request)
followed

---------

Co-authored-by: Rob Ferguson <[email protected]>
Co-authored-by: Chance <[email protected]>
Co-authored-by: Micah Nagel <[email protected]>
rjferguson21 added a commit that referenced this issue Jul 11, 2024
…operator (#328)

## Description

Add support for saml protocol and attributes and protocolMappers support
for keycloak clients.

## Related Issue
Relates to #326 
Relates to #305

## Type of change

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Other (security config, docs update, etc)

## Checklist before merging

- [x] Test, docs, adr added or updated as needed
- [x] [Contributor Guide
Steps](https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md)(https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md#submitting-a-pull-request)
followed

---------

Co-authored-by: Rob Ferguson <[email protected]>
Co-authored-by: Chance <[email protected]>
Co-authored-by: Micah Nagel <[email protected]>
@ericwyles
Copy link
Contributor Author

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

#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 ericwyles reopened this Jul 31, 2024
@ericwyles
Copy link
Contributor Author

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 mjnagel added this to the 0.27.0 milestone Aug 21, 2024
@UnicornChance UnicornChance self-assigned this Aug 26, 2024
rjferguson21 added a commit that referenced this issue Sep 3, 2024
## Description
This is just here to demonstrate POC of protocol mappers to kick off
discussion. At this point no intention to merge.

## Related Issue

Relates to #305

---------

Co-authored-by: Chance <[email protected]>
Co-authored-by: UnicornChance <[email protected]>
Co-authored-by: Rob Ferguson <[email protected]>
@mjnagel
Copy link
Contributor

mjnagel commented Sep 3, 2024

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

@mjnagel mjnagel closed this as completed Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants