-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add recommendation to verify matching authorization_endpoint #53
Add recommendation to verify matching authorization_endpoint #53
Conversation
Is there any reason to limit this to a SHOULD rather than a MUST? I think most security related things in the spec are marked as MUST and MUST NOT. A SHOULD seems weird to me here, because what is the alternative? Just allowing it through? |
I thought about making it a MUST but I anticipated pushback about that too. :) I would prefer a MUST, for the reasons you stated. |
Also preferring MUST wording. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be added to the file public/source/index.php not the one in the spec
folder. That file is generated from the other one.
The only reason I could see for a "SHOULD" here is if it's adding a significant burden to the developer in a way that they are likely to not do it anyway. |
It's an extra fetch if it's a different URL (i.e. not the URL the initial discovery ended up on) - but same discovery code already implemented. Behavior around accepting redirects might be worth specifying (maybe no 301? essentially "do as before, but do not accept the url changing"?) |
Ah, thanks, that wasn't clear to me. Is there a documented process for correctly building the files? And, sounds like the consensus is it SHOULD be a MUST. :) Will definitely make that change! |
bbbb9e2
to
6812dcc
Compare
Don't worry about the built files, they'll make it out in the next update I do.
This is a very good point, and something that needs thinking through to make sure all the cases are handled and described properly. |
I agree that the wording for redirect stuff needs to be addressed in general, and there's an open issue #36 for that. For the purpose of this spec change I think the two following facets need to be captured:
It might make sense to break the redirection logic aspect of 'discovery by clients' out into a separate common section that is referred to by both 'discovery by clients' and 'differing user profile URLs'. |
Hmm. We can short circuit this somehow? Something where we say "if the returned me is on a different domain than the user provided URL, the client SHOULD do endpoint discovery on the returned me. If the endpoint discovered for the returned me URL does not match the previously discovered endpoint, the client MUST NOT accept the authentication." Does that cover the entire use-case? I am not sure we care more or less about redirects here than we do during the initial discovery step. If we do care differently about redirects here: why? Brainstorming in 30 degree weather here, so take everything with a grain of salt. |
I would prefer to not limit that to different domains, but apply it always if the path differs, to secure subpaths on the same domain controlled by different entities too. |
Yeah, the whole point to this is that it addresses the issue of multiple identities on the same domain. We already have a clear-cut way of handling identities that resolve to a different domain — we don't. |
1. Requires that the final profile URL be redirection-normalized in the same way as initial profile discovery 2. Specifies the order of the validation steps/conditions This does not address the issue of the verbiage regarding redirection normalization, which is still covered in issue indieweb#36.
I think the main difference with following redirects is that the notion of "updating the profile URL" doesn't apply probably? Whatever the endpoint gave you should be the profile URL, so just follow all redirects? or only temporary redirects? A strict rule could be to not allow redirects at all, but there might be use cases for that? |
To make sure I understand this: after getting an access token, the client must download the |
See the matching issue (#35) for the example. |
I do not know why my 30 degree brain was thinking domains. That is not even the case we are trying to solve here. Apologies.
It to me does raise the question why not extend this to different domains. Surely any profile URL change, including domain change, could trigger re-discovery and be found to be a valid change? And if it is found to be valid, why not allow that.
I would almost expect the profile URL to stay the same. Because otherwise the authorization endpoint answered with a URL, that itself would never be a canonical profile URL? So the question is kind of what we are discovering here: are we verifying the returned profile URL as a valid canonical profile URL, or are we just verifying that the returned profile URL points at the same authorization endpoint? Another thing I was wondering about is if it makes sense to limit the re-discovery somehow if we can be sure that the URL change is not an impersonation. E.g. by checking that the returned profile URL is a subpath to the original canonical profile URL. As it is very likely that deeper paths are controlled by the same entity. But as @sknebel was quick to pointed out, exceptions are always a pitfall. And as @fluffy-critter remarked optimising for HTTP requests seems unnecessary for this specification. Still wanted to make note of this having been discussed. But I would not want to introduce any changes to this PR along these lines. I agree with the arguments raised by both @sknebel and @fluffy-critter. |
And just to point out that the original proposed change was to require that the resulting path be deeper than the original/verified path, and this was (rightfully) shot down as too complex and had too many holes in it. Anecdotally when I changed from verifying "deeper path" to "matching endpoint" Authl's code became much simpler and easier to test. PlaidWeb/Authl#84 |
Thanks for the proposed text! We're going to add a little more qualifications to this section based on the discussions from the IndieAuth session. |
This adds a MUST recommendation to verify that the target of a canonical profile URL declares the same
authorization_endpoint
as the original profile URL, to prevent identity hijacking on shared domains, and also specifies that the canonical profile URL must follow permanent redirections in the same manner as initial profile discovery.Fixes #35