Cryptokit clippy (#153)

* Add clippy check to cryptokit * Fix clippy warnings for cryptokit * Make all safer-ffi versions same * Update .github/workflows/native_build.yml Co-authored-by: Stephane Raux <94983192+stefunctional@users.noreply.github.com> * Fix clippy warnings * Fix more clippy warnings --------- Co-authored-by: mulmarta <mulmarta@amazon.com> Co-authored-by: Stephane Raux <94983192+stefunctional@users.noreply.github.com>
awslabs · May 7, 2024 · acca854 · acca854
1 parent 3662e44
commit acca854
Show file tree

Hide file tree

Showing 16 changed files with 49 additions and 90 deletions.
diff --git a/.github/workflows/native_build.yml b/.github/workflows/native_build.yml
@@ -88,6 +88,25 @@ jobs:
         run: cargo clippy --all-targets --all-features --workspace -- -D warnings
       - name: Clippy Bare Bones
         run: cargo clippy --all-targets --no-default-features --features std,test_util --workspace -- -D warnings
+  LintAndFormattingMacOS:
+    # XXX(RLB): It would be good to just use macos-latest here, but
+    # apparently if you do that, sometimes you get an older (not latest)
+    # version.  And we require v14 in order to build the CryptoKit provider.
+    runs-on: macos-14
+    steps: 
+      - uses: actions/checkout@v3
+      - uses: arduino/setup-protoc@v2
+        with:
+          version: "25.x"
+          repo-token: ${{ secrets.GITHUB_TOKEN }}
+      - uses: dtolnay/rust-toolchain@stable
+      - uses: Swatinem/rust-cache@v2
+        with:
+          save-if: ${{ github.ref == 'refs/heads/main' }}
+      - name: Rust Fmt
+        run: cargo fmt -p mls-rs-crypto-cryptokit -- --check
+      - name: Clippy
+        run: cargo clippy -p mls-rs-crypto-cryptokit -- -D warnings
   CodeCoverage:
     runs-on: ubuntu-latest
     steps:

diff --git a/mls-rs-codec/tests/macro_usage.rs b/mls-rs-codec/tests/macro_usage.rs
@@ -25,13 +25,6 @@ struct TestType {
     field_h: TestFieldStruct,
 }
 
-#[derive(Debug, Clone, PartialEq, Eq, MlsSize, MlsEncode)]
-struct BorrowedTestType<'a> {
-    field_a: u8,
-    field_b: Option<&'a [u8]>,
-    field_c: &'a [u16],
-}
-
 #[repr(u16)]
 #[derive(Debug, Clone, PartialEq, Eq, MlsSize, MlsEncode, MlsDecode)]
 enum TestEnum {

diff --git a/mls-rs-core/Cargo.toml b/mls-rs-core/Cargo.toml
@@ -26,7 +26,7 @@ mls-rs-codec = { version = "0.5.2", path = "../mls-rs-codec", default-features =
 zeroize = { version = "1", default-features = false, features = ["alloc", "zeroize_derive"] }
 arbitrary = { version = "1", features = ["derive"], optional = true }
 thiserror = { version = "1.0.40", optional = true }
-safer-ffi = { version = "0.1.3", default-features = false, optional = true }
+safer-ffi = { version = "0.1.7", default-features = false, optional = true }
 safer-ffi-gen = { version = "0.9.2", default-features = false, optional = true }
 maybe-async = "0.2.10"
 

diff --git a/mls-rs-crypto-cryptokit/src/kem.rs b/mls-rs-crypto-cryptokit/src/kem.rs
@@ -324,7 +324,7 @@ impl Kem {
         priv_buf.truncate(priv_len as usize);
         pub_buf.truncate(pub_len as usize);
 
-        return Ok((priv_buf.into(), pub_buf.into()));
+        Ok((priv_buf.into(), pub_buf.into()))
     }
 
     pub fn derive(&self, ikm: &[u8]) -> Result<(HpkeSecretKey, HpkePublicKey), KemError> {
@@ -351,13 +351,13 @@ impl Kem {
         priv_buf.truncate(priv_len as usize);
         pub_buf.truncate(pub_len as usize);
 
-        return Ok((priv_buf.into(), pub_buf.into()));
+        Ok((priv_buf.into(), pub_buf.into()))
     }
 
     pub fn public_key_validate(&self, key: &HpkePublicKey) -> Result<(), KemError> {
         let rv = unsafe { kem_public_key_validate(self.0 as u16, key.as_ptr(), key.len() as u64) };
 
-        (rv == 1).then(|| ()).ok_or(KemError::CryptoKitError)
+        (rv == 1).then_some(()).ok_or(KemError::CryptoKitError)
     }
 
     pub fn hpke_setup_s(

diff --git a/mls-rs-crypto-cryptokit/src/lib.rs b/mls-rs-crypto-cryptokit/src/lib.rs
@@ -147,7 +147,7 @@ impl CryptoKitCipherSuite {
 
     pub fn random_bytes(&self, out: &mut [u8]) -> Result<(), CryptoKitError> {
         random::fill(out)
-            .then(|| ())
+            .then_some(())
             .ok_or(CryptoKitError::RandError)
     }
 }
@@ -263,7 +263,7 @@ impl CipherSuiteProvider for CryptoKitCipherSuite {
         let (kem_output, mut ctx) = self.hpke_setup_s(remote_key, info)?;
         let ciphertext = ctx
             .seal(aad, pt)
-            .map_err(|e| <KemError as Into<CryptoKitError>>::into(e))?;
+            .map_err(<KemError as Into<CryptoKitError>>::into)?;
         Ok(HpkeCiphertext {
             kem_output,
             ciphertext,
@@ -281,12 +281,12 @@ impl CipherSuiteProvider for CryptoKitCipherSuite {
         let mut ctx =
             self.hpke_setup_r(&ciphertext.kem_output, local_secret, local_public, info)?;
         ctx.open(aad, &ciphertext.ciphertext)
-            .map_err(|e| <KemError as Into<CryptoKitError>>::into(e))
+            .map_err(<KemError as Into<CryptoKitError>>::into)
     }
 
     fn random_bytes(&self, out: &mut [u8]) -> Result<(), Self::Error> {
         random::fill(out)
-            .then(|| ())
+            .then_some(())
             .ok_or(CryptoKitError::RandError)
     }
 

diff --git a/mls-rs-crypto-cryptokit/src/sig.rs b/mls-rs-crypto-cryptokit/src/sig.rs
@@ -74,13 +74,10 @@ impl Signature {
     const DEFAULT_BUFFER_SIZE: usize = 192;
 
     fn supported_curve(curve: Curve) -> bool {
-        match curve {
-            Curve::P256 => true,
-            Curve::P384 => true,
-            Curve::P521 => true,
-            Curve::Ed25519 => true,
-            _ => false,
-        }
+        matches!(
+            curve,
+            Curve::P256 | Curve::P384 | Curve::P521 | Curve::Ed25519
+        )
     }
 
     pub fn new(cipher_suite: CipherSuite) -> Option<Self> {
@@ -91,7 +88,7 @@ impl Signature {
 
     pub fn new_from_curve(curve: Curve) -> Result<Self, SignatureError> {
         Self::supported_curve(curve)
-            .then(|| Self(curve))
+            .then_some(Self(curve))
             .ok_or(SignatureError::UnsupportedCurve)
     }
 
@@ -120,7 +117,7 @@ impl Signature {
         let pub_len = pub_len as usize;
         let pub_key = SignaturePublicKey::new_slice(&pub_buf[..pub_len]);
 
-        return Ok((priv_key, pub_key));
+        Ok((priv_key, pub_key))
     }
 
     pub fn derive_public(
@@ -146,7 +143,7 @@ impl Signature {
         let pub_len = pub_len as usize;
         let pub_key = SignaturePublicKey::new_slice(&pub_buf[..pub_len]);
 
-        return Ok(pub_key);
+        Ok(pub_key)
     }
 
     pub fn sign(
@@ -173,7 +170,7 @@ impl Signature {
         }
 
         let sig_len = sig_len as usize;
-        return Ok(sig_buf[..sig_len].to_vec());
+        Ok(sig_buf[..sig_len].to_vec())
     }
 
     pub fn verify(
@@ -195,7 +192,7 @@ impl Signature {
         };
 
         (rv == 1)
-            .then(|| ())
+            .then_some(())
             .ok_or(SignatureError::InvalidSignature)
     }
 }

diff --git a/mls-rs-crypto-openssl/src/x509.rs b/mls-rs-crypto-openssl/src/x509.rs
@@ -21,13 +21,13 @@ use openssl::{
     hash::MessageDigest,
     nid::Nid,
     pkey::{PKey, PKeyRef, Private, Public},
-    stack::{Stack, StackRef},
+    stack::Stack,
     x509::{
         extension::{BasicConstraints, KeyUsage, SubjectAlternativeName},
         store::{X509Store, X509StoreBuilder},
         verify::{X509VerifyFlags, X509VerifyParam},
-        X509Builder, X509Extension, X509Name, X509NameBuilder, X509NameRef, X509Ref,
-        X509ReqBuilder, X509StoreContext, X509VerifyResult, X509v3Context, X509,
+        X509Builder, X509Extension, X509Name, X509NameBuilder, X509ReqBuilder, X509StoreContext,
+        X509VerifyResult, X509v3Context, X509,
     },
 };
 use thiserror::Error;
@@ -372,10 +372,6 @@ fn build_subject_alt_name(
 }
 
 trait X509BuilderCommon {
-    fn set_pubkey(&mut self, pub_key: &PKeyRef<Public>) -> Result<(), ErrorStack>;
-    fn set_subject_name(&mut self, name: &X509NameRef) -> Result<(), ErrorStack>;
-    fn add_extensions(&mut self, extensions: &StackRef<X509Extension>) -> Result<(), ErrorStack>;
-    fn x509v3_context<'a>(&'a self, issuer: Option<&'a X509Ref>) -> X509v3Context<'a>;
     fn sign(&mut self, key: &PKeyRef<Private>, digest: MessageDigest) -> Result<(), ErrorStack>;
 
     fn sign_with_ec_signer(
@@ -395,46 +391,12 @@ trait X509BuilderCommon {
 }
 
 impl X509BuilderCommon for X509ReqBuilder {
-    fn set_pubkey(&mut self, pub_key: &PKeyRef<Public>) -> Result<(), ErrorStack> {
-        self.set_pubkey(pub_key)
-    }
-
-    fn set_subject_name(&mut self, name: &X509NameRef) -> Result<(), ErrorStack> {
-        self.set_subject_name(name)
-    }
-
-    fn add_extensions(&mut self, extensions: &StackRef<X509Extension>) -> Result<(), ErrorStack> {
-        self.add_extensions(extensions)
-    }
-
-    fn x509v3_context<'a>(&'a self, _issuer: Option<&X509Ref>) -> X509v3Context<'a> {
-        self.x509v3_context(None)
-    }
-
     fn sign(&mut self, key: &PKeyRef<Private>, digest: MessageDigest) -> Result<(), ErrorStack> {
         self.sign(key, digest)
     }
 }
 
 impl X509BuilderCommon for X509Builder {
-    fn set_pubkey(&mut self, pub_key: &PKeyRef<Public>) -> Result<(), ErrorStack> {
-        self.set_pubkey(pub_key)
-    }
-
-    fn set_subject_name(&mut self, name: &X509NameRef) -> Result<(), ErrorStack> {
-        self.set_subject_name(name)
-    }
-
-    fn add_extensions(&mut self, extensions: &StackRef<X509Extension>) -> Result<(), ErrorStack> {
-        extensions
-            .into_iter()
-            .try_for_each(|ex| self.append_extension2(ex))
-    }
-
-    fn x509v3_context<'a>(&'a self, issuer: Option<&'a X509Ref>) -> X509v3Context<'a> {
-        self.x509v3_context(issuer, None)
-    }
-
     fn sign(&mut self, key: &PKeyRef<Private>, digest: MessageDigest) -> Result<(), ErrorStack> {
         self.sign(key, digest)
     }

diff --git a/mls-rs-ffi/Cargo.toml b/mls-rs-ffi/Cargo.toml
@@ -23,5 +23,5 @@ mls-rs = { path = "../mls-rs", version = "0.39.0", features = ["ffi"] }
 mls-rs-crypto-openssl = { path = "../mls-rs-crypto-openssl", version = "0.9.0", optional = true }
 mls-rs-identity-x509 = { path = "../mls-rs-identity-x509", version = "0.11.0", optional = true }
 mls-rs-provider-sqlite = { path = "../mls-rs-provider-sqlite", version = "0.11.0", default-features = false, optional = true }
-safer-ffi = { version = "0.1.3", default-features = false }
+safer-ffi = { version = "0.1.7", default-features = false }
 safer-ffi-gen = { version = "0.9.2", default-features = false }
diff --git a/mls-rs/Cargo.toml b/mls-rs/Cargo.toml
@@ -68,7 +68,7 @@ mls-rs-crypto-openssl = { path = "../mls-rs-crypto-openssl", optional = true, ve
 # TODO: https://github.com/GoogleChromeLabs/wasm-bindgen-rayon
 rayon = { version = "1", optional = true }
 arbitrary = { version = "1", features = ["derive"], optional = true }
-safer-ffi = { version = "0.1.3", default-features = false, optional = true }
+safer-ffi = { version = "0.1.7", default-features = false, optional = true }
 safer-ffi-gen = { version = "0.9.2", default-features = false, optional = true }
 once_cell = { version = "1.18", optional = true }
 serde = { version = "1.0", default-features = false, features = ["alloc", "derive"], optional = true }

diff --git a/mls-rs/src/external_client/group.rs b/mls-rs/src/external_client/group.rs
@@ -569,11 +569,6 @@ where
     type OutputType = ExternalReceivedMessage;
     type CipherSuiteProvider = <C::CryptoProvider as CryptoProvider>::CipherSuiteProvider;
 
-    #[cfg(feature = "private_message")]
-    fn self_index(&self) -> Option<LeafIndex> {
-        None
-    }
-
     fn mls_rules(&self) -> Self::MlsRules {
         self.config.mls_rules()
     }

diff --git a/mls-rs/src/group/message_processor.rs b/mls-rs/src/group/message_processor.rs
@@ -861,8 +861,6 @@ pub(crate) trait MessageProcessor: Send + Sync {
 
     fn group_state(&self) -> &GroupState;
     fn group_state_mut(&mut self) -> &mut GroupState;
-    #[cfg(feature = "private_message")]
-    fn self_index(&self) -> Option<LeafIndex>;
     fn mls_rules(&self) -> Self::MlsRules;
     fn identity_provider(&self) -> Self::IdentityProvider;
     fn cipher_suite_provider(&self) -> &Self::CipherSuiteProvider;

diff --git a/mls-rs/src/group/mod.rs b/mls-rs/src/group/mod.rs
@@ -1604,11 +1604,6 @@ where
     type OutputType = ReceivedMessage;
     type CipherSuiteProvider = <C::CryptoProvider as CryptoProvider>::CipherSuiteProvider;
 
-    #[cfg(feature = "private_message")]
-    fn self_index(&self) -> Option<LeafIndex> {
-        Some(self.private_tree.self_index)
-    }
-
     #[cfg(feature = "private_message")]
     async fn process_ciphertext(
         &mut self,

diff --git a/mls-rs/src/group/proposal_cache.rs b/mls-rs/src/group/proposal_cache.rs
@@ -551,7 +551,7 @@ pub(crate) mod test_utils {
                 ConfirmationTag::empty(cipher_suite_provider).await,
             );
 
-            state.proposals.proposals = self.proposals.clone();
+            state.proposals.proposals.clone_from(&self.proposals);
             let proposals = self.resolve_for_commit(sender, proposal_list)?;
 
             state

diff --git a/mls-rs/src/group/test_utils.rs b/mls-rs/src/group/test_utils.rs
@@ -512,10 +512,4 @@ impl MessageProcessor for GroupWithoutKeySchedule {
         self.secrets = secrets;
         Ok(())
     }
-
-    #[cfg(feature = "private_message")]
-    #[cfg_attr(coverage_nightly, coverage(off))]
-    fn self_index(&self) -> Option<LeafIndex> {
-        <Group<TestClientConfig> as MessageProcessor>::self_index(&self.inner)
-    }
 }
diff --git a/mls-rs/src/psk.rs b/mls-rs/src/psk.rs
@@ -123,6 +123,7 @@ pub(crate) enum ResumptionPSKUsage {
     Branch = 3u8,
 }
 
+#[cfg(feature = "psk")]
 #[derive(Clone, Debug, PartialEq, MlsSize, MlsEncode)]
 struct PSKLabel<'a> {
     id: &'a PreSharedKeyID,

diff --git a/mls-rs/src/tree_kem/tree_hash.rs b/mls-rs/src/tree_kem/tree_hash.rs
@@ -190,7 +190,12 @@ impl TreeKemPublic {
             };
 
             let p = ps.parent;
-            filtered_sets[n] = filtered_sets[p as usize].clone();
+
+            // Clippy's suggestion `filtered_sets[n].clone_from(&filtered_sets[p as usize])` is wrong and does not compile
+            #[allow(clippy::assigning_clones)]
+            {
+                filtered_sets[n] = filtered_sets[p as usize].clone();
+            }
 
             if self.different_unmerged(*filtered_sets[p as usize].last().unwrap(), p)? {
                 filtered_sets[n].push(p);