Skip to content

Part of moving Parsec to use psa-crypto #28

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

Merged
merged 9 commits into from
Jun 16, 2020
Merged

Part of moving Parsec to use psa-crypto #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
19b70b0
Added get_key_attributes for export.
sbailey-arm Jun 8, 2020
3d1580e
Generate, import and get_key_attributes close key handle on completio…
sbailey-arm Jun 10, 2020
d361726
Changed error handling when psa-crypto-sys operation fails - prioriti…
sbailey-arm Jun 12, 2020
6fd4469
Added drop method to clean up mbedtls resources.
sbailey-arm Jun 12, 2020
fd73ed1
Removed unecessary shim binding for get_key_attributes
sbailey-arm Jun 15, 2020
d539aed
Fixes for CI
sbailey-arm Jun 15, 2020
6f34968
Addressed comments received.
sbailey-arm Jun 15, 2020
2d7b6ae
Addresses further comments:
sbailey-arm Jun 16, 2020
999187c
Fix conflict between generate and import test ranges when running sum…
sbailey-arm Jun 16, 2020
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
27 changes: 8 additions & 19 deletions psa-crypto-sys/src/c/shim.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,32 +72,21 @@ const psa_algorithm_t shim_PSA_ALG_TLS12_PSK_TO_MS_BASE =
const psa_algorithm_t shim_PSA_ALG_KEY_DERIVATION_MASK =
PSA_ALG_KEY_DERIVATION_MASK;

psa_key_id_t shim_get_key_id(const psa_key_attributes_t *attributes);

psa_algorithm_t shim_get_key_algorithm(const psa_key_attributes_t *attributes);
psa_status_t shim_get_key_attributes(psa_key_handle_t key_handle, psa_key_attributes_t *attributes);
size_t shim_get_key_bits(const psa_key_attributes_t *attributes);

psa_key_id_t shim_get_key_id(const psa_key_attributes_t *attributes);
psa_key_lifetime_t shim_get_key_lifetime(const psa_key_attributes_t *attributes);
psa_key_type_t shim_get_key_type(const psa_key_attributes_t *attributes);
psa_key_lifetime_t
shim_get_key_lifetime(const psa_key_attributes_t *attributes);
psa_algorithm_t shim_get_key_algorithm(const psa_key_attributes_t *attributes);
psa_key_usage_t
shim_get_key_usage_flags(const psa_key_attributes_t *attributes);
psa_key_usage_t shim_get_key_usage_flags(const psa_key_attributes_t *attributes);
psa_key_attributes_t shim_key_attributes_init(void);

void shim_set_key_algorithm(psa_key_attributes_t *attributes,
psa_algorithm_t alg);

void shim_set_key_algorithm(psa_key_attributes_t *attributes, psa_algorithm_t alg);
void shim_set_key_bits(psa_key_attributes_t *attributes, size_t bits);

void shim_set_key_id(psa_key_attributes_t *attributes, psa_key_id_t id);

void shim_set_key_lifetime(psa_key_attributes_t *attributes,
psa_key_lifetime_t lifetime);

void shim_set_key_lifetime(psa_key_attributes_t *attributes, psa_key_lifetime_t lifetime);
void shim_set_key_type(psa_key_attributes_t *attributes, psa_key_type_t type_);

void shim_set_key_usage_flags(psa_key_attributes_t *attributes,
psa_key_usage_t usage_flags);
void shim_set_key_usage_flags(psa_key_attributes_t *attributes, psa_key_usage_t usage_flags);

