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

Solution for Session lifetimes #57

Closed
mike-boquard opened this issue Nov 2, 2021 · 9 comments · Fixed by #59
Closed

Solution for Session lifetimes #57

mike-boquard opened this issue Nov 2, 2021 · 9 comments · Fixed by #59
Assignees

Comments

@mike-boquard
Copy link
Collaborator

The Session struct contained an explicit lifetime requirement on an instance of the Pkcs11 struct. Overall, this makes sense; you do not want an instance of a Session to outlive the instance of Pkcs11. However, this explicit lifetime requirement presents some interesting problems during implementation.

Take for example, a certificate authority that utilizes a PKCS#11 compliant token that contains a private key used to sign requests coming in. This certificate authority is multi-threaded so it can handle multiple requests at once. Each thread is required to open a session on the token in order to access the underlying object and wield it in order to sign the request. Now in order to wield this object, that session needs to be logged in.

One way of handling this is to simply login each session when it's created. If the session is already logged in, the login call will fail with CKR_USER_ALREADY_LOGGED_IN. That's annoying, but it can be handled in code. However, some tokens can't be logged in simply by password, some require smart cards inserted into the token, some require biometerics, etc. The point is that sometimes, login is interactive. And once the final session closes, that token is now logged out and a user is required to log back in.

To get around this, many CA application store off a 'management' session. This session is opened shortly after the application is started and the PKCS#11 library is initialized. This session is logged in and lives for as long as the application runs. The threads simply need to open a new session, do what they need to do, close the session and end.

The issue now, is say this CA stores the instance of Pkcs11 in a struct. Where can the instance of the 'management' session live?

struct MyCA {
  p11: Pkcs11,
  mgmt_session: Session,
}

This won't work, due to the explicit lifetime requirement of Session. This stack overflow question goes into more detail.

One possible solution is to add functions to the crate that allow the user to create the session without having to hold onto it. Something like create_management_session(slot: Slot, read_write: bool, user: UserType, pin: Option<&str>) and close_management_session(slot: Slot).

@ionut-arm
Copy link
Member

Thanks! This sounds like a very interesting use-case and I think your solution makes sense. I have a few questions around it:

  • Is that pin enough to log in? Or by interactive did you mean that someone has to press a button somewhere to allow the login attempt?
  • Do we need to take any other precautions around the management session?
  • Would users need any sort of access to it, apart from creating/destroying?

@mike-boquard
Copy link
Collaborator Author

Is that pin enough to log in? Or by interactive did you mean that someone has to press a button somewhere to allow the login attempt?

So that's why pin is an Option<str>. It depends on the token you're using, but hypothetically you could have one that has a separate smart card/PIN reader that logs the token in. Then the login call just provides a zero-length pin (pin = None) and that logs the session in.

Do we need to take any other precautions around the management session?

So you would want to key it to a slot and react to calls like C_CloseAllSessions. Maybe even a call to check its state from time to time, but if a user decides to call logout on one of their sessions, there isn't anything we can do about it, that would, in turn, log out that management session. I'm sure I'm forgetting something.

Would users need any sort of access to it, apart from creating/destroying?

I'd argue no. If they want to do any session stuff after that session is created and logged in, they can simply create their own session and do what they need to do with it (e.g. create a session object).

@mike-boquard
Copy link
Collaborator Author

I'm thinking this would be something Parsec would want to build into their Pkcs11 'engine'

@ionut-arm
Copy link
Member

I'm thinking this would be something Parsec would want to build into their Pkcs11 'engine'

Yes, indeed, I was actually thinking about it while reading your initial comment

@ionut-arm
Copy link
Member

but if a user decides to call logout on one of their sessions, there isn't anything we can do about it, that would, in turn, log out that management session.

Is that more appropriate than returning an error? I'm not particularly keen on passing boolean flags around, but we could have a separate method that logs out even when a management session exists (something like a forced logout), one that doesn't. But regardless of which way we go on this, what would happen to the management session if the user does log out? Do we close it?

@mike-boquard
Copy link
Collaborator Author

By spec, if one session calls logout, all sessions opened on the same token on the same process also log out.

This is something that is well defined in the spec and a P11 developer should know that. It's up to them to not do something "dumb".

mike-boquard added a commit to mike-boquard/rust-cryptoki that referenced this issue Nov 3, 2021
- Added `open_management_session`
- Added `close_management_session`
- Added test to test newly added functionality

Signed-off-by: Michael J Boquard <[email protected]>
mike-boquard added a commit to mike-boquard/rust-cryptoki that referenced this issue Nov 3, 2021
- Added `open_management_session`
- Added `close_management_session`
- Added test to test newly added functionality

Signed-off-by: Michael J Boquard <[email protected]>
mike-boquard added a commit to mike-boquard/rust-cryptoki that referenced this issue Nov 3, 2021
- Added `open_management_session`
- Added `close_management_session`
- Added test to test newly added functionality

Signed-off-by: Michael J Boquard <[email protected]>
@mike-boquard mike-boquard self-assigned this Nov 3, 2021
@vkkoskie
Copy link
Contributor

vkkoskie commented Nov 4, 2021

Poking around other issues being nosy. Why not just clone the context into the session? Or Rc or Arc or Arc+Mutex, whichever is appropriate for the threading assumptions. The context only holds the function pointers, which never mutate after initialization, right? The session could receive a clone of one of the above options instead of a reference.

@mike-boquard
Copy link
Collaborator Author

That's a really good point, I didn't think of that. I think a simple Arc would suffice as the Pkcs11 struct does not change, so no reason to lock it behind a mutex.

Well I had to close out my PR anyways to refactor it, so why not start from scratch 😉

@mike-boquard
Copy link
Collaborator Author

Thanks for the idea @vkkoskie!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants