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

MSC 1659 Proposal: Change Event IDs to Hashes #1659

Merged
merged 16 commits into from
Jan 30, 2019

Conversation

erikjohnston
Copy link
Member

@erikjohnston erikjohnston commented Sep 5, 2018

PR for #1640 MSC has been renumbered to match this PR number

Rendered

@erikjohnston erikjohnston requested a review from a team September 5, 2018 08:37
@turt2live turt2live added this to In review (just the PRs) in August 2018 r0 via automation Sep 5, 2018
@ara4n
Copy link
Member

ara4n commented Sep 5, 2018

@jevolk is your point that the eventId param may be redundant given it's now determinable from the event?

@jevolk
Copy link
Contributor

jevolk commented Sep 5, 2018

@ara4n No. This is just an area of the spec which complicates event_id-as-hashes because the payload is modified by two parties as the target server's signature is added. This has to be considered when specifying the behavior of the invite process. The target server might not be online when the process is initiated. Can an invite be made when the target server is not online? If not, why is it such a big deal to even have the target's signature before persisting the event both locally and broadcasting to the room DAG. Is that signature necessary? These are rhetorical questions at this point but the point is that this area needs to be reconsidered.

proposals/1640-event-id-as-hashes.md Outdated Show resolved Hide resolved
proposals/1640-event-id-as-hashes.md Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I note that the reference hash does not currently include the signatures on the event. I assume you're proposing to maintain that behaviour? I don't think it causes any security problems, but it might be a good time to revisit it?

@neilisfragile neilisfragile moved this from To Do to In Progress: Planned Project Work in Superceded by https://github.com/orgs/matrix-org/projects/8 Sep 21, 2018
@richvdh richvdh removed this from In Progress: Planned Project Work in Superceded by https://github.com/orgs/matrix-org/projects/8 Sep 28, 2018
@erikjohnston erikjohnston self-assigned this Oct 2, 2018
@erikjohnston
Copy link
Member Author

This is just an area of the spec which complicates event_id-as-hashes because the payload is modified by two parties as the target server's signature is added

We should probably spell out clearly that signature fields aren't included in the hash. (The protocol assumes in several places that adding signatures don't break hashes).

@erikjohnston
Copy link
Member Author

I note that the reference hash does not currently include the signatures on the event. I assume you're proposing to maintain that behaviour? I don't think it causes any security problems, but it might be a good time to revisit it?

Yup. I'm not particularly keen to change that behaviour as it feels reasonable to be able to add signatures without changing the ID of the event tbh.

proposals/1640-event-id-as-hashes.md Outdated Show resolved Hide resolved
proposals/1640-event-id-as-hashes.md Outdated Show resolved Hide resolved
proposals/1640-event-id-as-hashes.md Outdated Show resolved Hide resolved
proposals/1640-event-id-as-hashes.md Outdated Show resolved Hide resolved
proposals/1640-event-id-as-hashes.md Outdated Show resolved Hide resolved
proposals/1640-event-id-as-hashes.md Outdated Show resolved Hide resolved
@anoadragon453 anoadragon453 added proposal A matrix spec change proposal and removed T-Core labels Jan 4, 2019
Copy link
Member

@uhoreg uhoreg left a comment

Choose a reason for hiding this comment

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

looks reasonable, with just a couple minor issues

proposals/1640-event-id-as-hashes.md Outdated Show resolved Hide resolved
proposals/1640-event-id-as-hashes.md Outdated Show resolved Hide resolved
Co-Authored-By: erikjohnston <erikj@jki.re>
event to clients, however.

Note that the `auth_events` and `prev_events` fields aren't sent to clients, and
so the changes proposed above won't affect clients.
Copy link
Member

Choose a reason for hiding this comment

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

unless they use the federation event format, but then they're on their own anyways.

proposals/1659-event-id-as-hashes.md Show resolved Hide resolved
proposals/1659-event-id-as-hashes.md Show resolved Hide resolved
@erikjohnston
Copy link
Member Author

Heads up that I've updated this MSC to include making a room v3

@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 Jan 25, 2019
@mscbot
Copy link
Collaborator

mscbot commented Jan 25, 2019

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

@neilisfragile
Copy link
Contributor

Room v3 should be considered a stable room version - I've made this explicit on the proposal.

proposals/1659-event-id-as-hashes.md Outdated Show resolved Hide resolved
Co-Authored-By: erikjohnston <erikj@jki.re>
@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 Jan 30, 2019
@mscbot
Copy link
Collaborator

mscbot commented Jan 30, 2019

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

2 similar comments
@mscbot
Copy link
Collaborator

mscbot commented Jan 30, 2019

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

@mscbot
Copy link
Collaborator

mscbot commented Jan 30, 2019

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

@erikjohnston erikjohnston merged commit 1c0742e into master Jan 30, 2019
turt2live added a commit that referenced this pull request Jan 31, 2019
Original proposal: #1659
Implementation proofs (some traversing of the PR tree may be required to get all of them):
* matrix-org/synapse#4483
* matrix-org/synapse#4499

This doesn't intentionally change anything from the proposal.

**Implementation details**:

The simple part of this is the introduction of a rooms/v3.html document. The somewhat unclear part is the stuff done to the s2s definitions. This pulls `unsigned_pdu` out to `unsigned_pdu_base` (all fields except `event_id`) where it can be reused in `pdu` and `pdu_v3` (for rooms v3). These definitions are further moved into the room version specifications where they can highlight the exact schemas in detail.

Version 1 has been updated to include the pre-existing event format, however the core principles of the room have not been changed. The same applies to room version 2. Room versions have immutable core principles once in the spec, otherwise these format changes would land in a pre-existing version.

The client-server API event formats will need updating, however that is being punted to a different commit to try and keep these changes reviewable.
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed finished-final-comment-period labels Jan 31, 2019
@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Feb 1, 2019
@turt2live
Copy link
Member

merged via #1828 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal proposal-pr
Projects
No open projects
August 2018 r0
  
In review (just the PRs)
Development

Successfully merging this pull request may close these issues.

None yet