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.

Sign up for GitHub

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

Jump to bottom

Recognise a PKCS11 hardware token with its serial number instead of slot number #608

Merged
merged 3 commits into from
May 17, 2022
Merged

Recognise a PKCS11 hardware token with its serial number instead of slot number #608

merged 3 commits into from
May 17, 2022

Conversation

mohamedasaker-arm
Copy link
Contributor

Signed-off-by: mohoma01 mohamed.omarasaker@arm.com

@mohamedasaker-arm mohamedasaker-arm requested a review from a team as a code owner May 13, 2022 17:07
ionut-arm
ionut-arm reviewed May 16, 2022
View reviewed changes
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.

Thanks, looks good overall.

Could you also add some config tests: you're already covering the "both serial and slot numbers given" option in the main TOML for PKCS11, and there's already a test there where no slot or serial number is given, you'll need one test for "just a slot number", one for "just a serial number", and one for "serial and slot numbers, but they mismatch".

Also, we've not decided - are we merging mine or yours first? The 2nd to be merged will have to make some additional changes to their PR - I'm happy to do that, but I'll let you decide.

config.toml Outdated Show resolved Hide resolved
src/providers/pkcs11/mod.rs Outdated Show resolved Hide resolved
src/providers/pkcs11/mod.rs Outdated Show resolved Hide resolved
src/providers/pkcs11/mod.rs Outdated Show resolved Hide resolved
@mohamedasaker-arm
Copy link
Contributor Author

Thanks for the Review @ionut-arm!
Regarding which PR should go first, I am happy either way, as I believe the conflicts will be minor.
I think the ready one should go first 😄!

@mohamedasaker-arm mohamedasaker-arm force-pushed the 481-pkcs11-recognize-token-by-serialnumber branch from ae0f507 to 72c3925 Compare May 16, 2022 15:17
@mohamedasaker-arm
Recognise a PKCS11 hardware token with its serial number instead of s…
6b1a655 
…lot number

Signed-off-by: Mohamed Omar Asaker <mohamed.omarasaker@arm.com>
@mohamedasaker-arm
Fix PR review comments
d9c4f7a 
Signed-off-by: Mohamed Omar Asaker <mohamed.omarasaker@arm.com>
@mohamedasaker-arm
Add e2e_tests to cover different configurations for serial_number and…
0e71018 
… slot_number

Signed-off-by: Mohamed Omar Asaker <mohamed.omarasaker@arm.com>
@mohamedasaker-arm mohamedasaker-arm force-pushed the 481-pkcs11-recognize-token-by-serialnumber branch from 72c3925 to 0e71018 Compare May 17, 2022 09:32
ionut-arm
ionut-arm approved these changes May 17, 2022
View reviewed changes
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.

👍🏻

@ionut-arm ionut-arm merged commit 12090f8 into parallaxsecond:main May 17, 2022
@mohamedasaker-arm mohamedasaker-arm mentioned this pull request May 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anta5010 anta5010 anta5010 approved these changes

@ionut-arm ionut-arm ionut-arm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mohamedasaker-arm @anta5010 @ionut-arm