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 CanDoCrypto operation #129

Merged
merged 1 commit into from
Aug 23, 2021
Merged

Conversation

Kakemone
Copy link
Contributor

Added the page for the CanDoCrypto operation and also added the operation to the list of operations and to the table of operations coverage

Signed-off-by: Sam Davis sam.davis@arm.com

@Kakemone Kakemone force-pushed the CanDoCrypto_patch branch 3 times, most recently from 744d4a3 to cf6dc20 Compare August 11, 2021 16:04
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.

Thanks! A few comments

src/parsec_client/operations/README.md Show resolved Hide resolved
@@ -0,0 +1,54 @@
# CanDoCrypto
Copy link
Member

Choose a reason for hiding this comment

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

For the name, we can discuss. In the community meeting it was mentioned "CryptoCanDo" but then I thought if there is a new PSA Crypto operation named psa_crypto_can_do it is going to be PsaCryptoCanDo so might be weird.

Copy link
Member

Choose a reason for hiding this comment

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

There should probably be some notice of this operation being experimental here as well.

@Kakemone Kakemone force-pushed the CanDoCrypto_patch branch 3 times, most recently from 40f803c to dbe9bb1 Compare August 12, 2021 07:50
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.

Hi, thanks for the new operation! Looks pretty good overall, couple of comments

@@ -0,0 +1,54 @@
# CanDoCrypto
Copy link
Member

Choose a reason for hiding this comment

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

There should probably be some notice of this operation being experimental here as well.

| Name | Type | Description |
|--------------|-----------------|-----------------------------|
| `check_type` | `CheckType` | Type of the check performed |
| `attributes` | `KeyAttributes` | Value to be checked |
Copy link
Member

Choose a reason for hiding this comment

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

Worth adding a link to the place where KeyAttributes is defined

Opcode: 32 (`0x0020`)

**(EXPERIMENTAL) The parameters for key attestation are in an experimental phase. No guarantees are
offered around the stability of the interface for any key attestation mechanism.**
Copy link
Member

Choose a reason for hiding this comment

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

Bit of copy-pasta 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.

Oops yeah I forgot to edit it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does this sound instead?
**(EXPERIMENTAL) This operation is still being implemented and so no guarantees are offered around the stability of the interface for any key attestation mechanism.**

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, probably something pointing towards "capability discovery" instead of "key attestation" would be clearer!

Added the page for the CanDoCrypto operation and also added the operation to the list of operations and to the table of operations coverage

Signed-off-by: Sam Davis <sam.davis@arm.com>
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!

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.

Thanks! The name of the operation might change in the future but that's fine since it is experimental.

@hug-dev hug-dev merged commit 99ef3fc into parallaxsecond:main Aug 23, 2021
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