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

Update TS submodule and add asym encryption #406

Closed
wants to merge 1 commit into from

Conversation

ionut-arm
Copy link
Member

This commit pulls in new changes to the upstream TS project and adds
asymmetric encryption support for the provider.

Signed-off-by: Ionut Mihalcea ionut.mihalcea@arm.com

@ionut-arm ionut-arm added enhancement New feature or request firmware framework Issues related to compatibility with Arm FF-A labels Apr 29, 2021
@ionut-arm ionut-arm requested a review from hug-dev April 29, 2021 10:09
@ionut-arm ionut-arm self-assigned this Apr 29, 2021
Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

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

Good! I guess #295 can bet put do the "Done" column after this?

build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
@ionut-arm
Copy link
Member Author

Looks like the initial issue I was hoping was fixed hasn't been fixed yet, hence the test failures on asym encryption. I'll keep this open and update it once that gets fixed and upstreamed 😬

Comment on lines +15 to +30
pub fn create_key_id(max_current_id: &AtomicU32) -> Result<u32> {
// fetch_add adds 1 to the old value and returns the old value, so add 1 to local value for new ID
let new_key_id = max_current_id.fetch_add(1, Relaxed) + 1;
if new_key_id > super::context::PSA_KEY_ID_USER_MAX {
// If storing key failed and no other keys were created in the mean time, it is safe to
// decrement the key counter.
let _ = max_current_id.store(super::context::PSA_KEY_ID_USER_MAX, Relaxed);
error!(
"PSA max key ID limit of {} reached",
super::context::PSA_KEY_ID_USER_MAX
);
return Err(ResponseStatus::PsaErrorInsufficientMemory);
}

Ok(new_key_id)
}
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 replaced the dependency on the Mbed Crypto provider here to decouple the two PSA Crypto APIs we're abstracting over, so we can change them independently in the future. There's a bit of code duplication, but I'm not that worried about one function.

This commit pulls in new changes to the upstream TS project and adds
asymmetric encryption support for the provider.

Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>
Comment on lines +98 to +99
RUN git config --global user.email "some@email.com"
RUN git config --global user.name "Parsec Team"
Copy link
Member

Choose a reason for hiding this comment

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

Was it failing before without this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they've added some step in their build process to apply git patches to some dependencies, I think, and this is needed :(

src/providers/trusted_service/context/mod.rs Show resolved Hide resolved
@@ -66,7 +66,7 @@ mbed-crypto-provider = ["psa-crypto"]
pkcs11-provider = ["cryptoki", "picky-asn1-der", "picky-asn1", "picky-asn1-x509", "psa-crypto", "rand"]
tpm-provider = ["tss-esapi", "picky-asn1-der", "picky-asn1", "picky-asn1-x509", "hex"]
cryptoauthlib-provider = ["rust-cryptoauthlib"]
trusted-service-provider = ["mbed-crypto-provider", "bindgen", "prost-build", "prost"]
trusted-service-provider = ["psa-crypto", "bindgen", "prost-build", "prost"]
Copy link
Member

Choose a reason for hiding this comment

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

Where do we actually need psa-crypto for this provider?

Copy link
Member Author

Choose a reason for hiding this comment

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

All over the place - we use the conversions implemented in psa-crypto to native types to fill in the protobuf objects. For example, the protobuf contract for KeyPolicy contains a u32 alg field to which we can just convert using the PSA crate, because it expects the same values as PSA Crypto would (just in a protobuf wrapper)

Copy link
Member

Choose a reason for hiding this comment

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

Alright, make sense!

@ionut-arm
Copy link
Member Author

I'm putting this PR aside (again) because it seems asymmetric encryption and decryption is still broken for us 😢 Have raised #467 to deal with the bigger issue of updating the TS revision we use

@ionut-arm
Copy link
Member Author

This PR is not intended to go into the 0.8.0 release as there seems to be an issue with the underlying library. Will be fixed and merged for the next release.

@ionut-arm
Copy link
Member Author

Closing this PR in favour of a rebased version, PR to pop up shortly.

@ionut-arm ionut-arm closed this Jan 28, 2022
@ionut-arm ionut-arm deleted the ts-asym-encr-v2 branch July 22, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request firmware framework Issues related to compatibility with Arm FF-A
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants