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

Add RSA mechanisms and key serialization format #89

Merged
merged 3 commits into from
Feb 8, 2023
Merged

Conversation

sosthene-nitrokey
Copy link
Contributor

We will port the RSA implementation to a custom backend. While backends can have extensions, they can't have custom Mechanisms and Serialization for the normal APIs. This patch adds them so that they can be implemented by the custom backend.

@robin-nitrokey
Copy link
Member

I think we also need some changes to the constants in config, right?

main...Nitrokey:trussed:main#diff-cba64c21ab992eaad29fce147a08f4560a4769bc14682b8a96081a5fd02dbecd

@sosthene-nitrokey
Copy link
Contributor Author

Yes probably. I'm making the migration to a custom Backend in parallel and I will keep this as draft until I'm sure everything is enough.

@sosthene-nitrokey
Copy link
Contributor Author

This branch works with https://github.com/Nitrokey/trussed-rsa-backend

@sosthene-nitrokey sosthene-nitrokey marked this pull request as ready for review February 6, 2023 14:02
@sosthene-nitrokey
Copy link
Contributor Author

Looks like the CI has some issues downloading stuff to setup the docker image

@sosthene-nitrokey
Copy link
Contributor Author

I don't really understand why the CI fails

@daringer
Copy link
Member

daringer commented Feb 6, 2023

Yeah that's weird, especially those are all urls within Azure that fail, maybe some patience will help here. On a first glance this doesn't look like something we can debug...

@sosthene-nitrokey
Copy link
Contributor Author

There are similar errors with #71

@daringer
Copy link
Member

daringer commented Feb 6, 2023

yup yup, good indication that we should give it like 24h or something

@sosthene-nitrokey
Copy link
Contributor Author

Looks like the CI is fixed

src/config.rs Outdated
Comment on lines 47 to 48
// This is due to the fact that KEY_MATERIAL_LENGTH is bigger than MESSAGE_LENGTH for RSA.
pub const MAX_MESSAGE_LENGTH: usize = 1024;
Copy link
Member

Choose a reason for hiding this comment

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

The comment implies that the value changed, but we still use 1024. Which one is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a remnant of the original PRs for RSA. I removed it.

src/config.rs Show resolved Hide resolved
///
/// Since RSA keys have multiple parts, and that the [`SerializeKey`](crate::api::Reply::SerializeKey) and
/// [`UnsafeInjectKey`](crate::api::Request::UnsafeInjectKey) have only transfer one byte array, the RSA key is serialized with postcard
RsaParts,
Copy link
Member

Choose a reason for hiding this comment

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

How about calling this Serde or PostcardSerde to make it potentially reusable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to keep it generic and let the backend decide on the actual data format (for now at least)

Copy link
Member

@nickray nickray Feb 7, 2023

Choose a reason for hiding this comment

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

Internal or Private then? You could also re-use Raw if this is an internal implementation detail. The motivation is to keep serialization formats few and stable, as this concerns both the long-term API and the long-term storage of keys.

Copy link
Contributor Author

@sosthene-nitrokey sosthene-nitrokey Feb 7, 2023

Choose a reason for hiding this comment

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

and the long-term storage of keys

The serialization format doesn't affect how they're stored.

I don't want to reuse Raw as it feels misleading. Unlike ECC, there is no standard "raw" byte representation of RSA keys.

The "Parts" is in contrast to the PKCS format. The PKCS format is defined, but the "Parts" formats contains each element of a public key separately. The "part" name is more representative than "Internal" or "Private". "Serde" feels out of place. The use of postcard and serde is more of a workaround that the actual goal of the API. If you look at the traits in trussed-rsa-backend, they provide APIs directly over the structures, and hide away the parsing/serializing with postcard and serde (when possible):

fn unsafe_inject_rsa4096<'c>(
    &'c mut self,
    key_parts: RsaImportFormat,
    attributes: StorageAttributes,
) -> ClientResult<'c, reply::UnsafeInjectKey, Self>;

fn deserialize_rsa4096_public_key<'c>(
    &'c mut self,
    key_parts: RsaPublicParts,
    attributes: StorageAttributes,
) -> ClientResult<'c, reply::DeserializeKey, Self>;

Ideally I would even merge the RsaImportFormat an RsaPublicParts structures into Trussed proper but I want to be sure we can have something that fits all backends and use-cases first.

Maybe we should have mechanism extensions? That would mean more generics though.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I understand. My point is just that Pkcs8Der is an obviously valid general purpose format to upstream, while RsaParts is not. So can I'd like to merge this without this RsaParts. I believe you can use Raw (or we add something generic but general-purpose like Experimental) for your current work, and once it's set in stone, do a follow-up PR?

Copy link
Member

Choose a reason for hiding this comment

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

Or what about Parts :) That way you have your specific format to use, but unrelated algorithms might find a use too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point is just that Pkcs8Der is an obviously valid general purpose format to upstream, while RsaParts is not

I disagree. Whatever we do we will need a way to access specific parts of RSA keys. Both PIV and OpenPGP need that (OpenPGP needs it for both public and private keys, PIV at least for public keys). Pkcs8Der will also be incompatible with the SE050.

Or what about Parts :) That way you have your specific format to use, but unrelated algorithms might find a use too.

I'm not a fan of loosing readability for more flexibility when there are no obvious benefits to it. Do you know of any other algorithm that would use this?

@nickray nickray merged commit 20c7d62 into main Feb 8, 2023
@nickray nickray deleted the rsa-mechanism branch February 8, 2023 00:29
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.

4 participants