feat: integration with Hydra #2549

grantzvolsky · 2022-06-29T00:31:49Z

This PR adds the configuration option selfservice.hydra_admin_url. Setting this value to a Hydra admin endpoint URL turns Kratos into an OIDC identity provider. Resolves #273

Related issue(s)

#273

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change introduces a new feature.
I am following the contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I confirm that I got green light (please contact
[email protected]) from the maintainers to push the changes.
I have added tests that prove my fix is effective or that my feature works.
I have added or changed the documentation.

Further Comments

paulbalomiri · 2022-07-07T14:56:27Z

Hi @grantzvolsky I'd like to give this a shot, against better advice from @vinckr. Which field is the login_challenge registration/login parameter taken from? I'm using dynamic registration, so I do have a full OIDC client registered with hydra, but without authentication.

grantzvolsky · 2022-07-11T15:44:51Z

Hi @paulbalomiri,

I would recommend waiting until this is reviewed and merged. There are a couple of things that still need to be addressed in this PR, like consent remember, making sure that all e2e tests pass, and other things.

To answer your question: when your app redirects the user agent to Hydra, Hydra generates a login_challenge and redirects to the login / consent UI with this challenge as a query parameter. Your login / consent UI is responsible for taking this parameter and passing it to Kratos. A very basic example, for testing purposes only, is here[1}.

[1}: https://github.com/ory/kratos/pull/2549/files#diff-7b487be0ee3e241929c9bd2006b53fa9fb3d7e550b822796ae5382cc429a98d1R77

DzianisH · 2022-07-14T10:31:19Z

Hi @grantzvolsky

Can I help you somehow to speed things up?

I wanna play around with Hydra and Kratos in my pet project. And it looks like it's doesn't make sense to implement any glue code for Hydra-Kratos integration as soon as native integration is almost there.

aeneasr · 2022-07-14T15:01:11Z

Hi, @grantzvolsky is currently busy with other priorities and will return to this topic once those are resolved! I know that this topic is urgent for many, but we can't clone him unfortunately ;)

atreya2011 · 2022-07-23T11:58:22Z

@grantzvolsky @aeneasr
Anyway I can help with this PR? 🙏🏼
It’s not urgent or anything but I would to help out in any way I can 🙇🏼

codecov · 2022-08-14T19:03:45Z

Codecov Report

Merging #2549 (cb55ccf) into master (84ea0cf) will decrease coverage by 0.05%.
The diff coverage is 73.44%.

❗ Current head cb55ccf differs from pull request most recent head d904106. Consider uploading reports for the commit d904106 to get more accurate results

@@            Coverage Diff             @@
##           master    #2549      +/-   ##
==========================================
- Coverage   75.84%   75.79%   -0.06%     
==========================================
  Files         302      304       +2     
  Lines       17796    17976     +180     
==========================================
+ Hits        13498    13625     +127     
- Misses       3264     3300      +36     
- Partials     1034     1051      +17

Impacted Files	Coverage Δ
selfservice/strategy/code/sender.go	`80.76% <0.00%> (ø)`
session/manager_http.go	`73.18% <0.00%> (ø)`
hydra/fake.go	`44.44% <44.44%> (ø)`
courier/email_templates.go	`65.95% <60.00%> (ø)`
driver/config/config.go	`82.80% <63.63%> (-0.33%)`	⬇️
selfservice/flow/login/handler.go	`78.60% <64.81%> (-2.65%)`	⬇️
selfservice/flow/login/hook.go	`84.21% <65.00%> (-3.29%)`	⬇️
selfservice/flow/registration/hook.go	`79.13% <71.42%> (-0.50%)`	⬇️
hydra/hydra.go	`78.57% <78.57%> (ø)`
selfservice/flow/registration/handler.go	`80.39% <86.95%> (+1.00%)`	⬆️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

persistence/sql/migratest/fixtures/registration_flow/ef18b06e-4700-4021-9949-ef783cd86be8.json

persistence/sql/migratest/fixtures/login_flow/349c945a-60f8-436a-a301-7a42c92604f9.json

persistence/sql/migrations/sql/20220607000001000000_hydra_login_challenge.sqlite3.up.sql

selfservice/flow/login/flow.go

internal/hydraclient/hydraclient.go

aeneasr · 2022-09-14T08:35:00Z

selfservice/flow/login/handler.go

+			cr, err := hydraclient.AcceptHydraLoginRequest(
+				hydra_admin_url.String(),
+				hlc,
+				sess.IdentityID.String(),
+				h.d.Config().SessionPersistentCookie(r.Context()),
+				int64(h.d.Config().SessionLifespan(r.Context())/time.Second),
+				sess.AMR,
+			)


