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

xtest: add pkcs11_1031 for CKM_RSA_X_509 sign/verify (take 2) #762

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

etienne-lms
Copy link
Contributor

Basic test on signing/verifying with raw RSA mechanism CKM_RSA_X_509.

Replaces #757.

Basic test on signing/verifying with raw RSA mechanism CKM_RSA_X_509.

Signed-off-by: Etienne Carriere <[email protected]>
Skip the test if CFG_PKCS11_TA_RSA_X_509 is disabled.

Signed-off-by: Etienne Carriere <[email protected]>
Fix missing sub-case end call in test_rsa_raw_operations().

Signed-off-by: Etienne Carriere <[email protected]>
Use a well known padding scheme (PKCS#1 v1.5) to ensure the
generated signature has to expected size. With the previously
implemented padding scheme I found seldom occurrences where
the generated signature was 1 byte too short.

Signed-off-by: Etienne Carriere <[email protected]>
@etienne-lms
Copy link
Contributor Author

Updated with a more robust message padding scheme.

host/xtest/pkcs11_1000.c Outdated Show resolved Hide resolved
* key. If smaller, it is strongly recommended to inserrt padding
* bytes to reach to key size. Lets's use random data and use PKCS v1.5
* padding scheme to ensure input data to be signed will generate well
* szied signature.
Copy link
Contributor

Choose a reason for hiding this comment

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

sized

Fix inline comment typos.

Signed-off-by: Etienne Carriere <[email protected]>
@etienne-lms
Copy link
Contributor Author

Sorry for these typos.
Unfortunately i've just found another occurrence of badly sized generated signature. The padding scheme used here is to weak for regression tests. I'll update it.

Discard the padding used here. Whatever the message data, as long
as the message has the size of the private key, the PKCS#11
should be able to generate a signature.

Signed-off-by: Etienne Carriere <[email protected]>
@etienne-lms
Copy link
Contributor Author

Updated removing the padding scheme. Strong padding is not needed as far as PKCS#11 regression tests are concerned.
The issue I faced with the generated signature size are due to in issue in the PKCS#11 TA implementation, fixed with OP-TEE/optee_os#7179.

Remove unused local variable.

Signed-off-by: Etienne Carriere <[email protected]>
@etienne-lms
Copy link
Contributor Author

Fixed an omission in the previous fixup commit. CI test should be good now.

I missed a constraint on the input message: it must represent a value
smaller than the private key modulus. Clear the message leading bit!

Fix inline comment.

Remove the part testing the key attribute. RSA keys generation in the
PKCS#11 TA is already tested by pkcs11_1021, pkcs1022 and pkcs11_1023.
No need to test again RSA keys attributes.

Wrap line wider than 80char for consistency of this source file.

Signed-off-by: Etienne Carriere <[email protected]>
host/xtest/pkcs11_1000.c Outdated Show resolved Hide resolved
@etienne-lms
Copy link
Contributor Author

Fixed message leading bit + remove some useless tests on key attributes.

Replace static test on CFG_PKCS11_TA_RSA_X_509 with a runtime test
on whether or not the PKCS#11 TA supports CKM_RSA_X_509 for signature
computation and verification. This change makes xtest more flexible
regarding the tested embedded TA instead of requiring a specific
xtest build for a given PKCS#11 TA configuration.

Signed-off-by: Etienne Carriere <[email protected]>
Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jan 16, 2025
Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

LGTM, just two misspelled words, see below.

Acked-by: Jerome Forissier <[email protected]>

Do_ADBG_BeginSubCase(c, "Sign/Verify with valid data/signature");

/*
* Test C_Sign() with buffer too short, the C_Sign() operaiton should remain
Copy link
Contributor

Choose a reason for hiding this comment

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

operation


Do_ADBG_BeginSubCase(c, "Verify with altered signature/message");

/* Test signature wider than expect */
Copy link
Contributor

Choose a reason for hiding this comment

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

expected

@etienne-lms
Copy link
Contributor Author

Thanks for the review. However, this P-R cannot be merged before OP-TEE/optee_os#7179. Without this OP-TEE core fix, we way have failures with this added pkcs11_1031 test case because it can happen the generated signature it not well sized.

@github-actions github-actions bot removed the Stale label Jan 17, 2025
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.

2 participants