RustCrypto / RSA

RSA implementation in pure Rust
Apache License 2.0
550 stars 153 forks source link

Break after upgrade from v0.8 to v0.9 #329

Closed Xuanwo closed 1 year ago

Xuanwo commented 1 year ago

Hi, I found a possible break after upgrade from v0.8 to v0.9.

Backgound

I'm maintaining a lib that can generate signed url for google cloud storage services.

The code looks like the following:

https://github.com/Xuanwo/reqsign/blob/a3a6e962d1933fc9abf079ee24cde7873fe32565/src/google/signer.rs#L138-L141

let mut rng = rand::thread_rng();
let private_key = rsa::RsaPrivateKey::from_pkcs8_pem(&cred.private_key)?;
let signing_key = SigningKey::<sha2::Sha256>::new_with_prefix(private_key);
let signature = signing_key.sign_with_rng(&mut rng, string_to_sign.as_bytes());

Problem

After upgrading from version v0.8.1 to v0.9.1, this code became fragile and the related tests failed at a rate of 10%. However, after rolling back to version v0.8.1, it worked stably again.


Do you have any ideas? I'm willing to help get this issue fixed.

tarcieri commented 1 year ago

What is the actual error?

Also note pkcs1v15::SigningKey::new_with_prefix was deprecated: https://docs.rs/rsa/latest/rsa/pkcs1v15/struct.SigningKey.html#method.new_with_prefix

tarcieri commented 1 year ago

My best guess at what changed is LowerHex/UpperHex are used by the Display impl which you are calling via ToString, and they're now showing the unpadded value, rather than the padded value.

They should probably round trip serialize through SignatureEncoding.

tarcieri commented 1 year ago

330 might address the issue. Unfortunately we have no test on the fmt::{LowerHex, UpperHex} impls at the moment.

Xuanwo commented 1 year ago

Bravo! I confirmed that https://github.com/RustCrypto/RSA/pull/330 fixed this issue. Thanks for the quick response!

tarcieri commented 1 year ago

Fixed in #330