-
Notifications
You must be signed in to change notification settings - Fork 265
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
fix(crypto): Serialize sender data msk in base64 instead of numbers #4450
Conversation
01bf4ba
to
4e6c060
Compare
5496d77
to
3c94504
Compare
pub master_key: Box<Ed25519PublicKey>, | ||
} | ||
|
||
/// In an initial version the master key was serialized as an array of number, | ||
/// it is now exported in base64. This code adds backward compatibility. | ||
pub(crate) fn deserialize_sender_msk_base64_or_array<'de, D>( |
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 am not familiar with this serde visitor pattern. Let me know if it is sane or if there is a prefferred method? I am not sure about the error mappings
Filed #4459 about the test failure |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4450 +/- ##
==========================================
- Coverage 85.12% 85.09% -0.04%
==========================================
Files 283 283
Lines 31775 31790 +15
==========================================
+ Hits 27048 27051 +3
- Misses 4727 4739 +12 ☔ View full report in Codecov by Sentry. |
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 good to me, and the error mappings look about right.
@poljar should Edd25519PublicKey serialize as base64 by default?
No, only if we serialize into JSON. |
To expand on this, I think this PR is fine, but we don't want to move the base64 serialization into vodozemac, where |
3c94504
to
5ff556f
Compare
Follow up of #4449
While adding snapshot test, I detected that sender's data
KnownSenderData
was serializing the cross-signing master key as an array of number. This PR changes that and now serialize the key as a base64 string, adding migration for the old format.Signed-off-by: