feat: add keycloak authentication closes #1634 #1716

onecrazygenius · 2025-01-01T13:08:07Z

Affected core subsystem(s)

Alterations of the backend to support keycloak as an OAuth mechanism for user lookup functionality.

Description of change

I've got grant working with keycloak using the below config:

oauth: {
    provider: 'keycloak',

    grant: {
      defaults: {
        origin: 'http://localhost:8080',
        prefix: '/api/connect',
        transport: 'session',
      },

      keycloak: {
        key: 'bailo_app',
        secret: 'client_secret',
        dynamic: ['scope'],
        access_url: 'http://keycloak:8080/realms/bailo/protocol/openid-connect/token', // internal/backend container accessible route
        response: ['tokens', 'raw', 'jwt'],
        callback: '/',
        authorize_url: 'http://localhost:8888/realms/bailo/protocol/openid-connect/auth', // public url
        scope: ['openid'],
      },
    },
}

To support this authentication mechanism the backend needs updating to support another client from cognito so the code changes made are in aim of supporting multiple providers naively. This required the following changes:

additional config is required to communicate with keycloak for user list interactivity
creating the code to mimic the cognito client for keycloak to actually 'get' users
enabling a toggle system based on the config for the application to load the correct provider
updating the existing cognito client to recognise optional config

The aim is you can do this with the config:

    // cognito: {
    //   identityProviderClient: {
    //     region: 'eu-west-1',
    //     credentials: {
    //       accessKeyId: '',
    //       secretAccessKey: '',
    //     },
    //     userPoolId: '',
    //   },
    // },

    keycloak: {
      realm: 'baio',
      clientId: 'client_id',
      clientSecret: 'client_secret',
      serverUrl: 'http://keycloak:8080', // internal route 
    }

so you could use either keycloak or cognito and dont need to specifiy one or the other if they are not required, but i'll take your guidance on how we do this multiple provider interactivity!

Appreciate this is naïve implementation so feedback is appreciated!

onecrazygenius · 2025-01-01T13:20:28Z

For local setup and testing of this I have used the following configuration, which is not included in the PR as its just config:

Added a keycloak instance to the docker-compose:

# # keycloak for testing oauth integration
keycloak:
   image: quay.io/keycloak/keycloak:26.0.6
   environment:
     - KC_BOOTSTRAP_ADMIN_USERNAME=admin
     - KC_BOOTSTRAP_ADMIN_PASSWORD=admin
   ports:
     - 8888:8080
   command: start-dev

Then in Keycloak I have created an OAuth application with this exported config

config.json

Some Javascript

{
  "clientId": "bailo-app",
  "name": "",
  "description": "",
  "rootUrl": "",
  "adminUrl": "",
  "baseUrl": "",
  "surrogateAuthRequired": false,
  "enabled": true,
  "alwaysDisplayInConsole": false,
  "clientAuthenticatorType": "client-secret",
  "secret": "rG59lqdDgfhWBmZuV7uOP3RfVCLPlYPD",
  "redirectUris": [
    "http://localhost:8080/api/connect/keycloak/callback"
  ],
  "webOrigins": [
    "*"
  ],
  "notBefore": 0,
  "bearerOnly": false,
  "consentRequired": false,
  "standardFlowEnabled": true,
  "implicitFlowEnabled": false,
  "directAccessGrantsEnabled": true,
  "serviceAccountsEnabled": true,
  "publicClient": false,
  "frontchannelLogout": true,
  "protocol": "openid-connect",
  "attributes": {
    "realm_client": "false",
    "oidc.ciba.grant.enabled": "false",
    "client.secret.creation.time": "1735734708",
    "backchannel.logout.session.required": "true",
    "post.logout.redirect.uris": "+",
    "oauth2.device.authorization.grant.enabled": "true",
    "display.on.consent.screen": "false",
    "backchannel.logout.revoke.offline.tokens": "false"
  },
  "authenticationFlowBindingOverrides": {},
  "fullScopeAllowed": true,
  "nodeReRegistrationTimeout": -1,
  "protocolMappers": [
    {
      "name": "Client Host",
      "protocol": "openid-connect",
      "protocolMapper": "oidc-usersessionmodel-note-mapper",
      "consentRequired": false,
      "config": {
        "user.session.note": "clientHost",
        "id.token.claim": "true",
        "introspection.token.claim": "true",
        "access.token.claim": "true",
        "claim.name": "clientHost",
        "jsonType.label": "String"
      }
    },
    {
      "name": "Client IP Address",
      "protocol": "openid-connect",
      "protocolMapper": "oidc-usersessionmodel-note-mapper",
      "consentRequired": false,
      "config": {
        "user.session.note": "clientAddress",
        "id.token.claim": "true",
        "introspection.token.claim": "true",
        "access.token.claim": "true",
        "claim.name": "clientAddress",
        "jsonType.label": "String"
      }
    },
    {
      "name": "Client ID",
      "protocol": "openid-connect",
      "protocolMapper": "oidc-usersessionmodel-note-mapper",
      "consentRequired": false,
      "config": {
        "user.session.note": "client_id",
        "id.token.claim": "true",
        "introspection.token.claim": "true",
        "access.token.claim": "true",
        "claim.name": "client_id",
        "jsonType.label": "String"
      }
    }
  ],
  "defaultClientScopes": [
    "web-origins",
    "acr",
    "profile",
    "roles",
    "basic",
    "email"
  ],
  "optionalClientScopes": [
    "address",
    "phone",
    "organization",
    "offline_access",
    "microprofile-jwt"
  ],
  "access": {
    "view": true,
    "configure": true,
    "manage": true
  }
}

And then ensure the inherited user roles allow users to call realm management to list user

just if needed as a reference for local testing (myself included incase I lost this!)

JR40159 · 2025-01-02T18:24:17Z

backend/src/clients/keycloak.ts

Please include unit tests

backend/src/clients/keycloak.ts

JR40159 · 2025-01-03T16:27:59Z

backend/src/connectors/authentication/oauth.ts

Did you consider having two separate connectors, one for oauthCognito and one for oauthKeyloak? It might be slightly clearer to keep them separate rather than have one with conditional logic. The conditional logic might just be one if statement currently but more will be needed when support for groups and roles are added. Having them separate will probably make adding changes to either the Cognito or Keycloak implementations easier. I don't think there is much difference though, let me know your thoughts.

I think this bullet point you've included in an earlier comment summarises the drawback in have a single connector like you have here "enabling a toggle system based on the config for the application to load the correct provider". The connector design itself is a toggle and so your implementation is adding a toggle within a toggle.

I think the reasoning for going with this implementation is then each client type say you had a google oauth or azure entra is a future version, the flexibility provided comes from each client has a config and then the main oauth connector acts as a 'resolver'. So this implementation seemed to fit with the existing design to an extent with code seperated for the client and connector type

The challenge of splitting the connectors/clients completely apart would probably be duplication of code despite simplifying the configuration. I was aiming to take inspiration from next-auth where you have a general OAuth Connector and then configure your client to use for that OAuth connection. e.g?

/~https://github.com/nextauthjs/next-auth/blob/main/packages/core/src/index.ts
/~https://github.com/nextauthjs/next-auth/blob/main/packages/core/src/providers/cognito.ts

I think your lead on which way to implement is obviously best as I want to alter your codebase, so for the rest of comments for now I'll fix with the current implementation where possible but we can look at how this get structured. Either way is a trade of in flexibility at either a config or code level really.

JR40159 · 2025-01-03T16:32:16Z

backend/src/clients/keycloak.ts

+  } catch (err) {
+    throw InternalError('Error when querying Keycloak for users.', { err })
+  }
+  const resultsData = await results.json() as any[]


I would recommend using a type guard here rather than using a type assertion with an array of type any

See here for an example /~https://github.com/gchq/Bailo/blob/main/backend/src/clients/registry.ts#L61

added in new commit

JR40159 · 2025-01-03T16:37:01Z

backend/src/clients/keycloak.ts

+      },
+      body: params
+    })
+    const data = await response.json() as { access_token: string }


See other comment about using type guards rather type assertions.

added as above let me know if anything else needed

JR40159 · 2025-01-03T16:54:35Z

backend/src/clients/keycloak.ts

