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

MSC4147: Including device keys with Olm-encrypted events #4147

Merged
merged 12 commits into from
Dec 10, 2024

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented May 29, 2024

Rendered

disclosure: I am on the Spec Core Team and employed by Element. This MSC is written as part of my work in the Crypto team at Element


FCP tickyboxes

@uhoreg uhoreg changed the title MSCxxxx: Including device keys with Olm-encrypted events MSC4147: Including device keys with Olm-encrypted events May 29, 2024
@turt2live turt2live added proposal A matrix spec change proposal kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels May 29, 2024
Copy link
Member

@turt2live turt2live May 29, 2024

Choose a reason for hiding this comment

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

Implementation requirements:

  • Client sending this information
  • Client using this information
  • Demonstration of the core problem being solved (likely covered by the above tasks)

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been implemented in the Rust matrix-sdk-crypto crate, both sending and using. Demonstration of the problem being solved: https://youtu.be/b1jJlT2ENT8?feature=shared&t=345

Copy link
Member

Choose a reason for hiding this comment

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

A few links to the implementation.

On the sender side, the field is added here.

On the receiver side, it is deserialized after decryption into a DecryptedOlmV1Event. It is then used in SenderDataFinder whose job it is to construct a SenderData which records our knowledge about the sending device for a given session.

poljar pushed a commit to matrix-org/matrix-rust-sdk that referenced this pull request Jun 27, 2024
@richvdh
Copy link
Member

richvdh commented Nov 2, 2024

I think this is ready for FCP, so:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Nov 2, 2024

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

Concerns:

  • implementation-needs-checking label

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 Nov 2, 2024
@turt2live turt2live added implementation-needs-checking The MSC has an implementation, but the SCT has not yet checked it. and removed needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Nov 4, 2024
@turt2live
Copy link
Member

@mscbot concern implementation-needs-checking label

(the needs-implementation label was not removed when the FCP was proposed or implementations listed, so I am assuming they have not been checked out of an abundance of caution)

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Nov 4, 2024
@richvdh
Copy link
Member

richvdh commented Nov 5, 2024

I've looked at the implementation, and checked that it matches the proposal to the best of my ability.

@richvdh richvdh removed the implementation-needs-checking The MSC has an implementation, but the SCT has not yet checked it. label Nov 5, 2024
@turt2live
Copy link
Member

@mscbot resolve implementation-needs-checking label

@mscbot mscbot removed the unresolved-concerns This proposal has at least one outstanding concern label Nov 5, 2024
@turt2live turt2live added the matrix-2.0 Required for Matrix 2.0 label Nov 18, 2024
@dbkr
Copy link
Member

dbkr commented Nov 19, 2024

Thanks for writing up the alternative of keeping deleted devices in the device list: this otherwise would have been my question.

@ara4n
Copy link
Member

ara4n commented Dec 4, 2024

For the record, I am still not happy with the extreme duplication of the denormalised device data per-event, rather than keeping deleted devices around device lists until their to-device msgs have been exhausted.

On the other hand, the current behaviour amounts to a security issue, and in practice this has been already implemented and shown working in the Element clients. So I'm going to approve FCP in order that we ship the fix rapidly for everyone, and am going to pray that someday we remove the denormalisation again (perhaps around the same time as senders-as-keys).

Feels really wrong though, and it's a bad smell this didn't surface before implementation began.

Comment on lines +170 to +174
If a device is logged out, there is no indication why it was logged out. For
example, an attacker could steal a device and use it send a message. The user,
upon realizing that the device has been stolen, could log out the device, but
the message may still be sent, if the user does not notice the message and
redact it.
Copy link
Member

Choose a reason for hiding this comment

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

Can you spell out the impact of this? I think it is that by now including the keys the messages will be "trusted" and logging out the device won't make the messages automatically untrusted?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's exactly that, though @uhoreg wrote this bit so can maybe confirm.

I don't think it's a terribly significant concern, since it didn't work very well before. I think if we want users to be able to revoke signatures on devices, we need to add explicit support for that, rather making every logout be a half-assed effort at it.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough about having an explicit ability for that, just trying to understand impact!

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you spell out the impact of this? I think it is that by now including the keys the messages will be "trusted" and logging out the device won't make the messages automatically untrusted?

Yes, that's the issue.

I think that at some point, we want to have a mechanism for users to indicate that a device was compromised, and that messages from it shouldn't be trusted. But that would be a separate MSC and for now, users can probably do so manually by sending messages to rooms.

additional bookkeeping, and it is not clear whether this extra complexity is
worth the reduction in bandwidth.

### Alternative approach
Copy link
Member

Choose a reason for hiding this comment

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

Is persisting info into rooms as a permanent record another alternative? Does that even make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you could make room members be devices, rather than users (MLS-style); as part of that, you could embed the device keys/signatures in the state events. It's something we've considered in the past (it helps with account portability, among other things); however there are a lot of moving parts to get it right, and having to send a membership event to every room you're in every time you log in is a bit of a killer.

It would really be a radically different approach, whilst this MSC is more of an incremental improvement.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Not sure if this should be included or not as an alternative.

@mscbot
Copy link
Collaborator

mscbot commented Dec 5, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Dec 5, 2024
@clokep
Copy link
Member

clokep commented Dec 5, 2024

Thanks for answering all my questions @richvdh -- I think this is OK as an incremental improvement, but sounds like we might need to do a bigger improvement at some point in the future.

@mscbot
Copy link
Collaborator

mscbot commented Dec 10, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Dec 10, 2024
@richvdh richvdh merged commit 51ebe01 into matrix-org:main Dec 10, 2024
1 check passed
@richvdh richvdh added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Dec 10, 2024

We propose to solve this by including a copy of the device keys in the
Olm-encrypted message, along with the cross-signing signatures, so that the
recipient does not have to try to query the sender's keys.
Copy link
Member

Choose a reason for hiding this comment

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

Is the intention to make sender_device_keys mandatory, and drop to-device messages that do not include it?

Copy link
Member

Choose a reason for hiding this comment

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

That was very much not part of this MSC. It's something we could do in future, but IMHO it would need another MSC, and careful thought, since it would mean cutting off the significant part of the ecosystem which doesn't yet implement this MSC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec matrix-2.0 Required for Matrix 2.0 proposal A matrix spec change proposal spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec
Projects
Status: Requires spec writing
Development

Successfully merging this pull request may close these issues.

9 participants