feat: support OIDC flows for apps #3216

hperl · 2023-04-06T07:23:56Z

Flow for Kratos AppAuth

Call createNativeLoginFlow to create a flow of type api, and request a session token exchange code
Kratos sends flow ID, the session token exchange code, and the SSO URL to the app
App opens the SSO URL in the native browser
Browser opens the SSO URL, the authorization endpoint takes the user through authentication and consent
The authorization endpoint redirects to the callback url (for example https://nostalgic-cori-rxgved3nrs.projects.oryapis.com/self-service/methods/oidc/callback/google), containing the authorization code
The browser opens that URL. Kratos exchanges the authorization code for an identity token, and invokes the redirect (through the post-login redirect) to an URL registered to be handled by the app (Android App links or iOS Universal links)
7a. Upon receiving the URL, the app can retrieve the session token at /self-service/exchange-code-for-session-token&code=<session token exchange code>
7b. If the app can't receive a deep link (for example, if we are simulating the app on the web), then the app can also poll /self-service/exchange-code-for-session-token&code=<session token exchange code> and wait for a 200 status code with the session.

Related issue(s)

Docs PR: ory/docs#1379

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 the approval (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.

Security Considerations

During the authentication process we need to ensure that the following entities are all under the same control:

A1: The app that started the flow
B1: The browser that performs the authentication with the OIDC provider
B2: The browser that performs the OIDC callback with Kratos
A2: The app that fetches the session token

For example, it should not be possible for an attacker to start a flow, trick a victim into performing the authentication with the OIDC provider, and then be able to fetch the session token in the name of the victim.

We ensure that A1, A2, B1, B2 are under the same control as follows:

The initial_session_token_exchange_code binds A1 and A2 by generating in when the flow starts and requiring it in exchange for the session token
The OIDC state parameter binds B1 and B2 by deriving the state from the flow ID and the hashed code, which guarantees that no attacker can forge the state for B2
The return_to_code binds B2 and A2 by redirecting the browser to the specified and whitelisted return_to URL and including the code in a query parameter.

It then follows that A1, A2, B1, B2 are all under the same control

codecov · 2023-04-11T09:55:19Z

Codecov Report

Merging #3216 (891e8aa) into master (7f232bf) will increase coverage by 0.03%.
The diff coverage is 75.85%.

❗ Current head 891e8aa differs from pull request most recent head 509ec60. Consider uploading reports for the commit 509ec60 to get more accurate results

@@            Coverage Diff             @@
##           master    #3216      +/-   ##
==========================================
+ Coverage   77.93%   77.96%   +0.03%     
==========================================
  Files         324      324              
  Lines       20732    20780      +48     
==========================================
+ Hits        16158    16202      +44     
+ Misses       3365     3363       -2     
- Partials     1209     1215       +6

Impacted Files	Coverage Δ
driver/registry.go	`44.82% <ø> (ø)`
selfservice/flow/flow.go	`100.00% <ø> (ø)`
selfservice/flow/login/error.go	`69.84% <0.00%> (ø)`
selfservice/flow/registration/error.go	`68.11% <0.00%> (ø)`
selfservice/strategy/oidc/error.go	`75.00% <ø> (ø)`
session/manager.go	`100.00% <ø> (ø)`
selfservice/flow/login/hook.go	`84.67% <25.00%> (-0.93%)`	⬇️
selfservice/flow/registration/hook.go	`71.73% <25.00%> (-0.93%)`	⬇️
selfservice/hook/session_issuer.go	`79.41% <25.00%> (+0.46%)`	⬆️
selfservice/strategy/oidc/strategy.go	`60.35% <40.00%> (-0.88%)`	⬇️
... and 14 more

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

...istence/sql/migrations/sql/20230405000000000001_create_session_token_exchangers.mysql.up.sql

persistence/sql/persister_sessiontokenexchanger.go

selfservice/flow/login/handler.go

selfservice/sessiontokenexchange/handler.go

.github/workflows/ci.yaml

aeneasr

Very nice & clean, I have a few remarks. One question is whether we should add some tests to the oidc go test suite. It's a bit of duplication because we test the same thing in e2e, but those tests execute faster and are easier to debug / step through if something breaks

persistence/sql/migrations/sql/20230405000000000001_create_session_token_exchanges.mysql.up.sql

persistence/sql/migrations/sql/20230405000000000001_create_session_token_exchanges.up.sql

selfservice/flow/login/handler.go

selfservice/hook/session_issuer.go

selfservice/strategy/oidc/strategy.go

test/e2e/playwright/tests/app_login.spec.ts

selfservice/strategy/oidc/strategy_login.go

selfservice/strategy/oidc/strategy.go

aeneasr

Very nice, a couple of questions and remarks on the code-side. I will look into the flow description next!

aeneasr · 2023-04-21T15:07:24Z

persistence/sql/persister_sessiontokenexchanger.go

+init_code = ? AND init_code <> '' AND
+return_to_code = ? AND return_to_code <> '' AND


Do we need the non-empty assertions when the parameters are provided?

Is this not missing an index? Have you checked whether querying this scans excessive rows?

Do we need the non-empty assertions when the parameters are provided?

This is only a precaution that we never fetch from empty credentials.

Is this not missing an index? Have you checked whether querying this scans excessive rows?

The index should be this: CREATE INDEX session_token_exchanges_nid_code_idx ON session_token_exchanges (init_code, nid);. I didn't verify this with EXPLAIN though.

But that's missing return_to_code, no?

I now verified through EXPLAIN that everything works as expected:

Fetch by flow ID

explain select * from session_token_exchanges where nid = '212e6a8b-8a8f-4e30-b1e2-99b8478d773b' and flow_id = '7cf7bca9-f94e-41a8-8b8c-cb213b8b9cd9' and init_code <> '' and return_to_code <> '' and session_id is not null;

Index Scan using session_token_exchanges_nid_flow_id_idx on session_token_exchanges (cost=0.14..8.17 rows=1 width=372) Index Cond: ((flow_id = '7cf7bca9-f94e-41a8-8b8c-cb213b8b9cd9'::uuid) AND (nid = '212e6a8b-8a8f-4e30-b1e2-99b8478d773b'::uuid)) Filter: ((session_id IS NOT NULL) AND ((init_code)::text <> ''::text) AND ((return_to_code)::text <> ''::text))

Fetch by code

explain select * from session_token_exchanges where nid = '212e6a8b-8a8f-4e30-b1e2-99b8478d773b' and init_code = '5JxAMjfYsOCQNwsEyPoXfbC3u0Dbqb6TyGoB7z9now0BPNaJt4itcqecYr9YSR4t' and init_code <> '' and return_to_code = '3bkURN1fYEN2PtfvdE3juvUHC6XAzmcQufvHfbQ8gWSRa85j8kpvnyBh2gvmI9q7' and return_to_code <> '' and session_id is not null;

Index Scan using session_token_exchanges_nid_code_idx on session_token_exchanges (cost=0.14..8.17 rows=1 width=372) Index Cond: (((init_code)::text = '5JxAMjfYsOCQNwsEyPoXfbC3u0Dbqb6TyGoB7z9now0BPNaJt4itcqecYr9YSR4t'::text) AND (nid = '212e6a8b-8a8f-4e30-b1e2-99b8478d773b'::uuid)) Filter: ((session_id IS NOT NULL) AND ((return_to_code)::text <> ''::text) AND ((return_to_code)::text = '3bkURN1fYEN2PtfvdE3juvUHC6XAzmcQufvHfbQ8gWSRa85j8kpvnyBh2gvmI9q7'::text))

In both cases, the index is used first to narrow down the rows. Since (nid, flow_id) and (nid, init_code) both narrow down the row to a single result, the remaining filter is very fast. Adding more fields to the indices would not improve performance.

persistence/sql/persister_sessiontokenexchanger.go

aeneasr · 2023-04-21T15:10:06Z

selfservice/flow/login/handler.go

+	if ft == flow.TypeAPI && r.URL.Query().Get("return_session_token_exchange_code") == "true" {
+		e, err := h.d.SessionTokenExchangePersister().CreateSessionTokenExchanger(r.Context(), f.ID)
+		if err != nil {
+			return nil, nil, errors.WithStack(herodot.ErrInternalServerError.WithWrap(err))


Why not just return err - wrapping it here will probably lose some context?

As the user can't do anything about this we should return a 500 code. Is this always the case when we don't wrap? If so, I can remove the wrapping.

selfservice/flow/registration/handler.go

aeneasr · 2023-04-21T15:23:03Z

session/handler.go

+	InitCode string `json:"init_code"`
+
+	// The part of the code returned by the return_to URL.
+	//
+	// required: true
+	// in: query
+	ReturnToCode string `json:"return_to_code"`


In the future, this endpoint may support other token exchanges as well. Should we somehow prefix / clarify what code you're exchanging here?

I'm not sure what other usages you think of here. If there are other types of tokens, wouldn't this be a different end point? It would probably be easier to reason about the security then?

session/handler.go

aeneasr · 2023-04-21T15:27:42Z

session/handler.go

+//	  404: errorGeneric
+//	  410: errorGeneric
+//	  default: errorGeneric
+func (h *Handler) exchangeCode(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {


Go tests for this? :)

Here: https://github.com/ory/kratos/pull/3216/files/cfbc327b89911795a62b07fc43a704fe6f943152#diff-03d9986942bad676c4567865d479f219a54bb633d7ccc9eed5820703e3d296a3R180-R194

session/manager_http.go

selfservice/strategy/oidc/strategy.go

aeneasr · 2023-04-23T21:01:27Z

I wanted to execute the tests to check the flow end-to-end, but I was not able to do so, because of:

% npm run playwright

> @ory/[email protected] playwright
> playwright test

ReferenceError: Cypress is not defined

   at cypress/helpers/index.ts:43

  41 |   new DOMParser().parseFromString(html, "text/html")

How can I execute the tests?

Additionally I found the code in the react app a bit confusing. I kind of would have expected the code to be more obvious, but it's hiding behind several nested function calls that are relatively deep in the stack trace.

This made it difficult to do a final end-to-end review. It would be really helpful if there was a simple Makefile command (or something similar) to get the playwright tests started!

hperl · 2023-04-24T09:51:31Z

[...]

Additionally I found the code in the react app a bit confusing. I kind of would have expected the code to be more obvious, but it's hiding behind several nested function calls that are relatively deep in the stack trace.

The entry point to the OIDC handling in the native app is the 422 error returned by Kratos. @jonas-jonas told me that you discussed this with him as a means to signal to the native app that it needs to get a browser involved. If you have ideas to improve the code lets meet here: ory/kratos-selfservice-ui-react-native#69

This made it difficult to do a final end-to-end review. It would be really helpful if there was a simple Makefile command (or something similar) to get the playwright tests started!

I added a make target. You can now run the Playwright tests with

RN_UI_PATH="path/to/kratos-selfservice-ui-react-native" make test-e2e-playwright

(env var needed until ory/kratos-selfservice-ui-react-native#69 is merged)

Maragues · 2023-04-24T16:13:58Z

The entry point to the OIDC handling in the native app is the 422 error returned by Kratos.

Sorry to show out of the blue, but from client perspective a 422 forces us to handle things in a weird way. I'm using this PR to implement our OIDC login, but since there's no java client yet, we are doing raw calls with retrofit.

Given this definition

@Headers("Content-Type: application/json")
@POST("self-service/login")
fun getSocialSignInUrl(@Query("flow") flowId: String, @Body body: SocialSignInBody): Call<SocialSignInUrlResponse>

It's normally straightforward to get the response body through

response.body()

But because you are returning a 422 error code, Retrofit doesn't attempt to parse the body and encapsulates everything on errorBody, which forces me to parse it manually

return when (response.code()) {
                422 -> {
                    val errorBody = response.errorBody().string()

                    RetrofitClientFactory.gson.fromJson(errorBody, SocialSignInUrlResponse::class.java)
                }

                else -> throw ...
}

This won't affect your java client because you parse everything manually, but my two cents as an android developer.

I hope this gives you more information and maybe you reconsider using an error code on a successful response.

aeneasr · 2023-04-25T01:23:49Z

Some of the tests are failing for me:

Fruthermore, as far as I understand we now have two codes:

One code is returned by createNativeFlow
Second code is returned as part of return URL

We need both codes to complete the flow - is that correct?

hperl · 2023-04-25T06:54:48Z

Fruthermore, as far as I understand we now have two codes:

One code is returned by createNativeFlow
Second code is returned as part of return URL
We need both codes to complete the flow - is that correct?

Yes that's correct.

jonas-jonas

Code-wise this looks great to me. E2e just straight up fail for me locally. But probably an issue on my machine.

selfservice/flow/registration/handler.go

jonas-jonas · 2023-04-25T07:10:37Z

spec/api.json

is the response code for the registration flow handler missing here?

Your comment lost it's link to the source code. What is the issue here?

I was just wondering whether the (current) 422 response from the registration handler is documented in the open api doc.

test/e2e/playwright/kratos.config.json

test/e2e/playwright/tests/app_login.spec.ts

hperl requested review from aeneasr and zepatrik as code owners April 6, 2023 07:23

hperl self-assigned this Apr 6, 2023

hperl marked this pull request as draft April 6, 2023 07:24

hperl changed the title ~~feat: OIDC flows for apps~~ feat: support OIDC flows for apps Apr 6, 2023

hperl force-pushed the hperl/app-auth branch from 568d616 to 7bccac0 Compare April 11, 2023 10:49

jonas-jonas mentioned this pull request Apr 12, 2023

feat: support OIDC on API flows #3176

Closed

7 tasks

aeneasr reviewed Apr 13, 2023

View reviewed changes

hperl force-pushed the hperl/app-auth branch 4 times, most recently from 26ebe49 to 1e5f9ff Compare April 18, 2023 13:01

hperl commented Apr 18, 2023

View reviewed changes

.github/workflows/ci.yaml Show resolved Hide resolved

feat: support app-based OIDC flows

a80e7d7

hperl force-pushed the hperl/app-auth branch from 1e5f9ff to a80e7d7 Compare April 18, 2023 13:07

hperl requested review from aeneasr and jonas-jonas April 18, 2023 13:09

hperl marked this pull request as ready for review April 18, 2023 13:09

hperl mentioned this pull request Apr 18, 2023

feat: support OAuth2 and OpenID Connect in API-based flows #2346

Closed

2 tasks

aeneasr reviewed Apr 18, 2023

View reviewed changes

hperl added 3 commits April 19, 2023 09:10

fixup: address review comments, fix tests

b4853a6

fix indices

db6e385

add update limit clause

737342a

hperl commented Apr 19, 2023

View reviewed changes

selfservice/strategy/oidc/strategy.go Outdated Show resolved Hide resolved

hperl added 2 commits April 19, 2023 10:33

make sdk

e63bc32

test: add OIDC strategy test

44ff8ea

hperl force-pushed the hperl/app-auth branch from 1ffdbb0 to 44ff8ea Compare April 19, 2023 12:26

hperl requested review from aeneasr and alnr April 19, 2023 12:31

hperl added 4 commits April 19, 2023 18:35

fix state

c733110

move session token exchange point

c5af643

Merge remote-tracking branch 'origin/master' into hperl/app-auth

5c77dcc

add return_to code

cfbc327

aeneasr reviewed Apr 21, 2023

View reviewed changes

address review comments

a5188a0

hperl requested a review from aeneasr April 21, 2023 18:22

Add Playwright Make target

2d82176

add kratos.config.json for playwright

6aca4e0

de-paralellize playwright

071e762

jonas-jonas reviewed Apr 25, 2023

View reviewed changes

hperl added 2 commits April 25, 2023 12:56

add base kratos config

513d4aa

address review comments

509ec60

aeneasr approved these changes Apr 26, 2023

View reviewed changes

aeneasr merged commit cb10609 into master Apr 26, 2023

aeneasr deleted the hperl/app-auth branch April 26, 2023 13:09

MortalKastor mentioned this pull request Jun 12, 2023

Support OIDC in API-based settings flow #3311

Closed

6 tasks

jeremy-serenne mentioned this pull request Jun 19, 2023

Support more audiences #3331

Closed

6 tasks

Jorgagu mentioned this pull request Feb 19, 2024

feat: use hydra tokens to perform kratos flows #3764

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support OIDC flows for apps #3216

feat: support OIDC flows for apps #3216

hperl commented Apr 6, 2023 •

edited

Loading

codecov bot commented Apr 11, 2023 •

edited

Loading

aeneasr left a comment

aeneasr left a comment

aeneasr Apr 21, 2023

hperl Apr 21, 2023

aeneasr Apr 23, 2023

hperl Apr 24, 2023 •

edited

Loading

aeneasr Apr 21, 2023

hperl Apr 21, 2023

aeneasr Apr 21, 2023

hperl Apr 21, 2023

aeneasr Apr 21, 2023

hperl Apr 21, 2023

aeneasr commented Apr 23, 2023 •

edited

Loading

hperl commented Apr 24, 2023

Maragues commented Apr 24, 2023

aeneasr commented Apr 25, 2023

hperl commented Apr 25, 2023

jonas-jonas left a comment

jonas-jonas Apr 25, 2023

hperl Apr 25, 2023

jonas-jonas Apr 25, 2023

		init_code = ? AND init_code <> '' AND
		return_to_code = ? AND return_to_code <> '' AND

feat: support OIDC flows for apps #3216

feat: support OIDC flows for apps #3216

Conversation

hperl commented Apr 6, 2023 • edited Loading

Flow for Kratos AppAuth

Related issue(s)

Checklist

Security Considerations

codecov bot commented Apr 11, 2023 • edited Loading

Codecov Report

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

hperl Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

Fetch by flow ID

Fetch by code

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

aeneasr commented Apr 23, 2023 • edited Loading

hperl commented Apr 24, 2023

Maragues commented Apr 24, 2023

aeneasr commented Apr 25, 2023

hperl commented Apr 25, 2023

jonas-jonas 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

hperl commented Apr 6, 2023 •

edited

Loading

codecov bot commented Apr 11, 2023 •

edited

Loading

hperl Apr 24, 2023 •

edited

Loading

aeneasr commented Apr 23, 2023 •

edited

Loading