Add interruptible key generation PSA APIs #9528
Conversation
waleed-elmelegy-arm
force-pushed
the
add-iop-key-gen
branch
3 times, most recently
from
98d17bd to
3f2662c
Compare
waleed-elmelegy-arm
changed the title
waleed-elmelegy-arm
force-pushed
the
add-iop-key-gen
branch
6 times, most recently
from
784162f to
dc088e8
Compare
Signed-off-by: Waleed Elmelegy <[email protected]>
Signed-off-by: Waleed Elmelegy <[email protected]>
Signed-off-by: Waleed Elmelegy <[email protected]>
waleed-elmelegy-arm
force-pushed
the
add-iop-key-gen
branch
from
dc088e8 to
f7775a4
Compare
waleed-elmelegy-arm
added
needs-review
Signed-off-by: Waleed Elmelegy <[email protected]>
Signed-off-by: Waleed Elmelegy <[email protected]>
waleed-elmelegy-arm
requested review from
paul-elliott-arm and
gilles-peskine-arm
and removed request for
paul-elliott-arm
|
ABI CI failure is expected. |
| type = psa_get_key_type(attributes); | ||
|
|
||
| if (!PSA_KEY_TYPE_IS_ECC(type)) { | ||
| operation->error_occurred = 1; |
There was a problem hiding this comment.
Here and other places: We need to call abort internally if any error occurs, but not clean up the num_ops or error_occurred flags, to further protect against accidental operation re-use.
| * | ||
| * \retval #PSA_SUCCESS | ||
| * The operation started successfully - call \c mbedtls_psa_generate_key_complete() | ||
| * with the same context to complete the operation. |
There was a problem hiding this comment.
You are not referring to it as a context anywhere else, in crypto.h we referred to it as an operation object rather than a context.
| /* Make the struct non-empty if algs not supported. */ | ||
| unsigned MBEDTLS_PRIVATE(dummy); | ||
| #endif | ||
| } mbedtls_psa_generate_key_iop_operation_t; |
There was a problem hiding this comment.
Shouldn't this be mbedtls_psa_generate_key_iop_t? I see where you are coming from, but the double 'operation' seems clumsy, and we didn't use it for the other structures.
| } mbedtls_psa_generate_key_iop_operation_t; | ||
|
|
||
| #if defined(MBEDTLS_ECP_C) | ||
| #define MBEDTLS_PSA_KEY_AGREEMENT_INTERRUPTIBLE_OPERATION_INIT { { 0 }, 0 } |
There was a problem hiding this comment.
as above, we could just use MBEDTLS_PSA_KEY_AGREEMENT_IOP_INIT() here
| * interface specification for transparent drivers. | ||
| * | ||
| * \param[in] operation The \c mbedtls_psa_generate_key_iop_operation_t to use. | ||
| * This must be initialized first. |
There was a problem hiding this comment.
... and have had setup called on the same operation object.
| * \param[in] operation The \c mbedtls_psa_generate_key_iop_operation_t to use. | ||
| * This must be initialized first. | ||
| * \return Total number of operations. | ||
| */ |
There was a problem hiding this comment.
Here and others: I know these are not technically doing anything interruptible, but I'm unsure we should be leaving the _iop_ out of the function names. Its an interruptible style set of operations, it feels confusing to not name it in the same way.
| * interface specification for transparent drivers. | ||
| * | ||
| * \param[in] operation The \c mbedtls_psa_generate_key_iop_operation_t to abort. | ||
| * This must be initialized first. |
There was a problem hiding this comment.
Aborting an uninitialised operation object should be fine, it doesn't need to be initialised first.
| @@ -0,0 +1,4 @@ | |||
| Features | |||
| * Add new psa key agreements APIs to generate private key | |||
There was a problem hiding this comment.
agreement, not agreements I think.
Also to generate a private key
|
Based on my experience with #9490, this is too large. Please split it into manageable pull requests. As a rule of thumb, we should never have size-m pull requests. A size-m issue should be done in several increments. |
I agree this is a relatively large PR, but as with all of these interfaces, it is well nigh impossible to split things down any finer, whilst still having any tests at all. The individual functions do not stand on their own, and without testing, we should not be merging the PR. This is why we split into functionality and sanity testing / negative and further testing, I'm really not sure how we would go any finer than this whilst having a meaningful PR. |
Description
fixes #9106
Add interruptible key generation PSA APIs.
PR checklist
Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.
Notes for the submitter
Please refer to the contributing guidelines, especially the
checklist for PR contributors.
Help make review efficient: