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.

Sign up for GitHub

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

Jump to bottom

crypto: Error when sending keys to previously-verified users with identity-based strategy #3896

Merged
merged 6 commits into from
Sep 3, 2024
Merged

crypto: Error when sending keys to previously-verified users with identity-based strategy #3896

merged 6 commits into from
Sep 3, 2024

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Aug 27, 2024 •
edited
Loading

This is the rest of #3662, on top of #3884.

3662 says:

Part of invisible crypto, follow up of #3639

Fixes #3565

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@uhoreg
Copy link
Member Author

uhoreg commented Aug 27, 2024

Compared with 3662, this uses has_identity_verification_violation instead of its own calculation for whether the sender was previously verified, with the end result that it no longer looks at the pinned master signing key. I think that this is fine (as a result of a decision that we change the behaviour), but I think it also means that the pinning code is now unused. I think we need to double-check whether that's the case, and if so, possibly remove it.

@uhoreg uhoreg mentioned this pull request Aug 27, 2024
Merged
@codecov Codecov
Copy link

codecov bot commented Aug 27, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.09%. Comparing base (468ee53) to head (7324c3f).
Report is 68 commits behind head on main.

Files with missing lines Patch % Lines
...c/session_manager/group_sessions/share_strategy.rs 83.33% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3896      +/-   ##
==========================================
+ Coverage   84.07%   84.09%   +0.01%     
==========================================
  Files         266      266              
  Lines       27861    27872      +11     
==========================================
+ Hits        23425    23438      +13     
+ Misses       4436     4434       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@uhoreg uhoreg force-pushed the invisible_crypto/error_sending_to_previously_verified branch from 36bbe6b to 4be7bf5 Compare August 28, 2024 01:57
@uhoreg uhoreg requested a review from richvdh August 28, 2024 02:10
@uhoreg uhoreg marked this pull request as ready for review August 28, 2024 02:10
@uhoreg uhoreg requested review from a team as code owners August 28, 2024 02:10
richvdh
richvdh requested changes Aug 28, 2024
View reviewed changes
crates/matrix-sdk-crypto/src/error.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/error.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/error.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs Show resolved Hide resolved
crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs Show resolved Hide resolved
crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs Outdated Show resolved Hide resolved
bindings/matrix-sdk-ffi/CHANGELOG.md Outdated
Comment on lines 5 to 12
- `EventSendState` now has four additional variants: `VerifiedUserHasUnsignedDevice`,
`VerifiedUserChangedIdentity`, `CrossSigningNotSetup`, and
`SendingFromUnverifiedDevice`. The first two reflect problems with verified users in
the room and as such can only be returned when the room key recipient strategy has
`error_on_verified_user_problem` set, or when using the identity-based strategy. The
last two indicate that your own device is not properly cross-signed, which is a
requirement when using the identity-based strategy, and can only be returned when
using the identity-based strategy.
Copy link
Member

Choose a reason for hiding this comment

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

erm, we need to make new changelog entries, not just modify old changelogs that apply to old versions that are already built into applications.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I wasn't sure about this, because it's under the "unreleased" section, so wasn't sure if it was better to modify the existing entry, or have two separate entries that are related to the same area.

Copy link
Member

Choose a reason for hiding this comment

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

unfortunately everything is "unreleased" at the moment :(

@richvdh richvdh changed the title Error when sending keys to previously-verified with identity-based strategy Error when sending keys to previously-verified users with identity-based strategy Aug 28, 2024
@uhoreg
Copy link
Member Author

uhoreg commented Aug 31, 2024

I think that I've addressed all of the comments. I can re-write history if/when needed.

@uhoreg uhoreg changed the title Error when sending keys to previously-verified users with identity-based strategy crypto: Error when sending keys to previously-verified users with identity-based strategy Aug 31, 2024
@richvdh richvdh self-requested a review September 2, 2024 10:33
@richvdh richvdh mentioned this pull request Sep 2, 2024
Closed
1 task
richvdh
richvdh approved these changes Sep 2, 2024
View reviewed changes
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 otherwise

crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs Outdated
Comment on lines 1325 to 1326
.other()
.unwrap()
Copy link
Member

Choose a reason for hiding this comment

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

think this is redundant. (And likewise below)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@BillCarsonFr
Copy link
Member

but I think it also means that the pinning code is now unused. I think we need to double-check whether that's the case, and if so, possibly remove it.

It will be needed at least to display the warning notice that an identity has changed element-hq/element-meta#2513

@uhoreg uhoreg force-pushed the invisible_crypto/error_sending_to_previously_verified branch from 6c3d7b4 to 2e0c829 Compare September 3, 2024 15:15
@uhoreg uhoreg force-pushed the invisible_crypto/error_sending_to_previously_verified branch from 2e0c829 to 7324c3f Compare September 3, 2024 15:20
@uhoreg
Copy link
Member Author

uhoreg commented Sep 3, 2024

@richvdh can you hit the merge button for me?

@richvdh richvdh merged commit 1dd8c90 into matrix-org:main Sep 3, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richvdh richvdh richvdh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Invisible Crypto | Share Room Keys: Update the Identity Based sharing strategy to use the tofu_trusted flag
3 participants
@uhoreg @BillCarsonFr @richvdh