New function psa_hash_clone #18
Conversation
Clone a hash operation. Test good cases as part as multipart tests. Add new test functions for the state machine.
gilles-peskine-arm
added
enhancement
include/psa/crypto.h
Outdated
| * \retval #PSA_ERROR_BAD_STATE | ||
| * \p source_operation is not an active hash operation. | ||
| * \retval #PSA_ERROR_BAD_STATE | ||
| * \p source_operation is active. |
There was a problem hiding this comment.
These two return value documentations seem to contradict each other.
There was a problem hiding this comment.
Do you mean \p target_operation is active in the second \retval?
There was a problem hiding this comment.
Yes. I fixed the copypasta.
| @@ -921,6 +921,24 @@ psa_status_t psa_hash_verify(psa_hash_operation_t *operation, | |||
| */ | |||
| psa_status_t psa_hash_abort(psa_hash_operation_t *operation); | |||
|
|
|||
| /** Clone a hash operation. | |||
| * | |||
| * \param[in] source_operation The active hash operation to clone. | |||
There was a problem hiding this comment.
It should be documented that this should be initialized and active.
There was a problem hiding this comment.
Isn't the current wording sufficent? Active implies initialized.
There was a problem hiding this comment.
That was an oversight on my side, because I was expecting the wording to be "The XXX. Must be YYY." instead of "The YYY XXX". I'd prefer the former, but that's not a blocker.
library/psa_crypto.c
Outdated
| @@ -1421,6 +1421,67 @@ psa_status_t psa_hash_verify( psa_hash_operation_t *operation, | |||
| return( PSA_SUCCESS ); | |||
| } | |||
|
|
|||
| psa_status_t psa_hash_clone(const psa_hash_operation_t *source_operation, | |||
There was a problem hiding this comment.
Usually, the context acted upon is listed as the first argument; I'd therefore prefer the destination context to come first.
There was a problem hiding this comment.
There's no such convention in this API. There's a convention that input comes before output, and a convention that the context comes before other parameters. With an input context and an output context, the input context comes first.
| PSA_ASSERT( psa_hash_clone( &op_source, &op_init ) ); | ||
| PSA_ASSERT( psa_hash_finish( &op_init, | ||
| hash, sizeof( hash ), &hash_len ) ); | ||
| PSA_ASSERT( psa_hash_clone( &op_source, &op_finished ) ); |
There was a problem hiding this comment.
Looking at this, I noticed that the documentation doesn't indicate the abstract state of a hash context after it has been terminated (through psa_hash_finish, psa_hash_abort, or psa_hash_verify). From my understanding, it's initialized but not setup/active. If so, you might consider documenting it, e.g. here.
There was a problem hiding this comment.
This isn't critical to understanding the current behavior, and other types of operation should use consistent terminology, so I'll do that in a later writing pass. Tracked internally at
https://github.com/ARMmbed/psa-crypto/issues/132
| @@ -921,6 +921,24 @@ psa_status_t psa_hash_verify(psa_hash_operation_t *operation, | |||
| */ | |||
| psa_status_t psa_hash_abort(psa_hash_operation_t *operation); | |||
|
|
|||
| /** Clone a hash operation. | |||
There was a problem hiding this comment.
Everything else wouldn't make sense, but we might still mention here that the target context is active and in the same internal state as the source after a successful call to this function.
There was a problem hiding this comment.
I added a paragraph of English text that defines the behavior of the function.
library/psa_crypto.c
Outdated
| @@ -1421,6 +1421,67 @@ psa_status_t psa_hash_verify( psa_hash_operation_t *operation, | |||
| return( PSA_SUCCESS ); | |||
| } | |||
|
|
|||
| psa_status_t psa_hash_clone(const psa_hash_operation_t *source_operation, | |||
| psa_hash_operation_t *target_operation) | |||
There was a problem hiding this comment.
Style: spaces before and after the parentheses.
|
Found a minor style issue, but other than that it looks good to me! |
yanesca
removed
the
needs: review
|
CI was flaky and failed |
Includes PRs ARMmbed#6, ARMmbed#18, ARMmbed#19.
Clone a hash operation.
Test good cases as part as multipart tests. Add new test functions for
the state machine.