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.

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

Pass through unsigned data in /keys/query #17951

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Nov 21, 2024

We'd like a mechanism by which a client can add "unsigned" data to their device keys, and have it be accessible by other clients involved in E2EE discussions.

Most of this actually already works; the bit that doesn't is that client-side /keys/query strips out any "unsigned" data from the /keys/upload body. (Server-side /keys/query follows a different codepath and is fine).

This PR adds an experimental option which modifies client-side /keys/query so that unsigned data is preserved.

An MSC for this behaviour is pending, but will be matrix-org/matrix-spec-proposals#4229.

Fixes: #17943
Part of element-hq/element-meta#2467

@richvdh richvdh requested a review from a team as a code owner November 21, 2024 12:27
@richvdh richvdh changed the title Pass through unsigned data in /keys/query We'd like a mechanism by which a client can add "unsigned" data to their device keys, and have it be accessible by other clients involved in E2EE discussions. Most of this actually already works; the bit that doesn't is that *client-side* /keys/query strips out any "unsigned" data from the /keys/upload body. (Server-side /keys/query follows a different codepath and is fine). Pass through unsigned data in /keys/query Nov 21, 2024
We'd like a mechanism by which a client can add "unsigned" data to their device
keys, and have it be accessible by other clients involved in E2EE discussions.

Most of this actually already works; the bit that doesn't is that *client-side*
`/keys/query` strips out any "unsigned" data from the `/keys/upload`
body. (Server-side `/keys/query` follows a different codepath and is fine).

This commit adds an experimental option which modifies client-side
`/keys/query` so that `unsigned` data is preserved.
@richvdh richvdh force-pushed the rav/unsigned_data_in_keys_query branch from bf29370 to c7f443a Compare November 21, 2024 12:31
@@ -0,0 +1 @@
Add experimental option to pass through unsigned data in `/keys/query` responses.
Copy link
Contributor

@MadLittleMods MadLittleMods Nov 22, 2024

Choose a reason for hiding this comment

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

Seems relatively sane given we are already returning some data under unsigned .

It feels a bit weird that we're storing the unsigned data on /keys/upload already (not spec'ed) but it looks like we just store whatever JSON is given (doesn't seem like the best idea).

You explained the reason why we can't just bodge this in under any key other than unsigned is because of the following:

It's worth noting that we can't simply add this data to the body of the key uploaded with /keys/upload, otherwise a change to app_version will invalidate any cross-signing signatures, and hence require re-verification of the device. Instead, we must add it to the unsigned section, but that isn't currently exposed via /keys/upload. In short, whatever we do, we need server-side and spec changes.

-- element-hq/element-meta#2467 (comment)

And the code around that seems to be at:

# make sure that the device submitted matches what we have stored
stripped_signed_device = {
k: v for k, v in signed_device.items() if k not in ["signatures", "unsigned"]
}
stripped_stored_device = {
k: v for k, v in stored_device.items() if k not in ["signatures", "unsigned"]
}

Are clients meant to send/store things on unsigned? From our other usages, it sorta seems like unsigned is reserved specifically for the server wanting to return extra data (not for clients to add things to).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sender client info: Add unsigned to /keys/upload and include it in /keys/query
2 participants