The `From<RSAPublicKey>` implementation for `VerifyingKey` returns unprefixed Keys, this should be documented

[ixolius]

I created a pair of RSA keys using Openssl . private.key.pem.txt public.key.pem.txt Now I have the following code:

use std::fs::File;
use std::io::Read;

use rsa::pkcs8::LineEnding;
use rsa::traits::PublicKeyParts;

use rsa::pkcs8::spki::EncodePublicKey;
use rsa::RsaPrivateKey;
use rsa::RsaPublicKey;
use rsa::pkcs1v15::VerifyingKey;
use rsa::pkcs8::{DecodePrivateKey, DecodePublicKey};
use rsa::pkcs1v15::{SigningKey, Signature};
use rsa::signature::DigestSigner;
use rsa::signature::{Keypair, SignatureEncoding, Verifier, Signer};
use rsa::sha2::{Digest, Sha256};

fn main() {

    let private_key = 
        RsaPrivateKey::read_pkcs8_pem_file("private.key.pem.txt")
        .expect("Could not read private key");
    let signing_key: SigningKey<Sha256> = SigningKey::new(private_key);
    let mut public_key_string = String::new();
    let mut public_key_file = File::open("public.key.pem.txt").unwrap();
    public_key_file.read_to_string(&mut public_key_string).unwrap();
    let public_key = RsaPublicKey::from_public_key_pem(&public_key_string).unwrap();
    let verifying_key_lib = signing_key.verifying_key();
    let verifying_key_openssl: VerifyingKey<Sha256> = VerifyingKey::from(public_key.clone());
    //test public key equality
    let n_lib = verifying_key_lib.as_ref().n();
    let n_openssl = verifying_key_openssl.as_ref().n();
    assert_eq!(n_lib,n_openssl);
    let e_lib = verifying_key_lib.as_ref().e();
    let e_openssl = verifying_key_openssl.as_ref().e();
    assert_eq!(e_lib, e_openssl);
    let size_lib = verifying_key_lib.as_ref().size();
    let size_openssl = verifying_key_openssl.as_ref().size();
    assert_eq!(size_lib, size_openssl);

    // Sign
    let data = b"hello world";
    let mut digest = <Sha256 as Digest>::new();
    digest.update(data);
    let sig2 = signing_key.sign_digest(digest);
    let signature = <SigningKey<Sha256> as Signer<Signature>>::sign(&signing_key, data);
    println!("Public key (openssl): {}", verifying_key_openssl.as_ref().to_public_key_pem(LineEnding::LF).unwrap());
    println!("Public key (lib): {}", public_key.to_public_key_pem(LineEnding::LF).unwrap());
    assert_ne!(signature.to_bytes().as_ref(), data.as_slice());

    // Verify
    verifying_key_openssl.verify(data, &signature)
        .unwrap_or_else(|e|  println!("Error verifying data with direct signature: {}", e));
    verifying_key_openssl.verify(data, &sig2)
        .unwrap_or_else(|e|  println!("Error verifying data with digest signature: {}", e));
    verifying_key_lib.verify(data, &signature)
        .unwrap_or_else(|e|  println!("Error verifying data with direct signature (lib): {}", e));
    verifying_key_lib.verify(data, &sig2)
        .unwrap_or_else(|e|  println!("Error verifying data with digest signature (lib): {}", e));
}

This works for the lib Key, but not for the openssl key. And there are supposed to be equal!

However, if i change

 let verifying_key_openssl: VerifyingKey<Sha256> = VerifyingKey::from(public_key.clone());

to

let verifying_key_openssl: VerifyingKey<Sha256> = VerifyingKey::new(public_key.clone());

each verification passes. It took me hours to debug this. The problem is the definition of the from method.

impl<D> From<RsaPublicKey> for VerifyingKey<D>
where
    D: Digest,
{
    fn from(key: RsaPublicKey) -> Self {
        Self::new_unprefixed(key)
    }
}

(from: https://docs.rs/rsa/0.9.2/src/rsa/pkcs1v15/verifying_key.rs.html#167-169)

I'm not sure whether it is possible to modify the implementation and maybe there are good reasons for this behavior. But there should be something like a warning for the from method in the documentation, like this:

The resulting VerifyingKey is unprefixed, which is uncommon. In most cases you’ll want to use VerifyingKey::new instead.

as it is done with VerifyingKey::new_unprefixed.

This issue is closely related to #238

The Cargo.toml for the example code is Cargo.toml.txt

[tarcieri]

It seems like we should just change the implementation to call ::new.

It's a breaking change though, so it would have to be done in a v0.10 release.

[ixolius]

In my opinion, changing the implementation of ::new would be the preferable option. I'd offer to file a PR, if necessary.

[tarcieri]

You can open a PR and we can make the change in the next breaking release.