Make Session hold a reference to a Pkcs11 or an owned Pkcs11

[nwalfield]

Making Session take a reference to a Pkcs11 object means that the Session's lifetime is explicitly coupled to the Pkcs11 object. To allow users of the library to decouple these, extend Session to hold either a reference to a Pkcs11 or an Arc<Pkcs11>.

A user of the library chooses to use an Arc<Pkcs11> by using Session::open_owned_ro_session and Session::open_owned_rw_session instead of Session::open_ro_session and Session::open_rw_session. No other parallel APIs are needed.

This effective allows opting into the old behavior that was removed in this commit: 1994987 (part of #326).

I need these semantics in my project, as my PKCS11 abstraction is separate from Sessions and tying them together would add quite a bit of complexity with no obvious benefits as far as I can see.

I'm unsure of where to put the MaybeOwned data structure. I put it in the same module as the Pkcs11 data structure. We should discuss if it belongs there. Note: we can't use Cow, as Pkcs11 does not implement Clone.

[hug-dev]

Hello! Thanks for the PR!

@wiktor-k , do you remember why you added the change in 1994987? I thought in the beginning that it was indeed cleaner but if it is causing more issues than anticipated I am wondering if you had other reasons to add it :)

[wiktor-k]

@wiktor-k , do you remember why you added the change in 1994987? I thought in the beginning that it was indeed cleaner but if it is causing more issues than anticipated I am wondering if you had other reasons to add it :)

Hmm... it seems I removed the inner Arc as part of making the client manage the lifecycle of the Pkcs11 object.

Are the issues related to the fact that now one cannot hold a single Pkcs11 object and then open multiple sessions in different threads? If so, we may want to revert that commit.

Sorry for the trouble 👋

[hug-dev]

I see, thanks! I would say then that if the current way we handle life-cycle works reverting that commit then that could be a potentially easier solution than your changes @nwalfield to solve the same issue (if your use-case works then).

[nwalfield]

My use case works with the old interface (prior to 1994987). I'm fine with reverting that change and closing this PR. If there is a real use case for the object life-cycle management then I think my change is not very invasive, and a reasonable compromise to support both.

[wiktor-k]

By the way I wonder if the spec has any provisions on using a single Pkcs11 object/library from multiple threads for sessions. That is, I'm wondering if using Arc for Sessions is always safe. Let's summon our spec expert 🧙 🎆 @Jakuje 👋

[nwalfield]

The spec (3.2 draft 14) says:

the behavior of Cryptoki is undefined if multiple threads of an application attempt to access a common Cryptoki session simultaneously

It also refers to PKCS11-UG, which says:

Different threads of an application should never share sessions, unless they are extremely careful not to make function calls at the same time. This is true even if the Cryptoki library was initialized with locking enabled for thread-safety.

[wiktor-k]

Okay, that's good, thanks for the reference.

the behavior of Cryptoki is undefined if multiple threads of an application attempt to access a common Cryptoki session simultaneously

That's not an issue since Session is explicitly not Send and Sync.

... This is true even if the Cryptoki library was initialized with locking enabled for thread-safety.

So if the Cryptoki library is not initialized with locking enabled creating sessions on different threads may not be a good idea?

I'm considering the following scenario: Pkcs11 is used to create one session, then the Pkcs11 object is moved to a different thread to create another session there. Which shouldn't be possible if if wasn't initialized with locking enabled. And if that's not the case why are we using Arc instead of regular Rc? 🤔

Just throwing questions, see if there's any merit in there...

[nwalfield]

I don't think I completely understand your scenario. Here's a try, though: if the Pkcs11 object is initialized with locking enabled, then the Pkcs11 can be referenced from different threads and an Arc (as opposed to an Rc) is required. If the Pkcs11 object is not initialized with locking enabled, we can't change the type of Pkcs11 so we still have to use an Arc. Or, we need two Pkcs11 types. Does that help?

[wiktor-k]

That's not what I meant, maybe let me rephrase that in Rust (assuming cryptoki = "0.10.0"):

use std::thread;

fn main() -> testresult::TestResult {
    use cryptoki::context::{CInitializeArgs, Pkcs11};
    use cryptoki::mechanism::Mechanism;
    use cryptoki::object::Attribute;
    use cryptoki::session::UserType;
    use cryptoki::types::AuthPin;
    use std::env;

    // initialize a new Pkcs11 object using the module from the env variable
    let pkcs11 = Pkcs11::new(
        env::var("TEST_PKCS11_MODULE").unwrap_or_else(|_| "/usr/lib/libsofthsm2.so".to_string()),
    )?;

    pkcs11.initialize(CInitializeArgs::OsThreads)?;

    let slot = pkcs11.get_slots_with_token()?[0];

    // initialize a test token
    let so_pin = AuthPin::new("abcdef".into());
    pkcs11.init_token(slot, &so_pin, "Test Token")?;

    let user_pin = AuthPin::new("fedcba".into());

    let session = pkcs11.open_rw_session(slot)?;
    session.login(UserType::So, Some(&so_pin))?;
    session.init_pin(&user_pin)?;
    eprintln!("Thread ID: {:?}", thread::current().id());

    thread::spawn(move || {
        eprintln!("Thread ID: {:?}", thread::current().id());
        // login as a user, the token has to be already initialized
        let session = pkcs11.open_rw_session(slot).unwrap();
        session.login(UserType::User, Some(&user_pin)).unwrap();

        // template of the public key
        let pub_key_template = vec![
            Attribute::Token(true),
            Attribute::Private(false),
            Attribute::PublicExponent(vec![0x01, 0x00, 0x01]),
            Attribute::ModulusBits(1024.into()),
        ];

        let priv_key_template = vec![Attribute::Token(true)];

        // generate an RSA key according to passed templates
        let (public, private) = session
            .generate_key_pair(
                &Mechanism::RsaPkcsKeyPairGen,
                &pub_key_template,
                &priv_key_template,
            )
            .unwrap();
    })
    .join()
    .unwrap();
    drop(session);

    fn assert_sync<T: Sync>() {}
    assert_sync::<Pkcs11>();

    Ok(())
}

Now the Pkcs11 object is accessed from two threads:

Thread ID: ThreadId(1)
Thread ID: ThreadId(2)

My question was concerning the fact that Pkcs11 seems to be Sync and whether PKCS#11 spec allows that in general or not.

Btw, this code here has a funny side-effect, when ran against softhsm it prints:

called `Result::unwrap()` on an `Err` value: Pkcs11(UserAnotherAlreadyLoggedIn, Login)

but when the session.login(UserType::User, Some(&user_pin)).unwrap(); line is commented-out it prints:

called `Result::unwrap()` on an `Err` value: Pkcs11(UserNotLoggedIn, GenerateKeyPair)

A Schrödinger's session! 🐈‍⬛

I've also noticed that:

pub struct Session {
    ...
    // This is not used but to prevent Session to automatically implement Send and Sync
    _guard: PhantomData<*mut u32>,
}

but then below:

// Session does not implement Sync to prevent the same Session instance to be used from multiple
// threads.
unsafe impl Send for Session {}

So it's either the first comment is incorrect or moving sessions between threads is not a good idea 🤷‍♂️

Okay, I need to stop this spelunking session and get to actual work. Have a nice day and see you! 👋

[hug-dev]

I think since that seems to be fine for everyone, I would propose to revert the mentioned commit above. I would prefer to wait for more use-cases or bug reports before changing the interface with new types (that for me at least look a bit complicated 😅 )!

We can also definitely open a new bug ticket if we should remove the Send implementation of Session!