Remove primitive traits

[lumag]

This started as an attempt to remove traits implemented by src/raw.rs.

After removing them I stumbled upon the unused indirection on RsaPublicKey and RsaPrivateKey in the form of PublicKey and PrivateKey traits. We already have traits which can be implemented by high-level hardware accelerators: Signer/Verifiers and Encryptor/Decryptor families of API. Low level RSA implementations would more likely reimplement parts of crate::internal.

Thus this PR ended up dropping both families of traits: PublicKey, PrivateKey and
EncryptionPrimitive, DecriptionPrimitive.

[lumag]

cc @dignifiedquire @baloo @tarcieri

[tarcieri]

cc @str4d

I'm generally in favor of this change. However, I think there is value in using the rsa crate to take care of formatting messages for use with hardware devices that provide unpadded RSA private key operations.

I think we should provide a different API for that though: one that encodes/decodes padded messages according to a padding scheme, then you can plug those encoded messages into the accelerator. That's sort of what I was suggesting in #226.

[lumag]

Good question. Looking at the PKCS11, I'd expect that the hw crypto handles all the details (judging from CKM_RSA_PKCS_PSS or CKM_RSA_PKCS_OAEP).

Generally I think there are only two levels where hw accel can be plugged:

• raw RSA primitives (m^e mod n, m^d mod n)
• proper RSA implementation, including the message parsing and formatting (as seen for PKCS11, JavaCard, etc).

For the second one we obviously do not need any additional processing, for the first one we have to plug that into into high-level implementation. It would not be enough to just format the messages. One would have to dup all the checks and supporting code.

I hardly can imaging some "middle" form of implementation which would require just message formatting. But, I migth be wrong here. But if there is one, it might be better to change raw.rs to use BigUint for results too, move relevant code to key.rs and then rest of the code to internals.rs.

[tarcieri]

Good question. Looking at the PKCS11, I'd expect that the hw crypto handles all the details (judging from CKM_RSA_PKCS_PSS or CKM_RSA_PKCS_OAEP).

That's true of certain HSMs. However, many of the cryptographic accelerators for MCUs I'm familiar with provide only a low-level unpadded interface, and it would be nice if the rsa crate could be used for constructing the padded messages.

A YubiKey is one example: https://github.com/iqlusioninc/yubikey.rs/blob/0071566097bf4ca3b182d31547451581058593a3/src/certificate.rs#L399-L435

Anyway, this is all just something to keep in mind and probably better discussed on #226.

[tarcieri]

Approving this PR but I'd love to hear from @dignifiedquire and/or @str4d before merging

[lumag]

Thanks for the pointer, I'll take a look.
BTW: you made me wonder, so I started looking into an opposite direction: making all pss/pkcs1v15/oaep generic around PublicKey/PrivateKey implementations. Is this what you had in mind?

[tarcieri]

@lumag I was thinking something of the opposite direction: make all of the functionality for computing padded messages part of the public API, so anything that needs help constructing the padded messages can consume it. And rsa can consume that API internally.

That way we don't need to worry about building an API for cryptographic accelerators, which may need to e.g. use async for monitoring interrupts from the cryptographic accelerator.

[lumag]

Ah. I see. And sign_data is just m^d mod n?

[str4d]

#34 has the origins of and discussions about the APIs you are removing. The intention was that a crate like yubikey could eventually just implement DecryptionPrimitive, and get all the rest of the PrivateKey logic for the various RSA protocols from here. Then downstream users would just add rsa and yubikey as dependencies, and not be exposed to DecryptionPrimitive or any of the internal details that they could get wrong.

#32 is the issue for my original motivating use case of "just define OAEP etc. safely once inside the rsa crate and then enable it to be used with a YubiKey".

I don't mind what direction this crate goes, as I don't have a personally motivating use case for RSA-plus-YubiKey anymore (we used P256 instead for age-plugin-yubikey). But I do think that we should be making it as easy as possible for users to use RSA safely, which means simultaneously avoiding exposing the raw primitives unnecessarily, and avoiding exposing the internal details of specific schemes (or requiring users to reimplement them). Fun times.

Perhaps implementing an example of using rsa with the yubikey crate in an end-user application would be instructive for examining how the current and proposed future APIs would affect UX?

[lumag]

@str4d In the original design, what would be the user-visible type for the 'Rsa Private key handled by Yubikey'? It could not be RsaPrivateKey.

[lumag]

@tarcieri ok, I took a glance on separating the formatting to separate module. Intermediate question. What is the rule for zeroizing the ciphertext? Do we need to do that?

[tarcieri]

@lumag I left the original code that was doing that intact, however personally I don't see a practical reason to zeroize ciphertext... it's protected by cryptography!

[tarcieri]

The intention was that a crate like yubikey could eventually just implement DecryptionPrimitive, and get all the rest of the PrivateKey logic for the various RSA protocols from here

@str4d a system which exposes the encoding/decoding of padded messages would definitely be "some assembly required" versus one where you could just plug in an implementation of the primitive operations.

In the case of the existing EncryptionPrimitive trait, it has design problems around the current bag-of-bytes interface, which might be nice for cryptographic accelerators, but (problems of async interrupt-handling APIs aside) isn't necessarily a good fit for the software implementation, where converting to a BigUint more eagerly would help so we could perform checks against those BigUints more eagerly (so as to solve e.g. #272).

I think the traits being removed in this PR don't quite have a clear enough design to be useful for the intended use cases, which is why I'm in favor of removing them. They feel more like leaky abstractions which aren't actually being used to abstract over anything in practice, they're just complicating the implementation at present.

[lumag]

@tarcieri @str4d I moved all PKCS1v15/PSS/OAEP handling (except the RSA transformation) to separate module. These functions are not exported, but it would very easy to export them under the "hazmat" feature if the need arises (or to move them to some generic rsa-algo crate to be further reused by other implementations).

[tarcieri]

I like having all of the padding algorithms factored out separately. It should make it easier to reuse, if we can find the proper API for it.

[lumag]

I think the API is mostly there, we might need to meditate regarding BigUint vs &[u8]. I have been thinking about cutting out the internals / algorithms from the public API, but this is definitely a separate topic. Also we might as well cleanup the key API by removing the functions that just shortcut to the PaddingScheme/SignatureScheme calls.

[tarcieri]

Going to merge this and cut another prerelease