Skip to content

Commit

Permalink
[ci, sdk] Fix clippy warnings and run clippy on push in CI
Browse files Browse the repository at this point in the history
The `clippy` portion of #7058

This commit does the following:

- Run `cargo clippy` on push via GitHub CI
- Fix and warn on clippy::explicit_iter_loop
- Fix and warn on clippy::implicit_clone
- Fix and warn on clippy::unused_async
- Fix and warn on clippy::must_use_candidate
- Fix and warn on clippy::manual_let_else
- Fix clippy::needless_range_loop
- Fix clippy::unnecessary_to_owned
- Fix clippy::bool_assert_comparison
- Fix clippy::single_match
- Fix clippy::unnecessary_get_then_check
- Fix clippy::unnecessary_unwrap
- Fix clippy::unnecessary_lazy_evaluations
- Fix clippy::unit_arg
- Fix clippy::redundant_closure
- Fix clippy::map_clone
- Fix clippy::large_enum_variant
- Fix clippy::upper_case_acronyms
- Fix clippy::comparison_chain
- Fix clippy::useless_conversion
- Fix clippy::doc_lazy_continuation
- Fix clippy::to_string_in_format_args
- Fix clippy::comparison_to_empty
- Fix clippy::needless_borrow
- Fix clippy::default_constructed_unit_structs
- Fix clippy::init_numbered_fields
- Fix clippy::suspicious_doc_comments
- Fix clippy::needless_return
- Fix clippy::match_like_matches_macro
- Fix clippy::clone_on_copy
- Fix clippy::redundant_static_lifetimes
- Minor style fixes to EntityFacade::parse_decrypted_value()
- Don't warn on a few clippy things
  • Loading branch information
paw-hub authored and charlag committed Aug 30, 2024
1 parent 33eb0db commit def698a
Show file tree
Hide file tree
Showing 27 changed files with 217 additions and 220 deletions.
5 changes: 4 additions & 1 deletion .github/workflows/rust-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ jobs:

steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 #v4.1.1
- name: sdk warning check with clippy
working-directory: tuta-sdk/rust
run: cargo clippy --package tuta-sdk --no-deps -- -Dwarnings # -Dwarnings changes warnings to errors so that the check fails
- name: sdk tests
working-directory: tuta-sdk/rust
run: cargo test
run: cargo test
13 changes: 13 additions & 0 deletions tuta-sdk/rust/sdk/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,16 @@ tokio = { version = "1.38.0", features = ["rt", "macros"] }
[lib]
crate-type = ["cdylib", "staticlib", "lib"]
name = "tutasdk"

[lints.clippy]
new_without_default = "allow" # we don't want to implement Default for everything
enum_variant_names = "allow" # creates more problems than it solves
let_and_return = "allow" # we commonly use this style
too_many_arguments = "allow" # this is fine

# "pedantic" lints worth warning on
manual_let_else = "warn"
must_use_candidate = "warn"
unused_async = "warn"
implicit_clone = "warn"
explicit_iter_loop = "warn"
4 changes: 2 additions & 2 deletions tuta-sdk/rust/sdk/src/crypto/aes.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
///! Contains code to handle AES128/AES256 encryption and decryption
//! Contains code to handle AES128/AES256 encryption and decryption

use aes::cipher::{BlockCipher, BlockSizeUser};
use aes::cipher::block_padding::Pkcs7;
Expand Down Expand Up @@ -141,7 +141,7 @@ impl Clone for Iv {
/// This is implemented so that entity_facade_test_utils will work. You should never, ever, ever
/// re-use an IV, as this can lead to information leakage.
fn clone(&self) -> Self {
Iv(self.0.clone())
Iv(self.0)
}
}

Expand Down
36 changes: 18 additions & 18 deletions tuta-sdk/rust/sdk/src/crypto/crypto_facade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@ use crate::util::ArrayCastingError;

/// The name of the field that contains the session key encrypted
/// by the owner group's key in an entity
const OWNER_ENC_SESSION_FIELD: &'static str = "_ownerEncSessionKey";
const OWNER_ENC_SESSION_FIELD: &str = "_ownerEncSessionKey";
/// The name of the owner-encrypted session key version field in an entity
const OWNER_KEY_VERSION_FIELD: &'static str = "_ownerKeyVersion";
const OWNER_KEY_VERSION_FIELD: &str = "_ownerKeyVersion";
/// The name of the owner group field in an entity
const OWNER_GROUP_FIELD: &'static str = "_ownerGroup";
const OWNER_GROUP_FIELD: &str = "_ownerGroup";
/// The name of the ID field in an entity
const ID_FIELD: &'static str = "_id";
const ID_FIELD: &str = "_id";
/// The name of the bucket key field in an entity
const BUCKET_KEY_FIELD: &'static str = "bucketKey";
const BUCKET_KEY_FIELD: &str = "bucketKey";

