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

Fix for PKCS11_store_certificate generates CKR_ATTRIBUTE_TYPE_INVALID error #519

Merged
merged 9 commits into from
Oct 5, 2023
Merged

Fix for PKCS11_store_certificate generates CKR_ATTRIBUTE_TYPE_INVALID error #519

merged 9 commits into from
Oct 5, 2023

Conversation

mihaicristiantanase
Copy link
Contributor

This fixes the issue #518

@dengert
Copy link
Member

dengert commented Oct 3, 2023

C_GetInfo is called when the module is loaded:

https://github.com/OpenSC/libp11/blob/master/src/p11_load.c#L94-L107

The version number could be save once in PKCS11_CTX, rather then calling C_GetInfo each time the library version is needed.
Personally, I would save it as a CK_VERSION library_version structure. Or could be saved as major * 1000 + minor as you are doing.
There may be other places in libp11 that might need the version number too.

@mtrojnar comments?

@mtrojnar
Copy link
Member

mtrojnar commented Oct 3, 2023

I fully support @dengert's suggestions.

Also, please do not add another source file for two trivial functions. As @dengert pointed out, it's one PKCS11 API call after the module is loaded and initialized, and one field in the context structure, so no new functions seem to be needed.

@mihaicristiantanase
Copy link
Contributor Author

Thanks for the feedback.

First, saving the version at library load is a good idea. The only problem I see in reusing CK_VERSION is that I need to include "pkcs11.h" into "libp11.h", which is probably not a good idea. I'm thinking of creating a different type in "libp11.h" (ex: Version) that mirrors CK_VERSION. @dengert, what do you think?

Second, I don't see the issue with adding a new source file. Even if a source file contains trivial content, you don't really want to repeat it in all places it is used. Probably I'm missing something here...
Anyway, @mtrojnar, where do you suggest I should add version related logic?

@mtrojnar
Copy link
Member

mtrojnar commented Oct 4, 2023

Adding 9ac6919 was easier than explaining.

Avoid converting them to an integer or adding an encapsulation layer. An additional abstraction layer makes the code harder instead of simpler to understand. Simply create and appropriate condition every time it's needed.

General advice: Use the KISS principle.

@mihaicristiantanase
Copy link
Contributor Author

9ac6919 adds a new type to PKCS11_CTX. Any idea how can this be accessed from a PKCS11_SLOT_private object?

@mtrojnar
Copy link
Member

mtrojnar commented Oct 4, 2023

Good point. It shouldn't go into the public API anyway. Apply c6e69c9 and refer to slot->ctx->cryptoki_version.major and slot->ctx->cryptoki_version.minor.

@mihaicristiantanase
Copy link
Contributor Author

I've used the simpler CK_VERSION instead of struct _CK_VERSION. @mtrojnar, is there a reason you used the latter in PKCS11_SLOT_private?
Also, I've added a version comparison function in p11_cert.c instead of using a new source file. Probably, this will be moved to a separate place when the comparison is needed elsewhere.

@mtrojnar mtrojnar merged commit 50ec0af into OpenSC:master Oct 5, 2023
11 of 12 checks passed
@mtrojnar
Copy link
Member

mtrojnar commented Oct 5, 2023

Thank you very much for your work.

@mihaicristiantanase
Copy link
Contributor Author

No problem. Thank you for merging the changes!

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.

3 participants