feat: support OIDC on API flows #3176
Conversation
jonas-jonas
force-pushed
the
jonas-jonas/feat/oidcOnNative
branch
from
9f1b620 to
23dce4b
Compare
Codecov Report
@@ Coverage Diff @@
## master #3176 +/- ##
==========================================
- Coverage 77.95% 77.52% -0.43%
==========================================
Files 320 319 -1
Lines 20428 20336 -92
==========================================
- Hits 15924 15766 -158
- Misses 3301 3354 +53
- Partials 1203 1216 +13
... and 18 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
jonas-jonas
force-pushed
the
jonas-jonas/feat/oidcOnNative
branch
from
23dce4b to
af08165
Compare
| @@ -38,3 +41,18 @@ type Flow interface { | |||
| AppendTo(*url.URL) *url.URL | |||
| GetUI() *container.Container | |||
| } | |||
|
|
|||
| func IsWebViewFlow(ctx context.Context, conf *config.Config, f Flow) (bool, error) { | |||
There was a problem hiding this comment.
From a security perspective, any client can become a WebView by adding the correct return_to parameter, even a malicious JS script.
| w.Header().Set("Content-Type", "application/json") | ||
| returnTo.Path = path.Join(returnTo.Path, "success") | ||
| query := returnTo.Query() | ||
| query.Set("session_token", s.Token) |
There was a problem hiding this comment.
Sending the session token in the clear degrades security, especially when the IsWebViewFlow can be faked by using the webview return_to parameter when creating the flow.
| // Only used in API-type flows, when an id token has been received by mobile app directly from oidc provider. | ||
| // | ||
| // required: false | ||
| IDToken string `json:"id_token"` |
There was a problem hiding this comment.
What are the security implications when we get the ID token from the user instead of the OIDC provider?
There was a problem hiding this comment.
If I recall correctly, that should be fine. The ID token will be validated on our end and we will check both issuer and, more importantly, audience. By checking audience, we ensure that the ID token was issued on our behalf, and not someone elses behalf. That way we avoid the vulnerability that could be caused by instead accepting access tokens as proof of authentication, as access tokens don't have an audience and they could have been issued for any app.
Still, this allows a eve to trick adam into using eve's ID token. Would be good to know how others deal with that?
hperl
force-pushed
the
jonas-jonas/feat/oidcOnNative
branch
from
af08165 to
d51ba70
Compare
|
Superseded by #3216 |
Based on changes in #2346
Related issue(s)
Related to https://github.com/ory-corp/cloud/issues/3496
Checklist
introduces a new feature.
contributing code guidelines.
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.
works.
Further Comments
The
id_tokenflow looks as followssequenceDiagram participant Phone participant Kratos participant OIDC as OIDC Provider Phone->>+Kratos: Initialize API registration flow Kratos-->>-Phone: Return flow incl. UI/OIDC Nodes Phone->>+OIDC: Request id_token OIDC-->>Phone: Obtains consent from user OIDC-->>-Phone: Returns id_token Phone->>+Kratos: updateRegistrationFlow w/ id_token Kratos->>Kratos: Creates identity from data in id_token using JSONNet mapper from config Kratos-->>-Phone: Returns session (if session hook is set)