kilork / openid

OpenID Connect Rust Library
The Unlicense
63 stars 22 forks source link

Regression in at_hash behavior from Version 0.10.1 #45

Closed issackelly closed 1 year ago

issackelly commented 1 year ago

In the upgrade from base64 from 0.13 to 0.21 the behavior was lost where both unpadded or correctly padded strings were accepted. I am using dex which is currently sending me unpadded strings. I have not checked if this behavior is allowed as per the spec or not.


at_hash on  master [?] is 📦 v0.1.0 via 🐍 v3.10.12 (env) via 🦀 v1.70.0 
// base64 0.13.1

fn main() {
    let x = "zglPCMCEP7ilF3LP_NExow";
    match base64::decode_config(x, base64::URL_SAFE) {
        Ok(_) => println!("ok"),
        Err(e) => println!("error {}", e),
    };
}

❯ cargo run
   Compiling at_hash v0.1.0 (/home/issac/src/at_hash)
    Finished dev [unoptimized + debuginfo] target(s) in 0.21s
     Running `target/debug/at_hash`
ok

// base64 0.21
use base64::{engine::general_purpose::URL_SAFE, Engine as _};
fn main() {
    let x = "zglPCMCEP7ilF3LP_NExow";
    match URL_SAFE.decode(x) {
        Ok(_) => println!("ok"),
        Err(e) => println!("error {}", e),
    };
}

at_hash on  master [?] is 📦 v0.1.0 via 🐍 v3.10.12 (env) via 🦀 v1.70.0 
❯ cargo run             
   Compiling at_hash v0.1.0 (/home/issac/src/at_hash)
    Finished dev [unoptimized + debuginfo] target(s) in 0.21s
     Running `target/debug/at_hash`
error Invalid padding
issackelly commented 1 year ago

It appears that the spec is not thorough on this point, simply saying "base64url" which itself suggests that since we're always dealing with the first 128 bits, we may drop the padding. Would it be best to have the old behavior, where both methods are tried?

teohhanhui commented 1 year ago

Implementations MUST include appropriate pad characters at the end of encoded data unless the specification referring to this document explicitly states otherwise.

https://www.rfc-editor.org/rfc/rfc4648#section-3.2

Read together with that, it seems like the spec is clear. Padding is always required.

EDIT: lol - https://github.com/marshallpierce/rust-base64#i-want-canonical-base64-encodingdecoding

kilork commented 1 year ago

As change in the behavior was not intended, I consider this as regression.

issackelly commented 1 year ago

Thank you! I had been meaning to get around to this but we have an upstream patch so it was not pressing.