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

Add IBM specific mechanisms #669

Merged
merged 5 commits into from
Feb 12, 2025
Merged

Add IBM specific mechanisms #669

merged 5 commits into from
Feb 12, 2025

Conversation

fcallies
Copy link
Contributor

This series adds support for three IBM specific mechanisms and four IBM specific attributes.
Additionally for the rpc_C_DeriveKey function support for size queries for potential output fields of the mechanisms parameters is implemented.

@fcallies
Copy link
Contributor Author

fcallies commented Feb 4, 2025

@ueno can one of the maintainers please approve the check workflows?

ueno
ueno reviewed Feb 4, 2025
View reviewed changes
Copy link
Member

@ueno ueno left a comment

Choose a reason for hiding this comment

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

Do you have any documentation available for those mechanisms?

ZoltanFridrich
ZoltanFridrich requested changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@ZoltanFridrich ZoltanFridrich left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks nice overall except the compatibility break reported by Daiki. That needs to be addressed.
Also please address the issues reported by CI (ignore Fuzzing test).

@fcallies
Copy link
Contributor Author

fcallies commented Feb 5, 2025

You can find the documentation for the mechanisms here.

@fcallies
Copy link
Contributor Author

fcallies commented Feb 7, 2025

I pumped the RPC protocol version to 2 and included the requested changes by @ZoltanFridrich and I hope I fixed all the Ci bugs. It looks like they were originating in the meson build failing.

kalvdans
kalvdans reviewed Feb 7, 2025
View reviewed changes
@fcallies
Add IBM specific key attributes
47ed072 
This adds support for the following IBM specific attributes:
 - CKA_IBM_OPAQUE_REENC
 - CKA_IBM_OPAQUE_OLD
 - CKA_IBM_DILITHIUM_MODE
 - CKA_IBM_CCA_AES_KEY_MODE
@fcallies
Add support for CKM_IBM_ECDSA_OTHER
9010412 
This adds support for the CKM_IBM_ECDSA_OTHER IBM specific mechanism.
@fcallies
Rework rpc_C_DeriveKey to allow size queries
61423e9 
This adds support to the rpc_C_DeriveKey to send the mechanism parameter
back to the client after the actual call. This is necessary to allow
size queries for potential output fields in the parameter. In this case
the call itself will fail but instead of writing an error response the
client has to get the updated parameter. For this the server will send
the error code along with the parameter back to the client that then can
allocate space for the output fields in the parameter and do the call
again.
Since this will lead to a backwards compatability problem within the RPC
protocol the version is bumped.
@fcallies
Copy link
Contributor Author

The documentation requested by @kalvdans is added as a comment and code should be ready for another CI run.

@coveralls
Copy link

coveralls commented Feb 10, 2025 •
edited
Loading

Coverage Status

coverage: 69.125% (-0.4%) from 69.532%
when pulling ca5c97b on fcallies:devel
into f42d725 on p11-glue:master.

@ZoltanFridrich
Copy link
Contributor

@fcallies check the cppcheck failure. It seems to be complaining about uninitialized variables.
The autotools-no-asn1 failure should not be relevant and will probably be fixed after I rerun the test.

@fcallies
Copy link
Contributor Author

This should be a false positive as these values are set using the p11_rpc_buffer_get_ functions. As requested by @ZoltanFridrich I put them all in one if, maybe thats what confuses the test... The first of the values set in the if is not reported, but all the values after that are reported as uninitialized.

@ZoltanFridrich
Copy link
Contributor

This should be a false positive as these values are set using the p11_rpc_buffer_get_ functions. As requested by @ZoltanFridrich I put them all in one if, maybe thats what confuses the test... The first of the values set in the if is not reported, but all the values after that are reported as uninitialized.

Indeed that might be the cause because I dont think the cppcheck understands that all of the p11_rpc_buffer_get_ functions must succeed in order to continue to the assignment of the values. You can expand it to multiple ifs as previously and I can run the tests again.

@fcallies
Copy link
Contributor Author

I split it again in different ifs @ZoltanFridrich

@fcallies
Copy link
Contributor Author

Please restart the CI tests.

@ZoltanFridrich
Copy link
Contributor

Please restart the CI tests.

seems like it wasnt the issue afterall. Maybe just initialize the struct with initializer like params = { 0 };?

@fcallies
Add support for CKM_IBM_BTC_DERIVE
1c39fa1 
This adds support for the CKM_IBM_BTC_DERIVE IBM specific mechanism.
Additionally this exploits the rpc_C_DeriveKey rework.
@fcallies
Add support for CKM_IBM_KYBER
47221ee 
This adds support for the CKM_IBM_KYBER IBM specific mechanism.
Additionally this exploits the rpc_C_DeriveKey rework.
@fcallies
Copy link
Contributor Author

I initialized params and on my local machine the cppcheck succeeds. Ready for another CI run.

@fcallies
Copy link
Contributor Author

Lgtm, is there anything else to be done by me?

ZoltanFridrich
ZoltanFridrich approved these changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@ZoltanFridrich ZoltanFridrich left a comment

Choose a reason for hiding this comment

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

LGTM

@ZoltanFridrich ZoltanFridrich merged commit d7523b1 into p11-glue:master Feb 12, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ueno ueno ueno left review comments

@kalvdans kalvdans kalvdans left review comments

@ZoltanFridrich ZoltanFridrich ZoltanFridrich 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.
5 participants
@fcallies @coveralls @ZoltanFridrich @ueno @kalvdans