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

Upgrade and clean dependencies #246

Merged
merged 1 commit into from
Sep 16, 2020
Merged

Upgrade and clean dependencies #246

merged 1 commit into from
Sep 16, 2020

Conversation

hug-dev
Copy link
Member

@hug-dev hug-dev commented Sep 10, 2020

Also adds a nightly workflow

Signed-off-by: Hugues de Valon hugues.devalon@arm.com

@hug-dev hug-dev self-assigned this Sep 10, 2020
@hug-dev hug-dev added the enhancement New feature or request label Sep 10, 2020
@hug-dev hug-dev linked an issue Sep 10, 2020 that may be closed by this pull request
Verify which dependencies can/should be updated #158
Closed
@hug-dev hug-dev force-pushed the upgrade branch 3 times, most recently from 9a6564a to 21271c7 Compare September 14, 2020 10:25
@hug-dev
Copy link
Member Author

hug-dev commented Sep 14, 2020

I have:

2020-09-14T10:35:00.9127176Z [INFO  parsec_service::providers::pkcs11_provider::key_management] Generating RSA key pair in session 2
2020-09-14T10:35:00.9128115Z [ERROR parsec_service::providers::pkcs11_provider::key_management] Generate Key Pair operation failed; Error: PKCS#11: CKR_GENERAL_ERROR (0x5)
2020-09-14T10:35:00.9129244Z [ERROR parsec_service::providers::pkcs11_provider::utils] Error converted to PsaErrorCommunicationFailure; Error: 5

everytime I generate a key. I believe this is something wrong I do with casting but interesting in the sense that now all the tests are single-threaded and this error seems similar to the one we had before...
Will try to see if I can reproduce locally
cc @ionut-arm

@hug-dev hug-dev mentioned this pull request Sep 14, 2020
Closed
@hug-dev
Upgrade and clean dependencies
8070213 
Also adds a nightly workflow

Signed-off-by: Hugues de Valon <hugues.devalon@arm.com>
hug-dev
hug-dev commented Sep 15, 2020
View reviewed changes
e2e_tests/provider_cfg/mbed-crypto/Dockerfile
Comment on lines -11 to -14
RUN wget https://github.com/ARMmbed/mbed-crypto/archive/mbedcrypto-2.0.0.tar.gz
RUN tar xf mbedcrypto-2.0.0.tar.gz
RUN cd mbed-crypto-mbedcrypto-2.0.0 \
&& make
Copy link
Member Author

Choose a reason for hiding this comment

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

This is no longer needed because psa-crypto-sys does it for us 🤓

hug-dev
hug-dev commented Sep 15, 2020
View reviewed changes
src/providers/pkcs11_provider/utils.rs
@@ -558,7 +563,7 @@ pub fn parsec_to_pkcs11_params(
pub_template.push(CK_ATTRIBUTE::new(CKA_TOKEN).with_bool(&CK_TRUE));
pub_template.push(CK_ATTRIBUTE::new(CKA_PRIVATE).with_bool(&CK_FALSE));
pub_template.push(CK_ATTRIBUTE::new(CKA_PUBLIC_EXPONENT).with_bytes(&PUBLIC_EXPONENT));
pub_template.push(CK_ATTRIBUTE::new(CKA_MODULUS_BITS).with_ck_ulong(&attributes.bits));
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how this was working before 🔫 attributes is moved into this function but then freed! This reference would no longer be valid then...
I tried, after applying this change, to remove the mutex in the PKCS11 provider but the bug is still there.

Copy link
Member

Choose a reason for hiding this comment

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

Probably because it's on the stack, the value is loaded somewhere and then deallocated but not cleared, so when the call is done it's still "valid". Just that sometimes something else gets written on top

@hug-dev hug-dev requested a review from ionut-arm September 15, 2020 15:27
ionut-arm
ionut-arm approved these changes Sep 16, 2020
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.

Looks good to me. Hopefully this removes the last memory issues in the PKCS11 provider ... 😔

src/providers/pkcs11_provider/utils.rs
@@ -558,7 +563,7 @@ pub fn parsec_to_pkcs11_params(
pub_template.push(CK_ATTRIBUTE::new(CKA_TOKEN).with_bool(&CK_TRUE));
pub_template.push(CK_ATTRIBUTE::new(CKA_PRIVATE).with_bool(&CK_FALSE));
pub_template.push(CK_ATTRIBUTE::new(CKA_PUBLIC_EXPONENT).with_bytes(&PUBLIC_EXPONENT));
pub_template.push(CK_ATTRIBUTE::new(CKA_MODULUS_BITS).with_ck_ulong(&attributes.bits));
Copy link
Member

Choose a reason for hiding this comment

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

Probably because it's on the stack, the value is loaded somewhere and then deallocated but not cleared, so when the call is done it's still "valid". Just that sometimes something else gets written on top

e2e_tests/tests/per_provider/normal_tests/import_key.rs
@@ -73,8 +73,8 @@ fn check_format_import1() -> Result<()> {
let key_name = String::from("check_format_import");

let public_key = RSAPublicKey {
modulus: IntegerAsn1::from_unsigned_bytes_be(example_modulus_1024()),
public_exponent: IntegerAsn1::from_unsigned_bytes_be(vec![0x01, 0x00, 0x01]),
modulus: IntegerAsn1::from_bytes_be_unsigned(example_modulus_1024()),
Copy link
Member

Choose a reason for hiding this comment

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

👌

@hug-dev hug-dev merged commit b5e4760 into parallaxsecond:master Sep 16, 2020
@hug-dev hug-dev deleted the upgrade branch September 16, 2020 14:57
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

Assignees

@hug-dev hug-dev

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Verify which dependencies can/should be updated
2 participants
@hug-dev @ionut-arm