Repository: RustCrypto/password-hashes (language: rust)

Issue:
`sha256_crypt` doesn't need an error return
[`sha256_crypt`](https://docs.rs/sha-crypt/0.5.0/sha_crypt/fn.sha256_crypt.html)

The only thing it uses the error return for is checking the passed `Sha256Params`, but those were already validated in `Sha256Params::new`.


Discussion / hints:
Making it infallible sounds good to me

Fix patch:
diff --git a/sha-crypt/src/lib.rs b/sha-crypt/src/lib.rs
index 8ec3d83c..736403bd 100644
--- a/sha-crypt/src/lib.rs
+++ b/sha-crypt/src/lib.rs
@@ -16,8 +16,7 @@
 //! let params = Sha512Params::new(10_000).expect("RandomError!");
 //!
 //! // Hash the password for storage
-//! let hashed_password = sha512_simple("Not so secure password", &params)
-//!     .expect("Should not fail");
+//! let hashed_password = sha512_simple("Not so secure password", &params);
 //!
 //! // Verifying a stored password
 //! assert!(sha512_check("Not so secure password", &hashed_password).is_ok());
@@ -96,7 +95,7 @@ pub fn sha512_crypt(
     password: &[u8],
     salt: &[u8],
     params: &Sha512Params,
-) -> Result<[u8; BLOCK_SIZE_SHA512], CryptError> {
+) -> [u8; BLOCK_SIZE_SHA512] {
     let pw_len = password.len();
 
     let salt_len = salt.len();
@@ -106,10 +105,6 @@ pub fn sha512_crypt(
     };
     let salt_len = salt.len();
 
-    if params.rounds < ROUNDS_MIN || params.rounds > ROUNDS_MAX {
-        return Err(CryptError::RoundsError);
-    }
-
     let digest_a = sha512crypt_intermediate(password, salt);
 
     // 13.
@@ -178,7 +173,7 @@ pub fn sha512_crypt(
         digest_c.clone_from_slice(&hasher.finalize());
     }
 
-    Ok(digest_c)
+    digest_c
 }
 
 /// The SHA256 crypt function returned as byte vector
@@ -199,7 +194,7 @@ pub fn sha256_crypt(
     password: &[u8],
     salt: &[u8],
     params: &Sha256Params,
-) -> Result<[u8; BLOCK_SIZE_SHA256], CryptError> {
+) -> [u8; BLOCK_SIZE_SHA256] {
     let pw_len = password.len();
 
     let salt_len = salt.len();
@@ -209,10 +204,6 @@ pub fn sha256_crypt(
     };
     let salt_len = salt.len();
 
-    if params.rounds < ROUNDS_MIN || params.rounds > ROUNDS_MAX {
-        return Err(CryptError::RoundsError);
-    }
-
     let digest_a = sha256crypt_intermediate(password, salt);
 
     // 13.
@@ -281,7 +272,7 @@ pub fn sha256_crypt(
         digest_c.clone_from_slice(&hasher.finalize());
     }
 
-    Ok(digest_c)
+    digest_c
 }
 
 /// Same as sha512_crypt except base64 representation will be returned.
@@ -295,14 +286,9 @@ pub fn sha256_crypt(
 /// # Returns
 /// - `Ok(())` if calculation was successful
 /// - `Err(errors::CryptError)` otherwise
-pub fn sha512_crypt_b64(
-    password: &[u8],
-    salt: &[u8],
-    params: &Sha512Params,
-) -> Result<String, CryptError> {
-    let output = sha512_crypt(password, salt, params)?;
-    let r = String::from_utf8(b64::encode_sha512(&output).to_vec())?;
-    Ok(r)
+pub fn sha512_crypt_b64(password: &[u8], salt: &[u8], params: &Sha512Params) -> String {
+    let output = sha512_crypt(password, salt, params);
+    String::from_utf8(b64::encode_sha512(&output).to_vec()).unwrap()
 }
 
 /// Same as sha256_crypt except base64 representation will be returned.
@@ -316,14 +302,9 @@ pub fn sha512_crypt_b64(
 /// # Returns
 /// - `Ok(())` if calculation was successful
 /// - `Err(errors::CryptError)` otherwise
-pub fn sha256_crypt_b64(
-    password: &[u8],
-    salt: &[u8],
-    params: &Sha256Params,
-) -> Result<String, CryptError> {
-    let output = sha256_crypt(password, salt, params)?;
-    let r = String::from_utf8(b64::encode_sha256(&output).to_vec())?;
-    Ok(r)
+pub fn sha256_crypt_b64(password: &[u8], salt: &[u8], params: &Sha256Params) -> String {
+    let output = sha256_crypt(password, salt, params);
+    String::from_utf8(b64::encode_sha256(&output).to_vec()).unwrap()
 }
 
 /// Simple interface for generating a SHA512 password hash.
@@ -339,7 +320,7 @@ pub fn sha256_crypt_b64(
 /// [1]: https://www.akkadia.org/drepper/SHA-crypt.txt
 #[cfg(feature = "simple")]
 #[cfg_attr(docsrs, doc(cfg(feature = "simple")))]
-pub fn sha512_simple(password: &str, params: &Sha512Params) -> Result<String, CryptError> {
+pub fn sha512_simple(password: &str, params: &Sha512Params) -> String {
     let rng = thread_rng();
 
     let salt: String = rng
@@ -347,7 +328,7 @@ pub fn sha512_simple(password: &str, params: &Sha512Params) -> Result<String, Cr
         .take(SALT_MAX_LEN)
         .collect();
 
-    let out = sha512_crypt(password.as_bytes(), salt.as_bytes(), params)?;
+    let out = sha512_crypt(password.as_bytes(), salt.as_bytes(), params);
 
     let mut result = String::new();
     result.push_str(SHA512_SALT_PREFIX);
@@ -357,9 +338,9 @@ pub fn sha512_simple(password: &str, params: &Sha512Params) -> Result<String, Cr
     }
     result.push_str(&salt);
     result.push('$');
-    let s = String::from_utf8(b64::encode_sha512(&out).to_vec())?;
+    let s = String::from_utf8(b64::encode_sha512(&out).to_vec()).unwrap();
     result.push_str(&s);
-    Ok(result)
+    result
 }
 
 /// Simple interface for generating a SHA256 password hash.
@@ -375,7 +356,7 @@ pub fn sha512_simple(password: &str, params: &Sha512Params) -> Result<String, Cr
 /// [1]: https://www.akkadia.org/drepper/SHA-crypt.txt
 #[cfg(feature = "simple")]
 #[cfg_attr(docsrs, doc(cfg(feature = "simple")))]
-pub fn sha256_simple(password: &str, params: &Sha256Params) -> Result<String, CryptError> {
+pub fn sha256_simple(password: &str, params: &Sha256Params) -> String {
     let rng = thread_rng();
 
     let salt: String = rng
@@ -383,7 +364,7 @@ pub fn sha256_simple(password: &str, params: &Sha256Params) -> Result<String, Cr
         .take(SALT_MAX_LEN)
         .collect();
 
-    let out = sha256_crypt(password.as_bytes(), salt.as_bytes(), params)?;
+    let out = sha256_crypt(password.as_bytes(), salt.as_bytes(), params);
 
     let mut result = String::new();
     result.push_str(SHA256_SALT_PREFIX);
@@ -393,9 +374,9 @@ pub fn sha256_simple(password: &str, params: &Sha256Params) -> Result<String, Cr
     }
     result.push_str(&salt);
     result.push('$');
-    let s = String::from_utf8(b64::encode_sha256(&out).to_vec())?;
+    let s = String::from_utf8(b64::encode_sha256(&out).to_vec()).unwrap();
     result.push_str(&s);
-    Ok(result)
+    result
 }
 
 /// Checks that given password matches provided hash.
@@ -459,13 +440,13 @@ pub fn sha512_check(password: &str, hashed_value: &str) -> Result<(), CheckError
         ));
     }
 
-    let params = Sha512Params { rounds };
-
-    let output = match sha512_crypt(password.as_bytes(), salt.as_bytes(), &params) {
-        Ok(v) => v,
+    let params = match Sha512Params::new(rounds) {
+        Ok(p) => p,
         Err(e) => return Err(CheckError::Crypt(e)),
     };
 
+    let output = sha512_crypt(password.as_bytes(), salt.as_bytes(), &params);
+
     let hash = b64::decode_sha512(hash.as_bytes())?;
 
     use subtle::ConstantTimeEq;
@@ -537,13 +518,13 @@ pub fn sha256_check(password: &str, hashed_value: &str) -> Result<(), CheckError
         ));
     }
 
-    let params = Sha256Params { rounds };
-
-    let output = match sha256_crypt(password.as_bytes(), salt.as_bytes(), &params) {
-        Ok(v) => v,
+    let params = match Sha256Params::new(rounds) {
+        Ok(p) => p,
         Err(e) => return Err(CheckError::Crypt(e)),
     };
 
+    let output = sha256_crypt(password.as_bytes(), salt.as_bytes(), &params);
+
     let hash = b64::decode_sha256(hash.as_bytes())?;
 
     use subtle::ConstantTimeEq;
diff --git a/sha-crypt/src/params.rs b/sha-crypt/src/params.rs
index f62de51a..a994e910 100644
--- a/sha-crypt/src/params.rs
+++ b/sha-crypt/src/params.rs
@@ -4,18 +4,18 @@ use crate::errors;
 use core::default::Default;
 
 /// Default number of rounds.
-pub const ROUNDS_DEFAULT: usize = 5_000;
+pub const ROUNDS_DEFAULT: u32 = 5_000;
 
 /// Minimum number of rounds allowed.
-pub const ROUNDS_MIN: usize = 1_000;
+pub const ROUNDS_MIN: u32 = 1_000;
 
 /// Maximum number of rounds allowed.
-pub const ROUNDS_MAX: usize = 999_999_999;
+pub const ROUNDS_MAX: u32 = 999_999_999;
 
 /// Algorithm parameters.
 #[derive(Debug, Clone)]
 pub struct Sha512Params {
-    pub(crate) rounds: usize,
+    pub(crate) rounds: u32,
 }
 
 impl Default for Sha512Params {
@@ -28,7 +28,7 @@ impl Default for Sha512Params {
 
 impl Sha512Params {
     /// Create new algorithm parameters.
-    pub fn new(rounds: usize) -> Result<Sha512Params, errors::CryptError> {
+    pub fn new(rounds: u32) -> Result<Sha512Params, errors::CryptError> {
         if (ROUNDS_MIN..=ROUNDS_MAX).contains(&rounds) {
             Ok(Sha512Params { rounds })
         } else {
@@ -40,7 +40,7 @@ impl Sha512Params {
 /// Algorithm parameters.
 #[derive(Debug, Clone)]
 pub struct Sha256Params {
-    pub(crate) rounds: usize,
+    pub(crate) rounds: u32,
 }
 
 impl Default for Sha256Params {
@@ -53,7 +53,7 @@ impl Default for Sha256Params {
 
 impl Sha256Params {
     /// Create new algorithm parameters.
-    pub fn new(rounds: usize) -> Result<Sha256Params, errors::CryptError> {
+    pub fn new(rounds: u32) -> Result<Sha256Params, errors::CryptError> {
         if (ROUNDS_MIN..=ROUNDS_MAX).contains(&rounds) {
             Ok(Sha256Params { rounds })
         } else {


Test patch:
diff --git a/sha-crypt/tests/lib.rs b/sha-crypt/tests/lib.rs
index 9c95e661..62a046db 100644
--- a/sha-crypt/tests/lib.rs
+++ b/sha-crypt/tests/lib.rs
@@ -10,7 +10,7 @@ struct TestVector {
     salt: &'static str,
     result_sha256: &'static str,
     result_sha512: &'static str,
-    rounds: usize,
+    rounds: u32,
 }
 
 const TEST_VECTORS: &[TestVector] = &[
@@ -100,7 +100,7 @@ const TEST_VECTORS: &[TestVector] = &[
 fn test_sha512_crypt() {
     for t in TEST_VECTORS {
         let params = Sha512Params::new(t.rounds).expect("Rounds error");
-        let result = sha512_crypt_b64(t.input.as_bytes(), t.salt.as_bytes(), &params).unwrap();
+        let result = sha512_crypt_b64(t.input.as_bytes(), t.salt.as_bytes(), &params);
         assert!(result == t.result_sha512);
     }
 }
@@ -109,7 +109,7 @@ fn test_sha512_crypt() {
 fn test_sha256_crypt() {
     for t in TEST_VECTORS {
         let params = Sha256Params::new(t.rounds).expect("Rounds error");
-        let result = sha256_crypt_b64(t.input.as_bytes(), t.salt.as_bytes(), &params).unwrap();
+        let result = sha256_crypt_b64(t.input.as_bytes(), t.salt.as_bytes(), &params);
         println!("result  {:?}", result);
         println!("correct {:?}", t.result_sha256);
         assert!(result == t.result_sha256);
@@ -172,9 +172,7 @@ fn test_sha512_simple_check_roundtrip() {
     let pw = "this is my password";
     let params = Sha512Params::new(5_000).expect("Rounds error");
 
-    let r = sha512_simple(pw, &params);
-    assert!(r.is_ok());
-    let hash = r.unwrap();
+    let hash = sha512_simple(pw, &params);
 
     let c_r = sha512_check(pw, &hash);
     assert!(c_r.is_ok());
@@ -186,9 +184,7 @@ fn test_sha256_simple_check_roundtrip() {
     let pw = "this is my password";
     let params = Sha256Params::new(5_000).expect("Rounds error");
 
-    let r = sha256_simple(pw, &params);
-    assert!(r.is_ok());
-    let hash = r.unwrap();
+    let hash = sha256_simple(pw, &params);
 
     let c_r = sha256_check(pw, &hash);
     assert!(c_r.is_ok());