Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement generic OIDC-based authentication #1298

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Implement generic OIDC-based authentication #1298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c3f1b9
Implement generic OIDC-based authentication
lheckemann Feb 4, 2022
9a92fef
Set convenient variables for db access in shellHook
lheckemann Aug 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 5 additions & 18 deletions flake.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,24 +42,7 @@
overlays.default = final: prev: {

# Add LDAP dependencies that aren't currently found within nixpkgs.
perlPackages = prev.perlPackages // {

PrometheusTiny = final.perlPackages.buildPerlPackage {
pname = "Prometheus-Tiny";
version = "0.007";
src = final.fetchurl {
url = "mirror://cpan/authors/id/R/RO/ROBN/Prometheus-Tiny-0.007.tar.gz";
sha256 = "0ef8b226a2025cdde4df80129dd319aa29e884e653c17dc96f4823d985c028ec";
};
buildInputs = with final.perlPackages; [ HTTPMessage Plack TestException ];
meta = {
homepage = "https://github.com/robn/Prometheus-Tiny";
description = "A tiny Prometheus client";
license = with final.lib.licenses; [ artistic1 gpl1Plus ];
};
};

};
perlPackages = prev.perlPackages // import ./perl-packages.nix prev;

hydra = with final; let
perlDeps = buildEnv {
Expand Down Expand Up @@ -112,6 +95,7 @@
NetAmazonS3
NetPrometheus
NetStatsd
OIDCLite
PadWalker
ParallelForkManager
PerlCriticCommunity
Expand Down Expand Up @@ -214,6 +198,9 @@
mkdir -p .hydra-data
export HYDRA_DATA="$(pwd)/.hydra-data"
export HYDRA_DBI='dbi:Pg:dbname=hydra;host=localhost;port=64444'
export PGPORT=64444
export PGDATABASE=hydra
export PGHOST=localhost

popd >/dev/null
'';
Expand Down
77 changes: 77 additions & 0 deletions perl-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
prev:
with prev.perlPackages;
let inherit (prev) lib fetchurl;
in rec {
ClassErrorHandler = buildPerlPackage {
pname = "Class-ErrorHandler";
version = "0.04";
src = fetchurl {
url = "mirror://cpan/authors/id/T/TO/TOKUHIROM/Class-ErrorHandler-0.04.tar.gz";
sha256 = "342d2dcfc797a20bee8179b1b96b85c0ae7a5b48827359523cd8c74c3e704502";
};
meta = {
homepage = "https://github.com/tokuhirom/Class-ErrorHandler";
description = "Base class for error handling";
license = with lib.licenses; [ artistic1 gpl1Plus ];
};
};
OIDCLite = buildPerlModule {
pname = "OIDC-Lite";
version = "0.10";
src = fetchurl {
url = "mirror://cpan/authors/id/R/RI/RITOU/OIDC-Lite-0.10.tar.gz";
sha256 = "529096272a160d8cd947bec79e01b48639db93726432b4d93039a7507421245a";
};
buildInputs = [ CryptOpenSSLRSA TestMockLWPConditional TestMockObject ];
propagatedBuildInputs = [ ClassAccessor DataDump JSONWebToken JSONXS OAuthLite2 ParamsValidate ];
meta = {
homepage = "https://github.com/ritou/p5-oidc-lite";
description = "OpenID Connect Library";
license = with lib.licenses; [ artistic1 gpl1Plus ];
};
};
OAuthLite = buildPerlPackage {
pname = "OAuth-Lite";
version = "1.35";
src = fetchurl {
url = "mirror://cpan/authors/id/L/LY/LYOKATO/OAuth-Lite-1.35.tar.gz";
sha256 = "740528f8345bcb8849c1e3bfc91510b3c7f9df6255af09987d4175c1dea43c5e";
};
propagatedBuildInputs = [ ClassAccessor ClassDataAccessor ClassErrorHandler CryptOpenSSLRSA CryptOpenSSLRandom LWP ListMoreUtils UNIVERSALrequire URI ];
meta = {
description = "OAuth framework";
license = with lib.licenses; [ artistic1 gpl1Plus ];
};
};
TestMockLWPConditional = buildPerlModule {
pname = "Test-Mock-LWP-Conditional";
version = "0.04";
src = fetchurl {
url = "mirror://cpan/authors/id/M/MA/MASAKI/Test-Mock-LWP-Conditional-0.04.tar.gz";
sha256 = "8817129488f1eae4896aae59b8e09e94f720fdd697a73aef13241e8123940667";
};
buildInputs = [ ModuleBuildTiny TestFakeHTTPD TestUseAllModules TestTCP TestSharedFork ];
propagatedBuildInputs = [ ClassMethodModifiers LWP MathRandomSecure SubInstall ];
meta = {
homepage = "https://github.com/masaki/Test-Mock-LWP-Conditional";
description = "Stubbing on LWP request";
license = with lib.licenses; [ artistic1 gpl1Plus ];
};
};
OAuthLite2 = buildPerlModule {
pname = "OAuth-Lite2";
version = "0.11";
src = fetchurl {
url = "mirror://cpan/authors/id/R/RI/RITOU/OAuth-Lite2-0.11.tar.gz";
sha256 = "01417ec28acefd25a839bdb4b846056036ae122c181dab907e48e0bdb938686a";
};
buildInputs = [ ModuleBuildTiny ];
propagatedBuildInputs = [ ClassAccessor ClassErrorHandler DataDump HashMultiValue IOString JSONXS LWP ParamsValidate Plack StringRandom TryTiny URI XMLLibXML ];
meta = {
homepage = "https://github.com/ritou/p5-oauth-lite2";
description = "OAuth 2.0 Library";
license = with lib.licenses; [ artistic1 gpl1Plus ];
};
};

}
2 changes: 2 additions & 0 deletions src/lib/Hydra/Controller/Root.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ sub noLoginNeeded {
return $whitelisted ||
$c->request->path eq "api/push-github" ||
$c->request->path eq "google-login" ||
$c->request->path eq "oidc-login" ||
$c->request->path eq "oidc-redirect" ||
$c->request->path eq "github-redirect" ||
$c->request->path eq "github-login" ||
$c->request->path eq "login" ||
Expand Down
71 changes: 70 additions & 1 deletion src/lib/Hydra/Controller/User.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,26 @@ use Hydra::Config qw(getLDAPConfigAmbient);
use Hydra::Helper::Nix;
use Hydra::Helper::CatalystUtils;
use Hydra::Helper::Email;
use OIDC::Lite::Client::WebServer;
use LWP::UserAgent;
use JSON::MaybeXS;
use HTML::Entities;
use UUID4::Tiny;
use Encode qw(decode);


__PACKAGE__->config->{namespace} = '';

sub get_oidc_client {
my $c = shift;
return OIDC::Lite::Client::WebServer->new(
id => $c->config->{oidc_client_id},
secret => $c->config->{oidc_client_secret},
authorize_uri => $c->config->{oidc_auth_uri},
access_token_uri => $c->config->{oidc_token_uri},
);
}


sub login :Local :Args(0) :ActionClass('REST') { }

Expand Down Expand Up @@ -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\.\-\_]+$/;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.


$username = $email unless defined $username;

# If allowed_domains is set, check if the email address
# returned is on these domains. When not configured, allow all
# domains.
Expand Down Expand Up @@ -143,6 +157,38 @@ sub doEmailLogin {
}


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};
Copy link
Member

