lix-project/hydra
4
2
Fork 10
Code Issues 5 Pull requests 5 Projects Releases Packages Wiki Activity

OIDC login #73

Open
ma27 wants to merge 14 commits from oidc into main
pull from: oidc
merge into: lix-project:main
Conversation 1 Commits 14 Files changed 10 +468 -98
ma27 commented 2025-12-24 10:32:21 +00:00
Member
Copy link

Closes #38

cc @benaryorg @raito

Based on https://github.com/NixOS/hydra/pull/1298

Additional features:

  • actually validate the id token
  • use well-known endpoint rather than having to configure dozens of endpoints by hand
  • use oidc logout method
  • removes google/github login

Todo

  • Deploy to a live instance
  • Migration code for google/github users
  • Docs
Closes #38 cc @benaryorg @raito Based on https://github.com/NixOS/hydra/pull/1298 Additional features: * actually validate the id token * use well-known endpoint rather than having to configure dozens of endpoints by hand * use oidc logout method * removes google/github login Todo * [x] Deploy to a live instance * [x] Migration code for google/github users * [x] Docs
ma27 added 10 commits 2025-12-24 10:32:22 +00:00 
Co-Authored-By: Maximilian Bosch <maximilian@mbosch.me>
(cherry picked from commit f963dba1552d85f299859c8a78b1d9ef71f69e76)
(cherry picked from commit 039165e1dbad90bcf755f441c22d81ed5233f96b)
(cherry picked from commit 9d8b420281fd0f9bc7443474747de32e341e9748)
(cherry picked from commit 8d220903696a0be5459777d32ad7ef05c6ee7970)
(cherry picked from commit f9072362dd77fccb659913ed3c35bfb42a4d3a29)
(cherry picked from commit a9c16a19518a238d74fce789e27dd166ef7058b1)
* Struct with all parameters inside
* Normalize / process all parameters initially and fail early
* Use well-known URL and derive all other URLs from that.
We have generic OIDC now, no need for custom implementations!
ma27 changed title from Draft: OIDC login to OIDC login 2025-12-30 13:16:49 +00:00
lheckemann reviewed 2026-01-29 09:36:57 +00:00
lheckemann left a comment
Member
Copy link

A couple of typos, some practical concerns that I'm uncertain about.

Sorry for taking so long!

A couple of typos, some practical concerns that I'm uncertain about. Sorry for taking so long!
doc/manual/src/configuration.md Outdated
@ -261,6 +261,72 @@ default role mapping:
Note that configuring both the LDAP parameters in the hydra.conf and via
the environment variable is a fatal error.
OIDC Authentijcation backend
lheckemann commented 2026-01-29 09:19:57 +00:00
Member
Copy link

Typo: Authentication

Typo: Authentication
ma27 marked this conversation as resolved
doc/manual/src/configuration.md Outdated
@ -264,0 +270,4 @@
```
<oidc>
enable = 0 # 1 to enab le
lheckemann commented 2026-01-29 09:20:24 +00:00
Member
Copy link

typo: enable

typo: enable
ma27 marked this conversation as resolved
src/lib/Hydra/Config.pm Outdated
@ -155,1 +159,4 @@
my $oidcCfgCache;
sub downloadSettings {
lheckemann commented 2026-01-29 09:12:55 +00:00
Member
Copy link

I think I preferred the previous name downloadOIDCSettings, there isn't really that much context that makes it obvious that this is about OIDC without reading the implementation or call site?

I think I preferred the previous name `downloadOIDCSettings`, there isn't really that much context that makes it obvious that this is about OIDC without reading the implementation or call site?
ma27 marked this conversation as resolved
src/lib/Hydra/Controller/User.pm Outdated
@ -166,0 +230,4 @@
doEmailLogin($self, $c, "oidc", $claims->{email}, $claims->{name} // undef);
if ($oidcConfig->{groups_to_roles}) {
$c->user->userroles->delete;
lheckemann commented 2026-01-29 09:25:48 +00:00
Member
Copy link

This could lock an admin out if they've been using stateful role membership up to now? Seems potentially annoying. I think this should probably be mentioned in the docs at the very least.

This could lock an admin out if they've been using stateful role membership up to now? Seems potentially annoying. I think this should probably be mentioned in the docs at the very least.
ma27 commented 2026-02-07 18:02:39 +00:00
Author
Member
Copy link

I mean there are several relatively simple ways to recover:

  • hydra-create-user
  • Do the group mapping in the IdP.

Especially considering that this needs a flag to be enabled to work at all.

I mean there are several relatively simple ways to recover: * `hydra-create-user` * Do the group mapping in the IdP. Especially considering that this needs a flag to be enabled to work at all.
👍 1
lheckemann marked this conversation as resolved
src/lib/Hydra/Controller/User.pm Outdated
@ -166,0 +237,4 @@
my @mapping = @{$oidcConfig->{groups_to_roles}->{$group}};
for my $mapped_role (@mapping) {
next unless $mapped_role;
$c->user->userroles->create({ role => $mapped_role });
lheckemann commented 2026-01-29 09:34:01 +00:00
Member
Copy link

I'm not confident enough in my knowledge of the system as a whole to be sure of any of what I'm saying here, but...

This empties and recreates the users' roles in the DB on every login? Probably not an issue realistically, but it does cause some churn.

"Caching" the roles like this could also cause confusion when looking at users in the admin UI:

  • Stateful changes are still allowed, but will be overridden by the login?
  • They won't be updated if the group memberships change on the OIDC side, at least until the user logs in again. The stale info may be confusing.

Maybe the roles should be stored in the session instead, and the admin UI should make sure to show that the roles are unknown due to coming from OIDC mapping?

I'm not confident enough in my knowledge of the system as a whole to be sure of any of what I'm saying here, but... This empties and recreates the users' roles in the DB on every login? Probably not an issue realistically, but it does cause some churn. "Caching" the roles like this could also cause confusion when looking at users in the admin UI: - Stateful changes are still allowed, but will be overridden by the login? - They won't be updated if the group memberships change on the OIDC side, at least until the user logs in again. The stale info may be confusing. Maybe the roles should be stored in the session instead, and the admin UI should make sure to show that the roles are unknown due to coming from OIDC mapping?
ma27 commented 2026-02-07 18:00:55 +00:00
Author
Member
Copy link

Wouldn't it be way easier to just turn off that part of the UI when the user type is OIDC rather than having two places where roles can be stored?

Wouldn't it be way easier to just turn off that part of the UI when the user type is OIDC rather than having two places where roles can be stored?
lheckemann commented 2026-02-07 18:13:42 +00:00
Member
Copy link

Sure, that would be fine by me too.

Sure, that would be fine by me too.
src/lib/Hydra/Controller/User.pm Outdated
@ -166,0 +196,4 @@
accepted_alg => [$alg],
verify_sub => sub { return 1; },
verify_aud => sub { return 1; },
verify_iat => sub { return 1; }
lheckemann commented 2026-01-29 09:24:22 +00:00
Member
Copy link

We should probably validate the issued-at time in some way?

We should probably validate the issued-at time in some way?
ma27 commented 2026-02-07 17:59:45 +00:00
Author
Member
Copy link

This is already happening in decode_jwt: https://metacpan.org/dist/Crypt-JWT/source/lib/Crypt/JWT.pm#L239

I wonder if we should prohibit stuff that's too far in the past though?

This is already happening in decode_jwt: https://metacpan.org/dist/Crypt-JWT/source/lib/Crypt/JWT.pm#L239 I wonder if we should prohibit stuff that's too far in the past though?
src/lib/Hydra/Controller/User.pm Outdated
@ -166,0 +204,4 @@
}
unless ($decoded_token->{aud} eq $oidcConfig->{client_id}) {
error($c, "Token audience/subject don't match ID token", 401);
lheckemann commented 2026-01-29 09:22:44 +00:00
Member
Copy link

Shouldn't this be in verify_aud function passed to decode_jwt above?

Shouldn't this be in verify_aud function passed to decode_jwt above?
ma27 marked this conversation as resolved
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin oidc:oidc
git switch oidc

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff oidc
git switch oidc
git rebase main
git switch main
git merge --ff-only oidc
git switch oidc
git rebase main
git switch main
git merge --no-ff oidc
git switch main
git merge --squash oidc
git switch main
git merge --ff-only oidc
git switch main
git merge oidc
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
lix-project/hydra!73
No description provided.