Implement generic OIDC-based authentication #1298

lheckemann · 2023-08-20T17:47:47Z

No description provided.

Co-Authored-By: Maximilian Bosch <[email protected]>

dasJ

We actually have a patch for our idam that is less generic than yours (which is why it's not upstreamed). The comments probably reflect that ;)
I do like your approach which using proper libraries a lot! Also, how testable is this?

dasJ · 2023-08-21T06:43:15Z

src/lib/Hydra/Controller/User.pm

+
+    my $after = "/" . $c->req->params->{after};
+
+    $c->res->cookies->{'after_oidc'} = {


I saw this in GitHub as well and I really don't know why we would do this. We already have a session so why not do:

$c->session->{oidc_after} = $after;

You already do this for the state (which is what GitHub is not doing right now afaik 👀)

Yes, this is just taken directly from the other auth methods. We did discuss that it should also probably be a single session variable regardless of the auth method. I'd prefer to fix this later though.

dasJ · 2023-08-21T06:44:08Z

src/lib/Hydra/Controller/User.pm

+    $c->session->{oidc_state} = $state;
+    my $redirect_url = $oidc_client->uri_to_redirect(
+        redirect_uri => $c->uri_for('/oidc-login'),
+        scope        => q{openid},


Would be nice to have this configurable

I'm not sure that's desirable: we care about the name, email and sub fields which are standard in openid, and I would very much like to avoid going further than that and requiring mapping logic.

dasJ · 2023-08-21T06:44:49Z

src/lib/Hydra/Controller/User.pm

@@ -92,14 +104,16 @@ sub doLDAPLogin {
 }

 sub doEmailLogin {
-    my ($self, $c, $type, $email, $fullName) = @_;
+    my ($self, $c, $type, $email, $fullName, $username) = @_;

    die "No email address provided.\n" unless defined $email;

    # Be paranoid about the email address format, since we do use it
    # in URLs.
    die "Illegal email address.\n" unless $email =~ /^[a-zA-Z0-9\.\-\_]+@[a-zA-Z0-9\.\-\_]+$/;


Unrelated: this is the absolute worst because it excludes plus-emails

dasJ · 2023-08-21T06:46:36Z

src/lib/Hydra/Controller/User.pm

+    error($c, "Email address must be verified.", 401) unless $claims->{email_verified};
+
+    doEmailLogin($self, $c, "oidc", $claims->{email}, $claims->{name} // undef, sprintf("oidc:$claims->{sub}"));
+


I think it'd be nice to have some role mapping as well because otherwise everyone has no permissions. We use:

+ my $role_mapping = $c->config->{oidc}->{role_mapping} or (); + # Give out roles + $c->user->userroles->delete; + push(@{$user_data->{groups}}, ("default")); + foreach my $group (@{$user_data->{groups}}) { + if (defined($role_mapping->{$group})) { + my $roles = $role_mapping->{$group}; + if (ref($roles) eq 'ARRAY') { + for my $mapped_role (@{$roles}) { + $c->user->userroles->create({ role => $mapped_role }); + } + } else { + $c->user->userroles->create({ role => $roles }); + } + } + }

We discussed this at length (this was what took longest by far), and came to the conclusion that this is a real can of worms, because OIDC is about authentication and identity but not authorisation. For now, our recommended approach is to manage permissions manually.

dasJ · 2023-08-21T06:47:46Z

src/lib/Hydra/Controller/User.pm

+sub oidc_login :Path('/oidc-login') Args(0) {
+    my ($self, $c) = @_;
+
+    error($c, "Logging in via OIDC is not enabled.", 404) unless $c->config->{enable_oidc_login};


Why not scope these? Something like $c>config->{oidc}->{enable}? Or even better $c->config->{oidc}->{client_id} which can be undef or defined and enable/disable oidc auth that way

dasJ · 2023-08-21T06:48:48Z

src/lib/Hydra/Controller/User.pm

+
+    doEmailLogin($self, $c, "oidc", $claims->{email}, $claims->{name} // undef, sprintf("oidc:$claims->{sub}"));
+
+    $c->res->redirect($c->uri_for($c->res->cookies->{'after_oidc'}));


Same issue as with the GitHub auth: Doesn't work. If you use the cookie (and not the session which I recommended in another comment), you need $c->req->cookies, not res

Ergh, you're right. I think it did work for some reason though.

dasJ · 2023-08-21T06:49:18Z

src/root/topbar.tt

@@ -143,6 +143,10 @@
        <a class="dropdown-item" href="/github-redirect?after=[% c.req.path %]">Sign in with GitHub</a>
        <div class="dropdown-divider"></div>
      [% END %]
+      [% IF c.config.enable_oidc_login %]
+        <a class="dropdown-item" href="/oidc-redirect?after=[% c.req.path %]">Sign in with OIDC</a>


Would also be nice to be able to change the name of the provider so it can be consistent in an organization.

arianvp · 2023-09-04T10:21:47Z

src/lib/Hydra/Controller/User.pm

+    $c->session->{access_token} = $token_response->access_token;
+    $c->session->{expires_at} = time() + $token_response->expires_in;
+    $c->session->{refresh_token} = $token_response->refresh_token;
+


We really ought to check the id_token and its signature here and make sure that the id_token is bound to the audience (client_id).. And then save the sub and compare it to the sub received in userinfo.

Otherwise we are vulnerable against token substitution attacks as the access token is not audience-bound.

NOTE: Due to the possibility of token substitution attacks, the UserInfo Response is not guaranteed to be about the End-User identified by the sub (subject) element of the ID Token. The sub Claim in the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match, the UserInfo Response values MUST NOT be used.

https://openid.net/specs/openid-connect-basic-1_0.html#UserInfo

We're not reusing the access_token long-term but still. we should use the id_token as the source of truth about the user identity. Not the userinfo response. the userinfo endpoint is there to augment the id_token given that you know and verified the sub. Not as a replacement for the id_token

lheckemann and others added 2 commits August 20, 2023 19:46

Implement generic OIDC-based authentication

5c3f1b9

Co-Authored-By: Maximilian Bosch <[email protected]>

Set convenient variables for db access in shellHook

9a92fef

dasJ requested changes Aug 21, 2023

View reviewed changes

arianvp reviewed Sep 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement generic OIDC-based authentication #1298

Implement generic OIDC-based authentication #1298

lheckemann commented Aug 20, 2023

dasJ left a comment

dasJ Aug 21, 2023

lheckemann Aug 22, 2023

dasJ Aug 21, 2023

lheckemann Aug 22, 2023

dasJ Aug 21, 2023

lheckemann Aug 22, 2023

dasJ Aug 21, 2023

lheckemann Aug 22, 2023

dasJ Aug 21, 2023

dasJ Aug 21, 2023

lheckemann Aug 22, 2023

dasJ Aug 21, 2023

arianvp Sep 4, 2023 •

edited

Loading


		my $after = "/" . $c->req->params->{after};

		$c->res->cookies->{'after_oidc'} = {

		error($c, "Email address must be verified.", 401) unless $claims->{email_verified};

		doEmailLogin($self, $c, "oidc", $claims->{email}, $claims->{name} // undef, sprintf("oidc:$claims->{sub}"));


		doEmailLogin($self, $c, "oidc", $claims->{email}, $claims->{name} // undef, sprintf("oidc:$claims->{sub}"));

		$c->res->redirect($c->uri_for($c->res->cookies->{'after_oidc'}));

Implement generic OIDC-based authentication #1298

Are you sure you want to change the base?

Implement generic OIDC-based authentication #1298

Conversation

lheckemann commented Aug 20, 2023

dasJ left a 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

arianvp Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

arianvp Sep 4, 2023 •

edited

Loading