feat: redirect to OIDC providers only once in registration flows

[Saancreed]

Removes the requirement to redirect to OIDC providers more than once in registration flows where Jsonnet mapper is unable to provide all required identity traits.

Related issue(s)

#2863

Checklist

• [x] I have read the contributing guidelines.
• [ ] I have referenced an issue containing the design document if my change introduces a new feature.
• [x] I am following the contributing code guidelines.
• [x] I have read the security policy.
• [x] 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.
• [x] I have added tests that prove my fix is effective or that my feature works.
• [ ] I have added or changed the documentation.

Further Comments

While the change itself is relatively small, I have no tests to verify it (yet!) and the way I chose to resolve it (by persisting received claims and ID/Access/Refresh tokens in internal context) may not be the best idea. On the other hand, this allows us to simply continue the flow as if we were already after the redirect and does not rely on clients setting proper login_hint or the provider even respecting this parameter which as far as I know is not guaranteed or could require the user to supply their credentials again.

This is pretty much my first piece of code I've written in Go and my first dive into Kratos' codebase so please be gentle. That said, I'm submitting this to provide some context for questions I'd like to have answered and with them out of they way, I hope I'll be able to get this PR in shape so one day it could be merged.

1. What kind of test should I add? Would something similar to OIDC linking tests for settings flow be okay or should I try to prepare some e2e scenarios as well?
2. Is using internal context to persist initial provider ID, tokens and claims acceptable? Should I persist more so it's harder to continue the flow with bogus data? Right now I'm verifying that provider ID stays the same although I'm not sure if this is actually needed.
3. I'm storing tokens encrypted but claims unencrypted, perhaps I should encrypt claims as well?
4. The process of storing data into internal context is done on a best effort basis and any errors are silently ignored, in which case the user will be redirected again as before. However, when reading this data any error is considered fatal and interrupts the flow if it's found to have previously been stored after first callback.

[Saancreed]

There is now an e2e test that verifies only one redirect to OIDC provider takes place. I wasn't sure what are the exact URLs I should be looking for so I'm intercepting everything that looks like such a redirect and it seems to work.

About the security impact, I can imagine an attacker hijacking a registration flow if its ID was somehow leaked but unless there is a session post-registration hook, this is no worse than hijacking registrations using other methods. But we will consider this more thoroughly and come back with a detailed analysis soon.

[codecov[bot]]

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (492808c) 78.41% compared to head (45353fa) 78.37%. Report is 1 commits behind head on master.

Files	Patch %	Lines
selfservice/strategy/oidc/strategy_registration.go	28.57%	19 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3416      +/-   ##
==========================================
- Coverage   78.41%   78.37%   -0.04%     
==========================================
  Files         344      344              
  Lines       23487    23515      +28     
==========================================
+ Hits        18417    18431      +14     
- Misses       3686     3701      +15     
+ Partials     1384     1383       -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jakubfijalkowski]

So, after giving it some thought (and observing the behavior), we think it should be safe already, provided that we haven't missed something obvious (or you asked for something different and we just completely misunderstood).

It basically boils down to leaked flow id . If it does not leak the system (native app or browser app) that initiates the flow, we are safe. Also, it won't be a problem if the session hook is not enabled (because the only thing that the hijacker can do is to complete the flow early, without being able to sign in because they can't do the OIDC flow). They might see some data from OIDC claims, but I would consider that only a minor problem (but I don't think this is possible either, so bear with me).

Let's also exclude MITM attacks. Here, the attacker might just wait and hijack the session cookie/token directly.

We also need to consider only the case when the user is taken back to registration interface to fix form errors (e.g. missing traits), otherwise the flow will end the moment first redirect concludes, which was not affected by this PR and the same security guarantees hold.

Let's consider how can we prevent the leaked flow id when the user is redirected back to app with some error.

Native

Here, we can't do much - it boils down to whether the native app keeps flow id safe.

Since flow id will be kept in memory (most of the time; I can't really see a reason why it would be kept anywhere else), it is harder to hijack than session token that will be stored more permanently. If attacker can get access to flow id, they can access the token that will be retrieved soon after.

Browser

Here, the registration flow is already bound to the anti-CSRF token that is HTTPOnly. If the anti-CSRF token is not present, Kratos will start a new, empty flow. Thus, the only case where this can be a problem is if the attacker can get hold on the cookie. And if they can, then they can probably wait and get access to the session cookie.

---

What do you think?

[David-Wobrock]

Hey 👋

This patch looks amazing, and we'd love for it to be merged. How can we help? 😁

[Saancreed]

Hey @David-Wobrock, I admit this PR was slightly abandoned because I started seeing some e2e test failures for reasons that I couldn't explain and never had enough time to figure this out. I rebased my changes and pushed updated version but I imagine the two major blockers now are:

1. Figure out what's going wrong in e2e tests, assuming there are still some issues with them.
2. Attract some attention from Ory again to get some feedback on the writeup above.