Migrate from protoc to quick-protobuf #3066
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,7 @@ pub mod error; | |
|
|
||
| use self::error::*; | ||
| use crate::{keys_proto, PeerId}; | ||
| use std::borrow::Cow; | ||
| use std::convert::{TryFrom, TryInto}; | ||
|
|
||
| /// Identity keypair of a node. | ||
|
|
@@ -147,12 +148,10 @@ impl Keypair { | |
|
|
||
| /// Encode a private key as protobuf structure. | ||
| pub fn to_protobuf_encoding(&self) -> Result<Vec<u8>, DecodingError> { | ||
| let pk = match self { | ||
| Self::Ed25519(data) => keys_proto::PrivateKey { | ||
| Type: keys_proto::KeyType::Ed25519, | ||
| Data: Cow::from(data.encode().to_vec()), | ||
| }, | ||
| #[cfg(all(feature = "rsa", not(target_arch = "wasm32")))] | ||
| Self::Rsa(_) => return Err(DecodingError::encoding_unsupported("RSA")), | ||
|
|
@@ -162,35 +161,31 @@ impl Keypair { | |
| Self::Ecdsa(_) => return Err(DecodingError::encoding_unsupported("ECDSA")), | ||
| }; | ||
|
|
||
| Ok(quick_protobuf::serialize_into_vec(&pk).expect("Encoding to succeed.")) | ||
| } | ||
|
|
||
| /// Decode a private key from a protobuf structure and parse it as a [`Keypair`]. | ||
| pub fn from_protobuf_encoding(bytes: &[u8]) -> Result<Keypair, DecodingError> { | ||
| let mut private_key = | ||
| quick_protobuf::deserialize_from_slice::<keys_proto::PrivateKey>(bytes) | ||
| .map_err(|e| DecodingError::bad_protobuf("private key bytes", e)) | ||
| .map(zeroize::Zeroizing::new)?; | ||
|
|
||
| match private_key.Type { | ||
| keys_proto::KeyType::Ed25519 => { | ||
| ed25519::Keypair::decode(private_key.Data.to_mut()).map(Keypair::Ed25519) | ||
| } | ||
| keys_proto::KeyType::RSA => Err(DecodingError::decoding_unsupported("RSA")), | ||
| keys_proto::KeyType::Secp256k1 => Err(DecodingError::decoding_unsupported("secp256k1")), | ||
| keys_proto::KeyType::ECDSA => Err(DecodingError::decoding_unsupported("ECDSA")), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl zeroize::Zeroize for keys_proto::PrivateKey<'_> { | ||
| fn zeroize(&mut self) { | ||
| self.Type = keys_proto::KeyType::RSA; | ||
| self.Data.to_mut().zeroize(); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -232,23 +227,15 @@ impl PublicKey { | |
| /// Encode the public key into a protobuf structure for storage or | ||
| /// exchange with other nodes. | ||
| pub fn to_protobuf_encoding(&self) -> Vec<u8> { | ||
| let public_key = keys_proto::PublicKey::from(self); | ||
|
|
||
| quick_protobuf::serialize_into_vec(&public_key).expect("Encoding to succeed.") | ||
| } | ||
|
|
||
| /// Decode a public key from a protobuf structure, e.g. read from storage | ||
| /// or received from another node. | ||
| pub fn from_protobuf_encoding(bytes: &[u8]) -> Result<PublicKey, DecodingError> { | ||
| let pubkey: keys_proto::PublicKey = quick_protobuf::deserialize_from_slice(bytes) | ||
| .map_err(|e| DecodingError::bad_protobuf("public key bytes", e))?; | ||
|
|
||
| pubkey.try_into() | ||
|
|
@@ -260,67 +247,64 @@ impl PublicKey { | |
| } | ||
| } | ||
|
|
||
| impl From<&PublicKey> for keys_proto::PublicKey<'_> { | ||
| fn from(key: &PublicKey) -> Self { | ||
| match key { | ||
| PublicKey::Ed25519(key) => keys_proto::PublicKey { | ||
| Type: keys_proto::KeyType::Ed25519, | ||
| Data: Cow::from(key.encode().to_vec()), | ||
|
There was a problem hiding this comment. I think the usage of There was a problem hiding this comment. Seem like There was a problem hiding this comment. You should be able to add |
||
| }, | ||
| #[cfg(all(feature = "rsa", not(target_arch = "wasm32")))] | ||
| PublicKey::Rsa(key) => keys_proto::PublicKey { | ||
| Type: keys_proto::KeyType::RSA, | ||
| Data: Cow::from(key.encode_x509()), | ||
| }, | ||
| #[cfg(feature = "secp256k1")] | ||
| PublicKey::Secp256k1(key) => keys_proto::PublicKey { | ||
| Type: keys_proto::KeyType::Secp256k1, | ||
| Data: Cow::from(key.encode().to_vec()), | ||
| }, | ||
| #[cfg(feature = "ecdsa")] | ||
| PublicKey::Ecdsa(key) => keys_proto::PublicKey { | ||
| Type: keys_proto::KeyType::ECDSA, | ||
| Data: Cow::from(key.encode_der()), | ||
| }, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<keys_proto::PublicKey<'_>> for PublicKey { | ||
| type Error = DecodingError; | ||
|
|
||
| fn try_from(pubkey: keys_proto::PublicKey) -> Result<Self, Self::Error> { | ||
| match pubkey.Type { | ||
| keys_proto::KeyType::Ed25519 => { | ||
| ed25519::PublicKey::decode(&pubkey.Data).map(PublicKey::Ed25519) | ||
| } | ||
| #[cfg(all(feature = "rsa", not(target_arch = "wasm32")))] | ||
| keys_proto::KeyType::RSA => { | ||
| rsa::PublicKey::decode_x509(&pubkey.Data).map(PublicKey::Rsa) | ||
| } | ||
| #[cfg(any(not(feature = "rsa"), target_arch = "wasm32"))] | ||
| keys_proto::KeyType::RSA => { | ||
| log::debug!("support for RSA was disabled at compile-time"); | ||
| Err(DecodingError::missing_feature("rsa")) | ||
| } | ||
| #[cfg(feature = "secp256k1")] | ||
| keys_proto::KeyType::Secp256k1 => { | ||
| secp256k1::PublicKey::decode(&pubkey.Data).map(PublicKey::Secp256k1) | ||
| } | ||
| #[cfg(not(feature = "secp256k1"))] | ||
| keys_proto::KeyType::Secp256k1 => { | ||
| log::debug!("support for secp256k1 was disabled at compile-time"); | ||
| Err(DecodingError::missing_feature("secp256k1")) | ||
| } | ||
| #[cfg(feature = "ecdsa")] | ||
| keys_proto::KeyType::ECDSA => { | ||
| ecdsa::PublicKey::decode_der(&pubkey.Data).map(PublicKey::Ecdsa) | ||
| } | ||
| #[cfg(not(feature = "ecdsa"))] | ||
| keys_proto::KeyType::ECDSA => { | ||
| log::debug!("support for ECDSA was disabled at compile-time"); | ||
| Err(DecodingError::missing_feature("ecdsa")) | ||
| } | ||
|
|
@@ -349,9 +333,9 @@ mod tests { | |
| #[test] | ||
| fn keypair_from_protobuf_encoding() { | ||
| // E.g. retrieved from an IPFS config file. | ||
| let base_64_encoded = "RAgBEkC+r3SkLs50KLE61uxViPQLfl1+xAX9AZ2S4W+sPjfl0kEnXyJ5enO1ODbsVyZYSnEQZQcPXdLY9nXwSFX+pnDw"; | ||
| let expected_peer_id = | ||
|
Comment on lines
-352
to
-354
There was a problem hiding this comment. Why does this test change? We should not need to change this test. There was a problem hiding this comment. This changes because of this #3066 (comment). I've been thinking about this and feel uneasy about it. Should we instead use this pattern tafia/quick-protobuf#202 (comment)? That way we're consistent with the official encoding. There was a problem hiding this comment.
I think so yes. We need to be compatible here so this test should not need changing! |
||
| PeerId::from_str("16VfNUn8nhtbaQVrQwwBekWYnWNJ4uJRv8pD6T1n1h3CFJr1MMgZM").unwrap(); | ||
|
|
||
| let encoded = base64::decode(base_64_encoded).unwrap(); | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 believe this is infallible by looking at the code. Unfortunately, I have not found a concrete answer in the docs or issues.
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 believe so too. One error I could find is https://docs.rs/quick-protobuf/latest/src/quick_protobuf/writer.rs.html#379 which can't happen if the
Vecis allocated correctly.The other error path comes from lines like https://github.com/tafia/quick-protobuf/blob/d977371e05170a016f03a80512b2a925468e3a1a/quick-protobuf/examples/pb_rs/data_types.rs#L199 which end up being the same error case as above so I think we are good.
I believe this function could actually be made infallible in
quick-protobufbut I am not sure.