-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
MSC2965: OAuth 2.0 Authorization Server Metadata discovery #2965
base: main
Are you sure you want to change the base?
Conversation
Are any other examples planned? I’m using Ory for several apps that I’d like to also connect together with Matrix. It also strikes me as a conveniently lightweight example for Matrix, which also aligns well with Dendrite since it’s in Go. |
@erlend-sh Good suggestion, thank you - I've added element-hq/oidc-playground#3 to track this. |
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.
overall looks good to me - just a few (hopefully) easily correctable concerns
MSCs proposed for Final Comment Period (FCP) should meet the requirements outlined in the checklist prior to being accepted into the spec. This checklist is a bit long, but aims to reduce the number of follow-on MSCs after a feature lands. SCT members: please check off things you check for, and raise a concern against FCP if the checklist is incomplete. If an item doesn't apply, prefer to check it rather than remove it. Unchecking items is encouraged where applicable. Checklist:
|
Implementation lgtm. Concerns are relatively minor as well: @mscbot fcp merge |
Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people: Concerns:
Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
Co-authored-by: Travis Ralston <travpc@gmail.com>
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.
read-up-to marker
(thanks for keeping these updated)
|
||
## Proposal | ||
|
||
This introduces a new Client-Server API endpoint to discover the authorization server metadata used by the homeserver. |
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.
Can we include a rationale for why we don't hardcode the endpoint URLs to be under a /_matrix/oauth2
scheme? We still need the metdata discovery for other fields, so to an extent there is a question as to "why not", but still.
|
||
If the homeserver does not offer next-generation authentication as described in [MSC3861], this endpoint should return a 404 with the `M_UNRECOGNIZED` error code. | ||
|
||
In this case, clients should fall back to using the User-Interactive Authentication flows instead to authenticate the user. |
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.
or /#
?
|
||
In this case, clients should fall back to using the User-Interactive Authentication flows instead to authenticate the user. | ||
|
||
## Rationale for not using a `.well-known` document |
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.
Editorial nit: put this under the corresponding "Alternatives" entry, rather than making it its own h2 heading?
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.
Yes -- it is really confusing to present this before the alternative of using well-known.
However, the RFC states that an application leveraging this standard should define its own application-specific endpoint, e.g. `/.well-known/matrix-authorization-server`, and _not_ use the `.well-known/oauth-authorization-server` endpoint. | ||
To avoid confusion with the existing `.well-known/matrix/*` documents, this proposal suggests defining a new C-S API endpoint instead. | ||
|
||
### Discovery via the well-known client discovery |
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.
Nit: it's server discovery rather than client discovery, if anything.
maybe:
### Discovery via the well-known client discovery | |
### Discovery via existing `.well-known` mechanism |
A previous version of this proposal suggested using the well-known client discovery mechanism to discover the authentication server. | ||
Clients already discover the homeserver when doing a server discovery via the well-known document. | ||
|
||
A new `m.authentication` field is added to this document to support OpenID Connect Provider (OP) discovery. |
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.
A previous version of this proposal suggested using the well-known client discovery mechanism to discover the authentication server. | |
Clients already discover the homeserver when doing a server discovery via the well-known document. | |
A new `m.authentication` field is added to this document to support OpenID Connect Provider (OP) discovery. | |
A previous version of this proposal suggested using the existing [homeserver discovery mechanism](https://spec.matrix.org/v1.13/client-server-api/#server-discovery) to discover the authentication server. | |
A new `m.authentication` field is added to the `.well-known` document to support OpenID Connect Provider (OP) discovery. |
The first option would require making well-known documents mandatory on the server name domain, with a document that may need to be updated more frequently than existing ones. | ||
This isn't practical for some server deployments, and clients may find it challenging to consistently perform this discovery. | ||
|
||
The second option is also impractical, as all other Matrix APIs on this domain are prefixed with `/_matrix`, and it could easily be confused with the set of well-known documents hosted on the server name domain. |
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.
I don't understand this -- I think the thread (https://github.com/matrix-org/matrix-spec-proposals/pull/2965/files#r1924524746) talks about before/after finding the client API. That rationale seems to be missing here.
Frankly to me, I'd expect this to be under well-known.
} | ||
``` | ||
|
||
**Note**: The fields required for the main flow outlined by [MSC3861] and its sub-proposals are: |
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.
Here we define some required fields for the matrix proposals but I think some clarity is needed on whether requirements from RFC8414 also apply.
This is the method to get the server metadata in the latest draft of [MSC2965](matrix-org/matrix-spec-proposals#2965). We still keep the old behavior with `GET /auth_issuer` as fallback for now because it has wider server support. There are some pre-main commit cleanups to simplify the main commit. This can be reviewed commit by commit. The changes were tested with the oidc_cli example on beta.matrix.org. Closes #4550. --------- Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>
Rendered
Status:
Dependencies:
Clients and homeservers currently implement an older version of this proposal, and need to be updated:
/auth_metadata
endpoint defined in MSC2965. element-hq/synapse#18093/auth_metadata
API matrix-js-sdk#4626SCT:
tickyboxes
checklist