Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

Added requirement for redirect URIs to be the same hostname as client… #16

Closed
wants to merge 4 commits into from
Closed

Conversation

jaxoncreed
Copy link
Contributor

…_ids

the previous Dynamic Registration step. So, Alice's browser gets redirected
to `https://bob.example/auth-callback` (which is the URI Bob's POD uses as an OIDC
`redirect_uri` endpoint).
browser to a provided `redirect_uri` that `bob.example`. The `redirect_uri` MUST
Copy link

@TallTed TallTed Apr 25, 2019

Choose a reason for hiding this comment

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

to a provided `redirect_uri` that `bob.example`. The
-> to a `redirect_uri` provided by `bob.example`. The
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops sorry about that.

@RubenVerborgh
Copy link

Why? What problem do we address?

PS https://commit.style/ 😉

@jaxoncreed
Copy link
Contributor Author

@RubenVerborgh This is in reference to #12

Essentially instead of using the origin header in the to command access control over apps, we'll use the client_id from the provided bearer token. This is much more trustable than the origin header as the token can only be retrieved from a valid redirect back to the webpage and app is claiming to be.

@zenomt
Copy link

zenomt commented Apr 27, 2019

-1

this change requires that the client_id be the hostname of the redirect_uri, and that therefore the client_id's semantics can be examinable by a party other than the OP. this requirement is inappropriate for several reasons: client_ids MUST be unique for every new client registration (imagine a second user of a browser-app hosted at a site like github, or a second browser-app hosted at a site like github); client_ids have meaning only to the OP; non-unique client_ids would break important security aspects of OIDC.

clients have no ability to control the client_id they receive from an OP, nor should they. the only thing an RP should care about with regards to a client_id is "is my client_id the audience or authorized party to which the id_token was issued?"

please see the continued discussion in #12

@TallTed
Copy link

TallTed commented Apr 29, 2019

@zenomt - I think your thumb-down comment-reaction belongs on someone else's comment or the thread-start. Right now it looks like you're thumb-downing your own comment (which starts with a -1) on the PR.

@zenomt
Copy link

zenomt commented Apr 29, 2019

@TallTed you're right, i was githubbing wrong. thanks.

my -1 for this PR stands.

Copy link

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

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

I would say the client_id needs to be the equal to the "origin" (protocol, hostname and if non-default, port) of the redirect_uri, and include some examples of how to determine the client_id for various redirect_uri examples. Other than that, LGTM!

@michielbdejong
Copy link

@jaxoncreed ping

Copy link

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

Small clarification request: host and hostname are used interchangeably, but are not the same. host = hostname + port.

@jaxoncreed
Copy link
Contributor Author

I have fixed @RubenVerborgh 's issue but had to make a new pull because I deleted my fork and needed to make a new fork (#31)

@michielbdejong
Copy link

ok, so this PR can be closed in favour of #31 then, right?

@jaxoncreed
Copy link
Contributor Author

Yep

@jaxoncreed jaxoncreed closed this Jun 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants