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

Adding 2 new mechanisms: generic key generation and key derivation via encryption #178

Merged
merged 5 commits into from
Oct 18, 2023
Merged

Adding 2 new mechanisms: generic key generation and key derivation via encryption #178

merged 5 commits into from
Oct 18, 2023

Conversation

Nk185
Copy link
Contributor

@Nk185 Nk185 commented Oct 10, 2023

Added CKM_AES_CBC_ENCRYPT_DATA;
Added CKM_GENERIC_SECRET_KEY_GEN;
Added prettier MechanismInfo fmt.

@wiktor-k
Copy link
Collaborator

🤔 It looks rather good 👍

Do you know if SoftHSM supports these mechanisms? If so I think it'd be good to add tests (yeah, I know this is a chore but it's good as a source of examples and somehow mitigates regressions when others are "improving" your code).

Thanks for your time! 👋

@Nk185
Copy link
Contributor Author

Nk185 commented Oct 12, 2023

@wiktor-k
The two mechanisms are surely supported in v2.6.1:

  • The generic secret: here
  • The derivation via encryption: here

Sure, I will add tests over weekends, maybe earlier... How would you like those tests for the EKDF? Should it be a simple derive that makes sure a new symmetric key is created? Or should I also check that an expected key was generated?

@wiktor-k
Copy link
Collaborator

Should it be a simple derive that makes sure a new symmetric key is created? Or should I also check that an expected key was generated?

Either is fine for me and I guess we have both variants in existing tests 😅

Thanks for taking care of it!

@Nk185
Copy link
Contributor Author

Nk185 commented Oct 12, 2023

@wiktor-k
Well, apparently, I found some time earlier than I thought 😅
Tests added :)

wiktor-k
wiktor-k previously approved these changes Oct 13, 2023
View reviewed changes
Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

Okay Nikita, I'm super happy with the PR, it looks high-quality 👍

I've added a couple of small "improvements" but I'll approve it already.

Thanks for your time and contributing to cryptoki crate! 🙇

cryptoki/src/mechanism/mechanism_info.rs Outdated Show resolved Hide resolved
cryptoki/src/mechanism/mechanism_info.rs Outdated Show resolved Hide resolved
@Nk185 Nk185 requested a review from wiktor-k October 16, 2023 11:46
wiktor-k
wiktor-k previously approved these changes Oct 16, 2023
View reviewed changes
Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

LGTM 👍 thanks!

ionut-arm
ionut-arm previously approved these changes Oct 16, 2023
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 to me! :)

cryptoki/src/mechanism/ekdf.rs Outdated Show resolved Hide resolved
@Nk185 Nk185 dismissed stale reviews from ionut-arm and wiktor-k via 8d04e55 October 17, 2023 20:35
@Nk185 Nk185 requested review from wiktor-k and ionut-arm October 17, 2023 20:35
Nk185 and others added 5 commits October 17, 2023 23:51
@Nk185
Added CKM_AES_CBC_ENCRYPT_DATA;
79f8b70 
Added CKM_GENERIC_SECRET_KEY_GEN;
Added prettier `MechanismInfo` fmt.

Signed-off-by: nikita.kalinichenko <[email protected]>
@Nk185
fix formatting and docs
99c515f 
Signed-off-by: nikita.kalinichenko <[email protected]>
@Nk185
added tests for CKM_AES_CBC_ENCRYPT_DATA and `CKM_GENERIC_SECRET_KE…
5a9d31c 
…Y_GEN`

Signed-off-by: nikita.kalinichenko <[email protected]>
@Nk185
simplify MechanismInfo fmt
437ccbd 
Signed-off-by: nikita.kalinichenko <[email protected]>
@Nk185 @ionut-arm
Update cryptoki/src/mechanism/ekdf.rs
f21c575 
Accepting the suggested change in copyright notice;

Co-authored-by: Ionuț Mihalcea <[email protected]>
Signed-off-by: nikita.kalinichenko <[email protected]>
@Nk185 Nk185 force-pushed the feature/generic_key_gen-and-aes_cbc_encrypt_data branch from 8d04e55 to f21c575 Compare October 17, 2023 20:53
@ionut-arm ionut-arm merged commit d7ea453 into parallaxsecond:main Oct 18, 2023
6 checks passed
@wiktor-k wiktor-k mentioned this pull request Feb 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@wiktor-k wiktor-k wiktor-k approved these changes

@hug-dev hug-dev Awaiting requested review from hug-dev

@gowthamsk-arm gowthamsk-arm Awaiting requested review from gowthamsk-arm

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
@Nk185 @wiktor-k @ionut-arm