int shim_PSA_ALG_IS_HASH(psa_algorithm_t alg);
int shim_PSA_ALG_IS_MAC(psa_algorithm_t alg);
Expand Down
15 changes: 10 additions & 5 deletions psa-crypto-sys/src/constants.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,26 @@ use super::types::*;
// PSA error codes
pub const PSA_SUCCESS: psa_status_t = 0;
pub const PSA_ERROR_GENERIC_ERROR: psa_status_t = -132;
pub const PSA_ERROR_NOT_SUPPORTED: psa_status_t = -134;
pub const PSA_ERROR_NOT_PERMITTED: psa_status_t = -133;
pub const PSA_ERROR_NOT_SUPPORTED: psa_status_t = -134;
pub const PSA_ERROR_INVALID_ARGUMENT: psa_status_t = -135;
pub const PSA_ERROR_INVALID_HANDLE: psa_status_t = -136;
pub const PSA_ERROR_BAD_STATE: psa_status_t = -137;
pub const PSA_ERROR_BUFFER_TOO_SMALL: psa_status_t = -138;
pub const PSA_ERROR_ALREADY_EXISTS: psa_status_t = -139;
pub const PSA_ERROR_DOES_NOT_EXIST: psa_status_t = -140;
pub const PSA_ERROR_BAD_STATE: psa_status_t = -137;
pub const PSA_ERROR_INVALID_ARGUMENT: psa_status_t = -135;
pub const PSA_ERROR_INSUFFICIENT_MEMORY: psa_status_t = -141;
pub const PSA_ERROR_INSUFFICIENT_STORAGE: psa_status_t = -142;
pub const PSA_ERROR_INSUFFICIENT_DATA: psa_status_t = -143;
pub const PSA_ERROR_COMMUNICATION_FAILURE: psa_status_t = -145;
pub const PSA_ERROR_STORAGE_FAILURE: psa_status_t = -146;
pub const PSA_ERROR_HARDWARE_FAILURE: psa_status_t = -147;
pub const PSA_ERROR_INSUFFICIENT_ENTROPY: psa_status_t = -148;
pub const PSA_ERROR_INVALID_SIGNATURE: psa_status_t = -149;
pub const PSA_ERROR_INVALID_PADDING: psa_status_t = -150;
pub const PSA_ERROR_INSUFFICIENT_DATA: psa_status_t = -143;
pub const PSA_ERROR_INVALID_HANDLE: psa_status_t = -136;
pub const PSA_ERROR_CORRUPTION_DETECTED: psa_status_t = -151;
pub const PSA_ERROR_DATA_CORRUPT: psa_status_t = -152;
pub const PSA_ERROR_DATA_INVALID: psa_status_t = -153;

pub const PSA_MAX_KEY_BITS: usize = 65528;
pub const PSA_KEY_TYPE_NONE: psa_key_type_t = 0;
Expand Down Expand Up @@ -87,6 +90,8 @@ pub const PSA_KEY_USAGE_DECRYPT: psa_key_usage_t = 512;
pub const PSA_KEY_USAGE_SIGN: psa_key_usage_t = 1024;
pub const PSA_KEY_USAGE_VERIFY: psa_key_usage_t = 2048;
pub const PSA_KEY_USAGE_DERIVE: psa_key_usage_t = 4096;
pub const PSA_KEY_ID_USER_MIN: psa_key_id_t = 0x0000_0001;
pub const PSA_KEY_ID_USER_MAX: psa_key_id_t = 0x3fff_ffff;

#[cfg(feature = "implementation-defined")]
pub const PSA_DRV_SE_HAL_VERSION: u32 = 5;
4 changes: 2 additions & 2 deletions psa-crypto-sys/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ pub use types::*;
#[cfg(feature = "implementation-defined")]
pub use psa_crypto_binding::{
psa_close_key, psa_crypto_init, psa_destroy_key, psa_export_public_key, psa_generate_key,
psa_import_key, psa_key_attributes_t, psa_open_key, psa_reset_key_attributes, psa_sign_hash,
psa_verify_hash,
psa_get_key_attributes, psa_import_key, psa_key_attributes_t, psa_open_key,
psa_reset_key_attributes, psa_sign_hash, psa_verify_hash,
};

