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

Refactor of key_derivation #52

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

sbailey-arm
Copy link
Contributor

Currently, key derivation does not properly support key agreement. This PR is a possible solution.

Signed-off-by: Samuel Bailey samuel.bailey@arm.com

@sbailey-arm
Copy link
Contributor Author

Example isn't complete so this won't pass CI yet.

@sbailey-arm sbailey-arm force-pushed the add-key-derivation branch 3 times, most recently from 04116d1 to 400b1fa Compare August 10, 2020 11:06
@@ -32,7 +32,7 @@ pub enum Inputs<'a> {
/// Secret, used in the "extract" step. This is typically a key of type `Derive` , or the shared secret
/// resulting from a key agreement, using `Input::KeyAgreement`.
/// Must be a key or key agreement input if used with `psa_key_derivation_output_key`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it worth duplicating these structs to enforce this? So we'd have an operation struct for output_key and one for output_bytes where the output_key struct only allows a key or key agreement for the secret step?

Copy link
Member

Choose a reason for hiding this comment

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

No, I think that's overkill, as I assume most of the code would have to be duplicated too, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes most of it would have to be duplicated.

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 say writing a few lines to check the validity is better than duplicating tens/hundreds to make ensure that statically. If we have a regression test for it that should be enough. Do you think the static checking would bring a big benefit?

Copy link
Contributor Author

@sbailey-arm sbailey-arm Aug 10, 2020

Choose a reason for hiding this comment

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

Yes, you're probably right. If someone wants to use this on an embedded device, they may end up appreciating the code size savings over enforcing correct parameters.

Do you think the static checking would bring a big benefit?

Not big, no. It's easy enough to just read the documentation! But if there's a cheap enough way of forcing something to be correct, I'd take it (haven't though of a way to enforce this ).

@sbailey-arm
Copy link
Contributor Author

sbailey-arm commented Aug 10, 2020

mbedTLS doesn't support key output if key agreement is used as the input for the secret step. The commented out examples shows how it could be used, once this has been added (here is the issue).

You will receive PsaErrorNotPermitted if you try and use key agreement with output_key.

@sbailey-arm
Copy link
Contributor Author

Added two tests. One that uses a key for input of secret which runs and passes and a second that uses key agreement as input for secret which is ignored as it will not currently pass.

@sbailey-arm sbailey-arm marked this pull request as ready for review August 10, 2020 16:11
@ionut-arm
Copy link
Member

You seem to have added some commits with the original derivation/agreement work

@sbailey-arm
Copy link
Contributor Author

You seem to have added some commits with the original derivation/agreement work

I've rebased this onto the branch that I added all of the tests to. Plan was to have the other PR merged to main, then this should show the changes to key derivation.

Signed-off-by: Samuel Bailey <samuel.bailey@arm.com>
@ionut-arm ionut-arm merged commit 388af0d into parallaxsecond:master Aug 13, 2020
@ionut-arm
Copy link
Member

Thanks! Apologies for taking so long to get this in

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