Unless Ory Hydra tells us that it is ok to skip the login request:

lr, err := hydraclient.GetHydraLoginRequest(hydra_admin_url, hlc) if err != nil { // ... } if lr.Skip { // ... } else { // ... }

we should not auto-accept the login. Because it is possible that Ory Hydra is demanding us to show the login screen.

See this code here: https://github.com/ory/hydra-login-consent-node/blob/1496070c9bc5ddcefa327a7311d288c0ede60dcc/src/routes/login.ts#L26-L43

An important question here too is that the reference implementation uses the subject ( https://github.com/ory/hydra-login-consent-node/blob/1496070c9bc5ddcefa327a7311d288c0ede60dcc/src/routes/login.ts#L41 ) which is coming from Ory Hydra, not the own session store.

I think that Ory Hydra has a protection in place which would prevent us from Ory Hydra expecting one subject, and Ory Kratos sending another. But we should double-check that!

Long story short:

You need to fetch the login request

If skip is true, you can auto-accept it with the following requirements:

No session exists yet -> the subject value from Ory Hydra's login request response is used. See example

A session exists and kratos.identity.id == hydra.subject -> the identity id from the Ory Kratos session is used

A session exists and kratos.identity.id != hydra.subject:

Either send kratos.identity.id to Ory Hydra, which most likely will end up in an error or;

Invalidate the session and continue to the login form

If skip is false you need to:

If a session exists, log the user out, and perform a fresh log in

If a session does not exist, continue to the login form

Thank you for your insightful review!

Regarding the login logic, this is how I implemented it in the update that I just submitted:

If skip is true, you can auto-accept it with the following requirements:
i. No session exists yet -> the subject value from Ory Hydra's login request response is used. See example

I don't think this is ever the case, because we handle the Hydra challenge either if AlreadyLoggedIn (session exists) or in the post-login hook (session exists; it has just been created).

2.i and 2.ii

I believe the implementation satisfies both scenarios by always using kratos.identity.id. I just need to add a test for the error when there's a mismatch (will do over the weekend).

If skip is false you need to:
i. If a session exists, log the user out, and perform a fresh log in

I implemented this by adding a refresh=true to the login request initialization. This should have the added benefit that it won't "sign the user out of github when they don't complete a circleci login".

Regarding registration, no changes were necessary because the user always enters a password during registration, and the login challenge is handled in the post-registration hook.

Please let me know what you think :)

aeneasr

Please check the one long comment on login/handler.go

The same applies to the registration flow! Here too we need to respect the "skip" parameter

selfservice/flow/login/handler.go

selfservice/flow/registration/flow.go

internal/hydraclient/hydraclient.go

session/manager.go

aeneasr · 2022-09-14T08:42:28Z

selfservice/flow/login/handler.go

@@ -461,6 +514,26 @@
 		return
 	}

