-
Notifications
You must be signed in to change notification settings - Fork 260
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
Conversation
409ef16
to
40b1e75
Compare
40b1e75
to
401df94
Compare
401df94
to
e701342
Compare
Codecov ReportAttention: Patch coverage is
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. |
421bdc7
to
bb7383d
Compare
There was a problem hiding this 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
There was a problem hiding this 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.
daf7331
to
1d4cefb
Compare
1b1fb3f
to
de60e6e
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
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
Signed-off-by: