Consider opening visibility on ObjectHandle and SessionHandle

[jack-shannon]

I am working on handling a vendor extension that adds additional functions to the standard and so far all of the vendor specific accommodations work well. The one issue I've run into is with handles - accessing the underlying SessionHandle is pub(crate) as is instantiating an ObjectHandle or accessing the underlying handle, all of which I need to be able to do to add the extension cleanly.

Would changing these to pub be an unreasonable request/contribution?

[hug-dev]

Hello 👋 !

I am working on handling a vendor extension that adds additional functions to the standard and so far all of the vendor specific accommodations work well.

Thanks for the feedback, good to know!

Would changing these to pub be an unreasonable request/contribution?

The reason those are private is for safety: we restrict what is allowed to with the bare C API but everything that is exposed should be safe. I am wondering that if we allow to create/access the underlying SessionHandle and ObjectHandle then we will add a way for a user to "exit" the safe (and maybe error-free) usage of the crate?
We started though with the vendor extensions but there did not seem to be any other way there.

Can I ask why do you need those access? Is there any way we could add another "safe" which would allow you to add the extension cleanly while still maintaining the API safe for everyone?

[jack-shannon]

👋 !

That is definitely a reasonable goal, and I'm sure it would open up some ways to shoot yourself in the foot.

My goal is to handle vendor added functions, but continue to use the library's safe types and session handling everywhere outside of the extension.

To illustrate what I'm looking to accomplish:

pub trait CryptokiSessionExt {
    fn vendor_function(
        &self,
        mechanism: &Mechanism,
        input_key: ObjectHandle,
        pub_key_template: &[Attribute],
        priv_key_template: &[Attribute],
    ) -> Result<(ObjectHandle, ObjectHandle)>;
}

impl CryptokiSessionExt for Session {
    fn vendor_function(
        &self,
        mechanism: &Mechanism,
        input_key: ObjectHandle,
        pub_key_template: &[Attribute],
        priv_key_template: &[Attribute],
    ) -> Result<(ObjectHandle, ObjectHandle)> {
        let mut mechanism: CK_MECHANISM = mechanism.into();
        let mut pub_key_template: Vec<CK_ATTRIBUTE> =
            pub_key_template.iter().map(|attr| attr.into()).collect();
        let mut priv_key_template: Vec<CK_ATTRIBUTE> =
            priv_key_template.iter().map(|attr| attr.into()).collect();
        let mut pub_handle = 0;
        let mut priv_handle = 0;
        unsafe {
            // vendor added function
            Rv::from((vendor_symbols.C_VendorFunction)(
                self.handle(),  // need session handle
                &mut mechanism as CK_MECHANISM_PTR,
                input_key.handle(),  // need object handle for input
                pub_key_template.as_mut_ptr(),
                pub_key_template.len().try_into()?,
                priv_key_template.as_mut_ptr(),
                priv_key_template.len().try_into()?,
                &mut pub_handle,
                &mut priv_handle,
            ))
            .into_result(Function::GenerateKeyPair)?;
        }

        Ok((
            ObjectHandle::new(pub_handle),  // need to create safe ObjectHandle
            ObjectHandle::new(priv_handle),
        ))
    }
}

I'll have a think through if I can see this being accomplished through a safer utility, would be very happy with an alternative!

[jack-shannon]

A small thing could be to put pub accessors behind a feature that would force users to opt in/acknowledge they're doing something potentially less safe.

[jack-shannon]

Hi, I haven't had any further ideas on how to allow this kind of extension without changing the visibility, any further thoughts on hiding it behind a feature flag or some other kind of utility that might allow for it?

[placintaalexandru]

Hello,

I have the same use-case as @jack-shannon where Thales Luna's PKCS11 contains additional functions in their PKCS#11 library (e.g CA_SIMExtract, CA_SIMInsert, and many others as it can be seen here)

All the above mentioned functions require the raw CK_SESSION_HANDLE which cannot be accessed from the cryptoki::session::Session, a raw CK_OBJECT_HANDLE and they return the raw CK_OBJECT_HANDLE that cannot be used to build cryptoki::objects::ObjectHandle since the function cryptoki::objects::ObjectHandle::new is pub(crate).

Thank you!

[hug-dev]

Thanks for the explanation, both of you! I understand now why this would be needed. I thought also about the following options ranked in terms of benefit for the crate and you:

1. since when you will be using the raw values of a Session or creating an ObjectHandle that will be in a context where you are calling the C API with an unsafe block, we could also add those new functions as unsafe? Then it's the responsibility of the caller to make sure they are used in the right way. It does not also force a new unsafe block since one is needed anyway.
2. if the extensions you are writing are for PKCS11 modules which are public and well-known, could we potentially add those new APIs directly in the crate? For example we have seen namy people using Thales Luna's PKCS11.
3. the idea of having a feature for "extensions", off by default, could be a good enough choice here
4. worst for all of us: if you are adding little code, it could also make sense for you to fork the crate and add the extensions there.