[pbs-devel] [PATCH pbs] tfa: Improve TOTP algorithm parsing

[pbs-devel] [PATCH pbs] tfa: Improve TOTP algorithm parsing

[Maximiliano Sandoval]

It is very common for TOTP URIs to contain the algorithm in lowercase,
hence we convert to lowercase when doing From<&str> for Algorithm.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 proxmox-tfa/src/totp.rs | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/proxmox-tfa/src/totp.rs b/proxmox-tfa/src/totp.rs
index 7b8e6b3..97be715 100644
--- a/proxmox-tfa/src/totp.rs
+++ b/proxmox-tfa/src/totp.rs
@@ -99,10 +99,10 @@ impl std::str::FromStr for Algorithm {
     type Err = Error;
 
     fn from_str(s: &str) -> Result<Self, Error> {
-        Ok(match s {
-            "SHA1" => Algorithm::Sha1,
-            "SHA256" => Algorithm::Sha256,
-            "SHA512" => Algorithm::Sha512,
+        Ok(match s.to_lowercase().as_str() {
+            "sha1" => Algorithm::Sha1,
+            "sha256" => Algorithm::Sha256,
+            "sha512" => Algorithm::Sha512,
             _ => return Err(Error::UnsupportedAlgorithm(s.to_string())),
         })
     }
@@ -640,3 +640,23 @@ fn test_otp() {
     assert_eq!(parsed.issuer.as_deref(), Some("An Issuer"));
     assert_eq!(parsed.account_name.as_deref(), Some("The Account Name"));
 }
+
+#[test]
+fn test_algorithm_parsing() {
+    let secret = "AA";
+    let period = 30;
+    let digits = 6;
+    let issuer = "ISSUER";
+    let uri = format!("otpauth://totp/user%40hostname?secret={secret}&issuer={issuer}&algorithm=sha1&digits={digits}&period={period}");
+    let hotp: Totp = uri.parse().expect("failed to parse otp uri");
+
+    assert_eq!(hotp.algorithm, Algorithm::Sha1);
+    assert_eq!(hotp.period, period);
+    assert_eq!(hotp.digits, digits);
+    assert_eq!(hotp.issuer.as_deref(), Some(issuer));
+    assert_eq!(hotp.account_name.as_deref(), Some("user@hostname"));
+    assert_eq!(
+        &base32::encode(base32::Alphabet::RFC4648 { padding: false }, &hotp.secret()),
+        secret
+    )
+}
-- 
2.39.2

[pbs-devel] applied: [PATCH pbs] tfa: Improve TOTP algorithm parsing

[Wolfgang Bumiller]

applied, though not too happy when tools just randomly casefold stuff
instead of using the values listed in the "spec" (whatever this one's
worth...)

On Fri, Jun 09, 2023 at 05:52:25PM +0200, Maximiliano Sandoval wrote:
> It is very common for TOTP URIs to contain the algorithm in lowercase,
> hence we convert to lowercase when doing From<&str> for Algorithm.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  proxmox-tfa/src/totp.rs | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/proxmox-tfa/src/totp.rs b/proxmox-tfa/src/totp.rs
> index 7b8e6b3..97be715 100644
> --- a/proxmox-tfa/src/totp.rs
> +++ b/proxmox-tfa/src/totp.rs
> @@ -99,10 +99,10 @@ impl std::str::FromStr for Algorithm {
>      type Err = Error;
>  
>      fn from_str(s: &str) -> Result<Self, Error> {
> -        Ok(match s {
> -            "SHA1" => Algorithm::Sha1,
> -            "SHA256" => Algorithm::Sha256,
> -            "SHA512" => Algorithm::Sha512,
> +        Ok(match s.to_lowercase().as_str() {
> +            "sha1" => Algorithm::Sha1,
> +            "sha256" => Algorithm::Sha256,
> +            "sha512" => Algorithm::Sha512,
>              _ => return Err(Error::UnsupportedAlgorithm(s.to_string())),
>          })
>      }
> @@ -640,3 +640,23 @@ fn test_otp() {
>      assert_eq!(parsed.issuer.as_deref(), Some("An Issuer"));
>      assert_eq!(parsed.account_name.as_deref(), Some("The Account Name"));
>  }
> +
> +#[test]
> +fn test_algorithm_parsing() {
> +    let secret = "AA";
> +    let period = 30;
> +    let digits = 6;
> +    let issuer = "ISSUER";
> +    let uri = format!("otpauth://totp/user%40hostname?secret={secret}&issuer={issuer}&algorithm=sha1&digits={digits}&period={period}");
> +    let hotp: Totp = uri.parse().expect("failed to parse otp uri");
> +
> +    assert_eq!(hotp.algorithm, Algorithm::Sha1);
> +    assert_eq!(hotp.period, period);
> +    assert_eq!(hotp.digits, digits);
> +    assert_eq!(hotp.issuer.as_deref(), Some(issuer));
> +    assert_eq!(hotp.account_name.as_deref(), Some("user@hostname"));
> +    assert_eq!(
> +        &base32::encode(base32::Alphabet::RFC4648 { padding: false }, &hotp.secret()),
> +        secret
> +    )
> +}
> -- 
> 2.39.2