Swappable RSADP for YubiKey support

[str4d]

While working on yubikey-piv I was testing the RSA logic by implementing OAEP around it. What I ended up doing was copying in all the logic from #18, and replacing:

 let mut em = {
 let mut c = BigUint::from_bytes_be(ciphertext);
 let mut m = internals::decrypt(rng, priv_key, &c)?;
 let em = internals::left_pad(&m.to_bytes_be(), k);

 c.zeroize();
 m.zeroize();

 em
 };

with

 let mut em = {
 let m = yubikey.decrypt_data(&ciphertext, algorithm, slot).unwrap();
 // I forgot to check if the padding was actually necessary
 left_pad(&m, k)
 };

It seems like the way to achieve this more generally (across all decryption schemes) would be with a trait of the form:

trait PrivateKey {
 /// Do NOT use directly! Only for implementors.
 fn raw_decryption_primitive<R: Rng>(
 &self,
 rng: Option<&mut R>,
 ciphertext: &[u8],
 ) -> Result<Vec<u8>>;

 /// Decrypt the given message.
 fn decrypt(&self, padding: PaddingScheme, ciphertext: &[u8]) -> Result<Vec<u8>> {
 ...
 }

 /// Decrypt the given message.
 /// Uses `rng` to blind the decryption process.
 pub fn decrypt_blinded<R: Rng>(
 &self,
 rng: &mut R,
 padding: PaddingScheme,
 ciphertext: &[u8],
 ) -> Result<Vec<u8>> {
 ...
 }

 fn decrypt_oaep(...) -> Result<...> {
 oaep::decrypt(...)
 }
}

impl PrivateKey for RSAPrivateKey {
 fn raw_decryption_primitive<R: Rng>(
 &self,
 rng: Option<&mut R>,
 ciphertext: &[u8],
 ) -> Result<Vec<u8>> {
 let mut c = BigUint::from_bytes_be(ciphertext);
 let mut m = internals::decrypt(rng, priv_key, &c)?;
 let em = internals::left_pad(&m.to_bytes_be(), k);

 c.zeroize();
 m.zeroize();

 em
 }
}

Then in yubikey-piv we could do something like:

struct YubiKeyRsaPrivateKey {
 yubikey: &mut YubiKey,
 algorithm: AlgorithmId,
 slot: SlotId,
}

impl PrivateKey for YubiKeyRsaPrivateKey {
 fn raw_decryption_primitive<R: Rng>(
 &self,
 _rng: Option<&mut R>,
 ciphertext: &[u8],
 ) -> Result<Vec<u8>> {
 self.yubikey.decrypt_data(&ciphertext, self.algorithm, self.slot).map_err(|e| e.into())
 }
}

Thoughts? The part I dislike about the above sketch is that the raw RSA decryption primitive is exposed in the API, but this needs to happen somewhere if this kind of interoperability and code de-duplication were to happen at all.

[dignifiedquire]

I like the idea of having this be a trait, and allow code reuse like this, especially as adding yubikey and smart card support is sth I would really like to see.

I think if we add a trait like this we could mark it as unsafe, to indicate the risk involved (I know this abuses a little bit the definition of unsafe, but seems worth the added explicitness). In addition it could be hidden behind a feature flag, disabled by default, such that if we just use this crate, you don't accidentally use it.

[tarcieri]

I think if we add a trait like this we could mark it as unsafe, to indicate the risk involved (I know this abuses a little bit the definition of unsafe, but seems worth the added explicitness).

I would strongly recommend against using unsafe for anything other than memory safety.

[str4d]

A disabled-by-default feature flag is probably the right way to handle this, along with clear documentation. Another possibility might be to have trait DecryptionPrimitive in an underlying rsa-core crate that just has the primitive API, and trait PrivateKey: DecryptionPrimitive in the rsa crate (kind of like the split between RngCore and Rng in the rand crate stack).

[str4d]

So, was there a preference between exposing DecryptionPrimitive and EncryptionPrimitive via a default-off feature flag, or by moving DecryptionPrimitive and EncryptionPrimitive into an rsa-core crate? That seems to be the final blocker for external implementations of the PrivateKey and PublicKey traits.

I figure that even if we created an rsa-core crate, the algorithm-specific methods like rsa::oaep::decrypt would still live here, so we'd either need to decide to have them in the normal public API (instead of only exposed via concrete wrappers like RsaPrivateKey::decrypt), or have a feature flag that exposes them. If the latter, I think it should be a different flag from the one that exposes DecryptionPrimitive if any, because that enables someone to expose the algorithm-specific methods for use with some other crate's implementation of PrivateKey (like the yubikey crate), without exposing DecryptionPrimitive within their own namespace.

[dignifiedquire]

I generally prefer flags over adding another crate in this case.

[tarcieri]

These were added and ultimately removed in #300.

The internals now decouple padding from the RSA operation, which makes it potentially possible to reuse the padding implementation for these cases.

There are also now traits for both signing and encryption that can be implemented by 3rd party RSA providers like the yubikey crate.