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

{Signing,Verifying}KeyVisitor: visit_borrowed_bytes -> visit_bytes #602

Merged
merged 1 commit into from
Nov 17, 2023
Merged

{Signing,Verifying}KeyVisitor: visit_borrowed_bytes -> visit_bytes #602

merged 1 commit into from
Nov 17, 2023

Conversation

awesterb
Copy link
Contributor

This is a slight improvement to the deserialisation code for SigningKey and VerifyingKey.

The default implementations of visit_borrowed_bytes and visit_byte_buf forward to visit_bytes, but only visit_borrowed_bytes was implemented for the {Signing,Verifying}KeyVisitor. This means that {Signing,Verifying}Keys can only be deserialised from borrowed byte arrays, but not from owned or transient byte arrays. Example:

let bytes = base16ct::lower::decode_vec(
    "66b1419fae979516fb3807dda1b05026b2570a7ab2190254e524af4f0934ddd2",
)
.unwrap();
    
let d = serde::de::value::BorrowedBytesDeserializer::<serde::de::value::Error>::new(&bytes);
ed25519_dalek::VerifyingKey::deserialize(d).unwrap(); // deserialising from a borrowed byte array works
    
let d = serde::de::value::BytesDeserializer::<serde::de::value::Error>::new(&bytes);
ed25519_dalek::VerifyingKey::deserialize(d).unwrap_err(); // but not from a transient byte array

Implementing visit_bytes instead of visit_borrowed_bytes enables deserialisation from all three byte array types.

@rozbb
Copy link
Contributor

rozbb commented Nov 16, 2023

Thanks! This makes sense. The only reason we'd implement visit_borrowed_bytes separately is if we had a zero-copy deserialization routine for these keys. But all of them copy, so there's no special case here.

@rozbb rozbb merged commit a2ff6ba into dalek-cryptography:main Nov 17, 2023
14 checks passed
@robjtede
Copy link
Contributor

robjtede commented Jan 19, 2024 •
edited
Loading

Worth mentioning that this same issue was just filed over on the old repo (dalek-cryptography/ed25519-dalek#315) and this fix hasn't been released yet.

mikelodder7 pushed a commit to mikelodder7/curve25519-dalek-ml that referenced this pull request Feb 17, 2024
@awesterb @mikelodder7
{Signing,Verifying}KeyVisitor: visit_borrowed_bytes -> visit_bytes (d…
5f33af9 
…alek-cryptography#602)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@awesterb @rozbb @robjtede