Skip to content

fix(keycloak): find the managed client by id, not by name - #336

Open
aweingarten wants to merge 1 commit into
linode:mainfrom
aweingarten:fix/find-managed-client-by-id
Open

aweingarten wants to merge 1 commit into
linode:mainfrom
aweingarten:fix/find-managed-client-by-id

Conversation

@aweingarten

Copy link
Copy Markdown

The bug

keycloakRealmProviderConfigurer locates the client it reconciles with:

const client = createClient(uniqueUrls, env.KEYCLOAK_HOSTNAME_URL, env.KEYCLOAK_CLIENT_SECRET)
const allClients = (await api.clients.adminRealmsRealmClientsGet(keycloakRealm)).body
const existingClient = allClients.find((el) => el.name === client.name)

otomiClientCfgTpl sets no name, so client.name is undefined and that
expression means "the first client in the realm with no name".

That is correct only while the managed client is the only nameless one — and
nothing in the realm enforces it. Any client created through the admin API
without a name captures the lookup as soon as its clientId sorts earlier:
an operator adding one in the console, or external tooling provisioning its
own OIDC client.

Why it is worth fixing rather than working around

The operator then PUTs the managed client's representation — including
authorizationServicesEnabled: true — onto the intruder. If that client is
public, Keycloak refuses it in RepresentationToModel.createResourceServer:

Only confidential clients are allowed to set authorization settings

The 500 rolls the transaction back, which is what makes this so hard to
see. Nothing drifts. The managed client stays confidential, the intruder keeps
working, and every client in the realm reads as correct.

But the reconcile aborts at this step on every 30s retry and never reaches
IDPManagermanageUsers. Users added in the APL console are never
written to the realm.
Their logins fail user_not_found — surfaced in the UI
as "invalid username or password" — while their records sit in the apl-users
namespace, read correctly by the operator's own watcher. The operator log shows
only a repeating Updating otomi client followed by a 500 with no detail.

We hit this on a production cluster. Diagnosing it from the symptom took a
while, because every direct check of the realm says it is healthy.

The change

Match on Keycloak's identity fields instead: id — which otomiClientCfgTpl
already pins to otomi — or clientId, the same value and unique per realm.

Accepting either keeps realms whose client was created without the pinned id
resolving as well, rather than falling to the create branch and POSTing a
clientId that is already taken.

The lookup moves to realm-factory.ts as findManagedClient so it can be
tested directly — keycloakRealmProviderConfigurer is not exported.

It deliberately does not give the managed client a name. That would also
fix the collision, but it would break a rollback to any earlier image, whose
nameless lookup would then miss the client and fall to its create branch.

Tests

src/tasks/keycloak/realm-factory.test.ts — six cases: match by pinned id;
match by clientId when the internal id is a generated uuid; do not match
an unrelated nameless client (the regression); several nameless clients around
it; absent client returns undefined so the caller still creates it; explicit
clientId argument.

One caveat on verification

I could not execute the suite locally: the @linode/* dependencies resolve
from GitHub Packages and npm ci fails with 401 ... authentication token not provided, so no dev dependencies install and jest cannot start. The changed
files parse clean, the diff is small, and the new test file has no dependency
beyond findManagedClient and the ClientRepresentation type — but CI running
green here is the real check, not my say-so.

Happy to adjust naming or move findManagedClient elsewhere if you would
rather it live with the operator.

`keycloakRealmProviderConfigurer` located the client it reconciles with

    allClients.find((el) => el.name === client.name)

and `otomiClientCfgTpl` sets no `name`, so that expression evaluated to
"the first client in the realm with no name". That is correct only while
this client is the only nameless one, and nothing in the realm enforces
it: any client created through the admin API without a `name` — by an
operator in the console, or by external tooling provisioning its own OIDC
client — captures the lookup as soon as its clientId sorts earlier.

The consequence is severe and nearly invisible. The operator then PUTs
the managed client's representation, `authorizationServicesEnabled: true`
included, onto the intruder. If that client is public, Keycloak refuses:

    Only confidential clients are allowed to set authorization settings

The 500 rolls the transaction back, so nothing drifts and nothing in the
realm looks wrong afterwards — the managed client stays confidential, the
intruder keeps working, every client reads as correct. But the reconcile
aborts at this step on every 30s retry and never reaches IDPManager ->
manageUsers. Users added in the APL console are never written to the
realm, and their logins fail `user_not_found` while their records sit in
the `apl-users` namespace, read correctly by the operator's own watcher.
The operator log shows only a repeating "Updating otomi client" and a 500.

Match on Keycloak's identity fields instead — `id`, which
otomiClientCfgTpl already pins, or `clientId`, the same value and unique
per realm. Accepting either keeps realms whose client was created without
the pinned id resolving too, rather than falling to the create branch and
POSTing a clientId that is already taken.

The lookup moves to realm-factory.ts as `findManagedClient` so it can be
tested directly; `keycloakRealmProviderConfigurer` is not exported.

Deliberately does NOT give the managed client a `name`. That would also
fix the collision, but it would break a rollback to any earlier image,
whose nameless lookup would then miss the client and fall to its create
branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant