Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added serdect support #420

Merged
merged 18 commits into from
Apr 5, 2024
Merged

added serdect support #420

merged 18 commits into from
Apr 5, 2024

Conversation

LWEdslev
Copy link
Contributor

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

DecryptingKey
EncryptingKey
Signature
SigningKey<D>
VerifyingKey<D>

#419

@tarcieri
Copy link
Member

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 ?

@LWEdslev
Copy link
Contributor Author

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?

@tarcieri
Copy link
Member

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.

@LWEdslev
Copy link
Contributor Author

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

@dignifiedquire
Copy link
Member

sure 👍

@LWEdslev
Copy link
Contributor Author

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.

@tarcieri
Copy link
Member

@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.

@LWEdslev
Copy link
Contributor Author

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

@LWEdslev
Copy link
Contributor Author

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

Cargo.toml Outdated Show resolved Hide resolved
@LWEdslev LWEdslev changed the title added serde support for src/pkcs1v15 structs added serdect support Mar 22, 2024
@LWEdslev
Copy link
Contributor Author

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

@tarcieri
Copy link
Member

Sure

@LWEdslev
Copy link
Contributor Author

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.

@tarcieri
Copy link
Member

Can you post the code that isn’t working?

@LWEdslev
Copy link
Contributor Author

LWEdslev commented Mar 22, 2024

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>>`

src/pss/signature.rs Outdated Show resolved Hide resolved
src/pss/signature.rs Outdated Show resolved Hide resolved
src/pss/signing_key.rs Outdated Show resolved Hide resolved
src/pss/signing_key.rs Outdated Show resolved Hide resolved
src/pss/verifying_key.rs Outdated Show resolved Hide resolved
src/pss/verifying_key.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

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

@LWEdslev
Copy link
Contributor Author

Yeah, that's better

@LWEdslev
Copy link
Contributor Author

LWEdslev commented Mar 27, 2024

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

@tarcieri
Copy link
Member

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

@LWEdslev
Copy link
Contributor Author

@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.

@tarcieri
Copy link
Member

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

@LWEdslev LWEdslev requested a review from tarcieri March 27, 2024 20:59
@LWEdslev
Copy link
Contributor Author

@tarcieri Will you review this when you have time?

@tarcieri
Copy link
Member

tarcieri commented Apr 1, 2024

Yes

D: Digest,
{
fn eq(&self, other: &Self) -> bool {
self.inner == other.inner && self.salt_len == other.salt_len
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I just noticed the derived Eq/PartialEq on RsaPrivateKey are not constant-time, which seems bad, although not directly related to this PR except it's indirectly invoking it.

private_key_info
.algorithm
.assert_algorithm_oid(ID_RSASSA_PSS)?;
verify_algorithm_id(&private_key_info.algorithm)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is a fix for handling PKCS#8 keys with PSS OIDs

Cargo.toml Outdated Show resolved Hide resolved
@tarcieri tarcieri merged commit 4512b5a into RustCrypto:master Apr 5, 2024
9 checks passed
tarcieri added a commit that referenced this pull request Jun 5, 2024
Co-authored-by: Tony Arcieri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants