Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add option to enable encryption by default for new rooms #7639

Merged
merged 10 commits into from
Jun 10, 2020

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Jun 4, 2020

Fixes #2431 (however, I'm not sure from the issue whether it was desired to have this option affect public rooms)

Adds config option encryption_enabled_by_default_for_room_type, which determines whether encryption should be enabled with the default encryption algorithm in private or public rooms upon creation. Whether the room is private or public is decided based upon the room creation preset that is used.

Part of this PR is also pulling out all of the individual instances of m.megolm.v1.aes-sha2 into a constant variable to eliminate typos ala #7637

Based on #7637

@anoadragon453 anoadragon453 requested a review from a team June 4, 2020 17:57
@dklimpel
Copy link
Contributor

dklimpel commented Jun 4, 2020

Is it possible to add a second switch to have also an option to enable encryption by default for public rooms?

@anoadragon453
Copy link
Member Author

Is it possible to add a second switch to have also an option to enable encryption by default for public rooms?

Good call - I could see a homeserver wanting all rooms encrypted.

@clokep
Copy link
Member

clokep commented Jun 4, 2020

@anoadragon453 I think this could use a rebase now that the other PR has been merged?

@anoadragon453 anoadragon453 removed the request for review from a team June 5, 2020 12:53
@anoadragon453 anoadragon453 force-pushed the anoa/e2e_default_config branch 4 times, most recently from 84beb13 to ee77f2c Compare June 5, 2020 14:04
@anoadragon453 anoadragon453 requested a review from a team June 5, 2020 14:15
richvdh
richvdh previously requested changes Jun 5, 2020
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've got some other bikesheds to paint here, but before we get there, I think there is a problem:

as noted in https://github.com/matrix-org/matrix-doc/issues/2612#issuecomment-639577866, by basing this on the presets, we're not applying it to what other people seem to mean by "public room" :/.

docs/sample_config.yaml Outdated Show resolved Hide resolved
docs/sample_config.yaml Outdated Show resolved Hide resolved
docs/sample_config.yaml Outdated Show resolved Hide resolved
docs/sample_config.yaml Outdated Show resolved Hide resolved
@anoadragon453
Copy link
Member Author

@richvdh Fun, but thanks for noting down the current situation, this PR could hopefully be a step in the right direction then rather than one in the opposite. The reason this one initially went for presets was due to it being based off the dinsic PR which did the same thing.

So I suppose we want to follow Riot's lead and base things on the join rules? That doesn't sound too bad in theory :)

@anoadragon453
Copy link
Member Author

After discussing with @richvdh and @ara4n on matrix, we plan to move forward with using presets to determine what defines a "public" room vs. a "private" one, which is what this PR currently does.

More context in https://github.com/matrix-org/matrix-doc/issues/2612

synapse/handlers/room.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/room.py 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.

lgtm. couple of additional thoughts

synapse/config/room.py Outdated Show resolved Hide resolved
changelog.d/7639.feature Outdated Show resolved Hide resolved
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@anoadragon453 anoadragon453 changed the title Add option to enable encryption by default for private rooms Add option to enable encryption by default for new rooms Jun 10, 2020
@anoadragon453 anoadragon453 merged commit fcd6961 into develop Jun 10, 2020
@anoadragon453 anoadragon453 deleted the anoa/e2e_default_config branch June 10, 2020 16:44
@Bun-Bun
Copy link

Bun-Bun commented Jun 15, 2020

Biggest requested option since Riot turned on forced e2ee is the ability to disable e2ee in Synapse. Why wasn't that considered in this change?

This PR is much newer than all the others requesting the ability to fully disable e2ee.

babolivier added a commit that referenced this pull request Jul 11, 2020
Fixes #7821, introduced in #7639

Turns out PyYAML translates `off` into a `False` boolean if it's
unquoted (see https://stackoverflow.com/questions/36463531/pyyaml-automatically-converting-certain-keys-to-boolean-values),
which seems to be a liberal interpretation of this bit of the YAML spec: https://yaml.org/spec/1.1/current.html#id864510

An alternative fix would be to implement the solution mentioned in the
SO post linked above, but I'm aware it might break existing setups
(which might use these values in the configuration file) so it's
probably better just to add an extra check for this one. We should be
aware that this is a thing for the next times we do that though.

I didn't find any other occurrence of this bug elsewhere in the
codebase.
babolivier added a commit that referenced this pull request Jul 13, 2020
…7822)

Fixes #7821, introduced in #7639

Turns out PyYAML translates `off` into a `False` boolean if it's
unquoted (see https://stackoverflow.com/questions/36463531/pyyaml-automatically-converting-certain-keys-to-boolean-values),
which seems to be a liberal interpretation of this bit of the YAML spec: https://yaml.org/spec/1.1/current.html#id864510

An alternative fix would be to implement the solution mentioned in the
SO post linked above, but I'm aware it might break existing setups
(which might use these values in the configuration file) so it's
probably better just to add an extra check for this one. We should be
aware that this is a thing for the next times we do that though.

I didn't find any other occurrence of this bug elsewhere in the
codebase.
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '03619324f':
  Create a ListenerConfig object (#7681)
  Fix changelog wording
  1.15.1
  Wrap register_device coroutine in an ensureDeferred (#7684)
  Ensure the body is a string before comparing push rules. (#7701)
  Ensure etag is a string for GET room_keys/version response (#7691)
  Update m.id.phone to use 'phone' instead of 'number' (#7687)
  Fix "There was no active span when trying to log." error (#7698)
  Enable 3PID add/bind/unbind endpoints on r0 routes
  Discard RDATA from already seen positions. (#7648)
  Replace iteritems/itervalues/iterkeys with native versions. (#7692)
  Fix warnings about losing log context during UI auth. (#7688)
  Fix a typo when comparing the URI & method during UI Auth. (#7689)
  Remove "user_id" from GET /presence. (#7606)
  Increase the default SAML session expirary time to 15 minutes. (#7664)
  fix typo in sample_config.yaml (#7652)
  Take out a lock before modifying _CACHES (#7663)
  Add option to enable encryption by default for new rooms (#7639)
  Clean-up the fallback login code. (#7657)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

configuration flag to enable encryption by default in new rooms
6 participants