-
Notifications
You must be signed in to change notification settings - Fork 87
Add pReserved and pkcs11 flags #329
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,32 +2,169 @@ | |
| // SPDX-License-Identifier: Apache-2.0 | ||
| //! Locking related type | ||
| use cryptoki_sys::{CKF_OS_LOCKING_OK, CK_FLAGS}; | ||
| use cryptoki_sys::{CKF_LIBRARY_CANT_CREATE_OS_THREADS, CKF_OS_LOCKING_OK, CK_FLAGS, CK_RV}; | ||
|
|
||
| use std::ptr; | ||
| use std::{ | ||
| os::raw::c_void, | ||
| ptr::{self, NonNull}, | ||
| }; | ||
|
|
||
| /// Argument for the initialize function | ||
| /// Function pointer that creates a mutex | ||
| pub type CreateMutexFn = unsafe extern "C" fn(*mut *mut ::std::os::raw::c_void) -> CK_RV; | ||
|
|
||
| /// Function pointer that destroys a mutex | ||
| pub type DestroyMutexFn = unsafe extern "C" fn(*mut ::std::os::raw::c_void) -> CK_RV; | ||
|
|
||
| /// Function pointer that locks a mutex | ||
| pub type LockMutexFn = unsafe extern "C" fn(*mut ::std::os::raw::c_void) -> CK_RV; | ||
|
|
||
| /// Function pointer that unlocks a mutex | ||
| pub type UnlockMutexFn = unsafe extern "C" fn(*mut ::std::os::raw::c_void) -> CK_RV; | ||
|
|
||
| /// Provides function pointers for mutex-handling to ensure safe multi-threaded access. | ||
| #[derive(Copy, Clone, Debug)] | ||
| pub enum CInitializeArgs { | ||
| /// The library can use the native OS library for locking | ||
| pub struct CustomMutexHandling { | ||
| create_mutex: CreateMutexFn, | ||
| destroy_mutex: DestroyMutexFn, | ||
| lock_mutex: LockMutexFn, | ||
| unlock_mutex: UnlockMutexFn, | ||
| } | ||
|
|
||
| impl CustomMutexHandling { | ||
| /// Create a new `CustomMutexHandling` with the given function pointers | ||
| /// to handle library's thread safety. | ||
| /// | ||
| /// # Safety | ||
| /// Considered unsafe due to user's ability to pass any function pointer. | ||
| pub const unsafe fn new( | ||
| create_mutex: CreateMutexFn, | ||
| destroy_mutex: DestroyMutexFn, | ||
| lock_mutex: LockMutexFn, | ||
| unlock_mutex: UnlockMutexFn, | ||
| ) -> Self { | ||
| Self { | ||
| create_mutex: create_mutex, | ||
| destroy_mutex: destroy_mutex, | ||
| lock_mutex: lock_mutex, | ||
| unlock_mutex: unlock_mutex, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Flags to set for the initialize function | ||
| #[derive(Copy, Clone, Debug)] | ||
| pub enum CInitializeFlags { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think to start with a new abstraction of this type it would be nice to go with a "dumber" 1-1 mapping with the C type. I think it's nicer to start simple and straightforward and then improve over time depending on usage. I mean the following:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with the 1st point, but the purpose of the PR was to add the
pub enum Reserved {
None,
IBM(IBMInitParams),
Utimaco(UtimacoInitParams),
}
let p_reserved = Box::into_raw(Box::new(Reserved::IBM(...)));Regarding points 2 and 3 I see the following problems:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes 🤦 ! I forgot the whole purpose of the related issue 😬
I think the PKCS11 API does not prevent it so I guess we should not limit either... But agree that some combinations of those two are pretty weird.
Let's see what we decide with the other comment to see how we proceed here
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They mention that if |
||
| /// The library won’t be accessed from multiple threads simultaneously | ||
| None, | ||
| /// The library may not create its own threads | ||
| NoOsThreads, | ||
| /// The library can use the native OS library for locking or the custom | ||
| OsThreads, | ||
| // TODO: add variants for custom mutexes here and no multithreading, safety implications for | ||
| // that. | ||
| /// The library needs to use the supplied function pointers | ||
| /// for mutex-handling to ensure safe multi-threaded access. | ||
| CustomMutexHandling(CustomMutexHandling), | ||
| /// The library needs to use either the native operating system primitives | ||
| /// or the supplied function pointers for mutex-handling to ensure safe | ||
| /// multi-threaded access | ||
| OsThreadsOrCustomMutexHandling(CustomMutexHandling), | ||
| } | ||
|
|
||
| #[derive(Copy, Clone, Debug)] | ||
| /// Argument for the initialize function | ||
| pub struct CInitializeArgs { | ||
| flags: CInitializeFlags, | ||
| p_reserved: Option<NonNull<c_void>>, | ||
| } | ||
|
|
||
| impl CInitializeArgs { | ||
| /// Create a new `CInitializeArgs` with the given flags | ||
| /// | ||
| /// # Examples | ||
| /// ``` | ||
| /// use cryptoki::context::{CInitializeArgs, CInitializeFlags}; | ||
| /// | ||
| /// let args = CInitializeArgs::new(CInitializeFlags::OsThreads); | ||
| /// ``` | ||
| pub const fn new(flags: CInitializeFlags) -> Self { | ||
| Self { | ||
| flags, | ||
| p_reserved: None, | ||
| } | ||
| } | ||
|
|
||
| /// Create a new `CInitializeArgs` with the given flags and reserved pointer. | ||
| /// | ||
| /// # Safety | ||
| /// Considered unsafe due to the user's ability to pass any pointer. | ||
| /// | ||
| /// The user is responsible for managing the memory behind the pointer. | ||
| pub const unsafe fn new_with_reserved( | ||
| flags: CInitializeFlags, | ||
| p_reserved: NonNull<c_void>, | ||
| ) -> Self { | ||
| Self { | ||
| flags, | ||
| p_reserved: Some(p_reserved), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<CInitializeArgs> for cryptoki_sys::CK_C_INITIALIZE_ARGS { | ||
| fn from(c_initialize_args: CInitializeArgs) -> Self { | ||
| let mut flags = CK_FLAGS::default(); | ||
| match c_initialize_args { | ||
| CInitializeArgs::OsThreads => { | ||
| let p_reserved = c_initialize_args | ||
| .p_reserved | ||
| .map(|non_null| non_null.as_ptr()) | ||
| .unwrap_or_else(ptr::null_mut); | ||
|
|
||
| match c_initialize_args.flags { | ||
| CInitializeFlags::None => Self { | ||
| CreateMutex: None, | ||
| DestroyMutex: None, | ||
| LockMutex: None, | ||
| UnlockMutex: None, | ||
| flags, | ||
| pReserved: p_reserved, | ||
| }, | ||
| CInitializeFlags::NoOsThreads => { | ||
| flags |= CKF_LIBRARY_CANT_CREATE_OS_THREADS; | ||
| Self { | ||
| flags, | ||
| CreateMutex: None, | ||
| DestroyMutex: None, | ||
| LockMutex: None, | ||
| UnlockMutex: None, | ||
| pReserved: p_reserved, | ||
| } | ||
| } | ||
| CInitializeFlags::OsThreads => { | ||
| flags |= CKF_OS_LOCKING_OK; | ||
| Self { | ||
| flags, | ||
| CreateMutex: None, | ||
| DestroyMutex: None, | ||
| LockMutex: None, | ||
| UnlockMutex: None, | ||
| pReserved: ptr::null_mut(), | ||
| pReserved: p_reserved, | ||
| } | ||
| } | ||
| CInitializeFlags::CustomMutexHandling(custom_mutex_handling) => Self { | ||
| flags, | ||
| CreateMutex: Some(custom_mutex_handling.create_mutex), | ||
| DestroyMutex: Some(custom_mutex_handling.destroy_mutex), | ||
| LockMutex: Some(custom_mutex_handling.lock_mutex), | ||
| UnlockMutex: Some(custom_mutex_handling.unlock_mutex), | ||
| pReserved: p_reserved, | ||
| }, | ||
| CInitializeFlags::OsThreadsOrCustomMutexHandling(custom_mutex_handling) => { | ||
| flags |= CKF_OS_LOCKING_OK; | ||
| Self { | ||
| flags, | ||
| CreateMutex: Some(custom_mutex_handling.create_mutex), | ||
| DestroyMutex: Some(custom_mutex_handling.destroy_mutex), | ||
| LockMutex: Some(custom_mutex_handling.lock_mutex), | ||
| UnlockMutex: Some(custom_mutex_handling.unlock_mutex), | ||
| pReserved: p_reserved, | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to also create abstraction types for those functions, which would be safe abstractions over the
CK_LOCKMUTEX,CK_UNLOCKMUTEX,CK_DESTROYMUTEXandCK_CREATEMUTEXtypes. They would take in and return abstracted types and be converted to the C function in our code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I agree with that. the thing is I do not know how they would look like (I've never had to pass those functions when i initialize cryptoki)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are defined here but I could not come up with a nice way to do this is pure Rust.
I was thinking of defining a
trait Mutexwith all those methods but not sure how one could define trait methods as types 🤔I am wondering if to not make it easier for now we could ignore those functions? As in always set them to
Noneand remove it as parameters. Until someone needs them and then we can think about how to do it?Unless you come up with a good way to do this.