solid / webid-oidc-spec

WebID-OIDC Authentication Spec v0.1.0
MIT License
56 stars 18 forks source link

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

Closed jaxoncreed closed 5 years ago

jaxoncreed commented 5 years ago

Continuation of https://github.com/solid/webid-oidc-spec/pull/16 because of a mess up with forked repos

michielbdejong commented 5 years ago

@zenomt and @dmitrizagidulin can you review please?

michielbdejong commented 5 years ago

Also, @kjetilk @Mitzi-Laszlo @timbl @justinwb @RubenVerborgh: two of you need to approve this before it can be merged.

RubenVerborgh commented 5 years ago

it's not appropriate to place any constraint on the form of the client_id (for example, that it "must be identical to the value of the origin header") beyond what is already specified by OIDC

I wouldn't know why it is “not appropriate”, given that we also place constraints on OIDC to authenticate people with a WebID.

But I do think that the burden of proof is on @jaxoncreed to argue why indeed it needs to be a WebID, and to document that in this PR.

further, the redirect_uri should be entirely up to the application.

Why though? It seems perfectly reasonable to constrain this to the same domain, because this is the domain I'm giving permission.

zenomt commented 5 years ago

placing a constraint on the client_id is inappropriate because the form of the client_id is an implementation detail of the OP (except for the requirement that it's unique for every registration). OPs are free to make the client_id be whatever is convenient (for example, a key to a database table, or a structured blob encoding information about the client, or anything, as long as it's unique). this is discussed a lot more in #12.

regarding the redirect_uri: since that's where the OP sends the credentials (or the code), i've argued in #12 and #23 that the redirect_uri should be the application identifier.

RubenVerborgh commented 5 years ago

the form of the client_id is an implementation detail of the OP

But nothing stops us from adding more constraints on it for Solid-compatibility?

It would be a problem if making it a URL would be incompatible with the OIDC spec. But it does not seem to be incompatible.

the redirect_uri should be the application identifier

Very likely not (talking from a SemWeb perspective now); the URI of a a redirect page identifies a redirect page, not an app.

zenomt commented 5 years ago

But nothing stops us from adding more constraints on it for Solid-compatibility?

requiring it to be a URL (or requiring any particular form) would shut out lots of existing OIDC Provider implementations that would otherwise be able to be a WebID-OIDC Provider. currently the changes to make an OIDC Provider be a WebID-OIDC Provider are backward-compatible additions, like "put the WebID in a new webid claim, otherwise if the sub is a URI, it's the WebID", and for the proof-token thing, "have a way to set a cnf claim in the id_token". the other things that make it be WebID-OIDC are outside the OP, like that the WebID says the OP is an authorized issuer.

the uniqueness of the client_id is an important security aspect of OIDC/OAuth. if the client_id were a URL, it would still need to be a unique URL somehow (fragment?) for each registration.

for the OP to assign a client_id taking the form of a URL that has the same origin as the redirect_uri, that would require all redirect_uris being registered to be in the same origin (OIDC allows multiple redirect_uris, and they don't currently need to share an origin).

requiring all redirect_uris for a client to share an origin, and for that origin to be the same as something (there is no Origin header when redirecting to/loading the OP's login page, and dynamic registration could be done by a back-end server that could omit or set any Origin) would eliminate an entire class of possible application architectures, where different components of the application are in different origins (maybe some parts are in AWS Lambda/Azure Functions/OpenWhisk/etc, maybe logging in is a static github pages page, etc).

RubenVerborgh commented 5 years ago

requiring it to be a URL (or requiring any particular form) would shut out lots of existing OIDC Provider implementations that would otherwise be able to be a WebID-OIDC Provider.

+1

if the client_id were a URL, it would still need to be a unique URL somehow (fragment?) for each registration.

This is a major blocker indeed; makes the suggestion incompatible with OIDC.

Thanks for clarifying!

kjetilk commented 5 years ago

So, what do you say, @jaxoncreed ? Do you agree with @zenomt and @RubenVerborgh 's assessment?

justinwb commented 5 years ago

requiring it to be a URL (or requiring any particular form) would shut out lots of existing OIDC Provider implementations that would otherwise be able to be a WebID-OIDC Provider. currently the changes to make an OIDC Provider be a WebID-OIDC Provider are backward-compatible additions, like "put the WebID in a new webid claim, otherwise if the sub is a URI, it's the WebID", and for the proof-token thing, "have a way to set a cnf claim in the id_token". the other things that make it be WebID-OIDC are outside the OP, like that the WebID says the OP is an authorized issuer.

Anything that hinders compatibility with other OIDC provider implementations should be considered a non-starter. Really important that we foster interop.

jaxoncreed commented 5 years ago

Yes, I actually do agree with @zenomt at the moment. It's reflected in another pull request I have (https://github.com/solid/webid-oidc-spec/pull/27) for the spec but not this one. I believe this one can be thrown out.

justinwb commented 5 years ago

Yes, I actually do agree with @zenomt at the moment. It's reflected in another pull request I have (#27) for the spec but not this one. I believe this one can be thrown out.

@jaxoncreed to be clear - this pull request can be closed without merging in favor of #27 ?

michielbdejong commented 5 years ago

See also https://github.com/solid/webid-oidc-spec/issues/12#issuecomment-502723571

kjetilk commented 5 years ago

OK; I understand that this can be closed. If that was wrong, we can always reopen :-)

jaxoncreed commented 5 years ago

You're correct. It can be closed.