// Secure Element Driver definitions
Expand Down
2 changes: 1 addition & 1 deletion psa-crypto-sys/vendor
Open in desktop
Submodule vendor updated from ac15f8 to 3c4a46
3 changes: 2 additions & 1 deletion psa-crypto/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ static INITIALISED: AtomicBool = AtomicBool::new(false);
/// Applications are permitted to call this function more than once. Once a call succeeds,
/// subsequent calls are guaranteed to succeed.
///
///
/// # Example
///
/// ```rust
Expand All @@ -72,7 +73,7 @@ static INITIALISED: AtomicBool = AtomicBool::new(false);
/// ```
#[cfg(feature = "with-mbed-crypto")]
pub fn init() -> Result<()> {
// It it not a problem to call psa_crypto_init more than once.
// It is not a problem to call psa_crypto_init more than once.
Status::from(unsafe { psa_crypto_sys::psa_crypto_init() }).to_result()?;
let _ = INITIALISED.compare_and_swap(false, true, Ordering::Relaxed);

Expand Down
15 changes: 7 additions & 8 deletions psa-crypto/src/operations/asym_signature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub fn sign_hash(
let mut signature_length = 0;
let handle = key.handle()?;

Status::from(unsafe {
let sign_res = Status::from(unsafe {
psa_crypto_sys::psa_sign_hash(
handle,
alg.into(),
Expand All @@ -75,10 +75,9 @@ pub fn sign_hash(
&mut signature_length,
)
})
.to_result()?;

.to_result();
key.close_handle(handle)?;

sign_res?;
Copy link
Member

Choose a reason for hiding this comment

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

That is actually quite clever 😄 Never thought of doing it like that before.

Comment on lines -79 to +80
Copy link
Member

Choose a reason for hiding this comment

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

For these cases where you need to do a cleanup operation that may fail (after some other core operation might've failed in the first place) we have to decide: if both fail, which error do we want to be returned? I'd say the error on the "core" operation should take precedence, in which case the two branches (as in, if the core operation failed or not) will be different

Copy link
Member

Choose a reason for hiding this comment

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

Also, I wish we had a better approach to this kind of problem where you have some cleanup to do whenever you exit the function. In this case it's fine since there's not much branching, but in some cases it might end up being a pain having to call that close_handle for each possible path before doing ?.

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 originally had it returning the first operation error, then the close handle error but after a brief discussion with @hug-dev, it was changed to how it is now. I think the reason was the PSA API states keys aren't used, IDs are, so it should be getting removed at some point (soon? 😄) anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that makes sense for this case, my comment was more general - we should have a statement somewhere as to how to prioritize these errors throughout the library. I'd even say that this applies to Parsec too. The last error to occur is not necessarily the one we should return.

Ok(signature_length)
}

Expand Down Expand Up @@ -130,7 +129,7 @@ pub fn verify_hash(key: Id, alg: AsymmetricSignature, hash: &[u8], signature: &[

let handle = key.handle()?;

Status::from(unsafe {
let verify_res = Status::from(unsafe {
psa_crypto_sys::psa_verify_hash(
handle,
alg.into(),
Expand All @@ -140,7 +139,7 @@ pub fn verify_hash(key: Id, alg: AsymmetricSignature, hash: &[u8], signature: &[
signature.len(),
)
})
.to_result()?;

key.close_handle(handle)
.to_result();
key.close_handle(handle)?;
verify_res
}
65 changes: 39 additions & 26 deletions psa-crypto/src/operations/key_management.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
//! # Key Management operations

use crate::initialized;
use crate::types::key::{Attributes, Id};
use crate::types::key::{Attributes, Id, Lifetime};
use crate::types::status::{Result, Status};
use core::convert::TryFrom;
use psa_crypto_sys::{psa_key_handle_t, psa_key_id_t};

/// Generate a key or a key pair
///
Expand Down Expand Up @@ -47,25 +48,19 @@ use core::convert::TryFrom;
/// ```
pub fn generate(attributes: Attributes, id: Option<u32>) -> Result<Id> {
initialized()?;

let mut attributes = psa_crypto_sys::psa_key_attributes_t::try_from(attributes)?;
let mut key_attributes = psa_crypto_sys::psa_key_attributes_t::try_from(attributes)?;
let id = if let Some(id) = id {
unsafe { psa_crypto_sys::psa_set_key_id(&mut attributes, id) };
unsafe { psa_crypto_sys::psa_set_key_id(&mut key_attributes, id) };
id
} else {
0
};
let mut handle = 0;

Status::from(unsafe { psa_crypto_sys::psa_generate_key(&attributes, &mut handle) })
Status::from(unsafe { psa_crypto_sys::psa_generate_key(&key_attributes, &mut handle) })
.to_result()?;
Attributes::reset(&mut key_attributes);

Attributes::reset(&mut attributes);

Ok(Id {
id,
handle: Some(handle),
})
complete_new_key_operation(attributes.lifetime, id, handle)
}

/// Destroy a key
Expand Down Expand Up @@ -113,8 +108,7 @@ pub fn generate(attributes: Attributes, id: Option<u32>) -> Result<Id> {
pub unsafe fn destroy(key: Id) -> Result<()> {
initialized()?;
let handle = key.handle()?;
Status::from(psa_crypto_sys::psa_destroy_key(handle)).to_result()?;
key.close_handle(handle)
Status::from(psa_crypto_sys::psa_destroy_key(handle)).to_result()
}

/// Import a key in binary format
Expand Down Expand Up @@ -166,26 +160,23 @@ pub unsafe fn destroy(key: Id) -> Result<()> {
pub fn import(attributes: Attributes, id: Option<u32>, data: &[u8]) -> Result<Id> {
initialized()?;

let mut attributes = psa_crypto_sys::psa_key_attributes_t::try_from(attributes)?;
let mut key_attributes = psa_crypto_sys::psa_key_attributes_t::try_from(attributes)?;
let id = if let Some(id) = id {
unsafe { psa_crypto_sys::psa_set_key_id(&mut attributes, id) };
unsafe { psa_crypto_sys::psa_set_key_id(&mut key_attributes, id) };
id
} else {
0
};
let mut handle = 0;

Status::from(unsafe {
psa_crypto_sys::psa_import_key(&attributes, data.as_ptr(), data.len(), &mut handle)
psa_crypto_sys::psa_import_key(&key_attributes, data.as_ptr(), data.len(), &mut handle)
})
.to_result()?;

Attributes::reset(&mut attributes);
Attributes::reset(&mut key_attributes);

Ok(Id {
id,
handle: Some(handle),
})
complete_new_key_operation(attributes.lifetime, id, handle)
}

/// Export a public key or the public part of a key pair in binary format
Expand Down Expand Up @@ -227,17 +218,39 @@ pub fn export_public(key: Id, data: &mut [u8]) -> Result<usize> {
let handle = key.handle()?;
let mut data_length = 0;

Status::from(unsafe {
let export_res = Status::from(unsafe {
psa_crypto_sys::psa_export_public_key(
handle,
data.as_mut_ptr(),
data.len(),
&mut data_length,
)
})
.to_result()?;

.to_result();
key.close_handle(handle)?;

export_res?;
Ok(data_length)
}

/// Completes a new key operation (either generate or import)
///
/// If key is not `Volatile` (`Persistent` or `Custom(u32)`), handle is closed.
///
/// If a key is `Volatile`, `Id` returned contains the key `handle`. Otherwise, it does not.
fn complete_new_key_operation(
key_lifetime: Lifetime,
id: psa_key_id_t,
handle: psa_key_handle_t,
) -> Result<Id> {
if key_lifetime != Lifetime::Volatile {
Status::from(unsafe { psa_crypto_sys::psa_close_key(handle) }).to_result()?;
}
Ok(Id {
id,
handle: if key_lifetime == Lifetime::Volatile {
Some(handle)
} else {
None
},
})
}
54 changes: 52 additions & 2 deletions psa-crypto/src/types/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@
//! # PSA Key types

#![allow(deprecated)]

#[cfg(feature = "with-mbed-crypto")]
use crate::initialized;
use crate::types::algorithm::{Algorithm, Cipher};
#[cfg(feature = "with-mbed-crypto")]
use crate::types::status::Status;
use crate::types::status::{Error, Result};
#[cfg(feature = "with-mbed-crypto")]
use core::convert::{TryFrom, TryInto};
use log::error;
pub use psa_crypto_sys::{self, psa_key_id_t, PSA_KEY_ID_USER_MAX, PSA_KEY_ID_USER_MIN};
use serde::{Deserialize, Serialize};

/// Native definition of the attributes needed to fully describe
Expand Down Expand Up @@ -255,6 +257,54 @@ impl Attributes {
pub(crate) fn reset(attributes: &mut psa_crypto_sys::psa_key_attributes_t) {
unsafe { psa_crypto_sys::psa_reset_key_attributes(attributes) };
}

/// Gets the attributes for a given key ID
///
/// The `Id` structure can be created with the `from_persistent_key_id` constructor on `Id`.
///
/// # Example
///
/// ```
/// # use psa_crypto::operations::key_management;
/// # use psa_crypto::types::key::{Attributes, Type, Lifetime, Policy, UsageFlags};
/// # use psa_crypto::types::algorithm::{AsymmetricSignature, Hash};
/// # let mut attributes = Attributes {
/// # key_type: Type::RsaKeyPair,
/// # bits: 1024,
/// # lifetime: Lifetime::Volatile,
/// # policy: Policy {
/// # usage_flags: UsageFlags {
/// # sign_hash: true,
/// # sign_message: true,
/// # verify_hash: true,
/// # verify_message: true,
/// # ..Default::default()
/// # },
/// # permitted_algorithms: AsymmetricSignature::RsaPkcs1v15Sign {
/// # hash_alg: Hash::Sha256.into(),
/// # }.into(),
/// # },
/// # };
/// psa_crypto::init().unwrap();
/// let my_key_id = key_management::generate(attributes, None).unwrap();
/// //...
/// let key_attributes = Attributes::from_key_id(my_key_id);
/// ```
#[cfg(feature = "with-mbed-crypto")]
pub fn from_key_id(key_id: Id) -> Result<Self> {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth having a test for this?

Copy link
Member

Choose a reason for hiding this comment

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

As there is an example, let's say it is fine for now but think about adding one in the future 🤡

initialized()?;
let mut key_attributes = unsafe { psa_crypto_sys::psa_key_attributes_init() };
let handle = key_id.handle()?;
let get_attributes_res = Status::from(unsafe {
psa_crypto_sys::psa_get_key_attributes(handle, &mut key_attributes)
})
.to_result();
let attributes = Attributes::try_from(key_attributes);
Attributes::reset(&mut key_attributes);
key_id.close_handle(handle)?;
get_attributes_res?;
Ok(attributes?)
}
}

/// The lifetime of a key indicates where it is stored and which application and system actions
Expand Down Expand Up @@ -473,7 +523,7 @@ pub struct UsageFlags {
#[cfg(feature = "with-mbed-crypto")]
#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct Id {
pub(crate) id: psa_crypto_sys::psa_key_id_t,
pub(crate) id: psa_key_id_t,
pub(crate) handle: Option<psa_crypto_sys::psa_key_handle_t>,
}

Expand Down
13 changes: 6 additions & 7 deletions psa-crypto/src/types/status.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ impl From<psa_crypto_sys::psa_status_t> for Status {
psa_crypto_sys::PSA_ERROR_INVALID_PADDING => Error::InvalidPadding.into(),
psa_crypto_sys::PSA_ERROR_INSUFFICIENT_DATA => Error::InsufficientData.into(),
psa_crypto_sys::PSA_ERROR_INVALID_HANDLE => Error::InvalidHandle.into(),
psa_crypto_sys::PSA_ERROR_CORRUPTION_DETECTED => Error::CorruptionDetected.into(),
psa_crypto_sys::PSA_ERROR_DATA_CORRUPT => Error::DataCorrupt.into(),
psa_crypto_sys::PSA_ERROR_DATA_INVALID => Error::DataInvalid.into(),
s => {
error!("{} not recognised as a valid PSA status.", s);
Status::Error(Error::GenericError)
Expand Down Expand Up @@ -218,19 +221,15 @@ impl From<Error> for psa_crypto_sys::psa_status_t {
Error::InsufficientStorage => psa_crypto_sys::PSA_ERROR_INSUFFICIENT_STORAGE,
Error::CommunicationFailure => psa_crypto_sys::PSA_ERROR_COMMUNICATION_FAILURE,
Error::StorageFailure => psa_crypto_sys::PSA_ERROR_STORAGE_FAILURE,
//Error::DataCorrupt => psa_crypto_sys::PSA_ERROR_DATA_CORRUPT,
//Error::DataInvalid => psa_crypto_sys::PSA_ERROR_DATA_INVALID,
Error::DataCorrupt => psa_crypto_sys::PSA_ERROR_DATA_CORRUPT,
Error::DataInvalid => psa_crypto_sys::PSA_ERROR_DATA_INVALID,
Error::HardwareFailure => psa_crypto_sys::PSA_ERROR_HARDWARE_FAILURE,
//Error::CorruptionDetected => psa_crypto_sys::PSA_ERROR_CORRUPTION_DETECTED,
Error::CorruptionDetected => psa_crypto_sys::PSA_ERROR_CORRUPTION_DETECTED,
Error::InsufficientEntropy => psa_crypto_sys::PSA_ERROR_INSUFFICIENT_ENTROPY,
Error::InvalidSignature => psa_crypto_sys::PSA_ERROR_INVALID_SIGNATURE,
Error::InvalidPadding => psa_crypto_sys::PSA_ERROR_INVALID_PADDING,
Error::InsufficientData => psa_crypto_sys::PSA_ERROR_INSUFFICIENT_DATA,
Error::InvalidHandle => psa_crypto_sys::PSA_ERROR_INVALID_HANDLE,
e => {
error!("No equivalent of {:?} to a psa_status_t.", e);
psa_crypto_sys::PSA_ERROR_GENERIC_ERROR
}
}
}
}
Expand Down
Loading