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

Work on psa_import_key_ext() #7910

Draft
wants to merge 8 commits into
base: development
Choose a base branch
from

Conversation

ronald-cron-arm
Copy link
Contributor

No description provided.

Same test data and test cases as in test_suite_pkparse.
All tests skipped to start with.

Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
@ronald-cron-arm ronald-cron-arm marked this pull request as draft July 11, 2023 09:06
@ronald-cron-arm ronald-cron-arm added enhancement DO-NOT-MERGE component-psa PSA keystore/dispatch layer (storage, drivers, …) labels Jul 11, 2023
@ronald-cron-arm
Copy link
Contributor Author

@athoelke sorry but I replied too quickly during the meeting. The functions psa_import_key_ext() as it stands does not support the raw formats of psa_import_key(). But I don't see any major issue to add the support, it is mostly about associating them to a key data format.

Copy link
Contributor

@athoelke athoelke left a comment

Choose a reason for hiding this comment

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

Some feedback on the public API definition

@@ -2069,6 +2069,264 @@ static inline struct psa_pake_operation_s psa_pake_operation_init(void)
return v;
}

typedef enum {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have avoided using enumerations in the public Crypto API, and prefer to use a set of macro definitions. This makes implementation-specific extensions easier, and avoids some complications with the data size of an enumeration.

Please make psa_key_data_format_t a integer typedef, and the values a set of macros.

@@ -2069,6 +2069,264 @@ static inline struct psa_pake_operation_s psa_pake_operation_init(void)
return v;
}

typedef enum {
/** An invalid key data format identifier. */
PSA_KEY_DATA_FORMAT_NONE,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it particularly helpful to include 'DATA' in the type and value identifiers? - I think that PSA_KEY_FORMAT_XXX and psa_key_format_t would be unambiguous enough.

* modulus INTEGER, -- n
* publicExponent INTEGER -- e }
*
* Key attributes when importing:
Copy link
Contributor

Choose a reason for hiding this comment

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

It would help if it was clearer that this is about the way the supplied key attributes are used. Perhaps "When importing a key in this format, the supplied key attributes are used as follows:"

/** An invalid key data format identifier. */
PSA_KEY_DATA_FORMAT_NONE,
/**
* DER or PEM encoded RSAPublicKey data structure as defined in RFC 8017
Copy link
Contributor

Choose a reason for hiding this comment

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

The description of this identifier should mention that it is used to specify the key format in a call to psa_import_key_ex().

* DER or PEM encoded RSAPublicKey data structure as defined in RFC 8017
* (PKCS#1) with:
*
* RSAPublicKey ::= SEQUENCE {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this paragraph be rendered as preformatted text (code)?

*/
PSA_KEY_DATA_FORMAT_ONE_ASYMMETRIC_KEY,

PSA_KEY_DATA_FORMAT_COUNT
Copy link
Contributor

Choose a reason for hiding this comment

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

Not required: the public Crypto API does not define 'count' or terminator values for value sets like this.

* for a persistent key.
*
* \param[in] format The format of the key data in the \p data buffer.
* \param[out] key On success, an identifier to the newly created key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this parameter to the end of the definition list, to match the function declaration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-psa PSA keystore/dispatch layer (storage, drivers, …) DO-NOT-MERGE enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants