RSA icon indicating copy to clipboard operation
RSA copied to clipboard

added serde support for src/pkcs1v15 structs

Open LWEdslev opened this issue 1 year ago • 5 comments

Added serde support (Serialization, Deserialization) for structs in src/pkcs1v15/:

DecryptingKey
EncryptingKey
Signature
SigningKey<D>
VerifyingKey<D>

#419

LWEdslev avatar Mar 18 '24 19:03 LWEdslev

Since all of these have a straightforward serialization to bytes, it would be good to use that, rather than custom derive.

@dignifiedquire perhaps we should use the serdect crate for this like in e.g. https://github.com/rustcrypto/elliptic-curves ?

tarcieri avatar Mar 18 '24 19:03 tarcieri

Since all of these have a straightforward serialization to bytes, it would be good to use that, rather than custom derive.

@dignifiedquire perhaps we should use the serdect crate for this like in e.g. https://github.com/rustcrypto/elliptic-curves ?

Why has serdect not been used in src/key.rs PrivateKey?

LWEdslev avatar Mar 18 '24 20:03 LWEdslev

It hasn't been used in the rsa crate at all yet, but has opinions around how data is serialized, namely it uses strategies which are constant time across various Serde serializers, regardless of if they're for binary or human readable formats (where hex is used to encode the latter), which is important when serializing private keys.

tarcieri avatar Mar 18 '24 20:03 tarcieri

perhaps we should use the serdect

So kinda like this?

impl Serialize for Signature {
    fn serialize<S>(&self, serializer: S) -> core::result::Result<S::Ok, S::Error>
    where
        S: serdect::serde::Serializer,
    {
        serdect::slice::serialize_hex_lower_or_bin(&self.inner.to_bytes_be(), serializer)
    }
}

impl<'de> Deserialize<'de> for Signature {
    fn deserialize<D>(deserializer: D) -> core::result::Result<Self, D::Error>
    where
        D: serdect::serde::Deserializer<'de>,
    {
        let bytes = serdect::slice::deserialize_hex_or_bin_vec(deserializer)?;
        let inner = BigUint::from_bytes_be(&bytes);

        Ok(Self {
            inner,
            len: bytes.len(),
        })
    }
}

I can also do the others

LWEdslev avatar Mar 19 '24 07:03 LWEdslev

sure 👍

dignifiedquire avatar Mar 19 '24 08:03 dignifiedquire

I guess only Signature has a straight forward serdect implementation, since the other structs rely on RsaPublicKey, RsaPrivateKey and more, so that's gonna have to do for now.

LWEdslev avatar Mar 19 '24 11:03 LWEdslev

@LWEdslev the elliptic-curve crates use the SPKI serialization for public keys. You could do something similar for RsaPublicKey and VerifyingKey at least: computing the SPKI serialization with EncodePublicKey::to_public_key_der, and then encoding that using serdect.

tarcieri avatar Mar 19 '24 14:03 tarcieri

@tarcieri I see, I'll give it a whirl

LWEdslev avatar Mar 19 '24 14:03 LWEdslev

RsaPublicKey, RsaPrivateKey and the structs in pkcs1v15 now have the serdect implementations

LWEdslev avatar Mar 19 '24 23:03 LWEdslev

Should I add some serde_test tests like the key::tests::test_serde test for the new implementations?

LWEdslev avatar Mar 22 '24 11:03 LWEdslev

Sure

tarcieri avatar Mar 22 '24 14:03 tarcieri

Due to the PhantomData<D> fields that has D: Digest, I can't use PartialEq on all the structs and therefore I can't use serde_test::assert_tokens. I could not find a better solution than using serde_json for those structs.

LWEdslev avatar Mar 22 '24 17:03 LWEdslev

Can you post the code that isn’t working?

tarcieri avatar Mar 22 '24 17:03 tarcieri

Can you post the code that isn’t working?

For the oaep::EncryptingKey which uses the D: Digest generic I tried something like this:

#[test]
    #[cfg(feature = "serde")]
    fn test_serde() {
        use super::*;
        use rand_chacha::{rand_core::SeedableRng, ChaCha8Rng};
        use serde_test::{assert_tokens, Configure, Token};

        let mut rng = ChaCha8Rng::from_seed([42; 32]);
        let priv_key = crate::RsaPrivateKey::new(&mut rng, 64).expect("failed to generate key");
        let encrypting_key = EncryptingKey::<sha2::Sha256>::new(priv_key.to_public_key());

        let tokens = [
            Token::Struct { name: "EncryptingKey", len: 2 },
            Token::Str("inner"),
            Token::Str("3024300d06092a864886f70d01010105000313003010020900cc6c6130e35b46bf0203010001"),
            Token::Str("label"),
            Token::Str("None"),
            Token::StructEnd,
        ];
        assert_tokens(&encrypting_key.readable(), &tokens);
    }

To use the assert_tokens I need to add PartialEq to the EcnryptingKey struct. Then I get an error when running cargo test --features serde:

error[E0277]: can't compare `CoreWrapper<CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>, B0>, OidSha256>>` with `CoreWrapper<CtVariableCoreWrapper<Sha256VarCore, UInt<UInt<UInt<UInt<UInt<UInt<UTerm, B1>, B0>, B0>, B0>, B0>, B0>, OidSha256>>`

LWEdslev avatar Mar 22 '24 18:03 LWEdslev

Now all of them have the serde_test tests

LWEdslev avatar Mar 22 '24 19:03 LWEdslev

So what's next?

LWEdslev avatar Mar 26 '24 16:03 LWEdslev

As a bit of a meta comment on the above suggestions: all of these types have built-in serialization support that the serde impls can be thin wrappers for, rather than duplicating. It shouldn't be necessary to reach into inner, for example

tarcieri avatar Mar 26 '24 16:03 tarcieri

Yeah, that's better

LWEdslev avatar Mar 26 '24 19:03 LWEdslev

How do I make the pss::SigningKey use the correct algorithm OID when decoding? EncodePrivateKey uses pkcs1::ALGORITHM_ID but ID_RSASSA_PSS is required.

Edit: I see that this is addressed in #422

LWEdslev avatar Mar 27 '24 18:03 LWEdslev

@LWEdslev either OID should work now (pkcs1::ALGORITHM_ID or ID_RSASSA_PSS), now that #424 is merged, though you may need to rebase

tarcieri avatar Mar 27 '24 18:03 tarcieri

@tarcieri I don't see how either OID should work for the pss::SigningKey. With the merge of #424 the TryFrom<pkcs8::PrivateKeyInfo<'_>> for SigningKey<D> asserts

private_key_info
            .algorithm
            .assert_algorithm_oid(ID_RSASSA_PSS)?;

ID_RSASSA_PSS is the OID 1.2.840.113549.1.1.10 but the encoding is done with

self.inner.to_pkcs8_der()

And the inner RsaPrivateKey has EncodePrivateKey implemented such that algorithms (pkcs1::ALGORITHM_ID) OID is 1.2.840.113549.1.1.1 So encoding a pss::SigningKey it can't be decoded back into itself.

LWEdslev avatar Mar 27 '24 20:03 LWEdslev

@LWEdslev aah, so it does. It should probably use verify_algorithm_id which accepts either

tarcieri avatar Mar 27 '24 20:03 tarcieri

@tarcieri Will you review this when you have time?

LWEdslev avatar Mar 31 '24 16:03 LWEdslev

Yes

tarcieri avatar Apr 01 '24 16:04 tarcieri