+	if ar.HydraLoginChallenge != uuid.Nil {


Please add tests for this new functionality in Go code.

aeneasr · 2022-09-14T08:42:33Z

selfservice/flow/login/handler.go

 	if errors.Is(err, ErrAlreadyLoggedIn) {
+		if r.URL.Query().Has("login_challenge") {


Please add tests for this new functionality in Go code.

aeneasr · 2022-09-14T08:42:39Z

selfservice/flow/registration/handler.go

@@ -354,6 +357,26 @@
 		return
 	}

+	if ar.HydraLoginChallenge != uuid.Nil {


Please add tests for this new functionality in Go code.

aeneasr · 2022-09-14T08:42:51Z

selfservice/flow/registration/hook.go

@@ -202,6 +205,25 @@
 		return nil
 	}

+	if a.HydraLoginChallenge != uuid.Nil {


Please add tests for this new functionality in Go code.

aeneasr · 2022-09-14T08:45:28Z

test/e2e/cypress/integration/profiles/oidc-provider/login.spec.ts

+import * as uuid from 'uuid'
+import * as oauth2 from '../../../helpers/oauth2'
+
+context('OpenID Provider', () => {


Good set up in the tests :)

selfservice/flow/login/handler.go

jpollard-cs · 2022-10-04T12:53:40Z

@grantzvolsky I don't want to step on your toes here, but would you find it helpful if I fixed some of the breaking tests or are there more breaking changes to come such that it doesn't make sense to do that yet?

grantzvolsky · 2022-10-04T18:46:31Z

@jpollard-cs Do you mean the e2e tests? They should pass as soon as ory/kratos-selfservice-ui-node#198 is merged.

Regarding the tests that Aeneas requested to be added, the only tests that are missing at the moment are those for the registration handler and registration hook, and I expect to add those tomorrow.

jpollard-cs · 2022-10-05T01:48:19Z

ory/kratos-selfservice-ui-node#198

amazing! let me know if there's any other way I can help

grantzvolsky · 2022-10-06T02:15:56Z

selfservice/strategy/webauthn/login_test.go

@@ -414,6 +414,7 @@ func TestCompleteLogin(t *testing.T) {

 		t.Run("case=succeeds with passwordless login", func(t *testing.T) {
 			run := func(t *testing.T, spa bool) {
+				conf.MustSet(ctx, config.ViperKeySessionWhoAmIAAL, "aal1")


@aeneasr Please take a look at why this test passes in master without this configured.

aeneasr

I've left a few comments on things I noticed while going through the code.

I'll now take a look at the integration e2e!

hydra/hydra.go

selfservice/flow/login/handler.go

selfservice/flow/login/hook.go

selfservice/flow/registration/handler.go

aeneasr · 2022-10-07T07:00:28Z

selfservice/flow/registration/handler.go

@@ -354,6 +366,16 @@ func (h *Handler) fetchFlow(w http.ResponseWriter, r *http.Request, ps httproute
 		return
 	}

+	if ar.HydraLoginChallenge.Valid {


Please add tests for this in handler_test.go

If you don't want to boot hydra, you could also use a gomock implementation of the hydra client

selfservice/flow/login/handler.go

aeneasr · 2022-10-07T07:04:46Z

selfservice/strategy/password/op_login_test.go

@@ -0,0 +1,356 @@
+package password_test


Maybe move this file to the hydra directory?

test/e2e/hydra-kratos-login-consent/main.go

test/e2e/cypress/integration/profiles/oidc-provider/login.spec.ts

test/e2e/cypress/integration/profiles/oidc-provider/mfa.spec.ts

This commit removes the --e2e cypress option because it is broken in the current release. There is an unreleased fix[1], so we may re-add it later, but it isn't necessary because --e2e is the default. [1]: cypress-io/cypress#22829

When using Kratos with Hydra as an OIDC IDP, the Kratos options * session.cookie.persistent and * session.lifespan now also apply to third party login.

mitar · 2022-10-26T08:11:55Z

This is made obsolete by #2804?

aeneasr · 2022-10-26T08:23:50Z

yes

DzianisH · 2022-10-27T00:06:26Z

@grantzvolsky you are my hero, thank you =)

grantzvolsky mentioned this pull request Jun 29, 2022

feat: kratos-hydra integration grantzvolsky/kratos#1

Closed

aeneasr mentioned this pull request Jul 5, 2022

Make Hydra integration compatible with kratos-0.7.6, hydra-1.10.6 ory/kratos-selfservice-ui-node#149

Closed

5 tasks

atreya2011 mentioned this pull request Jul 23, 2022

NextAuth Support #2179

Closed

6 tasks

grantzvolsky force-pushed the feature/kratos-hydra-integration branch from 8fc0546 to 2fb5c3c Compare August 14, 2022 03:12

grantzvolsky force-pushed the feature/kratos-hydra-integration branch from 9e3c934 to ef8859b Compare September 1, 2022 22:21

grantzvolsky force-pushed the feature/kratos-hydra-integration branch 2 times, most recently from a735d5b to 4267949 Compare September 13, 2022 03:53

grantzvolsky marked this pull request as ready for review September 13, 2022 04:00

grantzvolsky requested review from aeneasr and zepatrik as code owners September 13, 2022 04:00

aeneasr requested changes Sep 14, 2022

View reviewed changes

aeneasr reviewed Sep 14, 2022

View reviewed changes

aeneasr force-pushed the feature/kratos-hydra-integration branch from 473a6a6 to 0cce52f Compare September 14, 2022 08:44

aeneasr reviewed Sep 14, 2022

View reviewed changes

github-advanced-security bot found potential problems Sep 14, 2022

View reviewed changes

selfservice/flow/login/handler.go Fixed Show fixed Hide fixed

aeneasr force-pushed the feature/kratos-hydra-integration branch from 0cce52f to 82bb054 Compare September 14, 2022 08:58

grantzvolsky force-pushed the feature/kratos-hydra-integration branch from 5245cc2 to 7ce1a88 Compare October 2, 2022 22:31

grantzvolsky commented Oct 6, 2022

View reviewed changes

aeneasr reviewed Oct 7, 2022

View reviewed changes

test/e2e/cypress/integration/profiles/oidc-provider/login.spec.ts Outdated Show resolved Hide resolved

test/e2e/cypress/integration/profiles/oidc-provider/mfa.spec.ts Outdated Show resolved Hide resolved

grantzvolsky force-pushed the feature/kratos-hydra-integration branch from 38f4e8f to ec3befb Compare October 13, 2022 02:32

grantzvolsky and others added 18 commits October 13, 2022 09:38

feat: integration with Hydra

dfcc0fd

test(kratos-hydra): fix mysql migration; fix e2e test

59ddec1

chore: format

76f4f47

code review: fix migration names

768e263

code review(kratos-hydra): pass remember, remember_for, and amr to Hydra

c646d24

When using Kratos with Hydra as an OIDC IDP, the Kratos options * session.cookie.persistent and * session.lifespan now also apply to third party login.

autogen(openapi): regenerate swagger spec and internal client

0118bcb

fix: remove debug info from error and trace errors

1f2f650

code review(oidc-provider): respect login skip attribute

3c259c9

chore: format

ff0608d

test: kratos-hydra session subject mismatch

cd4dc54

fix: require aal2 when check fails

3723f1a

test: ensure login_challenge is passed to redirect_to

05a4ce4

chore: update x/sys for M1

a265284

code review: trigger logout if signed-in user registers in OP flow

7fb38f8

code review: cleanup

4804b9f

code review: simplify login handler logic; add fake Hydra tests

45785ed

chore: fix linter errors

78ddde8

grantzvolsky force-pushed the feature/kratos-hydra-integration branch from 195815e to 78ddde8 Compare October 13, 2022 09:40

grantzvolsky added 2 commits October 13, 2022 10:38

chore: resolve merge conflicts

bb118cb

chore: resolve merge conflicts

3252529

grantzvolsky force-pushed the feature/kratos-hydra-integration branch from 7d7fb95 to 3252529 Compare October 13, 2022 11:07

grantzvolsky added 3 commits October 13, 2022 11:16

chore: update deprecated gh action

8f05d28

fix(ci): allow private ip ranges in Hydra e2e tests

c691127

fix(ci): allow private ip ranges in Hydra e2e tests

d904106

aeneasr closed this Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: integration with Hydra #2549

feat: integration with Hydra #2549

grantzvolsky commented Jun 29, 2022

paulbalomiri commented Jul 7, 2022

grantzvolsky commented Jul 11, 2022 •

edited

Loading

DzianisH commented Jul 14, 2022

aeneasr commented Jul 14, 2022

atreya2011 commented Jul 23, 2022

codecov bot commented Aug 14, 2022 •

edited

Loading

aeneasr Sep 14, 2022

grantzvolsky Sep 30, 2022

aeneasr left a comment

aeneasr Sep 14, 2022

aeneasr Sep 14, 2022

aeneasr Sep 14, 2022

aeneasr Sep 14, 2022

aeneasr Sep 14, 2022

jpollard-cs commented Oct 4, 2022 •

edited

Loading

grantzvolsky commented Oct 4, 2022

jpollard-cs commented Oct 5, 2022

grantzvolsky Oct 6, 2022

aeneasr left a comment

aeneasr Oct 7, 2022

aeneasr Oct 7, 2022

mitar commented Oct 26, 2022

aeneasr commented Oct 26, 2022

DzianisH commented Oct 27, 2022

		if errors.Is(err, ErrAlreadyLoggedIn) {
		if r.URL.Query().Has("login_challenge") {

feat: integration with Hydra #2549

feat: integration with Hydra #2549

Conversation

grantzvolsky commented Jun 29, 2022

Related issue(s)

Checklist

Further Comments

paulbalomiri commented Jul 7, 2022

grantzvolsky commented Jul 11, 2022 • edited Loading

DzianisH commented Jul 14, 2022

aeneasr commented Jul 14, 2022

atreya2011 commented Jul 23, 2022

codecov bot commented Aug 14, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr 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

jpollard-cs commented Oct 4, 2022 • edited Loading

grantzvolsky commented Oct 4, 2022

jpollard-cs commented Oct 5, 2022

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitar commented Oct 26, 2022

aeneasr commented Oct 26, 2022

DzianisH commented Oct 27, 2022

grantzvolsky commented Jul 11, 2022 •

edited

Loading

codecov bot commented Aug 14, 2022 •

edited

Loading

jpollard-cs commented Oct 4, 2022 •

edited

Loading