Skip to content

Another attempt at an OIDC integration#1568

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

Another attempt at an OIDC integration#1568
KJTsanaktsidis wants to merge 23 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.

@KJTsanaktsidis KJTsanaktsidis force-pushed the kjtsanaktsidis/oidc branch 2 times, most recently from f9f665d to 7075f60 Compare February 3, 2026 05:08
@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.

@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

@dasJ dasJ added the www hydra-www component label Feb 13, 2026
@Mic92
Copy link
Member

Mic92 commented Mar 26, 2026

@KJTsanaktsidis sorry, just see this just now. I will put it on my list.

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.
If presented with a "hydra_roles" claim, we will set the user roles to
that claim.
@Mic92 Mic92 force-pushed the kjtsanaktsidis/oidc branch from 5b86036 to 27d5d98 Compare March 27, 2026 00:01
@Mic92
Copy link
Member

Mic92 commented Mar 27, 2026

I rebased this for you.

Ma27 and others added 7 commits March 27, 2026 01:09
Without RP-Initiated Logout, users stay signed in at the IdP after
logging out of Hydra, so clicking "Sign in" immediately re-authenticates
without a password prompt.

The endpoint is read from the discovery document or can be set manually.

Adapted from https://git.lix.systems/lix-project/hydra/pulls/73

Co-authored-by: Jörg Thalheim <joerg@thalheim.io>
fixup! Implement OIDC login

Sanitize 'after' parameter to prevent open redirect: //evil.com would
be interpreted as a protocol-relative URL by browsers.
fixup! Implement OIDC login

Use constant-time comparison for state and nonce to avoid timing
side-channels.
fixup! Implement OIDC login

- Reject token endpoint responses with HTTP error but no 'error' field
- Validate aud claim exists before dereferencing
fixup! Add mapping of OIDC claims to roles

Wrap setRoles INSERT+DELETE in a transaction so role updates are
atomic.
fixup! Implement OIDC login

Don't let an unreachable IdP prevent Hydra from starting. Log the
error and disable OIDC login instead of dying in setup_finalize.
fixup! Add test for OIDC implementation

Move test-only Perl modules (TestWWWMechanize, HTTPCookieJar, ...) from
the main hydra package to hydra-tests where they belong.
fixup! hydra-server: redirect to OIDC end_session_endpoint on logout

Add CSRF protection to GET /logout: require a token derived from the
session ID (sha256(sessionid + ':logout')) so cross-site requests
cannot log the user out. The token is exposed to templates via the
stash and embedded in the Sign out link.
@Mic92
Copy link
Member

Mic92 commented Mar 27, 2026

@KJTsanaktsidis I found a few issues with the implementation. Please have a look.

);


# Derive a CSRF token for GET /logout from the session ID. Using a hash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are technically not doing this anywhere else yet, but I thought we could start at least. Hydra can also serve html from users, so this sounds like a bit of a dangerous combination.

@Mic92
Copy link
Member

Mic92 commented Mar 27, 2026

I pushed a stack of fixup! commits addressing issues found during review. Kept separate for easier review; once you're happy I'll autosquash.

  • 780d45f Open redirect: ?after=/evil.com became //evil.com (protocol-relative). Now strips leading / and \.
  • b40f60c state/nonce comparison now uses String::Compare::ConstantTime.
  • 913409f GET /logout now requires a CSRF token (sha256(sessionid + ":logout")), embedded in the Sign out link.
  • f5ad8fa Token endpoint: reject HTTP errors even when JSON lacks error field; validate aud claim exists.
  • 2d69ea1 setRoles: wrap INSERT+DELETE in a transaction.
  • 5627981 OIDC discovery failure at startup now logs a warning instead of preventing Hydra from booting.
  • b43f624 Moved test-only Perl modules (TestWWWMechanize, HTTPCookieJar, …) from hydra to hydra-tests.
  • a12ed58 Remove unused imports (Data::Dumper debug leftover, sha256_hex).
  • 970f717 Drop redundant base64URLEncode wrapper — MIME::Base64::encode_base64url already strips = padding.
  • 26dd7c1 Collapse 4× repeated config validation into a loop; error now names which field is missing.
  • f46aded Let Crypt::JWT verify iss/aud/exp/nbf instead of reimplementing OIDC §3.1.3.7 by hand (−57 lines).
  • ebd46f1 Drop Crypt::URandom::Token dependency and its flake overlay — encode_base64url(urandom(...)) satisfies RFC 6749/7636 requirements.
  • Commit 6e25009 config-fixup needs manual squash into "Add mapping of OIDC claims to roles" (not a proper fixup!).
  • bc6aa4d RP-Initiated Logout: redirect to IdP's end_session_endpoint on sign out. Adapted from https://git.lix.systems/lix-project/hydra/pulls/73, credited to Maximilian Bosch.
  • 48f82c5 Modernize with Perl 5.36+ signatures and try/catch — drops my (...) = @_ boilerplate and avoids the $@-clobbered-by-destructor footgun.
  • af3c3fd Make OIDC discovery lazy — resolve on first login with 1h cache instead of blocking startup; IdP outage no longer prevents Hydra from booting and recovers without restart.

Mic92 added 8 commits March 27, 2026 01:30
fixup! Implement OIDC login

Remove unused imports (Data::Dumper debug leftover, sha256_hex).
fixup! Implement OIDC login

Drop redundant base64URLEncode wrapper: MIME::Base64::encode_base64url
already strips '=' padding per RFC 4648 §5, so the s/=+$//r was a
no-op.
fixup! Implement OIDC login

Collapse 4x repeated config validation into a loop. Bonus: the error
now names which field is missing instead of listing all four.
fixup! Implement OIDC login

Let Crypt::JWT verify iss/aud/exp/nbf instead of reimplementing the
OIDC §3.1.3.7 checklist by hand. The library already handles the
array-or-scalar aud case and rejects alg=none. Keep only the
OIDC-specific nonce check and sub presence check.
fixup! Implement OIDC login

Drop Crypt::URandom::Token dependency (and its flake overlay).
base64url-encoding raw urandom bytes satisfies both RFC 6749
state/nonce and RFC 7636 PKCE verifier requirements without needing
a custom token-from-charset generator.
fixup! Implement OIDC login

Modernize with Perl 5.36+ features:
- Subroutine signatures replace 'my ($self, ...) = @_' boilerplate
- try/catch replaces eval/$@ (avoids the $@-clobbered-by-destructor
  footgun and gives a lexically-scoped error variable)
- Simplify scope construction with join+grep
fixup! Implement OIDC login

Make OIDC discovery lazy instead of blocking startup.

Previously resolveOIDCConfig did an HTTP GET to the discovery_url in
setup_finalize, meaning every Hydra start blocked on the IdP for up to
10s and an IdP that was down at boot left OIDC broken until restart.

Now resolveOIDCConfig only loads secrets from disk and validates static
config (no network). Discovery happens in _resolvedConf on the first
login attempt, cached for 1h via Plugin::Cache and also written back
into the in-process config so subsequent calls short-circuit. An IdP
outage now only affects users trying to log in (503), and recovers
automatically without a restart.
fixup! Add Kanidm to the Foreman configuration

Bump Kanidm startup timeout from 60s to 180s (overridable via
KANIDM_STARTUP_TIMEOUT) and dump the log tail on timeout. The 60s
limit was observed to be insufficient on loaded aarch64 GitHub CI
runners where tests run in parallel; and pointing at a log file
inside the sandbox is useless once the build fails.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

www hydra-www component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants