Page MenuHomeSoftware Heritage

Keycloak: add realm and client definition in puppet manifest
ClosedPublic

Authored by olasd on Mar 26 2020, 9:28 PM.

Details

Summary

This diff adds in puppet manifest for Keycloak:

  • the definition of the SoftwareHeritage realm
  • the definition of the swh-web client in that realm

I managed to test locally with docker by hacking on pupperware and using a docker image with
systemd support (inspired by docker-systemd) to run the puppet agent.
Everything works as expected regarding authentication and authorization in swh-web.

Nevertheless, puppet module for Keycloak needs to upgraded to its latest version (6.10).

Related to T1927

Diff Detail

Repository
rSPSITE puppet-swh-site
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

anlambert created this revision.Mar 26 2020, 9:28 PM
anlambert edited the summary of this revision. (Show Details)Mar 26 2020, 9:28 PM
anlambert edited the summary of this revision. (Show Details)
anlambert edited the summary of this revision. (Show Details)
anlambert added a reviewer: olasd.
ardumont added inline comments.
site-modules/profile/manifests/keycloak/primary.pp
48 ↗(On Diff #10303)

Make that configurable so this can also be testable on other "archive" (webapp.internal.staging.swh.network, webapp0.softwareheritage.org)...

anlambert added inline comments.Mar 27 2020, 9:55 AM
site-modules/profile/manifests/keycloak/primary.pp
48 ↗(On Diff #10303)

Oh right, I forgot those.

anlambert updated this revision to Diff 10318.Mar 27 2020, 10:00 AM

Update: Add more redirect uris and add missing calls to lookup

anlambert updated this revision to Diff 10321.Mar 27 2020, 10:20 AM

Update: Fix missing protocol in redirect uri

ardumont added inline comments.Mar 27 2020, 11:24 AM
site-modules/profile/manifests/keycloak/primary.pp
56 ↗(On Diff #10321)

I meant to make that as a list value in a defaults.yaml entry key.
So we can configure it per location.

defaulting to the production one as we do other services deployment.

keycloak::client::swh_web::redirect_uris:
  - http://localhost:5004/*                               # no idea if it's relevant here
  - https://archive.softwareheritage.org/*

as default for the production

and on other file (sesi_rocquencourt_staging.yml, staging for example):

keycloak::client::swh_web::redirect_uris:
  - ...
  - https://webapp.internal.staging.swh.network/*

for staging

etc...

And then in that file do a lookup of the variable and you are golden ;)

Thanks for this config canvas !

I think we might want separate realms for production, staging and "localhost testing". At the very least, we will want separate clients, because I don't think application credentials and user sessions should be shared across the different usecases.

If you agree with this I'm happy to pick this diff up to implement the idea.

site-modules/profile/manifests/keycloak/primary.pp
56 ↗(On Diff #10321)

That can't work. There's a single keycloak server shared across the different environments.

If you agree with this I'm happy to pick this diff up to implement the idea.

Sure go ahead !

I think we might want separate realms for production, staging and "localhost testing". At the very least, we will want separate clients, because I don't think application credentials and user sessions should be shared across the different usecases.

One realm per environment seems the right way to go I think. This will enable to have a different set of users for each of them and test new clients safely.

olasd commandeered this revision.Apr 1 2020, 2:41 PM
olasd edited reviewers, added: anlambert; removed: olasd.
olasd updated this revision to Diff 10453.Apr 1 2020, 2:42 PM

Apply my own comments: refactor config to use a hiera dict

olasd added a comment.Apr 1 2020, 2:44 PM

octocatalog-diff output:

diff origin/production/kelvingrove.internal.softwareheritage.org current/kelvingrove.internal.softwareheritage.org
*******************************************
+ Keycloak_client[swh-web on SoftwareHeritageStaging] =>
   parameters =>
      "client_id": "swh-web"
      "default_client_scopes": ["profile", "email", "roles", "web-origins"]
      "ensure": "present"
      "name": "swh-web"
      "optional_client_scopes": ["microprofile-jwt", "offline_access"]
      "public_client": true
      "realm": "SoftwareHeritageStaging"
      "redirect_uris": ["https://webapp.staging.swh.network/*", "https://webapp.internal.staging.swh.network/*"]
*******************************************
+ Keycloak_client[swh-web on SoftwareHeritage] =>
   parameters =>
      "client_id": "swh-web"
      "default_client_scopes": ["profile", "email", "roles", "web-origins"]
      "ensure": "present"
      "name": "swh-web"
      "optional_client_scopes": ["microprofile-jwt", "offline_access"]
      "public_client": true
      "realm": "SoftwareHeritage"
      "redirect_uris": ["https://archive.softwareheritage.org/*", "https://base.softwareheritage.org/*", "https://archive.internal.softwareheritage.org/*", "https://webapp0.softwareheritage.org/*"]
*******************************************
+ Keycloak_protocol_mapper[audience for swh-web on SoftwareHeritageStaging] =>
   parameters =>
      "client_scope": "swh-web"
      "ensure": "present"
      "included_client_audience": "swh-web"
      "realm": "SoftwareHeritageStaging"
      "resource_name": "audience"
      "type": "oidc-audience-mapper"
*******************************************
+ Keycloak_protocol_mapper[audience for swh-web on SoftwareHeritage] =>
   parameters =>
      "client_scope": "swh-web"
      "ensure": "present"
      "included_client_audience": "swh-web"
      "realm": "SoftwareHeritage"
      "resource_name": "audience"
      "type": "oidc-audience-mapper"
*******************************************
+ Keycloak_protocol_mapper[user groups for swh-web on SoftwareHeritageStaging] =>
   parameters =>
      "claim_name": "groups"
      "client_scope": "swh-web"
      "ensure": "present"
      "full_path": true
      "realm": "SoftwareHeritageStaging"
      "resource_name": "user groups"
      "type": "oidc-group-membership-mapper"
*******************************************
+ Keycloak_protocol_mapper[user groups for swh-web on SoftwareHeritage] =>
   parameters =>
      "claim_name": "groups"
      "client_scope": "swh-web"
      "ensure": "present"
      "full_path": true
      "realm": "SoftwareHeritage"
      "resource_name": "user groups"
      "type": "oidc-group-membership-mapper"
*******************************************
+ Keycloak_realm[SoftwareHeritageStaging] =>
   parameters =>
      "display_name": "Software Heritage (Staging)"
      "ensure": "present"
      "internationalization_enabled": true
      "login_with_email_allowed": true
      "remember_me": true
*******************************************
+ Keycloak_realm[SoftwareHeritage] =>
   parameters =>
      "display_name": "Software Heritage"
      "ensure": "present"
      "internationalization_enabled": true
      "login_with_email_allowed": true
      "remember_me": true
*******************************************

I'm not sure if the resource ordering will work but we can adapt that later.

anlambert requested changes to this revision.Apr 1 2020, 6:09 PM

Thanks for the improvements !

I could not resist to test locally. This is the puppet output of my first run once I plugged the code in my local puppet testing env : P633
We can see that realms are correctly created but swh-web client creation for SoftwareHeritageStaging realm failed and the
protocol mappers creation too.

After a (painful) debugging session, I noticed that an explicit id must be set for each client with the same name in order for Keycloak to
successfully create them using its admin API.
In a same manner, protocol mappers must be created using the client id and not its name.

I added inline comments fixing the observed issues. Resulting puppet run after those changes can be found here: P634

site-modules/profile/manifests/keycloak/resources.pp
38 ↗(On Diff #10453)

I did not know that present was also a keyword in puppet, we should use it instead of a string for consistency in that file.

49 ↗(On Diff #10453)

Let's compute a client id value here using fqdn_uuid from puppet standard library:

$client_id = fqdn_uuid("${realm_name}.${client_name}")

Keycloak also uses UUIDs for object ids, so let's do the same.

51 ↗(On Diff #10453)

Let's explicitly set client id here:

id => $client_id,
52–53 ↗(On Diff #10453)

These lines are redundant and can be removed.

64 ↗(On Diff #10453)

This line should be turned into:

keycloak_client_protocol_mapper {"${protocol_mapper_name} for ${$client_id} on ${realm_name}":

Indeed, Keycloak use the client id as argument of the /{realm}/clients/{id}/protocol-mappers/models endpoint of its admin API (see documentation).

This revision now requires changes to proceed.Apr 1 2020, 6:09 PM
olasd added inline comments.Apr 1 2020, 6:19 PM
site-modules/profile/manifests/keycloak/resources.pp
64 ↗(On Diff #10453)

So I guess the __client_id__ magic stuff isn't really useful anymore either?

anlambert added inline comments.Apr 1 2020, 6:29 PM
site-modules/profile/manifests/keycloak/resources.pp
64 ↗(On Diff #10453)

Yes it is still needed as decoding of JWT tokens will fail otherwise.

The client id in Keycloak is quite confusing as if you look at a client representation, there is :

  • an id field which is the internal id of the client object used by keycloak to manipulate it in its admin API
  • a clientId field, which basically is the client name, used to identify the client for users that want to access it

Maybe client_id variable could be renamed to client_internal_id or client_kc_id to disambiguate ?

olasd marked an inline comment as not done.Apr 1 2020, 6:33 PM
olasd added inline comments.
site-modules/profile/manifests/keycloak/resources.pp
64 ↗(On Diff #10453)

Ah! I understand. So in the end it doesn't need to match the name of the client as set in keycloak.

So I guess I can remove the magic name matching and just set it to swh-web.

(the client_id field is a parameter of keycloak_client_protocol_mapper so we can't really change its name)

anlambert added inline comments.Apr 1 2020, 6:39 PM
site-modules/profile/manifests/keycloak/resources.pp
64 ↗(On Diff #10453)

I think you can keep the magic name matching and rename keycloak::resources::protocol_mappers::webapp to keycloak::resources::protocol_mappers::audience.

This way you get a generic audience mapper that can be used for other clients than swh-web.

anlambert added inline comments.Apr 1 2020, 6:42 PM
site-modules/profile/manifests/keycloak/resources.pp
64 ↗(On Diff #10453)

Even better, split keycloak::resources::protocol_mappers::webapp into keycloak::resources::protocol_mappers::audience and keycloak::resources::protocol_mappers::groups

I paste below the current state of the puppet configuration and profile for Keycloak that I got locally working:

configuration
keycloak::resources::realms::common_settings:
  remember_me: true
  login_with_email_allowed: true
  internationalization_enabled: true

keycloak::resources::clients::common_settings:
  public_client: true
  default_client_scopes:
    - profile
    - email
    - roles
    - web-origins
  optional_client_scopes:
    - microprofile-jwt
    - offline_access

keycloak::resources::protocol_mappers::audience:
    name: audience
    type: oidc-audience-mapper
    included_client_audience: __client_id__

keycloak::resources::protocol_mappers::groups:
    name: groups
    type: oidc-group-membership-mapper
    claim_name: groups
    full_path: true

keycloak::resources::realms:
  SoftwareHeritage:
    settings:
      display_name: Software Heritage
    clients:
      swh-web:
        settings:
          redirect_uris:
            # Should match letsencrypt::certificates.archive_production.domains
            - https://archive.softwareheritage.org/*
            - https://base.softwareheritage.org/*
            - https://archive.internal.softwareheritage.org/*
            - https://webapp0.softwareheritage.org/*
        protocol_mappers:
          - "%{alias('keycloak::resources::protocol_mappers::audience')}"
          - "%{alias('keycloak::resources::protocol_mappers::groups')}"
  SoftwareHeritageStaging:
    settings:
      display_name: Software Heritage (Staging)
    clients:
      swh-web:
        settings:
          redirect_uris:
            # Should match letsencrypt::certificates.archive_staging.domains
            - https://webapp.staging.swh.network/*
            - https://webapp.internal.staging.swh.network/*
        protocol_mappers:
          - "%{alias('keycloak::resources::protocol_mappers::audience')}"
          - "%{alias('keycloak::resources::protocol_mappers::groups')}"
profile
class profile::keycloak::resources {
  $realms = lookup({
    name          => 'keycloak::resources::realms',
    value_type    => Hash,
    merge         => {
      strategy        => 'deep',
      knockout_prefix => '--',
    },
    default_value => {},
  })

  $realm_common_settings = lookup({
    name          => 'keycloak::resources::realms::common_settings',
    value_type    => Hash,
    merge         => {
      strategy        => 'deep',
      knockout_prefix => '--',
    },
    default_value => {},
  })

  $client_common_settings = lookup({
    name          => 'keycloak::resources::clients::common_settings',
    value_type    => Hash,
    merge         => {
      strategy        => 'deep',
      knockout_prefix => '--',
    },
    default_value => {},
  })

  $realms.each |$realm_name, $realm_data| {
    $_local_realm_settings = pick($realm_data['settings'], {})
    $_full_realm_settings = deep_merge($realm_common_settings, $_local_realm_settings)

    keycloak_realm {$realm_name:
      ensure => present,
      *      => $_full_realm_settings,
    }

    $clients = pick($realm_data['clients'], {})
    $realm_client_common_settings = deep_merge($client_common_settings,
                                               pick($realm_data['client_settings'], {}))

    $clients.each |$client_name, $client_data| {
      $_local_client_settings = pick($client_data['settings'], {})
      $_full_client_settings = deep_merge($realm_client_common_settings, $_local_client_settings)

      $client_id = fqdn_uuid("${realm_name}.${client_name}")

      keycloak_client {"${client_name} on ${realm_name}":
        ensure => present,
        id => $client_id,
        *  => $_full_client_settings,
      }

      $protocol_mappers = pick($client_data['protocol_mappers'], [])

      $protocol_mappers.each | Hash $protocol_mapper_data | {
        $_pm_data = Hash($protocol_mapper_data.map |$key, $value| {
          [$key, $value ? {'__client_id__' => $client_name, default => $value}]
        })

        $protocol_mapper_name = $protocol_mapper_data['name']
        $protocol_mapper_id = fqdn_uuid("${realm_name}.${client_name}.${protocol_mapper_name}")

        keycloak_client_protocol_mapper {"${protocol_mapper_data['name']} for ${client_id} on ${realm_name}":
          ensure => present,
          id     => $protocol_mapper_id,
          *      => $_pm_data,
        }
      }
    }
  }
}
olasd updated this revision to Diff 10526.Apr 3 2020, 4:00 PM

Reuse anlambert's changes

This revision was not accepted when it landed; it landed in state Needs Review.Apr 3 2020, 4:00 PM
This revision was automatically updated to reflect the committed changes.