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

Added psa_export_key & psa_generate_random to TS Provider #468

Merged
merged 1 commit into from
Jul 20, 2021

Conversation

MattDavis00
Copy link
Member

References #341

Waiting on #467 for breaking 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.

Thanks!! I left a few comments below

@ionut-arm
Copy link
Member

You can rebase on top of main now that #467 was merged 🚀

@MattDavis00 MattDavis00 marked this pull request as ready for review July 13, 2021 15:20
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.

👍🏻

@ionut-arm ionut-arm requested a review from hug-dev July 14, 2021 11:16
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.

🙆‍♂️

pub fn generate_random(&self, size: usize) -> Result<Vec<u8>, Error> {
info!("Handling GenerateRandom request");
let open_req: GenerateRandomIn = GenerateRandomIn {
size: size.try_into().unwrap(), // Can't fail usize->u64 conversion
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't it fail on 32 bits platforms? Looks like impl From<std::num::TryFromIntError> for Error is there so you can probably just use ? instead of unwrap here?

Copy link
Member

@ionut-arm ionut-arm Jul 14, 2021

Choose a reason for hiding this comment

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

Not really, if usize is 32 bits then extending to u64 can't fail. You can only get an error the other way.

The implementation is here, and that macro is implemented here. For some reason they went with TryFrom even though there isn't any possible error.......

Copy link
Member

Choose a reason for hiding this comment

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

Oh, but looking at that implementation, maybe size as u64 would be better, like in the try_from_unbounded implementation linked above.

Copy link
Member

Choose a reason for hiding this comment

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

Right that is the safe direction here. Could fail on 128 bits platforms 🤡
I would personally prefer try_into()? if that works easily but I'm ok with the as if not. Just so that when reading the code you don't have to think about it too much

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to size: size.try_into()? if that works

…ovider.

References parallaxsecond#341

Signed-off-by: Matt Davis <matt.davis@arm.com>
@ionut-arm ionut-arm merged commit c9f348e into parallaxsecond:main Jul 20, 2021
@MattDavis00 MattDavis00 deleted the feature-trusted-service branch September 2, 2021 14:44
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