From 194b17f18a65947b8413c8a4817b9726879647ef Mon Sep 17 00:00:00 2001 From: "pinkforest(she/her)" <36498018+pinkforest@users.noreply.github.com> Date: Mon, 19 Dec 2022 07:56:41 +1100 Subject: [PATCH] Fix all Clippy warnings (#244) - Add Clippy to CI - Rename InternalError variants without redundant Error suffix - Rename to_bytes to as_bytes on well known naming - Fix Redundant refs - Fix redundant lifetimes - Fix late declarations --- .github/workflows/rust.yml | 10 +++++++ src/batch.rs | 6 ++-- src/errors.rs | 28 +++++++++--------- src/signature.rs | 12 ++++---- src/signing.rs | 54 ++++++++++++++-------------------- src/verifying.rs | 59 +++++++++++++++++--------------------- tests/ed25519.rs | 6 ++-- 7 files changed, 82 insertions(+), 93 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 6019bcd..ee03f74 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -67,3 +67,13 @@ jobs: - uses: actions/checkout@v3 - uses: dtolnay/rust-toolchain@stable - run: cargo build --benches --features batch + + clippy: + name: Check that clippy is happy + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - uses: dtolnay/rust-toolchain@1.65 + with: + components: clippy + - run: cargo clippy diff --git a/src/batch.rs b/src/batch.rs index 39af1ca..002e2ac 100644 --- a/src/batch.rs +++ b/src/batch.rs @@ -218,7 +218,7 @@ pub fn verify_batch( || signatures.len() != verifying_keys.len() || verifying_keys.len() != messages.len() { - return Err(InternalError::ArrayLengthError { + return Err(InternalError::ArrayLength { name_a: "signatures", length_a: signatures.len(), name_b: "messages", @@ -292,11 +292,11 @@ pub fn verify_batch( once(-B_coefficient).chain(zs.iter().cloned()).chain(zhrams), B.chain(Rs).chain(As), ) - .ok_or(InternalError::VerifyError)?; + .ok_or(InternalError::Verify)?; if id.is_identity() { Ok(()) } else { - Err(InternalError::VerifyError.into()) + Err(InternalError::Verify.into()) } } diff --git a/src/errors.rs b/src/errors.rs index 85b1f64..257399b 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -23,23 +23,23 @@ use std::error::Error; /// need to pay any attention to these. #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] pub(crate) enum InternalError { - PointDecompressionError, - ScalarFormatError, + PointDecompression, + ScalarFormat, /// An error in the length of bytes handed to a constructor. /// /// To use this, pass a string specifying the `name` of the type which is /// returning the error, and the `length` in bytes which its constructor /// expects. - BytesLengthError { + BytesLength { name: &'static str, length: usize, }, /// The verification equation wasn't satisfied - VerifyError, + Verify, /// Two arrays did not match in size, making the called signature /// verification method impossible. #[cfg(any(feature = "batch", feature = "batch_deterministic"))] - ArrayLengthError { + ArrayLength { name_a: &'static str, length_a: usize, name_b: &'static str, @@ -48,22 +48,22 @@ pub(crate) enum InternalError { length_c: usize, }, /// An ed25519ph signature can only take up to 255 octets of context. - PrehashedContextLengthError, + PrehashedContextLength, /// A mismatched (public, secret) key pair. - MismatchedKeypairError, + MismatchedKeypair, } impl Display for InternalError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match *self { - InternalError::PointDecompressionError => write!(f, "Cannot decompress Edwards point"), - InternalError::ScalarFormatError => write!(f, "Cannot use scalar with high-bit set"), - InternalError::BytesLengthError { name: n, length: l } => { + InternalError::PointDecompression => write!(f, "Cannot decompress Edwards point"), + InternalError::ScalarFormat => write!(f, "Cannot use scalar with high-bit set"), + InternalError::BytesLength { name: n, length: l } => { write!(f, "{} must be {} bytes in length", n, l) } - InternalError::VerifyError => write!(f, "Verification equation was not satisfied"), + InternalError::Verify => write!(f, "Verification equation was not satisfied"), #[cfg(any(feature = "batch", feature = "batch_deterministic"))] - InternalError::ArrayLengthError { + InternalError::ArrayLength { name_a: na, length_a: la, name_b: nb, @@ -76,11 +76,11 @@ impl Display for InternalError { {} has length {}, {} has length {}.", na, la, nb, lb, nc, lc ), - InternalError::PrehashedContextLengthError => write!( + InternalError::PrehashedContextLength => write!( f, "An ed25519ph signature can only take up to 255 octets of context" ), - InternalError::MismatchedKeypairError => write!(f, "Mismatched Keypair detected"), + InternalError::MismatchedKeypair => write!(f, "Mismatched Keypair detected"), } } } diff --git a/src/signature.rs b/src/signature.rs index 9026cbb..fdf1700 100644 --- a/src/signature.rs +++ b/src/signature.rs @@ -73,7 +73,7 @@ fn check_scalar(bytes: [u8; 32]) -> Result { // This is compatible with ed25519-donna and libsodium when // -DED25519_COMPAT is NOT specified. if bytes[31] & 224 != 0 { - return Err(InternalError::ScalarFormatError.into()); + return Err(InternalError::ScalarFormat.into()); } Ok(Scalar::from_bits(bytes)) @@ -95,15 +95,15 @@ fn check_scalar(bytes: [u8; 32]) -> Result { } match Scalar::from_canonical_bytes(bytes).into() { - None => return Err(InternalError::ScalarFormatError.into()), - Some(x) => return Ok(x), - }; + None => Err(InternalError::ScalarFormat.into()), + Some(x) => Ok(x), + } } impl InternalSignature { /// Convert this `Signature` to a byte array. #[inline] - pub fn to_bytes(&self) -> [u8; SIGNATURE_LENGTH] { + pub fn as_bytes(&self) -> [u8; SIGNATURE_LENGTH] { let mut signature_bytes: [u8; SIGNATURE_LENGTH] = [0u8; SIGNATURE_LENGTH]; signature_bytes[..32].copy_from_slice(&self.R.as_bytes()[..]); @@ -182,6 +182,6 @@ impl TryFrom<&ed25519::Signature> for InternalSignature { impl From for ed25519::Signature { fn from(sig: InternalSignature) -> ed25519::Signature { - ed25519::Signature::from_bytes(&sig.to_bytes()).unwrap() + ed25519::Signature::from_bytes(&sig.as_bytes()).unwrap() } } diff --git a/src/signing.rs b/src/signing.rs index 0a3ee82..07d5553 100644 --- a/src/signing.rs +++ b/src/signing.rs @@ -124,7 +124,7 @@ impl SigningKey { let verifying_key = VerifyingKey::from_bytes(verifying_key.try_into().unwrap())?; if verifying_key != VerifyingKey::from(&secret_key) { - return Err(InternalError::MismatchedKeypairError.into()); + return Err(InternalError::MismatchedKeypair.into()); } Ok(SigningKey { @@ -300,9 +300,7 @@ impl SigningKey { { let expanded: ExpandedSecretKey = (&self.secret_key).into(); // xxx thanks i hate this - expanded - .sign_prehashed(prehashed_message, &self.verifying_key, context) - .into() + expanded.sign_prehashed(prehashed_message, &self.verifying_key, context) } /// Verify a signature on a message with this signing key's public key. @@ -473,7 +471,7 @@ impl Signer for SigningKey { /// Sign a message with this signing key's secret key. fn try_sign(&self, message: &[u8]) -> Result { let expanded: ExpandedSecretKey = (&self.secret_key).into(); - Ok(expanded.sign(&message, &self.verifying_key).into()) + Ok(expanded.sign(message, &self.verifying_key)) } } @@ -505,7 +503,7 @@ impl TryFrom<&[u8]> for SigningKey { SecretKey::try_from(bytes) .map(|bytes| Self::from_bytes(&bytes)) .map_err(|_| { - InternalError::BytesLengthError { + InternalError::BytesLength { name: "SecretKey", length: SECRET_KEY_LENGTH, } @@ -695,24 +693,20 @@ impl ExpandedSecretKey { #[allow(non_snake_case)] pub(crate) fn sign(&self, message: &[u8], verifying_key: &VerifyingKey) -> ed25519::Signature { let mut h: Sha512 = Sha512::new(); - let R: CompressedEdwardsY; - let r: Scalar; - let s: Scalar; - let k: Scalar; - h.update(&self.nonce); - h.update(&message); + h.update(self.nonce); + h.update(message); - r = Scalar::from_hash(h); - R = (&r * &ED25519_BASEPOINT_TABLE).compress(); + let r = Scalar::from_hash(h); + let R: CompressedEdwardsY = (&r * &ED25519_BASEPOINT_TABLE).compress(); h = Sha512::new(); h.update(R.as_bytes()); h.update(verifying_key.as_bytes()); - h.update(&message); + h.update(message); - k = Scalar::from_hash(h); - s = &(&k * &self.key) + &r; + let k = Scalar::from_hash(h); + let s: Scalar = (k * self.key) + r; InternalSignature { R, s }.into() } @@ -749,17 +743,11 @@ impl ExpandedSecretKey { { let mut h: Sha512; let mut prehash: [u8; 64] = [0u8; 64]; - let R: CompressedEdwardsY; - let r: Scalar; - let s: Scalar; - let k: Scalar; let ctx: &[u8] = context.unwrap_or(b""); // By default, the context is an empty string. if ctx.len() > 255 { - return Err(SignatureError::from( - InternalError::PrehashedContextLengthError, - )); + return Err(SignatureError::from(InternalError::PrehashedContextLength)); } let ctx_len: u8 = ctx.len() as u8; @@ -781,26 +769,26 @@ impl ExpandedSecretKey { // still bleeding from malleability, for fuck's sake. h = Sha512::new() .chain_update(b"SigEd25519 no Ed25519 collisions") - .chain_update(&[1]) // Ed25519ph - .chain_update(&[ctx_len]) + .chain_update([1]) // Ed25519ph + .chain_update([ctx_len]) .chain_update(ctx) - .chain_update(&self.nonce) + .chain_update(self.nonce) .chain_update(&prehash[..]); - r = Scalar::from_hash(h); - R = (&r * &ED25519_BASEPOINT_TABLE).compress(); + let r = Scalar::from_hash(h); + let R: CompressedEdwardsY = (&r * &ED25519_BASEPOINT_TABLE).compress(); h = Sha512::new() .chain_update(b"SigEd25519 no Ed25519 collisions") - .chain_update(&[1]) // Ed25519ph - .chain_update(&[ctx_len]) + .chain_update([1]) // Ed25519ph + .chain_update([ctx_len]) .chain_update(ctx) .chain_update(R.as_bytes()) .chain_update(verifying_key.as_bytes()) .chain_update(&prehash[..]); - k = Scalar::from_hash(h); - s = &(&k * &self.key) + &r; + let k = Scalar::from_hash(h); + let s: Scalar = (k * self.key) + r; Ok(InternalSignature { R, s }.into()) } diff --git a/src/verifying.rs b/src/verifying.rs index 51bec1a..2192a59 100644 --- a/src/verifying.rs +++ b/src/verifying.rs @@ -90,7 +90,7 @@ impl VerifyingKey { /// View this public key as a byte array. #[inline] - pub fn as_bytes<'a>(&'a self) -> &'a [u8; PUBLIC_KEY_LENGTH] { + pub fn as_bytes(&self) -> &[u8; PUBLIC_KEY_LENGTH] { &(self.0).0 } @@ -133,7 +133,7 @@ impl VerifyingKey { let compressed = CompressedEdwardsY(*bytes); let point = compressed .decompress() - .ok_or(InternalError::PointDecompressionError)?; + .ok_or(InternalError::PointDecompression)?; Ok(VerifyingKey(compressed, point)) } @@ -185,8 +185,6 @@ impl VerifyingKey { let signature = InternalSignature::try_from(signature)?; let mut h: Sha512 = Sha512::default(); - let R: EdwardsPoint; - let k: Scalar; let ctx: &[u8] = context.unwrap_or(b""); debug_assert!( @@ -197,20 +195,21 @@ impl VerifyingKey { let minus_A: EdwardsPoint = -self.1; h.update(b"SigEd25519 no Ed25519 collisions"); - h.update(&[1]); // Ed25519ph - h.update(&[ctx.len() as u8]); + h.update([1]); // Ed25519ph + h.update([ctx.len() as u8]); h.update(ctx); h.update(signature.R.as_bytes()); h.update(self.as_bytes()); h.update(prehashed_message.finalize().as_slice()); - k = Scalar::from_hash(h); - R = EdwardsPoint::vartime_double_scalar_mul_basepoint(&k, &(minus_A), &signature.s); + let k = Scalar::from_hash(h); + let R: EdwardsPoint = + EdwardsPoint::vartime_double_scalar_mul_basepoint(&k, &(minus_A), &signature.s); if R.compress() == signature.R { Ok(()) } else { - Err(InternalError::VerifyError.into()) + Err(InternalError::Verify.into()) } } @@ -285,32 +284,30 @@ impl VerifyingKey { let signature = InternalSignature::try_from(signature)?; let mut h: Sha512 = Sha512::new(); - let R: EdwardsPoint; - let k: Scalar; let minus_A: EdwardsPoint = -self.1; - let signature_R: EdwardsPoint; - match signature.R.decompress() { - None => return Err(InternalError::VerifyError.into()), - Some(x) => signature_R = x, - } + let signature_R: EdwardsPoint = match signature.R.decompress() { + None => return Err(InternalError::Verify.into()), + Some(x) => x, + }; // Logical OR is fine here as we're not trying to be constant time. if signature_R.is_small_order() || self.1.is_small_order() { - return Err(InternalError::VerifyError.into()); + return Err(InternalError::Verify.into()); } h.update(signature.R.as_bytes()); h.update(self.as_bytes()); - h.update(&message); + h.update(message); - k = Scalar::from_hash(h); - R = EdwardsPoint::vartime_double_scalar_mul_basepoint(&k, &(minus_A), &signature.s); + let k = Scalar::from_hash(h); + let R: EdwardsPoint = + EdwardsPoint::vartime_double_scalar_mul_basepoint(&k, &(minus_A), &signature.s); if R == signature_R { Ok(()) } else { - Err(InternalError::VerifyError.into()) + Err(InternalError::Verify.into()) } } } @@ -326,21 +323,20 @@ impl Verifier for VerifyingKey { let signature = InternalSignature::try_from(signature)?; let mut h: Sha512 = Sha512::new(); - let R: EdwardsPoint; - let k: Scalar; let minus_A: EdwardsPoint = -self.1; h.update(signature.R.as_bytes()); h.update(self.as_bytes()); - h.update(&message); + h.update(message); - k = Scalar::from_hash(h); - R = EdwardsPoint::vartime_double_scalar_mul_basepoint(&k, &(minus_A), &signature.s); + let k = Scalar::from_hash(h); + let R: EdwardsPoint = + EdwardsPoint::vartime_double_scalar_mul_basepoint(&k, &(minus_A), &signature.s); if R.compress() == signature.R { Ok(()) } else { - Err(InternalError::VerifyError.into()) + Err(InternalError::Verify.into()) } } } @@ -350,17 +346,14 @@ impl TryFrom<&[u8]> for VerifyingKey { #[inline] fn try_from(bytes: &[u8]) -> Result { - let bytes = bytes.try_into().map_err(|_| { - InternalError::BytesLengthError { - name: "VerifyingKey", - length: PUBLIC_KEY_LENGTH, - } + let bytes = bytes.try_into().map_err(|_| InternalError::BytesLength { + name: "VerifyingKey", + length: PUBLIC_KEY_LENGTH, })?; Self::from_bytes(bytes) } } - #[cfg(feature = "pkcs8")] impl DecodePublicKey for VerifyingKey {} diff --git a/tests/ed25519.rs b/tests/ed25519.rs index 10752a7..0406339 100644 --- a/tests/ed25519.rs +++ b/tests/ed25519.rs @@ -66,8 +66,7 @@ mod vectors { let pub_bytes = &pub_bytes[..PUBLIC_KEY_LENGTH].try_into().unwrap(); let signing_key = SigningKey::from_bytes(sec_bytes); - let expected_verifying_key = - VerifyingKey::from_bytes(pub_bytes).unwrap(); + let expected_verifying_key = VerifyingKey::from_bytes(pub_bytes).unwrap(); assert_eq!(expected_verifying_key, signing_key.verifying_key()); // The signatures in the test vectors also include the message @@ -93,8 +92,7 @@ mod vectors { let sig_bytes = hex!("98a70222f0b8121aa9d30f813d683f809e462b469c7ff87639499bb94e6dae4131f85042463c2a355a2003d062adf5aaa10b8c61e636062aaad11c2a26083406"); let signing_key = SigningKey::from_bytes(&sec_bytes); - let expected_verifying_key = - VerifyingKey::from_bytes(&pub_bytes).unwrap(); + let expected_verifying_key = VerifyingKey::from_bytes(&pub_bytes).unwrap(); assert_eq!(expected_verifying_key, signing_key.verifying_key()); let sig1 = Signature::try_from(&sig_bytes[..]).unwrap();