Skip to content
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.

Sign up for GitHub

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

Jump to bottom

MSC4254: Usage of RFC7009 Token Revocation for Matrix client logout #4254

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

MSC4254: Usage of RFC7009 Token Revocation for Matrix client logout #4254

wants to merge 1 commit into from

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented Jan 17, 2025 •
edited by turt2live
Loading

Rendered

Implemented in matrix-authentication-service, Element X iOS and Android (through the matrix-rust-sdk) and Element Web

In line with matrix-org/matrix-spec#1700, the following disclosure applies:

I am a Software Engineer at Element. This proposal was written and published as an Element employee.

SCT stuff:

checklist

FCP tickyboxes

@sandhose sandhose force-pushed the quenting/oauth2-revocation branch from 7780b3b to bc25ec8 Compare January 17, 2025 15:11
@sandhose sandhose force-pushed the quenting/oauth2-revocation branch from bc25ec8 to ac1602f Compare January 17, 2025 15:12
@sandhose sandhose changed the title MSC4242: Usage of RFC7009 Token Revocation for Matrix client logout MSC4254: Usage of RFC7009 Token Revocation for Matrix client logout Jan 17, 2025
@sandhose sandhose marked this pull request as ready for review January 17, 2025 15:18
@sandhose sandhose mentioned this pull request Jan 17, 2025
Open
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:core MSC which is critical to the protocol's success matrix-2.0 Required for Matrix 2.0 implementation-needs-checking The MSC has an implementation, but the SCT has not yet checked it. labels Jan 17, 2025
@turt2live turt2live mentioned this pull request Jan 22, 2025
Open
9 tasks
@turt2live turt2live removed the implementation-needs-checking The MSC has an implementation, but the SCT has not yet checked it. label Jan 27, 2025
turt2live
turt2live approved these changes Jan 27, 2025
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

nice and simple: call revoke to revoke

@turt2live
Copy link
Member

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:

  • Are appropriate implementation(s)
    specified in the MSC’s PR description?
  • Are all MSCs that this MSC depends on already accepted?
  • For each new endpoint that is introduced:
    • Have authentication requirements been specified?
    • Have rate-limiting requirements been specified?
    • Have guest access requirements been specified?
    • Are error responses specified?
      • Does each error case have a specified errcode (e.g. M_FORBIDDEN) and HTTP status code?
        • If a new errcode is introduced, is it clear that it is new?
  • Will the MSC require a new room version, and if so, has that been made clear?
    • Is the reason for a new room version clearly stated? For example,
      modifying the set of redacted fields changes how event IDs are calculated,
      thus requiring a new room version.
  • Are backwards-compatibility concerns appropriately addressed?
  • Are the endpoint conventions honoured?
    • Do HTTP endpoints use_underscores_like_this?
    • Will the endpoint return unbounded data? If so, has pagination been considered?
    • If the endpoint utilises pagination, is it consistent with
      the appendices?
  • An introduction exists and clearly outlines the problem being solved.
    Ideally, the first paragraph should be understandable by a non-technical audience.
  • All outstanding threads are resolved
    • All feedback is incorporated into the proposal text itself, either as a fix or noted as an alternative
  • While the exact sections do not need to be present,
    the details implied by the proposal template are covered. Namely:
    • Introduction
    • Proposal text
    • Potential issues
    • Alternatives
    • Dependencies
  • Stable identifiers are used throughout the proposal, except for the unstable prefix section
    • Unstable prefixes consider the awkward accepted-but-not-merged state
    • Chosen unstable prefixes do not pollute any global namespace (use “org.matrix.mscXXXX”, not “org.matrix”).
  • Changes have applicable Sign Off from all authors/editors/contributors
  • There is a dedicated "Security Considerations" section which detail
    any possible attacks/vulnerabilities this proposal may introduce, even if this is "None.".
    See RFC3552 for things to think about,
    but in particular pay attention to the OWASP Top Ten.

@turt2live
Copy link
Member

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Jan 27, 2025 •
edited by richvdh
Loading

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

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.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Jan 27, 2025
richvdh
richvdh reviewed Feb 11, 2025
View reviewed changes
proposals/4254-oauth2-revocation.md

The server may return an error response as defined in [RFC7009]. The client should handle these errors appropriately:

- If the token is already revoked, the server returns a 200 OK response
Copy link
Member

Choose a reason for hiding this comment

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

Implication of this is that the server has to retain a record old tokens for at least some time, so that it can respond appropriately?

proposals/4254-oauth2-revocation.md
Comment on lines +66 to +68
- If the token is already revoked, the server returns a 200 OK response
- If the client is not authorized to revoke the token, the server returns a 401 Unauthorized response
- For other errors, the server returns a 400 Bad Request response with error details
Copy link
Member

Choose a reason for hiding this comment

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

What exactly should happen if the token is completely unrecognised? I feel like I could argue for any of 200, 401 or 400 given this description.

Copy link

Choose a reason for hiding this comment

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

From RFC7009:

The authorization server responds with HTTP status code 200 if the
token has been revoked successfully or if the client submitted an
invalid token.

So it should be status 200 for invalid tokens.

Maybe this change would clarify

Suggested change
- If the token is already revoked, the server returns a 200 OK response
- If the client is not authorized to revoke the token, the server returns a 401 Unauthorized response
- For other errors, the server returns a 400 Bad Request response with error details
- If the token is already revoked or invalid, the server returns a 200 OK response
- If the client is not authorized to revoke the token, the server returns a 401 Unauthorized response
- For other errors, the server returns a 400 Bad Request response with error details

proposals/4254-oauth2-revocation.md

### Handling errors

The server may return an error response as defined in [RFC7009]. The client should handle these errors appropriately:
Copy link
Member

Choose a reason for hiding this comment

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

could we be explicit that the response is an RFC6749 error response rather than a Matrix error response?

Suggested change
The server may return an error response as defined in [RFC7009]. The client should handle these errors appropriately:
The server may return an error response as defined in [RFC7009]. Note that RFC7009 mandates a [RFC6749 error response](https://datatracker.ietf.org/doc/html/rfc6749#section-5.2) rather than a Matrix standard error response.
The client should handle these errors appropriately:

dbkr
dbkr reviewed Feb 11, 2025
View reviewed changes
proposals/4254-oauth2-revocation.md

The request includes:
- The `token` parameter containing either the access token or refresh token to revoke
- Optionally, the `token_type_hint` parameter, with either the `access_token` or `refresh_token` value. If provided, the server must use this value to determine which token to revoke
Copy link
Member

Choose a reason for hiding this comment

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

Didn't it say above that it revokes both?

Copy link

Choose a reason for hiding this comment

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

I think this specifies which type the token is, so if token is an access token and you specified a refresh token hint, it won't work, but if you specified access token, the request will go through and both will be revoked

FSG-Cat
FSG-Cat reviewed Feb 12, 2025
View reviewed changes
proposals/4254-oauth2-revocation.md
The request includes:
- The `token` parameter containing either the access token or refresh token to revoke
- Optionally, the `token_type_hint` parameter, with either the `access_token` or `refresh_token` value. If provided, the server must use this value to determine which token to revoke
- The `client_id` obtained during client registration
Copy link
Contributor

Choose a reason for hiding this comment

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

So after consulting the auth room it makes sense to make client id optional for access tokens atleast. They are live without this information and therefore it makes sense to enable revokation without it.

Copy link

Choose a reason for hiding this comment

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

Specifically to allow revoking a leaked access token which could be used maliciously without a client id, making it harder to do the right thing than the bad thing.

@zecakeh zecakeh mentioned this pull request Feb 17, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbkr dbkr dbkr left review comments

@richvdh richvdh richvdh left review comments

@tonkku107 tonkku107 tonkku107 left review comments

@morguldir morguldir morguldir left review comments

@FSG-Cat FSG-Cat FSG-Cat left review comments

@turt2live turt2live turt2live approved these changes

Assignees
No one assigned
Labels
client-server Client-Server API disposition-merge kind:core MSC which is critical to the protocol's success matrix-2.0 Required for Matrix 2.0 proposal A matrix spec change proposal proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period.
Projects
Spec Core Team Backlog
Status: Ready for FCP ticks
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sandhose @turt2live @mscbot @dbkr @richvdh @tonkku107 @morguldir @FSG-Cat