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

feature(crypto): Add support for master key local pinning #3639

Merged
merged 11 commits into from
Aug 2, 2024

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Jul 2, 2024

part of invisible crypto, follow up of #3607
Alternative fix for #3564
Alternative PR #3610

Add the capability to locally pin a public MSK for a ReadOnlyUserIdentity.
The first time an identity is seen for a user, the msk is pinned. Pin violation will be reported when an identity is rotated.

This PR only adds support for pinning, support for serialization/migration, persistance.
This could be used later by other PRs to report specific errors or show pinning violation to users.

Note about verification and pinning.
As part of this PR, if a new identity is detected it will still be seen as a pinning violation even if the new identity is signed by our usk. But the UserIdentity::has_identity_mismatch() will be ok. There is a pinning violation, but the new identity is verified and verification has priority.
That is to say that there is no auto-pinning if verified so far. To be discussed if we want it later

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@BillCarsonFr BillCarsonFr requested a review from richvdh July 2, 2024 13:14
@BillCarsonFr BillCarsonFr requested a review from dkasak July 2, 2024 13:15
@BillCarsonFr BillCarsonFr force-pushed the valere/invisible_crypto/identity_local_tofu_2 branch 2 times, most recently from 409ef16 to 40b1e75 Compare July 3, 2024 07:44
Base automatically changed from valere/invisible_crypto/indentity_base_sharing to main July 3, 2024 14:18
@BillCarsonFr BillCarsonFr force-pushed the valere/invisible_crypto/identity_local_tofu_2 branch from 40b1e75 to 401df94 Compare July 3, 2024 16:11
@BillCarsonFr BillCarsonFr marked this pull request as ready for review July 3, 2024 16:12
@BillCarsonFr BillCarsonFr requested a review from a team as a code owner July 3, 2024 16:12
@BillCarsonFr BillCarsonFr requested review from poljar and removed request for a team July 3, 2024 16:12
@BillCarsonFr BillCarsonFr force-pushed the valere/invisible_crypto/identity_local_tofu_2 branch from 401df94 to e701342 Compare July 3, 2024 16:55
Copy link

codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 95.55556% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.98%. Comparing base (925c5b2) to head (42aae6a).
Report is 77 commits behind head on main.

Files Patch % Lines
crates/matrix-sdk-crypto/src/identities/user.rs 95.55% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3639      +/-   ##
==========================================
- Coverage   84.02%   83.98%   -0.05%     
==========================================
  Files         260      260              
  Lines       26712    26739      +27     
==========================================
+ Hits        22446    22456      +10     
- Misses       4266     4283      +17     

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

@BillCarsonFr BillCarsonFr force-pushed the valere/invisible_crypto/identity_local_tofu_2 branch 2 times, most recently from 421bdc7 to bb7383d Compare July 4, 2024 19:36
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.

This looks broadly sensible to me, though I'd defer to @poljar on this.

There are a bunch of documentation nits I'd like to see cleaned up, but let's agree on the basic structure first

crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

I think the approach looks good, I left a bunch of nits though. I'll let @dkasak double check things.

crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
testing/matrix-sdk-test/src/test_json/keys_query_sets.rs Outdated Show resolved Hide resolved
testing/matrix-sdk-test/src/test_json/keys_query_sets.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/manager.rs Outdated Show resolved Hide resolved
@BillCarsonFr BillCarsonFr force-pushed the valere/invisible_crypto/identity_local_tofu_2 branch from daf7331 to 1d4cefb Compare July 22, 2024 15:29
@BillCarsonFr BillCarsonFr force-pushed the valere/invisible_crypto/identity_local_tofu_2 branch from 1b1fb3f to de60e6e Compare July 22, 2024 15:47
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Left a tiny nit, but I think that this is fine.

crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
Copy link
Member

@dkasak dkasak 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 left a bunch of wording suggestions and would advise changing the has_identity_mismatch method name to identity_needs_user_approval as discussed, but otherwise this seems good to me, so leaving an Approve for when suggestions are handled.

crates/matrix-sdk-crypto/src/identities/manager.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/manager.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/manager.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/identities/user.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants