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
MSC3983: Sending One-Time Key (OTK) claims to appservices #3983
base: main
Are you sure you want to change the base?
MSC3983: Sending One-Time Key (OTK) claims to appservices #3983
Changes from 12 commits
f82e463
90006ad
b128030
0630814
bbc79ab
91e6d02
0b9f6d9
8a1dd86
5a4a6c6
c770168
a95bf4f
2177b93
052d480
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.
I'm left wondering why the appservice would bother uploaded fallback keys when (to my reading) it could return them directly as part of the response.
I suppose uploading them would still be useful for error conditions, however.
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'm having a hard time understanding what this paragraph is attempting to say.
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.
Is it simply trying to say that implementations implementing MSC3983 should ignore
device_one_time_keys_count
?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's more of a warning to existing implementations of crypto: nearly all of them do a
if (count < 50) { generateAndUploadKeys() }
call, which would be a problem here.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.
Hmm sure, but what are implementations supposed to do instead? That's the part I'm unclear about from the above.
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.
"not that" :p
Normally implementation details like this wouldn't be called out, however given the chance for everyone to have the exact same bug (intending to use new API, uploads keys by accident, new API isn't used), this is called out here.
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.
Is this enough of a change that it need to be a
v4
of the endpoint, perhaps? (Or a query parameter which defaults to the current behavior?)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 it'd be fine, though open to thoughts. Currently clients appear to pick the "first" key returned, not validating if the extra data is useful to them. In theory this means we can include extra keys with no issues, though if there's a significant need to have it be a dedicated endpoint then let's do that.
We should probably only do this fallback stuff on an
/unstable/org.matrix.msc3983
endpoint though, just in case.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.
matrix-org/synapse#15462 implements this as a separate endpoint.
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.
Another piece of this I realized during implementation is if we need to also make a change on the federation side.
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 doesn't define if the appservice should be queried for fallback keys if a OTK is in the database.
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.
In lieu of MSC text: the appservice is supposed to be queried, similar to MSC3984 where the server uses what it knows if the appservice didn't provide 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.
Actually I suppose the text in the MSC is enough as is:
This would not match what you just wrote though and would mean it is not possible for an appservice to only provide fallback keys. I'm not sure if that's a good thing or a bad thing though. 🤷
If we really do want that then I think we want to use different endpoints or something. Otherwise the appservice wouldn't know if we truly want to query for OTKs or only for fallback keys (it could end up returning OTKs which go unused?)
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.
Who is offline in this sentence? And which API are we talking about? Also, who is they?
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 whole MSC is in context of an appservice.
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 gathered as much 😄 But I'm still having trouble parsing that sentence in exact terms. e.g. If I interpret "but is offline" as "but the appservice is offline", how is the appservice able to do anything given it's offline?
I'd suggest rewriting the sentence with less implicit words / pronouns.
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 appservice is opting into using the API by not uploading keys, effectively. This is it "using" the API, which might be the confusing part?
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.
Device IDs and user IDs can't produce signatures, so the proposed change would make it a bit clearer to me.