+
+  let results
+  try {
+    results = await fetch(url, {


Please use node-fetch as we have done for similar clients.

utilised node-fetch and added types of Response to results

JR40159 · 2025-01-03T16:59:07Z

backend/src/utils/config.ts

Please update the helm chart configuration with these changes here /~https://github.com/gchq/Bailo/blob/main/infrastructure/helm/bailo/templates/bailo/bailo.configmap.yaml and the values file.

updated chart with configmap and values changes.

JR40159 · 2025-01-03T17:00:16Z

backend/src/utils/config.ts

+  !config.oauth.keycloak &&
+  !config.oauth.cognito
+) {
+  throw new Error('If OAuth is configured, either Keycloak or Cognito configuration must be provided.')


This error type would be well suited here

Bailo/backend/src/utils/error.ts

Line 55 in 7b4b784

export function ConfigurationError(message: string, context?: BailoError['context'], logger?: Logger) {

JR40159 · 2025-01-03T17:15:10Z

backend/src/clients/keycloak.ts

+  let dnName: string
+  let realm: string
+  try {
+    if (!config?.oauth?.keycloak) {


Why the ? after config?

For readability, I would recommend moving this if statement to just before the try statement.

both this an configurationerror have been fixed, just missed them on a proof read

backend/src/clients/keycloak.ts

JR40159 · 2025-01-03T18:24:28Z

backend/src/utils/config.ts

@@ -114,11 +114,17 @@ export interface Config {
  oauth: {
    provider: string
    grant: grant.GrantConfig | grant.GrantOptions
-    cognito: {
+    cognito?: {


Having optional blocks of configuration isn't something we've decided to do for things like this previously as you can see from the oauth block which would only be used if the oauth connector is selected. We define everything anyway in the default configuration as a method of documenting example configuration so everything should always been defined there. This is another reason (in addition to /~https://github.com/gchq/Bailo/pull/1716/files#r190195685) why I'd have two separate connectors that can be chosen using the existing authentication connector config value rather than the presence of an optional config block. Most of the other existing optional config like the oauth config is very similar to this and is connector related and it only get used if the relevant connector is selected. As we don't explicitly define any of the configuration as optional though, it probably would a good idea to include this in documentation. I'll make a note to include this information in the connector documentation we're planning on adding.

I've added my thoughts around this to the top comment, let me know what you would prefer and I'm happy to add around this as needed

onecrazygenius · 2025-01-05T22:30:23Z

Hi @JR40159! I've responded to comments to add fixes/test and added my thoughts around the oauth config stuff. lmk what you think!

onecrazygenius added 4 commits November 27, 2024 20:39

feat(keycloak): allow new oauth config

b3fbcf1

feat(keycloak): keycloak provider for listing users.

416060c

fix(cognito): type error from keycloak changes.

d337b9b

fix: parse users correctly

a7a8a13

onecrazygenius marked this pull request as ready for review January 1, 2025 13:09

JR40159 requested changes Jan 3, 2025

View reviewed changes

onecrazygenius added 7 commits January 4, 2025 12:31

fix: add keycloak type guard clause

d58953c

fix: add keycloak type guard clause (no.2)

89f03a5

fix: used node-fetch and added types

a1ea1d1

feat: update helm chart to have keycloak config

52e9cc0

fix: use configurationerror instead of generic

2ae3800

fix: general keycloak file tidying

ad44bc4

test: add tests for keycloak client

2723eae

onecrazygenius requested a review from JR40159 January 6, 2025 11:46

Merge branch 'main' into main

26b9eaa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add keycloak authentication closes #1634 #1716

feat: add keycloak authentication closes #1634 #1716

onecrazygenius commented Jan 1, 2025

onecrazygenius commented Jan 1, 2025

Some Javascript

JR40159 Jan 2, 2025

JR40159 Jan 3, 2025

JR40159 Jan 3, 2025

onecrazygenius Jan 4, 2025

JR40159 Jan 3, 2025

JR40159 Jan 3, 2025

onecrazygenius Jan 4, 2025

JR40159 Jan 3, 2025

onecrazygenius Jan 4, 2025

JR40159 Jan 3, 2025

onecrazygenius Jan 4, 2025

JR40159 Jan 3, 2025

onecrazygenius Jan 4, 2025

JR40159 Jan 3, 2025

onecrazygenius Jan 4, 2025

JR40159 Jan 3, 2025

JR40159 Jan 3, 2025

onecrazygenius Jan 4, 2025

JR40159 Jan 3, 2025

onecrazygenius Jan 4, 2025

onecrazygenius commented Jan 5, 2025

feat: add keycloak authentication closes #1634 #1716

Are you sure you want to change the base?

feat: add keycloak authentication closes #1634 #1716

Conversation

onecrazygenius commented Jan 1, 2025

Affected core subsystem(s)

Description of change

onecrazygenius commented Jan 1, 2025

Some Javascript

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onecrazygenius commented Jan 5, 2025