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

MSC2472: Symmetric SSSS #2472

Merged
merged 6 commits into from
May 21, 2020
Merged

MSC2472: Symmetric SSSS #2472

merged 6 commits into from
May 21, 2020

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Mar 24, 2020

@uhoreg uhoreg changed the title MSCxxxx: Symmetric SSSS MSC2472: Symmetric SSSS Mar 24, 2020
@uhoreg uhoreg added proposal A matrix spec change proposal proposal-in-review labels Mar 24, 2020
@auscompgeek
Copy link
Contributor

Were AEAD modes such as GCM considered?

@uhoreg
Copy link
Member Author

uhoreg commented Mar 25, 2020

I believe GCM was considered earlier when e2ee was originally being implemented, but there was lack of support in some libraries. It may be better supported now, but I'd rather not add another encryption mode to the mix (we already use CBC and CTR), and stick with things that we're already using where possible. In the future, we may at some point do a bulk review of the encryption that we use and switch things across the board at that time, but for now, I think it's better to stay with things that we already use.

@ara4n
Copy link
Member

ara4n commented Mar 26, 2020

this lgtm; i'd suggest proposing a merge tbh.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

looks sane, and I trust others will verify the cryptobits much more closely than I am here.

@turt2live
Copy link
Member

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Mar 27, 2020

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

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 disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Mar 27, 2020
@dbkr
Copy link
Member

dbkr commented Mar 27, 2020

Updating the 1946 proposal to say the same thing seems quite confusing - I'd have probably either left it in and marked it as superseded or deleted it rather than have duplicated content.

Looks fine otherwise though.

@anoadragon453
Copy link
Member

Agree as well that copying the contents and modifying them in this MSC and putting a "superseded" warning in the old MSC would've been clearer, but oh well.

We should link the implementations of this, though that may be a lot of different PRs. Instead, I'll just mention that this has been implemented in Riot Web (riot-web, matrix-react-sdk, matrix-js-sdk), RiotX Android and Riot iOS.

@uhoreg
Copy link
Member Author

uhoreg commented Mar 30, 2020

I've made a couple of update:

  • keys don't need to be signed any more since they're trusted on the basis of us having the secret key (this was only necessary when using an asymmetric algorithm)
  • added some information so that we can check the key before we try to use it

As far as modifying the text of the original MSC, I'm not sure what's the best way to do it, and am open to suggestions. I think that all approaches have some sort of downside. I wanted to do it in a way that there was one document that people could look at to get the current recommendation, and to have something explaining the rationale behind the change.

@uhoreg
Copy link
Member Author

uhoreg commented Apr 14, 2020

Implemented in Riot-web by matrix-org/matrix-js-sdk#1238 and matrix-org/matrix-js-sdk#1294

@ara4n can you convert your "lgtm" to a ✔️ ?

@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 20, 2020
@matrix-org matrix-org deleted a comment from mscbot Apr 27, 2020
@richvdh
Copy link
Member

richvdh commented May 5, 2020

seems entirely plausible to me. I'm assuming that other people have thought about the crypto aspects of this.

@mscbot
Copy link
Collaborator

mscbot commented May 5, 2020

🔔 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 May 5, 2020
@mscbot
Copy link
Collaborator

mscbot commented May 10, 2020

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 final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels May 10, 2020
@turt2live
Copy link
Member

Somehow this missed the list of things that passed FCP

@turt2live turt2live merged commit e264124 into matrix-org:master May 21, 2020
@turt2live turt2live 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 May 21, 2020
@uhoreg uhoreg mentioned this pull request Jun 2, 2020
@uhoreg uhoreg added merged A proposal whose PR has merged into the spec! and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Jun 20, 2020
@uhoreg
Copy link
Member Author

uhoreg commented Jun 20, 2020

Merged! 🎉

@richvdh
Copy link
Member

richvdh commented Apr 12, 2023

Updating the 1946 proposal to say the same thing seems quite confusing - I'd have probably either left it in and marked it as superseded or deleted it rather than have duplicated content.

Coming back here three years later: +1 to this. It confused me greatly: let's not do that again. Or if we do, let's at least leave a note on the original PR to say what happened. (Now done for MSC1946.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants