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 all 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.
In addition to both fallback and OTKs, @dkasak suggested that it could be helpful to request multiple keys at once over the C-S API. In particular having 0 or 1 keys of multiple algorithms could be useful.
I implemented this using the same unstable endpoint by accepting a map of
{"<algo>": <count>}
instead of a single algorithm. I'm now seeing that the above has an array of algorithms to pass to the appservice instead though. I guess I should consolidate those to be the same though.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 Synapse PR was updated to use the same form sent to appservices (i.e. a list of algorithms).
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.
We'll probably want to add something saying that clients may need to deal with getting back fewer than the requested numbers of keys and that servers should protect against users request too many keys at once to avoid exhaustion.
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.