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.

Sign up for GitHub

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

Jump to bottom

Add RSA mechanisms and key serialization format #89

Merged
merged 3 commits into from
Feb 8, 2023
Merged

Add RSA mechanisms and key serialization format #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0706676
Add RSA mechanisms and key serialization
sosthene-nitrokey Feb 2, 2023
7fa9d07
Add RSA key kinds
sosthene-nitrokey Feb 3, 2023
b732ec3
Fix key serialization length to accomodate RSA
sosthene-nitrokey Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ pub mod request {

DeserializeKey:
- mechanism: Mechanism
- serialized_key: Message
- serialized_key: SerializedKey
- format: KeySerialization
- attributes: StorageAttributes

Expand Down Expand Up @@ -282,7 +282,7 @@ pub mod request {

UnsafeInjectKey:
- mechanism: Mechanism // -> implies key type
- raw_key: ShortData
- raw_key: SerializedKey
- attributes: StorageAttributes
- format: KeySerialization

Expand Down Expand Up @@ -443,7 +443,7 @@ pub mod reply {
- bytes: Message

SerializeKey:
- serialized_key: Message
- serialized_key: SerializedKey

Sign:
- signature: Signature
Expand Down
4 changes: 2 additions & 2 deletions src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ pub trait CryptoClient: PollClient {
attributes: StorageAttributes,
) -> ClientResult<'c, reply::DeserializeKey, Self> {
let serialized_key =
Message::from_slice(serialized_key).map_err(|_| ClientError::DataTooLarge)?;
SerializedKey::from_slice(serialized_key).map_err(|_| ClientError::DataTooLarge)?;
self.request(request::DeserializeKey {
mechanism,
serialized_key,
Expand Down Expand Up @@ -478,7 +478,7 @@ pub trait CryptoClient: PollClient {
) -> ClientResult<'_, reply::UnsafeInjectKey, Self> {
self.request(request::UnsafeInjectKey {
mechanism,
raw_key: ShortData::from_slice(raw_key).unwrap(),
raw_key: SerializedKey::from_slice(raw_key).unwrap(),
attributes: StorageAttributes::new().set_persistence(persistence),
format,
})
Expand Down
12 changes: 8 additions & 4 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@ pub type MAX_OBJECT_HANDLES = consts::U16;
pub type MAX_LABEL_LENGTH = consts::U256;
pub const MAX_MEDIUM_DATA_LENGTH: usize = 256;
pub type MAX_PATH_LENGTH = consts::U256;
pub const MAX_KEY_MATERIAL_LENGTH: usize = 128;
// must be above + 4
pub const MAX_SERIALIZED_KEY_LENGTH: usize = 132;
cfg_if::cfg_if! {
if #[cfg(feature = "clients-12")] {
pub type MAX_SERVICE_CLIENTS = consts::U12;
Expand Down Expand Up @@ -44,7 +41,14 @@ cfg_if::cfg_if! {
}
}
pub const MAX_SHORT_DATA_LENGTH: usize = 128;
pub const MAX_SIGNATURE_LENGTH: usize = 72;

pub const MAX_SIGNATURE_LENGTH: usize = 512 * 2;
// FIXME: Value from https://stackoverflow.com/questions/5403808/private-key-length-bytes for Rsa2048 Private key
pub const MAX_KEY_MATERIAL_LENGTH: usize = 1160 * 2 + 72;
nickray marked this conversation as resolved.
Show resolved Hide resolved

// must be MAX_KEY_MATERIAL_LENGTH + 4
pub const MAX_SERIALIZED_KEY_LENGTH: usize = MAX_KEY_MATERIAL_LENGTH + 4;

pub const MAX_USER_ATTRIBUTE_LENGTH: usize = 256;

pub const USER_ATTRIBUTE_NUMBER: u8 = 37;
9 changes: 9 additions & 0 deletions src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ pub enum Kind {
Symmetric(usize),
/// 32B symmetric key + nonce, the parameter is the length of the nonce in bytes
Symmetric32Nonce(usize),
Rsa2048,
Rsa3072,
Rsa4096,
Ed255,
P256,
X255,
Expand Down Expand Up @@ -150,6 +153,9 @@ impl Kind {
Kind::Ed255 => 4,
Kind::P256 => 5,
Kind::X255 => 6,
Kind::Rsa2048 => 7,
Kind::Rsa3072 => 8,
Kind::Rsa4096 => 9,
}
}

Expand All @@ -161,6 +167,9 @@ impl Kind {
4 => Self::Ed255,
5 => Self::P256,
6 => Self::X255,
7 => Kind::Rsa2048,
8 => Kind::Rsa3072,
9 => Kind::Rsa4096,
_ => return Err(Error::InvalidSerializedKey),
})
}
Expand Down
2 changes: 1 addition & 1 deletion src/mechanisms/ed255.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ impl SerializeKey for super::Ed255 {
}

KeySerialization::Raw => {
let mut serialized_key = Message::new();
let mut serialized_key = SerializedKey::new();
serialized_key
.extend_from_slice(public_key.as_bytes())
.map_err(|_| Error::InternalError)?;
Expand Down
5 changes: 3 additions & 2 deletions src/mechanisms/p256.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ impl SerializeKey for super::P256 {
crate::cbor_serialize_bytes(&cose_pk).map_err(|_| Error::CborError)?
}
KeySerialization::Raw => {
let mut serialized_key = Message::new();
let mut serialized_key = SerializedKey::new();
serialized_key
.extend_from_slice(&public_key.x())
.map_err(|_| Error::InternalError)?;
Expand All @@ -234,12 +234,13 @@ impl SerializeKey for super::P256 {
serialized_key
}
KeySerialization::Sec1 => {
let mut serialized_key = Message::new();
let mut serialized_key = SerializedKey::new();
serialized_key
.extend_from_slice(&public_key.to_compressed_sec1_bytes())
.map_err(|_| Error::InternalError)?;
serialized_key
}
_ => return Err(Error::InvalidSerializationFormat),
};

Ok(reply::SerializeKey { serialized_key })
Expand Down
2 changes: 1 addition & 1 deletion src/mechanisms/shared_secret.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ impl SerializeKey for super::SharedSecret {
if !key.flags.contains(key::Flags::SERIALIZABLE) {
return Err(Error::InvalidSerializedKey);
};
let mut serialized_key = Message::new();
let mut serialized_key = SerializedKey::new();
serialized_key.extend_from_slice(&key.material).unwrap();

Ok(reply::SerializeKey { serialized_key })
Expand Down
2 changes: 1 addition & 1 deletion src/mechanisms/x255.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ impl SerializeKey for super::X255 {
let key_id = request.key;
let public_key = load_public_key(keystore, &key_id)?;

let mut serialized_key = Message::new();
let mut serialized_key = SerializedKey::new();
match request.format {
KeySerialization::Raw => {
serialized_key
Expand Down
3 changes: 2 additions & 1 deletion src/store/keystore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use chacha20::ChaCha8Rng;
use littlefs2::path::PathBuf;

use crate::{
config::MAX_KEY_MATERIAL_LENGTH,
error::{Error, Result},
key,
store::{self, Store},
Expand Down Expand Up @@ -172,7 +173,7 @@ impl<S: Store> Keystore for ClientKeystore<S> {

let location = self.location(secrecy, id).ok_or(Error::NoSuchKey)?;

let bytes: Bytes<128> = store::read(self.store, location, &path)?;
let bytes: Bytes<{ MAX_KEY_MATERIAL_LENGTH }> = store::read(self.store, location, &path)?;

let key = key::Key::try_deserialize(&bytes)?;

Expand Down
10 changes: 10 additions & 0 deletions src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,13 +568,17 @@ pub enum Mechanism {
X255,
/// Used to serialize the output of a diffie-hellman
SharedSecret,
Rsa2048Pkcs1v15,
Rsa3072Pkcs1v15,
Rsa4096Pkcs1v15,
}

pub type LongData = Bytes<MAX_LONG_DATA_LENGTH>;
pub type MediumData = Bytes<MAX_MEDIUM_DATA_LENGTH>;
pub type ShortData = Bytes<MAX_SHORT_DATA_LENGTH>;

pub type Message = Bytes<MAX_MESSAGE_LENGTH>;
pub type SerializedKey = Bytes<MAX_KEY_MATERIAL_LENGTH>;

#[derive(Copy, Clone, Eq, PartialEq, Debug, Serialize, Deserialize)]
pub enum KeySerialization {
Expand All @@ -584,6 +588,12 @@ pub enum KeySerialization {
EcdhEsHkdf256,
Raw,
Sec1,
/// Used by backends implementing RSA.
///
/// 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 •
edited
Loading

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 •
edited
Loading

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?

Pkcs8Der,
}

pub type Signature = Bytes<MAX_SIGNATURE_LENGTH>;
Expand Down