Choose a reason for hiding this comment

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

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

my $oidc_client = get_oidc_client($c);

error($c, q{OIDC state mismatch. Is this a CSRF attack?}) unless $c->session->{oidc_state} && $c->session->{oidc_state} eq $c->request->param("state");

my $code = $c->request->param("code");
my $token_response = $oidc_client->get_access_token(
code => $code,
redirect_uri => $c->uri_for('/oidc-login'),
) or error($c, $oidc_client->errstr);

$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;

Copy link
Member

@arianvp arianvp Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

my $res = LWP::UserAgent->new->get(
$c->config->{oidc_userinfo_uri},
Authorization => sprintf(q{Bearer %s}, $token_response->access_token)
);

my $claims = decode_json($res->decoded_content) or error($c, q{Could not decode claims.}, 401);

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}"));

Copy link
Member

Choose a reason for hiding this comment

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

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 });
+            }
+        }
+    }

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

sub google_login :Path('/google-login') Args(0) {
my ($self, $c) = @_;
requirePost($c);
Expand Down Expand Up @@ -226,6 +272,29 @@ sub github_redirect :Path('/github-redirect') Args(0) {
$c->res->redirect("https://github.com/login/oauth/authorize?client_id=$client_id&scope=user:email");
}

sub oidc_redirect :Path('/oidc-redirect') Args(0) {

my ($self, $c) = @_;

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

$c->res->cookies->{'after_oidc'} = {
Copy link
Member

Choose a reason for hiding this comment

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

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 👀)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

name => 'after_oidc',
value => $after,
};

my $oidc_client = get_oidc_client($c);
my $state = UUID4::Tiny::create_uuid_string();
$c->session->{oidc_state} = $state;
my $redirect_url = $oidc_client->uri_to_redirect(
redirect_uri => $c->uri_for('/oidc-login'),
scope => q{openid},
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have this configurable

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

state => $state,
);

$c->res->redirect( $redirect_url );
}


sub captcha :Local Args(0) {
my ($self, $c) = @_;
Expand Down
4 changes: 4 additions & 0 deletions src/root/topbar.tt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

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

<div class="dropdown-divider"></div>
[% END %]
<a class="dropdown-item" href="#hydra-signin" data-toggle="modal">Sign in with a Hydra account</a>
[% END %]
[% END %]
Expand Down
Loading