Don't raise an error at close when it's expected

[EliseChouleur]

If session's close raises a SessionHandleInvalid error, that just means the session is already closed by other means.
So don't raise an error but just a warn message in this case.

[hug-dev]

Thanks for the PR!

means the session is already closed by other means

I am wondering if it's possible for the session to be closed by other means if the API has been used only using safe blocks? If not, it could be good to still have this message to show that something unexpected happened?

But maybe I am missing a flow where this could happen!

[wiktor-k]

If not, it could be good to still have this message to show that something unexpected happened?

Yep, it sounds as if there would be a double close in this scenario, which would be good to have in logs.

I wonder about the use case of other means of closing the session... 👀

[hug-dev]

I implemented something similar than what you propose here as part of #326 but checking explicitly is the session is already closed

[EliseChouleur]

Hi !

This happens to me when I remove the physical token before dropping the session. In this case, the session is automatically closed by the library in charge of this token.
To be correct, I think the session isn't really closed but the handle became invalid, that should be why I don't get the SessionClosed error as you're handling in the PR @hug-dev

[hug-dev]

Would my PR suffice then? That would allow you to explicitly call session.close() and filter on the error on your side?

[EliseChouleur]

I wanted to test the behaviour with your PR but the new implementation with the lifetime parameter on the session is a breaking change not compatible with my current implementation with the Pkcs11 context in a mutex and the session in a thread_local ref cell 😅

[hug-dev]

oh no 🙈 Could you maybe produce a minimal example of your usage of Session and Pkcs11 so that we could see how to adapt our code to make it work? Could also add examples to make sure we do not regress

[EliseChouleur]

I've added an example (helped by Claude ^^ so very verbose sorry)
This same example inside your branche gives me those errors:

error[E0106]: missing lifetime specifier
  --> cryptoki/examples/thread_local_session.rs:57:43
   |
57 |     static PKCS11_SESSION: RefCell<Option<Session>> = RefCell::new(None);
   |                                           ^^^^^^^ expected named lifetime parameter
   |
   = help: this function's return type contains a borrowed value, but there is no value for it to be borrowed from
help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`, or if you will only have owned values
   |
57 |     static PKCS11_SESSION: RefCell<Option<Session<'static>>> = RefCell::new(None);
   |                                                  +++++++++

error[E0597]: `ctx_lock` does not live long enough
   --> cryptoki/examples/thread_local_session.rs:132:23
    |
131 |             let ctx_lock = PKCS11_CTX.lock().unwrap();
    |                 -------- binding `ctx_lock` declared here
132 |             let ctx = ctx_lock
    |                       ^^^^^^^^ borrowed value does not live long enough
...
159 |         } else {
    |         - `ctx_lock` dropped here while still borrowed
...
169 |     })
    |     - borrow might be used here, when `session_opt` is dropped and runs the destructor for type `RefMut<'_, Option<Session<'_>>>`
    |
    = note: values in a scope are dropped in the opposite order they are defined

Some errors have detailed explanations: E0106, E0597.
For more information about an error, try `rustc --explain E0106`.
error: could not compile `cryptoki` (example "thread_local_session") due to 2 previous errors

[EliseChouleur]

Thanks to the example, a way to resolve the issue I had with your branch is to use a OnceLock for PKCS11 context, I'll try that on my work and get back to you with the outcome :)

[wiktor-k]

I think this looks good 👌 thanks!

[EliseChouleur]

I've tested my code with your PR @hug-dev, and I can't manage to close the session before the drop:
Logs:

Closing existing thread-local PKCS#11 session
[cryptoki::session::session_management /Users/elisechouleur/dev/rust-cryptoki-hug-dev/cryptoki/src/session/session_management.rs:00023] Failed to close session: Function::CloseSession: PKCS11 error: The specified session handle was invalid at the time that the function was invoked.  Note that this can happen if the session’s token is removed before the function invocation, since removing a token closes all sessions with it.
Error closing PKCS#11 session: Function::CloseSession: PKCS11 error: The specified session handle was invalid at the time that the function was invoked.  Note that this can happen if the session’s token is removed before the function invocation, since removing a token closes all sessions with it.

Code:

            if let Some(session) = session_mut.take() {
                debug!("Closing existing thread-local PKCS#11 session");
                match session.close() {
                    Ok(_) => {
                        debug!("PKCS#11 session closed successfully");
                    }
                    Err(err) => {
                        debug!("Error closing PKCS#11 session: {err}");
                    }
                }
            }

[hug-dev]

We reverted a commit on main recently that might affect you! How is everything working now on your side?

[hug-dev]

I think the recent changes should help a bit!

[wiktor-k]

any though on this ? To have another opinion 😁

I think cryptoki should include the automatically Dropping wrapper alongside the non Dropping variant and let the user choose. The Dropping variant is not that straightforward since it needs to call finalize only if finalize has not been previously called.

This would make easy things easy (I'd usually use the Dropping variant) and hard things possible (the non Dropping for manual lifecycle management).

Does that make things clearer or more muddled? 😅

[hug-dev]

That's fine for me! Then I think we all agree that we can implement the 2 option you proposed @EliseChouleur :)

[EliseChouleur]

Ok, so if I understand this well, I'll create a a Cargo feature (e.g. no-close-on-drop) to remove Drop impl when this feature is activated. And I also add the close security to not call close twice ?

[hug-dev]

Maybe instead of a Cargo feature we can modify the API to be able to choose if we want the Session to close on Drop or not? That way both are available without changing build flags :)

To not add breaking changes we could add new constructors, open_ro(w)_session_no_drop, that would not close on Drop and modify the default constructors so that they do?

And I also add the close security to not call close twice ?

Yes exactly!

[wiktor-k]

Okay, this is definitely a high-quality PR with quite some goodies in here - thanks for your time 🙇

There's a little bit of stuff to unpack there so I left a couple of nits, maybe @hug-dev will see and tell what they think about them.

I like that this thing is tested, it's quite an intricate behavior and your added entire mock and logger lib to test everything thoroughly 👏

Funnily enough I've been playing with writing a Rust-PKCS#11 token too (https://gitlab.archlinux.org/wiktor/pkcs11-proxy/-/blob/main/src/lib.rs?ref_type=heads#L182, heavily incomplete) and I wish I had something simpler that we could use to replace the C code (which isn't bad, it's very readable).

One small concern I have with the test logger is that it tests if the log was captured which may be brittle if the message gets edited. Sadly I don't see a better way since the session is managed by the PKCS#11 object 🤔 maybe more flags in the mock pkcs#11?

Thanks for your contribution, I hope we find a way to merge it soon 🙏

[hug-dev]

Thanks a lot for the changes! Good to have your example in there as well.

[hug-dev]

Really appreciate the effort you put into adding a whole mock PKCS11 library to be able to test this but I am wondering if it might not be a bit "too much" for the scope of this change. I am thinking about the tradeoff between testing this and adding a lot of code that we will have to maintain in the future 😅

Would it be possible somehow to get drop to fail with something else than "already close" without the need of the entire mock library?

If not maybe in this case we can say that it's been "tested on your machine" 😅

[EliseChouleur]

In addition to be able to test this case it was also to suggest a mock version of tests to be able to test edge cases later on, but I'm not sure there's really some other edge cases to test ^^

[hug-dev]

I see, then I think we could maybe do a simple test triggering a fault with a new_from_raw method as described below and maybe revisit this mock-PKCS11 idea once we have another use case?

[EliseChouleur]

Yup, I've done this in 38aaf5a but couldn't find a way to test without checking the logs which isn't a nice test assertion, so I think you're right, it's better to remove.

[wiktor-k]

I think it looks small and good 👌

[wiktor-k]

@hug-dev did you mean that the set should be positioned after the call to C_CloseSession?

Or maybe this one should be done in drop() when the close_inner() function returns Ok(())?

🤔