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

Fix CKA_ALLOWED_MECHANISMS attribute generation #397

Merged
merged 7 commits into from
May 30, 2024
Merged

Fix CKA_ALLOWED_MECHANISMS attribute generation #397

merged 7 commits into from
May 30, 2024

Conversation

simo5
Copy link
Member

@simo5 simo5 commented May 23, 2024 •
edited
Loading

Description

The spec says the length is the size of the data
The current code sets the number of elements as length, but that seems incorrect.

Checklist

  • Code modified for feature
  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • Documentation updated

Reviewer's checklist:

  • Any issues marked for closing are addressed
  • There is a test suite reasonably covering new functionality or modifications
  • This feature/change has adequate documentation added
  • Code conform to coding style that today cannot yet be enforced via the check style test
  • Commits have short titles and sensible commit messages
  • Coverity Scan has run if needed (code PR) and no new defects were found

@simo5 simo5 changed the title FIx CKA_ALLOWED_MECHANISM atteibute generation FIx CKA_ALLOWED_MECHANISM attribute generation May 23, 2024
@simo5 simo5 changed the title FIx CKA_ALLOWED_MECHANISM attribute generation FIx CKA_ALLOWED_MECHANISMS attribute generation May 23, 2024
@simo5 simo5 requested a review from Jakuje May 23, 2024 19:30
Jakuje
Jakuje previously approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@Jakuje Jakuje 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 some tests with this attribute? I know softhsm supports this but I do not recall if we used this in the tests somewhere. I am not aware of any other implementation supporting this attribute for now.

@simo5
Copy link
Member Author

simo5 commented May 23, 2024

@Jakuje
I do not have a test and this bothers me, do you know if we can enable this on softhsm easily?
I think we should have a test to ensure we are doing the right thing instead of blindly following the spec. The spec sometimes is wrong or confusing, or under-specified, although this specific case is probably straightforward enough.

@Jakuje
Copy link
Contributor

Jakuje commented May 24, 2024

I do not have a test and this bothers me, do you know if we can enable this on softhsm easily?

I was writing a test for opensc for this when I introduced support for this into pkcs11-tool:

https://github.com/OpenSC/OpenSC/blob/master/tests/test-pkcs11-tool-allowed-mechanisms.sh

Technically, the only needed thing is to present the CKA_ALLOWED_MECHANISMS when generating/unwrapping/writing key as part of the template:

https://github.com/OpenSC/OpenSC/blob/master/src/tools/pkcs11-tool.c#L3183

I think the same think should be possible to make working with kryoptic too.

@simo5 simo5 changed the title FIx CKA_ALLOWED_MECHANISMS attribute generation Fix CKA_ALLOWED_MECHANISMS attribute generation May 28, 2024
@simo5 simo5 mentioned this pull request May 28, 2024
Closed
@simo5 simo5 requested a review from Jakuje May 28, 2024 22:48
@simo5
Copy link
Member Author

simo5 commented May 28, 2024

Soo it looks like Ubuntu has a version of softhsm that is too old ?

@simo5
Copy link
Member Author

simo5 commented May 28, 2024

Soo it looks like Ubuntu has a version of softhsm that is too old ?

Ugh no that is not the problem, somehow Ubuntu is getting confused by the request to use CTR-DRBG via provider ...

genpkey: Error generating RSA-PSS key
40173885727F0000:error:13000084:engine routines:dynamic_load:dso not found:../crypto/engine/eng_dyn.c:442:
40173885727F0000:error:13000074:engine routines:ENGINE_by_id:no such engine:../crypto/engine/eng_list.c:433:id=rdrand
40173885727F0000:error:0308010C:digital envelope routines:inner_evp_generic_fetch:unsupported:../crypto/evp/evp_fetch.c:386:Global default library context, Algorithm (CTR-DRBG : 122), Properties (provider=pkcs11)

@beldmit have you seen this before perchance?

@beldmit
Copy link
Collaborator

beldmit commented May 29, 2024

Not for rdrand...

Jakuje
Jakuje reviewed May 29, 2024
View reviewed changes
src/keymgmt.c Outdated Show resolved Hide resolved
src/keymgmt.c Show resolved Hide resolved
@simo5 simo5 force-pushed the issue_396 branch 3 times, most recently from fb23c2e to 97d5cca Compare May 29, 2024 14:02
@simo5 simo5 requested a review from Jakuje May 29, 2024 14:02
@simo5
Copy link
Member Author

simo5 commented May 29, 2024

@Jakuje I restored setting the default PSS mechanisms and added more fine-tuned error checking. PTAL.

@simo5 simo5 added the covscan Triggers Coverity Scanner label May 29, 2024
@github-actions github-actions bot removed the covscan Triggers Coverity Scanner label May 29, 2024
simo5 added 6 commits May 29, 2024 14:25
@simo5
The spec says the length is the size of the data
2c4f4f5 
The current code sets the number of elements as length, but that seems
incorrect.

Fixes latchset#396

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Allow setting DIGEST restrictions for RSA PSS
844656b 
This is translated to setting the CKA_ALLOWED_MECHANISMS param.
No other parameter restritctions is currently supported by
PKCS#11 specs, and there is no guarantee that the restriction
is supported nor respected by pkcs11 modules.

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Enable Encoders/Decoders for RSA-PSS too
40ad7c0 
Seem like we forgot to explicitly enable them in the past. OpenSSL
considers RSA-PSS a separate key type and requires explicit encoders,
will not fallback to RSA encoders/decoders from a provider and instead
will try to export private keys to the default provider to use the base
encoders/decoders.

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Add test for RSA-PSS restrictions
f3dce64 
SoftHSM supports applying restrictions to keys that have
CKA_ALLOWED_MECHANISMS at key generation.

Softoken ingests the attribute but then performs no enforcement,
so we do not enable the test for it.

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Make provider preferential, not mandatory
4116318 
Apparently Ubuntu has some configuration that tries hard to load a
rdrand engine. When the propquery is set to a hard provider=pkcs11 this
fails as the tpe of DRBG being sourced becomes incompatible with the
mandate property.
Soften the test to only prefer the provider so that the operations we
care for will come from the pkcs11 provider (we check errors anyway) and
we do not get the noise of unrelated failures.

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Allow also Raw PSS mechanisms in the default set
06db9f0 
When an application pre-hashes the content to be signed it can use the
raw CKM_RSA_PKCS_PSS mechanism to apply a signature. This may be done
with simple hardware tokens that do not support digest operations on
board and need to rely on the software to deal with that part.
We should not preclude such use for key we generate.

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Copy link
Member Author

simo5 commented May 29, 2024

Fixed a minor issue found by coverity and rebase on main.

@simo5 simo5 added the no-covscan-obsolete-label Do not use this label label May 29, 2024
Jakuje
Jakuje reviewed May 30, 2024
View reviewed changes
src/keymgmt.c Show resolved Hide resolved
tests/trsapssam Show resolved Hide resolved
@simo5
Add negative test for PSS key used for PKCS1 sig
32d059f 
Signed-off-by: Simo Sorce <simo@redhat.com>
Jakuje
Jakuje approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

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

lgtm!

@simo5 simo5 added no-covscan-obsolete-label Do not use this label and removed no-covscan-obsolete-label Do not use this label labels May 30, 2024
@simo5
Copy link
Member Author

simo5 commented May 30, 2024

Uhmm I need to fix the CI scripts to recognize that the no-covscan label is already present when a PR gets updated ... and as usual Bind test fails, but that test is not binding (pun fully intended :-) ).

Thanks for the review @Jakuje

@simo5 simo5 merged commit 8a38819 into latchset:main May 30, 2024
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jakuje Jakuje Jakuje approved these changes

Assignees
No one assigned
Labels
no-covscan-obsolete-label Do not use this label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@simo5 @Jakuje @beldmit