-
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
Introduce Server Metadata and Iss Parameter #102
Conversation
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.
Couple of minor tweaks, but looking good
public/source/index.php
Outdated
|
||
<ul> | ||
<li>That the <code>state</code> parameter in the request is valid and matches the state parameter that it initially created, in order to prevent CSRF attacks. The state value can also store session information to enable development of clients that cannot store data themselves.</li> | ||
<li>That the <code>iss</code> parameter in the request is valid and matches the issuer parameter provided by the Server Metadata endpoint during Discovery as outlined in <a href="https://www.ietf.org/archive/id/draft-ietf-oauth-iss-auth-resp-02.html">OAuth 2.0 Authorization Server Issuer Identification]</a>. Clients MUST compare the parameters using simple string comparison. If the value does not match the expected issuer identifier, clients MUST reject the authorization response and MUST NOT proceed with the authorization grant. For error responses, clients MUST NOT assume that the error originates from the intended authorization server. </li> |
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.
Need to add: If there is no iss
parameter in the request but the Authorization Server's Metadata has an entry for authorization_response_iss_parameter_supported
that is true
(see https://www.ietf.org/archive/id/draft-ietf-oauth-iss-auth-resp-02.html#name-authorization-server-metada), the client MUST reject the response.
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.
@omz13 I thought about that. But if the iss parameter is now required by all IndieAuth implementations, then the authorization_response_iss_parameter_supported is only for compatibility with OAuth2.0, because if an IndieAuth client and server has been updated to support server metadata, it should support the iss check by default.
The indicator for an IndieAuth endpoint would be not having the server metadata file at all, in which case the client could choose to omit the iss check for compatibility purposes.
|
||
<ul> | ||
<li>That the <code>state</code> parameter in the request is valid and matches the state parameter that it initially created, in order to prevent CSRF attacks. The state value can also store session information to enable development of clients that cannot store data themselves.</li> | ||
<li>That the <code>iss</code> parameter in the request is valid and matches the issuer parameter provided by the Server Metadata endpoint during Discovery as outlined in <a href="https://www.ietf.org/archive/id/draft-ietf-oauth-iss-auth-resp-02.html">OAuth 2.0 Authorization Server Issuer Identification</a>. Clients MUST compare the parameters using simple string comparison. If the value does not match the expected issuer identifier, clients MUST reject the authorization response and MUST NOT proceed with the authorization grant. For error responses, clients MUST NOT assume that the error originates from the intended authorization server. </li> |
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.
Because it's been a while since I've read the RFC for how to write RFCs, should this link out to https://www.ietf.org/archive/id/draft-ietf-oauth-iss-auth-resp-02.html
be a reference below?
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.
Almost there 🙌
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.
🚀 Good stuff
This covers #43 and #101. Added the Iss parameter requirement as part of this PR as the Issuer Identifier had to be defined in Metadata.
This defines the metadata endpoint, notes the fallback to the old way of doing things, and requires that the iss parameter be returned by the authorization response and be verified against the metadata endpoint issuer parameter.