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

attribute extension defining Unhanled attribute type and EC Kdf improvements #241

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 14 additions & 6 deletions cryptoki/src/mechanism/elliptic_curve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,17 @@ use std::ptr;
#[repr(C)]
pub struct Ecdh1DeriveParams<'a> {
/// Key derivation function
kdf: CK_EC_KDF_TYPE,
pub kdf: CK_EC_KDF_TYPE,
/// Length of the optional shared data used by some of the key
/// derivation functions
shared_data_len: Ulong,
pub shared_data_len: Ulong,
/// Address of the optional data or `std::ptr::null()` of there is
/// no shared data
shared_data: *const u8,
pub shared_data: *const u8,
/// Length of the other party's public key
public_data_len: Ulong,
pub public_data_len: Ulong,
/// Pointer to the other party public key
public_data: *const u8,
pub public_data: *const u8,
/// Marker type to ensure we don't outlive shared and public data
_marker: PhantomData<&'a [u8]>,
}
Expand Down Expand Up @@ -82,7 +82,15 @@ pub struct EcKdf<'a> {
shared_data: Option<&'a [u8]>,
}

impl EcKdf<'_> {
impl<'a> EcKdf<'a> {
/// Define KDF_TYPE and shared_data
pub fn new(kdf_type: CK_EC_KDF_TYPE, shared_data: Option<&'a [u8]>) -> Self {
Self {
kdf_type,
shared_data,
}
}

/// The null transformation. The derived key value is produced by
/// taking bytes from the left of the agreed value. The new key
/// size is limited to the size of the agreed value.
Expand Down
10 changes: 5 additions & 5 deletions cryptoki/src/mechanism/rsa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,15 +137,15 @@ pub struct PkcsPssParams {
pub struct PkcsOaepParams<'a> {
/// mechanism ID of the message digest algorithm used to calculate the digest of the encoding
/// parameter
hash_alg: MechanismType,
pub hash_alg: MechanismType,
/// mask generation function to use on the encoded block
mgf: PkcsMgfType,
pub mgf: PkcsMgfType,
/// source of the encoding parameter
source: CK_RSA_PKCS_OAEP_SOURCE_TYPE,
pub source: CK_RSA_PKCS_OAEP_SOURCE_TYPE,
/// data used as the input for the encoding parameter source
source_data: *const c_void,
pub source_data: *const c_void,
/// length of the encoding parameter source input
source_data_len: Ulong,
pub source_data_len: Ulong,
/// marker type to ensure we don't outlive the source_data
_marker: PhantomData<&'a [u8]>,
}
Expand Down
22 changes: 18 additions & 4 deletions cryptoki/src/object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::error::{Error, Result};
use crate::mechanism::MechanismType;
use crate::types::{Date, Ulong};
use cryptoki_sys::*;
use log::error;
use log::{error, warn};
use std::convert::TryFrom;
use std::convert::TryInto;
use std::ffi::c_void;
Expand Down Expand Up @@ -136,6 +136,8 @@ pub enum AttributeType {
Wrap,
/// Indicates that the key can only be wrapped with a wrapping key that has the Trusted attribute
WrapWithTrusted,
/// Wraps undefined and custom attribute types.
Unhandled(CK_ATTRIBUTE_TYPE),
Copy link
Member

Choose a reason for hiding this comment

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

Hey @ilkerBaltaci , thanks for your PR 🙏!

I understand the idea behind the Unhandled types, but I think we strive to only expose in the cryptoki crate idiomatic structures and functions which are safe to use and abstracted away from the raw PKCS11 types. That's summarized in our README:

All the PKCS11 items might not be implemented but everything that is implemented is safe.

I will let other maintainers give their opinions but I think that if you have new types/algos you want to add it's best to do so in the high-level "way" where you add corresponding cryptoki structures/functions for those new types

}

impl AttributeType {
Expand Down Expand Up @@ -328,6 +330,7 @@ impl From<AttributeType> for CK_ATTRIBUTE_TYPE {
AttributeType::VerifyRecover => CKA_VERIFY_RECOVER,
AttributeType::Wrap => CKA_WRAP,
AttributeType::WrapWithTrusted => CKA_WRAP_WITH_TRUSTED,
AttributeType::Unhandled(attr_type) => attr_type,
}
}
}
Expand Down Expand Up @@ -397,8 +400,10 @@ impl TryFrom<CK_ATTRIBUTE_TYPE> for AttributeType {
CKA_WRAP => Ok(AttributeType::Wrap),
CKA_WRAP_WITH_TRUSTED => Ok(AttributeType::WrapWithTrusted),
attr_type => {
error!("Attribute type {} not supported.", attr_type);
Err(Error::NotSupported)
// error!("Attribute type {} not supported.", attr_type);
// Err(Error::NotSupported)
warn!("Unhandled attribute type {} detected.", attr_type);
Ok(AttributeType::Unhandled(attr_type))
}
}
}
Expand Down Expand Up @@ -526,6 +531,8 @@ pub enum Attribute {
Wrap(bool),
/// Indicates that the key can only be wrapped with a wrapping key that has the Trusted attribute
WrapWithTrusted(bool),
/// Not defined attributes enter this option.
Unhandled(CK_ATTRIBUTE_TYPE, Vec<u8>),
}

impl Attribute {
Expand Down Expand Up @@ -591,6 +598,7 @@ impl Attribute {
Attribute::VerifyRecover(_) => AttributeType::VerifyRecover,
Attribute::Wrap(_) => AttributeType::Wrap,
Attribute::WrapWithTrusted(_) => AttributeType::WrapWithTrusted,
Attribute::Unhandled(attribute_type, _) => AttributeType::Unhandled(*attribute_type),
}
}

Expand Down Expand Up @@ -658,6 +666,7 @@ impl Attribute {
Attribute::AllowedMechanisms(mechanisms) => {
size_of::<CK_MECHANISM_TYPE>() * mechanisms.len()
}
Attribute::Unhandled(_, bytes) => bytes.len(),
}
}

Expand Down Expand Up @@ -730,7 +739,8 @@ impl Attribute {
| Attribute::Subject(bytes)
| Attribute::Url(bytes)
| Attribute::Value(bytes)
| Attribute::Id(bytes) => bytes.as_ptr() as *mut c_void,
| Attribute::Id(bytes)
| Attribute::Unhandled(_, bytes) => bytes.as_ptr() as *mut c_void,
// Unique types
Attribute::CertificateType(certificate_type) => {
certificate_type as *const _ as *mut c_void
Expand Down Expand Up @@ -930,6 +940,7 @@ impl TryFrom<CK_ATTRIBUTE> for Attribute {
}
}
}
AttributeType::Unhandled(_) => todo!(),
}
}
}
Expand Down Expand Up @@ -1006,6 +1017,8 @@ impl ObjectClass {
pub const MECHANISM: ObjectClass = ObjectClass { val: CKO_MECHANISM };
/// An OTP key object
pub const OTP_KEY: ObjectClass = ObjectClass { val: CKO_OTP_KEY };
/// A profile object
pub const PROFILE: ObjectClass = ObjectClass { val: CKO_PROFILE };
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 perfect to add though!


pub(crate) fn stringify(class: CK_OBJECT_CLASS) -> String {
match class {
Expand All @@ -1018,6 +1031,7 @@ impl ObjectClass {
CKO_DOMAIN_PARAMETERS => String::from(stringify!(CKO_DOMAIN_PARAMETERS)),
CKO_MECHANISM => String::from(stringify!(CKO_MECHANISM)),
CKO_OTP_KEY => String::from(stringify!(CKO_OTP_KEY)),
CKO_PROFILE => String::from(stringify!(CKO_PROFILE)),
_ => format!("unknown ({class:08x})"),
}
}
Expand Down