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

Use session close function from oxide fork of yubihsm.rs. #193

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

flihp
Copy link
Collaborator

@flihp flihp commented Apr 2, 2024

The Client type previously had a Drop implementation that closed the session if the Client had an open one. This seems to have caused problems in other downstream projects and was subsequently removed: iqlusioninc/tmkms#37
iqlusioninc/yubihsm.rs#265

The replacement was to provide a session() function that returns an Arc / MutexGuard wrapped reference to the optional session. This isn't useful for us here because we don't and AFAIK can't take ownership of the session which we need because the Sesison::close function consumes the session (it can't be reopened). Our solution requires an upstream change to the Client type adding a close_session function that just closes the session if one is open.

This resolves #190

@flihp flihp requested a review from plotnick April 2, 2024 21:20
@flihp flihp force-pushed the session-close branch 2 times, most recently from cca9d8d to 648c485 Compare April 2, 2024 21:30
The `Client` type previously had a `Drop` implementation that closed the
session if the Client had an open one. This seems to have caused
problems in other downstream projects and was subsequently removed:
iqlusioninc/tmkms#37
iqlusioninc/yubihsm.rs#265

The replacement was to provide a `session()` function that returns an
Arc / MutexGuard wrapped reference to the optional session. This isn't
useful for us here because we don't and AFAIK can't take ownership of
the session which we need because the Sesison::close function consumes
the session (it can't be reopened). Our solution requires an upstream
change to the `Client` type adding a `close_session` function that just
closes the session if one is open.
Copy link

@plotnick plotnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, this is the right shape of solution to the upstream issue. May need to be tweaked later depending on what upstream decides, but should work for us for now.

@flihp flihp merged commit eeab7fa into oxidecomputer:main Apr 2, 2024
5 checks passed
@flihp flihp deleted the session-close branch April 2, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I'm outta sessions
2 participants