Skip to content

Added Option<Slot> to PKCS 11 Provider constructor #402

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

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

Sven-bg
Copy link
Contributor

@Sven-bg Sven-bg commented Apr 23, 2021

Fix the issue #375

@hug-dev hug-dev added the enhancement New feature or request label Apr 23, 2021
@hug-dev hug-dev linked an issue Apr 23, 2021 that may be closed by this pull request
Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

Hi! 👋🏻

Thanks for the patch, the code looks good, but the "optionality" should be propagated to service spin-up: the slot number should be optional in the service configuration file as well, and have it passed down like that from the ServiceBuilder.

I'd also recommend "testing" it, either by removing the slot number from the config file we usually use for end-to-end tests, or by creating a new configuration test (the latter is probably better).

@Sven-bg
Copy link
Contributor Author

Sven-bg commented Apr 23, 2021

Hi @ionut-arm if you prefer I can cancel the PR, add the requested changes and make a new one.
About the testing, I disccused it with @hug-dev and we agreed on applying the patch and later on add some new tests to cover this specific feature, but if you prefer, I can add the tests too.
Thank you for the hints.
Adriano

@ionut-arm
Copy link
Member

No, keep the PR open and build on top of it/adjust the commits. And leaving the testing for another PR sounds alright!

Comment on lines 86 to 90
if !slots.is_empty() {
slots[0]
Copy link
Member

@ionut-arm ionut-arm Apr 23, 2021

Choose a reason for hiding this comment

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

Actually, I'm wondering - should we do it this way, grabbing the "first" token, or should we fail if slots.len() != 1? My worry is whether the result is deterministic - what if there are two slots and you get them arranged at random? Have to admit, I've not looked at the spec too closely.

Copy link
Member

Choose a reason for hiding this comment

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

(or, what if a new slot/token is added at some later point and that one becomes the first one? your service will spin up, but you'll essentially lose a lot of keys)

Copy link
Member

@hug-dev hug-dev Apr 23, 2021

Choose a reason for hiding this comment

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

Yes that's a very good point and I think a good suggestion. I checked the PKCS 11 docs and nothing says that the order is deterministic but also because of the other reason you indicated I think it's better to check that there is only one slot to use.

So replacing the above with:

-if !slots.is_empty() {
+if slots.len() == 1 {

We should make it clear that this behaviour is used in the config file comments.

@Sven-bg Sven-bg requested a review from a team as a code owner July 20, 2021 11:00
Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

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

Thanks, that will be useful! A few comments added.
You might need to cargo fmt before sending it again, when parallaxsecond/rust-cryptoki#35 is merged and imported here!

Next step would be to write a test, but that's totally something we can handle ourselves!

@hug-dev hug-dev merged commit f7b5620 into parallaxsecond:main Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the slot_number field optional
3 participants