Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC2918: Refresh tokens #2918
MSC2918: Refresh tokens #2918
Changes from 8 commits
ab50b62
f8dad2a
0e615f7
870cded
6530ecc
b320001
d433e3b
87566c3
269fcac
4d73b7e
db8ceab
9bbb4c5
a050dc3
2c11e6f
4cd94e3
04ae1c3
488e9e1
4cf821c
c076763
a157cc3
ed54213
70b2dfc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Shouldn't this be if its false? I.e. we include the params when we return a valid access token?
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.
The spec says:
So that sentence seems correct? If
inhibit_login
istrue
, it will not return the additional fieldsThere 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.
it's not the easiest thing to grok, but @sandhose is right, and @erikjohnston is confused.
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.
Does the refresh token expire? How does one manually expire it?
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.
also would be interested to see how this plays with soft logout: if the access token expires, but the refresh token is still live, should the server be using
soft_logout: true
in expiration responses? If so, how should the server clean up the device once the refresh token also expires?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.
Refresh token don't expire, but they get invalidated on use.
By the way, the current implementation in Synapse is incompatible with the
session_lifetime
config parameter, which was the one that made access token expire and led to soft logouts.I was not aware of how
soft_logout
works. :)But also clients should now be aware when the token expires, so
soft_logout
on access token expiration should not happen as often, but then I'd clarify thatsoft_logout
also applies when using the/refresh
endpointThere 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 particular piece is a bit concerning, as it means that refresh tokens are hanging around waiting to give access back to the account. On the other hand, this somewhat fixes the scripts usecase as it can then store the refresh token and use that on the next run.
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.
It still heavily mitigates the impact of token leakage, since they are rotating.
This is what a security best practices for OAuth 2.0 document from the IETF says about refresh tokens:
tl;dr: if there is an attempt to use an old refresh token, there might be a token leak somewhere and the whole session should be invalidated. This could be mentioned in the MSC and/or in the spec, and implemented in Synapse if you thing it makes sense.
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.
Also clarified about
soft_logout
in the refresh token API in 2c11e6fThere 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.
ah ha, so the server would revoke the access token when a refresh token is used twice?
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.
It could, although not strictly enforced by this MSC (and the current implementation in Synapse does not do that)
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.
@turt2live is this clear enough now?
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.
to expand on this, and the "potential issues" section above, what are the concerns with introducing it as some form of opt-in (or opt-out) mechanism for things like long-lived bots or scripts which do not easily have a refresh opportunity? For example, a nightly batch job to prune rooms/events/etc could use a static access token instead of having to login, do the work, then log out again, which would put the password near the script rather than a single revocable token.
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 think for both use cases (bots and scripts) I'd rather make use of the
org.matrix.login.jwt
login type with some KMS signing it for the initial login and still have the token refresh. Storing long-lived access tokens without proper secret handling is at least as bad as storing the login/pass of the bot IMO, especially if the user has admin access.If we still need some kind of static access token, I'd rather have that in Synapse (in the config or something) than in the spec
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.
Fair, that sounds reasonable. Just wanted to expand on the potential usecase, but agreed that scripts can find other ways to authenticate (or better yet: be replaced by features within the protocol/homeserver implementation)
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 think an authentication option for scripts needs to be in the spec. I have a lot of scripts that push notifications or upload files from CI jobs for example. Those use access tokens, because CI jobs do sometimes get compromised (happened once because of codecov) and that way the access token can be easily rotated without being a homeserver admin. If the script used username and password instead, an attacker would have been able to get past UIA and change the password and just in general do much more nasty stuff than with an access token. The jobs also can't refresh the access token, since they may be running concurrently and can't change CI variables.
What would be my alternative for that use case, that works independent of the specific homeserver implementation?
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.
There is an ongoing effort to rework the whole authentication process, with use cases like scripts running in CI in mind. This MSC is also done to prepare clients for the eventual migration to this new authentication stack without having them to logout all their existing sessions.
The login API with non-expiring token will hopefully stay until this new auth stack is ready, so when you would need to migrate you will have a proper alternative.
In the meantime, if you want to still adopt refresh tokens and you are admin of your homeserver, I suggest you look into the
org.matrix.login.jwt
login type in Synapse. Even though it is not standard, it will let you login using a JWT signed by some party.It might still need some changes in Synapse to allow restricting the tokens (like not being able to use them for UIA to avoid letting the account to be nuked), but I prefer to go that route rather than adding this kind of special cases in this MSC if it will be superseded by something else soon-ish.