#[derive(uniffi::Object)]
pub struct CryptoFacade {
Expand Down Expand Up @@ -88,7 +88,7 @@ impl CryptoFacade {

let group_key: GenericAesKey = self.key_loader_facade.load_sym_group_key(owner_group, owner_key_version, None).await?;

let session_key = group_key.decrypt_aes_key(&owner_enc_session_key)?;
let session_key = group_key.decrypt_aes_key(owner_enc_session_key)?;
// TODO: performance: should we reuse owner_enc_session_key?
Ok(Some(ResolvedSessionKey { session_key, owner_enc_session_key: owner_enc_session_key.clone() }))
}
Expand Down Expand Up @@ -157,7 +157,7 @@ impl CryptoFacade {
}
AsymmetricKeyPair::RSAEccKeyPair(RSAEccKeyPair { rsa_key_pair: k, .. }) | AsymmetricKeyPair::RSAKeyPair(k) => {
let bucket_key_bytes = Zeroizing::new(k.private_key.decrypt(pub_enc_bucket_key)?);
let decrypted_bucket_key = GenericAesKey::from_bytes(bucket_key_bytes.as_slice())?.into();
let decrypted_bucket_key = GenericAesKey::from_bytes(bucket_key_bytes.as_slice())?;
ResolvedBucketKey {
decrypted_bucket_key,
sender_identity_key: None,
Expand All @@ -166,7 +166,7 @@ impl CryptoFacade {
}
} else if let Some(_group_enc_bucket_key) = &bucket_key.groupEncBucketKey {
// TODO: to be used with secure external
let _key_group = bucket_key.keyGroup.as_ref().unwrap_or_else(|| owner_group);
let _key_group = bucket_key.keyGroup.as_ref().unwrap_or(owner_group);
auth_status = Some(EncryptionAuthStatus::AESNoAuthentication);
todo!("secure external resolveWithGroupReference")
} else {
Expand Down Expand Up @@ -215,7 +215,7 @@ pub enum EncryptionAuthStatus {
#[repr(i64)]
pub enum CryptoProtocolVersion {
/// Legacy asymmetric encryption (RSA-2048)
RSA = 0,
Rsa = 0,

/// Secure external
SymmetricEncryption = 1,
Expand Down Expand Up @@ -248,7 +248,7 @@ impl<'a> EntityOwnerKeyData<'a> {
}

let owner_enc_session_key = get_nullable_field!(entity, OWNER_ENC_SESSION_FIELD, Bytes)?;
let owner_key_version = get_nullable_field!(entity, OWNER_KEY_VERSION_FIELD, Number)?.map(|v| *v);
let owner_key_version = get_nullable_field!(entity, OWNER_KEY_VERSION_FIELD, Number)?.copied();
let owner_group = get_nullable_field!(entity, OWNER_GROUP_FIELD, IdGeneratedId)?;

Ok(EntityOwnerKeyData {
Expand Down Expand Up @@ -346,7 +346,7 @@ mod test {
let crypto_facade = make_crypto_facade(randomizer_facade.clone(), constants.group_key.clone(), constants.sender_key_version, asymmetric_keypair.clone());
let pub_enc_bucket_key = asymmetric_keypair.public_key.encrypt(&randomizer_facade, constants.bucket_key.as_bytes()).unwrap();

let mut raw_mail = make_raw_mail(&constants, pub_enc_bucket_key, CryptoProtocolVersion::RSA as i64);
let mut raw_mail = make_raw_mail(&constants, pub_enc_bucket_key, CryptoProtocolVersion::Rsa as i64);
let mail_type_model = get_mail_type_model();
let key = crypto_facade.resolve_session_key(&mut raw_mail, &mail_type_model)
.await
Expand All @@ -365,7 +365,7 @@ mod test {
let crypto_facade = make_crypto_facade(randomizer_facade.clone(), constants.group_key.clone(), constants.sender_key_version, asymmetric_keypair.clone());
let pub_enc_bucket_key = asymmetric_keypair.rsa_key_pair.public_key.encrypt(&randomizer_facade, constants.bucket_key.as_bytes()).unwrap();

let mut raw_mail = make_raw_mail(&constants, pub_enc_bucket_key, CryptoProtocolVersion::RSA as i64);
let mut raw_mail = make_raw_mail(&constants, pub_enc_bucket_key, CryptoProtocolVersion::Rsa as i64);
let mail_type_model = get_mail_type_model();
let key = crypto_facade.resolve_session_key(&mut raw_mail, &mail_type_model)
.await
Expand All @@ -378,7 +378,7 @@ mod test {
fn get_mail_type_model() -> TypeModel {
let provider = init_type_model_provider();
let mail_type_ref = Mail::type_ref();
provider.get_type_model(&mail_type_ref.app, &mail_type_ref.type_).unwrap().to_owned()
provider.get_type_model(mail_type_ref.app, mail_type_ref.type_).unwrap().to_owned()
}

fn make_raw_mail(constants: &BucketKeyConstants, pub_enc_bucket_key: Vec<u8>, protocol_version: i64) -> ParsedEntity {
Expand Down Expand Up @@ -417,15 +417,15 @@ mod test {

impl BucketKeyConstants {
fn new(randomizer_facade: &RandomizerFacade) -> Self {
let group_key = GenericAesKey::from(Aes256Key::generate(&randomizer_facade));
let bucket_key = Aes256Key::generate(&randomizer_facade);
let mail_session_key = GenericAesKey::from(Aes256Key::generate(&randomizer_facade));
let group_key = GenericAesKey::from(Aes256Key::generate(randomizer_facade));
let bucket_key = Aes256Key::generate(randomizer_facade);
let mail_session_key = GenericAesKey::from(Aes256Key::generate(randomizer_facade));
let instance_id = GeneratedId::test_random();
let instance_list = GeneratedId::test_random();
let key_group = GeneratedId::test_random();
let bucket_key_generic = GenericAesKey::from(bucket_key.clone());

let bucket_enc_session_key_iv = Iv::generate(&randomizer_facade);
let bucket_enc_session_key_iv = Iv::generate(randomizer_facade);
let bucket_enc_session_key = bucket_key_generic.encrypt_key(&mail_session_key, bucket_enc_session_key_iv);
let sender_key_version = 1;
let recipient_key_version = sender_key_version;
Expand All @@ -450,7 +450,7 @@ mod test {

let mut key_loader = MockKeyLoaderFacade::default();
key_loader.expect_get_current_sym_group_key()
.returning(move |_| Ok(VersionedAesKey { version: sender_key_version, object: group_key.clone().into() }))
.returning(move |_| Ok(VersionedAesKey { version: sender_key_version, object: group_key.clone() }))
.once();
key_loader.expect_load_key_pair()
.returning(move |_, _| Ok(asymmetric_keypair.clone().into()))
Expand Down
4 changes: 2 additions & 2 deletions tuta-sdk/rust/sdk/src/crypto/hkdf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
pub fn hkdf(salt: &[u8], input_key_material: &[u8], info: &[u8], length_bytes: usize) -> Vec<u8> {
let generator = hkdf::Hkdf::<sha2::Sha256>::new(Some(salt), input_key_material);
let mut output_buffer = vec![0u8; length_bytes];
generator.expand(&info, &mut output_buffer).unwrap();
generator.expand(info, &mut output_buffer).unwrap();
output_buffer
}

Expand All @@ -19,4 +19,4 @@ mod tests {
assert_eq!(test_data.hkdf_hex, result);
}
}
}
}
19 changes: 10 additions & 9 deletions tuta-sdk/rust/sdk/src/crypto/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use super::tuta_crypt::*;

#[derive(Clone, PartialEq)]
#[cfg_attr(test, derive(Debug))] // only allow Debug in tests because this prints the key!
#[allow(clippy::large_enum_variant)]
pub enum AsymmetricKeyPair {
RSAKeyPair(RSAKeyPair),
RSAEccKeyPair(RSAEccKeyPair),
Expand Down Expand Up @@ -44,8 +45,8 @@ impl GenericAesKey {
/// The returned AES key is zeroized on drop
pub fn decrypt_aes_key(&self, encrypted_key: &[u8]) -> Result<GenericAesKey, KeyLoadError> {
let decrypted = match self {
Self::Aes128(key) => aes_128_decrypt_no_padding_fixed_iv(&key, encrypted_key)?,
Self::Aes256(key) => aes_256_decrypt_no_padding(&key, encrypted_key)?.data,
Self::Aes128(key) => aes_128_decrypt_no_padding_fixed_iv(key, encrypted_key)?,
Self::Aes256(key) => aes_256_decrypt_no_padding(key, encrypted_key)?.data,
};

let decrypted = Zeroizing::new(decrypted);
Expand All @@ -57,16 +58,16 @@ impl GenericAesKey {
/// The return decrypted data is not zeroized
pub fn decrypt_data(&self, ciphertext: &[u8]) -> Result<Vec<u8>, AesDecryptError> {
let decrypted = match self {
Self::Aes128(key) => aes_128_decrypt(&key, ciphertext)?,
Self::Aes256(key) => aes_256_decrypt(&key, ciphertext)?,
Self::Aes128(key) => aes_128_decrypt(key, ciphertext)?,
Self::Aes256(key) => aes_256_decrypt(key, ciphertext)?,
};
Ok(decrypted.data)
}

pub fn decrypt_data_and_iv(&self, ciphertext: &[u8]) -> Result<PlaintextAndIv, AesDecryptError> {
let decrypted = match self {
Self::Aes128(key) => aes_128_decrypt(&key, ciphertext)?,
Self::Aes256(key) => aes_256_decrypt(&key, ciphertext)?,
Self::Aes128(key) => aes_128_decrypt(key, ciphertext)?,
Self::Aes256(key) => aes_256_decrypt(key, ciphertext)?,
};
Ok(decrypted)
}
Expand All @@ -82,8 +83,8 @@ impl GenericAesKey {
/// Encrypts `text` with this key.
pub fn encrypt_data(&self, text: &[u8], iv: Iv) -> Result<Vec<u8>, AesEncryptError> {
let ciphertext = match self {
Self::Aes128(key) => aes_128_encrypt(&key, text, &iv, PaddingMode::WithPadding, MacMode::WithMac)?,
Self::Aes256(key) => aes_256_encrypt(&key, text, &iv, PaddingMode::WithPadding)?,
Self::Aes128(key) => aes_128_encrypt(key, text, &iv, PaddingMode::WithPadding, MacMode::WithMac)?,
Self::Aes256(key) => aes_256_encrypt(key, text, &iv, PaddingMode::WithPadding)?,
};
Ok(ciphertext)
}
Expand Down Expand Up @@ -178,4 +179,4 @@ mod tests {

assert_eq!(raw_text, text.as_slice());
}
}
}
2 changes: 1 addition & 1 deletion tuta-sdk/rust/sdk/src/crypto/kyber.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ impl Debug for KyberPrivateKey {
impl KyberPrivateKey {
/// Instantiate a private key from bytes
pub fn from_bytes(bytes: &[u8]) -> Result<Self, KyberKeyError> {
let private_key = PQCryptoKyber1024SecretKey::from_bytes(&bytes)
let private_key = PQCryptoKyber1024SecretKey::from_bytes(bytes)
.map_err(|reason| KyberKeyError { reason: format!("kyber API error: {reason}") })?;

Ok(Self { private_key })
Expand Down
3 changes: 2 additions & 1 deletion tuta-sdk/rust/sdk/src/crypto/randomizer_facade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ impl RngCore for RandomizerFacade {
}

fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> {
Ok(self.fill_slice(dest))
self.fill_slice(dest);
Ok(())
}
}

Expand Down
19 changes: 8 additions & 11 deletions tuta-sdk/rust/sdk/src/crypto/rsa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ pub struct RSAPublicKey(rsa::RsaPublicKey);

impl RSAPublicKey {
pub fn new(public_key: rsa::RsaPublicKey) -> Self {
RSAPublicKey {
0: public_key,
}
Self(public_key)
}

/// Create a key from a PEM-encoded ASN.1 SPKI
Expand All @@ -40,7 +38,7 @@ impl RSAPublicKey {
/// Returns `Err` if the modulus is the wrong size.
pub fn from_components(modulus: &[u8], public_exponent: u32) -> Result<Self, RSAKeyError> {
rsa::RsaPublicKey::new(BigUint::from_bytes_be(modulus), public_exponent.into())
.map(|o| Self(o))
.map(Self)
.map_err(|e| RSAKeyError { reason: format!("rsa public key parse error: {e}") })
}

Expand Down Expand Up @@ -76,9 +74,7 @@ pub struct RSAPrivateKey(rsa::RsaPrivateKey);

impl RSAPrivateKey {
pub fn new(private_key: rsa::RsaPrivateKey) -> Self {
RSAPrivateKey {
0: private_key,
}
Self(private_key)
}
/// Derives an PKCS1 RSA private key from an ASN.1-DER encoded private key
pub fn from_pkcs1_der(private_key: &[u8]) -> Result<Self, RSAKeyError> {
Expand Down Expand Up @@ -223,9 +219,9 @@ fn decode_nibble_arrays<const SIZE: usize>(arrays: &[u8]) -> Result<[&[u8]; SIZE
let mut result = [[0u8; 0].as_slice(); SIZE];
let mut remaining = arrays;

for i in 0..SIZE {
for (array_index, array_result) in result.iter_mut().enumerate() {
if remaining.len() < 2 {
return Err(RSAKeyError { reason: format!("invalid encoded RSA key (only got {i} array(s), expected {SIZE})") });
return Err(RSAKeyError { reason: format!("invalid encoded RSA key (only got {array_index} array(s), expected {SIZE})") });
}
let (len_bytes, after) = remaining.split_at(2);

Expand All @@ -235,7 +231,7 @@ fn decode_nibble_arrays<const SIZE: usize>(arrays: &[u8]) -> Result<[&[u8]; SIZE
}
let (arr, new_remaining) = after.split_at(length);

result[i] = arr;
*array_result = arr;
remaining = new_remaining;
}

Expand Down Expand Up @@ -316,7 +312,8 @@ impl RngCore for SeedBufferRng {
}

fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> {
Ok(self.fill_bytes(dest))
self.fill_bytes(dest);
Ok(())
}
}

Expand Down
14 changes: 7 additions & 7 deletions tuta-sdk/rust/sdk/src/crypto/tuta_crypt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,14 @@ impl PQMessage {
bucket_key: &Aes256Key,
iv: Iv,
) -> Result<Self, PQError> {
let ecc_shared_secret = ecc_encapsulate(&sender_ecc_keypair.private_key, &ephemeral_ecc_keypair.private_key, &recipient_ecc_key);
let ecc_shared_secret = ecc_encapsulate(&sender_ecc_keypair.private_key, &ephemeral_ecc_keypair.private_key, recipient_ecc_key);
let encapsulation = recipient_kyber_key.encapsulate();

let kek = derive_pq_kek(
&sender_ecc_keypair.public_key,
&ephemeral_ecc_keypair.public_key,
&recipient_ecc_key,
&recipient_kyber_key,
recipient_ecc_key,
recipient_kyber_key,
&encapsulation.ciphertext,
&encapsulation.shared_secret,
&ecc_shared_secret,
Expand All @@ -101,8 +101,8 @@ impl PQMessage {
let kek_enc_bucket_key = aes_256_encrypt(&kek, bucket_key.as_bytes(), &iv, PaddingMode::WithPadding)?;

Ok(Self {
sender_identity_public_key: sender_ecc_keypair.public_key.to_owned(),
ephemeral_public_key: ephemeral_ecc_keypair.public_key.to_owned(),
sender_identity_public_key: sender_ecc_keypair.public_key.clone(),
ephemeral_public_key: ephemeral_ecc_keypair.public_key.clone(),
encapsulation: PQBucketKeyEncapsulation {
kyber_ciphertext: encapsulation.ciphertext,
kek_enc_bucket_key,
Expand All @@ -114,7 +114,7 @@ impl PQMessage {
#[derive(Copy, Clone)]
#[repr(u8)]
enum CryptoProtocolVersion {
RSA = 0,
Rsa = 0,
SymmetricEncryption = 1,
TutaCrypt = 2,
}
Expand Down Expand Up @@ -240,7 +240,7 @@ mod tests {
let iv = Iv::from_bytes(&i.seed[i.seed.len() - 16..]).unwrap();

let encapsulation = PQMessage::encapsulate(
&sender_ecc_keypair,
sender_ecc_keypair,
&ephemeral_ecc_keypair,
&ecc_keys.public_key,
&kyber_keys.public_key,
Expand Down
2 changes: 1 addition & 1 deletion tuta-sdk/rust/sdk/src/crypto_entity_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ mod tests {
);
let mail_type_ref = TypeRef { app: "tutanota", type_: "Mail" };
let mail_type_model: &'static TypeModel = my_favorite_leak
.get_type_model(&mail_type_ref.app, &mail_type_ref.type_)
.get_type_model(mail_type_ref.app, mail_type_ref.type_)
.expect("Error in type_model_provider");

// Set up the mock of the plain unencrypted entity client
Expand Down
4 changes: 4 additions & 0 deletions tuta-sdk/rust/sdk/src/date.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,22 @@ use serde::de::{Error, Visitor};
pub struct DateTime(SystemTime);

impl DateTime {
#[must_use]
pub fn new(time: SystemTime) -> Self {
Self(time)
}

#[must_use]
pub fn from_millis(millis: u64) -> Self {
Self(SystemTime::UNIX_EPOCH + Duration::from_millis(millis))
}

#[must_use]
pub fn get_time(self) -> SystemTime {
self.0
}

#[must_use]
pub fn as_millis(self) -> u64 {
self.0.duration_since(SystemTime::UNIX_EPOCH).unwrap().as_millis() as u64
}
Expand Down
Loading

0 comments on commit def698a

Please sign in to comment.