Skip to content

Another attempt at an OIDC integration#1568

Open
KJTsanaktsidis wants to merge 7 commits intoNixOS:masterfrom
KJTsanaktsidis:kjtsanaktsidis/oidc
Open

Another attempt at an OIDC integration#1568
KJTsanaktsidis wants to merge 7 commits intoNixOS:masterfrom
KJTsanaktsidis:kjtsanaktsidis/oidc

Conversation

@KJTsanaktsidis
Copy link

Building on the work of @lheckemann in #1298 & in @ners in https://github.com/ners/hydra/tree/oidc, I took a stab at trying to shape that work up in a way that can get merged.

This implementation:

  • Allows you to specify multiple OIDC providers in the config file, using a syntax like
<oidc>
  <provider keycloak>
    display_name = "Keycloak"
    discovery_url = "http://localhost:64446/realms/hydra-dev/.well-known/openid-configuration"
    client_id = "hydra-local"
    client_secret = "hydra-local-secret"
  </provider>
  <provider other_provider>
      ...
  </provider>
</oidc>
  • Fetches most of the configuration from the discovery_url (although you can specify issuer, authorization_endpoint, token_endpoint, and jwks_url manually)
  • Properly validates the returned ID token against the jwks keyset fetched from jwks_url (well, I hope it's properly, anyway!)
  • Looks for a hydra_roles claim, and, if present, sets roles on the user based on that. I did think about implementing some kind of mapping mechanism, but Keycloak, at least, seems to make it pretty easy to beat the mapping into shape on that side, so perhaps there is no need.
  • I made it set the username of OIDC users to "provier:$sub", which means I had to muck around with a few templates that kind of expect to display something that visibly identifies the user (like... an email address) and the $sub that keycloak, at least, uses, is just a UUID.

I banged my head trying to write some good tests for this, but really what this needs to meaningfully test things are full-system integration tests. I have a working KeycloakContext.pm on my machine which starts a Keycloak instance in the test framework, but I'm not really sure how to write the kind of browser test that would be needed to go through the login flow. Any suggestions?

@ners
Copy link
Member

ners commented Jan 31, 2026

Keycloak, at least, seems to make it pretty easy to beat the mapping into shape on that side, so perhaps there is no need

I use Kanidm, which does not have support for custom roles / claims. That makes this approach DOA for me.

@KJTsanaktsidis
Copy link
Author

What can Kanidm do/what would you expect the role mapping configuration on the Hydra side to look like?

@KJTsanaktsidis
Copy link
Author

I’ll have a play around with this when I get a chance, but it looks like kanidm can map its groups to custom claims with this mechanism - https://kanidm.github.io/kanidm/stable/integrations/oauth2/custom_claims.html

sidebar, but kanidm looks like it might be a lot less painful to integrate into the test/development environment than keycloak so I might try swapping it

@stepbrobd
Copy link
Member

for context, my hydra instance with @ners patch use kanidm as well and it looks like this

https://github.com/stepbrobd/dotfiles/blob/5f859debb62e710950780980047e279f4d3d778d/hosts/server/odake/hydra.nix#L53

@KJTsanaktsidis
Copy link
Author

Hm, OK, so the reason kanidm can't literally just put the inside-hydra role names in a custom claim is because the custom claim values have to match the regex ^[0-9a-zA-Z_]+$: https://github.com/kanidm/kanidm/blob/cbd9c5135e08671d642284758ad977dac5391ac1/server/lib/src/value.rs#L1947

Most of the hydra roles (all except admin) have a - in them.

This limitation is probably a bit too strict on the kanidm side IMO - it's been discussed a few times kanidm/kanidm#2641, kanidm/kanidm#2882. I'll go add my two-cents worth into there. And in the meanwhile, I guess, make the hydra side accept e.g. bump_to_front as an alias for the bump-to-front role perhaps.

This will be of use when developing & testing OIDC
The OIDC work needs to cache some information (at least, the JWKS keys)
so we don't ask for them over and over again needlessly. An in-process
cache is in theory fine, but using Plugin::Cache nicely gives us expiry
support which we need too (to catch key revocations).
The login code in doEmailLogin is treating email address & username as
the same concept, but it shouldn't be. Allow passing in a username field
explicitly to doEmailLogin to set as the user's username column.

The templates have also been changed to call a new usernameForDisplay
method which will be able to customise the display of the username in
circumstances where the username would look stupid (e.g. in the
forthcoming OIDC support)
OIDC providers are configured in the config file under <oidc> <provider
foobar>. There can be multiple providers, each one will get a menu item
in the dropdown to log in with.

After login is successful, we validate all the claims and then perform
doEmailLogin, creating the user if it did not exist.
@KJTsanaktsidis
Copy link
Author

OK I gritted my teeth and managed to write an end-to-end test for the OIDC integration (against a temporary Kanidm instance) based on WWW::Mechanize. Thankfully both Hydra and Kanidm both don't need any javascript.

If presented with a "hydra_roles" claim, we will set the user roles to
that claim.
@lheckemann
Copy link
Member

Note also the work in the lix fork of hydra: https://git.lix.systems/lix-project/hydra/pulls/73

@KJTsanaktsidis
Copy link
Author

Oh interesting

  • use oidc logout method

this is probably worth doing here too

  • removes google/github login

this also sounds worth doing (or, at least reimplementing these specia cases in terms of the generic OIDC support) but probably not in a first pass - need to think carefully about the migration strategy to avoid breaking existing deployments

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.

4 participants