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

refactor!: Room's Key ID generation #33329

Merged
merged 18 commits into from
Oct 2, 2024

Conversation

KevLehman
Copy link
Contributor

@KevLehman KevLehman commented Sep 20, 2024

Proposed changes (including videos or screenshots)

Our current way of generating the e2eRoomKeyId is flawed. Our current steps are:

  • Generate a CryptoKey (AES)
  • Export CryptoKey using exportJWKKey, this produces an object similar to this:
{
    "alg": "A128CBC",
    "ext": true,
    "k": "6RkYqlDAx_-xxxxxxxxxx",
    "key_ops": [
        "encrypt",
        "decrypt"
    ],
    "kty": "oct"
}
  • We stringify the object
  • We convert the stringified to base64
  • Then, we take an slice of 12 chars.

Since after stringify/encoding the object above, the first 12 chars would be the string {"alg": "A which encoded is eyJhbGciOiJB, and then this is defined as the "room key id"

We currently don't use this value for anything that would require it to be random or "secure". We just use it to "identify" the key. However, since a room only has 1 key, it didn't matter if a key was actually "different", system would think it was "the key" for the room.

This would cause the client to try to decrypt the message and failing (if the key was actually different) as all the keys produced the same identifier.

This PR introduces a change in this keyID mechanism, creating an SHA-256 hash of the exported session key, and then taking the first 12 characters of the hash. Current rooms with current keys will continue to work as they should, as we now will use the keyID stored in the room object instead of calculating it from the key everytime.

Issue(s)

https://rocketchat.atlassian.net/browse/E2EE2-68

Steps to test or reproduce

Further comments

Copy link

changeset-bot bot commented Sep 20, 2024

🦋 Changeset detected

Latest commit: f7d8aeb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 33 packages
Name Type
@rocket.chat/meteor Major
@rocket.chat/core-typings Major
@rocket.chat/rest-typings Major
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/gazzodown Major
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Major
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/network-broker Patch
@rocket.chat/models Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-client Major
@rocket.chat/ui-video-conf Major
@rocket.chat/web-ui-registration Major
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

dionisio-bot bot commented Sep 20, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

Copy link
Contributor

github-actions bot commented Sep 20, 2024

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-10-02 16:19 UTC

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.32%. Comparing base (bf02ce5) to head (f7d8aeb).
Report is 11 commits behind head on release-7.0.0.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff               @@
##           release-7.0.0   #33329   +/-   ##
==============================================
  Coverage          75.32%   75.32%           
==============================================
  Files                383      383           
  Lines              19383    19383           
  Branches            4980     4980           
==============================================
  Hits               14601    14601           
  Misses              4212     4212           
  Partials             570      570           
Flag Coverage Δ
unit 75.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@KevLehman KevLehman changed the base branch from develop to release-7.0.0 September 23, 2024 16:19
@KevLehman KevLehman force-pushed the feat/e2e-key-id-randomization branch 3 times, most recently from fab0eda to 528f198 Compare September 23, 2024 16:29
@KevLehman KevLehman marked this pull request as ready for review September 24, 2024 18:03
@KevLehman KevLehman added this to the 7.0 milestone Sep 24, 2024
@ggazzo ggazzo requested review from a team as code owners September 25, 2024 00:27
@KevLehman KevLehman force-pushed the feat/e2e-key-id-randomization branch from 989ebd9 to 0ba5073 Compare September 25, 2024 13:39
julio-cfa
julio-cfa previously approved these changes Sep 25, 2024
@ggazzo ggazzo dismissed julio-cfa’s stale review September 25, 2024 16:15

The merge-base changed after approval.

@KevLehman KevLehman force-pushed the feat/e2e-key-id-randomization branch from 0323e70 to 08cc7d8 Compare September 25, 2024 16:22
@KevLehman KevLehman requested a review from julio-cfa September 25, 2024 16:22
@KevLehman KevLehman changed the title refactor: Randomize e2eKeyId instead of derive it from encoded key refactor!: Randomize e2eKeyId instead of derive it from encoded key Oct 1, 2024
Copy link
Contributor

@hugocostadev hugocostadev left a comment

Choose a reason for hiding this comment

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

Congrats @KevLehman every problem that we discussed in not related with this code.

@KevLehman KevLehman added the stat: QA assured Means it has been tested and approved by a company insider label Oct 2, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Oct 2, 2024
@KevLehman KevLehman changed the title refactor!: Randomize e2eKeyId instead of derive it from encoded key refactor!: Room's Key ID generation Oct 2, 2024
@KevLehman KevLehman merged commit 020deec into release-7.0.0 Oct 2, 2024
54 checks passed
@KevLehman KevLehman deleted the feat/e2e-key-id-randomization branch October 2, 2024 16:19
ggazzo added a commit that referenced this pull request Oct 8, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
sampaiodiego pushed a commit that referenced this pull request Oct 9, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
sampaiodiego pushed a commit that referenced this pull request Oct 9, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
abhinavkrin pushed a commit that referenced this pull request Oct 11, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
ggazzo added a commit that referenced this pull request Oct 11, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
ggazzo added a commit that referenced this pull request Oct 11, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
ggazzo added a commit that referenced this pull request Oct 11, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
ggazzo added a commit that referenced this pull request Oct 11, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
ggazzo added a commit that referenced this pull request Oct 14, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
MartinSchoeler pushed a commit that referenced this pull request Oct 14, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
ggazzo added a commit that referenced this pull request Oct 15, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
ggazzo added a commit that referenced this pull request Oct 17, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
ggazzo added a commit that referenced this pull request Oct 17, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
ggazzo added a commit that referenced this pull request Oct 17, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
This was referenced Oct 20, 2024
abhinavkrin pushed a commit that referenced this pull request Oct 25, 2024
Co-authored-by: Hugo Costa <hugocarreiracosta@gmail.com>
Co-authored-by: Guilherme Gazzo <guilhermegazzo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants