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

Add support for psa_generate_random operation for MbedCrypto provider #208

Merged
merged 1 commit into from
Sep 7, 2020

Conversation

joechrisellis
Copy link
Contributor

This patch adds support for the psa_generate_random operation for the MbedCrypto provider. Once again, I am expecting this to fail CI because it's dependent on the other patches. 😄

@joechrisellis joechrisellis force-pushed the psa_generate_random branch 2 times, most recently from 8b782d3 to 6499343 Compare August 4, 2020 09:41
@hug-dev
Copy link
Member

hug-dev commented Aug 4, 2020

Thanks for adding this! It will need new versions on psa-crypto and parsec-interface. Since more operations are going to be implemented there in the next few days, let's wait to version those repos to not have too many versioning to do :)

@joechrisellis
Copy link
Contributor Author

Thanks for adding this! It will need new versions on psa-crypto and parsec-interface. Since more operations are going to be implemented there in the next few days, let's wait to version those repos to not have too many versioning to do :)

Sounds good to me! 💯

@joechrisellis joechrisellis force-pushed the psa_generate_random branch 4 times, most recently from 2922af1 to d49f50c Compare August 25, 2020 16:52
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.

All look good to me except for the peer credential authenticator 😄

return;
}

// Less than one in ~35 billion chance of collision. Should be good enough for our testing.
Copy link
Member

Choose a reason for hiding this comment

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

This is PR bingo, who will be the winner PR making that test fail 😮 ?

@@ -0,0 +1,45 @@
// Copyright 2019 Contributors to the Parsec project.
Copy link
Member

Choose a reason for hiding this comment

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

Should we commit that file in this PR? Are you trying to sneakily make it in 👀 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, oops. Removed. 😄

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.

🥂

@hug-dev hug-dev requested a review from ionut-arm August 26, 2020 11:41
@hug-dev hug-dev added the enhancement New feature or request label Aug 26, 2020
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! 💯

/// Execute a GenerateRandom operation.
fn psa_generate_random(
&self,
_app_name: ApplicationName,
Copy link
Member

Choose a reason for hiding this comment

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

I'd argue against requesting an app_name. The operation can then be performed even for non-authenticated users, I think that should be alright!

}),
Err(error) => {
let error = ResponseStatus::from(error);
format_error!("Generate random status: ", error);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
format_error!("Generate random status: ", error);
format_error!("Generate random status", error);

The format_error! macro adds the : for you, but maybe I should change that since everyone seems tempted to use it as the normal error!

Comment on lines 8 to 10
pub(super) fn psa_generate_random_internal(
&self,
_app_name: ApplicationName,
op: psa_generate_random::Operation,
) -> Result<psa_generate_random::Result> {
Copy link
Member

Choose a reason for hiding this comment

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

Now that I look at it, you could just put this one straight in the psa_generate_random method, there isn't that much code to warrant an _internal one, but feel free to keep it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll leave this -- it looks like most of the operations have an *_internal method. If you have strong opinions here feel free to reply and I'll change. 😄

@joechrisellis joechrisellis force-pushed the psa_generate_random branch 3 times, most recently from ca95108 to 9a25f8f Compare August 26, 2020 20:27
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!

@joechrisellis joechrisellis force-pushed the psa_generate_random branch 2 times, most recently from 1ff71d0 to ab9c964 Compare September 7, 2020 09:28
Signed-off-by: Joe Ellis <joe.ellis@arm.com>
@hug-dev hug-dev merged commit 40bf768 into parallaxsecond:master Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants